-
Bug
-
Resolution: Fixed
-
Minor
-
4.0.6, 4.1, 4.1.1
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:
- Requesting the read only version of pages should never cause document conversions
- 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:
- Student uploads a submission
- Teacher annotates it
- Student can view this annotated version by clicking the "View annotated PDF" link which has now become available from the assignment
- Student edits their submission, uploading a new one
- The "View annotated PDF" link should continue to show the old annotated PDF
- The teacher grades the assignment again
- The teacher should see the new unannotated PDF
- The teacher annotates and saves
- 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:
- A student has updated their submission
- 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:
- We want the readonly pages
- The readonly pages do not exist for the new attempt
- The page numbers do not match
- The code to generate images runs
- 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:
- 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)
- Create a course, create an assignment with enabled pdf annotation feedback, enrol a student.
- As a student submit a doc/odt file that has N pages.
- Run the CLI command to convert the submission:
php admin/cli/adhoc_task.php --execute
- Confirm, that The document has been successfully converted is printed.
- 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)
- As a student, edit the submission and submit another doc/odt file that has M pages where N <> M
- Run the CLI command to convert the submission:
php admin/cli/adhoc_task.php --execute
- 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()
- 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)
- This shows that readonly pages not being re-generated when a submission file is replaced.
- has a non-specific relationship to
-
MDL-45580 PDF Annotations Remain After PDF is Replaced
- Closed
-
MDL-62700 Convert submissions task encountering numerous instances of error: Could not find readonly pages for grade
- Closed
- has been marked as being related by
-
MDL-76659 Assign grader doesn't correctly detect changes
- Closed
- is a regression caused by
-
MDL-66626 Assignfeedback_editpdf sending infinite request when page ready is not equal to page number of combined pdf
- Closed