mirror_php/moodle
1
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-04-20 07:56:06 +02:00
Code Issues Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Neill Magill 2015-11-11 13:06:31 +00:00
parent 2b11b94c0c
commit 3fca693751
6 changed files with 103 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
lib/tablelib.php
View File

@ -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();

18
mod/assign/gradingtable.php
View File

@ -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();
}
}

1
mod/assign/lang/en/assign.php
View File

@ -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';

66
mod/assign/locallib.php
View File

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

27
mod/assign/tests/locallib_test.php
View File

@ -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')));
}
}

15
mod/assign/view.php
View File

@ -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);