Moodle
  1. Moodle
  2. MDL-27045

disabledIf does not work for advcheckbox

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.9.11, 2.0.5, 2.1.2, 2.2
    • Fix Version/s: 2.0.6, 2.1.3
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      0) Note: Due to the imminent minor and major releases this has to be tested under ALL branches. Important!

      1) Test that other fields can depend on an advchechbox:

      Verify that the following pattern of enabled/disabled text fields is shown (with + meaning "enabled" and - meaning "greyed out"):

      When checkbox is ticked: + - + - +
      When checkbox is not ticked: - + - + -

      2) Test that an advcheckbox can depend on other elements:

      • Go the the question bank and ask to create a new calculatedmulti question. Verify that no warnings like "Warning: Missing argument 1 for HTML_QuickForm_advcheckbox::getPrivateName()" are shown.
      Show
      0) Note: Due to the imminent minor and major releases this has to be tested under ALL branches. Important! 1) Test that other fields can depend on an advchechbox: Place testcheckbox.php (as attached) into your main Moodle directory. Open the page in a web browser, as http://....pathtoyourmoodle.../testcheckbox.php Tick and untick the checkbox on this page. Verify that the following pattern of enabled/disabled text fields is shown (with + meaning "enabled" and - meaning "greyed out"): When checkbox is ticked: + - + - + When checkbox is not ticked: - + - + - 2) Test that an advcheckbox can depend on other elements: Go the the question bank and ask to create a new calculatedmulti question. Verify that no warnings like "Warning: Missing argument 1 for HTML_QuickForm_advcheckbox::getPrivateName()" are shown.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      When creating a form element that uses disabledIf that checks against a advcheckbox type, the element always remains disabled. This is related to MDL-18522

        Gliffy Diagrams

        1. checkbox19.patch
          0.5 kB
          Tim Gus
        2. checkboxtest_extra.php
          2 kB
          Tim Hunt
        3. checkboxtest.php
          1 kB
          Henning Bostelmann
        4. formlib.patch
          3 kB
          Tom Potts

          Issue Links

            Activity

            Hide
            Tom Potts added a comment -

            This is still a bug in the development branch, so could someone please confirm and update the issue?

            Thanks

            Show
            Tom Potts added a comment - This is still a bug in the development branch, so could someone please confirm and update the issue? Thanks
            Hide
            Tom Potts added a comment -

            Patch to fix this problem.
            Patched against git branch MOODLE_21_STABLE 2011-08-26.
            Works with my own tests, with 'checked', 'notchecked', 'eq' and 'neq' conditions. Contains no comments to keep with the rest of the file, but note there are three unused lines that can be pruned in 'checked'.

            Show
            Tom Potts added a comment - Patch to fix this problem. Patched against git branch MOODLE_21_STABLE 2011-08-26. Works with my own tests, with 'checked', 'notchecked', 'eq' and 'neq' conditions. Contains no comments to keep with the rest of the file, but note there are three unused lines that can be pruned in 'checked'.
            Hide
            Katarzyna Potocka added a comment -

            Is this patch recommended or will there be a different solution to enable disabledif for advcheckbox?!?

            Show
            Katarzyna Potocka added a comment - Is this patch recommended or will there be a different solution to enable disabledif for advcheckbox?!?
            Hide
            Henning Bostelmann added a comment -

            I'm not sure where (and if) advcheckbox is used in conjunction with disabledif in Moodle core. I'm therefore attaching a test script for reproducing/testing this bug. Place the file checboxtext.php into your main Moodle directory and call it via the web server. It contains an advanced checkbox and five text fields that depend on the checkbox via "disabledif", with five different conditions.

            Show
            Henning Bostelmann added a comment - I'm not sure where (and if) advcheckbox is used in conjunction with disabledif in Moodle core. I'm therefore attaching a test script for reproducing/testing this bug. Place the file checboxtext.php into your main Moodle directory and call it via the web server. It contains an advanced checkbox and five text fields that depend on the checkbox via "disabledif", with five different conditions.
            Hide
            Henning Bostelmann added a comment -

            I'm submitting Tom's fix for review. Note that the master version is not a simple cherry-pick from the 2.1 version, since there have been intermediate changes to form.js.

            Show
            Henning Bostelmann added a comment - I'm submitting Tom's fix for review. Note that the master version is not a simple cherry-pick from the 2.1 version, since there have been intermediate changes to form.js.
            Hide
            Rajesh Taneja added a comment -

            Thanks Tom and Henning for proving a fix for this

            I feel this can be improved a bit. As the patch is looking for hidden elements and not advance checkbox, this might lead to situation where checkbox and hidden elements with same name are defined and can lead to new bugs.

            This should be fixed by:

            1. Distinguishing advance checkbox from usual checkbox (Probably by adding a specific class to advcheckbox)
            2. forms.js should check if checkbox is advanced checkbox and behave accordingly.
            Show
            Rajesh Taneja added a comment - Thanks Tom and Henning for proving a fix for this I feel this can be improved a bit. As the patch is looking for hidden elements and not advance checkbox, this might lead to situation where checkbox and hidden elements with same name are defined and can lead to new bugs. This should be fixed by: Distinguishing advance checkbox from usual checkbox (Probably by adding a specific class to advcheckbox) forms.js should check if checkbox is advanced checkbox and behave accordingly.
            Hide
            Tim Hunt added a comment -

            I just hit this problem. I have an advcheckbox called qmfilter, and I am trying to do

            $mform->disabledIf('qmfilter', 'users', 'eq', quiz_attempt_report::WITHOUT);

            I get
            Warning: Missing argument 1 for HTML_QuickForm_advcheckbox::getPrivateName(), called in /fs1/www_root/tjh238/moodle_head/lib/formslib.php on line 1974 and defined in /fs1/www_root/tjh238/moodle_head/lib/pear/HTML/QuickForm/advcheckbox.php on line 99

            Notice: Undefined variable: elementName in /fs1/www_root/tjh238/moodle_head/lib/pear/HTML/QuickForm/advcheckbox.php on line 101

            I think we need this change:

            diff --git a/lib/formslib.php b/lib/formslib.php
            index f02469e..ba9666c 100644
            --- a/lib/formslib.php
            +++ b/lib/formslib.php
            @@ -1970,7 +1970,8 @@ function validate_' . $this->_formName . '(frm) {
                     } else if (is_a($element, 'HTML_QuickForm_hidden')) {
                         return array();
             
            -        } else if (method_exists($element, 'getPrivateName')) {
            +        } else if (method_exists($element, 'getPrivateName') &&
            +                !($element instanceof HTML_QuickForm_advcheckbox)) {
                         return array($element->getPrivateName());
             
                     } else {
            

            because HTML_QuickForm_advcheckbox defines a getPrivateName method, but that method is really private to advcheckbox (actually, it is deprecated and not used), it does not follow the generic method signature that this code is looking for.

            Show
            Tim Hunt added a comment - I just hit this problem. I have an advcheckbox called qmfilter, and I am trying to do $mform->disabledIf('qmfilter', 'users', 'eq', quiz_attempt_report::WITHOUT); I get Warning: Missing argument 1 for HTML_QuickForm_advcheckbox::getPrivateName(), called in /fs1/www_root/tjh238/moodle_head/lib/formslib.php on line 1974 and defined in /fs1/www_root/tjh238/moodle_head/lib/pear/HTML/QuickForm/advcheckbox.php on line 99 Notice: Undefined variable: elementName in /fs1/www_root/tjh238/moodle_head/lib/pear/HTML/QuickForm/advcheckbox.php on line 101 I think we need this change: diff --git a/lib/formslib.php b/lib/formslib.php index f02469e..ba9666c 100644 --- a/lib/formslib.php +++ b/lib/formslib.php @@ -1970,7 +1970,8 @@ function validate_' . $this->_formName . '(frm) { } else if (is_a($element, 'HTML_QuickForm_hidden')) { return array();   - } else if (method_exists($element, 'getPrivateName')) { + } else if (method_exists($element, 'getPrivateName') && + !($element instanceof HTML_QuickForm_advcheckbox)) { return array($element->getPrivateName());   } else { because HTML_QuickForm_advcheckbox defines a getPrivateName method, but that method is really private to advcheckbox (actually, it is deprecated and not used), it does not follow the generic method signature that this code is looking for.
            Hide
            Tim Hunt added a comment -

            I have done a revised version of Henning's fix, that I think deals with Rajesh's comment in a simple way, and which also fixes the problem that I found.

            I am really grateful that Henning made testcheckbox.php!

            Show
            Tim Hunt added a comment - I have done a revised version of Henning's fix, that I think deals with Rajesh's comment in a simple way, and which also fixes the problem that I found. I am really grateful that Henning made testcheckbox.php!
            Hide
            Tim Hunt added a comment -

            Argh! Just found a minor problem with my own patch. The this.ancestor('div.felement.fcheckbox') test is too strict. It does not work when the advcheckbox is inside a group. I think I need to try to change it to look for a sibling input element with the same name ...

            Show
            Tim Hunt added a comment - Argh! Just found a minor problem with my own patch. The this.ancestor('div.felement.fcheckbox') test is too strict. It does not work when the advcheckbox is inside a group. I think I need to try to change it to look for a sibling input element with the same name ...
            Hide
            Tim Hunt added a comment -

            Here is a super-crazy extension to Henning's test script. The most interesting bit for the regression I just commented on is the final checkbox (which is in a group) and the text field that comes after it.

            All branches now amended to make that work.

            Show
            Tim Hunt added a comment - Here is a super-crazy extension to Henning's test script. The most interesting bit for the regression I just commented on is the final checkbox (which is in a group) and the text field that comes after it. All branches now amended to make that work.
            Hide
            Tim Hunt added a comment -

            Rajesh, can you predict when you will be able to peer-review this? It is quite a nasty bug (if you have forms using advcheckbox, which we do in the question types) so I would really like to get this in before 2.2 if possible.

            If you don't have time to look at this, let me know so I can try to find someone else. Thanks.

            Show
            Tim Hunt added a comment - Rajesh, can you predict when you will be able to peer-review this? It is quite a nasty bug (if you have forms using advcheckbox, which we do in the question types) so I would really like to get this in before 2.2 if possible. If you don't have time to look at this, let me know so I can try to find someone else. Thanks.
            Hide
            Tim Hunt added a comment -

            We are starting to get duplicate reports, so upping the priority.

            Show
            Tim Hunt added a comment - We are starting to get duplicate reports, so upping the priority.
            Hide
            Rajesh Taneja added a comment -

            Hello Tim,
            Sorry, Last week I was not working. I will do it today

            Show
            Rajesh Taneja added a comment - Hello Tim, Sorry, Last week I was not working. I will do it today
            Hide
            Rajesh Taneja added a comment -

            Patch seems to be spot-on Tim
            I can't think of a better solution then this.

            FYI:
            You might have seen:
            As, getPrivateName in advcheckbox is a deprecated function, it might be nice to get rid of this in master and remove extra checking in forms.php for performance sake.

            Show
            Rajesh Taneja added a comment - Patch seems to be spot-on Tim I can't think of a better solution then this. FYI: You might have seen: As, getPrivateName in advcheckbox is a deprecated function, it might be nice to get rid of this in master and remove extra checking in forms.php for performance sake.
            Hide
            Tim Hunt added a comment -

            Thanks Rajesh. Submitting for integration now.

            I could have cleaned up the deprecated function, but it did not occur to me at the time, I am not going to change the code now.

            Show
            Tim Hunt added a comment - Thanks Rajesh. Submitting for integration now. I could have cleaned up the deprecated function, but it did not occur to me at the time, I am not going to change the code now.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I've created MDL-30418 as one followup of this to be done after releases.

            I'm going to integrate this right now, only because it's required by those bloody new uses, grrr. I don't like modifying that days before stable and major releases.

            Many thanks, anyway! Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - I've created MDL-30418 as one followup of this to be done after releases. I'm going to integrate this right now, only because it's required by those bloody new uses, grrr. I don't like modifying that days before stable and major releases. Many thanks, anyway! Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
            Hide
            Aparup Banerjee added a comment -

            sorry, grabbing this - we need testing to be done for roll upstream.

            Show
            Aparup Banerjee added a comment - sorry, grabbing this - we need testing to be done for roll upstream.
            Hide
            Aparup Banerjee added a comment -

            Thanks for the awesome tests! this has passed for me!

            Show
            Aparup Banerjee added a comment - Thanks for the awesome tests! this has passed for me!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            And this has landed upstream, just on time for the upcoming new releases week. Thanks for it!

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - And this has landed upstream, just on time for the upcoming new releases week. Thanks for it! Ciao
            Hide
            Tim Gus added a comment -

            Will there be a fix for 1.9?

            Show
            Tim Gus added a comment - Will there be a fix for 1.9?
            Hide
            Tim Hunt added a comment -

            No. This will almost certainly not get fixed in 1.9. Support for 1.9 is now on a security-issues-only basis.

            Of course, if someone volunteers to create a fix, it will probably get integrated, but don't expect any of the Moodle HQ developers (or me) to work on it.

            Show
            Tim Hunt added a comment - No. This will almost certainly not get fixed in 1.9. Support for 1.9 is now on a security-issues-only basis. Of course, if someone volunteers to create a fix, it will probably get integrated, but don't expect any of the Moodle HQ developers (or me) to work on it.
            Hide
            Tim Gus added a comment -

            Patch for 1.9, taken from the version 2, which removes the missing argument warnings.

            Show
            Tim Gus added a comment - Patch for 1.9, taken from the version 2, which removes the missing argument warnings.
            Hide
            Aparup Banerjee added a comment -

            Hi Tim Gus,
            Thanks for helping out and providing a patch . This issue has already been closed though so you may want to open up a new (and linked) issue to look into getting this fix in 1.9

            Show
            Aparup Banerjee added a comment - Hi Tim Gus, Thanks for helping out and providing a patch . This issue has already been closed though so you may want to open up a new (and linked) issue to look into getting this fix in 1.9

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: