From c30fb8fc3b5532d51380ba69b493fb258c2d70b3 Mon Sep 17 00:00:00 2001
From: Markus Heck <markus.heck@hs-kempten.de>
Date: Sun, 12 May 2024 02:41:31 +0200
Subject: [PATCH] documentation about actual behaviour

---
 lib.php            | 14 ++++++++------
 tests/lib_test.php |  1 +
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/lib.php b/lib.php
index a39eea2..e7160fb 100644
--- 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 {
@@ -120,13 +122,13 @@ function adleradaptivity_delete_instance(int $instance_id): bool {
     } 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
index 016d957..a422e35 100644
--- a/tests/lib_test.php
+++ b/tests/lib_test.php
@@ -10,6 +10,7 @@
 require_once($CFG->dirroot . '/mod/adleradaptivity/tests/lib/adler_testcase.php');
 require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php');
 
+// TODO: properly adjust testcases for exception instead of "false" response
 
 class lib_test extends adler_testcase {
     public function test_add_instance() {