diff --git a/lib.php b/lib.php index a389c04..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 { @@ -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 index 09d9cdc..cdf233a 100644 --- 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,9 +161,9 @@ 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')); @@ -169,16 +171,17 @@ public function test_delete_instance_failure_due_to_question_deletion_failure() $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')); + } } } \ No newline at end of file