From 4dec38cf49d9db1b77aa8e86e33f363050dde53a Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Mon, 14 Oct 2024 19:05:07 +0200 Subject: [PATCH 01/24] Refactoring: - Use constants from tables class only. - Change some subqueries to joins. - Static variables for verbal feedback instances, when fetching by category or criterion. --- classes/api.php | 122 ++++++++---------- classes/repository/instance_repository.php | 5 +- classes/repository/submission_repository.php | 2 +- classes/repository/tables.php | 4 + .../template_category_repository.php | 28 ++-- 5 files changed, 75 insertions(+), 86 deletions(-) diff --git a/classes/api.php b/classes/api.php index c29753f..630999e 100644 --- a/classes/api.php +++ b/classes/api.php @@ -29,21 +29,14 @@ use dml_exception; use Exception; use moodle_exception; -use stdClass; -use core_date; -use mod_verbalfeedback\api\gradebook; use mod_verbalfeedback\model\response; use mod_verbalfeedback\model\submission; use mod_verbalfeedback\model\submission_status; use mod_verbalfeedback\repository\instance_repository; use mod_verbalfeedback\repository\submission_repository; use mod_verbalfeedback\repository\tables; -use mod_verbalfeedback\utils\user; -use mod_verbalfeedback\utils\instance; use mod_verbalfeedback\utils\user_utils; -// Quick hack for issue #43. -ini_set('memory_limit', '256M'); /** * Class for performing DB actions for the verbal feedback activity module. @@ -53,13 +46,6 @@ */ class api { - /** The instance database table. */ - const DB_INSTANCE = "verbalfeedback"; - /** The response database table. */ - const DB_RESPONSE = "verbalfeedback_response"; - /** The submission database table. */ - const DB_SUBMISSION = "verbalfeedback_submission"; - /** Status when a user has not yet provided feedback to another user. */ const STATUS_PENDING = 0; /** Status when a user has begun providing feedback to another user. */ @@ -101,8 +87,8 @@ public static function get_instance($verbalfeedbackid) { global $DB; $id = (int)$verbalfeedbackid; - if (!array_key_exists($id, static::$instances)) { - static::$instances[$id] = $DB->get_record('verbalfeedback', ['id' => $id], '*', MUST_EXIST); + if (!array_key_exists($id, static::$instances) && !PHPUNIT_TEST) { + static::$instances[$id] = $DB->get_record(tables::INSTANCE_TABLE, ['id' => $id], '*', MUST_EXIST); } return static::$instances[$id]; } @@ -127,14 +113,19 @@ public static function is_ready($verbalfeedbackid) { */ public static function get_instance_by_itemid($itemid) { global $DB; - return $DB->get_record_sql("SELECT * - FROM {" . tables::INSTANCE_TABLE . - "} WHERE id = (SELECT instanceid - FROM {" . tables::INSTANCE_CATEGORY_TABLE . - "} WHERE id = (SELECT categoryid - FROM {" . tables::INSTANCE_CRITERION_TABLE . - "} WHERE id = ?))", - [$itemid], IGNORE_MISSING); + [$vftable, $cattable, $crittable] = [tables::INSTANCE_TABLE, tables::INSTANCE_CATEGORY_TABLE, tables::INSTANCE_CRITERION_TABLE]; + $id = (int)$itemid; + static $instances = []; + if (isset($instances[$id])) { + return $instances[$id]; + } + $instances[$id] = $DB->get_record_sql(" + SELECT * FROM {{$vftable} vf + INNER JOIN {{$cattable}} cat ON cat.instanceid = vf.id + INNER JOIN {{$crittable}} crit ON crit.categoryid = cat.id + WHERE crit.id = ?", + [$id], IGNORE_MISSING); + return $instances[$id]; } /** @@ -146,12 +137,18 @@ public static function get_instance_by_itemid($itemid) { */ public static function get_instance_by_categoryid($categoryid) { global $DB; - return $DB->get_record_sql("SELECT * - FROM {" . tables::INSTANCE_TABLE . - "} WHERE id = (SELECT instanceid - FROM {" . tables::INSTANCE_CATEGORY_TABLE . - "} WHERE id = ?)", - [$categoryid], IGNORE_MISSING); + [$vftable, $cattable] = [tables::INSTANCE_TABLE, tables::INSTANCE_CATEGORY_TABLE]; + $id = (int)$categoryid; + static $instances = []; + if (isset($instances[$id])) { + return $instances[$id]; + } + $instances[$id] = $DB->get_record_sql(" + SELECT * FROM {{$vftable}} vf + INNER JOIN {{$cattable}} cat ON cat.instanceid = vf.id + WHERE cat.id = ?", + [$id], IGNORE_MISSING); + return $instances[$id]; } /** @@ -237,7 +234,7 @@ public static function generate_verbalfeedback_feedback_states($verbalfeedbackid global $DB; $submissionrepo = new submission_repository(); - $verbalfeedback = $DB->get_record(self::DB_INSTANCE, ['id' => $verbalfeedbackid], '*', MUST_EXIST); + $verbalfeedback = self::get_instance($verbalfeedbackid); $wheres = [ 'u.id NOT IN ( SELECT fs.touserid @@ -309,42 +306,29 @@ function($user) { */ public static function get_scales() { - $s0 = new stdClass(); - $s0->scale = null; - $s0->scalelabel = get_string('notapplicableabbr', 'verbalfeedback'); - $s0->description = get_string('scalenotapplicable', 'mod_verbalfeedback'); - - $s1 = new stdClass(); - $s1->scale = 0; - $s1->scalelabel = '0'; - $s1->description = get_string('scalestronglydisagree', 'mod_verbalfeedback'); - - $s2 = new stdClass(); - $s2->scale = 1; - $s2->scalelabel = '1'; - $s2->description = get_string('scaledisagree', 'mod_verbalfeedback'); - - $s3 = new stdClass(); - $s3->scale = 2; - $s3->scalelabel = '2'; - $s3->description = get_string('scalesomewhatdisagree', 'mod_verbalfeedback'); - - $s4 = new stdClass(); - $s4->scale = 3; - $s4->scalelabel = '3'; - $s4->description = get_string('scalesomewhatagree', 'mod_verbalfeedback'); - - $s5 = new stdClass(); - $s5->scale = 4; - $s5->scalelabel = '4'; - $s5->description = get_string('scaleagree', 'mod_verbalfeedback'); - - $s6 = new stdClass(); - $s6->scale = 5; - $s6->scalelabel = '5'; - $s6->description = get_string('scalestronglyagree', 'mod_verbalfeedback'); - - return [$s1, $s2, $s3, $s4, $s5, $s6, $s0]; + $wordscale = [ + 'scalestronglydisagree', + 'scaledisagree', + 'scalesomewhatdisagree', + 'scalesomewhatagree', + 'scaleagree', + 'scalestronglyagree', + ]; + $scales = []; + for ($i = 0; $i <= 5; $i++) { + $scales[] = (object)[ + 'scale' => $i, + 'scalelabel' => '' . $i, + 'description' => get_string($wordscale[$i], 'mod_verbalfeedback'), + ]; + } + $scales[] = (object)[ + 'scale' => null, + 'scalelabel' => get_string('notapplicableabbr', 'verbalfeedback'), + 'description' => get_string('scalenotapplicable', 'mod_verbalfeedback'), + ]; + + return $scales; } /** @@ -463,7 +447,7 @@ public static function count_users_awaiting_feedback($verbalfeedbackid, $user) { // Check first if the user can write feedback to other participants. if (user_utils::can_respond($verbalfeedback, $user) === true) { - if (!$DB->record_exists(self::DB_SUBMISSION, ['instance' => $verbalfeedback->id, 'fromuser' => $user])) { + if (!$DB->record_exists(tables::SUBMISSION_TABLE, ['instance' => $verbalfeedback->id, 'fromuser' => $user])) { // Generate submission records if there are no submission records yet. self::generate_verbalfeedback_feedback_states($verbalfeedback->id, $user, $verbalfeedback->with_self_review); } @@ -473,7 +457,7 @@ public static function count_users_awaiting_feedback($verbalfeedbackid, $user) { $select = "instance = :verbalfeedback AND fromuser = :fromuser AND status $insql"; $params['verbalfeedback'] = $verbalfeedbackid; $params['fromuser'] = $user; - return $DB->count_records_select(self::DB_SUBMISSION, $select, $params); + return $DB->count_records_select(tables::SUBMISSION_TABLE, $select, $params); } return 0; diff --git a/classes/repository/instance_repository.php b/classes/repository/instance_repository.php index a46bfee..d28b1fd 100644 --- a/classes/repository/instance_repository.php +++ b/classes/repository/instance_repository.php @@ -27,6 +27,7 @@ defined('MOODLE_INTERNAL') || die(); use Exception; +use mod_verbalfeedback\api; use mod_verbalfeedback\model\instance; use mod_verbalfeedback\model\instance_category; use mod_verbalfeedback\model\instance_status; @@ -46,7 +47,7 @@ class instance_repository { /** * Gets the instance with the given id - * @param int $id The language id. + * @param int $id The verbal feedback id. * @return instance|null The language. */ public static function get_by_id(int $id): instance { @@ -58,7 +59,7 @@ public static function get_by_id(int $id): instance { return $byid[$id]; } - $dboinstance = $DB->get_record(tables::INSTANCE_TABLE, ["id" => $id]); + $dboinstance = api::get_instance($id); $instance = db_instance::to_instance($dboinstance); $dbocategories = $DB->get_records(tables::INSTANCE_CATEGORY_TABLE, ["instanceid" => $id]); diff --git a/classes/repository/submission_repository.php b/classes/repository/submission_repository.php index 53c967f..56a2bbf 100644 --- a/classes/repository/submission_repository.php +++ b/classes/repository/submission_repository.php @@ -81,7 +81,7 @@ public function save(submission $submission) { */ public function delete_by_id(int $id): bool { global $DB; - return $DB->delete_records('verbalfeedback_language', ['id' => $id]); + return $DB->delete_records(tables::LANGUAGE_TABLE, ['id' => $id]); } /** diff --git a/classes/repository/tables.php b/classes/repository/tables.php index 4c1305a..8a2e6fb 100644 --- a/classes/repository/tables.php +++ b/classes/repository/tables.php @@ -56,6 +56,10 @@ class tables { * */ const TEMPLATE_TABLE = 'verbalfeedback_template'; + /** + * Template category table name. + */ + const TEMPLATE_CATEGORY_TABLE = 'verbalfeedback_t_category'; /** * */ diff --git a/classes/repository/template_category_repository.php b/classes/repository/template_category_repository.php index f2b0881..f958821 100644 --- a/classes/repository/template_category_repository.php +++ b/classes/repository/template_category_repository.php @@ -51,7 +51,7 @@ public function get_all(): array { } $results = []; - $rs = $DB->get_recordset("verbalfeedback_t_category"); + $rs = $DB->get_recordset(tables::TEMPLATE_CATEGORY_TABLE); $templatecategories = []; foreach ($rs as $dbocategory) { $templatecategory = db_template_category::to_template_category($dbocategory); @@ -97,10 +97,10 @@ public function save(template_category $templatecategory) { $transaction = $DB->start_delegated_transaction(); $dbocategory = db_template_category::from_template_category($templatecategory); if ($templatecategory->get_id() === null || $templatecategory->get_id() == 0) { - $id = $DB->insert_record('verbalfeedback_t_category', $dbocategory); + $id = $DB->insert_record(tables::TEMPLATE_CATEGORY_TABLE, $dbocategory); $templatecategory->set_id($id); } else { - $DB->update_record('verbalfeedback_t_category', $dbocategory); + $DB->update_record(tables::TEMPLATE_CATEGORY_TABLE, $dbocategory); } // Insert or update category headers. @@ -109,18 +109,18 @@ public function save(template_category $templatecategory) { localized_string_type::TEMPLATE_CATEGORY_HEADER, $templatecategory->get_id()); if ($header->get_id() == 0) { - $DB->insert_record('verbalfeedback_local_string', $dboheader); + $DB->insert_record(tables::LOCALIZED_STRING_TABLE, $dboheader); } else { - $DB->update_record('verbalfeedback_local_string', $dboheader); + $DB->update_record(tables::LOCALIZED_STRING_TABLE, $dboheader); } } // Update linked criteria. - $DB->delete_records('verbalfeedback_t_param_crit', ['categoryid' => $templatecategory->get_id()]); + $DB->delete_records(tables::PARAMETRIZED_TEMPLATE_CRITERION_TABLE, ['categoryid' => $templatecategory->get_id()]); foreach ($templatecategory->get_template_criteria() as $criterion) { $dboparamcriterion = db_parametrized_criterion::from_parametrized_criterion($criterion, $templatecategory->get_id()); - $DB->insert_record('verbalfeedback_t_param_crit', $dboparamcriterion); + $DB->insert_record(tables::PARAMETRIZED_TEMPLATE_CRITERION_TABLE, $dboparamcriterion); } $transaction->allow_commit(); @@ -140,10 +140,10 @@ public function delete_by_id(int $id): bool { global $DB; try { $transaction = $DB->start_delegated_transaction(); - $DB->delete_records('verbalfeedback_local_string', ['foreignkey' => $id, + $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $id, 'type' => localized_string_type::TEMPLATE_CATEGORY_HEADER, ]); - $DB->delete_records('verbalfeedback_t_param_crit', ['categoryid' => $id]); - $DB->delete_records('verbalfeedback_t_category', ['id' => $id]); + $DB->delete_records(tables::PARAMETRIZED_TEMPLATE_CRITERION_TABLE, ['categoryid' => $id]); + $DB->delete_records(tables::TEMPLATE_CATEGORY_TABLE, ['id' => $id]); $transaction->allow_commit(); } catch (Exception $e) { @@ -162,7 +162,7 @@ public function delete_by_id(int $id): bool { private function get_headers($foreignkey): array { global $DB; - $dboheaders = $DB->get_records('verbalfeedback_local_string', + $dboheaders = $DB->get_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $foreignkey, 'type' => localized_string_type::TEMPLATE_CATEGORY_HEADER]); $headers = []; foreach ($dboheaders as $dboheader) { @@ -182,7 +182,7 @@ private function get_headers_by_category_ids(): array { global $DB; $headers = []; - $rs = $DB->get_recordset('verbalfeedback_local_string', ['type' => localized_string_type::TEMPLATE_CATEGORY_HEADER]); + $rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE, ['type' => localized_string_type::TEMPLATE_CATEGORY_HEADER]); foreach ($rs as $row) { if (!\array_key_exists($row->foreignkey, $headers)) { $headers[$row->foreignkey] = []; @@ -203,7 +203,7 @@ private function get_headers_by_category_ids(): array { private function get_parametrized_criteria($categoryid): array { global $DB; - $dboparametrizedcriteria = $DB->get_records('verbalfeedback_t_param_crit', ['categoryid' => $categoryid]); + $dboparametrizedcriteria = $DB->get_records(tables::PARAMETRIZED_TEMPLATE_CRITERION_TABLE, ['categoryid' => $categoryid]); $parametrizedcriteria = []; foreach ($dboparametrizedcriteria as $dboparametrizedcriterion) { $parametrizedcriteria[] = db_parametrized_criterion::to_parametrized_criterion($dboparametrizedcriterion); @@ -220,7 +220,7 @@ private function get_parameterized_criteria_by_categoryid(): array { global $DB; $critbycatid = []; - $rs = $DB->get_recordset('verbalfeedback_t_param_crit'); + $rs = $DB->get_recordset(tables::PARAMETRIZED_TEMPLATE_CRITERION_TABLE); foreach ($rs as $row) { if (!\array_key_exists($row->categoryid, $critbycatid)) { $critbycatid[$row->categoryid] = []; From e7b81abd8562ed3368d504f6ccdb6333ba778340 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Thu, 17 Oct 2024 18:28:42 +0200 Subject: [PATCH 02/24] Fetch strings only by type and subratingid. --- classes/repository/instance_repository.php | 24 ++++++++-------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/classes/repository/instance_repository.php b/classes/repository/instance_repository.php index d28b1fd..1f13ebf 100644 --- a/classes/repository/instance_repository.php +++ b/classes/repository/instance_repository.php @@ -29,7 +29,6 @@ use Exception; use mod_verbalfeedback\api; use mod_verbalfeedback\model\instance; -use mod_verbalfeedback\model\instance_category; use mod_verbalfeedback\model\instance_status; use mod_verbalfeedback\repository\model\db_instance; use mod_verbalfeedback\repository\model\db_instance_category; @@ -176,11 +175,12 @@ public static function get_by_id(int $id): instance { private static function get_strings(string $type, int $subratingid, bool $throwonerror = false): array { global $DB; - static $sortedstrings = null; + static $sortedstrings = []; + $cachekey = $type . '~~' . $subratingid; - if ($sortedstrings === null || PHPUNIT_TEST) { - $sortedstrings = []; - $rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE); + if (!array_key_exists($cachekey, $sortedstrings) || PHPUNIT_TEST) { + $sortedstrings[$cachekey] = []; + $rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE, ['type' => $type, 'foreignkey' => $subratingid]); foreach ($rs as $dboheader) { $dbobj = new db_localized_string; $dbobj->id = $dboheader->id; @@ -189,25 +189,19 @@ private static function get_strings(string $type, int $subratingid, bool $throwo $dbobj->type = $dboheader->type; $dbobj->foreignkey = $dboheader->foreignkey; - $sortedstrings[$dbobj->type][$dbobj->foreignkey][$dbobj->languageid] = $dbobj; + $sortedstrings[$cachekey][$dbobj->languageid] = $dbobj; } $rs->close(); } - if (!isset($sortedstrings[$type])) { + if (empty($sortedstrings[$cachekey])) { if ($throwonerror) { - throw new \coding_exception("Invalid type .$type"); - } - return []; - } - if (!isset($sortedstrings[$type][$subratingid])) { - if ($throwonerror) { - throw new \coding_exception("Invalid subratingid $subratingid for type $type"); + throw new \coding_exception("No strings for type $type and subratingid $subratingid"); } return []; } - return $sortedstrings[$type][$subratingid]; + return $sortedstrings[$cachekey]; } /** From 4b6bb4630bdff74b467a4249b8e30c6b05970f64 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Thu, 17 Oct 2024 18:29:13 +0200 Subject: [PATCH 03/24] Remove quick hack to increase memory limit. --- edit_instance.php | 3 --- questionnaire.php | 3 --- report.php | 3 --- report_download.php | 3 --- view.php | 3 --- 5 files changed, 15 deletions(-) diff --git a/edit_instance.php b/edit_instance.php index bb0336a..c4e8206 100644 --- a/edit_instance.php +++ b/edit_instance.php @@ -49,9 +49,6 @@ require_login($course, true, $cm); -// Quick hack for issue #43. -ini_set('memory_limit', '256M'); - $instancerepository = new instance_repository(); if (!$instance = $instancerepository->get_by_id($cm->instance)) { diff --git a/questionnaire.php b/questionnaire.php index c337379..c97b003 100644 --- a/questionnaire.php +++ b/questionnaire.php @@ -45,9 +45,6 @@ require_login($course, true, $cm); -// Quick hack for issue #43. -ini_set('memory_limit', '256M'); - $context = context_module::instance($cm->id); $instancerepository = new instance_repository(); $submissionrepository = new submission_repository(); diff --git a/report.php b/report.php index 0f12c0c..5c55592 100644 --- a/report.php +++ b/report.php @@ -36,9 +36,6 @@ require_login($course, true, $cm); -// Quick hack for issue #43. -ini_set('memory_limit', '256M'); - $context = context_module::instance($cm->id); $instancerepo = new instance_repository(); diff --git a/report_download.php b/report_download.php index 8dc9c3d..7be2174 100644 --- a/report_download.php +++ b/report_download.php @@ -54,9 +54,6 @@ throw new moodle_exception('errorreportnotavailable', 'mod_verbalfeedback'); } -// Quick hack for issue #43. -ini_set('memory_limit', '256M'); - $PAGE->set_context($context); $PAGE->set_cm($cm, $course); $PAGE->set_pagelayout('incourse'); diff --git a/view.php b/view.php index 220979a..86bf48b 100644 --- a/view.php +++ b/view.php @@ -39,9 +39,6 @@ $instancerepository = new instance_repository(); -// Quick hack for issue #43. -ini_set('memory_limit', '256M'); - $context = context_module::instance($cm->id); $instance = $instancerepository->get_by_id($cm->instance); From ef0a12bcfff4b443615d47f0614fc347c7d6b2a0 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Tue, 22 Oct 2024 17:48:40 +0200 Subject: [PATCH 04/24] Change column type from type to typeid in languages. Optimize string fetching for templates. --- classes/repository/instance_repository.php | 25 ++--- .../repository/model/db_localized_string.php | 6 +- .../model/localized_string_type.php | 92 +++++++++++-------- .../template_category_repository.php | 13 ++- .../template_criterion_repository.php | 48 +++++----- db/upgrade.php | 24 +++++ version.php | 2 +- 7 files changed, 129 insertions(+), 81 deletions(-) diff --git a/classes/repository/instance_repository.php b/classes/repository/instance_repository.php index 1f13ebf..8b9ae60 100644 --- a/classes/repository/instance_repository.php +++ b/classes/repository/instance_repository.php @@ -180,13 +180,16 @@ private static function get_strings(string $type, int $subratingid, bool $throwo if (!array_key_exists($cachekey, $sortedstrings) || PHPUNIT_TEST) { $sortedstrings[$cachekey] = []; - $rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE, ['type' => $type, 'foreignkey' => $subratingid]); + $rs = $DB->get_recordset( + tables::LOCALIZED_STRING_TABLE, + ['typeid' => localized_string_type::str2id($type), 'foreignkey' => $subratingid] + ); foreach ($rs as $dboheader) { $dbobj = new db_localized_string; $dbobj->id = $dboheader->id; $dbobj->languageid = $dboheader->languageid; $dbobj->string = $dboheader->string; - $dbobj->type = $dboheader->type; + $dbobj->typeid = $dboheader->typeid; $dbobj->foreignkey = $dboheader->foreignkey; $sortedstrings[$cachekey][$dbobj->languageid] = $dbobj; @@ -277,7 +280,7 @@ public function save(instance $instance) { $id = $DB->insert_record(tables::INSTANCE_TABLE, $dboinstance); $instance->set_id($id); // Set the grade. - $DB->set_field('verbalfeedback', 'grade', $instance->grade, ['id' => $id]); + $DB->set_field(tables::INSTANCE_TABLE, 'grade', $instance->grade, ['id' => $id]); } else { $DB->update_record(tables::INSTANCE_TABLE, $dboinstance); } @@ -408,7 +411,7 @@ public function delete(int $instanceid) { foreach ($dbocategories as $dbocategory) { // Delete category headers. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_CATEGORY_HEADER, "foreignkey" => $dbocategory->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_CATEGORY_HEADER), "foreignkey" => $dbocategory->id]); // Delete category criteria. $dbocriteria = $DB->get_records(tables::INSTANCE_CRITERION_TABLE, ["categoryid" => $dbocategory->id]); @@ -416,7 +419,7 @@ public function delete(int $instanceid) { // Delete criterion description. $dbodescriptions = $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_CRITERION, "foreignkey" => $dbocriterion->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_CRITERION), "foreignkey" => $dbocriterion->id]); // Delete criterion subratings. $dbosubratings = $DB->get_records(tables::INSTANCE_SUBRATING_TABLE, ["criterionid" => $dbocriterion->id]); @@ -424,27 +427,27 @@ public function delete(int $instanceid) { // Delete subrating titles. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_SUBRATING_TITLE, "foreignkey" => $dbosubrating->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_TITLE), "foreignkey" => $dbosubrating->id]); // Delete subrating descriptions. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_SUBRATING_DESCRIPTION, "foreignkey" => $dbosubrating->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_DESCRIPTION), "foreignkey" => $dbosubrating->id]); // Delete subrating very negative texts. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_SUBRATING_VERY_NEGATIVE, "foreignkey" => $dbosubrating->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_VERY_NEGATIVE), "foreignkey" => $dbosubrating->id]); // Delete subrating negative texts. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_SUBRATING_NEGATIVE, "foreignkey" => $dbosubrating->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_NEGATIVE), "foreignkey" => $dbosubrating->id]); // Delete subrating positive texts. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_SUBRATING_POSITIVE, "foreignkey" => $dbosubrating->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_POSITIVE), "foreignkey" => $dbosubrating->id]); // Delete subrating very positive texts. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE, "foreignkey" => $dbosubrating->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE), "foreignkey" => $dbosubrating->id]); } // Delete subratings. diff --git a/classes/repository/model/db_localized_string.php b/classes/repository/model/db_localized_string.php index 9ff237e..f3c14dc 100644 --- a/classes/repository/model/db_localized_string.php +++ b/classes/repository/model/db_localized_string.php @@ -34,9 +34,9 @@ class db_localized_string { */ public $id; /** - * @var string The type + * @var int The type */ - public $type; + public $typeid; /** * @var int The foreign key */ @@ -66,7 +66,7 @@ public static function from_localized_string(localized_string $localizedstring, } $dbo = new db_localized_string(); $dbo->id = $localizedstring->get_id(); - $dbo->type = $type; + $dbo->typeid = localized_string_type::str2id($type); $dbo->foreignkey = $foreignkey; $dbo->languageid = $localizedstring->get_language_id(); $dbo->string = $localizedstring->get_string(); diff --git a/classes/repository/model/localized_string_type.php b/classes/repository/model/localized_string_type.php index 4c028c2..531cac0 100644 --- a/classes/repository/model/localized_string_type.php +++ b/classes/repository/model/localized_string_type.php @@ -62,48 +62,68 @@ class localized_string_type { public const TEMPLATE_SUBRATING_VERY_POSITIVE = 'template_subrating_verypositive'; /** - * Return whether a type exists + * Return all existing types in a order, so that the numeric value + 1 can + * be used as a string. + * + * @return string[] + */ + public static function getStringTypes(): array { + return [ + self::INSTANCE_CRITERION, + self::INSTANCE_CATEGORY_HEADER, + self::INSTANCE_SUBRATING_TITLE, + self::INSTANCE_SUBRATING_DESCRIPTION, + self::INSTANCE_SUBRATING_VERY_NEGATIVE, + self::INSTANCE_SUBRATING_NEGATIVE, + self::INSTANCE_SUBRATING_POSITIVE, + self::INSTANCE_SUBRATING_VERY_POSITIVE, + self::TEMPLATE_CRITERION, + self::TEMPLATE_CATEGORY_HEADER, + self::TEMPLATE_SUBRATING_TITLE, + self::TEMPLATE_SUBRATING_DESCRIPTION, + self::TEMPLATE_SUBRATING_VERY_NEGATIVE, + self::TEMPLATE_SUBRATING_NEGATIVE, + self::TEMPLATE_SUBRATING_POSITIVE, + self::TEMPLATE_SUBRATING_VERY_POSITIVE, + ]; + } + /** + * Return whether a type string exists. + * * @param string $type A string type * @return bool If a string type exists */ public static function exists(string $type) { - switch($type) { - case self::INSTANCE_CRITERION: - return true; - case self::INSTANCE_CATEGORY_HEADER: - return true; - case self::INSTANCE_SUBRATING_TITLE: - return true; - case self::INSTANCE_SUBRATING_DESCRIPTION: - return true; - case self::INSTANCE_SUBRATING_VERY_NEGATIVE: - return true; - case self::INSTANCE_SUBRATING_NEGATIVE: - return true; - case self::INSTANCE_SUBRATING_POSITIVE: - return true; - case self::INSTANCE_SUBRATING_VERY_POSITIVE: - return true; + return in_array($type, self::getStringTypes()); + } - case self::TEMPLATE_CRITERION: - return true; - case self::TEMPLATE_CATEGORY_HEADER: - return true; - case self::TEMPLATE_SUBRATING_TITLE: - return true; - case self::TEMPLATE_SUBRATING_DESCRIPTION: - return true; - case self::TEMPLATE_SUBRATING_VERY_NEGATIVE: - return true; - case self::TEMPLATE_SUBRATING_NEGATIVE: - return true; - case self::TEMPLATE_SUBRATING_POSITIVE: - return true; - case self::TEMPLATE_SUBRATING_VERY_POSITIVE: - return true; + /** + * Convert string constant to id. + * + * @param string $type + * @return int + * @throws \InvalidArgumentException + */ + public static function str2id(string $type):int { + $key = array_search($type, self::getStringTypes()); + if ($key === false) { + throw new \InvalidArgumentException("Invalid str: $type"); + } + return $key + 1; + } - default: - return false; + /** + * Convert id to string contant. + * + * @param int $id + * @return string + * @throws \InvalidArgumentException + */ + public static function id2str(int $id):string { + $constants = self::getStringTypes(); + if ($id < 1 || $id > count($constants)) { + throw new \InvalidArgumentException("Invalid id: $id"); } + return $constants[$id - 1]; } } diff --git a/classes/repository/template_category_repository.php b/classes/repository/template_category_repository.php index f958821..c156b67 100644 --- a/classes/repository/template_category_repository.php +++ b/classes/repository/template_category_repository.php @@ -140,8 +140,10 @@ public function delete_by_id(int $id): bool { global $DB; try { $transaction = $DB->start_delegated_transaction(); - $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $id, - 'type' => localized_string_type::TEMPLATE_CATEGORY_HEADER, ]); + $DB->delete_records(tables::LOCALIZED_STRING_TABLE, [ + 'foreignkey' => $id, + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_CATEGORY_HEADER), + ]); $DB->delete_records(tables::PARAMETRIZED_TEMPLATE_CRITERION_TABLE, ['categoryid' => $id]); $DB->delete_records(tables::TEMPLATE_CATEGORY_TABLE, ['id' => $id]); @@ -163,7 +165,7 @@ private function get_headers($foreignkey): array { global $DB; $dboheaders = $DB->get_records(tables::LOCALIZED_STRING_TABLE, - ['foreignkey' => $foreignkey, 'type' => localized_string_type::TEMPLATE_CATEGORY_HEADER]); + ['foreignkey' => $foreignkey, 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_CATEGORY_HEADER)]); $headers = []; foreach ($dboheaders as $dboheader) { $headers[] = db_localized_string::to_localized_string($dboheader); @@ -182,7 +184,10 @@ private function get_headers_by_category_ids(): array { global $DB; $headers = []; - $rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE, ['type' => localized_string_type::TEMPLATE_CATEGORY_HEADER]); + $rs = $DB->get_recordset( + tables::LOCALIZED_STRING_TABLE, + ['typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_CATEGORY_HEADER)] + ); foreach ($rs as $row) { if (!\array_key_exists($row->foreignkey, $headers)) { $headers[$row->foreignkey] = []; diff --git a/classes/repository/template_criterion_repository.php b/classes/repository/template_criterion_repository.php index 61c1caa..ac787a9 100644 --- a/classes/repository/template_criterion_repository.php +++ b/classes/repository/template_criterion_repository.php @@ -243,23 +243,23 @@ public function delete(int $id) { $dbosubratings = $DB->get_records(tables::TEMPLATE_SUBRATINGS_TABLE, ['criterionid' => $id]); foreach ($dbosubratings as $dbosubrating) { $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $dbosubrating->id, - 'type' => localized_string_type::TEMPLATE_SUBRATING_TITLE, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_TITLE), ], ); $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $dbosubrating->id, - 'type' => localized_string_type::TEMPLATE_SUBRATING_DESCRIPTION, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_DESCRIPTION), ], ); $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $dbosubrating->id, - 'type' => localized_string_type::TEMPLATE_SUBRATING_VERY_NEGATIVE, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_VERY_NEGATIVE), ], ); $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $dbosubrating->id, - 'type' => localized_string_type::TEMPLATE_SUBRATING_NEGATIVE, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_NEGATIVE), ], ); $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $dbosubrating->id, - 'type' => localized_string_type::TEMPLATE_SUBRATING_POSITIVE, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_POSITIVE), ], ); $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $dbosubrating->id, - 'type' => localized_string_type::TEMPLATE_SUBRATING_VERY_POSITIVE, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_VERY_POSITIVE), ], ); } $DB->delete_records(tables::TEMPLATE_SUBRATINGS_TABLE, ['criterionid' => $id]); // Delete localized strings. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $id, - 'type' => localized_string_type::TEMPLATE_CRITERION, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_CRITERION), ], ); // Delete criterion. $DB->delete_records(tables::TEMPLATE_CRITERION_TABLE, ['id' => $id]); @@ -286,36 +286,33 @@ public static function dbo_to_template_criterion($dbo) { /** * Get all localized strings for a type, hashed by foreignkey, cached in memory for speed. * - * // phpcs:ignore moodle.Commenting.ValidTags.Invalid - * @TODO - evaluate this for memory usage. * @param string $type + * @param int $foreignkey * @return array * @throws \dml_exception */ - private function get_all_localized_strings_for_type(string $type): array { + private function get_all_localized_strings_for_type(string $type, int $foreignkey): array { global $DB; static $strings = []; - if (isset($strings[$type]) && !PHPUNIT_TEST) { + $typeid = localized_string_type::str2id($type); + $cachekey = $typeid . '~~' . $foreignkey; + + if (array_key_exists($cachekey, $strings) && !PHPUNIT_TEST) { // We already have this cached, so return it. - return $strings[$type]; + return $strings[$cachekey]; } - $strings[$type] = []; + $strings[$cachekey] = []; - $rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE, ['type' => $type]); + $rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE, ['typeid' => $typeid, 'foreignkey' => $foreignkey]); foreach ($rs as $row) { - $type = $row->type; - $key = $row->foreignkey; - if (!isset($strings[$type][$key])) { - $strings[$type][$key] = []; - } - $strings[$type][$key][$row->id] = db_localized_string::to_localized_string($row); + $strings[$cachekey][$row->id] = db_localized_string::to_localized_string($row); } $rs->close(); - return $strings[$type]; + return $strings[$cachekey]; } /** @@ -331,13 +328,12 @@ private function get_localized_strings(int $foreignkey, string $type): array { throw new \Exception("Unknown localized string type."); } - $allstrings = $this->get_all_localized_strings_for_type($type); - $localizedstrings = $allstrings[$foreignkey] ?? null; - if (!$localizedstrings) { - throw new coding_exception("Couldn't find localized strings by type '$type' and foreignkey '$foreignkey'"); + $allstrings = $this->get_all_localized_strings_for_type($type, $foreignkey); + if (empty($allstrings)) { + throw new \coding_exception("Couldn't find localized strings by type '$type' and foreignkey '$foreignkey'"); } - return $localizedstrings; + return $allstrings; } /** diff --git a/db/upgrade.php b/db/upgrade.php index 66358fe..284ba38 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -82,5 +82,29 @@ function xmldb_verbalfeedback_upgrade($oldversion) { upgrade_main_savepoint(true, 2021100103); } + if ($oldversion < 2024101700) { + require_once($CFG->dirroot . '/mod/verbalfeedback/classes/repository/model/localized_string_type.php'); + $table = new xmldb_table('verbalfeedback_local_string'); + $field = new xmldb_field('typeid', XMLDB_TYPE_INTEGER, '3', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, 0); + + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + + // Convert all strings to ids. + $offset = 0; + while ($records = $DB->get_records('verbalfeedback_local_string', null, 'id', 'id, type, typeid', $offset, 100)) { + foreach ($records as $record) { + $record->typeid = \mod_verbalfeedback\repository\model\localized_string_type::str2id($record->type); + $DB->update_record('verbalfeedback_local_string', $record); + } + $offset += 100; + } + $dbman->drop_field($table, new xmldb_field('type')); + $table->add_index('subitemtype', XMLDB_INDEX_NOTUNIQUE, ['foreignkey', 'typeid']); + + upgrade_mod_savepoint(true, 2024101700, 'verbalfeedback'); + } + return true; } diff --git a/version.php b/version.php index e38601a..70f76df 100644 --- a/version.php +++ b/version.php @@ -25,7 +25,7 @@ defined('MOODLE_INTERNAL') || die(); $plugin->component = 'mod_verbalfeedback'; -$plugin->version = 2024071300; +$plugin->version = 2024101700; $plugin->requires = 2022112800; $plugin->maturity = MATURITY_STABLE; $plugin->cron = 0; From 5cc931594ecdfb4a5f9fb36e6e507e6c751d85b8 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Tue, 22 Oct 2024 18:06:49 +0200 Subject: [PATCH 05/24] Change install that typeid instead of type column is installed --- db/install.xml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/db/install.xml b/db/install.xml index 89526d0..e60fa4b 100644 --- a/db/install.xml +++ b/db/install.xml @@ -20,7 +20,7 @@ - + @@ -28,6 +28,9 @@ + + + From a0f89378a5f9ccafdacffce15a0cd668a4e50247 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Tue, 22 Oct 2024 20:10:52 +0200 Subject: [PATCH 06/24] Fix rewritten queries that did not select the vf table when removing subquery statements. --- classes/api.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/classes/api.php b/classes/api.php index 630999e..5a1e287 100644 --- a/classes/api.php +++ b/classes/api.php @@ -120,7 +120,7 @@ public static function get_instance_by_itemid($itemid) { return $instances[$id]; } $instances[$id] = $DB->get_record_sql(" - SELECT * FROM {{$vftable} vf + SELECT vf.* FROM {{$vftable} vf INNER JOIN {{$cattable}} cat ON cat.instanceid = vf.id INNER JOIN {{$crittable}} crit ON crit.categoryid = cat.id WHERE crit.id = ?", @@ -144,7 +144,7 @@ public static function get_instance_by_categoryid($categoryid) { return $instances[$id]; } $instances[$id] = $DB->get_record_sql(" - SELECT * FROM {{$vftable}} vf + SELECT vf.* FROM {{$vftable}} vf INNER JOIN {{$cattable}} cat ON cat.instanceid = vf.id WHERE cat.id = ?", [$id], IGNORE_MISSING); From 3879f39cafceaf4e1841d751bd50942d51776ca1 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Wed, 23 Oct 2024 10:17:04 +0200 Subject: [PATCH 07/24] Fix sql queries with variable replacements. --- classes/api.php | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/classes/api.php b/classes/api.php index 5a1e287..c2e5002 100644 --- a/classes/api.php +++ b/classes/api.php @@ -113,17 +113,17 @@ public static function is_ready($verbalfeedbackid) { */ public static function get_instance_by_itemid($itemid) { global $DB; - [$vftable, $cattable, $crittable] = [tables::INSTANCE_TABLE, tables::INSTANCE_CATEGORY_TABLE, tables::INSTANCE_CRITERION_TABLE]; $id = (int)$itemid; static $instances = []; if (isset($instances[$id])) { return $instances[$id]; } - $instances[$id] = $DB->get_record_sql(" - SELECT vf.* FROM {{$vftable} vf - INNER JOIN {{$cattable}} cat ON cat.instanceid = vf.id - INNER JOIN {{$crittable}} crit ON crit.categoryid = cat.id - WHERE crit.id = ?", + $instances[$id] = $DB->get_record_sql(sprintf(' + SELECT vf.* FROM {%s} vf + INNER JOIN {%s} cat ON cat.instanceid = vf.id + INNER JOIN {%s} crit ON crit.categoryid = cat.id + WHERE crit.id = ?', + tables::INSTANCE_TABLE, tables::INSTANCE_CATEGORY_TABLE, tables::INSTANCE_CRITERION_TABLE), [$id], IGNORE_MISSING); return $instances[$id]; } @@ -137,16 +137,16 @@ public static function get_instance_by_itemid($itemid) { */ public static function get_instance_by_categoryid($categoryid) { global $DB; - [$vftable, $cattable] = [tables::INSTANCE_TABLE, tables::INSTANCE_CATEGORY_TABLE]; $id = (int)$categoryid; static $instances = []; if (isset($instances[$id])) { return $instances[$id]; } - $instances[$id] = $DB->get_record_sql(" - SELECT vf.* FROM {{$vftable}} vf - INNER JOIN {{$cattable}} cat ON cat.instanceid = vf.id - WHERE cat.id = ?", + $instances[$id] = $DB->get_record_sql(sprintf(' + SELECT vf.* FROM {%s} vf + INNER JOIN {%s} cat ON cat.instanceid = vf.id + WHERE cat.id = ?', + tables::INSTANCE_TABLE, tables::INSTANCE_CATEGORY_TABLE), [$id], IGNORE_MISSING); return $instances[$id]; } From cc21a73a4af96692c7ab83954698e71274061132 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Wed, 23 Oct 2024 10:55:57 +0200 Subject: [PATCH 08/24] Add test for the string to id conversion and make sure that strings do not change. --- .../model/localized_string_type_test.php | 134 ++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 tests/repository/model/localized_string_type_test.php diff --git a/tests/repository/model/localized_string_type_test.php b/tests/repository/model/localized_string_type_test.php new file mode 100644 index 0000000..effc99e --- /dev/null +++ b/tests/repository/model/localized_string_type_test.php @@ -0,0 +1,134 @@ +. + +/** + * Class for performing DB actions for the verbal feedback activity module. + * + * @package mod_verbalfeedback + * @copyright 2024 Stephan Robotta + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace mod_verbalfeedback; + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; + +use mod_verbalfeedback\repository\model\localized_string_type; + +/** + * Verbal feedback language repository test class + */ +final class localized_string_type_test extends \advanced_testcase { + + /** + * The string constants that are used in the verbal feedback module but are stored as ids in the database. + */ + private $stringConstants = [ + 'instance_criterion', + 'instance_category_header', + 'instance_subrating_title', + 'instance_subrating_description', + 'instance_subrating_verynegative', + 'instance_subrating_negative', + 'instance_subrating_positive', + 'instance_subrating_verypositive', + 'template_criterion', + 'template_category_header', + 'template_subrating_title', + 'template_subrating_description', + 'template_subrating_verynegative', + 'template_subrating_negative', + 'template_subrating_positive', + 'template_subrating_verypositive', + ]; + + /** + * Test that the contants are defined correctly. + */ + public function test_constants(): void { + $this->assertEquals('instance_criterion', localized_string_type::INSTANCE_CRITERION); + $this->assertEquals('instance_category_header', localized_string_type::INSTANCE_CATEGORY_HEADER); + $this->assertEquals('instance_subrating_title', localized_string_type::INSTANCE_SUBRATING_TITLE); + $this->assertEquals('instance_subrating_description', localized_string_type::INSTANCE_SUBRATING_DESCRIPTION); + $this->assertEquals('instance_subrating_verynegative', localized_string_type::INSTANCE_SUBRATING_VERY_NEGATIVE); + $this->assertEquals('instance_subrating_negative', localized_string_type::INSTANCE_SUBRATING_NEGATIVE); + $this->assertEquals('instance_subrating_positive', localized_string_type::INSTANCE_SUBRATING_POSITIVE); + $this->assertEquals('instance_subrating_verypositive', localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE); + $this->assertEquals('template_criterion', localized_string_type::TEMPLATE_CRITERION); + $this->assertEquals('template_category_header', localized_string_type::TEMPLATE_CATEGORY_HEADER); + $this->assertEquals('template_subrating_title', localized_string_type::TEMPLATE_SUBRATING_TITLE); + $this->assertEquals('template_subrating_description', localized_string_type::TEMPLATE_SUBRATING_DESCRIPTION); + $this->assertEquals('template_subrating_verynegative', localized_string_type::TEMPLATE_SUBRATING_VERY_NEGATIVE); + $this->assertEquals('template_subrating_negative', localized_string_type::TEMPLATE_SUBRATING_NEGATIVE); + $this->assertEquals('template_subrating_positive', localized_string_type::TEMPLATE_SUBRATING_POSITIVE); + $this->assertEquals('template_subrating_verypositive', localized_string_type::TEMPLATE_SUBRATING_VERY_POSITIVE); + } + + /** + * Test the string to id conversion. + * + * @covers \mod_verbalfeedback\repository\model\localized_string_type::str2id + */ + public function test_str2id(): void { + $i = 1; + foreach ($this->stringConstants as $string) { + $this->assertEquals($i, localized_string_type::str2id($string)); + $i++; + } + } + + /** + * Test the id to string conversion. + * + * @covers \mod_verbalfeedback\repository\model\localized_string_type::id2str + */ + public function test_id2str(): void { + $i = 1; + foreach ($this->stringConstants as $string) { + $this->assertEquals($string, localized_string_type::id2str($i)); + $i++; + } + $this->expectException(\InvalidArgumentException::class); + localized_string_type::id2str(17); + $this->expectException(\InvalidArgumentException::class); + localized_string_type::id2str(0); + } + + /** + * Test the getStringTypes method. + * + * @covers \mod_verbalfeedback\repository\model\localized_string_type::getStringTypes + */ + public function test_getStringTypes(): void { + $this->assertEquals($this->stringConstants, localized_string_type::getStringTypes()); + } + + /** + * Test the exists method. + * + * @covers \mod_verbalfeedback\repository\model\localized_string_type::exists + */ + public function test_exists(): void { + foreach ($this->stringConstants as $string) { + $this->assertTrue(localized_string_type::exists($string)); + $this->assertFalse(localized_string_type::exists(strtoupper($string))); + $this->assertFalse(localized_string_type::exists(ucfirst($string))); + } + $this->assertFalse(localized_string_type::exists('nonexisting')); + } +} From 3c42c18830b936b57780607cf20d576a268b0b54 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Wed, 23 Oct 2024 11:11:01 +0200 Subject: [PATCH 09/24] Add Moodle 4.5 to CI. --- .github/workflows/moodle-plugin-ci.yml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/.github/workflows/moodle-plugin-ci.yml b/.github/workflows/moodle-plugin-ci.yml index 465f8db..33cdbc8 100644 --- a/.github/workflows/moodle-plugin-ci.yml +++ b/.github/workflows/moodle-plugin-ci.yml @@ -67,6 +67,12 @@ jobs: - php: 8.1 moodle-branch: MOODLE_404_STABLE database: pgsql + - php: 8.1 + moodle-branch: MOODLE_405_STABLE + database: mariadb + - php: 8.1 + moodle-branch: MOODLE_405_STABLE + database: pgsql - php: 8.2 moodle-branch: MOODLE_402_STABLE database: mariadb @@ -85,12 +91,24 @@ jobs: - php: 8.2 moodle-branch: MOODLE_404_STABLE database: pgsql + - php: 8.2 + moodle-branch: MOODLE_405_STABLE + database: mariadb + - php: 8.2 + moodle-branch: MOODLE_405_STABLE + database: pgsql - php: 8.3 moodle-branch: MOODLE_404_STABLE database: mariadb - php: 8.3 moodle-branch: MOODLE_404_STABLE database: pgsql + - php: 8.3 + moodle-branch: MOODLE_405_STABLE + database: mariadb + - php: 8.3 + moodle-branch: MOODLE_405_STABLE + database: pgsql steps: - name: Check out repository code From 2e4f9707874580450ce5005e77af7e316cbc90de Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Wed, 23 Oct 2024 11:12:14 +0200 Subject: [PATCH 10/24] Increase the visible release number and dependency array. --- version.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/version.php b/version.php index 70f76df..0408482 100644 --- a/version.php +++ b/version.php @@ -29,5 +29,5 @@ $plugin->requires = 2022112800; $plugin->maturity = MATURITY_STABLE; $plugin->cron = 0; -$plugin->supported = [401, 404]; -$plugin->release = 'v4.4-r2'; +$plugin->supported = [401, 405]; +$plugin->release = 'v4.4-r3'; From 770d2986ab70fab6d0aa343663ed3a78c761384f Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Wed, 23 Oct 2024 11:26:00 +0200 Subject: [PATCH 11/24] Remove a view unnecessary use statements. --- template_category_delete.php | 2 -- template_criterion_delete.php | 2 -- template_delete.php | 2 -- 3 files changed, 6 deletions(-) diff --git a/template_category_delete.php b/template_category_delete.php index 8136328..b3e16bd 100644 --- a/template_category_delete.php +++ b/template_category_delete.php @@ -25,8 +25,6 @@ use mod_verbalfeedback\forms\template_category_delete_form; use mod_verbalfeedback\repository\template_category_repository; -use mod_verbalfeedback\model\template\template_category; - require_once(__DIR__ . '/../../config.php'); // Require own locallib.php. diff --git a/template_criterion_delete.php b/template_criterion_delete.php index 94d2719..8a213c2 100644 --- a/template_criterion_delete.php +++ b/template_criterion_delete.php @@ -23,8 +23,6 @@ */ use mod_verbalfeedback\forms\template_criterion_delete_form; - -use mod_verbalfeedback\model\template\template_criterion; use mod_verbalfeedback\repository\template_criterion_repository; require_once(__DIR__ . '/../../config.php'); diff --git a/template_delete.php b/template_delete.php index 19a1ba4..6886a9b 100644 --- a/template_delete.php +++ b/template_delete.php @@ -25,8 +25,6 @@ use mod_verbalfeedback\forms\template_delete_form; use mod_verbalfeedback\repository\template_repository; -use mod_verbalfeedback\model\template\template; - require_once(__DIR__ . '/../../config.php'); // Require own locallib.php. From c188c938ad25ff77ab3256e3c1f086cba605367c Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Wed, 23 Oct 2024 16:24:50 +0200 Subject: [PATCH 12/24] Change templates to omit lint errors. --- templates/list_verbalfeedback_items.mustache | 130 +++++++++---------- templates/questionnaire.mustache | 9 +- templates/report.mustache | 26 ---- tests/behat/configure_verbalfeedback.feature | 13 +- 4 files changed, 73 insertions(+), 105 deletions(-) diff --git a/templates/list_verbalfeedback_items.mustache b/templates/list_verbalfeedback_items.mustache index e83183b..684cdf8 100644 --- a/templates/list_verbalfeedback_items.mustache +++ b/templates/list_verbalfeedback_items.mustache @@ -911,9 +911,9 @@ "weight": "0.20" }], "verbalfeedbackid": 21, - "viewurl": {}, - "makeavailableurl": {}, - "previewurl": {}, + "viewurl": "http://example.org/mod/verbalfeedback/view.php?id=1073", + "makeavailableurl": "http://example.org/mod/verbalfeedback/view.php?id=1073&makeavailable=1", + "previewurl": "http://example.org/mod/verbalfeedback/questionnaire.php?instance=2&preview=1", "maxgrade": "15.00", "cmid": 465, "sesskey": "BABEgbUslM" @@ -959,88 +959,74 @@ {{/previewurl}}
- {{#str}}totalpercentage, mod_verbalfeedback{{/str}} - - % + {{#str}}totalpercentage, mod_verbalfeedback{{/str}} + + %
-
- {{# categories }} - - - - - - - - - - - - - - - - {{# criteria }} - - - - - - - {{/ criteria }} - - {{/ categories }} -
{{ header }} - {{#str}}percentage, mod_verbalfeedback{{/str}}:  -
- - - {{#js}} - require(['jquery'], function($) { - $('#category-{{id}}-percentage option[value="{{weight}}"]').prop('selected', true); - }); - {{/js}} + {{# categories }} +
+
{{ header }}
+
+ {{#str}}percentage, mod_verbalfeedback{{/str}}:  +
+ + + {{#js}} + require(['jquery'], function($) { + $('#category-{{id}}-percentage option[value="{{weight}}"]').prop('selected', true); + }); + {{/js}} + + +
+
+
+
+
+
{{#str}}multiplier, mod_verbalfeedback{{/str}}
+
+ {{# criteria }} +
+
+ {{ text }} +
+
+
+ + +
-
 {{#str}}multiplier, mod_verbalfeedback{{/str}}  
- {{ text }} - -
- - - - - -
-
- -
+ + + {{/ criteria }} + {{/ categories }} {{#js}} - require(['jquery', 'mod_verbalfeedback/edit_items'], function($, EditItems) { + require(['mod_verbalfeedback/edit_items'], function(EditItems) { let editItems = new EditItems({{verbalfeedbackid}}); editItems.updatePercentageSum(); }); diff --git a/templates/questionnaire.mustache b/templates/questionnaire.mustache index cb10857..5a45d41 100644 --- a/templates/questionnaire.mustache +++ b/templates/questionnaire.mustache @@ -258,7 +258,14 @@ saveandreturn, mod_verbalfeedback{{/str}} {{#js}} - require(['mod_verbalfeedback/questionnaire', 'jquery', 'core/yui', 'core/templates', 'core/notification', 'core/str'], function(Questionnaire, $) { + require([ + 'mod_verbalfeedback/questionnaire', + 'jquery', + 'core/yui', + 'core/templates', + 'core/notification', + 'core/str' + ], function(Questionnaire) { new Questionnaire(); }); {{/js}} diff --git a/templates/report.mustache b/templates/report.mustache index 4d16c8d..95ba0d9 100644 --- a/templates/report.mustache +++ b/templates/report.mustache @@ -183,31 +183,5 @@ - - \ No newline at end of file diff --git a/tests/behat/configure_verbalfeedback.feature b/tests/behat/configure_verbalfeedback.feature index 98559ec..c84947b 100644 --- a/tests/behat/configure_verbalfeedback.feature +++ b/tests/behat/configure_verbalfeedback.feature @@ -43,12 +43,13 @@ Feature: Configure a verbal feedback activity And I am on the "Test verbal feedback" "verbalfeedback activity" page logged in as teacher1 And I follow "Edit verbal feedback items" And I should see "Edit verbal feedback items" - And I set the field "Percentage" in the "Structure" "table_row" to "0%" - And I set the field "Percentage" in the "Body language" "table_row" to "25%" - And I set the field "Percentage" in the "Content" "table_row" to "25%" - And I set the field "Percentage" in the "Speech" "table_row" to "25%" - And I set the field "Percentage" in the "Media" "table_row" to "25%" - And I set the field "Multiplier" in the "The main points build on each other and are in line with your purpose." "table_row" to "0.00" + And I set the field with xpath "//*[text()='Structure']/../..//select" to "0.00" + And I set the field with xpath "//*[text()='Body language']/../..//select" to "0.25" + And I set the field with xpath "//*[text()='Content']/../..//select" to "0.25" + And I set the field with xpath "//*[text()='Speech']/../..//select" to "0.25" + And I set the field with xpath "//*[text()='Media']/../..//select" to "0.25" + # And I set the field "Multiplier" in the "The main points build on each other and are in line with your purpose." "table_row" to "0.00" + And I set the field with xpath "(//input[@data-action='change-item-multiplier'])[9]" to "0.00" And I follow "Preview" And I should not see "Structure" Then I should not see "The main parts build on each other and are purposeful." From 1e17983999f58f4987c6a7ea720070aee94690a6 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Fri, 25 Oct 2024 10:19:41 +0200 Subject: [PATCH 13/24] Change upgrade query. --- db/upgrade.php | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/db/upgrade.php b/db/upgrade.php index 284ba38..ad85f43 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -42,6 +42,8 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use mod_verbalfeedback\repository\model\localized_string_type; + /** * Upgrade code for the verbalfeedback activity plugin. * @@ -83,7 +85,6 @@ function xmldb_verbalfeedback_upgrade($oldversion) { } if ($oldversion < 2024101700) { - require_once($CFG->dirroot . '/mod/verbalfeedback/classes/repository/model/localized_string_type.php'); $table = new xmldb_table('verbalfeedback_local_string'); $field = new xmldb_field('typeid', XMLDB_TYPE_INTEGER, '3', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, 0); @@ -92,13 +93,11 @@ function xmldb_verbalfeedback_upgrade($oldversion) { } // Convert all strings to ids. - $offset = 0; - while ($records = $DB->get_records('verbalfeedback_local_string', null, 'id', 'id, type, typeid', $offset, 100)) { - foreach ($records as $record) { - $record->typeid = \mod_verbalfeedback\repository\model\localized_string_type::str2id($record->type); - $DB->update_record('verbalfeedback_local_string', $record); - } - $offset += 100; + foreach (localized_string_type::getStringTypes() as $type) { + $DB->execute( + 'UPDATE {verbalfeedback_local_string} SET typeid = ? WHERE type = ?', + [localized_string_type::str2id($type), $type] + ); } $dbman->drop_field($table, new xmldb_field('type')); $table->add_index('subitemtype', XMLDB_INDEX_NOTUNIQUE, ['foreignkey', 'typeid']); From 14d549c9ffa7051c0c67ad12b478a4cb9449d3a4 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Tue, 26 Nov 2024 10:22:30 +0100 Subject: [PATCH 14/24] Add two more static variables for caching. --- classes/repository/instance_repository.php | 81 ++++++++++++---------- 1 file changed, 46 insertions(+), 35 deletions(-) diff --git a/classes/repository/instance_repository.php b/classes/repository/instance_repository.php index 8b9ae60..51a13ea 100644 --- a/classes/repository/instance_repository.php +++ b/classes/repository/instance_repository.php @@ -216,25 +216,30 @@ private static function get_strings(string $type, int $subratingid, bool $throwo private static function get_criteria_by_category_for_instance_id(int $id): array { global $DB; - $crittab = tables::INSTANCE_CRITERION_TABLE; - $cattab = tables::INSTANCE_CATEGORY_TABLE; - - $sql = "SELECT crit.* - FROM {{$crittab}} crit - JOIN {{$cattab}} cat - ON crit.categoryid = cat.id - WHERE cat.instanceid = ?"; - - $bycat = []; - $rs = $DB->get_recordset_sql($sql, [$id]); - foreach ($rs as $dbocriterion) { - if (!isset($bycat[$dbocriterion->categoryid])) { - $bycat[$dbocriterion->categoryid] = []; + static $bycats = []; + + if (!array_key_exists($id, $bycats) || PHPUNIT_TEST) { + $crittab = tables::INSTANCE_CRITERION_TABLE; + $cattab = tables::INSTANCE_CATEGORY_TABLE; + + $sql = "SELECT crit.* + FROM {{$crittab}} crit + JOIN {{$cattab}} cat + ON crit.categoryid = cat.id + WHERE cat.instanceid = ?"; + + $bycat = []; + $rs = $DB->get_recordset_sql($sql, [$id]); + foreach ($rs as $dbocriterion) { + if (!isset($bycat[$dbocriterion->categoryid])) { + $bycat[$dbocriterion->categoryid] = []; + } + $bycat[$dbocriterion->categoryid][$dbocriterion->id] = db_instance_criterion::to_instance_criterion($dbocriterion); } - $bycat[$dbocriterion->categoryid][$dbocriterion->id] = db_instance_criterion::to_instance_criterion($dbocriterion); + $rs->close(); + $bycats[$id] = $bycat; } - $rs->close(); - return $bycat; + return $bycats[$id]; } /** @@ -245,26 +250,32 @@ private static function get_criteria_by_category_for_instance_id(int $id): array */ private static function get_subratings_by_criterion_for_instance(int $id): array { global $DB; - $crittab = tables::INSTANCE_CRITERION_TABLE; - $cattab = tables::INSTANCE_CATEGORY_TABLE; - $srattab = tables::INSTANCE_SUBRATING_TABLE; - $sql = "SELECT srat.* - FROM {{$srattab}} srat - JOIN {{$crittab}} crit - ON srat.criterionid = crit.id - JOIN {{$cattab}} mvic - ON crit.categoryid = mvic.id - WHERE mvic.instanceid = ?"; - $rs = $DB->get_recordset_sql($sql, [$id]); - $bycrit = []; - foreach ($rs as $dbosubrating) { - if (!isset($bycrit[$dbosubrating->criterionid])) { - $bycrit[$dbosubrating->criterionid] = []; + + static $bycrits = []; + + if (!array_key_exists($id, $bycrits) || PHPUNIT_TEST) { + $crittab = tables::INSTANCE_CRITERION_TABLE; + $cattab = tables::INSTANCE_CATEGORY_TABLE; + $srattab = tables::INSTANCE_SUBRATING_TABLE; + $sql = "SELECT srat.* + FROM {{$srattab}} srat + JOIN {{$crittab}} crit + ON srat.criterionid = crit.id + JOIN {{$cattab}} mvic + ON crit.categoryid = mvic.id + WHERE mvic.instanceid = ?"; + $rs = $DB->get_recordset_sql($sql, [$id]); + $bycrit = []; + foreach ($rs as $dbosubrating) { + if (!isset($bycrit[$dbosubrating->criterionid])) { + $bycrit[$dbosubrating->criterionid] = []; + } + $bycrit[$dbosubrating->criterionid][$dbosubrating->id] = db_instance_subrating::to_subrating($dbosubrating); } - $bycrit[$dbosubrating->criterionid][$dbosubrating->id] = db_instance_subrating::to_subrating($dbosubrating); + $rs->close(); + $bycrits[$id] = $bycrit; } - $rs->close(); - return $bycrit; + return $bycrits[$id]; } /** From 7da679abe09acdd4d2f88711738d223c62b150ab Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Wed, 4 Dec 2024 16:13:25 +0100 Subject: [PATCH 15/24] Add index to local string table to load all strings for one instance at once. --- classes/repository/instance_repository.php | 71 ++++++++++++--- .../repository/model/db_localized_string.php | 15 +++- db/upgrade.php | 89 +++++++++++++++++++ version.php | 2 +- 4 files changed, 165 insertions(+), 12 deletions(-) diff --git a/classes/repository/instance_repository.php b/classes/repository/instance_repository.php index 51a13ea..06a2356 100644 --- a/classes/repository/instance_repository.php +++ b/classes/repository/instance_repository.php @@ -70,7 +70,7 @@ public static function get_by_id(int $id): instance { $category = db_instance_category::to_instance_category($dbocategory); // Load category headers. - $dbolocalizedstrings = self::get_strings(localized_string_type::INSTANCE_CATEGORY_HEADER, $category->get_id()); + $dbolocalizedstrings = self::get_strings(localized_string_type::INSTANCE_CATEGORY_HEADER, $category->get_id(), $id); foreach ($dbolocalizedstrings as $dbo) { $header = db_localized_string::to_localized_string($dbo); $category->add_header($header); @@ -81,7 +81,7 @@ public static function get_by_id(int $id): instance { foreach ($criteria as $criterion) { // Load criterion description. - $dbodescriptions = self::get_strings(localized_string_type::INSTANCE_CRITERION, $criterion->get_id()); + $dbodescriptions = self::get_strings(localized_string_type::INSTANCE_CRITERION, $criterion->get_id(), $id); foreach ($dbodescriptions as $dbodescription) { $description = db_localized_string::to_localized_string($dbodescription); $criterion->add_description($description); @@ -104,7 +104,7 @@ public static function get_by_id(int $id): instance { // Load subrating very positive texts. localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE => 'verypositives', ] as $type => $attribute) { - $dbotitles = self::get_strings($type, $subrating->get_id()); + $dbotitles = self::get_strings($type, $subrating->get_id(), $id); foreach ($dbotitles as $dbotitle) { $item = db_localized_string::to_localized_string($dbotitle); $subrating->{$attribute}[] = $item; @@ -113,7 +113,7 @@ public static function get_by_id(int $id): instance { // Load subrating descriptions. $dbosubratingdescriptions = self::get_strings(localized_string_type::INSTANCE_SUBRATING_DESCRIPTION, - $subrating->get_id()); + $subrating->get_id(), $id); foreach ($dbosubratingdescriptions as $dbosubratingdescription) { $subratingdescription = db_localized_string::to_localized_string($dbosubratingdescription); $subrating->add_description($subratingdescription); @@ -121,7 +121,7 @@ public static function get_by_id(int $id): instance { // Load subrating very negative texts. $dboverynegatives = self::get_strings(localized_string_type::INSTANCE_SUBRATING_VERY_NEGATIVE, - $subrating->get_id()); + $subrating->get_id(), $id); foreach ($dboverynegatives as $dboverynegative) { $verynegative = db_localized_string::to_localized_string($dboverynegative); $subrating->add_verynegative($verynegative); @@ -129,7 +129,7 @@ public static function get_by_id(int $id): instance { // Load subrating negative texts. $dbonegatives = self::get_strings(localized_string_type::INSTANCE_SUBRATING_NEGATIVE, - $subrating->get_id()); + $subrating->get_id(), $id); foreach ($dbonegatives as $dbonegative) { $negative = db_localized_string::to_localized_string($dbonegative); $subrating->add_negative($negative); @@ -137,7 +137,7 @@ public static function get_by_id(int $id): instance { // Load subrating positive texts. $dbopositives = self::get_strings(localized_string_type::INSTANCE_SUBRATING_POSITIVE, - $subrating->get_id()); + $subrating->get_id(), $id); foreach ($dbopositives as $dbopositive) { $positive = db_localized_string::to_localized_string($dbopositive); $subrating->add_positive($positive); @@ -145,7 +145,7 @@ public static function get_by_id(int $id): instance { // Load subrating very positive texts. $dboverypositives = self::get_strings(localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE, - $subrating->get_id()); + $subrating->get_id(), $id); foreach ($dboverypositives as $dboverypositive) { $verypositive = db_localized_string::to_localized_string($dboverypositive); $subrating->add_verypositive($verypositive); @@ -167,22 +167,35 @@ public static function get_by_id(int $id): instance { * * @param string $type * @param int $subratingid + * @param int $instanceid * @param bool $throwonerror * @return array * @throws \coding_exception * @throws \dml_exception */ - private static function get_strings(string $type, int $subratingid, bool $throwonerror = false): array { + private static function get_strings(string $type, int $subratingid, int $instanceid = 0, bool $throwonerror = false): array { global $DB; static $sortedstrings = []; + $typeid = localized_string_type::str2id($type); + + if ($instanceid > 0) { + $strings = self::get_strings_for_instance($instanceid); + if (array_key_exists($typeid, $strings) && array_key_exists($subratingid, $strings[$typeid])) { + return $strings[$typeid][$subratingid]; + } + if ($throwonerror) { + throw new \coding_exception("No strings for type $type and subratingid $subratingid"); + } + return []; + } $cachekey = $type . '~~' . $subratingid; if (!array_key_exists($cachekey, $sortedstrings) || PHPUNIT_TEST) { $sortedstrings[$cachekey] = []; $rs = $DB->get_recordset( tables::LOCALIZED_STRING_TABLE, - ['typeid' => localized_string_type::str2id($type), 'foreignkey' => $subratingid] + ['typeid' => $typeid, 'foreignkey' => $subratingid] ); foreach ($rs as $dboheader) { $dbobj = new db_localized_string; @@ -207,6 +220,44 @@ private static function get_strings(string $type, int $subratingid, bool $throwo return $sortedstrings[$cachekey]; } + /** + * Same as get_strings but for a specific instance, load the string all at once. + * + * @param int $id - instance id of the verbal feedback activity. + * @return array + */ + private static function get_strings_for_instance(int $id): array { + global $DB; + + static $byinstance = []; + + if (!array_key_exists($id, $byinstance) || PHPUNIT_TEST) { + $byinstance[$id] = []; + $rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE, ['instanceid' => $id]); + foreach ($rs as $dboheader) { + $dbobj = new db_localized_string; + $dbobj->id = $dboheader->id; + $dbobj->languageid = $dboheader->languageid; + $dbobj->string = $dboheader->string; + $dbobj->typeid = $dboheader->typeid; + $dbobj->foreignkey = $dboheader->foreignkey; + $dbobj->instanceid = $dboheader->instanceid; + + if (!array_key_exists($dbobj->typeid, $byinstance[$id])) { + $byinstance[$id][$dbobj->typeid] = []; + } + if (!array_key_exists($dbobj->foreignkey, $byinstance[$id][$dbobj->typeid])) { + $byinstance[$id][$dbobj->typeid][$dbobj->foreignkey] = []; + } + // Store the localized string by instanceid, typeid, foreignkey, languageid. + $byinstance[$id][$dbobj->typeid][$dbobj->foreignkey][$dbobj->languageid] = $dbobj; + } + $rs->close(); + } + + return $byinstance[$id]; + } + /** * Gets all criteria hashed by category id for a specific instance. * @param int $id - instance id diff --git a/classes/repository/model/db_localized_string.php b/classes/repository/model/db_localized_string.php index f3c14dc..b7ca0c1 100644 --- a/classes/repository/model/db_localized_string.php +++ b/classes/repository/model/db_localized_string.php @@ -29,38 +29,50 @@ * The database localized string class */ class db_localized_string { + /** * @var int The id */ public $id; + /** * @var int The type */ public $typeid; + /** * @var int The foreign key */ + public $foreignkey; + /** * @var string The language id */ public $languageid; + /** * @var string The string */ public $string; + /** + * @var int The instance id of the verbal feedback activity + */ + public $instanceid; + /** * Return a localized string database object * * @param localized_string $localizedstring The localized string * @param string $type The string type * @param int $foreignkey The foreign key + * @param int $instanceid The instance id * @return db_localized_string * @throws \Exception */ public static function from_localized_string(localized_string $localizedstring, string $type, - int $foreignkey): db_localized_string { + int $foreignkey, int $instanceid): db_localized_string { if (!localized_string_type::exists($type)) { throw new \Exception("unknown localized_string_type"); } @@ -70,6 +82,7 @@ public static function from_localized_string(localized_string $localizedstring, $dbo->foreignkey = $foreignkey; $dbo->languageid = $localizedstring->get_language_id(); $dbo->string = $localizedstring->get_string(); + $dbo->instanceid = $instanceid; return $dbo; } diff --git a/db/upgrade.php b/db/upgrade.php index ad85f43..85770bf 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -42,6 +42,7 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use mod_verbalfeedback\repository\tables; use mod_verbalfeedback\repository\model\localized_string_type; /** @@ -105,5 +106,93 @@ function xmldb_verbalfeedback_upgrade($oldversion) { upgrade_mod_savepoint(true, 2024101700, 'verbalfeedback'); } + if ($oldversion < 2024120400) { + // Add instance id to localized string table and put an index on it so that when loading + // the strings, we can load them all at once by instance id of the verbal feedback activity. + $table = new xmldb_table('verbalfeedback_local_string'); + $field = new xmldb_field('instanceid', XMLDB_TYPE_INTEGER, '3', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, 0); + + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + + add_instance_to_localized_string(); + $table->add_index('instanceidx', XMLDB_INDEX_NOTUNIQUE, ['instanceid']); + + upgrade_mod_savepoint(true, 2024120400, 'verbalfeedback'); + } + return true; } + + +/** + * Go over all existing strings in the database and add instance id of the verbal feedback activity to the localized string. + */ +function add_instance_to_localized_string() { + global $DB; + + // Category header. + $sql = sprintf(' + UPDATE {%2$s} SET instanceid = {%1$s}.instanceid FROM {%1$s} + WHERE {%1$s}.id = {%2$s}.foreignkey AND {%2$s}.typeid IN (?, ?)', + tables::INSTANCE_CATEGORY_TABLE, + tables::LOCALIZED_STRING_TABLE, + ); + $DB->execute($sql, [ + localized_string_type::str2id(localized_string_type::INSTANCE_CATEGORY_HEADER), + localized_string_type::str2id(localized_string_type::TEMPLATE_CATEGORY_HEADER), + ]); + + // Category criterion. + $sql = sprintf(' + SELECT {%1$s}.id, {%2$s}.instanceid FROM {%1$s} JOIN {%2$s} ON {%2$s}.id = {%1$s}.categoryid + WHERE {%2$s}.id IN (SELECT DISTINCT(id) FROM {%2$s}) + ', + tables::INSTANCE_CRITERION_TABLE, + tables::INSTANCE_CATEGORY_TABLE + ); + $results = $DB->get_records_sql($sql); + foreach ($results as $result) { + $DB->execute( + sprintf('UPDATE {%s} SET instanceid = ? + WHERE foreignkey = ? AND typeid IN (?, ?)', tables::LOCALIZED_STRING_TABLE), + [ + $result->instanceid, + $result->id, + localized_string_type::str2id(localized_string_type::INSTANCE_CRITERION), + localized_string_type::str2id(localized_string_type::TEMPLATE_CRITERION), + ] + ); + } + + // Subcriteria. + $sql = sprintf(' + SELECT {%1$s}.id, {%3$s}.instanceid + FROM {%1$s} + JOIN {%2$s} ON {%1$s}.criterionid = {%2$s}.id + JOIN {%3$s} ON {%2$s}.categoryid = {%3$s}.id + ', tables::INSTANCE_SUBRATING_TABLE, tables::INSTANCE_CRITERION_TABLE, tables::INSTANCE_CATEGORY_TABLE); + $results = $DB->get_records_sql($sql); + foreach ($results as $result) { + $DB->execute( + sprintf('UPDATE {%s} SET instanceid = ? + WHERE foreignkey = ? AND typeid IN (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', tables::LOCALIZED_STRING_TABLE), + [ + $result->instanceid, + $result->id, + localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_TITLE), + localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_DESCRIPTION), + localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_VERY_NEGATIVE), + localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_NEGATIVE), + localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_POSITIVE), + localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE), + localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_DESCRIPTION), + localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_VERY_NEGATIVE), + localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_NEGATIVE), + localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_POSITIVE), + localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_VERY_POSITIVE), + ] + ); + } +} \ No newline at end of file diff --git a/version.php b/version.php index 0408482..60663b5 100644 --- a/version.php +++ b/version.php @@ -25,7 +25,7 @@ defined('MOODLE_INTERNAL') || die(); $plugin->component = 'mod_verbalfeedback'; -$plugin->version = 2024101700; +$plugin->version = 2024120400; $plugin->requires = 2022112800; $plugin->maturity = MATURITY_STABLE; $plugin->cron = 0; From ba405cc3a5e301ee69a7d39b263375a29c2833b4 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Wed, 4 Dec 2024 18:08:12 +0100 Subject: [PATCH 16/24] Add missing parameter for string function. Make it optional when not available. --- classes/repository/instance_repository.php | 55 ++++--------------- .../repository/model/db_localized_string.php | 2 +- 2 files changed, 11 insertions(+), 46 deletions(-) diff --git a/classes/repository/instance_repository.php b/classes/repository/instance_repository.php index 06a2356..5b89c12 100644 --- a/classes/repository/instance_repository.php +++ b/classes/repository/instance_repository.php @@ -358,7 +358,7 @@ public function save(instance $instance) { foreach ($category->get_headers() as $header) { $dbolocalizedstring = db_localized_string::from_localized_string($header, - localized_string_type::INSTANCE_CATEGORY_HEADER, $category->get_id()); + localized_string_type::INSTANCE_CATEGORY_HEADER, $category->get_id(), $instance->get_id()); if ($header->get_id() == 0) { $DB->insert_record(tables::LOCALIZED_STRING_TABLE, $dbolocalizedstring); } else { @@ -377,7 +377,7 @@ public function save(instance $instance) { foreach ($criterion->get_descriptions() as $localizedstring) { $dbolocalizedstring = db_localized_string::from_localized_string($localizedstring, - localized_string_type::INSTANCE_CRITERION, $criterion->get_id()); + localized_string_type::INSTANCE_CRITERION, $criterion->get_id(), $instance->get_id()); if ($localizedstring->get_id() == 0) { $DB->insert_record(tables::LOCALIZED_STRING_TABLE, $dbolocalizedstring); } else { @@ -396,7 +396,7 @@ public function save(instance $instance) { foreach ($subrating->get_titles() as $title) { $dbotitle = db_localized_string::from_localized_string($title, - localized_string_type::INSTANCE_SUBRATING_TITLE, $subrating->get_id()); + localized_string_type::INSTANCE_SUBRATING_TITLE, $subrating->get_id(), $instance->get_id()); if ($title->get_id() == 0) { $DB->insert_record(tables::LOCALIZED_STRING_TABLE, $dbotitle); } else { @@ -407,7 +407,7 @@ public function save(instance $instance) { // Subrating description. foreach ($subrating->get_descriptions() as $description) { $dbodescription = db_localized_string::from_localized_string($description, - localized_string_type::INSTANCE_SUBRATING_DESCRIPTION, $subrating->get_id()); + localized_string_type::INSTANCE_SUBRATING_DESCRIPTION, $subrating->get_id(), $instance->get_id()); if ($description->get_id() == 0) { $DB->insert_record(tables::LOCALIZED_STRING_TABLE, $dbodescription); } else { @@ -417,7 +417,7 @@ public function save(instance $instance) { foreach ($subrating->get_verynegatives() as $verynegative) { $dboverynegative = db_localized_string::from_localized_string($verynegative, - localized_string_type::INSTANCE_SUBRATING_VERY_NEGATIVE, $subrating->get_id()); + localized_string_type::INSTANCE_SUBRATING_VERY_NEGATIVE, $subrating->get_id(), $instance->get_id()); if ($verynegative->get_id() == 0) { $DB->insert_record(tables::LOCALIZED_STRING_TABLE, $dboverynegative); } else { @@ -426,7 +426,7 @@ public function save(instance $instance) { } foreach ($subrating->get_negatives() as $negative) { $dbonegative = db_localized_string::from_localized_string($negative, - localized_string_type::INSTANCE_SUBRATING_NEGATIVE, $subrating->get_id()); + localized_string_type::INSTANCE_SUBRATING_NEGATIVE, $subrating->get_id(), $instance->get_id()); if ($negative->get_id() == 0) { $DB->insert_record(tables::LOCALIZED_STRING_TABLE, $dbonegative); } else { @@ -435,7 +435,7 @@ public function save(instance $instance) { } foreach ($subrating->get_positives() as $positive) { $dbopositive = db_localized_string::from_localized_string($positive, - localized_string_type::INSTANCE_SUBRATING_POSITIVE, $subrating->get_id()); + localized_string_type::INSTANCE_SUBRATING_POSITIVE, $subrating->get_id(), $instance->get_id()); if ($positive->get_id() == 0) { $DB->insert_record(tables::LOCALIZED_STRING_TABLE, $dbopositive); } else { @@ -444,7 +444,7 @@ public function save(instance $instance) { } foreach ($subrating->get_verypositives() as $verypositive) { $dboverypositive = db_localized_string::from_localized_string($verypositive, - localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE, $subrating->get_id()); + localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE, $subrating->get_id(), $instance->get_id()); if ($verypositive->get_id() == 0) { $DB->insert_record(tables::LOCALIZED_STRING_TABLE, $dboverypositive); } else { @@ -471,47 +471,12 @@ public function delete(int $instanceid) { $dbocategories = $DB->get_records(tables::INSTANCE_CATEGORY_TABLE, ["instanceid" => $instanceid]); foreach ($dbocategories as $dbocategory) { - // Delete category headers. - $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_CATEGORY_HEADER), "foreignkey" => $dbocategory->id]); + // Delete all strings. + $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ["instanceid" => $instanceid]); // Delete category criteria. $dbocriteria = $DB->get_records(tables::INSTANCE_CRITERION_TABLE, ["categoryid" => $dbocategory->id]); foreach ($dbocriteria as $dbocriterion) { - - // Delete criterion description. - $dbodescriptions = $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_CRITERION), "foreignkey" => $dbocriterion->id]); - - // Delete criterion subratings. - $dbosubratings = $DB->get_records(tables::INSTANCE_SUBRATING_TABLE, ["criterionid" => $dbocriterion->id]); - foreach ($dbosubratings as $dbosubrating) { - - // Delete subrating titles. - $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_TITLE), "foreignkey" => $dbosubrating->id]); - - // Delete subrating descriptions. - $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_DESCRIPTION), "foreignkey" => $dbosubrating->id]); - - // Delete subrating very negative texts. - $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_VERY_NEGATIVE), "foreignkey" => $dbosubrating->id]); - - // Delete subrating negative texts. - $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_NEGATIVE), "foreignkey" => $dbosubrating->id]); - - // Delete subrating positive texts. - $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_POSITIVE), "foreignkey" => $dbosubrating->id]); - - // Delete subrating very positive texts. - $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE), "foreignkey" => $dbosubrating->id]); - } - // Delete subratings. $dbosubratings = $DB->delete_records(tables::INSTANCE_SUBRATING_TABLE, ["criterionid" => $dbocriterion->id]); } diff --git a/classes/repository/model/db_localized_string.php b/classes/repository/model/db_localized_string.php index b7ca0c1..a01a74a 100644 --- a/classes/repository/model/db_localized_string.php +++ b/classes/repository/model/db_localized_string.php @@ -72,7 +72,7 @@ class db_localized_string { * @throws \Exception */ public static function from_localized_string(localized_string $localizedstring, string $type, - int $foreignkey, int $instanceid): db_localized_string { + int $foreignkey, int $instanceid = 0): db_localized_string { if (!localized_string_type::exists($type)) { throw new \Exception("unknown localized_string_type"); } From 8590c4de48a1d554e7104149adef95f1c74b26cd Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Wed, 4 Dec 2024 18:13:11 +0100 Subject: [PATCH 17/24] Remove category template header from upgrade, these have no connection to the vb instance. --- db/upgrade.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/db/upgrade.php b/db/upgrade.php index 85770bf..d7e85c2 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -135,13 +135,12 @@ function add_instance_to_localized_string() { // Category header. $sql = sprintf(' UPDATE {%2$s} SET instanceid = {%1$s}.instanceid FROM {%1$s} - WHERE {%1$s}.id = {%2$s}.foreignkey AND {%2$s}.typeid IN (?, ?)', + WHERE {%1$s}.id = {%2$s}.foreignkey AND {%2$s}.typeid = ?', tables::INSTANCE_CATEGORY_TABLE, tables::LOCALIZED_STRING_TABLE, ); $DB->execute($sql, [ localized_string_type::str2id(localized_string_type::INSTANCE_CATEGORY_HEADER), - localized_string_type::str2id(localized_string_type::TEMPLATE_CATEGORY_HEADER), ]); // Category criterion. From 94c3e37e96a5f1df8043914fd7fa7e2832b06e7f Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Wed, 4 Dec 2024 18:19:43 +0100 Subject: [PATCH 18/24] Remove criterion template header from upgrade, these have no connection to the vb instance. --- db/upgrade.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/db/upgrade.php b/db/upgrade.php index d7e85c2..0f70678 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -155,12 +155,11 @@ function add_instance_to_localized_string() { foreach ($results as $result) { $DB->execute( sprintf('UPDATE {%s} SET instanceid = ? - WHERE foreignkey = ? AND typeid IN (?, ?)', tables::LOCALIZED_STRING_TABLE), + WHERE foreignkey = ? AND typeid = ?', tables::LOCALIZED_STRING_TABLE), [ $result->instanceid, $result->id, localized_string_type::str2id(localized_string_type::INSTANCE_CRITERION), - localized_string_type::str2id(localized_string_type::TEMPLATE_CRITERION), ] ); } From 547dcedfe7e9caabaf1d49b1c2835052609f0f3f Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Wed, 4 Dec 2024 18:25:13 +0100 Subject: [PATCH 19/24] Add new field and key in install.xml --- db/install.xml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/db/install.xml b/db/install.xml index e60fa4b..7e5b6a9 100644 --- a/db/install.xml +++ b/db/install.xml @@ -21,6 +21,7 @@ + @@ -30,6 +31,7 @@ + From b5f81d4597beff1ee025806c4b9e938ac4dec6c7 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Thu, 5 Dec 2024 13:27:53 +0100 Subject: [PATCH 20/24] Improve delete instance. --- classes/repository/instance_repository.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/classes/repository/instance_repository.php b/classes/repository/instance_repository.php index 5b89c12..06c0e06 100644 --- a/classes/repository/instance_repository.php +++ b/classes/repository/instance_repository.php @@ -204,6 +204,7 @@ private static function get_strings(string $type, int $subratingid, int $instanc $dbobj->string = $dboheader->string; $dbobj->typeid = $dboheader->typeid; $dbobj->foreignkey = $dboheader->foreignkey; + $dbobj->instanceid = $dboheader->instanceid; $sortedstrings[$cachekey][$dbobj->languageid] = $dbobj; } @@ -471,19 +472,20 @@ public function delete(int $instanceid) { $dbocategories = $DB->get_records(tables::INSTANCE_CATEGORY_TABLE, ["instanceid" => $instanceid]); foreach ($dbocategories as $dbocategory) { - // Delete all strings. - $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ["instanceid" => $instanceid]); - // Delete category criteria. $dbocriteria = $DB->get_records(tables::INSTANCE_CRITERION_TABLE, ["categoryid" => $dbocategory->id]); foreach ($dbocriteria as $dbocriterion) { // Delete subratings. - $dbosubratings = $DB->delete_records(tables::INSTANCE_SUBRATING_TABLE, ["criterionid" => $dbocriterion->id]); + $DB->delete_records(tables::INSTANCE_SUBRATING_TABLE, ["criterionid" => $dbocriterion->id]); } $DB->delete_records(tables::INSTANCE_CRITERION_TABLE, ["categoryid" => $dbocategory->id]); } + // Delete all strings. + $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ["instanceid" => $instanceid]); + // Delete categories. $DB->delete_records(tables::INSTANCE_CATEGORY_TABLE, ["instanceid" => $instanceid]); + // Delete the instance itself. return $DB->delete_records(tables::INSTANCE_TABLE, ["id" => $instanceid]); } From 415cc103ff55267cc062dbe4bfd26f4b346a4bde Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Thu, 5 Dec 2024 13:28:11 +0100 Subject: [PATCH 21/24] Fix mapping in backup and restore. --- .../backup_verbalfeedback_stepslib.php | 56 +++++++++++++------ .../restore_verbalfeedback_stepslib.php | 16 +++++- 2 files changed, 53 insertions(+), 19 deletions(-) diff --git a/backup/moodle2/backup_verbalfeedback_stepslib.php b/backup/moodle2/backup_verbalfeedback_stepslib.php index 95b78f2..6a1a865 100644 --- a/backup/moodle2/backup_verbalfeedback_stepslib.php +++ b/backup/moodle2/backup_verbalfeedback_stepslib.php @@ -144,26 +144,50 @@ protected function define_structure() { $language->set_source_table(tables::LANGUAGE_TABLE, []); $category->set_source_table(tables::INSTANCE_CATEGORY_TABLE, ['instanceid' => backup::VAR_PARENTID], 'id ASC'); - $categoryheader->set_source_table(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => backup::VAR_PARENTID, - 'type' => backup_helper::is_sqlparam(localized_string_type::INSTANCE_CATEGORY_HEADER), ]); + $categoryheader->set_source_table(tables::LOCALIZED_STRING_TABLE, [ + 'foreignkey' => backup::VAR_PARENTID, + 'typeid' => backup_helper::is_sqlparam( + localized_string_type::str2id(localized_string_type::INSTANCE_CATEGORY_HEADER) + ), ]); $criterion->set_source_table(tables::INSTANCE_CRITERION_TABLE, ['categoryid' => backup::VAR_PARENTID], 'id ASC'); - $criteriontext->set_source_table(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => backup::VAR_PARENTID, - 'type' => backup_helper::is_sqlparam(localized_string_type::INSTANCE_CRITERION), ]); + $criteriontext->set_source_table(tables::LOCALIZED_STRING_TABLE, [ + 'foreignkey' => backup::VAR_PARENTID, + 'typeid' => backup_helper::is_sqlparam( + localized_string_type::str2id(localized_string_type::INSTANCE_CRITERION) + ), ]); $subrating->set_source_table(tables::INSTANCE_SUBRATING_TABLE, ['criterionid' => backup::VAR_PARENTID], 'id ASC'); - $subratingtitle->set_source_table(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => backup::VAR_PARENTID, - 'type' => backup_helper::is_sqlparam(localized_string_type::INSTANCE_SUBRATING_TITLE), ]); - $subratingdescription->set_source_table(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => backup::VAR_PARENTID, - 'type' => backup_helper::is_sqlparam(localized_string_type::INSTANCE_SUBRATING_DESCRIPTION), ]); - $subratingverynegative->set_source_table(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => backup::VAR_PARENTID, - 'type' => backup_helper::is_sqlparam(localized_string_type::INSTANCE_SUBRATING_VERY_NEGATIVE), ]); - $subratingnegative->set_source_table(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => backup::VAR_PARENTID, - 'type' => backup_helper::is_sqlparam(localized_string_type::INSTANCE_SUBRATING_NEGATIVE), ]); - $subratingpositive->set_source_table(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => backup::VAR_PARENTID, - 'type' => backup_helper::is_sqlparam(localized_string_type::INSTANCE_SUBRATING_POSITIVE), ]); - $subratingverypositive->set_source_table(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => backup::VAR_PARENTID, - 'type' => backup_helper::is_sqlparam(localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE), ]); + $subratingtitle->set_source_table(tables::LOCALIZED_STRING_TABLE, [ + 'foreignkey' => backup::VAR_PARENTID, + 'typeid' => backup_helper::is_sqlparam( + localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_TITLE) + ), ]); + $subratingdescription->set_source_table(tables::LOCALIZED_STRING_TABLE, [ + 'foreignkey' => backup::VAR_PARENTID, + 'typeid' => backup_helper::is_sqlparam( + localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_DESCRIPTION) + ), ]); + $subratingverynegative->set_source_table(tables::LOCALIZED_STRING_TABLE, [ + 'foreignkey' => backup::VAR_PARENTID, + 'typeid' => backup_helper::is_sqlparam( + localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_VERY_NEGATIVE) + ), ]); + $subratingnegative->set_source_table(tables::LOCALIZED_STRING_TABLE, [ + 'foreignkey' => backup::VAR_PARENTID, + 'typeid' => backup_helper::is_sqlparam( + localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_NEGATIVE) + ), ]); + $subratingpositive->set_source_table(tables::LOCALIZED_STRING_TABLE, [ + 'foreignkey' => backup::VAR_PARENTID, + 'typeid' => backup_helper::is_sqlparam( + localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_POSITIVE) + ), ]); + $subratingverypositive->set_source_table(tables::LOCALIZED_STRING_TABLE, [ + 'foreignkey' => backup::VAR_PARENTID, + 'typeid' => backup_helper::is_sqlparam( + localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE) + ), ]); // All the rest of elements only happen if we are including user info. if ($userinfo) { diff --git a/backup/moodle2/restore_verbalfeedback_stepslib.php b/backup/moodle2/restore_verbalfeedback_stepslib.php index e28fc1b..8a09fd5 100644 --- a/backup/moodle2/restore_verbalfeedback_stepslib.php +++ b/backup/moodle2/restore_verbalfeedback_stepslib.php @@ -22,6 +22,7 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use mod_verbalfeedback\repository\model\localized_string_type; use mod_verbalfeedback\repository\tables; /** @@ -33,6 +34,13 @@ */ class restore_verbalfeedback_activity_structure_step extends restore_activity_structure_step { + /** + * The instance id when the item is newly inserted. + * + * @var int + */ + private $instanceid; + /** * Function that will return the structure to be processed by this restore_step. * Must return one array of @restore_path_element elements @@ -108,9 +116,9 @@ protected function process_verbalfeedback($data) { } // Insert the verbal feedback record. - $newitemid = $DB->insert_record(tables::INSTANCE_TABLE, $data); + $this->instanceid = $DB->insert_record(tables::INSTANCE_TABLE, $data); // Immediately after inserting "activity" record, call this. - $this->apply_activity_instance($newitemid); + $this->apply_activity_instance($this->instanceid); } /** @@ -326,8 +334,10 @@ private function process_localized_string($foreigenkeymapping, $data) { $data = (object) $data; $data->foreignkey = $this->get_mappingid($foreigenkeymapping, $data->foreignkey); - $data->languageid = $this->get_mappingid('language', $data->languageid); + $data->typeid = localized_string_type::str2id($data->type); + unset($data->type); + $data->instanceid = $this->instanceid; $DB->insert_record('verbalfeedback_local_string', $data); } } From 259929474a61701ba461be2eff67fc8a1735e97d Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Thu, 5 Dec 2024 14:49:49 +0100 Subject: [PATCH 22/24] Improvement when fetching template strings. --- classes/repository/instance_repository.php | 2 +- .../model/localized_string_type.php | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/classes/repository/instance_repository.php b/classes/repository/instance_repository.php index 06c0e06..e5178ee 100644 --- a/classes/repository/instance_repository.php +++ b/classes/repository/instance_repository.php @@ -179,7 +179,7 @@ private static function get_strings(string $type, int $subratingid, int $instanc static $sortedstrings = []; $typeid = localized_string_type::str2id($type); - if ($instanceid > 0) { + if ($instanceid > 0 || localized_string_type::is_template_type($type)) { $strings = self::get_strings_for_instance($instanceid); if (array_key_exists($typeid, $strings) && array_key_exists($subratingid, $strings[$typeid])) { return $strings[$typeid][$subratingid]; diff --git a/classes/repository/model/localized_string_type.php b/classes/repository/model/localized_string_type.php index 531cac0..31d629c 100644 --- a/classes/repository/model/localized_string_type.php +++ b/classes/repository/model/localized_string_type.php @@ -126,4 +126,23 @@ public static function id2str(int $id):string { } return $constants[$id - 1]; } + + /** + * Return whether a type is a template type. + * + * @param string $type + * @return bool + */ + public static function is_template_type(string $type): bool { + return in_array($type, [ + self::TEMPLATE_CRITERION, + self::TEMPLATE_CATEGORY_HEADER, + self::TEMPLATE_SUBRATING_TITLE, + self::TEMPLATE_SUBRATING_DESCRIPTION, + self::TEMPLATE_SUBRATING_VERY_NEGATIVE, + self::TEMPLATE_SUBRATING_NEGATIVE, + self::TEMPLATE_SUBRATING_POSITIVE, + self::TEMPLATE_SUBRATING_VERY_POSITIVE, + ]); + } } From b306ee6523d7ecbf39d8c9de08cba1e695318d1b Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Thu, 5 Dec 2024 14:50:11 +0100 Subject: [PATCH 23/24] Fix typeid in backup and restore. --- .../moodle2/backup_verbalfeedback_stepslib.php | 16 ++++++++-------- .../moodle2/restore_verbalfeedback_stepslib.php | 3 +-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/backup/moodle2/backup_verbalfeedback_stepslib.php b/backup/moodle2/backup_verbalfeedback_stepslib.php index 6a1a865..c4bbb66 100644 --- a/backup/moodle2/backup_verbalfeedback_stepslib.php +++ b/backup/moodle2/backup_verbalfeedback_stepslib.php @@ -58,35 +58,35 @@ protected function define_structure() { $category = new backup_nested_element('category', ['id'], ['instanceid', 'paramtemplatecategoryid', 'position', 'weight']); $categoryheaders = new backup_nested_element('categoryheaders'); - $categoryheader = new backup_nested_element('categoryheader', ['id'], ['foreignkey', 'type', 'languageid', 'string']); + $categoryheader = new backup_nested_element('categoryheader', ['id'], ['foreignkey', 'typeid', 'languageid', 'string']); $criteria = new backup_nested_element('criteria'); $criterion = new backup_nested_element('criterion', ['id'], ['paramtemplatecriterionid', 'categoryid', 'position', 'weight']); $criteriontexts = new backup_nested_element('criteriontexts'); - $criteriontext = new backup_nested_element('criteriontext', ['id'], ['foreignkey', 'type', 'languageid', 'string']); + $criteriontext = new backup_nested_element('criteriontext', ['id'], ['foreignkey', 'typeid', 'languageid', 'string']); $subratings = new backup_nested_element('subratings'); $subrating = new backup_nested_element('subrating', ['id'], ['criterionid']); $subratingtitles = new backup_nested_element('titles'); - $subratingtitle = new backup_nested_element('title', ['id'], ['foreignkey', 'type', 'languageid', 'string']); + $subratingtitle = new backup_nested_element('title', ['id'], ['foreignkey', 'typeid', 'languageid', 'string']); $subratingdescriptions = new backup_nested_element('descriptions'); - $subratingdescription = new backup_nested_element('description', ['id'], ['foreignkey', 'type', 'languageid', 'string']); + $subratingdescription = new backup_nested_element('description', ['id'], ['foreignkey', 'typeid', 'languageid', 'string']); $subratingverynegatives = new backup_nested_element('verynegatives'); - $subratingverynegative = new backup_nested_element('verynegative', ['id'], ['foreignkey', 'type', 'languageid', 'string']); + $subratingverynegative = new backup_nested_element('verynegative', ['id'], ['foreignkey', 'typeid', 'languageid', 'string']); $subratingnegatives = new backup_nested_element('negatives'); - $subratingnegative = new backup_nested_element('negative', ['id'], ['foreignkey', 'type', 'languageid', 'string']); + $subratingnegative = new backup_nested_element('negative', ['id'], ['foreignkey', 'typeid', 'languageid', 'string']); $subratingpositives = new backup_nested_element('positives'); - $subratingpositive = new backup_nested_element('positive', ['id'], ['foreignkey', 'type', 'languageid', 'string']); + $subratingpositive = new backup_nested_element('positive', ['id'], ['foreignkey', 'typeid', 'languageid', 'string']); $subratingverypositives = new backup_nested_element('verypositives'); - $subratingverypositive = new backup_nested_element('verypositive', ['id'], ['foreignkey', 'type', 'languageid', 'string']); + $subratingverypositive = new backup_nested_element('verypositive', ['id'], ['foreignkey', 'typeid', 'languageid', 'string']); $submissions = new backup_nested_element('submissions'); $submission = new backup_nested_element('submission', ['id'], ['instanceid', 'fromuserid', 'touserid', 'status', diff --git a/backup/moodle2/restore_verbalfeedback_stepslib.php b/backup/moodle2/restore_verbalfeedback_stepslib.php index 8a09fd5..2023fe6 100644 --- a/backup/moodle2/restore_verbalfeedback_stepslib.php +++ b/backup/moodle2/restore_verbalfeedback_stepslib.php @@ -335,8 +335,7 @@ private function process_localized_string($foreigenkeymapping, $data) { $data = (object) $data; $data->foreignkey = $this->get_mappingid($foreigenkeymapping, $data->foreignkey); $data->languageid = $this->get_mappingid('language', $data->languageid); - $data->typeid = localized_string_type::str2id($data->type); - unset($data->type); + $data->typeid = $this->get_mappingid('typeid', $data->typeid); $data->instanceid = $this->instanceid; $DB->insert_record('verbalfeedback_local_string', $data); } From 80eac599fe7b0c8ded3c3f1562b29d77a35e7054 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Thu, 5 Dec 2024 16:27:14 +0100 Subject: [PATCH 24/24] Fix restore strings. --- backup/moodle2/restore_verbalfeedback_stepslib.php | 1 - 1 file changed, 1 deletion(-) diff --git a/backup/moodle2/restore_verbalfeedback_stepslib.php b/backup/moodle2/restore_verbalfeedback_stepslib.php index 2023fe6..ca538a6 100644 --- a/backup/moodle2/restore_verbalfeedback_stepslib.php +++ b/backup/moodle2/restore_verbalfeedback_stepslib.php @@ -335,7 +335,6 @@ private function process_localized_string($foreigenkeymapping, $data) { $data = (object) $data; $data->foreignkey = $this->get_mappingid($foreigenkeymapping, $data->foreignkey); $data->languageid = $this->get_mappingid('language', $data->languageid); - $data->typeid = $this->get_mappingid('typeid', $data->typeid); $data->instanceid = $this->instanceid; $DB->insert_record('verbalfeedback_local_string', $data); }