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:
    • Rank:
      16679

      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

      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: