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:
    • Rank:
      52017

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

        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: