Moodle
  1. Moodle
  2. MDL-35074

Ease limit on number of students per page in Grader Report imposed by max_input_vars

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.1, 2.3.4, 2.4.1
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Gradebook
    • Labels:
    • Testing Instructions:
      Hide

      (These testing instructions are largely copied from MDL-26275, with a few changes)

      Note that there are two known issues that you might encounter while testing.
      MDL-32306. Shift tabbing in the grader report isnt quite correct.
      MDL-31036. With quick feedback on you may encounter html tags in student feedback and have to un-override overridden grades twice.

      When the testing instructions say to refresh the grader report this is most safely done by clicking "Grader report" in the navigation breadcrumbs. F5 refreshing should be avoided to avoid accidentally repeating non-ajax actions.

      Go to a course with at least 10 students and at least five gradeable activities (and/or manual grade items). Set an activity to have a numeric grade and make a note of the maximum grade.

      Make sure that your student doesn't already have an overridden grade for the activity. You can un-override a grade by going to the grader report, turning on editing, clicking the edit icon and unchecking the overridden checkbox. You may need to do this twice (MDL-31036).

      Students per page safety net test

      Count the number of grade items (including category/course totals)
      Add or edit max_input_vars in php.ini. php.ini is at /etc/php5/apache2 for me but your location may be different.
      Set max_input_vars to be more than the number of grade items per student but less than twice that number. For example, if you have 6 grade items (including totals), you could set max_input_vars to 10.

      Restart apache and refresh the grader report. You should still see multiple students - up to (max_input_vars-1). You should only get a debug message similar to the following if you have more than (max_input_vars-1) students in the course:

      Reduced maximum students per page from 20 to 10. Consider increasing the PHP setting max_input_vars from 10.

      You may get other PHP warnings and errors because you've set max_input_vars so low as to make Moodle not work. Turn it back up. 1000 is the default. Restart apache.

      Grader report ajax/non-ajax test

      You may want to keep the grader report settings ("Grader report" under "My preferences" at the bottom of the dropdown) available in separate tabs for easy access; the report defaults are a site-wide setting.
      Turn grade_report_enableajax OFF.
      Turn grade_report_showquickfeedback ON.

      Go to the grader report. Clicking in empty space in a student grade cell should do nothing.

      Turn grader report ajax on and refresh the grader report. Now clicking on a cell should display two boxes. The first is for the student's grade. The second is for feedback. Enter something valid in both fields and press enter.

      The grade should change (or appear if there was no previous grade) and the cell background color should change.

      Refresh the grader report and check that the grade and feedback were saved.

      Turn ajax off then turn editing on on the grader report. All the fields will display.

      Click on edit hand/pencil icon in a student grade cell and check that the edit grade page loads after only a single click. You shouldn't have to click it twice.

      Go back to the grader report. Enter a new grade and feedback for your student and click "Update". Check that the grade and feedback were updated.

      Turn ajax on and refresh the grader report. Check that the "Update" button is hidden (may take a few seconds while the JS loads).

      Enter another grade and different feedback and tab to another grade. Refresh the grader report and check that the changes were saved.

      Enter another grade and different feedback and press enter without leaving the feedback field. Refresh the grader report and check that the changes were saved.

      Go to an empty (no grade) grade cell. Enter a grade and tab away. Refresh the grader report and check that the grade was saved.

      Turn off quick feedback and repeat the above.
      Editing mode off, ajax off (nothing happens)
      Editing mode off, ajax on
      Editing mode on, ajax off
      Editing mode on, ajax on

      Choose another grade item that uses a custom scale. Repeat these again.
      Editing mode off, ajax off (nothing happens)
      Editing mode off, ajax on
      Editing mode on, ajax off
      Editing mode on, ajax on

      Show
      (These testing instructions are largely copied from MDL-26275 , with a few changes) Note that there are two known issues that you might encounter while testing. MDL-32306 . Shift tabbing in the grader report isnt quite correct. MDL-31036 . With quick feedback on you may encounter html tags in student feedback and have to un-override overridden grades twice. When the testing instructions say to refresh the grader report this is most safely done by clicking "Grader report" in the navigation breadcrumbs. F5 refreshing should be avoided to avoid accidentally repeating non-ajax actions. Go to a course with at least 10 students and at least five gradeable activities (and/or manual grade items). Set an activity to have a numeric grade and make a note of the maximum grade. Make sure that your student doesn't already have an overridden grade for the activity. You can un-override a grade by going to the grader report, turning on editing, clicking the edit icon and unchecking the overridden checkbox. You may need to do this twice ( MDL-31036 ). Students per page safety net test Count the number of grade items (including category/course totals) Add or edit max_input_vars in php.ini. php.ini is at /etc/php5/apache2 for me but your location may be different. Set max_input_vars to be more than the number of grade items per student but less than twice that number. For example, if you have 6 grade items (including totals), you could set max_input_vars to 10. Restart apache and refresh the grader report. You should still see multiple students - up to (max_input_vars-1). You should only get a debug message similar to the following if you have more than (max_input_vars-1) students in the course: Reduced maximum students per page from 20 to 10. Consider increasing the PHP setting max_input_vars from 10. You may get other PHP warnings and errors because you've set max_input_vars so low as to make Moodle not work. Turn it back up. 1000 is the default. Restart apache. Grader report ajax/non-ajax test You may want to keep the grader report settings ("Grader report" under "My preferences" at the bottom of the dropdown) available in separate tabs for easy access; the report defaults are a site-wide setting. Turn grade_report_enableajax OFF. Turn grade_report_showquickfeedback ON. Go to the grader report. Clicking in empty space in a student grade cell should do nothing. Turn grader report ajax on and refresh the grader report. Now clicking on a cell should display two boxes. The first is for the student's grade. The second is for feedback. Enter something valid in both fields and press enter. The grade should change (or appear if there was no previous grade) and the cell background color should change. Refresh the grader report and check that the grade and feedback were saved. Turn ajax off then turn editing on on the grader report. All the fields will display. Click on edit hand/pencil icon in a student grade cell and check that the edit grade page loads after only a single click. You shouldn't have to click it twice. Go back to the grader report. Enter a new grade and feedback for your student and click "Update". Check that the grade and feedback were updated. Turn ajax on and refresh the grader report. Check that the "Update" button is hidden (may take a few seconds while the JS loads). Enter another grade and different feedback and tab to another grade. Refresh the grader report and check that the changes were saved. Enter another grade and different feedback and press enter without leaving the feedback field. Refresh the grader report and check that the changes were saved. Go to an empty (no grade) grade cell. Enter a grade and tab away. Refresh the grader report and check that the grade was saved. Turn off quick feedback and repeat the above. Editing mode off, ajax off (nothing happens) Editing mode off, ajax on Editing mode on, ajax off Editing mode on, ajax on Choose another grade item that uses a custom scale. Repeat these again. Editing mode off, ajax off (nothing happens) Editing mode off, ajax on Editing mode on, ajax off Editing mode on, ajax on
    • Workaround:
      Hide

      Turn up the PHP setting max_input_vars

      Show
      Turn up the PHP setting max_input_vars
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      MDL-35074_24_2
    • Pull Master Branch:
      MDL-35074_master_2
    • Rank:
      43693

      Description

      It would seem that the workaround for max_input_vars (MDL-26275) may be causing some confusion, as it manifests as Moodle not paying attention to the page size setting: http://moodle.org/mod/forum/discuss.php?d=209680

      The documentation for max_input_vars states that the limit "applies only to each nesting level of a multi-dimensional input array" (http://www.php.net/manual/en/info.configuration.php#ini.max-input-vars). Rather than limiting the number of students displayed per page, we can turn the grader report fields into arrays (they're currently all top-level form fields, i.e. elements of the top-level array) - thereby removing the need to limit the number of students displayed per page (or at least, increasing it hugely - it might still be sensible to keep it there, but make it count only top-level fields).

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Clearly it would be better if we could work around such form limits.

          I was trying to find related issues about the forms library accepting array inputs. At one stage I believe this became a no-no, but I could be wrong. I couldn't find any specific issues about it.

          If the array solutions is not feasible, it would still be nice to have a solution to this problem.

          Show
          Michael de Raadt added a comment - Clearly it would be better if we could work around such form limits. I was trying to find related issues about the forms library accepting array inputs. At one stage I believe this became a no-no, but I could be wrong. I couldn't find any specific issues about it. If the array solutions is not feasible, it would still be nice to have a solution to this problem.
          Hide
          Paul Nicholls added a comment - - edited

          I was worried about that too, but the grader report uses data_submitted() to just get everything in one go - it doesn't use the (optional|required)param() functions (the array versions of which only support single-dimensional arrays, not multi-dimensional (though a little recursion is probably all it'd take to bump them to multi-dimensional array support)). That still would've allowed us to ease the limit greatly, since we could have still gone with fields names such as grade{$userid}[{$itemid}] - i.e. only a couple of top-level fields per student rather than multiplying that by the number of grade items.

          Since data_submitted() doesn't have that limitation, I'm going for a multidimensional array (i.e. grade[{$userid}][{$itemid}], which gives the biggest boost to the number of students we can display per page.

          I'm currently intending to do this just for master and MOODLE_23_STABLE - would you like me to backport it any further than that? I'm not sure how far back the page limiting was ported, but it might be worth backporting the change to arrays anyway - we even got hit by max_input_vars on 1.9 when max_input_vars was introduced by a patch to PHP 5.1.6.

          Oh dear, noformat tags aren't the answer! I'll monospace some bits and see what happens.

          Show
          Paul Nicholls added a comment - - edited I was worried about that too, but the grader report uses data_submitted() to just get everything in one go - it doesn't use the (optional|required) param() functions (the array versions of which only support single-dimensional arrays, not multi-dimensional (though a little recursion is probably all it'd take to bump them to multi-dimensional array support)). That still would've allowed us to ease the limit greatly, since we could have still gone with fields names such as grade {$userid} [{$itemid}] - i.e. only a couple of top-level fields per student rather than multiplying that by the number of grade items. Since data_submitted() doesn't have that limitation, I'm going for a multidimensional array (i.e. grade [{$userid}] [{$itemid}] , which gives the biggest boost to the number of students we can display per page. I'm currently intending to do this just for master and MOODLE_23_STABLE - would you like me to backport it any further than that? I'm not sure how far back the page limiting was ported, but it might be worth backporting the change to arrays anyway - we even got hit by max_input_vars on 1.9 when max_input_vars was introduced by a patch to PHP 5.1.6. Oh dear, noformat tags aren't the answer! I'll monospace some bits and see what happens.
          Hide
          Paul Nicholls added a comment -

          Patch attached for master and MOODLE_23_STABLE. Note that the diff may look rather broken, or like there are some funky/unrelated changes happening - this seems to be due to the changed indentation from adding a couple of nested foreach loops.

          In spite of the wonky diff, Winmerge (comparing the changed version to the current master or MOODLE_23_STABLE as appropriate) set to ignore whitespace changes will correctly show that there are actually only a few non-whitespace changes.

          Show
          Paul Nicholls added a comment - Patch attached for master and MOODLE_23_STABLE. Note that the diff may look rather broken, or like there are some funky/unrelated changes happening - this seems to be due to the changed indentation from adding a couple of nested foreach loops. In spite of the wonky diff, Winmerge (comparing the changed version to the current master or MOODLE_23_STABLE as appropriate) set to ignore whitespace changes will correctly show that there are actually only a few non-whitespace changes.
          Hide
          Paul Nicholls added a comment -

          I didn't realise I hadn't submitted this for peer review yet - so here goes!

          Show
          Paul Nicholls added a comment - I didn't realise I hadn't submitted this for peer review yet - so here goes!
          Hide
          Paul Nicholls added a comment -

          I see this is still awaiting peer review - Michael, are you able to get this onto somebody's peer review queue? Let me know if it needs to be rebased onto the latest MOODLE_23_STABLE / master (it's been a while!).

          Show
          Paul Nicholls added a comment - I see this is still awaiting peer review - Michael, are you able to get this onto somebody's peer review queue? Let me know if it needs to be rebased onto the latest MOODLE_23_STABLE / master (it's been a while!).
          Hide
          Dan Poltawski added a comment -

          Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..

          Show
          Dan Poltawski added a comment - Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..
          Hide
          Dan Poltawski added a comment -

          Sending this back to 'waiting for peer review' as I think it needs some review before going in. Hopefully Andrew D will be able to take a look.

          Show
          Dan Poltawski added a comment - Sending this back to 'waiting for peer review' as I think it needs some review before going in. Hopefully Andrew D will be able to take a look.
          Hide
          Hubert Simms added a comment -

          This problem is really causing us major problems. We have around 11,000 courses with around 800 courses a semester. This semester, I had many courses that lost assignments. I had a class with 106 students where the first 6 assignment grades were dropped from the gradebook. Our faculty is starting to lose faith in the system when it is doing things like this. The current max_input_vars is at 20.000 in Apache. We are running Moodle 2.3.2.

          Show
          Hubert Simms added a comment - This problem is really causing us major problems. We have around 11,000 courses with around 800 courses a semester. This semester, I had many courses that lost assignments. I had a class with 106 students where the first 6 assignment grades were dropped from the gradebook. Our faculty is starting to lose faith in the system when it is doing things like this. The current max_input_vars is at 20.000 in Apache. We are running Moodle 2.3.2.
          Hide
          Andrew Davis added a comment -

          "// we must verify course id here!"

          I realize you didn't add it but can you please get rid of this comment. I really cant think of any additional verification that is necessary but I wonder about it every time I see the comment.

          Although the diff looks a little hairy the change is actually pretty simple. I'm happy with it.

          Once this is integrated I'm happy to take on the testing.

          Show
          Andrew Davis added a comment - "// we must verify course id here!" I realize you didn't add it but can you please get rid of this comment. I really cant think of any additional verification that is necessary but I wonder about it every time I see the comment. Although the diff looks a little hairy the change is actually pretty simple. I'm happy with it. Once this is integrated I'm happy to take on the testing.
          Hide
          Paul Nicholls added a comment -

          I tried to rebase, but I didn't fancy scrutinising everything in detail to make sure it wasn't either messing things up or ditching any other changes - so I redid the changes on fresh branches, adding one for 2.4 as I went. I've removed the "we must verify course id here!" comment.

          Show
          Paul Nicholls added a comment - I tried to rebase, but I didn't fancy scrutinising everything in detail to make sure it wasn't either messing things up or ditching any other changes - so I redid the changes on fresh branches, adding one for 2.4 as I went. I've removed the "we must verify course id here!" comment.
          Hide
          Andrew Davis added a comment -

          Submit for integration whenever you're ready. If you are not able to let me know and Ill do it for you.

          Show
          Andrew Davis added a comment - Submit for integration whenever you're ready. If you are not able to let me know and Ill do it for you.
          Hide
          Paul Nicholls added a comment -

          Hi Andrew,
          Can you please submit this for integration? I don't seem to have permission to do it (or, if I do, it's hidden very well).

          Cheers,
          Paul

          Show
          Paul Nicholls added a comment - Hi Andrew, Can you please submit this for integration? I don't seem to have permission to do it (or, if I do, it's hidden very well). Cheers, Paul
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Dan Poltawski added a comment -

          Great work, thanks Paul! Integrated to master, 24 and 23.

          Show
          Dan Poltawski added a comment - Great work, thanks Paul! Integrated to master, 24 and 23.
          Hide
          Andrew Davis added a comment -

          All tests fine in master.

          Show
          Andrew Davis added a comment - All tests fine in master.
          Hide
          Andrew Davis added a comment -

          2.3 looks good. I'm going offline for a few hours. I'll test 2.4 when I get back. If anyone else wants to jump in before then and test 2.4 that's fine too.

          Show
          Andrew Davis added a comment - 2.3 looks good. I'm going offline for a few hours. I'll test 2.4 when I get back. If anyone else wants to jump in before then and test 2.4 that's fine too.
          Hide
          Andrew Davis added a comment -

          2.4 looks good as well. Passing.

          Show
          Andrew Davis added a comment - 2.4 looks good as well. Passing.
          Hide
          David Monllaó added a comment -

          Wow Andrew you are fast! After assigning all the tests I've reviewed my coherence filter and I've realized that you was also the peer reviewer so I've changed the tester, reassigning back you again, sorry for the confusion

          Show
          David Monllaó added a comment - Wow Andrew you are fast! After assigning all the tests I've reviewed my coherence filter and I've realized that you was also the peer reviewer so I've changed the tester, reassigning back you again, sorry for the confusion
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao
          Hide
          Mary Cooch added a comment - - edited

          Just made a quick note of this in the 2.3 and 2.4 docs - hope I have understood it correctly: http://docs.moodle.org/24/en/Grade_settings#Grader_report_preferences

          Show
          Mary Cooch added a comment - - edited Just made a quick note of this in the 2.3 and 2.4 docs - hope I have understood it correctly: http://docs.moodle.org/24/en/Grade_settings#Grader_report_preferences
          Hide
          Tim Hunt added a comment -

          Paul (and others). The change you did here was great wiht PHP 5.3, but PHP 5.4 tightened the rules again, and now we have dataloss again.

          I guess we either need to revert this change, or do a fix like MDL-41451 in the grader report.

          I am prepared to revert this change. I don't have time to do a MDL-41451-like fix now. Could anyone else take that on?

          Show
          Tim Hunt added a comment - Paul (and others). The change you did here was great wiht PHP 5.3, but PHP 5.4 tightened the rules again, and now we have dataloss again. I guess we either need to revert this change, or do a fix like MDL-41451 in the grader report. I am prepared to revert this change. I don't have time to do a MDL-41451 -like fix now. Could anyone else take that on?
          Hide
          Tim Hunt added a comment -

          MDL-41819 already reported. I am going to add more information there.

          Show
          Tim Hunt added a comment - MDL-41819 already reported. I am going to add more information there.

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: