refactor: update qtype_questionpy table

questionpy-org · Aug 29, 2024 · 0271685 · 0271685
1 parent a0e3f0a
commit 0271685
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 39 deletions.
diff --git a/classes/question_service.php b/classes/question_service.php
@@ -77,8 +77,8 @@ public function get_question(int $questionid): object {
                 'qtype_questionpy',
                 'package',
                 $record->id,
-                'itemid, filepath, filename',
-                false
+                includedirs: false,
+                limitnum: 1,
             );
             if (count($files) === 0) {
                 throw new \coding_exception(
@@ -88,10 +88,10 @@ public function get_question(int $questionid): object {
             }
         } else {
             // Package was selected.
-            $package = package_version::get_by_id($record->pkgversionid);
+            $package = package_version::get_by_hash($record->pkgversionhash);
             if (is_null($package)) {
                 throw new \coding_exception(
-                    "No package version record with ID '{$record->pkgversionid}' was found despite being referenced" .
+                    "No package version record with hash '{$record->pkversionhash}' was found despite being referenced" .
                     " by question {$questionid}"
                 );
             }
@@ -123,25 +123,31 @@ public function upsert_question(object $question): void {
         $question->qpy_package_hash ??= $question->qpy_package_file_hash;
 
         $file = null;
-        $pkgversionid = null;
         if ($question->qpy_package_source === 'upload') {
             if (isset($question->qpy_package_path_name_hash)) {
                 $file = $filestorage->get_file_by_hash($question->qpy_package_path_name_hash);
             } else {
                 $file = $this->packagefileservice->get_draft_file($question->qpy_package_file);
             }
+            $rawpackage = api::extract_package_info($file);
+            $pkgversionhash = $rawpackage->hash;
+            $pkgversionnamespace = $rawpackage->namespace;
+            $pkgversionshortname = $rawpackage->shortname;
         } else {
-            $pkgversionid = package_version::get_by_hash($question->qpy_package_hash)->id ?? null;
-            if (!$pkgversionid) {
+            $pkgversion = package_version::get_by_hash($question->qpy_package_hash) ?? null;
+            if (!$pkgversion) {
                 throw new moodle_exception(
                     'package_not_found',
                     'qtype_questionpy',
                     '',
                     (object)['packagehash' => $question->qpy_package_hash]
                 );
             }
-            $packageid = package::get_by_version($pkgversionid)->id;
-            last_used_service::add($question->context->id, $packageid);
+            $package = package::get_by_version($pkgversion->id);
+            last_used_service::add($question->context->id, $package->id);
+            $pkgversionhash = $pkgversion->hash;
+            $pkgversionnamespace = $package->namespace;
+            $pkgversionshortname = $package->shortname;
         }
 
         $existingrecord = $DB->get_record(self::QUESTION_TABLE, [
@@ -161,12 +167,13 @@ public function upsert_question(object $question): void {
             // Question record already exists, update it if necessary.
             $update = ['id' => $existingrecord->id];
 
+            if ($existingrecord->pkgversionhash !== $pkgversionhash) {
+                $update['pkgversionhash'] = $pkgversionhash;
+            }
+
             if ($existingrecord->state !== $response->state) {
                 $update['state'] = $response->state;
             }
-            if ($pkgversionid !== $existingrecord->pkgversionid) {
-                $update['pkgversionid'] = $pkgversionid;
-            }
 
             if (count($update) > 1) {
                 $DB->update_record(self::QUESTION_TABLE, (object) $update);
@@ -176,9 +183,9 @@ public function upsert_question(object $question): void {
             // Insert a new record with the question state only containing the options.
             $questionid = $DB->insert_record(self::QUESTION_TABLE, [
                 'questionid' => $question->id,
-                'feedback' => '',
+                // TODO: retrieve the identifier directly from the package?
+                'packageidentifier' => "@{$pkgversionnamespace}/{$pkgversionshortname}",
                 'pkgversionhash' => $question->qpy_package_hash,
-                'pkgversionid' => $pkgversionid,
                 'islocal' => $islocal,
                 'state' => $response->state,
             ]);

diff --git a/db/install.xml b/db/install.xml
@@ -25,16 +25,14 @@
       <FIELDS>
         <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
         <FIELD NAME="questionid" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false" COMMENT="Foreign key references question.id" />
-        <FIELD NAME="feedback" TYPE="text" NOTNULL="false" SEQUENCE="false" COMMENT="Feedback shown for any response."/>
+        <FIELD NAME="packageidentifier" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false" />
         <FIELD NAME="pkgversionhash" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false"/>
-        <FIELD NAME="pkgversionid" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/>
         <FIELD NAME="islocal" TYPE="int" LENGTH="1" NOTNULL="true" SEQUENCE="false"/>
         <FIELD NAME="state" TYPE="text" NOTNULL="true" SEQUENCE="false"/>
       </FIELDS>
       <KEYS>
         <KEY NAME="primary" TYPE="primary" FIELDS="id" />
         <KEY NAME="questionid" TYPE="foreign-unique" FIELDS="questionid" REFTABLE="question" REFFIELDS="id" />
-        <KEY NAME="pkgversionid" TYPE="foreign" FIELDS="pkgversionid" REFTABLE="qtype_questionpy_pkgversion" REFFIELDS="id"/>
       </KEYS>
     </TABLE>
 

diff --git a/tests/question_service_test.php b/tests/question_service_test.php
@@ -63,8 +63,8 @@ public function test_get_question_should_load_package_and_state(): void {
         $this->resetAfterTest();
 
         $pvi = package_versions_info_provider();
-        [, [$pkgversionid]] = $pvi->upsert();
-        [$statestr, $qpyid] = $this->setup_question($pvi->versions[0]->hash, $pkgversionid);
+        $pvi->upsert();
+        [$statestr, $qpyid] = $this->setup_question($pvi->versions[0]->hash);
 
         $result = $this->questionservice->get_question(1);
 
@@ -103,9 +103,9 @@ public function test_upsert_question_should_update_existing_record_if_changed():
         $this->resetAfterTest();
 
         $pvi = package_versions_info_provider(null, [["version" => "0.2.0"], ["version" => "0.1.0"]]);
-        [, [$newpackageid, $oldpackageid]] = $pvi->upsert();
+        $pvi->upsert();
 
-        $oldstate = $this->setup_question($pvi->versions[1]->hash, $oldpackageid)[0];
+        $oldstate = $this->setup_question($pvi->versions[1]->hash)[0];
 
         $newstate = json_encode(["this is" => "new state"]);
         $formdata = ["this is" => "form data"];
@@ -127,7 +127,7 @@ public function test_upsert_question_should_update_existing_record_if_changed():
             ]
         );
 
-        $this->assert_single_question(1, $newpackageid, $newstate);
+        $this->assert_single_question(1, $pvi->versions[0]->hash, $newstate);
     }
 
     /**
@@ -143,17 +143,17 @@ public function test_upsert_question_should_do_nothing_if_unchanged(): void {
         $this->resetAfterTest();
 
         $pvi = package_versions_info_provider();
-        [, [$pkgversionid]] = $pvi->upsert();
+        $pvi->upsert();
 
-        $oldstate = $this->setup_question($pvi->versions[0]->hash, $pkgversionid)[0];
+        $oldstate = $this->setup_question($pvi->versions[0]->hash)[0];
 
         $formdata = ["this is" => "form data"];
 
         $this->packageapi
             ->expects($this->once())
             ->method("create_question")
             ->with($oldstate, (object) $formdata)
-            ->willReturn(new question_response($oldstate, "", ""));
+            ->willReturn(new question_response($oldstate, ""));
 
         $this->questionservice->upsert_question(
             (object)[
@@ -166,7 +166,7 @@ public function test_upsert_question_should_do_nothing_if_unchanged(): void {
             ]
         );
 
-        $this->assert_single_question(1, $pkgversionid, $oldstate);
+        $this->assert_single_question(1, $pvi->versions[0]->hash, $oldstate);
     }
 
     /**
@@ -183,7 +183,7 @@ public function test_upsert_question_should_insert_record(): void {
         $this->resetAfterTest();
 
         $pvi = package_versions_info_provider();
-        [, [$pkgversionid]] = $pvi->upsert();
+        $pvi->upsert();
 
         $newstate = json_encode(["this is" => "new state"]);
         $formdata = ["this is" => "form data"];
@@ -192,7 +192,7 @@ public function test_upsert_question_should_insert_record(): void {
             ->expects($this->once())
             ->method("create_question")
             ->with(null, (object) $formdata)
-            ->willReturn(new question_response($newstate, "", ""));
+            ->willReturn(new question_response($newstate, ""));
 
         $this->questionservice->upsert_question(
             (object)[
@@ -205,7 +205,7 @@ public function test_upsert_question_should_insert_record(): void {
             ]
         );
 
-        $this->assert_single_question(42, $pkgversionid, $newstate);
+        $this->assert_single_question(42, $pvi->versions[0]->hash, $newstate);
     }
 
     /**
@@ -348,8 +348,8 @@ public function test_delete_question(): void {
         $this->resetAfterTest();
 
         $pvi = package_versions_info_provider();
-        [, [$pkgversionid]] = $pvi->upsert();
-        $this->setup_question($pvi->versions[0]->hash, $pkgversionid);
+        $pvi->upsert();
+        $this->setup_question($pvi->versions[0]->hash);
 
         global $DB;
         $this->assertEquals(1, $DB->count_records("qtype_questionpy"));
@@ -363,11 +363,10 @@ public function test_delete_question(): void {
      * Inserts a question using the given package into the DB and returns the state string and question id.
      *
      * @param string $pkgversionhash package version hash
-     * @param int|null $pkgversionid database ID (not hash) of a package version
      * @return array[string, int]
      * @throws dml_exception
      */
-    private function setup_question(string $pkgversionhash, ?int $pkgversionid): array {
+    private function setup_question(string $pkgversionhash): array {
         $this->resetAfterTest();
 
         $statestr = '
@@ -380,10 +379,8 @@ private function setup_question(string $pkgversionhash, ?int $pkgversionid): arr
         $qpyid = $DB->insert_record("qtype_questionpy", [
             "id" => 1,
             "questionid" => 1,
-            "feedback" => "",
             "pkgversionhash" => $pkgversionhash,
-            "pkgversionid" => $pkgversionid,
-            "islocal" => is_null($pkgversionid),
+            "islocal" => false,
             "state" => $statestr,
         ]);
 
@@ -394,19 +391,19 @@ private function setup_question(string $pkgversionhash, ?int $pkgversionid): arr
      * Asserts that a single question exists in `qtype_questionpy` and it matches the given arguments.
      *
      * @param int $id expected question id
-     * @param int $pkgversionid expected package id
+     * @param string $pkgversionhash expected package hash
      * @param string $state expected state
      * @return void
      * @throws dml_exception
      */
-    private function assert_single_question(int $id, int $pkgversionid, string $state) {
+    private function assert_single_question(int $id, string $pkgversionhash, string $state) {
         global $DB;
         $records = $DB->get_records("qtype_questionpy");
         $this->assertCount(1, $records);
         $record = current($records);
 
         $this->assertEquals((string) $id, $record->questionid);
-        $this->assertEquals((string) $pkgversionid, $record->pkgversionid);
+        $this->assertEquals((string) $pkgversionhash, $record->pkgversionhash);
         $this->assertEquals($state, $record->state);
     }
 }