Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-27837

Unmask Checkboxes Don't Work As Expected With More Than One Enrolment Key

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 2.1.1, 2.2
    • Component/s: Enrolments
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Create a course with two or more self-enrolment options with enrolment keys. As a student who isn't already enroled in the course, attempt to enrol yourself. You should see multiple enrolment key options. Each one should have its own Unmask checkbox next to it and clicking each checkbox should unmask the corresponding key entry.

      Show
      Create a course with two or more self-enrolment options with enrolment keys. As a student who isn't already enroled in the course, attempt to enrol yourself. You should see multiple enrolment key options. Each one should have its own Unmask checkbox next to it and clicking each checkbox should unmask the corresponding key entry.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-27837

      Description

      When a course has more than one self-enrolment type with enrolment keys, the "Unmask" checkboxes for keys after the first one do not show up in the correct place and don't unmask the field. See attached screenshot of a course with two self-enrolment types, each with its own key. Instead of having one Unmask checkbox next to each enrolment key field, both checkboxes appear next to the first key field. Also, the first checkbox unmasks the first key but the second checkbox does nothing--it does not unmask either key.

      In the class definition of enrol_self_enrol_form, $mform->addElement() is passed the string "enrolpassword" which is used to generate the names and IDs of the fields and divs. This is fine with one enrolment type but with more than one, all the options have the same names and IDs. The Unmask checkboxes are inserted via Javascript into a div with ID: id_enrolpasswordunmaskdiv. There are two divs with that ID on the page so all the Unmask checkboxes show up for the first enrolment key, as seen in the screenshot.

      Unique IDs would prevent these unexpected behaviors when there is more than one enrolment key. Perhaps using the heading as part of the string would be helpful. I tried that and it puts the checkboxes in the proper places but changing the string in the addElement() call changes the field name in addition to the div ID and the unknown name breaks the form submission. So that will need to be worked out somehow too.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            mblake Michael Blake added a comment -

            This issues has been reported as causing problems for a MP. Please give it priority.

            Show
            mblake Michael Blake added a comment - This issues has been reported as causing problems for a MP. Please give it priority.
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this.

            This will be given priority attention following the release of Moodle 2.1.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this. This will be given priority attention following the release of Moodle 2.1.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            As part of this fix, password_unmask has been re-written.
            Previous implementation doesn't allow more then one password_unmask fields on one page. Also, it was not using YUI framework.

            Show
            rajeshtaneja Rajesh Taneja added a comment - As part of this fix, password_unmask has been re-written. Previous implementation doesn't allow more then one password_unmask fields on one page. Also, it was not using YUI framework.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Raj,

            This looks great.
            My +1 to submit it for integration.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Raj, This looks great. My +1 to submit it for integration.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Raj, this has been integrate now.
            However it has only been integrated to master and MOODLE_21_STABLE, I'll update the fix version shortly to reflect this.
            The reason for this is we don't make API changes to stable branches normally. In the case of MOODLE_21_STABLE this API change is safe and as such I've chosen to integrate it, however 20_STABLE is now truly stable.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Raj, this has been integrate now. However it has only been integrated to master and MOODLE_21_STABLE, I'll update the fix version shortly to reflect this. The reason for this is we don't make API changes to stable branches normally. In the case of MOODLE_21_STABLE this API change is safe and as such I've chosen to integrate it, however 20_STABLE is now truly stable. Cheers Sam
            Hide
            samhemelryk Sam Hemelryk added a comment -

            There was originally a 20_STABLE fix for this:
            Branch: wip-mdl-27837-MOODLE_20_STABLE
            Diff: https://github.com/rajeshtaneja/moodle/compare/MOODLE_20_STABLE...wip-mdl-27837-MOODLE_20_STABLE

            Show
            samhemelryk Sam Hemelryk added a comment - There was originally a 20_STABLE fix for this: Branch: wip-mdl-27837-MOODLE_20_STABLE Diff: https://github.com/rajeshtaneja/moodle/compare/MOODLE_20_STABLE...wip-mdl-27837-MOODLE_20_STABLE
            Hide
            skodak Petr Skoda added a comment -

            Tested, works as described. Big thanks!

            Show
            skodak Petr Skoda added a comment - Tested, works as described. Big thanks!
            Hide
            clicx geoff robinson added a comment -

            Hi

            I have a similar problem with 2.0.2 will be upgrading, but thought I'd patch it anyway but the link is broken. Is this patch still available? Thanks if anyone can let me know.

            Geoff

            Show
            clicx geoff robinson added a comment - Hi I have a similar problem with 2.0.2 will be upgrading, but thought I'd patch it anyway but the link is broken. Is this patch still available? Thanks if anyone can let me know. Geoff
            Hide
            skodak Petr Skoda added a comment -

            Hello, you can search for MDL-27837 inside commit messages in your local git checkout.

            Show
            skodak Petr Skoda added a comment - Hello, you can search for MDL-27837 inside commit messages in your local git checkout.

              People

              • Votes:
                16 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  1/Aug/11