From 9e3eee671f75e3c582cd01863bea963e780ecf9a Mon Sep 17 00:00:00 2001 From: Damyon Wiese Date: Fri, 25 Jul 2014 14:25:37 +0800 Subject: [PATCH] MDL-46171 Assign: Always require a submission record if there is a grade Also - update unit tests to match the new expectations. --- .../moodle2/restore_assign_stepslib.php | 28 ++++++++++++++++++- mod/assign/db/install.xml | 4 ++- mod/assign/db/upgrade.php | 27 ++++++++++++++++-- mod/assign/externallib.php | 12 ++++---- mod/assign/lib.php | 13 ++++----- mod/assign/locallib.php | 19 ++++++------- mod/assign/renderer.php | 6 ++-- mod/assign/tests/externallib_test.php | 25 +++++++++++++++++ mod/assign/tests/lib_test.php | 4 ++- mod/assign/tests/locallib_test.php | 16 +++++++++-- mod/assign/upgrade.txt | 8 ++++++ mod/assign/version.php | 2 +- 12 files changed, 129 insertions(+), 35 deletions(-) diff --git a/mod/assign/backup/moodle2/restore_assign_stepslib.php b/mod/assign/backup/moodle2/restore_assign_stepslib.php index c2d199c1d63..db0f0990a2c 100644 --- a/mod/assign/backup/moodle2/restore_assign_stepslib.php +++ b/mod/assign/backup/moodle2/restore_assign_stepslib.php @@ -237,7 +237,10 @@ class restore_assign_activity_structure_step extends restore_activity_structure_ * @return void */ protected function set_latest_submission_field() { - global $DB; + global $DB, $CFG; + + // Required for constants. + require_once($CFG->dirroot . '/mod/assign/locallib.php'); $assignmentid = $this->get_new_parentid('assign'); // This code could be rewritten as a monster SQL - but the point of adding this "latest" field @@ -277,6 +280,29 @@ class restore_assign_activity_structure_step extends restore_activity_structure_ $DB->update_record('assign_submission', $submission); } } + + // Now check for records with a grade, but no submission record. + $records = $DB->get_records_sql('SELECT g.id, g.userid + FROM {assign_grades} g + LEFT JOIN {assign_submission} s + ON s.assignment = g.assignment + AND s.userid = g.userid + WHERE s.id IS NULL AND g.assignment = ?', array($assignmentid)); + + $submissions = array(); + foreach ($records as $record) { + $submission = new stdClass(); + $submission->assignment = $assignmentid; + $submission->userid = $record->userid; + $submission->status = ASSIGN_SUBMISSION_STATUS_NEW; + $submission->groupid = 0; + $submission->latest = 1; + $submission->timecreated = time(); + $submission->timemodified = time(); + array_push($submissions, $submission); + } + + $DB->insert_records('assign_submission', $submissions); } /** diff --git a/mod/assign/db/install.xml b/mod/assign/db/install.xml index 7befb9c1cd2..63b4dab9a7c 100755 --- a/mod/assign/db/install.xml +++ b/mod/assign/db/install.xml @@ -1,5 +1,5 @@ - @@ -52,6 +52,7 @@ + @@ -61,6 +62,7 @@ + diff --git a/mod/assign/db/upgrade.php b/mod/assign/db/upgrade.php index 78ee9a5a127..2a6bd17264e 100755 --- a/mod/assign/db/upgrade.php +++ b/mod/assign/db/upgrade.php @@ -523,14 +523,14 @@ function xmldb_assign_upgrade($oldversion) { // Assign savepoint reached. upgrade_mod_savepoint(true, 2014072401, 'assign'); } - if ($oldversion < 2014072403) { + if ($oldversion < 2014072405) { // Prevent running this multiple times. $countsql = 'SELECT COUNT(id) FROM {assign_submission} WHERE latest = ?;'; $count = $DB->count_records_sql($countsql, array(1)); - if ($count == 0) { + if ($count != 342234) { // Mark the latest attempt for every submission in mod_assign. $maxattemptsql = 'SELECT assignment, userid, groupid, max(attemptnumber) AS maxattempt @@ -547,10 +547,31 @@ function xmldb_assign_upgrade($oldversion) { $select = 'id IN(' . $maxattemptidssql . ')'; $DB->set_field_select('assign_submission', 'latest', 1, $select); + // Look for grade records with no submission record. + $records = $DB->get_records_sql('SELECT g.id, g.assignment, g.userid + FROM {assign_grades} g + LEFT JOIN {assign_submission} s + ON s.assignment = g.assignment + AND s.userid = g.userid + WHERE s.id IS NULL'); + $submissions = array(); + foreach ($records as $record) { + $submission = new stdClass(); + $submission->assignment = $record->assignment; + $submission->userid = $record->userid; + $submission->status = 'new'; + $submission->groupid = 0; + $submission->latest = 1; + $submission->timecreated = time(); + $submission->timemodified = time(); + array_push($submissions, $submission); + } + + $DB->insert_records('assign_submission', $submissions); } // Assign savepoint reached. - upgrade_mod_savepoint(true, 2014072403, 'assign'); + upgrade_mod_savepoint(true, 2014072405, 'assign'); } return true; diff --git a/mod/assign/externallib.php b/mod/assign/externallib.php index 16caa5f4d75..cb85521015a 100644 --- a/mod/assign/externallib.php +++ b/mod/assign/externallib.php @@ -138,13 +138,13 @@ class mod_assign_external extends external_api { ag.grader, ag.grade, ag.attemptnumber - FROM {assign_grades} ag, {assign_submission} as - WHERE as.assignment $inorequalsql - AND as.userid = ag.userid - AND as.latest = 1 - AND as.attemptnumber = ag.attemptnumber + FROM {assign_grades} ag, {assign_submission} s + WHERE s.assignment $inorequalsql + AND s.userid = ag.userid + AND s.latest = 1 + AND s.attemptnumber = ag.attemptnumber AND ag.timemodified >= :since - AND ag.assignment = as.assignment + AND ag.assignment = s.assignment ORDER BY ag.assignment, ag.id"; $placeholders['since'] = $params['since']; diff --git a/mod/assign/lib.php b/mod/assign/lib.php index efb948a7168..fe082dc360d 100644 --- a/mod/assign/lib.php +++ b/mod/assign/lib.php @@ -390,7 +390,7 @@ function assign_print_overview($courses, &$htmlarray) { if (!isset($unmarkedsubmissions)) { // Build up and array of unmarked submissions indexed by assignment id/ userid // for use where the user has grading rights on assignment. - $dbparams = array_merge($assignmentidparams, array(ASSIGN_SUBMISSION_STATUS_SUBMITTED), $assignmentidparams); + $dbparams = array_merge(array(ASSIGN_SUBMISSION_STATUS_SUBMITTED), $assignmentidparams); $rs = $DB->get_recordset_sql('SELECT s.assignment as assignment, s.userid as userid, @@ -440,7 +440,7 @@ function assign_print_overview($courses, &$htmlarray) { if (!isset($mysubmissions)) { // Get all user submissions, indexed by assignment id. - $dbparams = array_merge(array($USER->id, $USER->id), $assignmentidparams); + $dbparams = array_merge(array($USER->id), $assignmentidparams, array($USER->id)); $mysubmissions = $DB->get_records_sql('SELECT a.id AS assignment, a.nosubmissions AS nosubmissions, @@ -448,16 +448,15 @@ function assign_print_overview($courses, &$htmlarray) { g.grader AS grader, g.grade AS grade, s.status AS status - FROM {assign} a + FROM {assign} a, {assign_submission} s LEFT JOIN {assign_grades} g ON - g.assignment = a.id AND + g.assignment = s.assignment AND g.userid = ? AND g.attemptnumber = s.attemptnumber - LEFT JOIN {assign_submission} s ON + WHERE a.id ' . $sqlassignmentids . ' AND s.latest = 1 AND s.assignment = a.id AND - s.userid = ? - WHERE a.id ' . $sqlassignmentids, $dbparams); + s.userid = ?', $dbparams); } $str .= '
'; diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index 0408758655a..98bb4c5310f 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -27,6 +27,7 @@ defined('MOODLE_INTERNAL') || die(); // Assignment submission statuses. +define('ASSIGN_SUBMISSION_STATUS_NEW', 'new'); define('ASSIGN_SUBMISSION_STATUS_REOPENED', 'reopened'); define('ASSIGN_SUBMISSION_STATUS_DRAFT', 'draft'); define('ASSIGN_SUBMISSION_STATUS_SUBMITTED', 'submitted'); @@ -1562,10 +1563,6 @@ class assign { s.status = :submissionstatus'; $params['groupuserid'] = 0; } else { - $maxattemptsql = 'SELECT mxs.userid, MAX(mxs.attemptnumber) AS maxattempt - FROM {assign_submission} mxs - WHERE mxs.assignment = :assignid2 GROUP BY mxs.userid'; - $sql = 'SELECT COUNT(s.userid) FROM {assign_submission} s JOIN(' . $esql . ') e ON e.id = s.userid @@ -2024,6 +2021,7 @@ class assign { * @param int $groupid The id of the group for this user - may be 0 in which * case it is determined from the userid. * @param bool $create If set to true a new submission object will be created in the database + * with the status set to "new". * @param int $attemptnumber - -1 means the latest attempt * @return stdClass The submission */ @@ -2085,7 +2083,7 @@ class assign { // This is the case when we need to set latest to 0 for all the other attempts. $DB->set_field('assign_submission', 'latest', 0, $params); } - $submission->status = ASSIGN_SUBMISSION_STATUS_DRAFT; + $submission->status = ASSIGN_SUBMISSION_STATUS_NEW; $sid = $DB->insert_record('assign_submission', $submission); return $DB->get_record('assign_submission', array('id' => $sid)); } @@ -2654,7 +2652,7 @@ class assign { * * @param int $userid The id of the user whose submission we want or 0 in which case USER->id is used * @param bool $create optional - defaults to false. If set to true a new submission object - * will be created in the database. + * will be created in the database with the status set to "new". * @param int $attemptnumber - -1 means the latest attempt * @return stdClass The submission */ @@ -2686,7 +2684,7 @@ class assign { $submission->userid = $userid; $submission->timecreated = time(); $submission->timemodified = $submission->timecreated; - $submission->status = ASSIGN_SUBMISSION_STATUS_DRAFT; + $submission->status = ASSIGN_SUBMISSION_STATUS_NEW; if ($attemptnumber >= 0) { $submission->attemptnumber = $attemptnumber; } else { @@ -2701,10 +2699,11 @@ class assign { } else { // We need to work this out. $result = $DB->get_records('assign_submission', $params, 'attemptnumber DESC', 'attemptnumber', 0, 1); + $latestsubmission = null; if ($result) { $latestsubmission = reset($result); } - if (!$latestsubmission || ($attemptnumber == $latestsubmission->attemptnumber)) { + if (empty($latestsubmission) || ($attemptnumber > $latestsubmission->attemptnumber)) { $submission->latest = 1; } } @@ -2794,9 +2793,9 @@ class assign { if ($attemptnumber < 0) { // Make sure this grade matches the latest submission attempt. if ($this->get_instance()->teamsubmission) { - $submission = $this->get_group_submission($userid, 0, false); + $submission = $this->get_group_submission($userid, 0, true); } else { - $submission = $this->get_user_submission($userid, false); + $submission = $this->get_user_submission($userid, true); } if ($submission) { $attemptnumber = $submission->attemptnumber; diff --git a/mod/assign/renderer.php b/mod/assign/renderer.php index 5f45d9efd39..5a6ce924a06 100644 --- a/mod/assign/renderer.php +++ b/mod/assign/renderer.php @@ -483,7 +483,7 @@ class mod_assign_renderer extends plugin_renderer_base { $row = new html_table_row(); $cell1 = new html_table_cell(get_string('submissionstatus', 'assign')); if (!$status->teamsubmissionenabled) { - if ($status->submission) { + if ($status->submission && $status->submission->status != ASSIGN_SUBMISSION_STATUS_NEW) { $statusstr = get_string('submissionstatus_' . $status->submission->status, 'assign'); $cell2 = new html_table_cell($statusstr); $cell2->attributes = array('class'=>'submissionstatus' . $status->submission->status); @@ -499,7 +499,7 @@ class mod_assign_renderer extends plugin_renderer_base { } else { $row = new html_table_row(); $cell1 = new html_table_cell(get_string('submissionstatus', 'assign')); - if ($status->teamsubmission) { + if ($status->teamsubmission && $status->teamsubmission->status != ASSIGN_SUBMISSION_STATUS_NEW) { $teamstatus = $status->teamsubmission->status; $submissionsummary = get_string('submissionstatus_' . $teamstatus, 'assign'); $groupid = 0; @@ -690,7 +690,7 @@ class mod_assign_renderer extends plugin_renderer_base { // Links. if ($status->view == assign_submission_status::STUDENT_VIEW) { if ($status->canedit) { - if (!$submission) { + if (!$submission || $submission->status == ASSIGN_SUBMISSION_STATUS_NEW) { $o .= $this->output->box_start('generalbox submissionaction'); $urlparams = array('id' => $status->coursemoduleid, 'action' => 'editsubmission'); $o .= $this->output->single_button(new moodle_url('/mod/assign/view.php', $urlparams), diff --git a/mod/assign/tests/externallib_test.php b/mod/assign/tests/externallib_test.php index f4516361ee7..9b33ebe0fe1 100644 --- a/mod/assign/tests/externallib_test.php +++ b/mod/assign/tests/externallib_test.php @@ -77,6 +77,18 @@ class mod_assign_external_testcase extends externallib_advanced_testcase { // Create a student and give them 2 grades (for 2 attempts). $student = self::getDataGenerator()->create_user(); + + $submission = new stdClass(); + $submission->assignment = $assign->id; + $submission->userid = $student->id; + $submission->status = ASSIGN_SUBMISSION_STATUS_NEW; + $submission->latest = 0; + $submission->attemptnumber = 0; + $submission->groupid = 0; + $submission->timecreated = time(); + $submission->timemodified = time(); + $DB->insert_record('assign_submission', $submission); + $grade = new stdClass(); $grade->assignment = $assign->id; $grade->userid = $student->id; @@ -87,6 +99,17 @@ class mod_assign_external_testcase extends externallib_advanced_testcase { $grade->attemptnumber = 0; $DB->insert_record('assign_grades', $grade); + $submission = new stdClass(); + $submission->assignment = $assign->id; + $submission->userid = $student->id; + $submission->status = ASSIGN_SUBMISSION_STATUS_NEW; + $submission->latest = 1; + $submission->attemptnumber = 1; + $submission->groupid = 0; + $submission->timecreated = time(); + $submission->timemodified = time(); + $DB->insert_record('assign_submission', $submission); + $grade = new stdClass(); $grade->assignment = $assign->id; $grade->userid = $student->id; @@ -962,6 +985,8 @@ class mod_assign_external_testcase extends externallib_advanced_testcase { // No warnings. $this->assertEquals(0, count($result)); + $records = $DB->get_records('assign_submission'); + $records = $DB->get_records('assign_grades'); $result = mod_assign_external::get_grades(array($instance->id)); $this->assertEquals($result['assignments'][0]['grades'][0]['grade'], '50.0'); diff --git a/mod/assign/tests/lib_test.php b/mod/assign/tests/lib_test.php index f24a792218f..64b9a36c5c3 100644 --- a/mod/assign/tests/lib_test.php +++ b/mod/assign/tests/lib_test.php @@ -169,13 +169,15 @@ class mod_assign_lib_testcase extends mod_assign_base_testcase { } public function test_assign_user_complete() { - global $PAGE; + global $PAGE, $DB; $this->setUser($this->editingteachers[0]); $assign = $this->create_instance(array('submissiondrafts' => 1)); $PAGE->set_url(new moodle_url('/mod/assign/view.php', array('id' => $assign->get_course_module()->id))); $submission = $assign->get_user_submission($this->students[0]->id, true); + $submission->status = ASSIGN_SUBMISSION_STATUS_DRAFT; + $DB->update_record('assign_submission', $submission); $this->expectOutputRegex('/Draft/'); assign_user_complete($this->course, $this->students[0], $assign->get_course_module(), $assign->get_instance()); diff --git a/mod/assign/tests/locallib_test.php b/mod/assign/tests/locallib_test.php index 84a1c3e8dca..66ab13dd0f3 100644 --- a/mod/assign/tests/locallib_test.php +++ b/mod/assign/tests/locallib_test.php @@ -661,6 +661,8 @@ class mod_assign_locallib_testcase extends mod_assign_base_testcase { // Simulate a submission. $this->setUser($this->extrastudents[0]); $submission = $assign->get_user_submission($this->extrastudents[0]->id, true); + $submission->status = ASSIGN_SUBMISSION_STATUS_DRAFT; + $assign->testable_update_submission($submission, $this->extrastudents[0]->id, true, false); // Leave this one as DRAFT. $data = new stdClass(); $data->onlinetext_editor = array('itemid'=>file_get_unused_draft_itemid(), @@ -1373,12 +1375,22 @@ class mod_assign_locallib_testcase extends mod_assign_base_testcase { $formdata->instance = $formdata->id; $assign->update_instance($formdata); - // Check we can repopen still. + // Mark the submission again. + $data = new stdClass(); + $data->grade = '60.0'; + $assign->testable_apply_grade_to_user($data, $this->students[0]->id, 1); + + // Check the grade exists. + $grades = $assign->get_user_grades_for_gradebook($this->students[0]->id); + $this->assertEquals(60, (int)$grades[$this->students[0]->id]->rawgrade); + + // Check we can reopen still. $result = $assign->testable_process_add_attempt($this->students[0]->id); $this->assertEquals(true, $result); + // Should no longer have a grade because there is no grade for the latest attempt. $grades = $assign->get_user_grades_for_gradebook($this->students[0]->id); - $this->assertEquals(50, (int)$grades[$this->students[0]->id]->rawgrade); + $this->assertEmpty($grades); } diff --git a/mod/assign/upgrade.txt b/mod/assign/upgrade.txt index 3856c723ed2..a0e65a851a5 100644 --- a/mod/assign/upgrade.txt +++ b/mod/assign/upgrade.txt @@ -1,4 +1,12 @@ This files describes API changes in the assign code. +=== 2.8 === +* Some DB changes were made to simplify the SQL required to query the latest attempt. + - The assign_submission table now has a column "latest" which is set to 1 for the latest submission attempt. + - There will always be a submission row if there is a grade (so the latest grade can be found by joining with the submission) + - There is a new submission status "new" for a submission that has never been attempted by a student (but the record exists purely + to mark the latest attempt number as 0). The function get_user_submission will create a record with the status set to "new" + by default (the previous default was "draft"). + === 2.7 === * Added setting sendstudentnotifications to assign DB table with admin defaults. This sets the default value for the diff --git a/mod/assign/version.php b/mod/assign/version.php index d3989f103d4..2ec6bdb882a 100644 --- a/mod/assign/version.php +++ b/mod/assign/version.php @@ -25,7 +25,7 @@ defined('MOODLE_INTERNAL') || die(); $plugin->component = 'mod_assign'; // Full name of the plugin (used for diagnostics). -$plugin->version = 2014072403; // The current module version (Date: YYYYMMDDXX). +$plugin->version = 2014072405; // The current module version (Date: YYYYMMDDXX). $plugin->requires = 2014050800; // Requires this Moodle version. $plugin->cron = 60;