MDL-48778 assign: Use proper latest attempt for quickgrading prechecks
authorEric Merrill <merrill@oakland.edu>
Wed, 21 Jan 2015 14:48:15 +0000 (09:48 -0500)
committerSimey Lameze <simey@moodle.com>
Mon, 8 Feb 2016 02:34:24 +0000 (10:34 +0800)
The code that stored lastmodified in gradingtable used a different
method to produce it than the processor used to generate the check
value. Fix that and also check that the attempt number has not changed.

mod/assign/gradingtable.php
mod/assign/locallib.php
mod/assign/tests/events_test.php
mod/assign/tests/locallib_test.php

index 1d5ec06..6e80cad 100644 (file)
@@ -774,6 +774,9 @@ class assign_grading_table extends table_sql implements renderable {
         $selectcol .= '<input type="hidden"
                               name="grademodified_' . $row->userid . '"
                               value="' . $row->timemarked . '"/>';
+        $selectcol .= '<input type="hidden"
+                              name="gradeattempt_' . $row->userid . '"
+                              value="' . $row->attemptnumber . '"/>';
         return $selectcol;
     }
 
index f353948..46008dc 100644 (file)
@@ -5399,6 +5399,7 @@ class assign {
         // Gets a list of possible users and look for values based upon that.
         foreach ($participants as $userid => $unused) {
             $modified = optional_param('grademodified_' . $userid, -1, PARAM_INT);
+            $attemptnumber = optional_param('gradeattempt_' . $userid, -1, PARAM_INT);
             // Gather the userid, updated grade and last modified value.
             $record = new stdClass();
             $record->userid = $userid;
@@ -5410,6 +5411,7 @@ class assign {
                 // This user was not in the grading table.
                 continue;
             }
+            $record->attemptnumber = $attemptnumber;
             $record->lastmodified = $modified;
             $record->gradinginfo = grade_get_grades($this->get_course()->id,
                                                     'mod',
@@ -5428,19 +5430,20 @@ class assign {
         $params['assignid2'] = $this->get_instance()->id;
 
         // Check them all for currency.
-        $grademaxattempt = 'SELECT mxg.userid, MAX(mxg.attemptnumber) AS maxattempt
-                            FROM {assign_grades} mxg
-                            WHERE mxg.assignment = :assignid1 GROUP BY mxg.userid';
-
-        $sql = 'SELECT u.id as userid, g.grade as grade, g.timemodified as lastmodified, uf.workflowstate, uf.allocatedmarker
-                    FROM {user} u
-                LEFT JOIN ( ' . $grademaxattempt . ' ) gmx ON u.id = gmx.userid
-                LEFT JOIN {assign_grades} g ON
-                    u.id = g.userid AND
-                    g.assignment = :assignid2 AND
-                    g.attemptnumber = gmx.maxattempt
-                LEFT JOIN {assign_user_flags} uf ON uf.assignment = g.assignment AND uf.userid = g.userid
-                WHERE u.id ' . $userids;
+        $grademaxattempt = 'SELECT s.userid, s.attemptnumber AS maxattempt
+                              FROM {assign_submission} s
+                             WHERE s.assignment = :assignid1 AND s.latest = 1';
+
+        $sql = 'SELECT u.id AS userid, g.grade AS grade, g.timemodified AS lastmodified,
+                       uf.workflowstate, uf.allocatedmarker, gmx.maxattempt AS attemptnumber
+                  FROM {user} u
+             LEFT JOIN ( ' . $grademaxattempt . ' ) gmx ON u.id = gmx.userid
+             LEFT JOIN {assign_grades} g ON
+                       u.id = g.userid AND
+                       g.assignment = :assignid2 AND
+                       g.attemptnumber = gmx.maxattempt
+             LEFT JOIN {assign_user_flags} uf ON uf.assignment = g.assignment AND uf.userid = g.userid
+                 WHERE u.id ' . $userids;
         $currentgrades = $DB->get_recordset_sql($sql, $params);
 
         $modifiedusers = array();
@@ -5504,7 +5507,9 @@ class assign {
                 if ($this->grading_disabled($modified->userid)) {
                     continue;
                 }
-                if ((int)$current->lastmodified > (int)$modified->lastmodified) {
+                $badmodified = (int)$current->lastmodified > (int)$modified->lastmodified;
+                $badattempt = (int)$current->attemptnumber != (int)$modified->attemptnumber;
+                if ($badmodified || $badattempt) {
                     // Error - record has been modified since viewing the page.
                     return get_string('errorrecordmodified', 'assign');
                 } else {
index 14d68ec..787350b 100644 (file)
@@ -436,6 +436,7 @@ class assign_events_testcase extends mod_assign_base_testcase {
 
         $data = array(
             'grademodified_' . $this->students[0]->id => time(),
+            'gradeattempt_' . $this->students[0]->id => '',
             'quickgrade_' . $this->students[0]->id => '60.0',
             'quickgrade_' . $this->students[0]->id . '_workflowstate' => 'inmarking'
         );
@@ -565,8 +566,10 @@ class assign_events_testcase extends mod_assign_base_testcase {
         // Test process_save_quick_grades.
         $sink = $this->redirectEvents();
 
+        $grade = $assign->get_user_grade($this->students[0]->id, false);
         $data = array(
             'grademodified_' . $this->students[0]->id => time(),
+            'gradeattempt_' . $this->students[0]->id => $grade->attemptnumber,
             'quickgrade_' . $this->students[0]->id => '60.0'
         );
         $assign->testable_process_save_quick_grades($data);
index dee37fa..547ac86 100644 (file)
@@ -2329,5 +2329,75 @@ Anchor link 2:<a title=\"bananas\" href=\"../logo-240x60.gif\">Link text</a>
         $this->assertTrue(in_array($this->extrastudents[0]->id, $allgroupmembers));
         $this->assertTrue(in_array($this->extrastudents[1]->id , $allgroupmembers));
     }
-}
 
+    /**
+     * Test the quicksave grades processor
+     */
+    public function test_process_save_quick_grades() {
+        $this->editingteachers[0]->ignoresesskey = true;
+        $this->setUser($this->editingteachers[0]);
+
+        $assign = $this->create_instance(array('attemptreopenmethod' => ASSIGN_ATTEMPT_REOPEN_METHOD_MANUAL));
+
+        // Initially grade the user.
+        $grade = $assign->get_user_grade($this->students[0]->id, false);
+        if (!$grade) {
+            $grade = new stdClass();
+            $grade->attemptnumber = '';
+            $grade->timemodified = '';
+        }
+        $data = array(
+            'grademodified_' . $this->students[0]->id => $grade->timemodified,
+            'gradeattempt_' . $this->students[0]->id => $grade->attemptnumber,
+            'quickgrade_' . $this->students[0]->id => '60.0'
+        );
+        $result = $assign->testable_process_save_quick_grades($data);
+        $this->assertContains(get_string('quickgradingchangessaved', 'assign'), $result);
+        $grade = $assign->get_user_grade($this->students[0]->id, false);
+        $this->assertEquals('60.0', $grade->grade);
+
+        // Attempt to grade with a past attempts grade info.
+        $assign->testable_process_add_attempt($this->students[0]->id);
+        $data = array(
+            'grademodified_' . $this->students[0]->id => $grade->timemodified,
+            'gradeattempt_' . $this->students[0]->id => $grade->attemptnumber,
+            'quickgrade_' . $this->students[0]->id => '50.0'
+        );
+        $result = $assign->testable_process_save_quick_grades($data);
+        $this->assertContains(get_string('errorrecordmodified', 'assign'), $result);
+        $grade = $assign->get_user_grade($this->students[0]->id, false);
+        $this->assertFalse($grade);
+
+        // Attempt to grade a the attempt.
+        $submission = $assign->get_user_submission($this->students[0]->id, false);
+        $data = array(
+            'grademodified_' . $this->students[0]->id => '',
+            'gradeattempt_' . $this->students[0]->id => $submission->attemptnumber,
+            'quickgrade_' . $this->students[0]->id => '40.0'
+        );
+        $result = $assign->testable_process_save_quick_grades($data);
+        $this->assertContains(get_string('quickgradingchangessaved', 'assign'), $result);
+        $grade = $assign->get_user_grade($this->students[0]->id, false);
+        $this->assertEquals('40.0', $grade->grade);
+
+        // Catch grade update conflicts.
+        // Save old data for later.
+        $pastdata = $data;
+        // Update the grade the 'good' way.
+        $data = array(
+            'grademodified_' . $this->students[0]->id => $grade->timemodified,
+            'gradeattempt_' . $this->students[0]->id => $grade->attemptnumber,
+            'quickgrade_' . $this->students[0]->id => '30.0'
+        );
+        $result = $assign->testable_process_save_quick_grades($data);
+        $this->assertContains(get_string('quickgradingchangessaved', 'assign'), $result);
+        $grade = $assign->get_user_grade($this->students[0]->id, false);
+        $this->assertEquals('30.0', $grade->grade);
+
+        // Now update using 'old' data. Should fail.
+        $result = $assign->testable_process_save_quick_grades($pastdata);
+        $this->assertContains(get_string('errorrecordmodified', 'assign'), $result);
+        $grade = $assign->get_user_grade($this->students[0]->id, false);
+        $this->assertEquals('30.0', $grade->grade);
+    }
+}