Skip to content

Commit

Permalink
MDL-44330 Assign: Assignment grading may not display expected student
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@NeillM
NeillM committed Nov 24, 2015
1 parent 2825fc7 commit 07ae362
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 25 deletions.
1 change: 1 addition & 0 deletions lib/tablelib.php
View file
Expand Up @@ -61,6 +61,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();
Expand Down
18 changes: 16 additions & 2 deletions mod/assign/gradingtable.php
View file
Expand Up @@ -821,7 +821,8 @@ public function col_grade(stdClass $row) {
'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;
Expand Down Expand Up @@ -986,7 +987,8 @@ public function col_userid(stdClass $row) {

$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;

Expand Down Expand Up @@ -1389,4 +1391,16 @@ protected function show_hide_link($column, $index) {
}
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();
}
}
1 change: 1 addition & 0 deletions mod/assign/lang/en/assign.php
View file
Expand Up @@ -443,6 +443,7 @@
$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';
Expand Down
66 changes: 52 additions & 14 deletions mod/assign/locallib.php
View file
Expand Up @@ -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();

Expand Down Expand Up @@ -175,6 +178,9 @@ public function __construct($coursemodulecontext, $coursemodule, $course) {

$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);
}

/**
Expand Down Expand Up @@ -467,18 +473,18 @@ public function view($action='') {
$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';
Expand Down Expand Up @@ -511,7 +517,7 @@ public function view($action='') {
}

$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.
Expand Down Expand Up @@ -2985,16 +2991,16 @@ protected function view_single_grade_page($mform) {

// 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);
Expand Down Expand Up @@ -3370,6 +3376,14 @@ protected function view_grading_table() {
$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()) {
Expand Down Expand Up @@ -6055,9 +6069,9 @@ public function add_grade_form_elements(MoodleQuickForm $mform, stdClass $data,
$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);
Expand Down Expand Up @@ -6214,7 +6228,7 @@ public function add_grade_form_elements(MoodleQuickForm $mform, stdClass $data,
$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));
Expand Down Expand Up @@ -6909,13 +6923,15 @@ protected function process_save_grade(&$mform) {
$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);
Expand Down Expand Up @@ -7403,6 +7419,28 @@ public function get_grading_status($userid) {
}
}
}

/**
* 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;
}
}

/**
Expand Down
27 changes: 26 additions & 1 deletion mod/assign/tests/locallib_test.php
View file
Expand Up @@ -2143,7 +2143,7 @@ public function test_feedback_comment_commentinline() {
$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);
Expand Down Expand Up @@ -2329,5 +2329,30 @@ public function test_get_shared_group_members() {
$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')));
}
}

15 changes: 7 additions & 8 deletions mod/assign/view.php
View file
Expand Up @@ -27,23 +27,22 @@

$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);
Expand Down

0 comments on commit 07ae362

Please sign in to comment.