Fix some issues noticed while reviewing the previous change

attempt.php

@@ -63,7 +63,6 @@
 
 $studentquiz = mod_studentquiz_load_studentquiz($cmid, $context->id);
 
-global $USER, $COURSE;
 $userid = $USER->id;
 
 $questionusage = question_engine::load_questions_usage_by_activity($attempt->questionusageid);

classes/completion/custom_completion.php

@@ -26,30 +26,25 @@
 use stdClass;
 
 /**
- * Activity custom completion subclass for the data activity.
+ * Activity custom completion subclass for the StudentQuiz activity.
  *
  * Class for defining mod_studentquiz's custom completion rules and fetching the completion statuses
  * of the custom completion rules for a given data instance and a user.
  *
- * @package mod_studentquiz
+ * @package   mod_studentquiz
  * @copyright 2023 The Open University
- * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class custom_completion extends activity_custom_completion {
-    /**
-     * Fetches the completion state for a given completion rule.
-     *
-     * @param string $rule The completion rule.
-     * @return int The completion state.
-     */
+
     public function get_state(string $rule): int {
         global $DB;
+
         $studentquizid = $this->cm->instance;
-        if (!$studentquiz = $DB->get_record('studentquiz', ['id' => $studentquizid])) {
-            throw new \moodle_exception('Unable to find studentquiz with id ' . $studentquizid);
-        }
+        $studentquiz = $DB->get_record('studentquiz', ['id' => $studentquizid], '*', MUST_EXIST);
         $report = new mod_studentquiz_report($this->cm->id, $this->userid);
         $userstats = $report->get_user_stats();
+
         switch ($rule) {
             case 'completionpoint':
                 $status = $studentquiz->completionpoint <= (int) $userstats->points;
@@ -61,18 +56,13 @@ public function get_state(string $rule): int {
                 $status = $studentquiz->completionquestionapproved <= (int) $userstats->questions_approved;
                 break;
             default:
-                $status = COMPLETION_INCOMPLETE;
+                $status = false;
                 break;
         }
 
         return $status ? COMPLETION_COMPLETE : COMPLETION_INCOMPLETE;
     }
 
-    /**
-     * Fetch the list of custom completion rules that this module defines.
-     *
-     * @return array An array of custom completion rules.
-     */
     public static function get_defined_custom_rules(): array {
         return [
             'completionpoint',
@@ -81,11 +71,6 @@ public static function get_defined_custom_rules(): array {
         ];
     }
 
-    /**
-     * Returns an associative array of the descriptions of custom completion rules.
-     *
-     * @return array An array of custom completion rules description.
-     */
     public function get_custom_rule_descriptions(): array {
         $completionpoint = $this->cm->customdata->customcompletionrules['completionpoint'] ?? 0;
         $completionquestionpublished = $this->cm->customdata->customcompletionrules['completionquestionpublished'] ?? 0;
@@ -100,11 +85,6 @@ public function get_custom_rule_descriptions(): array {
         ];
     }
 
-    /**
-     * Returns an array of all completion rules, in the order they should be displayed to users.
-     *
-     * @return array An array of all completion rules.
-     */
     public function get_sort_order(): array {
         $defaults = [
            'completionview',
@@ -113,19 +93,17 @@ public function get_sort_order(): array {
         return array_merge($defaults, self::get_defined_custom_rules());
     }
 
-
     /**
-     * Update state for the completion.
+     * Update completion state for a given user on a given StudentQuiz.
      *
-     * @param stdClass $course The course object.
-     * @param stdClass|cm_info $cm Course module.
-     * @param int|null $userid Id of user creating the question.
+     * @param stdClass $course The course containing the StudentQuiz to update.
+     * @param stdClass|cm_info $cm The cm for the StudentQuiz to update.
+     * @param int|null $userid The user to update state for.
      */
     public static function update_state(stdClass $course, $cm, ?int $userid = null): void {
         $completion = new \completion_info($course);
         if ($completion->is_enabled($cm)) {
             $completion->update_state($cm, COMPLETION_UNKNOWN, $userid);
         }
     }
-
 }

classes/external/update_question_state.php

@@ -107,9 +107,10 @@ public static function execute($courseid, $cmid, $studentquizquestionid, $state)
             $statename = studentquiz_helper::$statename[$state];
             mod_studentquiz_event_notification_question($statename, $studentquizquestion, $course, $cm);
         }
-        $userid = $studentquizquestion->get_question()->createdby;
+
         // Update completion state.
-        \mod_studentquiz\completion\custom_completion::update_state($course, $cm, $userid);
+        \mod_studentquiz\completion\custom_completion::update_state($course, $cm,
+            $studentquizquestion->get_question()->createdby);
 
         $result = [];
         $result['status'] = get_string('api_state_change_success_title', 'studentquiz');

save.php

@@ -36,24 +36,14 @@
 global $COURSE;
 
 // Get parameters.
-$cmid = optional_param('cmid', 0, PARAM_INT);
+$cmid = required_param('cmid', PARAM_INT);
 $studentquizquestionid = required_param('studentquizquestionid', PARAM_INT);
 $save = required_param('save', PARAM_NOTAGS);
 
-// Load course and course module requested.
-if ($cmid) {
-    if (!$module = get_coursemodule_from_id('studentquiz', $cmid)) {
-        throw new moodle_exception("invalidcoursemodule");
-    }
-    if (!$course = $DB->get_record('course', array('id' => $module->course))) {
-        throw new moodle_exception("coursemisconf");
-    }
-} else {
-    throw new moodle_exception("invalidcoursemodule");
-}
+[$course, $cm] = get_course_and_cm_from_cmid($cmid, 'studentquiz');
 
 // Authentication check.
-require_login($module->course, false, $module);
+require_login($course, false, $cm);
 require_sesskey();
 
 $data = new \stdClass();
@@ -72,13 +62,14 @@
         mod_studentquiz\utils::save_rate($data);
         break;
 }
+
 $contextmodule = \context_module::instance($cmid);
-$cm = get_coursemodule_from_id('studentquiz', $cmid);
 $studentquiz = mod_studentquiz_load_studentquiz($cmid, $contextmodule->id);
-$studentquizquestion = new mod_studentquiz\local\studentquiz_question($studentquizquestionid, null,
-    $studentquiz);
-$userid = $studentquizquestion->get_question()->createdby;
+$studentquizquestion = new mod_studentquiz\local\studentquiz_question(
+    $studentquizquestionid, null, $studentquiz);
+
 // Update completion state.
-\mod_studentquiz\completion\custom_completion::update_state($COURSE, $cm, $userid);
+\mod_studentquiz\completion\custom_completion::update_state(
+    $course, $cm, $studentquizquestion->get_question()->createdby);
 
 header('Content-Type: text/html; charset=utf-8');

tests/behat/custom_completion.feature

@@ -45,9 +45,9 @@ Feature: Set a studentquiz to be marked complete when the student meets the cond
   Scenario: Check studentquiz mark done when the student meets the conditions of the completion created
     Given I add a "StudentQuiz" to section "1"
     When I set the following fields to these values:
-      | StudentQuiz Name                 | StudentQuiz 1                |
-      | Description                      | Test studentquiz description |
-      | completion                       | 2                            |
+      | StudentQuiz Name                   | StudentQuiz 1                |
+      | Description                        | Test studentquiz description |
+      | completion                         | 2                            |
       | completionquestionpublishedenabled | 1                            |
       | completionquestionpublished        | 2                            |
     And I press "Save and display"

tests/custom_completion_test.php

@@ -36,7 +36,6 @@ class custom_completion_test extends advanced_testcase {
      */
     public function test_get_defined_custom_rules(): void {
         $rules = custom_completion::get_defined_custom_rules();
-        $this->assertCount(3, $rules);
         $this->assertEquals(
             ['completionpoint', 'completionquestionpublished', 'completionquestionapproved'],
             $rules