Moodle
  1. Moodle
  2. MDL-41092

Assignment cron mail function - submissions SQL incorrect

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5.3
    • Component/s: Assignment
    • Labels:

      Description

      When the cron function under assingments is run , the SQL used to get the submissions for each course is incorrect. The SQL starts by selecting the course column from the assign table which is a foreign key ( to course table). Hence , the get_records_sql() method outputs this error whenever it finds more than one course linked to an assignment ( which is a perfectly normal case ):

      Error

       Did you remember to make the first column something unique in your call to get_records? Duplicate value 'xxxxxx' found in column 'course'.* line 1014 of /lib/dml/mysqli_native_moodle_database.php: call to debugging()
      * line 1493 of /mod/assign/locallib.php: call to mysqli_native_moodle_database->get_records_sql()
      * line 859 of /mod/assign/lib.php: call to assign::cron()
      * line 267 of /lib/cronlib.php: call to assign_cron()
      * line 61 of /admin/cli/cron.php: call to cron_run()
      

      Code in Moodle 2.5 looks like this :

      /mod/assign/locallib.php

      	        $sql = 'SELECT a.course, a.name, a.blindmarking, a.revealidentities,
      	                       g.*, g.id as gradeid, g.timemodified as lastmodified
      	                 FROM {assign} a
      	                 JOIN {assign_grades} g ON g.assignment = a.id
      	                 LEFT JOIN {assign_user_flags} uf ON uf.assignment = a.id AND uf.userid = g.userid
      	                WHERE g.timemodified >= :yesterday AND
      	                      g.timemodified <= :today AND
      	                      uf.mailed = 0';
      

      In my understanding,SQL used in Moodle 2.2.6 took care of that by calling on the submissions table first like this:

      /mod/assignment/lib.php

      function assignment_get_unmailed_submissions($starttime, $endtime) {
      	    global $CFG, $DB;
      	
      	    return $DB->get_records_sql("SELECT s.*, a.course, a.name
      	                                   FROM {assignment_submissions} s,
      	                                        {assignment} a
      	                                  WHERE s.mailed = 0
      	                                        AND s.timemarked <= ?
      	                                        AND s.timemarked >= ?
      	                                        AND s.assignment = a.id", array($endtime, $starttime));
      }
      

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Thanks for reporting that and suggesting a solution.

            To trigger this, I assume you would need to have left-over unsent messages from multiple courses.

            This is not a security issue.

            Show
            Michael de Raadt added a comment - Thanks for reporting that and suggesting a solution. To trigger this, I assume you would need to have left-over unsent messages from multiple courses. This is not a security issue.
            Hide
            Hittesh added a comment -

            Agreed. Not a major issue if debugging is turned off for the instance. Not a security issue either.

            Show
            Hittesh added a comment - Agreed. Not a major issue if debugging is turned off for the instance. Not a security issue either.
            Hide
            Michael de Raadt added a comment -

            So as well as causing an error to be displayed during cron, this also means that assignment feedback emails are sent out with only one email coming from each course in each run of cron.

            The problem is in locallib.php as pointed out by Hitesh above.

            A solution is to add a unique first field as follows...

            locallib.php, line 1563

            $sql = 'SELECT g.id, a.course, a.name, a.blindmarking, a.revealidentities,
            

            Show
            Michael de Raadt added a comment - So as well as causing an error to be displayed during cron, this also means that assignment feedback emails are sent out with only one email coming from each course in each run of cron. The problem is in locallib.php as pointed out by Hitesh above. A solution is to add a unique first field as follows... locallib.php, line 1563 $sql = 'SELECT g.id, a.course, a.name, a.blindmarking, a.revealidentities,
            Hide
            Ankit Agarwal added a comment - - edited

            Thanks Michael,
            I have assigned you as the author of the commit( I made a slight change to keep alias as before).

            Pushing forward for review.

            Show
            Ankit Agarwal added a comment - - edited Thanks Michael, I have assigned you as the author of the commit( I made a slight change to keep alias as before). Pushing forward for review.
            Hide
            Mark Nelson added a comment -

            Hmm, I was wondering why this wasn't backported to 2.4 but the query was changed. In 2.4 they perform a join with assign_submission which provides a unique id for each case. Any ways, patch looks fine. Submitting to integration.

            Show
            Mark Nelson added a comment - Hmm, I was wondering why this wasn't backported to 2.4 but the query was changed. In 2.4 they perform a join with assign_submission which provides a unique id for each case. Any ways, patch looks fine. Submitting to integration.
            Hide
            Damyon Wiese added a comment -

            Thanks for the patch. I could not resist modifying the unit tests to cover this (was only a tiny change) so I have done this and amended the testing instructions.

            I'll pass this issue now because the unit tests prove it is passing.

            Integrated to 25 and master.

            Show
            Damyon Wiese added a comment - Thanks for the patch. I could not resist modifying the unit tests to cover this (was only a tiny change) so I have done this and amended the testing instructions. I'll pass this issue now because the unit tests prove it is passing. Integrated to 25 and master.
            Hide
            Damyon Wiese added a comment -

            Modified unit tests fail without the fix and pass with it.

            Passing.

            Show
            Damyon Wiese added a comment - Modified unit tests fail without the fix and pass with it. Passing.
            Hide
            Dan Poltawski added a comment -

            Congratulations - this issue has been included in Moodle and is now available on our git mirrors and shortly will become available on the download servers shortly.

            Show
            Dan Poltawski added a comment - Congratulations - this issue has been included in Moodle and is now available on our git mirrors and shortly will become available on the download servers shortly.

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: