From e4784db1364533537426a38515a3ed8237be746f Mon Sep 17 00:00:00 2001 From: Mikhail Golenkov Date: Fri, 17 Jun 2022 18:04:17 +1000 Subject: [PATCH] MDL-71468 assignfeedback_editpdf: Convert submissions via adhoc tasks --- config-dist.php | 8 - lib/upgrade.txt | 2 + mod/assign/feedback/editpdf/ajax.php | 12 +- .../editpdf/classes/event/observer.php | 22 +-- .../editpdf/classes/privacy/provider.php | 1 - .../classes/task/convert_submission.php | 77 ++++++++++ .../classes/task/convert_submissions.php | 144 ------------------ mod/assign/feedback/editpdf/db/install.xml | 12 -- mod/assign/feedback/editpdf/db/tasks.php | 40 ----- mod/assign/feedback/editpdf/db/upgrade.php | 24 +++ .../feedback/editpdf/tests/feedback_test.php | 75 +++------ mod/assign/feedback/editpdf/version.php | 2 +- 12 files changed, 130 insertions(+), 289 deletions(-) create mode 100644 mod/assign/feedback/editpdf/classes/task/convert_submission.php delete mode 100644 mod/assign/feedback/editpdf/classes/task/convert_submissions.php delete mode 100644 mod/assign/feedback/editpdf/db/tasks.php diff --git a/config-dist.php b/config-dist.php index 4fde4223506..a431ce4324f 100644 --- a/config-dist.php +++ b/config-dist.php @@ -637,14 +637,6 @@ $CFG->admin = 'admin'; // // $CFG->upgradekey = 'put_some_password-like_value_here'; // -// Document conversion limit -// -// How many times the background task should attempt to convert a given attempt -// before removing it from the queue. Currently this limit is only used by the -// mod_assign conversion task. -// -// $CFG->conversionattemptlimit = 3; -// // Font used in exported PDF files. When generating a PDF, Moodle embeds a subset of // the font in the PDF file so it will be readable on the widest range of devices. // The default font is 'freesans' which is part of the GNU FreeFont collection. diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 903b32b332b..2f7cda01b14 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -243,6 +243,8 @@ defined or can't be applied. user_can_edit_blocks() to return false where necessary. This makes it possible to remove block editing on a page from ALL users, including admins, where required on pages with multi region layouts exist, such as "My courses". * Add an early $CFG->session_redis_acquire_lock_warn option +* Removed $CFG->conversionattemptlimit setting from config.php. assignfeedback_editpdf\task\convert_submissions task + is now replaced with adhoc tasks with standard fail delay approach. === 3.11.4 === * A new option dontforcesvgdownload has been added to the $options parameter of the send_file() function. diff --git a/mod/assign/feedback/editpdf/ajax.php b/mod/assign/feedback/editpdf/ajax.php index 6652e1ab20c..a0eda7c35b3 100644 --- a/mod/assign/feedback/editpdf/ajax.php +++ b/mod/assign/feedback/editpdf/ajax.php @@ -82,21 +82,11 @@ if ($action === 'pollconversions') { $completestatuslist = [combined_document::STATUS_COMPLETE, combined_document::STATUS_FAILED]; if (in_array($response->status, $readystatuslist)) { - // It seems that the files for this submission haven't been combined by the - // "\assignfeedback_editpdf\task\convert_submissions" scheduled task. + // It seems that the files for this submission haven't been combined in cron yet. // Try to combine them in the user session. $combineddocument = document_services::get_combined_pdf_for_attempt($assignment, $userid, $attemptnumber); $response->status = $combineddocument->get_status(); $response->filecount = $combineddocument->get_document_count(); - - // Check status of the combined document and remove the submission - // from the task queue if combination completed. - if (in_array($response->status, $completestatuslist)) { - $submission = $assignment->get_user_submission($userid, false, $attemptnumber); - if ($submission) { - $DB->delete_records('assignfeedback_editpdf_queue', array('submissionid' => $submission->id)); - } - } } if (in_array($response->status, $completestatuslist)) { diff --git a/mod/assign/feedback/editpdf/classes/event/observer.php b/mod/assign/feedback/editpdf/classes/event/observer.php index 0aa8f73d9f3..278a694d5da 100644 --- a/mod/assign/feedback/editpdf/classes/event/observer.php +++ b/mod/assign/feedback/editpdf/classes/event/observer.php @@ -51,20 +51,12 @@ class observer { * @param \mod_assign\event\base $event The submission created/updated event. */ protected static function queue_conversion($event) { - global $DB; - - $submissionid = $event->other['submissionid']; - $submissionattempt = $event->other['submissionattempt']; - $fields = array( 'submissionid' => $submissionid, 'submissionattempt' => $submissionattempt); - $record = (object) $fields; - - $exists = $DB->get_record('assignfeedback_editpdf_queue', $fields); - if (!$exists) { - $DB->insert_record('assignfeedback_editpdf_queue', $record); - } else { - // This submission attempt was already queued, so just reset the existing failure counter to ensure it gets processed. - $exists->attemptedconversions = 0; - $DB->update_record('assignfeedback_editpdf_queue', $exists); - } + $data = [ + 'submissionid' => $event->other['submissionid'], + 'submissionattempt' => $event->other['submissionattempt'], + ]; + $task = new \assignfeedback_editpdf\task\convert_submission; + $task->set_custom_data($data); + \core\task\manager::queue_adhoc_task($task, true); } } diff --git a/mod/assign/feedback/editpdf/classes/privacy/provider.php b/mod/assign/feedback/editpdf/classes/privacy/provider.php index 3ad2fba4fe2..c089b40dc5e 100644 --- a/mod/assign/feedback/editpdf/classes/privacy/provider.php +++ b/mod/assign/feedback/editpdf/classes/privacy/provider.php @@ -175,6 +175,5 @@ class provider implements $DB->delete_records_select('assignfeedback_editpdf_annot', "gradeid $sql", $params); $DB->delete_records_select('assignfeedback_editpdf_cmnt', "gradeid $sql", $params); $DB->delete_records_select('assignfeedback_editpdf_rot', "gradeid $sql", $params); - // Submission records in assignfeedback_editpdf_queue will be cleaned up in a scheduled task } } diff --git a/mod/assign/feedback/editpdf/classes/task/convert_submission.php b/mod/assign/feedback/editpdf/classes/task/convert_submission.php new file mode 100644 index 00000000000..51ec20d8437 --- /dev/null +++ b/mod/assign/feedback/editpdf/classes/task/convert_submission.php @@ -0,0 +1,77 @@ +. + +namespace assignfeedback_editpdf\task; + +use core\task\adhoc_task; +use assignfeedback_editpdf\document_services; +use assignfeedback_editpdf\combined_document; +use context_module; +use assign; + +/** + * An adhoc task to convert submissions to pdf in the background. + * + * @copyright 2022 Mikhail Golenkov + * @package assignfeedback_editpdf + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class convert_submission extends adhoc_task { + + /** + * Run the task. + */ + public function execute() { + global $CFG, $DB; + require_once($CFG->dirroot . '/mod/assign/locallib.php'); + + $data = $this->get_custom_data(); + + $submission = $DB->get_record('assign_submission', ['id' => $data->submissionid], '*', IGNORE_MISSING); + if (!$submission) { + mtrace('Submission no longer exists'); + return; + } + + $cm = get_coursemodule_from_instance('assign', $submission->assignment, 0, false, MUST_EXIST); + $context = context_module::instance($cm->id); + $assign = new assign($context, null, null); + + if ($submission->userid) { + $users = [$submission->userid]; + } else { + $users = []; + $members = $assign->get_submission_group_members($submission->groupid, true); + foreach ($members as $member) { + $users[] = $member->id; + } + } + + foreach ($users as $userid) { + mtrace('Converting submission for user id ' . $userid); + $combineddocument = document_services::get_combined_pdf_for_attempt($assign, $userid, $data->submissionattempt); + $status = $combineddocument->get_status(); + if ($status == combined_document::STATUS_COMPLETE) { + document_services::get_page_images_for_attempt($assign, $userid, $data->submissionattempt, false); + document_services::get_page_images_for_attempt($assign, $userid, $data->submissionattempt, true); + + mtrace('The document has been successfully converted'); + } else { + throw new \coding_exception('Document conversion completed with status ' . $status); + } + } + } +} diff --git a/mod/assign/feedback/editpdf/classes/task/convert_submissions.php b/mod/assign/feedback/editpdf/classes/task/convert_submissions.php deleted file mode 100644 index eb06b2b4d65..00000000000 --- a/mod/assign/feedback/editpdf/classes/task/convert_submissions.php +++ /dev/null @@ -1,144 +0,0 @@ -. - -/** - * A scheduled task. - * - * @package assignfeedback_editpdf - * @copyright 2016 Damyon Wiese - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ -namespace assignfeedback_editpdf\task; - -use core\task\scheduled_task; -use assignfeedback_editpdf\document_services; -use assignfeedback_editpdf\combined_document; -use context_module; -use assign; - -/** - * Simple task to convert submissions to pdf in the background. - * @copyright 2016 Damyon Wiese - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ -class convert_submissions extends scheduled_task { - - /** - * Get a descriptive name for this task (shown to admins). - * - * @return string - */ - public function get_name() { - return get_string('preparesubmissionsforannotation', 'assignfeedback_editpdf'); - } - - /** - * Do the job. - * Throw exceptions on errors (the job will be retried). - */ - public function execute() { - global $CFG, $DB; - - require_once($CFG->dirroot . '/mod/assign/locallib.php'); - - // Conversion speed varies significantly and mostly depends on the documents content. - // We don't want the task to get stuck forever trying to process the whole queue in one go, - // so fetch 100 records only to make sure the task will be working for reasonable time. - // With the task's default schedule, 100 records per run means the task is capable to process - // 9600 conversions per day (100 * 4 * 24). - $records = $DB->get_records('assignfeedback_editpdf_queue', [], '', '*', 0, 100); - - $assignmentcache = array(); - - $conversionattemptlimit = !empty($CFG->conversionattemptlimit) ? $CFG->conversionattemptlimit : 3; - foreach ($records as $record) { - $submissionid = $record->submissionid; - $submission = $DB->get_record('assign_submission', array('id' => $submissionid), '*', IGNORE_MISSING); - if (!$submission || $record->attemptedconversions >= $conversionattemptlimit) { - // Submission no longer exists; or we've exceeded the conversion attempt limit. - $DB->delete_records('assignfeedback_editpdf_queue', array('id' => $record->id)); - continue; - } - - // Record that we're attempting the conversion ahead of time. - // We can't do this afterwards as its possible for the conversion process to crash the script entirely. - $DB->set_field('assignfeedback_editpdf_queue', 'attemptedconversions', - $record->attemptedconversions + 1, ['id' => $record->id]); - - $assignmentid = $submission->assignment; - $attemptnumber = $record->submissionattempt; - - if (empty($assignmentcache[$assignmentid])) { - $cm = get_coursemodule_from_instance('assign', $assignmentid, 0, false, MUST_EXIST); - $context = context_module::instance($cm->id); - - $assignment = new assign($context, null, null); - $assignmentcache[$assignmentid] = $assignment; - } else { - $assignment = $assignmentcache[$assignmentid]; - } - - $users = array(); - if ($submission->userid) { - array_push($users, $submission->userid); - } else { - $members = $assignment->get_submission_group_members($submission->groupid, true); - - foreach ($members as $member) { - array_push($users, $member->id); - } - } - - mtrace('Convert ' . count($users) . ' submission attempt(s) for assignment ' . $assignmentid); - $conversionrequirespolling = false; - - foreach ($users as $userid) { - try { - $combineddocument = document_services::get_combined_pdf_for_attempt($assignment, $userid, $attemptnumber); - 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. - $conversionrequirespolling = true; - continue 2; - } - document_services::get_page_images_for_attempt( - $assignment, - $userid, - $attemptnumber, - false - ); - document_services::get_page_images_for_attempt( - $assignment, - $userid, - $attemptnumber, - true - ); - } catch (\moodle_exception $e) { - mtrace('Conversion failed with error:' . $e->errorcode); - } - } - - // Remove from queue. - if (!$conversionrequirespolling) { - $DB->delete_records('assignfeedback_editpdf_queue', array('id' => $record->id)); - } - - } - } - -} diff --git a/mod/assign/feedback/editpdf/db/install.xml b/mod/assign/feedback/editpdf/db/install.xml index 311541142d1..31e13fe4812 100644 --- a/mod/assign/feedback/editpdf/db/install.xml +++ b/mod/assign/feedback/editpdf/db/install.xml @@ -59,18 +59,6 @@ - - - - - - - - - - - -
diff --git a/mod/assign/feedback/editpdf/db/tasks.php b/mod/assign/feedback/editpdf/db/tasks.php deleted file mode 100644 index de0bf359717..00000000000 --- a/mod/assign/feedback/editpdf/db/tasks.php +++ /dev/null @@ -1,40 +0,0 @@ -. - -/** - * Definition of editpdf scheduled tasks. - * - * @package assignfeedback_editpdf - * @category task - * @copyright 2016 Damyon Wiese - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - -defined('MOODLE_INTERNAL') || die(); - -/* List of handlers */ - -$tasks = array( - array( - 'classname' => 'assignfeedback_editpdf\task\convert_submissions', - 'blocking' => 0, - 'minute' => '*/15', - 'hour' => '*', - 'day' => '*', - 'dayofweek' => '*', - 'month' => '*' - ), -); diff --git a/mod/assign/feedback/editpdf/db/upgrade.php b/mod/assign/feedback/editpdf/db/upgrade.php index 55747adb549..cd82d03c88b 100644 --- a/mod/assign/feedback/editpdf/db/upgrade.php +++ b/mod/assign/feedback/editpdf/db/upgrade.php @@ -95,5 +95,29 @@ function xmldb_assignfeedback_editpdf_upgrade($oldversion) { // Automatically generated Moodle v4.0.0 release upgrade line. // Put any upgrade step following this. + if ($oldversion < 2022061000) { + $table = new xmldb_table('assignfeedback_editpdf_queue'); + if ($dbman->table_exists($table)) { + // Convert not yet converted submissions into adhoc tasks. + $rs = $DB->get_recordset('assignfeedback_editpdf_queue'); + foreach ($rs as $record) { + $data = [ + 'submissionid' => $record->submissionid, + 'submissionattempt' => $record->submissionattempt, + ]; + $task = new assignfeedback_editpdf\task\convert_submission; + $task->set_custom_data($data); + \core\task\manager::queue_adhoc_task($task, true); + } + $rs->close(); + + // Drop the table. + $dbman->drop_table($table); + } + + // Editpdf savepoint reached. + upgrade_plugin_savepoint(true, 2022061000, 'assignfeedback', 'editpdf'); + } + return true; } diff --git a/mod/assign/feedback/editpdf/tests/feedback_test.php b/mod/assign/feedback/editpdf/tests/feedback_test.php index 3a3efa9ef1c..1d2e2f5ddb2 100644 --- a/mod/assign/feedback/editpdf/tests/feedback_test.php +++ b/mod/assign/feedback/editpdf/tests/feedback_test.php @@ -329,14 +329,16 @@ class feedback_test extends \advanced_testcase { $this->assertEmpty($file3); } + /** + * Test Convert submission ad-hoc task. + * + * @covers \assignfeedback_editpdf\task\convert_submission + */ public function test_conversion_task() { - global $DB; $this->require_ghostscript(); $this->resetAfterTest(); cron_setup_user(); - $task = new \assignfeedback_editpdf\task\convert_submissions; - $course = $this->getDataGenerator()->create_course(); $student = $this->getDataGenerator()->create_and_enrol($course, 'student'); $assignopts = [ @@ -351,39 +353,35 @@ class feedback_test extends \advanced_testcase { $this->add_file_submission($student, $assign); // Run the conversion task. + $task = \core\task\manager::get_next_adhoc_task(time()); ob_start(); $task->execute(); + \core\task\manager::adhoc_task_complete($task); $output = ob_get_clean(); - // Verify it acted on both submissions in the queue. - $this->assertStringContainsString("Convert 1 submission attempt(s) for assignment {$assign->get_instance()->id}", $output); - $this->assertEquals(0, $DB->count_records('assignfeedback_editpdf_queue')); - - // Set a known limit. - set_config('conversionattemptlimit', 3); + // Confirm, that submission has been converted and the task queue is now empty. + $this->assertStringContainsString('Converting submission for user id ' . $student->id, $output); + $this->assertStringContainsString('The document has been successfully converted', $output); + $this->assertNull(\core\task\manager::get_next_adhoc_task(time())); // Trigger a re-queue by 'updating' a submission. $submission = $assign->get_user_submission($student->id, true); $plugin = $assign->get_submission_plugin_by_type('file'); $plugin->save($submission, (new \stdClass)); + $task = \core\task\manager::get_next_adhoc_task(time()); // Verify that queued a conversion task. - $this->assertEquals(1, $DB->count_records('assignfeedback_editpdf_queue')); - - // Fake some failed attempts for it. - $queuerecord = $DB->get_record('assignfeedback_editpdf_queue', ['submissionid' => $submission->id]); - $queuerecord->attemptedconversions = 3; - $DB->update_record('assignfeedback_editpdf_queue', $queuerecord); + $this->assertNotNull($task); ob_start(); $task->execute(); + \core\task\manager::adhoc_task_complete($task); $output = ob_get_clean(); - // Verify that the cron task skipped the submission. - $this->assertStringNotContainsString("Convert 1 submission attempt(s) for assignment {$assign->get_instance()->id}", $output); - // And it removed it from the queue. - $this->assertEquals(0, $DB->count_records('assignfeedback_editpdf_queue')); - + // Confirm, that submission has been converted and the task queue is now empty. + $this->assertStringContainsString('Converting submission for user id ' . $student->id, $output); + $this->assertStringContainsString('The document has been successfully converted', $output); + $this->assertNull(\core\task\manager::get_next_adhoc_task(time())); } /** @@ -517,41 +515,4 @@ class feedback_test extends \advanced_testcase { // No modification. $this->assertFalse($plugin->is_feedback_modified($grade, $data)); } - - /** - * Test Convert submissions scheduled task limit. - * - * @covers \assignfeedback_editpdf\task\convert_submissions - */ - public function test_conversion_task_limit() { - global $DB; - $this->require_ghostscript(); - $this->resetAfterTest(); - cron_setup_user(); - - $course = $this->getDataGenerator()->create_course(); - $assignopts = [ - 'assignsubmission_file_enabled' => 1, - 'assignsubmission_file_maxfiles' => 1, - 'assignfeedback_editpdf_enabled' => 1, - 'assignsubmission_file_maxsizebytes' => 1000000, - ]; - $assign = $this->create_instance($course, $assignopts); - - // Generate 110 submissions. - for ($i = 0; $i < 110; $i++) { - $student = $this->getDataGenerator()->create_and_enrol($course, 'student'); - $this->add_file_submission($student, $assign); - } - $this->assertEquals(110, $DB->count_records('assignfeedback_editpdf_queue')); - - // Run the conversion task. - $task = new \assignfeedback_editpdf\task\convert_submissions; - ob_start(); - $task->execute(); - ob_end_clean(); - - // Confirm, that 100 records were processed and 10 were left for the next task run. - $this->assertEquals(10, $DB->count_records('assignfeedback_editpdf_queue')); - } } diff --git a/mod/assign/feedback/editpdf/version.php b/mod/assign/feedback/editpdf/version.php index e07c820e5fe..d7cd8ba4cc5 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 = 2022041900; +$plugin->version = 2022061000; $plugin->requires = 2022041200; $plugin->component = 'assignfeedback_editpdf';