MDL-44330 Assign: Assignment grading may not display expected student

Before this patch it was possible for the student displayed on the grading page to
not be the student that the user selected to grade. This would occur if:

1) The user had the table ordered by a value that could be modified,
   for example Last modified (submission), Grade, Last modified (grade)
2) Another user performed an action that was recorded in Moodle in the time
   between the user generating the table and clicking on a grade link.

If a user did not notice a different user had been loaded it could result in them giving
a grade to the incorrect user.

This patch ensures that the state of the table is cached every time it is viewed by a user
who has the capability to grade.

diff --git a/lib/tablelib.php b/lib/tablelib.php
index e2c4b7f..53b64a6 100644 (file)
--- a/lib/tablelib.php
+++ b/lib/tablelib.php
@@ -67,6 +67,7 @@ class flexible_table {
     var $column_suppress = array();
     var $column_nosort   = array('userpic');
     private $column_textsort = array();
+    /** @var boolean Stores if setup has already been called on this flixible table. */
     var $setup           = false;
     var $baseurl         = NULL;
     var $request         = array();

diff --git a/mod/assign/gradingtable.php b/mod/assign/gradingtable.php
index 6ffecb8..91a5e42 100644 (file)
--- a/mod/assign/gradingtable.php
+++ b/mod/assign/gradingtable.php
@@ -828,7 +828,8 @@ class assign_grading_table extends table_sql implements renderable {
                                             'mod_assign');
             $urlparams = array('id' => $this->assignment->get_course_module()->id,
                                'rownum'=>$this->rownum,
-                               'action'=>'grade');
+                               'action' => 'grade',
+                               'useridlistid' => $this->assignment->get_useridlist_key_id());
             $url = new moodle_url('/mod/assign/view.php', $urlparams);
             $link = $this->output->action_link($url, $icon);
             $grade .= $link . $separator;
@@ -993,7 +994,8 @@ class assign_grading_table extends table_sql implements renderable {
 
         $urlparams = array('id'=>$this->assignment->get_course_module()->id,
                            'rownum'=>$this->rownum,
-                           'action'=>'grade');
+                           'action' => 'grade',
+                           'useridlistid' => $this->assignment->get_useridlist_key_id());
         $url = new moodle_url('/mod/assign/view.php', $urlparams);
         $noimage = null;
 
@@ -1396,4 +1398,16 @@ class assign_grading_table extends table_sql implements renderable {
         }
         return '';
     }
+
+    /**
+     * Overides setup to ensure it will only run a single time.
+     */
+    public function setup() {
+        // Check if the setup function has been called before, we should not run it twice.
+        // If we do the sortorder of the table will be broken.
+        if (!empty($this->setup)) {
+            return;
+        }
+        parent::setup();
+    }
 }

diff --git a/mod/assign/lang/en/assign.php b/mod/assign/lang/en/assign.php
index fea73ea..8d5d47a 100644 (file)
--- a/mod/assign/lang/en/assign.php
+++ b/mod/assign/lang/en/assign.php
@@ -446,6 +446,7 @@ $string['updategrade'] = 'Update grade';
 $string['updatetable'] = 'Save and update table';
 $string['upgradenotimplemented'] = 'Upgrade not implemented in plugin ({$a->type} {$a->subtype})';
 $string['userextensiondate'] = 'Extension granted until: {$a}';
+$string['useridlistnotcached'] = 'Aborting save. Moodle could not determine which user to save the grade against.';
 $string['userswhoneedtosubmit'] = 'Users who need to submit: {$a}';
 $string['usergrade'] = 'User grade';
 $string['validmarkingworkflowstates'] = 'Valid marking workflow states';

diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php
index aad3f96..dd61e45 100644 (file)
--- a/mod/assign/locallib.php
+++ b/mod/assign/locallib.php
@@ -140,6 +140,9 @@ class assign {
     /** @var bool whether to exclude users with inactive enrolment */
     private $showonlyactiveenrol = null;
 
+    /** @var string A key used to identify cached userlists created by this object. */
+    private $useridlistid = null;
+
     /** @var array cached list of participants for this assignment. The cache key will be group, showactive and the context id */
     private $participants = array();
 
@@ -178,6 +181,9 @@ class assign {
 
         $this->submissionplugins = $this->load_plugins('assignsubmission');
         $this->feedbackplugins = $this->load_plugins('assignfeedback');
+
+        // Extra entropy is required for uniqid() to work on cygwin.
+        $this->useridlistid = clean_param(uniqid('', true), PARAM_ALPHANUM);
     }
 
     /**
@@ -470,18 +476,18 @@ class assign {
                     $action = 'redirect';
                     $nextpageparams['action'] = 'grade';
                     $nextpageparams['rownum'] = optional_param('rownum', 0, PARAM_INT) + 1;
-                    $nextpageparams['useridlistid'] = optional_param('useridlistid', time(), PARAM_INT);
+                    $nextpageparams['useridlistid'] = optional_param('useridlistid', $this->get_useridlist_key_id(), PARAM_ALPHANUM);
                 }
             } else if (optional_param('nosaveandprevious', null, PARAM_RAW)) {
                 $action = 'redirect';
                 $nextpageparams['action'] = 'grade';
                 $nextpageparams['rownum'] = optional_param('rownum', 0, PARAM_INT) - 1;
-                $nextpageparams['useridlistid'] = optional_param('useridlistid', time(), PARAM_INT);
+                $nextpageparams['useridlistid'] = optional_param('useridlistid', $this->get_useridlist_key_id(), PARAM_ALPHANUM);
             } else if (optional_param('nosaveandnext', null, PARAM_RAW)) {
                 $action = 'redirect';
                 $nextpageparams['action'] = 'grade';
                 $nextpageparams['rownum'] = optional_param('rownum', 0, PARAM_INT) + 1;
-                $nextpageparams['useridlistid'] = optional_param('useridlistid', time(), PARAM_INT);
+                $nextpageparams['useridlistid'] = optional_param('useridlistid', $this->get_useridlist_key_id(), PARAM_ALPHANUM);
             } else if (optional_param('savegrade', null, PARAM_RAW)) {
                 // Save changes button.
                 $action = 'grade';
@@ -514,7 +520,7 @@ class assign {
         }
 
         $returnparams = array('rownum'=>optional_param('rownum', 0, PARAM_INT),
-                              'useridlistid' => optional_param('useridlistid', time(), PARAM_INT));
+                              'useridlistid' => optional_param('useridlistid', $this->get_useridlist_key_id(), PARAM_ALPHANUM));
         $this->register_return_link($action, $returnparams);
 
         // Now show the right view page.
@@ -3006,16 +3012,16 @@ class assign {
 
         // If userid is passed - we are only grading a single student.
         $rownum = required_param('rownum', PARAM_INT);
-        $useridlistid = optional_param('useridlistid', time(), PARAM_INT);
+        $useridlistid = optional_param('useridlistid', $this->get_useridlist_key_id(), PARAM_ALPHANUM);
         $userid = optional_param('userid', 0, PARAM_INT);
         $attemptnumber = optional_param('attemptnumber', -1, PARAM_INT);
 
         $cache = cache::make_from_params(cache_store::MODE_SESSION, 'mod_assign', 'useridlist');
         if (!$userid) {
-            if (!$useridlist = $cache->get($this->get_course_module()->id . '_' . $useridlistid)) {
+            if (!$useridlist = $cache->get($this->get_useridlist_key($useridlistid))) {
                 $useridlist = $this->get_grading_userid_list();
             }
-            $cache->set($this->get_course_module()->id . '_' . $useridlistid, $useridlist);
+            $cache->set($this->get_useridlist_key($useridlistid), $useridlist);
         } else {
             $rownum = 0;
             $useridlist = array($userid);
@@ -3393,6 +3399,14 @@ class assign {
             $o .= $this->get_renderer()->render($gradingtable);
         }
 
+        if ($this->can_grade()) {
+            // We need to cache the order of uses in the table as the person may wish to grade them.
+            // This is done based on the row number of the user.
+            $cache = cache::make_from_params(cache_store::MODE_SESSION, 'mod_assign', 'useridlist');
+            $useridlist = $gradingtable->get_column_data('userid');
+            $cache->set($this->get_useridlist_key(), $useridlist);
+        }
+
         $currentgroup = groups_get_activity_group($this->get_course_module(), true);
         $users = array_keys($this->list_participants($currentgroup, true));
         if (count($users) != 0 && $this->can_grade()) {
@@ -6080,9 +6094,9 @@ class assign {
         $attemptnumber = $params['attemptnumber'];
         if (!$userid) {
             $cache = cache::make_from_params(cache_store::MODE_SESSION, 'mod_assign', 'useridlist');
-            if (!$useridlist = $cache->get($this->get_course_module()->id . '_' . $useridlistid)) {
+            if (!$useridlist = $cache->get($this->get_useridlist_key($useridlistid))) {
                 $useridlist = $this->get_grading_userid_list();
-                $cache->set($this->get_course_module()->id . '_' . $useridlistid, $useridlist);
+                $cache->set($this->get_useridlist_key($useridlistid), $useridlist);
             }
         } else {
             $useridlist = array($userid);
@@ -6239,7 +6253,7 @@ class assign {
         $mform->setType('rownum', PARAM_INT);
         $mform->setConstant('rownum', $rownum);
         $mform->addElement('hidden', 'useridlistid', $useridlistid);
-        $mform->setType('useridlistid', PARAM_INT);
+        $mform->setType('useridlistid', PARAM_ALPHANUM);
         $mform->addElement('hidden', 'attemptnumber', $attemptnumber);
         $mform->setType('attemptnumber', PARAM_INT);
         $mform->addElement('hidden', 'ajax', optional_param('ajax', 0, PARAM_INT));
@@ -6934,13 +6948,15 @@ class assign {
         $instance = $this->get_instance();
         $rownum = required_param('rownum', PARAM_INT);
         $attemptnumber = optional_param('attemptnumber', -1, PARAM_INT);
-        $useridlistid = optional_param('useridlistid', time(), PARAM_INT);
+        $useridlistid = optional_param('useridlistid', $this->get_useridlist_key_id(), PARAM_ALPHANUM);
         $userid = optional_param('userid', 0, PARAM_INT);
         $cache = cache::make_from_params(cache_store::MODE_SESSION, 'mod_assign', 'useridlist');
         if (!$userid) {
-            if (!$useridlist = $cache->get($this->get_course_module()->id . '_' . $useridlistid)) {
-                $useridlist = $this->get_grading_userid_list();
-                $cache->set($this->get_course_module()->id . '_' . $useridlistid, $useridlist);
+            if (!$useridlist = $cache->get($this->get_useridlist_key($useridlistid))) {
+                // If the userid list is not cached we must not save, as it is possible that the user in a
+                // given row position may not be the same now as when the grading page was generated.
+                $url = new moodle_url('/mod/assign/view.php', array('id' => $this->get_course_module()->id));
+                throw new moodle_exception('useridlistnotcached', 'mod_assign', $url);
             }
         } else {
             $useridlist = array($userid);
@@ -7428,6 +7444,28 @@ class assign {
             }
         }
     }
+
+    /**
+     * The id used to uniquily identify the cache for this instance of the assign object.
+     *
+     * @return string
+     */
+    public function get_useridlist_key_id() {
+        return $this->useridlistid;
+    }
+
+    /**
+     * Generates the key that should be used for an entry in the useridlist cache.
+     *
+     * @param string $id Generate a key for this instance (optional)
+     * @return string The key for the id, or new entry if no $id is passed.
+     */
+    public function get_useridlist_key($id = null) {
+        if ($id === null) {
+            $id = $this->get_useridlist_key_id();
+        }
+        return $this->get_course_module()->id . '_' . $id;
+    }
 }
 
 /**

diff --git a/mod/assign/tests/locallib_test.php b/mod/assign/tests/locallib_test.php
index af74eeb..30a55b1 100644 (file)
--- a/mod/assign/tests/locallib_test.php
+++ b/mod/assign/tests/locallib_test.php
@@ -2143,7 +2143,7 @@ Anchor link 2:<a title=\"bananas\" href=\"../logo-240x60.gif\">Link text</a>
         $pagination = array('userid'=>$this->students[0]->id,
                             'rownum'=>0,
                             'last'=>true,
-                            'useridlistid'=>time(),
+                            'useridlistid' => $assign->get_useridlist_key_id(),
                             'attemptnumber'=>0);
         $formparams = array($assign, $data, $pagination);
         $mform = new mod_assign_grade_form(null, $formparams);
@@ -2329,5 +2329,30 @@ 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 that the useridlist cache will retive the correct values
+     * when using assign::get_useridlist_key and assign::get_useridlist_key_id.
+     */
+    public function test_useridlist_cache() {
+        // Create an assignment object, we will use this to test the key generation functions.
+        $course = self::getDataGenerator()->create_course();
+        $assign = self::getDataGenerator()->create_module('assign', array('course' => $course->id));
+        list($courserecord, $cm) = get_course_and_cm_from_instance($assign->id, 'assign');
+        $context = context_module::instance($cm->id);
+        $assign = new assign($context, $cm, $courserecord);
+        // Create the cache.
+        $cache = cache::make_from_params(cache_store::MODE_SESSION, 'mod_assign', 'useridlist');
+        // Create an entry that we will insert into the cache.
+        $entry = array(0 => '5', 1 => '6325', 2 => '67783');
+        // Insert the value into the cache.
+        $cache->set($assign->get_useridlist_key(), $entry);
+        // Now test we can retrive the entry.
+        $this->assertEquals($entry, $cache->get($assign->get_useridlist_key()));
+        $useridlistid = clean_param($assign->get_useridlist_key_id(), PARAM_ALPHANUM);
+        $this->assertEquals($entry, $cache->get($assign->get_useridlist_key($useridlistid)));
+        // Check it will not retrive anything on an invalid key.
+        $this->assertFalse($cache->get($assign->get_useridlist_key('notvalid')));
+    }
 }
 

