Moodle
  1. Moodle
  2. MDL-32150

Select all button should not check disabled checkboxes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.2
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide
      1. copy attached files in your moodle website
      2. log in as admin and navigate to test-form.php
      3. Click on "check all" and make sure all checkbox get checked except disabled one
      4. Click on "uncheck all" and make sure all checkbox get unchecked except disabled one
      5. click on last checkbox and checkbox (one above gets disabled)
      6. Now repeat 3 and 4, make sure disabled one doesn't get updated.

      Test 2:

      1. Log in as admin
      2. navigate to test-mform.php
      3. click both "select all/none", and make sure checkbox disable toggle, except for disabled ones
      4. uncheck X (first checkbox) and Y1(last checkbox), get enabled
      5. now click second "select all/none" and make sure they get checked and unchecked.
      6. Now disable JS and reload page
      7. Click "select all/none" and make sure disabled one's doesn't change.

      note
      make sure to test all branches on at-least ie7 and ff

      Show
      copy attached files in your moodle website log in as admin and navigate to test-form.php Click on "check all" and make sure all checkbox get checked except disabled one Click on "uncheck all" and make sure all checkbox get unchecked except disabled one click on last checkbox and checkbox (one above gets disabled) Now repeat 3 and 4, make sure disabled one doesn't get updated. Test 2: Log in as admin navigate to test-mform.php click both "select all/none", and make sure checkbox disable toggle, except for disabled ones uncheck X (first checkbox) and Y1(last checkbox), get enabled now click second "select all/none" and make sure they get checked and unchecked. Now disable JS and reload page Click "select all/none" and make sure disabled one's doesn't change. note make sure to test all branches on at-least ie7 and ff
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-32150
    • Rank:
      38874

      Description

      The specific instance of this that was encountered can be found on MDL-32137. But I guess this is a more generic stuff. "checkall" function checks all check-boxes doesn't matter if they are tagged disabled or not. Same is the case with "select none", it unchecks everything.
      I am not sure if this is going to cause any security risk or not, normally we have validation on the server side for these cases, still if we proper validation is missing than it can cause security risk.

      1. test-form.php
        2 kB
        Rajesh Taneja
      2. test-mform.php
        1 kB
        Rajesh Taneja

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          Thanks for repoting this Ankit

          Show
          Rajesh Taneja added a comment - Thanks for repoting this Ankit
          Hide
          Rajesh Taneja added a comment -

          There are two commits for this:

          1. Fixing check all in static library (These button don't work in NON-JS mode.)
          2. Fix for checkboxcontroller. With JS it was working fine (YUI takes care of it), but without JS it was breaking.

          I think this form should be converted to mform, probably another issue.

          Show
          Rajesh Taneja added a comment - There are two commits for this: Fixing check all in static library (These button don't work in NON-JS mode.) Fix for checkboxcontroller. With JS it was working fine (YUI takes care of it), but without JS it was breaking. I think this form should be converted to mform, probably another issue.
          Hide
          Sam Hemelryk added a comment -

          Raj, have you tested this with a form where elements are disabled by default because of rules, but can later be enabled by those rules through JS?

          Show
          Sam Hemelryk added a comment - Raj, have you tested this with a form where elements are disabled by default because of rules, but can later be enabled by those rules through JS?
          Hide
          Rajesh Taneja added a comment -

          Good point Sam,

          AFAIK it should work, but will test this and put the same in testing instruction.
          FYI:
          I did some testing with checkbox controller, and seems all is working fine.

          Show
          Rajesh Taneja added a comment - Good point Sam, AFAIK it should work, but will test this and put the same in testing instruction. FYI: I did some testing with checkbox controller, and seems all is working fine.
          Hide
          Rajesh Taneja added a comment -

          You were right Sam, with disabled if, YUI was not not working as expected.
          Have updated the fix and attaching test case.

          Show
          Rajesh Taneja added a comment - You were right Sam, with disabled if, YUI was not not working as expected. Have updated the fix and attaching test case.
          Hide
          Rajesh Taneja added a comment -

          test-form.php is test-case with simple form elements.
          test-mform.php is test case with mform.

          Show
          Rajesh Taneja added a comment - test-form.php is test-case with simple form elements. test-mform.php is test case with mform.
          Hide
          Rajesh Taneja added a comment -

          javascript-ststic.js has three more functions doing similar job. Not sure if we need to fix them as well.

          1. select_all_in_element_with_id
          2. select_all_in
          3. deselect_all_in
          Show
          Rajesh Taneja added a comment - javascript-ststic.js has three more functions doing similar job. Not sure if we need to fix them as well. select_all_in_element_with_id select_all_in deselect_all_in
          Hide
          Jason Fowler added a comment -

          Code looks fine Raj

          Show
          Jason Fowler added a comment - Code looks fine Raj
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Rajesh Taneja added a comment -

          Branches re-based

          Show
          Rajesh Taneja added a comment - Branches re-based
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          Sending this back so that you can tidy up the undefined $newselectvalue variable in 21, and 22 stable.
          Also can you please confirm that you have tested the 21 changes in IE7, the reason I ask is because I am unsure how cross browser inputs[i].disabled || inputs[i].readOnly is.
          If it works in IE7 (which is the minimum version for 21) then I am happy. I imagine disabled will be fine, but I am unsure whether readOnly works.
          It would also be great to work those clauses into the if in the line above so that we don't need a second if when there is no else.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, Sending this back so that you can tidy up the undefined $newselectvalue variable in 21, and 22 stable. Also can you please confirm that you have tested the 21 changes in IE7, the reason I ask is because I am unsure how cross browser inputs [i] .disabled || inputs [i] .readOnly is. If it works in IE7 (which is the minimum version for 21) then I am happy. I imagine disabled will be fine, but I am unsure whether readOnly works. It would also be great to work those clauses into the if in the line above so that we don't need a second if when there is no else. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          I didn't test this on 2.1, but will check this now.
          I checked the changes on master, and checking for both disabled and readonly was working on ie7. The reason I checked both is to make sure we don't miss on readonly elements.

          Also, can you please suggest if changes in following make sense?

          1. select_all_in_element_with_id
          2. select_all_in
          3. deselect_all_in
          Show
          Rajesh Taneja added a comment - I didn't test this on 2.1, but will check this now. I checked the changes on master, and checking for both disabled and readonly was working on ie7. The reason I checked both is to make sure we don't miss on readonly elements. Also, can you please suggest if changes in following make sense? select_all_in_element_with_id select_all_in deselect_all_in
          Hide
          Rajesh Taneja added a comment - - edited

          sorry for wrong patches on 22 and 21. all fixed, tested 21 on ie7 and it works fine.

          Show
          Rajesh Taneja added a comment - - edited sorry for wrong patches on 22 and 21. all fixed, tested 21 on ie7 and it works fine.
          Hide
          Sam Hemelryk added a comment -

          Thanks Raj, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Raj, this has been integrated now.
          Hide
          Andrew Davis added a comment -

          Works as described in master, 2.2 stable and 2.1 stable. I dont have access to IE but tested it in FF and chrome.

          The behaviour is a little odd when one checkbox is disabling another. Hopefully thats just a product of the test script. If I check a checkbox it disables another checkbox. If I click a button that checks that same checkbox nothing happens.

          Show
          Andrew Davis added a comment - Works as described in master, 2.2 stable and 2.1 stable. I dont have access to IE but tested it in FF and chrome. The behaviour is a little odd when one checkbox is disabling another. Hopefully thats just a product of the test script. If I check a checkbox it disables another checkbox. If I click a button that checks that same checkbox nothing happens.
          Hide
          Rajesh Taneja added a comment -

          This is how testcase is designed. Tried to get different possible combinations.

          Show
          Rajesh Taneja added a comment - This is how testcase is designed. Tried to get different possible combinations.
          Hide
          Aparup Banerjee added a comment -

          The code here has been spread to upstream moodle repositories and mirrors for anyone to use .

          Closing, have a good weekend!

          Show
          Aparup Banerjee added a comment - The code here has been spread to upstream moodle repositories and mirrors for anyone to use . Closing, have a good weekend!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: