From 825609ae082caf763131d282d736cb45232e36b5 Mon Sep 17 00:00:00 2001 From: Cameron Ball Date: Tue, 13 Dec 2022 10:20:16 +0800 Subject: [PATCH 1/3] MDL-75898 assignfeedback_editpdf: Improve page count mismatch logic 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. --- .../editpdf/classes/document_services.php | 26 +++++--- .../editpdf/tests/behat/annotate_pdf.feature | 61 +++++++++++++++++++ 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/mod/assign/feedback/editpdf/classes/document_services.php b/mod/assign/feedback/editpdf/classes/document_services.php index 739ac40d795..86e0119b806 100644 --- a/mod/assign/feedback/editpdf/classes/document_services.php +++ b/mod/assign/feedback/editpdf/classes/document_services.php @@ -580,14 +580,26 @@ EOD; } } + // 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); } diff --git a/mod/assign/feedback/editpdf/tests/behat/annotate_pdf.feature b/mod/assign/feedback/editpdf/tests/behat/annotate_pdf.feature index 762be8cc47d..468db0d79d0 100644 --- a/mod/assign/feedback/editpdf/tests/behat/annotate_pdf.feature +++ b/mod/assign/feedback/editpdf/tests/behat/annotate_pdf.feature @@ -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 From 90350915c29345ed430053dcae48f3ebc0d1938b Mon Sep 17 00:00:00 2001 From: Cameron Ball Date: Tue, 13 Dec 2022 10:17:25 +0800 Subject: [PATCH 2/3] MDL-75898 assignfeedback_editpdf: Don't poll readonly pages When requesting the readonly version of pages (which contain teacher annotations), they should always be available - the PHP side even throws an exception when they're not. This means we don't need to worry about polling document converters from the JS side and can just return the pages immediately. --- ...odle-assignfeedback_editpdf-editor-debug.js | 18 ++++++++++++++++++ ...moodle-assignfeedback_editpdf-editor-min.js | 10 +++++----- .../moodle-assignfeedback_editpdf-editor.js | 18 ++++++++++++++++++ .../editpdf/yui/src/editor/js/editor.js | 18 ++++++++++++++++++ 4 files changed, 59 insertions(+), 5 deletions(-) diff --git a/mod/assign/feedback/editpdf/yui/build/moodle-assignfeedback_editpdf-editor/moodle-assignfeedback_editpdf-editor-debug.js b/mod/assign/feedback/editpdf/yui/build/moodle-assignfeedback_editpdf-editor/moodle-assignfeedback_editpdf-editor-debug.js index a5728757953..bbca05bc937 100644 --- a/mod/assign/feedback/editpdf/yui/build/moodle-assignfeedback_editpdf-editor/moodle-assignfeedback_editpdf-editor-debug.js +++ b/mod/assign/feedback/editpdf/yui/build/moodle-assignfeedback_editpdf-editor/moodle-assignfeedback_editpdf-editor-debug.js @@ -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. diff --git a/mod/assign/feedback/editpdf/yui/build/moodle-assignfeedback_editpdf-editor/moodle-assignfeedback_editpdf-editor-min.js b/mod/assign/feedback/editpdf/yui/build/moodle-assignfeedback_editpdf-editor/moodle-assignfeedback_editpdf-editor-min.js index 78f7e469e95..525b9391fc3 100644 --- a/mod/assign/feedback/editpdf/yui/build/moodle-assignfeedback_editpdf-editor/moodle-assignfeedback_editpdf-editor-min.js +++ b/mod/assign/feedback/editpdf/yui/build/moodle-assignfeedback_editpdf-editor/moodle-assignfeedback_editpdf-editor-min.js @@ -1,9 +1,9 @@ -YUI.add("moodle-assignfeedback_editpdf-editor",function(u,e){var c,i,s,n,a,h,l,p,f,_,m,b,w,o=M.cfg.wwwroot+"/mod/assign/feedback/editpdf/ajax.php",k=M.cfg.wwwroot+"/mod/assign/feedback/editpdf/ajax_progress.php",y="assignfeedback_editpdf_widget",v=".navigate-previous-button",x=" .navigate-next-button",A=".searchcommentsbutton",S=".expcolcommentsbutton",N=".assignfeedback_editpdf_commentsearch input",D=".assignfeedback_editpdf_commentsearch ul",T=".navigate-page-select",I=".loading",C=".progress-info.progress-striped",r=".drawingregion",g=".drawingcanvas",q=".commentcolourbutton",E=".annotationcolourbutton",Y=".warningmessages",z=".infoicon",R='input[name="assignfeedback_editpdf_haschanges"]',X=".currentstampbutton",L='[data-region="user-info"]',P=".rotateleftbutton",j=".rotaterightbutton",O={white:"rgb(255,255,255)",yellow:"rgb(255,236,174)",red:"rgb(249,181,179)",green:"rgb(214,234,178)",blue:"rgb(203,217,237)",clear:"rgba(255,255,255, 0)"},d={white:"rgb(255,255,255)",yellow:"rgb(255,207,53)",red:"rgb(239,69,64)",green:"rgb(152,202,62)",blue:"rgb(125,159,211)",black:"rgb(51,51,51)"},B={comment:".commentbutton",pen:".penbutton",line:".linebutton",rectangle:".rectanglebutton",oval:".ovalbutton",stamp:".stampbutton",select:".selectbutton",drag:".dragbutton",highlight:".highlightbutton"},t=function(t,e){this.x=parseInt(t,10),this.y=parseInt(e,10),this.clip=function(t){return this.xt.x+t.width&&(this.x=t.x+t.width),this.yt.y+t.height&&(this.y=t.y+t.height),this}};M.assignfeedback_editpdf=M.assignfeedback_editpdf||{},M.assignfeedback_editpdf.point=t,t=function(t,e,i,s){this.x=t,this.y=e,this.width=i,this.height=s,this.bound=function(t){for(var e,i=0,s=0,n=0,a=0,o=0,o=0;os||0===o)&&(s=e.x),(e.ya||0===o)&&(a=e.y);return this.x=i,this.y=n,this.width=s-i,this.height=a-n,this},this.has_min_width=function(){return 5<=this.width},this.has_min_height=function(){return 5<=this.height},this.set_min_width=function(){this.width=5},this.set_min_height=function(){this.height=5}},M.assignfeedback_editpdf=M.assignfeedback_editpdf||{},M.assignfeedback_editpdf.rect=t,t=function(){this.start=!1,this.end=!1,this.starttime=0,this.annotationstart=!1,this.tool="drag",this.commentcolour="yellow",this.annotationcolour="red",this.stamp="",this.path=[]},M.assignfeedback_editpdf=M.assignfeedback_editpdf||{},M.assignfeedback_editpdf.edit=t,t=function(t){this.editor=t,this.shapes=[],this.nodes=[],this.erase=function(){if(this.shapes)for(;0'),i=u.Node.create(''),e.setAttrs({alt:M.util.get_string("deleteannotation","assignfeedback_editpdf")}),e.setStyles({backgroundColor:"white"}),i.addClass("deleteannotationbutton"),i.append(e),s.append(i),i.setData("annotation",this),i.on("click",this.remove,this),i.on("key",this.remove,"space,enter",this),i.setX(n[0]+t.x+t.width-18),i.setY(n[1]+t.y+6),this.drawable.nodes.push(i)),this.drawable},draw:function(){return this.draw_highlight(),this.drawable},remove:function(t){var e,i;for(t.preventDefault(),e=this.editor.pages[this.editor.currentpage].annotations,i=0;it.x+t.width&&(this.x=t.x+t.width),this.yt.y+t.height&&(this.y=t.y+t.height),this}};M.assignfeedback_editpdf=M.assignfeedback_editpdf||{},M.assignfeedback_editpdf.point=t,t=function(t,e,i,s){this.x=t,this.y=e,this.width=i,this.height=s,this.bound=function(t){for(var e,i=0,s=0,n=0,a=0,o=0,o=0;os||0===o)&&(s=e.x),(e.ya||0===o)&&(a=e.y);return this.x=i,this.y=n,this.width=s-i,this.height=a-n,this},this.has_min_width=function(){return 5<=this.width},this.has_min_height=function(){return 5<=this.height},this.set_min_width=function(){this.width=5},this.set_min_height=function(){this.height=5}},M.assignfeedback_editpdf=M.assignfeedback_editpdf||{},M.assignfeedback_editpdf.rect=t,t=function(){this.start=!1,this.end=!1,this.starttime=0,this.annotationstart=!1,this.tool="drag",this.commentcolour="yellow",this.annotationcolour="red",this.stamp="",this.path=[]},M.assignfeedback_editpdf=M.assignfeedback_editpdf||{},M.assignfeedback_editpdf.edit=t,t=function(t){this.editor=t,this.shapes=[],this.nodes=[],this.erase=function(){if(this.shapes)for(;0'),i=u.Node.create(''),e.setAttrs({alt:M.util.get_string("deleteannotation","assignfeedback_editpdf")}),e.setStyles({backgroundColor:"white"}),i.addClass("deleteannotationbutton"),i.append(e),s.append(i),i.setData("annotation",this),i.on("click",this.remove,this),i.on("key",this.remove,"space,enter",this),i.setX(n[0]+t.x+t.width-18),i.setY(n[1]+t.y+6),this.drawable.nodes.push(i)),this.drawable},draw:function(){return this.draw_highlight(),this.drawable},remove:function(t){var e,i;for(t.preventDefault(),e=this.editor.pages[this.editor.currentpage].annotations,i=0;i");return s.addClass("annotation"),s.addClass("stamp"),s.setStyles({position:"absolute",display:"inline-block",backgroundImage:"url("+this.editor.get_stamp_image_url(this.path)+")",width:this.endx-this.x,height:this.endy-this.y,backgroundSize:"100% 100%"}),e.append(s),s.setX(i.x),s.setY(i.y),t.store_position(s,i.x,i.y),this.editor.get("readonly")||(s.on("gesturemovestart",this.editor.edit_start,null,this.editor),s.on("gesturemove",this.editor.edit_move,null,this.editor),s.on("gesturemoveend",this.editor.edit_end,null,this.editor)),t.nodes.push(s),this.drawable=t,l.superclass.draw.apply(this)},draw_current_edit:function(t){var e,i,s=new M.assignfeedback_editpdf.rect,n=new M.assignfeedback_editpdf.drawable(this.editor),a=this.editor.get_dialogue_element(r);return s.bound([t.start,t.end]),i=this.editor.get_window_coordinates(new M.assignfeedback_editpdf.point(s.x,s.y)),(e=u.Node.create("
")).addClass("annotation"),e.addClass("stamp"),e.setStyles({position:"absolute",display:"inline-block",backgroundImage:"url("+this.editor.get_stamp_image_url(t.stamp)+")",width:s.width,height:s.height,backgroundSize:"100% 100%"}),a.append(e),e.setX(i.x),e.setY(i.y),n.store_position(e,i.x,i.y),n.nodes.push(e),n},init_from_edit:function(t){var e=new M.assignfeedback_editpdf.rect;return e.bound([t.start,t.end]),e.width<40&&(e.width=40),e.height<40&&(e.height=40),this.gradeid=this.editor.get("gradeid"),this.pageno=this.editor.currentpage,this.x=e.x,this.y=e.y,this.endx=e.x+e.width,this.endy=e.y+e.height,this.colour=t.annotationcolour,this.path=t.stamp,!0},move:function(t,e){t-=this.x,e-=this.y;this.x+=t,this.y+=e,this.endx+=t,this.endy+=e,this.drawable&&this.drawable.erase(),this.editor.drawables.push(this.draw())}}),M.assignfeedback_editpdf=M.assignfeedback_editpdf||{},M.assignfeedback_editpdf.annotationstamp=l,u.extend(p=function(t){t.draggable=!1,t.centered=!1,t.width="auto",t.visible=!1,t.footerContent="",p.superclass.constructor.apply(this,[t])},M.core.dialogue,{initializer:function(t){var e,i;p.superclass.initializer.call(this,t),this.get("boundingBox").addClass("assignfeedback_editpdf_dropdown"),e=this.get("buttonNode"),t=this.bodyNode,(i=u.Node.create("

")).addClass("accesshide"),i.setHTML(this.get("headerText")),t.prepend(i),t.on("clickoutside",function(t){this.get("visible")&&t.target.get("id")!==e.get("id")&&t.target.ancestor().get("id")!==e.get("id")&&(t.preventDefault(),this.hide())},this),e.on("click",function(t){t.preventDefault(),this.show()},this),e.on("key",this.show,"enter,space",this)},show:function(){var t=this.get("buttonNode"),e=p.superclass.show.call(this);return this.align(t,[u.WidgetPositionAlign.TL,u.WidgetPositionAlign.BL]),e}},{NAME:"Dropdown menu",ATTRS:{headerText:{value:""},buttonNode:{value:null}}}),u.Base.modifyAttrs(p,{modal:{getter:function(){return!1}}}),M.assignfeedback_editpdf=M.assignfeedback_editpdf||{},M.assignfeedback_editpdf.dropdown=p,u.extend(f=function(t){f.superclass.constructor.apply(this,[t])},M.assignfeedback_editpdf.dropdown,{initializer:function(t){var e,n=u.Node.create('