Skip to content

Commit

Permalink
Merge branch 'MDL-75898-master' of https://github.com/matthewhilton/m…
Browse files Browse the repository at this point in the history 
…oodle
  • Loading branch information
@junpataleta
junpataleta committed Jun 8, 2023
2 parents f31d8f6 + 94cf5b1 commit d1295a8
Show file tree
Hide file tree
Showing 7 changed files with 166 additions and 15 deletions.
26 changes: 19 additions & 7 deletions mod/assign/feedback/editpdf/classes/document_services.php
View file
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
30 changes: 27 additions & 3 deletions mod/assign/feedback/editpdf/tests/behat/behat_assignfeedback_editpdf.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,23 +56,47 @@ public function ghostscript_is_installed() {
* @When /^I draw on the pdf$/
*/
public function i_draw_on_the_pdf() {
// There appears to be a bug with detecting changes to
// annotations. If a PDF is annotated, then the student
// updates the submission, if the teacher then draws the
// exact same annotations on the new PDF, the readonly
// pages are not updated and the student's view of the
// annotated PDF still shows the old PDF. So we add some
// randomness in this step to ensure the annotations are
// different every time.
//
// This was added to test MDL-75898. See MDL-76659 for
// more details about the bug.
//
// Note - the start, move and end locations must all be different.
// If they are the same, it's possible the PDF tool selected does not activate.
$startx = 100 + rand(0, 50);
$starty = 250 + rand(0, 50);
$js = ' (function() {
var instance = M.assignfeedback_editpdf.instance;
var event = { clientX: 100, clientY: 250, preventDefault: function() {} };
var event = { clientX: ' . $startx . ', clientY: ' . $starty . ', preventDefault: function() {} };
instance.edit_start(event);
}()); ';
$this->execute_script($js);
sleep(1);

// Move slightly in one direction.
$movex = $startx + 50;
$movey = $starty + 30;
$js = ' (function() {
var instance = M.assignfeedback_editpdf.instance;
var event = { clientX: 150, clientY: 275, preventDefault: function() {} };
var event = { clientX: ' . $movex . ', clientY: ' . $movey . ', preventDefault: function() {} };
instance.edit_move(event);
}()); ';
$this->execute_script($js);
sleep(1);

// Move a little further to stop.
$endx = $movex + 15;
$endy = $movey + 15;
$js = ' (function() {
var instance = M.assignfeedback_editpdf.instance;
var event = { clientX: 200, clientY: 300, preventDefault: function() {} };
var event = { clientX: ' . $endx . ', clientY: ' . $endy . ', preventDefault: function() {} };
instance.edit_end(event);
}()); ';
$this->execute_script($js);
Expand Down
18 changes: 18 additions & 0 deletions .../build/moodle-assignfeedback_editpdf-editor/moodle-assignfeedback_editpdf-editor-debug.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3780,6 +3780,24 @@ EDITOR.prototype = {
var data = this.handle_response_data(response),
poll = false;
if (data) {
// When we are requesting the readonly version of the pages, they should
// always be available (see document_services::get_page_images_for_attempt)
// so we can just serve them immediately without triggering any document
// conversion or polling.
//
// This is necessary to prevent situations where the student has updated
// their submission and the teacher has annotated a previous version of
// the submission in the assignment grader. In this situation if a student
// views the online version of the annotated PDF ("View annotated PDF" link)
// the readonly pages here and the updated pages (awaiting conversion) will
// never match, and the code endlessly polls.
//
// See also: MDL-45580, MDL-66626, MDL-75898.
if (this.get('readonly') === true) {
this.prepare_pages_for_display(data);
return;
}

this.documentstatus = data.status;
if (data.status === 0) {
// The combined document is still waiting for input to be ready.
Expand Down
Loading

0 comments on commit d1295a8

Please sign in to comment.