Skip to content

Commit

Permalink
MDL-64540 assign: Backport of MDL-60008
Browse files Browse the repository at this point in the history 
This patch combines the following changes:

* Basically we should produce a combined PDF from all the files that it was possible to convert.
* If any one submission input file fails conversion, it should just be omitted in the result.
* When not all files are available in the online pdf, display a warning to graders
that some files must be downloaded.
* Better support for information messages from the conversion process.
* Remove the z-index values from the expand / collapse panels. They cause errors when mixed with dialogues.
* The edit pdf menus are all broken on small screen sizes. They consume 100% of the page with for no reason.
* The heights of the buttons to hide/show panels are arbitrarily big. There is not need to set them specifically.

Amended on integration to get rid if incorrect 'destroyed' block of
code. See the issue for more information.
  • Loading branch information
@stronk7
Damyon Wiese authored and stronk7 committed Feb 19, 2019
1 parent 02bcf46 commit 2165cd3
Show file tree
Hide file tree
Showing 16 changed files with 347 additions and 98 deletions.
9 changes: 7 additions & 2 deletions mod/assign/feedback/editpdf/ajax.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,21 @@
'filecount' => 0,
'pagecount' => 0,
'pageready' => 0,
'partial' => false,
'pages' => [],
];

$combineddocument = document_services::get_combined_document_for_attempt($assignment, $userid, $attemptnumber);
$response->status = $combineddocument->get_status();
$response->filecount = $combineddocument->get_document_count();

