MDL-31414 mod_assign: Code cleanup for notification improvements

* No need to manually call update of providers that happens automatically.
* Added upgrade code for the new sendlatenotifications field.
* Added AMOS syntax for the two renamed strings
* Refactored assign::cron to use half as many DB queries
* Cleaned up unused vars and globals, coding style and phpdocs.
* Removed string notifications added previously but never used.

AMOS BEGIN
   MOV [emailgradermail,mod_assign],[gradersubmissionupdatedtext,mod_assign]
   MOV [emailgradermailhtml,mod_assign],[gradersubmissionupdatedhtml,mod_assign]
AMOS END

diff --git a/mod/assign/db/messages.php b/mod/assign/db/messages.php
index 2f4d6ba..77581d7 100644 (file)
--- a/mod/assign/db/messages.php
+++ b/mod/assign/db/messages.php
@@ -24,7 +24,7 @@
 
 $messageproviders = array (
 
-/// Ordinary assignment submissions
+    // Ordinary assignment submissions
     'assign_student_notification' => array (
         'capability' => 'mod/assign:submit'
     ),

diff --git a/mod/assign/db/upgrade.php b/mod/assign/db/upgrade.php
index 14dafc0..e918367 100644 (file)
--- a/mod/assign/db/upgrade.php
+++ b/mod/assign/db/upgrade.php
@@ -28,9 +28,25 @@
  * @return bool
  */
 function xmldb_assign_upgrade($oldversion) {
+    global $CFG, $DB;
+
+    $dbman = $DB->get_manager();
+
     if ($oldversion < 2012051700) {
-        message_update_providers('mod_assign');
+
+        // Define field sendlatenotifications to be added to assign
+        $table = new xmldb_table('assign');
+        $field = new xmldb_field('sendlatenotifications', XMLDB_TYPE_INTEGER, '2', null, XMLDB_NOTNULL, null, '0', 'sendnotifications');
+
+        // Conditionally launch add field sendlatenotifications
+        if (!$dbman->field_exists($table, $field)) {
+            $dbman->add_field($table, $field);
+        }
+
+        // Assign savepoint reached.
+        upgrade_mod_savepoint(true, 2012051700, 'assign');
     }
+
     return true;
 }
 

diff --git a/mod/assign/lang/en/assign.php b/mod/assign/lang/en/assign.php
index e083b94..0daa8ca 100644 (file)
--- a/mod/assign/lang/en/assign.php
+++ b/mod/assign/lang/en/assign.php
@@ -172,7 +172,6 @@ $string['notgradedyet'] = 'Not graded yet';
 $string['notsubmittedyet'] = 'Not submitted yet';
 $string['notifications'] = 'Notifications';
 $string['nousersselected'] = 'No users selected';
-$string['notification'] = 'Notification';
 $string['numberofdraftsubmissions'] = 'Drafts';
 $string['numberofparticipants'] = 'Participants';
 $string['numberofsubmittedassignments'] = 'Submitted';
@@ -262,4 +261,4 @@ $string['viewownsubmissionform'] = 'View own submit assignment page.';
 $string['viewownsubmissionstatus'] = 'View own submission status page.';
 $string['viewsubmissionforuser'] = 'View submission for user: {$a}';
 $string['viewsubmission'] = 'View submission';
-$string['viewsubmissiongradingtable'] = 'View submission grading table.';
+$string['viewsubmissiongradingtable'] = 'View submission grading table.';
\ No newline at end of file

diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php
index 6e4460f..abf9fec 100644 (file)
--- a/mod/assign/locallib.php
+++ b/mod/assign/locallib.php
@@ -1071,86 +1071,118 @@ class assign {
     }
 
     /**
-     *  Cron function to be run periodically according to the moodle cron
-     *  Finds all assignment notifications that have yet to be mailed out, and mails them
+     * Finds all assignment notifications that have yet to be mailed out, and mails them.
+     *
+     * Cron function to be run periodically according to the moodle cron
      *
      * @return bool
      */
     static function cron() {
-        global $CFG, $DB;
-
-        // used to cache DB lookups
-        $croncache = array();
+        global $DB;
 
         // only ever send a max of one days worth of updates
         $yesterday = time() - (24 * 3600);
         $timenow   = time();
 
-        // email students about new feedback
-        $submissions = $DB->get_records_sql("SELECT s.*, a.course, a.name, g.*, g.id as gradeid, g.timemodified as lastmodified
-                                   FROM {assign} a
-                                        JOIN {assign_grades} g ON g.assignment = a.id
-                                        LEFT JOIN {assign_submission} s ON s.assignment = a.id AND s.userid = g.userid
-                                  WHERE g.timemodified >= :yesterday
-                                        AND g.timemodified <= :today
-                                        AND g.mailed = 0", array('yesterday'=>$yesterday, 'today'=>$timenow));
+        // Collect all submissions from the past 24 hours that require mailing.
+        $sql = "SELECT s.*, a.course, a.name, g.*, g.id as gradeid, g.timemodified as lastmodified
+                 FROM {assign} a
+                 JOIN {assign_grades} g ON g.assignment = a.id
+            LEFT JOIN {assign_submission} s ON s.assignment = a.id AND s.userid = g.userid
+                WHERE g.timemodified >= :yesterday AND
+                      g.timemodified <= :today AND
+                      g.mailed = 0";
+        $params = array('yesterday' => $yesterday, 'today' => $timenow);
+        $submissions = $DB->get_records_sql($sql, $params);
 
         mtrace('Processing ' . count($submissions) . ' assignment submissions ...');
 
+        // Preload courses we are going to need those.
+        $courseids = array();
+        foreach ($submissions as $submission) {
+            $courseids[] = $submission->course;
+        }
+        // Filter out duplicates
+        $courseids = array_unique($courseids);
+        $ctxselect = context_helper::get_preload_record_columns_sql('ctx');
+        list($courseidsql, $params) = $DB->get_in_or_equal($courseids, SQL_PARAMS_NAMED);
+        $sql = "SELECT c.*, {$ctxselect}
+                  FROM {course} c
+             LEFT JOIN {context} ctx ON ctx.instanceid = c.id AND ctx.contextlevel = :contextlevel
+                 WHERE c.id {$courseidsql}";
+        $params['contextlevel'] = CONTEXT_COURSE;
+        $courses = $DB->get_records_sql($sql, $params);
+        // Clean up... this could go on for a while.
+        unset($courseids);
+        unset($ctxselect);
+        unset($courseidsql);
+        unset($params);
+
+        // Simple array we'll use for caching modules.
+        $modcache = array();
+
+        // Email students about new feedback
         foreach ($submissions as $submission) {
 
             mtrace("Processing assignment submission $submission->id ...");
 
             // do not cache user lookups - could be too many
-            if (! $user = $DB->get_record("user", array("id"=>$submission->userid))) {
+            if (!$user = $DB->get_record("user", array("id"=>$submission->userid))) {
                 mtrace("Could not find user $submission->userid");
                 continue;
             }
 
             // use a cache to prevent the same DB queries happening over and over
-            if (! $course = array_search('course_' . $submission->course, $croncache)) {
-                if (! $course = $DB->get_record("course", array("id"=>$submission->course))) {
-                    mtrace("Could not find course $submission->course");
-                    continue;
-                }
-                $croncache['course_' . $submission->course] = $course;
+            if (!array_key_exists($submission->course, $courses)) {
+                mtrace("Could not find course $submission->course");
+                continue;
+            }
+            $course = $courses[$submission->course];
+            if (isset($course->ctxid)) {
+                // Context has not yet been preloaded. Do so now.
+                context_helper::preload_from_record($course);
             }
 
-            /// Override the language and timezone of the "current" user, so that
-            /// mail is customised for the receiver.
+            // Override the language and timezone of the "current" user, so that
+            // mail is customised for the receiver.
             cron_setup_user($user, $course);
 
             // context lookups are already cached
-            $coursecontext = context_course::instance($submission->course);
-            $courseshortname = format_string($course->shortname, true, array('context' => $coursecontext));
+            $coursecontext = context_course::instance($course->id);
             if (!is_enrolled($coursecontext, $user->id)) {
+                $courseshortname = format_string($course->shortname, true, array('context' => $coursecontext));
                 mtrace(fullname($user)." not an active participant in " . $courseshortname);
                 continue;
             }
 
-            if (! $grader = $DB->get_record("user", array("id"=>$submission->grader))) {
+            if (!$grader = $DB->get_record("user", array("id"=>$submission->grader))) {
                 mtrace("Could not find grader $submission->grader");
                 continue;
             }
 
-
-            if (! $mod = array_search('mod_' . $submission->assignment, $croncache)) {
+            if (!array_key_exists($submission->assignment, $modcache)) {
                 if (! $mod = get_coursemodule_from_instance("assign", $submission->assignment, $course->id)) {
                     mtrace("Could not find course module for assignment id $submission->assignment");
                     continue;
                 }
-                $croncache['mod_' . $submission->assignment] = $mod;
+                $modcache[$submission->assignment] = $mod;
+            } else {
+                $mod = $modcache[$submission->assignment];
             }
-
             // context lookups are already cached
             $contextmodule = context_module::instance($mod->id);
 
-            if (! $mod->visible) {    /// Hold mail notification for hidden assignments until later
+            if (!$mod->visible) {
+                // Hold mail notification for hidden assignments until later
                 continue;
             }
 
             // need to send this to the student
-            self::send_assignment_notification($grader, $user, 'feedbackavailable', 'assign_student_notification', $submission->lastmodified, $mod, $contextmodule, $course, get_string('modulename', 'assign'), $submission->name);
+            $messagetype = 'feedbackavailable';
+            $eventtype = 'assign_student_notification';
+            $updatetime = $submission->lastmodified;
+            $modulename = get_string('modulename', 'assign');
+            self::send_assignment_notification($grader, $user, $messagetype, $eventtype, $updatetime, $mod, $contextmodule, $course, $modulename, $submission->name);
 
             $grade = new stdClass();
             $grade->id = $submission->gradeid;
@@ -1162,8 +1194,10 @@ class assign {
         mtrace('Done processing ' . count($submissions) . ' assignment submissions');
 
         cron_setup_user();
-        // free up memory
-        unset($croncache);
+
+        // Free up memory just to be sure
+        unset($courses);
+        unset($modcache);
 
         return true;
     }
@@ -2294,7 +2328,7 @@ class assign {
     }
 
     /**
-     * Format a notification based on it's type
+     * Format a notification for plain text
      *
      * @param string $messagetype
      * @param stdClass $info
@@ -2314,10 +2348,16 @@ class assign {
     }
 
     /**
-     * Format a notification based on it's type
+     * Format a notification for HTML
      *
      * @param string $messagetype
      * @param stdClass $info
+     * @param stdClass $course
+     * @param stdClass $context
+     * @param string $modulename
+     * @param stdClass $coursemodule
+     * @param string $assignmentname
+     * @param stdClass $info
      */
     private static function format_notification_message_html($messagetype, $info, $course, $context, $modulename, $coursemodule, $assignmentname) {
         global $CFG;
@@ -2334,8 +2374,6 @@ class assign {
     /**
      * email someone about something (static so it can be called from cron)
      *
-     * @global stdClass $CFG
-     * @global moodle_database $DB
      * @param stdClass $userfrom
      * @param stdClass $userto
      * @param string $messagetype
@@ -2345,16 +2383,13 @@ class assign {
      * @param stdClass $context
      * @param stdClass $course
      * @param string $modulename
-     * @param string $moduleplural
      * @param string $assignmentname
      * @return void
      */
     public static function send_assignment_notification($userfrom, $userto, $messagetype, $eventtype,
                                                         $updatetime, $coursemodule, $context, $course,
                                                         $modulename, $assignmentname) {
-        global $CFG, $DB;
-
-        $strnotification  = get_string('notification', 'assign');
+        global $CFG;
 
         $info = new stdClass();
         $info->username = fullname($userfrom, true);
@@ -2383,15 +2418,16 @@ class assign {
         $eventdata->contexturlname  = $info->assignment;
 
         message_send($eventdata);
-
     }
 
     /**
      * email someone about something
      *
-     * @global stdClass $CFG
-     * @global moodle_database $DB
-     * @param stdClass $submission
+     * @param stdClass $userfrom
+     * @param stdClass $userto
+     * @param string $messagetype
+     * @param string $eventtype
+     * @param int $updatetime
      * @return void
      */
     public function send_notification($userfrom, $userto, $messagetype, $eventtype, $updatetime) {
@@ -2399,9 +2435,8 @@ class assign {
     }
 
     /**
-     * email student upon successful submission
+     * Email student upon successful submission
      *
-     * @global stdClass $CFG
      * @global moodle_database $DB
      * @param stdClass $submission
      * @return void
@@ -2410,19 +2445,17 @@ class assign {
         global $DB;
 
         $adminconfig = $this->get_admin_config();
-        if (!$adminconfig->submissionreceipts) {          // No need to do anything
+        if (!$adminconfig->submissionreceipts) {
+            // No need to do anything
             return;
         }
-
         $user = $DB->get_record('user', array('id'=>$submission->userid), '*', MUST_EXIST);
-
         $this->send_notification($user, $user, 'submissionreceipt', 'assign_student_notification', $submission->timemodified);
     }
 
     /**
-     * email graders upon student submissions
+     * Email graders upon student submissions
      *
-     * @global stdClass $CFG
      * @global moodle_database $DB
      * @param stdClass $submission
      * @return void
@@ -2437,13 +2470,7 @@ class assign {
         }
 
         $user = $DB->get_record('user', array('id'=>$submission->userid), '*', MUST_EXIST);
-
         if ($teachers = $this->get_graders($user->id)) {
-
-            $strassignments = $this->get_module_name_plural();
-            $strassignment  = $this->get_module_name();
-            $strsubmitted  = get_string('submitted', 'assign');
-
             foreach ($teachers as $teacher) {
                 $this->send_notification($user, $teacher, 'gradersubmissionupdated', 'assign_grader_notification', $submission->timemodified);
             }

diff --git a/mod/assign/settings.php b/mod/assign/settings.php
index ae3ffaa..7860b64 100644 (file)
--- a/mod/assign/settings.php
+++ b/mod/assign/settings.php
@@ -54,5 +54,4 @@ if ($ADMIN->fulltree) {
                    new lang_string('configshowrecentsubmissions', 'assign'), 0));
     $settings->add(new admin_setting_configcheckbox('assign/submissionreceipts',
                    get_string('sendsubmissionreceipts', 'mod_assign'), get_string('sendsubmissionreceipts_help', 'mod_assign'), 1));
-
-}
+}
\ No newline at end of file

diff --git a/mod/assign/version.php b/mod/assign/version.php
index 962f4d7..b2b2bfa 100644 (file)
--- a/mod/assign/version.php
+++ b/mod/assign/version.php
@@ -24,9 +24,9 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$module->component = 'mod_assign';
-$module->version  = 2012051700;
-$module->requires = 2012050300;  // Requires this Moodle version
+$module->component = 'mod_assign'; // Full name of the plugin (used for diagnostics)
+$module->version  = 2012051700;    // The current module version (Date: YYYYMMDDXX)
+$module->requires = 2012050300;    // Requires this Moodle version
 $module->cron     = 60;