From 1e98c4ad27303fdd808e9ab724d1bd69abf178c1 Mon Sep 17 00:00:00 2001 From: Cameron Ball Date: Wed, 27 Jul 2022 15:35:18 +0800 Subject: [PATCH 1/2] MDL-68943 assignfeedback_editpdf: Reconvert updated files --- mod/assign/feedback/editpdf/lib.php | 31 ++++++++ .../feedback/editpdf/tests/feedback_test.php | 73 +++++++++++++++++-- .../editpdf/tests/fixtures/submission.txt | 3 + 3 files changed, 102 insertions(+), 5 deletions(-) create mode 100644 mod/assign/feedback/editpdf/tests/fixtures/submission.txt diff --git a/mod/assign/feedback/editpdf/lib.php b/mod/assign/feedback/editpdf/lib.php index 8505f17c94f..815e35749e7 100644 --- a/mod/assign/feedback/editpdf/lib.php +++ b/mod/assign/feedback/editpdf/lib.php @@ -101,3 +101,34 @@ function assignfeedback_editpdf_pluginfile( } } + +/** + * Files API hook to remove stale conversion records. + * + * When a file is update, its contenthash will change, but its ID + * remains the same. The document converter API records source file + * IDs and destination file IDs. When a file is updated, the document + * converter API has no way of knowing that the content of the file + * has changed, so it just serves the previously stored destination + * file. + * + * In this hook we check if the contenthash has changed, and if it has + * we delete the existing conversion so that a new one will be created. + * + * @param stdClass $file The updated file record. + * @param stdClass $filepreupdate The file record pre-update. + */ +function assignfeedback_editpdf_after_file_updated(stdClass $file, stdClass $filepreupdate) { + $contenthashchanged = $file->contenthash !== $filepreupdate->contenthash; + if ($contenthashchanged && $file->component == 'assignsubmission_file' && $file->filearea == 'submission_files') { + $fs = get_file_storage(); + $file = $fs->get_file_by_id($file->id); + $conversions = \core_files\conversion::get_conversions_for_file($file, 'pdf'); + + foreach ($conversions as $conversion) { + if ($conversion->get('id')) { + $conversion->delete(); + } + } + } +} diff --git a/mod/assign/feedback/editpdf/tests/feedback_test.php b/mod/assign/feedback/editpdf/tests/feedback_test.php index 1d2e2f5ddb2..a777955e95b 100644 --- a/mod/assign/feedback/editpdf/tests/feedback_test.php +++ b/mod/assign/feedback/editpdf/tests/feedback_test.php @@ -47,7 +47,14 @@ class feedback_test extends \advanced_testcase { } } - protected function add_file_submission($student, $assign) { + /** + * Helper method to add a file to a submission. + * + * @param stdClass $student Student submitting. + * @param assign $assign Assignment being submitted. + * @param bool $textfile Use textfile fixture instead of pdf. + */ + protected function add_file_submission($student, $assign, $textfile = false) { global $CFG; $this->setUser($student); @@ -56,16 +63,16 @@ class feedback_test extends \advanced_testcase { $submission = $assign->get_user_submission($student->id, true); $fs = get_file_storage(); - $pdfsubmission = (object) array( + $filerecord = (object) array( 'contextid' => $assign->get_context()->id, 'component' => 'assignsubmission_file', 'filearea' => ASSIGNSUBMISSION_FILE_FILEAREA, 'itemid' => $submission->id, 'filepath' => '/', - 'filename' => 'submission.pdf' + 'filename' => $textfile ? 'submission.txt' : 'submission.pdf' ); - $sourcefile = $CFG->dirroot.'/mod/assign/feedback/editpdf/tests/fixtures/submission.pdf'; - $fs->create_file_from_pathname($pdfsubmission, $sourcefile); + $sourcefile = $CFG->dirroot . '/mod/assign/feedback/editpdf/tests/fixtures/submission.' . ($textfile ? 'txt' : 'pdf'); + $fs->create_file_from_pathname($filerecord, $sourcefile); $data = new \stdClass(); $plugin = $assign->get_submission_plugin_by_type('file'); @@ -515,4 +522,60 @@ class feedback_test extends \advanced_testcase { // No modification. $this->assertFalse($plugin->is_feedback_modified($grade, $data)); } + + /** + * Test that overwriting a submission file deletes any associated conversions. + * + * @covers \core_files\conversion::get_conversions_for_file + */ + public function test_submission_file_overridden() { + $this->resetAfterTest(); + $course = $this->getDataGenerator()->create_course(); + $student = $this->getDataGenerator()->create_and_enrol($course, 'student'); + $assign = $this->create_instance($course, [ + 'assignsubmission_onlinetext_enabled' => 1, + 'assignsubmission_file_enabled' => 1, + 'assignsubmission_file_maxfiles' => 1, + 'assignfeedback_editpdf_enabled' => 1, + 'assignsubmission_file_maxsizebytes' => 1000000, + ]); + + $this->add_file_submission($student, $assign, true); + $submission = $assign->get_user_submission($student->id, true); + + $fs = get_file_storage(); + $sourcefile = $fs->get_file( + $assign->get_context()->id, + 'assignsubmission_file', + ASSIGNSUBMISSION_FILE_FILEAREA, + $submission->id, + '/', + 'submission.txt' + ); + + $conversion = new \core_files\conversion(0, (object)[ + 'sourcefileid' => $sourcefile->get_id(), + 'targetformat' => 'pdf' + ]); + $conversion->create(); + + $conversions = \core_files\conversion::get_conversions_for_file($sourcefile, 'pdf'); + $this->assertCount(1, $conversions); + + $filerecord = (object)[ + 'contextid' => $assign->get_context()->id, + 'component' => 'core', + 'filearea' => 'unittest', + 'itemid' => $submission->id, + 'filepath' => '/', + 'filename' => 'submission.txt' + ]; + + $fs = get_file_storage(); + $newfile = $fs->create_file_from_string($filerecord, 'something totally different'); + $sourcefile->replace_file_with($newfile); + + $conversions = \core_files\conversion::get_conversions_for_file($sourcefile, 'pdf'); + $this->assertCount(0, $conversions); + } } diff --git a/mod/assign/feedback/editpdf/tests/fixtures/submission.txt b/mod/assign/feedback/editpdf/tests/fixtures/submission.txt new file mode 100644 index 00000000000..add62815e9e --- /dev/null +++ b/mod/assign/feedback/editpdf/tests/fixtures/submission.txt @@ -0,0 +1,3 @@ +你行你上啊！ + +不行别BB From 01074798e1e9fdbdb14fc6d59b9081d1b30de041 Mon Sep 17 00:00:00 2001 From: Cameron Ball Date: Tue, 23 Aug 2022 10:57:49 +0800 Subject: [PATCH 2/2] MDL-68943 assignfeedback_editpdf: Upgrade step for stale conversions --- .../bump_submission_for_stale_conversions.php | 103 ++++++++++++++++++ mod/assign/feedback/editpdf/db/upgrade.php | 11 ++ mod/assign/feedback/editpdf/version.php | 2 +- 3 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 mod/assign/feedback/editpdf/classes/task/bump_submission_for_stale_conversions.php diff --git a/mod/assign/feedback/editpdf/classes/task/bump_submission_for_stale_conversions.php b/mod/assign/feedback/editpdf/classes/task/bump_submission_for_stale_conversions.php new file mode 100644 index 00000000000..baa53f2e8a4 --- /dev/null +++ b/mod/assign/feedback/editpdf/classes/task/bump_submission_for_stale_conversions.php @@ -0,0 +1,103 @@ +. + +/** + * Bump submission timemodified for conversions that are stale. + * + * @package assignfeedback_editpdf + * @copyright 2022 Catalyst IT Australia Pty Ltd + * @author Cameron Ball + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace assignfeedback_editpdf\task; + +use core\task\adhoc_task; + +/** + * Adhoc task to bump the submission timemodified associated with a stale conversion. + * + * @package assignfeedback_editpdf + * @copyright 2022 Catalyst IT Australia Pty Ltd + * @author Cameron Ball + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class bump_submission_for_stale_conversions extends adhoc_task { + + /** + * Run the task. + */ + public function execute() { + global $DB; + + // Used to only get records after whenever document conversion was enabled for this site. + $earliestconversion = $DB->get_record_sql("SELECT MIN(timecreated) AS min + FROM {files} + WHERE filearea = 'documentconversion'"); + + if ($earliestconversion) { + ['sql' => $extensionsql, 'params' => $extensionparams] = array_reduce( + ['doc', 'docx', 'rtf', 'xls', 'xlsx', 'ppt', 'pptx', 'html', 'odt', 'ods', 'png', 'jpg', 'txt', 'gif'], + function(array $c, string $ext) use ($DB): array { + return [ + 'sql' => $c['sql'] . ($c['sql'] ? ' OR ' : '') . $DB->sql_like('f1.filename', ':' . $ext), + 'params' => $c['params'] + [$ext => '%.' . $ext] + ]; + }, + ['sql' => '', 'params' => []] + ); + + // A converted file has its filename set to the contenthash of the file it converted. + // Find all files in the relevant file areas for which there is no corresponding + // file with the contenthash as the file name. + // + // Also check if the file has a greater modified time than the submission, if it does + // that means it is both stale (as per the above) and will never be reconverted. + $sql = "SELECT f3.id, f3.timemodified as fmodified, asu.id as submissionid + FROM {files} f1 + LEFT JOIN {files} f2 ON f1.contenthash = f2.filename + AND f2.component = 'core' AND f2.filearea = 'documentconversion' + JOIN {assign_submission} asu ON asu.id = f1.itemid + JOIN {assign_grades} asg ON asg.userid = asu.userid AND asg.assignment = asu.assignment + JOIN {files} f3 ON f3.itemid = asg.id + WHERE f1.filearea = 'submission_files' + AND f3.timecreated >= :earliest + AND ($extensionsql) + AND f2.filename IS NULL + AND f3.component = 'assignfeedback_editpdf' + AND f3.filearea = 'combined' + AND f3.filename = 'combined.pdf' + AND f3.timemodified >= asu.timemodified"; + + $submissionstobump = $DB->get_records_sql($sql, ['earliest' => $earliestconversion->min] + $extensionparams); + foreach ($submissionstobump as $submission) { + + // Set the submission modified time to one second later than the + // converted files modified time, this will cause assign to reconvert + // everything and delete the old files when the assignment grader is + // viewed. See get_page_images_for_attempt in document_services.php. + $newmodified = $submission->fmodified + 1; + $record = (object)[ + 'id' => $submission->submissionid, + 'timemodified' => $newmodified + ]; + + mtrace('Set submission ' . $submission->submissionid . ' timemodified to ' . $newmodified); + $DB->update_record('assign_submission', $record); + } + } + } +} diff --git a/mod/assign/feedback/editpdf/db/upgrade.php b/mod/assign/feedback/editpdf/db/upgrade.php index dc6029be389..c439bb98ccb 100644 --- a/mod/assign/feedback/editpdf/db/upgrade.php +++ b/mod/assign/feedback/editpdf/db/upgrade.php @@ -82,5 +82,16 @@ function xmldb_assignfeedback_editpdf_upgrade($oldversion) { upgrade_plugin_savepoint(true, 2022061000, 'assignfeedback', 'editpdf'); } + if ($oldversion < 2022082200) { + // Conversion records need to be removed in order for conversions to restart. + $DB->delete_records('file_conversion'); + + // Schedule an adhoc task to fix existing stale conversions. + $task = new \assignfeedback_editpdf\task\bump_submission_for_stale_conversions(); + \core\task\manager::queue_adhoc_task($task); + + upgrade_plugin_savepoint(true, 2022082200, 'assignfeedback', 'editpdf'); + } + return true; } diff --git a/mod/assign/feedback/editpdf/version.php b/mod/assign/feedback/editpdf/version.php index d7cd8ba4cc5..d2599d740cd 100644 --- a/mod/assign/feedback/editpdf/version.php +++ b/mod/assign/feedback/editpdf/version.php @@ -24,6 +24,6 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2022061000; +$plugin->version = 2022082200; $plugin->requires = 2022041200; $plugin->component = 'assignfeedback_editpdf';