diff --git a/mod/assign/view.php b/mod/assign/view.php
index 0bcae0c..01d0edd 100644 (file)
--- a/mod/assign/view.php
+++ b/mod/assign/view.php
@@ -27,23 +27,22 @@ require_once($CFG->dirroot . '/mod/assign/locallib.php');
 
 $id = required_param('id', PARAM_INT);
 
-$urlparams = array('id' => $id,
-                  'action' => optional_param('action', '', PARAM_TEXT),
-                  'rownum' => optional_param('rownum', 0, PARAM_INT),
-                  'useridlistid' => optional_param('useridlistid', time(), PARAM_INT));
-
-$url = new moodle_url('/mod/assign/view.php', $urlparams);
-
 list ($course, $cm) = get_course_and_cm_from_cmid($id, 'assign');
 
 require_login($course, true, $cm);
-$PAGE->set_url($url);
 
 $context = context_module::instance($cm->id);
 
 require_capability('mod/assign:view', $context);
 
 $assign = new assign($context, $cm, $course);
+$urlparams = array('id' => $id,
+                  'action' => optional_param('action', '', PARAM_TEXT),
+                  'rownum' => optional_param('rownum', 0, PARAM_INT),
+                  'useridlistid' => optional_param('useridlistid', $assign->get_useridlist_key_id(), PARAM_ALPHANUM));
+
+$url = new moodle_url('/mod/assign/view.php', $urlparams);
+$PAGE->set_url($url);
 
 $completion=new completion_info($course);
 $completion->set_module_viewed($cm);