Merge branch 'MDL-68943-master' of https://github.com/cameron1729/moodle

2022-10-28 11:49:42 +08:00
parent eba92e07de 01074798e1
commit df90988239
6 changed files with 217 additions and 6 deletions
@@ -0,0 +1,103 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Bump submission timemodified for conversions that are stale.
+ *
+ * @package    assignfeedback_editpdf
+ * @copyright  2022 Catalyst IT Australia Pty Ltd
+ * @author     Cameron Ball <[email protected]>
+ * @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 <[email protected]>
+ * @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);
+            }
+        }
+    }
+}
@@ -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;
 }
@@ -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();
+            }
+        }
+    }
+}
@@ -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);
+    }
 }
@@ -0,0 +1,3 @@
+你行你上啊！
+
+不行别BB
@@ -24,6 +24,6 @@

 defined('MOODLE_INTERNAL') || die();

-$plugin->version   = 2022061000;
+$plugin->version   = 2022082200;
 $plugin->requires  = 2022041200;
 $plugin->component = 'assignfeedback_editpdf';