From e5e8bc4b22b54c20e7de31440e392f55392bb032 Mon Sep 17 00:00:00 2001 From: Mikhail Golenkov Date: Thu, 18 Aug 2022 11:44:19 +1000 Subject: [PATCH] MDL-75448 assignfeedback_editpdf: Fix conversion for rotated images --- .../editpdf/classes/document_services.php | 11 +- .../editpdf/tests/document_services_test.php | 165 ++++++++++++++++++ 2 files changed, 172 insertions(+), 4 deletions(-) create mode 100644 mod/assign/feedback/editpdf/tests/document_services_test.php diff --git a/mod/assign/feedback/editpdf/classes/document_services.php b/mod/assign/feedback/editpdf/classes/document_services.php index f417141596a..6fd49f44fe0 100644 --- a/mod/assign/feedback/editpdf/classes/document_services.php +++ b/mod/assign/feedback/editpdf/classes/document_services.php @@ -916,10 +916,13 @@ EOD; $oldfile = $fs->get_file($record->contextid, $record->component, $record->filearea, $record->itemid, $record->filepath, $record->filename); - $newhash = sha1($newfilepath); - - // Delete old file if exists. - if ($oldfile && $newhash !== $oldfile->get_contenthash()) { + if ($oldfile) { + $newhash = \file_storage::hash_from_path($newfilepath); + if ($newhash === $oldfile->get_contenthash()) { + // Use existing file if contenthash match. + return $oldfile; + } + // Delete existing file. $oldfile->delete(); } diff --git a/mod/assign/feedback/editpdf/tests/document_services_test.php b/mod/assign/feedback/editpdf/tests/document_services_test.php new file mode 100644 index 00000000000..e39ec8accf0 --- /dev/null +++ b/mod/assign/feedback/editpdf/tests/document_services_test.php @@ -0,0 +1,165 @@ +. + +namespace assignfeedback_editpdf; + +use mod_assign_test_generator; +use advanced_testcase; +use ReflectionMethod; + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/mod/assign/tests/generator.php'); + +/** + * Unit tests for document services. + * + * @package assignfeedback_editpdf + * @category test + * @covers \assignfeedback_editpdf\document_services + * @copyright 2022 Mikhail Golenkov + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class document_services_test extends advanced_testcase { + use mod_assign_test_generator; + + /** + * Test that the save file method saves the file. + * @covers ::save_file() + */ + public function test_save_file_saves_the_file() { + global $DB; + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course(); + $assign = $this->create_instance($course); + $user = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($user->id, $course->id, 'student'); + + $method = new ReflectionMethod('\assignfeedback_editpdf\document_services', 'save_file'); + $method->setAccessible(true); + + $filearea = document_services::TMP_ROTATED_JPG_FILEAREA; + $content = 'some random content'; + $tempfile = make_temp_directory('assignfeedback_editpdf') . DIRECTORY_SEPARATOR . 'mock.file'; + file_put_contents($tempfile, $content); + + // Invoke the method and confirm, that the file is saved. + $file1 = $method->invoke(null, $assign, $user->id, 1, $filearea, $tempfile); + $this->assertInstanceOf('stored_file', $file1); + $this->assertEquals(1, $DB->count_records('files', ['id' => $file1->get_id()])); + + // Invoke the method again and confirm, that exising file is returned. + $file2 = $method->invoke(null, $assign, $user->id, 1, $filearea, $tempfile); + $this->assertEquals($file1->get_id(), $file2->get_id()); + } + + /** + * Test that save_rotated_image_file() method saves the file. + * @covers ::save_rotated_image_file() + */ + public function test_save_rotated_image_file_saves_the_file() { + global $CFG, $DB; + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course(); + $assign = $this->create_instance($course); + $user = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($user->id, $course->id, 'student'); + + $method = new ReflectionMethod('\assignfeedback_editpdf\document_services', 'save_rotated_image_file'); + $method->setAccessible(true); + + $imagecontent = file_get_contents($CFG->dirroot . '/lib/filestorage/tests/fixtures/testimage.png'); + $imageresource = imagecreatefromstring($imagecontent); + + // Invoke the method and confirm, that the file is saved. + $file1 = $method->invoke(null, $assign, $user->id, 1, $imageresource, 'testimage.png'); + $this->assertInstanceOf('stored_file', $file1); + $this->assertEquals(1, $DB->count_records('files', ['id' => $file1->get_id()])); + + // Invoke the method again and confirm, that exising file is returned. + $file2 = $method->invoke(null, $assign, $user->id, 1, $imageresource, 'testimage.png'); + $this->assertEquals($file1->get_id(), $file2->get_id()); + } + + /** + * Test that get_combined_document_for_attempt() method rotates the image only once. + * @covers ::get_combined_document_for_attempt() + */ + public function test_get_combined_document_for_attempt_rotates_image() { + global $CFG, $DB; + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course(); + $assignparams = [ + 'assignsubmission_file_enabled' => 1, + 'assignsubmission_file_maxfiles' => 1, + 'assignsubmission_file_maxsizebytes' => 1024 * 1024, + ]; + $assign = $this->create_instance($course, $assignparams); + $student = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($student->id, $course->id, 'student'); + $this->setUser($student); + + $notices = []; + $submission = $assign->get_user_submission($student->id, true, 1); + $data = (object) ['files_filemanager' => $submission->id]; + $assign->save_submission($data, $notices); + + // This image was manually rotated to be upside down. Also, Orientation, ExifImageWidth + // and ExifImageLength EXIF tags were written into its metadata. + // This is needed to make sure that this image will be rotated by stored_file::rotate_image() + // and stored as a new rotated file. + $filename = 'testimage_rotated.jpg'; + $filepath = $CFG->dirroot . '/lib/filestorage/tests/fixtures/' . $filename; + $filerecord = [ + 'contextid' => $assign->get_context()->id, + 'component' => 'assignsubmission_file', + 'filearea' => ASSIGNSUBMISSION_FILE_FILEAREA, + 'itemid' => $submission->id, + 'filepath' => '/', + 'filename' => $filename, + ]; + $fs = get_file_storage(); + $fs->create_file_from_pathname($filerecord, $filepath); + + $params = [ + 'filearea' => document_services::TMP_ROTATED_JPG_FILEAREA, + 'component' => document_services::COMPONENT, + 'filename' => $filename, + ]; + + // Combine the document and get the rotated file. + document_services::get_combined_document_for_attempt($assign, $student->id, 1); + $records = $DB->get_records('files', $params); + $this->assertCount(1, $records); + $record1 = reset($records); + + // Polling file converters do this twice: one call to start a conversion and another one + // to poll the converted file. So we combine the document again here. + document_services::get_combined_document_for_attempt($assign, $student->id, 1); + $records = $DB->get_records('files', $params); + $this->assertCount(1, $records); + $record2 = reset($records); + + // Confirm, that the second get_combined_document_for_attempt() call doesn't create new + // rotated file and re-uses the one that was created as part of the first + // get_combined_document_for_attempt() call. + $this->assertEquals($record1->id, $record2->id); + } +}