if ($response->status === combined_document::STATUS_READY) {
$readystatuslist = [combined_document::STATUS_READY, combined_document::STATUS_READY_PARTIAL];
$completestatuslist = [combined_document::STATUS_COMPLETE, combined_document::STATUS_FAILED];

if (in_array($response->status, $readystatuslist)) {
$combineddocument = document_services::get_combined_pdf_for_attempt($assignment, $userid, $attemptnumber);
$response->pagecount = $combineddocument->get_page_count();
} else if ($response->status === combined_document::STATUS_COMPLETE || $response->status === combined_document::STATUS_FAILED) {
} else if (in_array($response->status, $completestatuslist)) {
$pages = document_services::get_page_images_for_attempt($assignment,
$userid,
$attemptnumber,
Expand All @@ -95,6 +99,7 @@
if ($readonly) {
$filearea = document_services::PAGE_IMAGE_READONLY_FILEAREA;
}
$response->partial = $combineddocument->is_partial_conversion();

foreach ($pages as $id => $pagefile) {
$index = count($response->pages);
Expand Down
60 changes: 50 additions & 10 deletions mod/assign/feedback/editpdf/classes/combined_document.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ class combined_document {
*/
const STATUS_READY = 1;

/**
* Status value representing all documents are ready to be combined as are supported.
*/
const STATUS_READY_PARTIAL = 3;

/**
* Status value representing a successful conversion.
*/
Expand Down Expand Up @@ -76,6 +81,9 @@ class combined_document {

/**
* Check the current status of the document combination.
* Note that the combined document may not contain all the source files if some of the
* source files were not able to be converted. An example is an audio file with a pdf cover sheet. Only
* the cover sheet will be included in the combined document.
*
* @return int
*/
Expand All @@ -97,6 +105,7 @@ public function get_status() {
}

$pending = false;
$partial = false;
foreach ($this->sourcefiles as $file) {
// The combined file has not yet been generated.
// Check the status of each source file.
Expand All @@ -108,14 +117,19 @@ public function get_status() {
$pending = true;
break;

// There are 4 status flags, so the only remaining one is complete which is fine.
case \core_files\conversion::STATUS_FAILED:
return self::STATUS_FAILED;
$partial = true;
break;
}
}
}
if ($pending) {
return self::STATUS_PENDING_INPUT;
} else {
if ($partial) {
return self::STATUS_READY_PARTIAL;
}
return self::STATUS_READY;
}
}
Expand All @@ -131,6 +145,20 @@ public function set_combined_file($file) {
return $this;
}

/**
* Return true of the combined file contained only some of the submission files.
*
* @return boolean
*/
public function is_partial_conversion() {
$combinedfile = $this->get_combined_file();
if (empty($combinedfile)) {
return false;
}
$filearea = $combinedfile->get_filearea();
return $filearea == document_services::PARTIAL_PDF_FILEAREA;
}

/**
* Retrieve the completed combined file.
*
Expand Down Expand Up @@ -210,11 +238,12 @@ public function combine_files($contextid, $itemid) {
global $CFG;

$currentstatus = $this->get_status();
$readystatuslist = [self::STATUS_READY, self::STATUS_READY_PARTIAL];
if ($currentstatus === self::STATUS_FAILED) {
$this->store_empty_document($contextid, $itemid);

return $this;
} else if ($currentstatus !== self::STATUS_READY) {
} else if (!in_array($currentstatus, $readystatuslist)) {
// The document is either:
// * already combined; or
// * pending input being fully converted; or
Expand All @@ -235,7 +264,10 @@ public function combine_files($contextid, $itemid) {
// Note: We drop non-compatible files.
$compatiblepdf = false;
if (is_a($file, \core_files\conversion::class)) {
$compatiblepdf = pdf::ensure_pdf_compatible($file->get_destfile());
$status = $file->get('status');
if ($status == \core_files\conversion::STATUS_COMPLETE) {
$compatiblepdf = pdf::ensure_pdf_compatible($file->get_destfile());
}
} else {
$compatiblepdf = pdf::ensure_pdf_compatible($file);
}
Expand Down Expand Up @@ -270,7 +302,7 @@ public function combine_files($contextid, $itemid) {
}

// Store the newly created file as a stored_file.
$this->store_combined_file($tmpfile, $contextid, $itemid);
$this->store_combined_file($tmpfile, $contextid, $itemid, ($currentstatus == self::STATUS_READY_PARTIAL));

// Note the verified page count.
$this->pagecount = $verifypagecount;
Expand All @@ -295,11 +327,12 @@ protected function mark_combination_failed() {
* @param string $tmpfile The path to the file on disk to be stored.
* @param int $contextid The contextid for the file to be stored under
* @param int $itemid The itemid for the file to be stored under
* @param boolean $partial The combined pdf contains only some of the source files.
* @return $this
*/
protected function store_combined_file($tmpfile, $contextid, $itemid) {
protected function store_combined_file($tmpfile, $contextid, $itemid, $partial = false) {
// Store the file.
$record = $this->get_stored_file_record($contextid, $itemid);
$record = $this->get_stored_file_record($contextid, $itemid, $partial);
$fs = get_file_storage();

// Delete existing files first.
Expand Down Expand Up @@ -349,12 +382,14 @@ public function get_page_count() {
return $this->pagecount;
}

if ($this->get_status() === self::STATUS_FAILED) {
$status = $this->get_status();

if ($status === self::STATUS_FAILED) {
// The empty document will be returned.
return 1;
}

if ($this->get_status() !== self::STATUS_COMPLETE) {
if ($status !== self::STATUS_COMPLETE) {
// No pages yet.
return 0;
}
Expand Down Expand Up @@ -391,13 +426,18 @@ public function get_document_count() {
*
* @param int $contextid The contextid for the file to be stored under
* @param int $itemid The itemid for the file to be stored under
* @param boolean $partial The combined file contains only some of the source files.
* @return stdClass
*/
protected function get_stored_file_record($contextid, $itemid) {
protected function get_stored_file_record($contextid, $itemid, $partial = false) {
$filearea = document_services::COMBINED_PDF_FILEAREA;
if ($partial) {
$filearea = document_services::PARTIAL_PDF_FILEAREA;
}
return (object) [
'contextid' => $contextid,
'component' => 'assignfeedback_editpdf',
'filearea' => document_services::COMBINED_PDF_FILEAREA,
'filearea' => $filearea,
'itemid' => $itemid,
'filepath' => '/',
'filename' => document_services::COMBINED_PDF_FILENAME,
Expand Down
13 changes: 12 additions & 1 deletion mod/assign/feedback/editpdf/classes/document_services.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ class document_services {
const FINAL_PDF_FILEAREA = 'download';
/** File area for combined pdf */
const COMBINED_PDF_FILEAREA = 'combined';
/** File area for partial combined pdf */
const PARTIAL_PDF_FILEAREA = 'partial';
/** File area for importing html */
const IMPORT_HTML_FILEAREA = 'importhtml';
/** File area for page images */
Expand Down Expand Up @@ -261,15 +263,23 @@ public static function get_combined_document_for_attempt($assignment, $userid, $
$submission = $assignment->get_user_submission($userid, false, $attemptnumber);
}


$contextid = $assignment->get_context()->id;
$component = 'assignfeedback_editpdf';
$filearea = self::COMBINED_PDF_FILEAREA;
$partialfilearea = self::PARTIAL_PDF_FILEAREA;
$itemid = $grade->id;
$filepath = '/';
$filename = self::COMBINED_PDF_FILENAME;
$fs = get_file_storage();

$combinedpdf = $fs->get_file($contextid, $component, $filearea, $itemid, $filepath, $filename);
$partialpdf = $fs->get_file($contextid, $component, $partialfilearea, $itemid, $filepath, $filename);
if (!empty($partialpdf)) {
$combinedpdf = $partialpdf;
} else {
$combinedpdf = $fs->get_file($contextid, $component, $filearea, $itemid, $filepath, $filename);
}

if ($combinedpdf && $submission) {
if ($combinedpdf->get_timemodified() < $submission->timemodified) {
// The submission has been updated since the PDF was generated.
Expand Down Expand Up @@ -381,6 +391,7 @@ protected static function generate_page_images_for_attempt($assignment, $userid,

$tmpdir = \make_temp_directory('assignfeedback_editpdf/pageimages/' . self::hash($assignment, $userid, $attemptnumber));
$combined = $tmpdir . '/' . self::COMBINED_PDF_FILENAME;

$document->get_combined_file()->copy_content_to($combined); // Copy the file.

$pdf = new pdf();
Expand Down
17 changes: 9 additions & 8 deletions mod/assign/feedback/editpdf/classes/renderer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,15 +217,14 @@ public function render_assignfeedback_editpdf_widget(assignfeedback_editpdf_widg

$canvas = html_writer::div($loading, 'drawingcanvas');
$canvas = html_writer::div($canvas, 'drawingregion');
$changesmessage = html_writer::tag('div',
get_string('draftchangessaved', 'assignfeedback_editpdf'),
array(
'class' => 'assignfeedback_editpdf_unsavedchanges warning label label-info'
));

$changesmessage = html_writer::div($changesmessage, 'unsaved-changes');
// Place for messages, but no warnings displayed yet.
$changesmessage = html_writer::div('', 'warningmessages');
$canvas .= $changesmessage;

$infoicon = $this->image_icon('i/info', '');
$infomessage = html_writer::div($infoicon, 'infoicon');
$canvas .= $infomessage;

$body .= $canvas;

$footer = '';
Expand Down Expand Up @@ -268,7 +267,9 @@ public function render_assignfeedback_editpdf_widget(assignfeedback_editpdf_widg
'stamp',
'stamppicker',
'cannotopenpdf',
'pagenumber'
'pagenumber',
'partialwarning',
'draftchangessaved'
), 'assignfeedback_editpdf');

return $html;
Expand Down
3 changes: 1 addition & 2 deletions mod/assign/feedback/editpdf/classes/task/convert_submissions.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,9 @@ public function execute() {
foreach ($users as $userid) {
try {
$combineddocument = document_services::get_combined_pdf_for_attempt($assignment, $userid, $attemptnumber);
$status = $combineddocument->get_status();

switch ($combineddocument->get_status()) {
case combined_document::STATUS_READY:
case combined_document::STATUS_READY_PARTIAL:
case combined_document::STATUS_PENDING_INPUT:
// The document has not been converted yet or is somehow still ready.
continue 2;
Expand Down
1 change: 1 addition & 0 deletions mod/assign/feedback/editpdf/lang/en/assignfeedback_editpdf.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
$string['pagenumber'] = 'Page {$a}';
$string['pagexofy'] = 'Page {$a->page} of {$a->total}';
$string['pen'] = 'Pen';
$string['partialwarning'] = 'Some of the files in this submission can only be accessed by direct download.';
$string['pluginname'] = 'Annotate PDF';
$string['privacy:metadata:colourpurpose'] = 'Colour of the comment or annotation';
$string['privacy:metadata:conversionpurpose'] = 'Files are converted to PDFs to allow for annotations.';
Expand Down
23 changes: 12 additions & 11 deletions mod/assign/feedback/editpdf/styles.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,17 @@
cursor: move;
}

.assignfeedback_editpdf_widget .infoicon {
display: none;
}

.assignfeedback_editpdf_widget .warningmessages {
position: absolute;
margin-left: 20px;
margin-right: 20px;
bottom: 20px;
}

.assignfeedback_editpdf_widget .drawingregion {
border: 1px solid #ccc;
left: 1em;
Expand Down Expand Up @@ -59,17 +70,6 @@
padding: 0;
}

.assignfeedback_editpdf_widget .assignfeedback_editpdf_unsavedchanges.haschanges {
display: inline-block;
}

.assignfeedback_editpdf_widget .assignfeedback_editpdf_unsavedchanges {
display: none;
position: absolute;
left: 20px;
top: 60px;
}

.yui3-colourpicker-hidden,
.yui3-commentsearch-hidden,
.yui3-commentmenu-hidden {
Expand Down Expand Up @@ -229,6 +229,7 @@
margin-right: 0;
margin-top: 0;
border-radius: 4px;
width: 30px;
}

.moodle-dialogue-base .moodle-dialogue.assignfeedback_editpdf_dropdown .moodle-dialogue-bd {
Expand Down
Loading

0 comments on commit 2165cd3

Please sign in to comment.