Moodle
  1. Moodle
  2. MDL-29121 Various problems with the manual enrolment UI (js)
  3. MDL-37350

Opening and closing the 'Add role' popup multiple times causes a role to be displayed multiple times on the final when finally selecting a role

    Details

    • Testing Instructions:
      Hide
      • Open the Enrol Users page
      • Click the '+' in the roles cell to open the "Add role" popup
      • Close the dialogue
        • Repeat this a few times
      • Open the Add role popup again
      • Select a role
        • Confirm that the role is only displayed once
      • Refresh the page
        • Confirm that the new role is displayed correctly
      Show
      Open the Enrol Users page Click the '+' in the roles cell to open the "Add role" popup Close the dialogue Repeat this a few times Open the Add role popup again Select a role Confirm that the role is only displayed once Refresh the page Confirm that the new role is displayed correctly
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-37350-m24
    • Pull Master Branch:
    • Rank:
      46969

      Description

      There is an on('submit') event which doesn't get detached correctly if the 'Add role' popup is closed without selecting a role.

        Activity

        Hide
        Andrew Nicols added a comment -

        Following peer review, I'll produce patches for all affected branches.

        Show
        Andrew Nicols added a comment - Following peer review, I'll produce patches for all affected branches.
        Hide
        Rajesh Taneja added a comment -

        Thanks for fixing this Andrew,

        Patch look good to me. Feel free to push it for integration when you are ready.

        [y] Syntax
        [y] Output
        [y] Whitespace
        [-] Language
        [-] Databases
        [y] Testing
        [-] Security
        [-] Documentation
        [y] Git
        [y] Sanity check

        FYI: Adding detach event in addRoleCallback seems redundant at first, but thinking about double click makes it reasonable to be there.

        Show
        Rajesh Taneja added a comment - Thanks for fixing this Andrew, Patch look good to me. Feel free to push it for integration when you are ready. [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [-] Security [-] Documentation [y] Git [y] Sanity check FYI: Adding detach event in addRoleCallback seems redundant at first, but thinking about double click makes it reasonable to be there.
        Hide
        Andrew Nicols added a comment -

        This whole area of code could do with a refactor to be honest. We create a new ROLEUSER object (which extends Y.Base) for each enrolled user shown on the page which has the potential to be a huge memory hog.

        I have some thoughts as to how we could improve a lot of this, but it's still at the design stage (presently on paper). See MDL-37349 for more details.

        Show
        Andrew Nicols added a comment - This whole area of code could do with a refactor to be honest. We create a new ROLEUSER object (which extends Y.Base) for each enrolled user shown on the page which has the potential to be a huge memory hog. I have some thoughts as to how we could improve a lot of this, but it's still at the design stage (presently on paper). See MDL-37349 for more details.
        Hide
        Sam Hemelryk added a comment -

        Thanks Andrew, this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now.
        Hide
        Frédéric Massart added a comment -

        Test passed, thanks! Just noting that this popup does not set the focus on the close button (or action button) as other do. Also the look and feel is a bit different than our modal windows (drop shadow, padding, ...), is there an issue raised for that Andrew?

        Show
        Frédéric Massart added a comment - Test passed, thanks! Just noting that this popup does not set the focus on the close button (or action button) as other do. Also the look and feel is a bit different than our modal windows (drop shadow, padding, ...), is there an issue raised for that Andrew?
        Hide
        Eloy Lafuente (stronk7) added a comment -

        A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

        (and won't be revisiting it unless some regression is found)

        Thanks and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: