MDL-46171 Assign: Always require a submission record if there is a grade

Also - update unit tests to match the new expectations.

diff --git a/mod/assign/backup/moodle2/restore_assign_stepslib.php b/mod/assign/backup/moodle2/restore_assign_stepslib.php
index c2d199c..db0f099 100644 (file)
--- 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 7befb9c..63b4dab 100755 (executable)
--- a/mod/assign/db/install.xml
+++ b/mod/assign/db/install.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8" ?>
-<XMLDB PATH="mod/assign/db" VERSION="20131220" COMMENT="XMLDB file for Moodle mod/assign"
+<XMLDB PATH="mod/assign/db" VERSION="20140724" COMMENT="XMLDB file for Moodle mod/assign"
     xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
     xsi:noNamespaceSchemaLocation="../../../lib/xmldb/xmldb.xsd"
 >
@@ -52,6 +52,7 @@
         <FIELD NAME="status" TYPE="char" LENGTH="10" NOTNULL="false" SEQUENCE="false" COMMENT="The status of this assignment submission. The current statuses are DRAFT and SUBMITTED."/>
         <FIELD NAME="groupid" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="The group id for team submissions"/>
         <FIELD NAME="attemptnumber" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="Used to track attempts for an assignment"/>
+        <FIELD NAME="latest" TYPE="int" LENGTH="2" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="Greatly simplifies queries wanting to know information about only the latest attempt."/>
       </FIELDS>
       <KEYS>
         <KEY NAME="primary" TYPE="primary" FIELDS="id" COMMENT="The unique id for this assignment submission."/>
@@ -61,6 +62,7 @@
         <INDEX NAME="userid" UNIQUE="false" FIELDS="userid"/>
         <INDEX NAME="attemptnumber" UNIQUE="false" FIELDS="attemptnumber"/>
         <INDEX NAME="uniqueattemptsubmission" UNIQUE="true" FIELDS="assignment, userid, groupid, attemptnumber"/>
+        <INDEX NAME="latestattempt" UNIQUE="false" FIELDS="assignment, userid, groupid, latest" COMMENT="Speed up queries for the latest attempt."/>
       </INDEXES>
     </TABLE>
     <TABLE NAME="assign_grades" COMMENT="Grading information about a single assignment submission.">

diff --git a/mod/assign/db/upgrade.php b/mod/assign/db/upgrade.php
index 78ee9a5..2a6bd17 100755 (executable)
--- 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 16caa5f..cb85521 100644 (file)
--- 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 efb948a..fe082dc 100644 (file)
--- 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 .= '<div class="details">';

diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php
index 0408758..98bb4c5 100644 (file)
--- 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 5f45d9e..5a6ce92 100644 (file)
--- 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 f451636..9b33ebe 100644 (file)
--- 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 f24a792..64b9a36 100644 (file)
--- 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 84a1c3e..66ab13d 100644 (file)
--- 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 3856c72..a0e65a8 100644 (file)
--- 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 d3989f1..2ec6bdb 100644 (file)
--- 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;