MDL-72451 core: Stop keep regrading when there is an error

Set needsupdate to finish to avoid regrading each call.
Add error messages for regrading.
Display the error message on the quiz page.

Co-authored-by: Mark Johnson <[email protected]>

lang/en/grades.php

@@ -184,7 +184,7 @@
 $string['encoding_help'] = 'Select the character encoding used for the data. (The standard encoding is UTF-8.) If the wrong encoding is selected by mistake, it will be noticeable when previewing the data for import.';
 $string['errorcalculationnoequal'] = 'Formula must start with equal sign (=1+2)';
 $string['errorcalculationunknown'] = 'Invalid formula';
-$string['errorcalculationbroken'] = 'Probably circular reference or broken calculation formula';
+$string['errorcalculationbroken'] = 'Probably circular reference or broken calculation formula (Item name: {$a})';
 $string['errorgradevaluenonnumeric'] = 'Received non-numeric for low or high grade for';
 $string['errornocalculationallowed'] = 'Calculations are not allowed for this item';
 $string['errornocategorisedid'] = 'Could not get an uncategorised id!';

lib/gradelib.php

@@ -443,13 +443,20 @@ function grade_get_grades($courseid, $itemtype, $itemmodule, $iteminstance, $use
     $return = new stdClass();
     $return->items    = array();
     $return->outcomes = array();
+    $return->errors = [];
 
-    $course_item = grade_item::fetch_course_item($courseid);
+    $courseitem = grade_item::fetch_course_item($courseid);
     $needsupdate = array();
-    if ($course_item->needsupdate) {
+    if ($courseitem->needsupdate) {
         $result = grade_regrade_final_grades($courseid);
         if ($result !== true) {
             $needsupdate = array_keys($result);
+            // Return regrade errors if the user has capability.
+            $context = context_course::instance($courseid);
+            if (has_capability('moodle/grade:edit', $context)) {
+                $return->errors = $result;
+            }
+            $courseitem->regrading_finished();
         }
     }
 
@@ -1309,7 +1316,10 @@ function grade_regrade_final_grades($courseid, $userid=null, $updated_item=null,
                     continue; // this one is ok
                 }
                 $grade_items[$gid]->force_regrading();
-                $errors[$grade_items[$gid]->id] = get_string('errorcalculationbroken', 'grades');
+                if (!empty($grade_items[$gid]->calculation) && empty($errors[$gid])) {
+                    $itemname = $grade_items[$gid]->get_name();
+                    $errors[$gid] = get_string('errorcalculationbroken', 'grades', $itemname);
+                }
             }
             break; // Found error.
         }

lib/tests/gradelib_test.php

@@ -302,4 +302,48 @@ public function test_get_grade_letters_custom() {
 
         $this->assertEquals($expected, $actual);
     }
+
+    /**
+     * When getting a calculated grade containing an error, we mark grading finished and don't keep trying to regrade.
+     *
+     * @covers \grade_get_grades()
+     * @return void
+     */
+    public function test_grade_get_grades_errors() {
+        $this->resetAfterTest();
+
+        // Setup some basics.
+        $course = $this->getDataGenerator()->create_course();
+        $user1 = $this->getDataGenerator()->create_user();
+        $this->getDataGenerator()->enrol_user($user1->id, $course->id, 'student');
+        $user2 = $this->getDataGenerator()->create_user();
+        $this->getDataGenerator()->enrol_user($user2->id, $course->id, 'editingteacher');
+        // Set up 2 gradeable activities.
+        $assign = $this->getDataGenerator()->create_module('assign', ['idnumber' => 'a1', 'course' => $course->id]);
+        $quiz = $this->getDataGenerator()->create_module('quiz', ['idnumber' => 'q1', 'course' => $course->id]);
+
+        // Create a calculated grade item using the activities.
+        $params = ['courseid' => $course->id];
+        $g1 = new \grade_item($this->getDataGenerator()->create_grade_item($params));
+        $g1->set_calculation('=[[a1]] + [[q1]]');
+
+        // Now delete one of the activities to break the calculation.
+        course_delete_module($assign->cmid);
+
+        // Course grade item has needsupdate.
+        $this->assertEquals(1, \grade_item::fetch_course_item($course->id)->needsupdate);
+
+        // Get grades for the quiz, to trigger a regrade.
+        $this->setUser($user2);
+        $grades1 = grade_get_grades($course->id, 'mod', 'quiz', $quiz->id);
+        // We should get an error for the broken calculation.
+        $this->assertNotEmpty($grades1->errors);
+        $this->assertEquals(get_string('errorcalculationbroken', 'grades', $g1->itemname), reset($grades1->errors));
+        // Course grade item should not have needsupdate so that we don't try to regrade again.
+        $this->assertEquals(0, \grade_item::fetch_course_item($course->id)->needsupdate);
+
+        // Get grades for the quiz again. This should not trigger the regrade and resulting error this time.
+        $grades2 = grade_get_grades($course->id, 'mod', 'quiz', $quiz->id);
+        $this->assertEmpty($grades2->errors);
+    }
 }

mod/quiz/view.php

@@ -124,9 +124,9 @@
 
 $item = null;
 
-$grading_info = grade_get_grades($course->id, 'mod', 'quiz', $quiz->id, $USER->id);
-if (!empty($grading_info->items)) {
-    $item = $grading_info->items[0];
+$gradinginfo = grade_get_grades($course->id, 'mod', 'quiz', $quiz->id, $USER->id);
+if (!empty($gradinginfo->items)) {
+    $item = $gradinginfo->items[0];
     if (isset($item->grades[$USER->id])) {
         $grade = $item->grades[$USER->id];
 
@@ -250,6 +250,13 @@
 
 echo $OUTPUT->header();
 
+if (!empty($gradinginfo->errors)) {
+    foreach ($gradinginfo->errors as $error) {
+        $errortext = new \core\output\notification($error, \core\output\notification::NOTIFY_ERROR);
+        echo $OUTPUT->render($errortext);
+    }
+}
+
 if (isguestuser()) {
     // Guests can't do a quiz, so offer them a choice of logging in or going back.
     echo $output->view_page_guest($course, $quiz, $cm, $context, $viewobj->infomessages, $viewobj);