Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-75898

Assignment readonly pages can be requested when they do not exist

XMLWordPrintable

    • MOODLE_400_STABLE, MOODLE_401_STABLE
    • MOODLE_400_STABLE, MOODLE_401_STABLE, MOODLE_402_STABLE
    • MDL-75898-master
    • Hide

      NB:

      • As part of these tests we will be triggering the convert_submission adhoc task. This task calls the document conversion API to convert submissions to PDFs. However we will be submitting PDFs (which don't need to be converted) so a document converter is not required. The task will simply finish instantly and say the conversion is complete.
      • Make sure the "pathtogs" config setting is set.

      Test async conversions

      master / 4.2 / 4.1

      1. Create a course with an assignment and enrol a student
      2. As the student, submit the file "Submission 1.pdf" attached to this issue
      3. Run:

        php admin/cli/adhoc_task.php --execute
        

      4. Verify you see "The document has been successfully converted" and no errors
      5. As the student edit the submission, deleting the old PDF and replacing it with "Submission 2.pdf" attached to this issue
      6. Run:

        php admin/cli/adhoc_task.php --execute
        

        1. Verify you see "The document has been successfully converted" and no errors

      4.0

      1. Create a course with an assignment and enrol a student
      2. As the student, submit the file "Submission 1.pdf" attached to this issue
      3. Run:

        php admin/cli/scheudled_task.php --execute="\assignfeedback_editpdf\task\convert_submissions"
        

        1. Verify there is no error about not being able to find readonly pages:

          Conversion failed with error:Could not find readonly pages for grade 1
          

      4. As the student edit the submission, deleting the old PDF and replacing it with "Submission 2.pdf" attached to this issue
      5. Run:

        php admin/cli/adhoc_task.php --execute
        

        1. Verify there is no error about not being able to find readonly pages:

          Conversion failed with error:Could not find readonly pages for grade 1
          

      Regression Testing

      1. Repeat the testing instructions from MDL-45580
      2. Repeat the testing instructions from MDL-66626

      master / 4.2 / 4.1 only (not 4.0)

        • Except when it says to run:

          php admin/tool/task/cli/schedule_task.php --execute='\assignfeedback_editpdf\task\convert_submissions'
          

        • Instead run:

          php admin/cli/adhoc_task.php --execute
          

      Show
      NB : As part of these tests we will be triggering the convert_submission adhoc task. This task calls the document conversion API to convert submissions to PDFs. However we will be submitting PDFs (which don't need to be converted) so a document converter is not required. The task will simply finish instantly and say the conversion is complete. Make sure the " pathtogs " config setting is set. Test async conversions master / 4.2 / 4.1 Create a course with an assignment and enrol a student As the student, submit the file "Submission 1.pdf" attached to this issue Run: php admin/cli/adhoc_task.php --execute Verify you see "The document has been successfully converted" and no errors As the student edit the submission, deleting the old PDF and replacing it with "Submission 2.pdf" attached to this issue Run: php admin/cli/adhoc_task.php --execute Verify you see "The document has been successfully converted" and no errors 4.0 Create a course with an assignment and enrol a student As the student, submit the file "Submission 1.pdf" attached to this issue Run: php admin/cli/scheudled_task.php --execute="\assignfeedback_editpdf\task\convert_submissions" Verify there is no error about not being able to find readonly pages: Conversion failed with error:Could not find readonly pages for grade 1 As the student edit the submission, deleting the old PDF and replacing it with "Submission 2.pdf" attached to this issue Run: php admin/cli/adhoc_task.php --execute Verify there is no error about not being able to find readonly pages: Conversion failed with error:Could not find readonly pages for grade 1 Regression Testing Repeat the testing instructions from MDL-45580 Repeat the testing instructions from MDL-66626 master / 4.2 / 4.1 only (not 4.0) Except when it says to run: php admin/tool/task/cli/schedule_task.php --execute='\assignfeedback_editpdf\task\convert_submissions' Instead run: php admin/cli/adhoc_task.php --execute

      The problems observed here mostly appear to be a result of an incomplete understanding of how the readonlypages filearea is intended to be used. MDL-66626 introduced a situation where if the number of readonlypages vs the number of pages don't match, then all pages get regenerated. However it is sometimes valid for the number of pages in each area to not match.

      Background

      Read only pages
      The concept of readonly pages was introduced in MDL-45580 and their purpose is to provide a set of pages for the student to access after the PDF has been annotated. This implies a few things:

      1. Requesting the read only version of pages should never cause document conversions
      2. The read only version of pages should only be accessible via the UI in situations after the teacher has annotated the PDF

      The intended flow for read only pages is as follows:

      1. Student uploads a submission
      2. Teacher annotates it
      3. Student can view this annotated version by clicking the "View annotated PDF" link which has now become available from the assignment
      4. Student edits their submission, uploading a new one
      5. The "View annotated PDF" link should continue to show the old annotated PDF
      6. The teacher grades the assignment again
      7. The teacher should see the new unannotated PDF
      8. The teacher annotates and saves
      9. The student should now see the new annotated PDF after pressing the "View annotated PDF" link

      In the steps above, once the teacher has completed their annotations and saved, that's when the PDF gets copied to the readonly area, and that is the PDF the student sees.

      Conversion polling
      The assignment grader (which is also re-used in a trimmed down form as part of the "View annotated PDF" link) polls document conversions; and tries to figure out when the conversion is complete. It does this a little naively by comparing the number of pages that are converted vs the number of pages currently being displayed in the document.

      When the readonly version is displayed to a student, we can end up in a situation where these numbers will never match (e.g., the readonly version has 1 page, and the updated version has 2). The readonly version used to still get displayed, but in the background the grader was sending constant requests to poll for more pages. In this situation it is valid for the number of pages in the readonly area to not match the number of pages in the pages area.

      Image pages
      Conversion from PDF to images (each page in the assign grader is actually an image generated from the original PDF), usually happens via cron, and if this process is interrupted, it can lead to a situation where the number of pages in the PDF do not match the number of images generated. In this situation we need to regenerate the images, and that's what MDL-66626 tried to do. However another situation where the counts won't match, is the one mentioned above - when the student updates the assignment. In this case we do not want to do anything. And this is where the main bug crept in.

      The bug

      The original code (pre MDL-66626) had a block like this:

      \assignfeedback_editpdf\document_services::get_page_images_for_attempt

              if (empty($pages) {
                  if ($readonly) {
                      // This should never happen, there should be a version of the pages available
                      // whenever we are requesting the readonly version.
                      throw new \moodle_exception('Could not find readonly pages for grade ' . $grade->id);
                  }
                  $pages = self::generate_page_images_for_attempt($assignment, $userid, $attemptnumber, $resetrotation);
              }
      

      MDL-66626 changed it to if (empty($pages) || count($pages) != $totalpagesforattempt) { which allows the block to execute in the situation where:

      1. A student has updated their submission
      2. The previous version of the submission was annotated

      In this situation the student has a link to "View annotated PDF" (which requests the readonly pages) and since their new submission is different, this causes a page number mistmatch between the old annotated PDF and the new submission (this is the situation outlined above in the Read only pages section), so the get_page_images_for_attempt code runs and the following is true:

      1. We want the readonly pages
      2. The readonly pages do not exist for the new attempt
      3. The page numbers do not match
      4. The code to generate images runs
      5. The exception is thrown as the readonly pages do not exist

      This also causes exceptions to be thrown as part of document conversions running on cron (which is what this issue was initially about).

      Original report below

      This seems to be closely related to MDL-68943.

      Steps to replicate the issue:

      1. You'd need a polling converted to replicate this (eg https://github.com/catalyst/moodle-fileconverter_librelambda or https://github.com/catalyst/moodle-fileconverter_dummy)
      2. Create a course, create an assignment with enabled pdf annotation feedback, enrol a student.
      3. As a student submit a doc/odt file that has N pages.
      4. Run the CLI command to convert the submission:

        php admin/cli/adhoc_task.php --execute
        

      5. Confirm, that The document has been successfully converted is printed.
      6. Run the SQL and confirm that the number of readonly pages and normal pages match:

        moodle=# select id, contenthash, filearea, filename from mdl_files where contextid = 39 and component = 'assignfeedback_editpdf' and itemid = 3 and filearea in ('readonlypages', 'pages') and filename <> '.' order by filearea, filename;
         id  |               contenthash                |   filearea    |    filename     
        -----+------------------------------------------+---------------+-----------------
         310 | e973fbbbf0e04ecaceb6355e116bfe9cb159b7fb | pages         | image_page0.png
         312 | a395553ba39792a40d463e1454ec40eadb442340 | pages         | image_page1.png
         313 | 8400018de64f8d6ee6c4e467a1551707fff4b25d | pages         | image_page2.png
         314 | e51e1f2d4dd79cc676855b7f57ee541ac7b7045f | pages         | image_page3.png
         315 | 35eded0ac025a786343d1713b19439155fe3828e | pages         | image_page4.png
         317 | e973fbbbf0e04ecaceb6355e116bfe9cb159b7fb | readonlypages | image_page0.png
         318 | a395553ba39792a40d463e1454ec40eadb442340 | readonlypages | image_page1.png
         319 | 8400018de64f8d6ee6c4e467a1551707fff4b25d | readonlypages | image_page2.png
         320 | e51e1f2d4dd79cc676855b7f57ee541ac7b7045f | readonlypages | image_page3.png
         321 | 35eded0ac025a786343d1713b19439155fe3828e | readonlypages | image_page4.png
        (10 rows)
        

      7. As a student, edit the submission and submit another doc/odt file that has M pages where N <> M
      8. Run the CLI command to convert the submission:

        php admin/cli/adhoc_task.php --execute
        

      9. Confirm, that the task fails:

        178cb2962225:95 2022-10-05 11:20:56 Adhoc task failed: assignfeedback_editpdf\task\convert_submission,error/Could not find readonly pages for grade 3
        178cb2962225:95 2022-10-05 11:20:56 Backtrace:
        178cb2962225:95 2022-10-05 11:20:56 * line 105 of /mod/assign/feedback/editpdf/classes/task/convert_submission.php: call to assignfeedback_editpdf\document_services::get_page_images_for_attempt()
        178cb2962225:95 2022-10-05 11:20:56 * line 359 of /lib/cronlib.php: call to assignfeedback_editpdf\task\convert_submission->execute()
        178cb2962225:95 2022-10-05 11:20:56 * line 198 of /lib/cronlib.php: call to cron_run_inner_adhoc_task()
        178cb2962225:95 2022-10-05 11:20:56 * line 131 of /admin/cli/adhoc_task.php: call to cron_run_adhoc_tasks()
        

      10. Run the SQL and confirm that the number of readonly pages and normal pages do not match anymore:

        moodle=# select id, contenthash, filearea, filename from mdl_files where contextid = 39 and component = 'assignfeedback_editpdf' and itemid = 3 and filearea in ('readonlypages', 'pages') and filename <> '.' order by filearea, filename;
         id  |               contenthash                |   filearea    |    filename     
        -----+------------------------------------------+---------------+-----------------
         334 | e973fbbbf0e04ecaceb6355e116bfe9cb159b7fb | pages         | image_page0.png
         336 | 29dce51fdc4a18545cbe41e9e69c3f401a6352ca | pages         | image_page1.png
         337 | ffe1bedca8f892a28b413092f2327f44702e4530 | pages         | image_page2.png
         317 | e973fbbbf0e04ecaceb6355e116bfe9cb159b7fb | readonlypages | image_page0.png
         318 | a395553ba39792a40d463e1454ec40eadb442340 | readonlypages | image_page1.png
         319 | 8400018de64f8d6ee6c4e467a1551707fff4b25d | readonlypages | image_page2.png
         320 | e51e1f2d4dd79cc676855b7f57ee541ac7b7045f | readonlypages | image_page3.png
         321 | 35eded0ac025a786343d1713b19439155fe3828e | readonlypages | image_page4.png
        (8 rows)
        

      11. This shows that readonly pages not being re-generated when a submission file is replaced.

        1. (I) Passed -- (400)MDL-75898.png
          (I) Passed -- (400)MDL-75898.png
          221 kB
        2. (I) Passed -- (401)MDL-75898.png
          (I) Passed -- (401)MDL-75898.png
          215 kB
        3. (I) Passed -- (Master)MDL-75898.png
          (I) Passed -- (Master)MDL-75898.png
          198 kB
        4. (II) Passed -- (400)MDL-75898.png
          (II) Passed -- (400)MDL-75898.png
          126 kB
        5. (II) Passed -- (401)MDL-75898.png
          (II) Passed -- (401)MDL-75898.png
          320 kB
        6. (II) Passed -- (Master)MDL-75898.png
          (II) Passed -- (Master)MDL-75898.png
          136 kB
        7. (III) Passed -- (400)MDL-75898.png
          (III) Passed -- (400)MDL-75898.png
          69 kB
        8. (III) Passed -- (401)MDL-75898.png
          (III) Passed -- (401)MDL-75898.png
          248 kB
        9. (III) Passed -- (Master)MDL-75898.png
          (III) Passed -- (Master)MDL-75898.png
          94 kB
        10. (IV) Passed -- (400)MDL-75898.png
          (IV) Passed -- (400)MDL-75898.png
          48 kB
        11. (IV) Passed -- (401)MDL-75898.png
          (IV) Passed -- (401)MDL-75898.png
          38 kB
        12. (IV) Passed -- (Master)MDL-75898.png
          (IV) Passed -- (Master)MDL-75898.png
          37 kB
        13. 75898-401-1.png
          75898-401-1.png
          35 kB
        14. 75898-401-2.png
          75898-401-2.png
          35 kB
        15. 75898-402-1.png
          75898-402-1.png
          34 kB
        16. 75898-402-2.png
          75898-402-2.png
          34 kB
        17. 75898-master-1.png
          75898-master-1.png
          42 kB
        18. 75898-master-2.png
          75898-master-2.png
          35 kB
        19. image-2023-05-01-20-59-03-970.png
          image-2023-05-01-20-59-03-970.png
          116 kB
        20. MDL-45580-backuprestore.png
          MDL-45580-backuprestore.png
          64 kB
        21. MDL-45580-step17.png
          MDL-45580-step17.png
          20 kB
        22. MDL-45580-step3a.png
          MDL-45580-step3a.png
          31 kB
        23. MDL-45580-step3b.png
          MDL-45580-step3b.png
          20 kB
        24. MDL-66626-40-cli.png
          MDL-66626-40-cli.png
          50 kB
        25. MDL-66626-40-db.png
          MDL-66626-40-db.png
          36 kB
        26. MDL-66626-step13.png
          MDL-66626-step13.png
          32 kB
        27. MDL-66626-step15.png
          MDL-66626-step15.png
          36 kB
        28. MDL-66626-step20.png
          MDL-66626-step20.png
          81 kB
        29. Submission 1.pdf
          8 kB
        30. Submission 2.pdf
          9 kB

            matthewhilton Matthew Hilton
            mikhailgolenkov Misha Golenkov
            Dmitrii Metelkin Dmitrii Metelkin
            Jun Pataleta Jun Pataleta
            Mikel Martín Corrales Mikel Martín Corrales
            Votes:
            14 Vote for this issue
            Watchers:
            31 Start watching this issue

              Created:
              Updated:
              Resolved:

                Estimated:
                Original Estimate - 0 minutes
                0m
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 1 day, 1 hour, 30 minutes
                1d 1h 30m

                  Error rendering 'clockify-timesheets-time-tracking-reports:timer-sidebar'. Please contact your Jira administrators.