Moodle
  1. Moodle
  2. MDL-36289

Save and Show Next, Next and Previous buttons in Assignment Grading copies files to next student

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.2, 2.4
    • Fix Version/s: 2.3.4
    • Component/s: Assignment
    • Labels:
    • Testing Instructions:
      Hide
      1. Create an assignment with in a course with multiple students that has "Feedback files" set to "Yes"
      2. Go to the grading table for the assignment (View/grade all submissions)
      3. Click the grade button for the first submission in the table
      4. Upload a file to the "Feedback files" area of the grading form
      5. Click the "Save and next" button at the bottom of the form
      6. Check that the grading form now contains the information for the second student - but the files in the "Feedback files" area defaults to empty.
      7. Click the "Previous" button at the bottom of the grading form
      8. Check that the grading form now contains the information for the first student - but the files in the "Feedback files" area defaults the file that was previously uploaded.
      Show
      Create an assignment with in a course with multiple students that has "Feedback files" set to "Yes" Go to the grading table for the assignment (View/grade all submissions) Click the grade button for the first submission in the table Upload a file to the "Feedback files" area of the grading form Click the "Save and next" button at the bottom of the form Check that the grading form now contains the information for the second student - but the files in the "Feedback files" area defaults to empty. Click the "Previous" button at the bottom of the grading form Check that the grading form now contains the information for the first student - but the files in the "Feedback files" area defaults the file that was previously uploaded.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      45077

      Description

      When using the assignment grading "Save and Next" (or just Next) - the files entered in a the file manager are carried forward to the next student.

        Issue Links

          Activity

          Hide
          Damyon Wiese added a comment -

          In trying to solve this issue - the problems is caused by a combination of things in the forms/file api.

          1. file_save_draft_area_files does not purge the draft file area (deliberately - there is a comment about that)
          2. file_get_submitted_draft_itemid does not take into account the current form identifier (which would solve this issue - but would be a big api change)

          Description of the bug:

          1. So - student1 grading form is submitted (draft id 5)
          2. Next page - grades are saved for student1 and a form is created for student2
          3. The form for student2 re-uses draft id 5 because it file_get_submitted_draft_itemid just looks for a _REQUEST variable and finds the one from student1.

          The smallest change I can think of (which is ugly) would be to change the name of the filemanager element for each student.

          A cleaner (but not backwards compatible) change would be to require an mform in file_get_submitted_draft_itemid and re-use the logic in _process_submission from moodleform.

          Show
          Damyon Wiese added a comment - In trying to solve this issue - the problems is caused by a combination of things in the forms/file api. 1. file_save_draft_area_files does not purge the draft file area (deliberately - there is a comment about that) 2. file_get_submitted_draft_itemid does not take into account the current form identifier (which would solve this issue - but would be a big api change) Description of the bug: So - student1 grading form is submitted (draft id 5) Next page - grades are saved for student1 and a form is created for student2 The form for student2 re-uses draft id 5 because it file_get_submitted_draft_itemid just looks for a _REQUEST variable and finds the one from student1. The smallest change I can think of (which is ugly) would be to change the name of the filemanager element for each student. A cleaner (but not backwards compatible) change would be to require an mform in file_get_submitted_draft_itemid and re-use the logic in _process_submission from moodleform.
          Hide
          Damyon Wiese added a comment -

          I'll post a patch which just does the small change described above.

          Show
          Damyon Wiese added a comment - I'll post a patch which just does the small change described above.
          Hide
          Damyon Wiese added a comment -

          I'll remove some comments above to prevent confusion.

          The attached patch changes the name of the filemanager element for each gradingform which gets around the issue for now. I tried clearing the user draft areas between the grading forms - but then you get no files when you should have files.

          Show
          Damyon Wiese added a comment - I'll remove some comments above to prevent confusion. The attached patch changes the name of the filemanager element for each gradingform which gets around the issue for now. I tried clearing the user draft areas between the grading forms - but then you get no files when you should have files.
          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
          Tim Hunt added a comment -

          I think the cleanest way to handle this would be, when you click Save and Show Next, Next or Previous, that should POST to whatever script handles this form, which should then do a redirect (GET) to display the data for the next student. That would remove any chance of data from the previous student getting tangled up with the next student. It also avoids problems with the teacher clicking reload on the grading page.

          The cost is one extra HTTP request.

          Anyway, that would be the approach I recommend. The patch is likely to be smaller.

          (By the way, Moodle style is "Save and show next" - only capital letter at the start.)

          Show
          Tim Hunt added a comment - I think the cleanest way to handle this would be, when you click Save and Show Next, Next or Previous, that should POST to whatever script handles this form, which should then do a redirect (GET) to display the data for the next student. That would remove any chance of data from the previous student getting tangled up with the next student. It also avoids problems with the teacher clicking reload on the grading page. The cost is one extra HTTP request. Anyway, that would be the approach I recommend. The patch is likely to be smaller. (By the way, Moodle style is "Save and show next" - only capital letter at the start.)
          Hide
          Dan Poltawski added a comment -

          Hi Damyon,

          Tim's suggested approach seems sensible to me, what do you think?

          Show
          Dan Poltawski added a comment - Hi Damyon, Tim's suggested approach seems sensible to me, what do you think?
          Hide
          Dan Poltawski added a comment -

          Reopening this as I think Tim's suggestion makes sense.

          Show
          Dan Poltawski added a comment - Reopening this as I think Tim's suggestion makes sense.
          Hide
          Damyon Wiese added a comment -

          Hi Dan,

          This will not work for this case because the save and show next pages need to remember the order of the users from the grading table that was displayed when the page was opened. This is (potentially) too much data to pass on the query string via a redirect. The reason that the list needs to be remembered is so that if a teacher opens the grading page for the first student, and keeps grading and clicking "save and show next" until there are no more students, they should expect to have graded every student - without remembering the original order of the list of students this is not possible (the act of grading a student may change the order of the list).

          Show
          Damyon Wiese added a comment - Hi Dan, This will not work for this case because the save and show next pages need to remember the order of the users from the grading table that was displayed when the page was opened. This is (potentially) too much data to pass on the query string via a redirect. The reason that the list needs to be remembered is so that if a teacher opens the grading page for the first student, and keeps grading and clicking "save and show next" until there are no more students, they should expect to have graded every student - without remembering the original order of the list of students this is not possible (the act of grading a student may change the order of the list).
          Hide
          Damyon Wiese added a comment -

          Also - the state could be remembered with MUC but that would not be possible to backport to 2.3.

          Show
          Damyon Wiese added a comment - Also - the state could be remembered with MUC but that would not be possible to backport to 2.3.
          Hide
          Damyon Wiese added a comment -

          Queried this with Dan and this is the current best solution - so re-submitting it.

          Show
          Damyon Wiese added a comment - Queried this with Dan and this is the current best solution - so re-submitting it.
          Hide
          Dan Poltawski added a comment -

          Thanks for clarifying Damyon, i've integrated this now (23 and master).

          Show
          Dan Poltawski added a comment - Thanks for clarifying Damyon, i've integrated this now (23 and master).
          Hide
          Mark Nelson added a comment -

          Works as expected. Well done all, you deserve a beer.

          Show
          Mark Nelson added a comment - Works as expected. Well done all, you deserve a beer.
          Hide
          Dan Poltawski added a comment -

          Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

          ciao
          Dan

          Show
          Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan
          Hide
          Gilles-Philippe Leblanc added a comment -

          When we fix MDL-35427, this problem appeared.

          Show
          Gilles-Philippe Leblanc added a comment - When we fix MDL-35427 , this problem appeared.

            People

            • Votes:
              24 Vote for this issue
              Watchers:
              20 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: