Moodle
  1. Moodle
  2. MDL-26275

Large gradebook won't save changes due to POST size limit

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.9.10, 2.1.3, 2.2, STABLE backlog
    • Fix Version/s: 2.3
    • Component/s: Gradebook
    • Environment:
      Apache 2.2.10, PHP 5.2.14, SLES 11 SP1, Oracle 11g
    • Database:
      Oracle
    • Testing Instructions:
      Hide

      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 two students and at least one gradeable activity. 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

      Edit /grade/report/grader/lib.php around line 1710. You need to print out the value of $fieldsperstudent ie var_dump($fieldsperstudent); or similar.
      Refresh the grader report and make a note of the number of fields needed per student. It depends on the number of activities plus some other settings.
      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 $fieldsperstudent but less than 2 * $fieldsperstudent. For example, my $fieldsperstudent was 18 so I did this:
      max_input_vars = 20

      Restart apache and refresh the grader report. Only a single student should be displayed. You should get a debug message similar to the following plus a stack trace:

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

      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 two following site settings available in separate tabs for easy access.
      Turn grade_report_enableajax OFF. Grader report ajax is a site setting.
      Turn grade_report_showquickfeedback ON. Show quick feedback is also a site setting.

      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
      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 two students and at least one gradeable activity. 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 Edit /grade/report/grader/lib.php around line 1710. You need to print out the value of $fieldsperstudent ie var_dump($fieldsperstudent); or similar. Refresh the grader report and make a note of the number of fields needed per student. It depends on the number of activities plus some other settings. 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 $fieldsperstudent but less than 2 * $fieldsperstudent. For example, my $fieldsperstudent was 18 so I did this: max_input_vars = 20 Restart apache and refresh the grader report. Only a single student should be displayed. You should get a debug message similar to the following plus a stack trace: Reduced maximum students per page from 20 to 1. Consider increasing the PHP setting max_input_vars from 20. 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 two following site settings available in separate tabs for easy access. Turn grade_report_enableajax OFF. Grader report ajax is a site setting. Turn grade_report_showquickfeedback ON. Show quick feedback is also a site setting. 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

      Increase the value of max_input_vars in php.ini.

      Show
      Increase the value of max_input_vars in php.ini.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-26275_large_grader
    • Rank:
      15881

      Description

      We have a course with 47 students and 18 assignments/tests/quizzes. When we try to update the grades for the students in the gradebook, only part of the grades are saved. For the ones that don't save, we have to click on the little "hand" icon to manually "edit" the grade, and it will save then.

      Using Firebug, I traced this down to a problem with how Moodle saves the changes to the gradebook, basically by creating one giant form POST containing all the fields and values, like:

      id=2161&sesskey=F9J3pymnk6&report=grader&oldgrade_13214_4098=8.00&grade_13214_4098=8.00&oldgrade_13214_4099=&grade_13214_4099=&oldgrade_13214_4100=&grade_13214_4100=&oldgrade_13214_4101=.....

      In our case, the form post for this gradebook ended up being 33184 bytes, which appears to be too large for something (Apache/Firefox/IE/PHP/linux/etc.) to handle.

      To test things out, I made a php page in our Moodle instance (moodle/test/index.php) containing the following code to output all the information it received:

      <?php
         if ($_REQUEST) {
            echo "<table border=1> \n";
            echo " <tr> \n";
            echo "  <td><b>result_name </b></td> \n ";
            echo "  <td><b>result_val  </b></td> \n ";
            echo " </tr> \n";
            while (list($result_nme, $result_val) = each($_REQUEST)) {
               echo " <tr> \n";
               echo "  <td> $result_nme </td> \n";
               echo "  <td> $result_val </td> \n";
               echo " </tr> \n";
            }
            echo "</table> \n";
         }
      ?>
      

      I then created a copy of the index.php page inside moodle/grade/report/grader/ and pointed that copy of the page to post its data to the test page I created.

      I've attached a couple of files to help illustrate this:
      outputPostAttribs.pdf contains the browser output of that test recipient page I made.
      FirebugLocationWithRequestHeaders.txt is an output of the Form POST data that Firebug reported

      The two grades I entered (at the bottom of the list) were the grades for students with Moodle ID's 32738 and 7348 (I gave them a grade of 2.) When you parse through the Firebug file, you can see the following values:
      grade_32738_4098=2
      grade_7348_4098=2
      However, those fields aren't in the list that Apache received.

      The bad thing is that I'm not aware of an easy workaround for this issue short of a complete overhaul of the Moodle gradebook.

      1. FirebugLocationWithRequestHeaders.txt
        32 kB
        Chris Myers
      2. outputPostAttribs.pdf
        107 kB
        Chris Myers

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          Hello,
          this is usually caused by "security" extensions such as Suhosin, please make sure it is disabled or tweak the configuration as necessary.
          Hopefully somebody will rewrite the grader report in some future Moodle version.

          Petr

          Show
          Petr Škoda added a comment - Hello, this is usually caused by "security" extensions such as Suhosin, please make sure it is disabled or tweak the configuration as necessary. Hopefully somebody will rewrite the grader report in some future Moodle version. Petr
          Hide
          Chris Myers added a comment -

          Cool, disabling suhosin took care of it.

          For others who might run across this issue, you can add the following lines to your php.ini file:

          [suhosin]
          ;Disable suhosin
          suhosin.simulation = On

          Show
          Chris Myers added a comment - Cool, disabling suhosin took care of it. For others who might run across this issue, you can add the following lines to your php.ini file: [suhosin] ;Disable suhosin suhosin.simulation = On
          Hide
          Andrew Davis added a comment -

          There is some forum discussion of what sounds like the same issue. http://moodle.org/mod/forum/discuss.php?d=195431

          Show
          Andrew Davis added a comment - There is some forum discussion of what sounds like the same issue. http://moodle.org/mod/forum/discuss.php?d=195431
          Hide
          Michael de Raadt added a comment -

          It looks like this has been reported a number of times around the tracker.

          To summarise what others have said, this is not a bug in Moodle, it is a limitation of PHP, which, since 5.3.9 limits the number of parameters that can be passed in a request. This is controlled by max_input_vars, set in php.ini, which defaults to 1000.

          So the question is: what to do about it? It is possible that we could (at least in the Gradebook, but possibly elsewhere) compare the number of input elements against this limit and constrain the user with paging so this error does not occur. This would be better than a warning, I think.

          Input on resolving this issue is welcome.

          Show
          Michael de Raadt added a comment - It looks like this has been reported a number of times around the tracker. To summarise what others have said, this is not a bug in Moodle, it is a limitation of PHP, which, since 5.3.9 limits the number of parameters that can be passed in a request. This is controlled by max_input_vars , set in php.ini, which defaults to 1000. So the question is: what to do about it? It is possible that we could (at least in the Gradebook, but possibly elsewhere) compare the number of input elements against this limit and constrain the user with paging so this error does not occur. This would be better than a warning, I think. Input on resolving this issue is welcome.
          Hide
          Andrew Davis added a comment - - edited

          There are a few things we could do. This all assumes we can either get the value of max_input_vars or make a reasonable guess as to its value.

          1) Overrule the "students-per-page" grader report setting if it would result in a number of fields above max_input_vars.

          2) display a warning on the grader report if the total number of fields is above max_input_vars.

          3) display a warning on the environment page about max_input_vars. Not sure what we would say exactly as we'd have to kind of guess what value to tell them to set it to. Or we could query the DB to find the course with the most grade items and students. Not sure this is a good idea.

          4) prevent the user turning editing mode on (with an explanatory error message) if the number of fields would be above max_input_vars.

          4. Change the way grades are submitted to the server to reduce the number of fields that must be submitted. That would be good although some of the above still wants doing as there will be someone out there who will hit max_input_vars anyway.

          I think my vote goes to paging (#1) plus displaying a warning explaining why if the the number of students per page has been reduced.

          Show
          Andrew Davis added a comment - - edited There are a few things we could do. This all assumes we can either get the value of max_input_vars or make a reasonable guess as to its value. 1) Overrule the "students-per-page" grader report setting if it would result in a number of fields above max_input_vars. 2) display a warning on the grader report if the total number of fields is above max_input_vars. 3) display a warning on the environment page about max_input_vars. Not sure what we would say exactly as we'd have to kind of guess what value to tell them to set it to. Or we could query the DB to find the course with the most grade items and students. Not sure this is a good idea. 4) prevent the user turning editing mode on (with an explanatory error message) if the number of fields would be above max_input_vars. 4. Change the way grades are submitted to the server to reduce the number of fields that must be submitted. That would be good although some of the above still wants doing as there will be someone out there who will hit max_input_vars anyway. I think my vote goes to paging (#1) plus displaying a warning explaining why if the the number of students per page has been reduced.
          Hide
          Floyd Saner added a comment -

          The preference stated by Andrew Davis (Overrule the "students-per-page" setting) really places an undue burden back on teachers.

          Let's say you end up with 8 pages of students. Here is what happens when you grade an assignment:
          1) You enter grades for page one, then Update.
          2) You end up at the top view of the grade book. You must remember which page you finished grading and click that page number. Although this sounds trivial, it is not easy. Have you ever tried grading a class of a hundred students and then try to remember which grading page you just completed?
          3) You have to scroll across all your grade book columns to find the assignment being graded. This scrolling is also a pain with a large grade book.

          Those steps have to be repeated over and over and become very annoying.

          So, I apologize for not having a good solution, but I appeal for a solution that does not make it difficult for teachers.

          Show
          Floyd Saner added a comment - The preference stated by Andrew Davis (Overrule the "students-per-page" setting) really places an undue burden back on teachers. Let's say you end up with 8 pages of students. Here is what happens when you grade an assignment: 1) You enter grades for page one, then Update. 2) You end up at the top view of the grade book. You must remember which page you finished grading and click that page number. Although this sounds trivial, it is not easy. Have you ever tried grading a class of a hundred students and then try to remember which grading page you just completed? 3) You have to scroll across all your grade book columns to find the assignment being graded. This scrolling is also a pain with a large grade book. Those steps have to be repeated over and over and become very annoying. So, I apologize for not having a good solution, but I appeal for a solution that does not make it difficult for teachers.
          Hide
          Andrew Davis added a comment -

          I hadn't noticed the grader report forgetting what page you're on when you submit grades. I couldn't find an existing issue for that so I've raised MDL-31818.

          Show
          Andrew Davis added a comment - I hadn't noticed the grader report forgetting what page you're on when you submit grades. I couldn't find an existing issue for that so I've raised MDL-31818 .
          Hide
          Floyd Saner added a comment -

          Ah, thanks Andrew. I checked on this and stand corrected. You can just click the 'Next' link at the top of the grade book. The theme I was using did not provide enough contrast between the current page number and the other numbers, so I did not notice page tracking. But 'Previous' and 'Next' work just fine. Sorry for the false alarm - you can remove MDL-31818.

          The issue of excessive scrolling still remains when you have multiple grade book pages. That is an ongoing problem with the grade book interface.

          I was able to get around the max_input_vars issue in one case by collapsing categories that were not being graded. That actually is easier than using multiple pages.

          I am not familiar with the grade book code, but do have programming experience. Here is a suggestion to think about.

          Data entry in the grade book is usually done one assignment at a time, or one student at a time - i.e. you are editing one row or one column. Is it possible to change the 'Edit' mode of the grade book? I believe it is very rare that someone needs to have all the cells open for editing. Perhaps the grade book interface could be coded so only selected rows or columns are opened for editing - click on a row or column label to edit. This will greatly reduce the possibility of hitting the max_input_vars limit of 1000.

          I see several guiding questions for selecting a solution to this problem -
          1. What is the best interface design for users?
          2. How do graders use the grade book?
          3. Is a complete solution required (e.g. no possibility of exceeding max_input_vars), or is it OK to implement a solution that solves the problem in 99% of the cases?
          4. How does a solution to this problem tie in with any plans for a redesigned grade book interface?
          5. How does a solution affect other grade book plugins?
          6. How much coding effort is required to implement a solution?

          Show
          Floyd Saner added a comment - Ah, thanks Andrew. I checked on this and stand corrected. You can just click the 'Next' link at the top of the grade book. The theme I was using did not provide enough contrast between the current page number and the other numbers, so I did not notice page tracking. But 'Previous' and 'Next' work just fine. Sorry for the false alarm - you can remove MDL-31818 . The issue of excessive scrolling still remains when you have multiple grade book pages. That is an ongoing problem with the grade book interface. I was able to get around the max_input_vars issue in one case by collapsing categories that were not being graded. That actually is easier than using multiple pages. I am not familiar with the grade book code, but do have programming experience. Here is a suggestion to think about. Data entry in the grade book is usually done one assignment at a time, or one student at a time - i.e. you are editing one row or one column. Is it possible to change the 'Edit' mode of the grade book? I believe it is very rare that someone needs to have all the cells open for editing. Perhaps the grade book interface could be coded so only selected rows or columns are opened for editing - click on a row or column label to edit. This will greatly reduce the possibility of hitting the max_input_vars limit of 1000. I see several guiding questions for selecting a solution to this problem - 1. What is the best interface design for users? 2. How do graders use the grade book? 3. Is a complete solution required (e.g. no possibility of exceeding max_input_vars), or is it OK to implement a solution that solves the problem in 99% of the cases? 4. How does a solution to this problem tie in with any plans for a redesigned grade book interface? 5. How does a solution affect other grade book plugins? 6. How much coding effort is required to implement a solution?
          Hide
          Gilles-Philippe Leblanc added a comment - - edited

          I would choose the following solution mentioned by Andrew:

          "4. Change the way grades are Submitted to the server to Reduce the number of fields That Must Be Submitted. That Would be good although some of The Above still wants doing as There Will be someone out there Who Will max_input_vars hit anyway."

          I think it's the cleanest solution and one that would minimize the impact on users.
          I do think it's the most complex.

          Another good way should to find a way to raise the max_input_vars value for this php page only. Do someone know how to do this ?

          Show
          Gilles-Philippe Leblanc added a comment - - edited I would choose the following solution mentioned by Andrew: "4. Change the way grades are Submitted to the server to Reduce the number of fields That Must Be Submitted. That Would be good although some of The Above still wants doing as There Will be someone out there Who Will max_input_vars hit anyway." I think it's the cleanest solution and one that would minimize the impact on users. I do think it's the most complex. Another good way should to find a way to raise the max_input_vars value for this php page only. Do someone know how to do this ?
          Hide
          Chris Myers added a comment -

          I don't suppose you could do that by saving the "original" values in hidden attribs on the page, then comparing the current value with the original value on submit, and only actually submitting the updated values? We do stuff like that on a couple of our internal projects.

          Show
          Chris Myers added a comment - I don't suppose you could do that by saving the "original" values in hidden attribs on the page, then comparing the current value with the original value on submit, and only actually submitting the updated values? We do stuff like that on a couple of our internal projects.
          Hide
          Andrew Davis added a comment -

          Chris, only sending changed values isn't possible (as far as I know) if Javascript is disabled. Whatever we do here has to work both with and without Javascript.

          Show
          Andrew Davis added a comment - Chris, only sending changed values isn't possible (as far as I know) if Javascript is disabled. Whatever we do here has to work both with and without Javascript.
          Hide
          Andrew Davis added a comment -

          Altering the gradebook to limit the user to entering grades for either a single student or a single activity at a time is an interesting idea. That seems like it would work in reasonably well with how I believe most teachers work. At a minimum I suspect relatively few teachers sit down with the expectation of grading all activities for all students at once.

          "Is a complete solution required (e.g. no possibility of exceeding max_input_vars), or is it OK to implement a solution that solves the problem in 99% of the cases?"

          We do really need to push for a complete solution. Moodle is used by tens of thousands of institutions so, even if only 1% of them run into a problem, that still results in hundreds of broken gradebooks.

          Show
          Andrew Davis added a comment - Altering the gradebook to limit the user to entering grades for either a single student or a single activity at a time is an interesting idea. That seems like it would work in reasonably well with how I believe most teachers work. At a minimum I suspect relatively few teachers sit down with the expectation of grading all activities for all students at once. "Is a complete solution required (e.g. no possibility of exceeding max_input_vars), or is it OK to implement a solution that solves the problem in 99% of the cases?" We do really need to push for a complete solution. Moodle is used by tens of thousands of institutions so, even if only 1% of them run into a problem, that still results in hundreds of broken gradebooks.
          Hide
          Andrew Davis added a comment -

          Here is the beginnings of a potential solution. Not even close to being ready for production (literal string in the debugging() call, not properly tested) but I thought Id put it up for feedback.

          It attempts to calculate the number of fields that will be output then compares that with PHP max_input_var setting. If necessary it turns down the number of students per page and outputs a debugging message.

          Although we will have to do some work to reduce the number of fields that are required we will still need some sort of safety net like this for the 1% who still run into trouble.

          Show
          Andrew Davis added a comment - Here is the beginnings of a potential solution. Not even close to being ready for production (literal string in the debugging() call, not properly tested) but I thought Id put it up for feedback. It attempts to calculate the number of fields that will be output then compares that with PHP max_input_var setting. If necessary it turns down the number of students per page and outputs a debugging message. Although we will have to do some work to reduce the number of fields that are required we will still need some sort of safety net like this for the 1% who still run into trouble.
          Hide
          Floyd Saner added a comment -

          @Andrew I support your idea of automatically decreasing the number of students per page as a safety net. That is definitely needed.

          I still support an additional general solution that accommodates the way most teachers use the grade book - entering grades for one assignment or one student, and doing so without having to click through multiple pages. I believe you allude to this in your last sentence where you state, "...we will have to do some work to reduce the number of fields that are required..."

          For a 15 week course it is not unusual to have 2-3 graded activities per week, resulting in 30-45 total total graded activities for each student. With a class of 35 students, this can result in over 1,500 grade book fields. So it's easy to exceed the default 1,000 max_input_vars.

          Thanks for moving forward on this.

          Show
          Floyd Saner added a comment - @Andrew I support your idea of automatically decreasing the number of students per page as a safety net. That is definitely needed. I still support an additional general solution that accommodates the way most teachers use the grade book - entering grades for one assignment or one student, and doing so without having to click through multiple pages. I believe you allude to this in your last sentence where you state, "...we will have to do some work to reduce the number of fields that are required..." For a 15 week course it is not unusual to have 2-3 graded activities per week, resulting in 30-45 total total graded activities for each student. With a class of 35 students, this can result in over 1,500 grade book fields. So it's easy to exceed the default 1,000 max_input_vars. Thanks for moving forward on this.
          Hide
          Andrew Davis added a comment - - edited

          I have uploaded a safety check to github for the number of fields that are being used.
          It checks the PHP max_input_vars setting, calculates how many fields will be used and turns down the number of students per page if necessary. The algorithm its uses will require tweaking as we alter the grader report to use less fields.

          Currently we use up to 4 fields for each grade item.
          1) the visible grade field
          2) a hidden field containing the unchanged grade
          3) the visible feedback field
          4) a hidden field containing the unchanged grade

          The hidden fields are used in two places.
          1) by client side Javascript when editing is OFF.
          2) By the server when the form is submitted to determine what fields need updating. This happens in /grade/report/grader/lib.php around line 195
          $oldvalue = $data->

          {'old'.$varname}

          ;

          I propose two changes. The following is quite involved...

          1) The ajax/javascript be enhanced to avoid the user bulk submitting grades. Using Javascript we can hide the "update" button and save the grade/feedback each time the user moves to another field. It would perform very similarly with editing on as to how it currently performs with editing off except that the input fields would be visible all the time. If Javascript is not available in the browser the Update button would not be hidden and it would fall back to the non-ajax behaviour (ie bulk updating grades).

          2) We don't output the hidden fields at all. Instead, for non-ajax, the server retrieves the old grades from the database. This means additional server load but halves the number of fields we're using and relying on data from the client in this way is probably bad anyway.

          For ajax we'll need to move the old grades and feedback out of hidden inputs and into a Javascript array or something similar. At first glance you might think we could leave the ajax version as is, using an unlimited number of form fields but only submitting changed grades one at a time to avoid bulk submission, however should ajax be turned on on the server but Javascript be unavailable on the client it will fall back to bulk submission and we're back to violating PHP's max_input_vars.

          Combined the above will cut the number of fields we're using in half and probably made the grader report a bit faster to use in the process.

          Show
          Andrew Davis added a comment - - edited I have uploaded a safety check to github for the number of fields that are being used. It checks the PHP max_input_vars setting, calculates how many fields will be used and turns down the number of students per page if necessary. The algorithm its uses will require tweaking as we alter the grader report to use less fields. Currently we use up to 4 fields for each grade item. 1) the visible grade field 2) a hidden field containing the unchanged grade 3) the visible feedback field 4) a hidden field containing the unchanged grade The hidden fields are used in two places. 1) by client side Javascript when editing is OFF. 2) By the server when the form is submitted to determine what fields need updating. This happens in /grade/report/grader/lib.php around line 195 $oldvalue = $data-> {'old'.$varname} ; I propose two changes. The following is quite involved... 1) The ajax/javascript be enhanced to avoid the user bulk submitting grades. Using Javascript we can hide the "update" button and save the grade/feedback each time the user moves to another field. It would perform very similarly with editing on as to how it currently performs with editing off except that the input fields would be visible all the time. If Javascript is not available in the browser the Update button would not be hidden and it would fall back to the non-ajax behaviour (ie bulk updating grades). 2) We don't output the hidden fields at all. Instead, for non-ajax, the server retrieves the old grades from the database. This means additional server load but halves the number of fields we're using and relying on data from the client in this way is probably bad anyway. For ajax we'll need to move the old grades and feedback out of hidden inputs and into a Javascript array or something similar. At first glance you might think we could leave the ajax version as is, using an unlimited number of form fields but only submitting changed grades one at a time to avoid bulk submission, however should ajax be turned on on the server but Javascript be unavailable on the client it will fall back to bulk submission and we're back to violating PHP's max_input_vars. Combined the above will cut the number of fields we're using in half and probably made the grader report a bit faster to use in the process.
          Hide
          Matthew Putz added a comment -

          FYI - this also affects 2.2

          Show
          Matthew Putz added a comment - FYI - this also affects 2.2
          Hide
          Andrew Davis added a comment -

          Adding testing instructions.

          Show
          Andrew Davis added a comment - Adding testing instructions.
          Hide
          Andrew Davis added a comment -

          I'm putting some changes up for peer review. This halves the number of fields used. I've also fixed up a bunch of Javascript. Its a pretty significant change so will require a good deal of testing. I'm going to work on the surrounding gradebook issues over the next week.

          Show
          Andrew Davis added a comment - I'm putting some changes up for peer review. This halves the number of fields used. I've also fixed up a bunch of Javascript. Its a pretty significant change so will require a good deal of testing. I'm going to work on the surrounding gradebook issues over the next week.
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          Not being very familiar wit the gradebook myself I think I followed your solution and it seems sensible.

          I noticed that the get_pref('studentsperpage') was still being used in index.php, don't know if that is significant

          Is there a significant DB impact by retrieving these grades from the DB rather than client side?

          Show
          Dan Poltawski added a comment - Hi Andrew, Not being very familiar wit the gradebook myself I think I followed your solution and it seems sensible. I noticed that the get_pref('studentsperpage') was still being used in index.php, don't know if that is significant Is there a significant DB impact by retrieving these grades from the DB rather than client side?
          Hide
          Andrew Davis added a comment -

          Just did some more testing and may have found a problem. I'll have to come back to fix that and to answer your questions Dan. Hopefully I'll get to it tomorrow...

          Show
          Andrew Davis added a comment - Just did some more testing and may have found a problem. I'll have to come back to fix that and to answer your questions Dan. Hopefully I'll get to it tomorrow...
          Hide
          Andrew Davis added a comment -

          Dan, Ive fixed up that called to get_pref(). Well spotted.

          There shouldn't be any DB impact. The previous code was getting all the new and old grades from the client, doing writes then loading all the grades from the database to display the following page. Now we're just loading the grades a little earlier so we can use them for detecting changed grades as well as building the page.

          Show
          Andrew Davis added a comment - Dan, Ive fixed up that called to get_pref(). Well spotted. There shouldn't be any DB impact. The previous code was getting all the new and old grades from the client, doing writes then loading all the grades from the database to display the following page. Now we're just loading the grades a little earlier so we can use them for detecting changed grades as well as building the page.
          Hide
          Andrew Davis added a comment -

          I think I might be done. I've squished the changes down to 3 commits. I'm going to bash on it a little longer then put it up for integration.

          Show
          Andrew Davis added a comment - I think I might be done. I've squished the changes down to 3 commits. I'm going to bash on it a little longer then put it up for integration.
          Hide
          Andrew Davis added a comment -

          I've done some more testing and fixed some more little issues I spotted.

          Dan, I have revise my previous answer to your question about DB impact. Now, if the user has overridden a grade the grades will be loaded, the updating done, then the grades will be reloaded. Its only a single query. If the user hasn't changing anything or is only changing feedback then reloading will not be done.

          I ran into trouble because grade items like the course total are dependent on other grade items. Changes to one potentially ripple through other grade items. Not going back to the database after grade alterations was proving error prone.

          Show
          Andrew Davis added a comment - I've done some more testing and fixed some more little issues I spotted. Dan, I have revise my previous answer to your question about DB impact. Now, if the user has overridden a grade the grades will be loaded, the updating done, then the grades will be reloaded. Its only a single query. If the user hasn't changing anything or is only changing feedback then reloading will not be done. I ran into trouble because grade items like the course total are dependent on other grade items. Changes to one potentially ripple through other grade items. Not going back to the database after grade alterations was proving error prone.
          Hide
          Andrew Davis added a comment -

          There are lots of comments above so for anyone seeing this issue for the first time now the description of the solution is at https://tracker.moodle.org/browse/MDL-26275?focusedCommentId=150091&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-150091

          Show
          Andrew Davis added a comment - There are lots of comments above so for anyone seeing this issue for the first time now the description of the solution is at https://tracker.moodle.org/browse/MDL-26275?focusedCommentId=150091&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-150091
          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
          Andrew Davis added a comment -

          Rebased. Also, because of the volume of code changes to a very important and notoriously hard to comprehensively test part of Moodle I've put out a call for additional testing at http://moodle.org/mod/forum/discuss.php?d=200943

          Show
          Andrew Davis added a comment - Rebased. Also, because of the volume of code changes to a very important and notoriously hard to comprehensively test part of Moodle I've put out a call for additional testing at http://moodle.org/mod/forum/discuss.php?d=200943
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          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
          Andrew Davis added a comment -

          rebased

          Show
          Andrew Davis added a comment - rebased
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew, ping me about this one when you come online please.
          It looks very close, just a couple of things that you could probably clean up real quickly that I'd like to discuss with you.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, ping me about this one when you come online please. It looks very close, just a couple of things that you could probably clean up real quickly that I'd like to discuss with you. Cheers Sam
          Hide
          Andrew Davis added a comment -

          I have spoken to Sam and I believe I have fixed up the troublesome spots.

          Show
          Andrew Davis added a comment - I have spoken to Sam and I believe I have fixed up the troublesome spots.
          Hide
          Sam Hemelryk added a comment -

          Awesome thanks Andrew, this has been integrated now.
          Thanks for cleaning up those comparisons!

          Show
          Sam Hemelryk added a comment - Awesome thanks Andrew, this has been integrated now. Thanks for cleaning up those comparisons!
          Hide
          Rossiani Wijaya added a comment -

          This is working great.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working great. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -
          UPDATE tracker_issues
             SET status = 'Closed',
                comment = 'Thanks!'
          WHEN participants = 'Did a gorgeous work'
          

          This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

          Show
          Eloy Lafuente (stronk7) added a comment - UPDATE tracker_issues SET status = 'Closed', comment = 'Thanks!' WHEN participants = 'Did a gorgeous work' This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

            People

            • Votes:
              15 Vote for this issue
              Watchers:
              17 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: