refactor module deletion code to throw exception on errors, instead o…

…f returning false (as suggested by documentation)
ProjektAdLer · May 13, 2024 · 6735ab1 · 6735ab1
1 parent 2fb525d
commit 6735ab1
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 25 deletions.
diff --git a/lib.php b/lib.php
@@ -85,6 +85,8 @@ function adleradaptivity_delete_instance(int $instance_id): bool {
     $adleradaptivity_repository = new adleradaptivity_repository();
 
 
+    // there is no transaction above this level. Unsuccessful deletions basically result in unpredictable
+    // behaviour. This at least ensures this module is either deleted completely or not at all.
     $transaction = $DB->start_delegated_transaction();
 
     try {
@@ -119,13 +121,14 @@ function adleradaptivity_delete_instance(int $instance_id): bool {
         $transaction->allow_commit();
     } catch (Exception $e) {
         $logger->error('Could not delete adleradaptivity instance with id ' . $instance_id);
+        $logger->error($e->getMessage());
+        // although the existing documentation suggests this method should return true|false depending
+        // on whether the deletion succeeded, this does not actually lead to the desired behaviour.
+        // In case of an error this should throw an exception as otherwise:
+        // - postgresql databases are not rolled back
+        // - course deletion succeeds without indication of an error
+        // - only module deletion behaves as expected and shows an error
         $transaction->rollback($e);
-        try {
-            $transaction->rollback($e);
-        } catch (Exception $e) {
-            // rollback triggers an exception, but I don't care. This method is expected to return false in case of an error.
-        }
-        return false;
     }
 
     return true;

diff --git a/tests/lib_test.php b/tests/lib_test.php
@@ -3,12 +3,13 @@
 use mod_adleradaptivity\external\answer_questions;
 use mod_adleradaptivity\external\external_test_helpers;
 use mod_adleradaptivity\lib\adler_testcase;
+use mod_adleradaptivity\local\db\adleradaptivity_attempt_repository;
+use mod_adleradaptivity\local\db\adleradaptivity_question_repository;
 
 global $CFG;
 require_once($CFG->dirroot . '/mod/adleradaptivity/tests/lib/adler_testcase.php');
 require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php');
 
-
 class lib_test extends adler_testcase {
     public function test_add_instance() {
         global $DB;
@@ -129,13 +130,14 @@ public function test_delete_instance_failure() {
         // Try to delete a non-existing instance.
         $nonExistingId = 999999; // This ID should be non-existing.
 
-        $result = adleradaptivity_delete_instance($nonExistingId);
-
-        // Check that the method returned false.
-        $this->assertFalse($result);
+        $this->expectException(moodle_exception::class);
 
-        // Ensure that no instances were deleted.
-        $this->assertCount(0, $DB->get_records('adleradaptivity'));
+        try {
+            adleradaptivity_delete_instance($nonExistingId);
+        } finally {
+            // Ensure that no instances were deleted.
+            $this->assertCount(0, $DB->get_records('adleradaptivity'));
+        }
     }
 
 
@@ -159,26 +161,27 @@ public function test_delete_instance_failure_due_to_question_deletion_failure()
         $answer_question_result = answer_questions::execute($answerdata[0], $answerdata[1]);
 
         // Mock the repository object
-        $mockRepo = Mockery::mock('alias:mod_adleradaptivity\local\db\adleradaptivity_question_repository');
+        $mockRepo = Mockery::mock('overload:' . adleradaptivity_question_repository::class);
         // Make the method throw an exception
-        $mockRepo->shouldReceive('delete_question_by_id')->andThrow(new Exception('Could not delete'));
+        $mockRepo->shouldReceive('delete_question_by_id')->andThrow(new moodle_exception('Could not delete'));
 
         // verify created elements before deletion
         $this->assertCount(1, $DB->get_records('adleradaptivity'));
         $this->assertCount(2, $DB->get_records('adleradaptivity_tasks'));
         $this->assertCount(1, $DB->get_records('adleradaptivity_questions'));
         $this->assertCount(1, $DB->get_records('adleradaptivity_attempts'));
 
-        // Try to delete the complex instance.
-        $result = adleradaptivity_delete_instance($complex_adleradaptivity_module['module']->id);
-
-        // Check that the method returned false.
-        $this->assertFalse($result);
+        $this->expectException(moodle_exception::class);
 
-        // Ensure that no instances were deleted.
-        $this->assertCount(1, $DB->get_records('adleradaptivity'));
-        $this->assertCount(2, $DB->get_records('adleradaptivity_tasks'));
-        $this->assertCount(1, $DB->get_records('adleradaptivity_questions'));
-        $this->assertCount(1, $DB->get_records('adleradaptivity_attempts'));
+        try {
+            // Try to delete the complex instance.
+            adleradaptivity_delete_instance($complex_adleradaptivity_module['module']->id);
+        } finally {
+            // Ensure that no instances were deleted.
+            $this->assertCount(1, $DB->get_records('adleradaptivity'));
+            $this->assertCount(2, $DB->get_records('adleradaptivity_tasks'));
+            $this->assertCount(1, $DB->get_records('adleradaptivity_questions'));
+            $this->assertCount(1, $DB->get_records('adleradaptivity_attempts'));
+        }
     }
 }