Skip to content

Commit

Permalink
MDL-75898 assignfeedback_editpdf: Improve page count mismatch logic
Browse files Browse the repository at this point in the history 
MDL-45580 introduced the readonlypages filearea, and when loading
page images for an attempt, the code would check if the pages existed
, creating them if not. The code inside this block also contained
a guard clause for the case where no readonly pages existed - which
is a situation that should not happen. Whenever readonly pages are
requested, they should exist.

MDL-66626 introduced a situation where page counts not matching would
also retrigger page generation. However this led to a situation where
the guard clause could be entered when requesting readonly pages.

This patch refactors the guard clause, and improves the logic to
regenerate pages.
  • Loading branch information
@cameron1729 @junpataleta
cameron1729 authored and junpataleta committed Jun 8, 2023
1 parent 3fa4195 commit 2fbb766
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 7 deletions.
26 changes: 19 additions & 7 deletions mod/assign/feedback/editpdf/classes/document_services.php
View file
Expand Up @@ -580,14 +580,26 @@ public static function get_page_images_for_attempt($assignment, $userid, $attemp
}
}

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

// There are two situations where the number of page images generated does not
// match the number of pages in the PDF:
//
// 1. The document conversion adhoc task was interrupted somehow (node died, solar flare, etc)
// 2. The submission has been updated by the student
//
// In the case of 1. we need to regenerate the pages, see MDL-66626.
// In the case of 2. we should do nothing, see MDL-45580.
//
// To differentiate between 1. and 2. we can check if the submission has been modified since the
// pages were generated. If it has, then we're in situation 2.
$totalpagesforattempt = self::page_number_for_attempt($assignment, $userid, $attemptnumber, false);
// Here we are comparing the total number of images against the total number of pages from the combined PDF.
if (empty($pages) || count($pages) != $totalpagesforattempt) {
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);
}
$submissionmodified = isset($pagemodified) && $submission->timemodified > $pagemodified;
if (empty($pages) || (count($pages) != $totalpagesforattempt && !$submissionmodified)) {
$pages = self::generate_page_images_for_attempt($assignment, $userid, $attemptnumber, $resetrotation);
}

Expand Down
61 changes: 61 additions & 0 deletions mod/assign/feedback/editpdf/tests/behat/annotate_pdf.feature
View file
Expand Up @@ -4,6 +4,67 @@ Feature: In an assignment, teacher can annotate PDF files during grading
As a teacher
I need to use the PDF editor

@javascript
Scenario: Submit a PDF file as a student and annotate the PDF as a teacher then overwrite the submission as a student
Given ghostscript is installed
And the following "courses" exist:
| fullname | shortname | category | groupmode |
| Course 1 | C1 | 0 | 1 |
And the following "users" exist:
| username | firstname | lastname | email |
| teacher1 | Teacher | 1 | teacher1@example.com |
| student1 | Student | 1 | student1@example.com |
And the following "course enrolments" exist:
| user | course | role |
| teacher1 | C1 | editingteacher |
| student1 | C1 | student |
And the following "activity" exists:
| activity | assign |
| course | C1 |
| name | Test assignment name |
| assignfeedback_editpdf_enabled | 1 |
| assignfeedback_comments_enabled | 1 |
| assignsubmission_file_enabled | 1 |
| assignsubmission_file_maxfiles | 2 |
| assignsubmission_file_maxsizebytes | 102400 |
| maxfilessubmission | 2 |
| submissiondrafts | 0 |
And the following "mod_assign > submission" exists:
| assign | Test assignment name |
| user | student1 |
| file | mod/assign/feedback/editpdf/tests/fixtures/testgs.pdf |

When I am on the "Test assignment name" Activity page logged in as teacher1
And I follow "View all submissions"
And I click on "Grade" "link" in the "Submitted for grading" "table_row"
Then I should see "Page 1 of 1"
And I wait for the complete PDF to load
And I click on ".linebutton" "css_element"
And I draw on the pdf
And I press "Save changes"
And I should see "The changes to the grade and feedback were saved"
And I am on the "Test assignment name" Activity page logged in as student1
And I follow "View annotated PDF..."
Then I should see "Page 1 of 1"
And I click on ".closebutton" "css_element"
And I press "Edit submission"
And I upload "mod/assign/feedback/editpdf/tests/fixtures/submission.pdf" file to "File submissions" filemanager
And I press "Save changes"
And I follow "View annotated PDF..."
Then I should see "Page 1 of 1"
And I am on the "Test assignment name" Activity page logged in as teacher1
And I follow "View all submissions"
And I click on "Grade" "link" in the "Submitted for grading" "table_row"
Then I should see "Page 1 of 3"
And I wait for the complete PDF to load
And I click on ".linebutton" "css_element"
And I draw on the pdf
And I press "Save changes"
And I should see "The changes to the grade and feedback were saved"
And I am on the "Test assignment name" Activity page logged in as student1
And I follow "View annotated PDF..."
Then I should see "Page 1 of 3"

@javascript
Scenario: Submit a PDF file as a student and annotate the PDF as a teacher
Given ghostscript is installed
Expand Down

0 comments on commit 2fbb766

Please sign in to comment.