MDL-32932 mod_assign: Coding style clean up and security improvement
authorSam Hemelryk <sam@moodle.com>
Tue, 22 May 2012 01:42:39 +0000 (13:42 +1200)
committerSam Hemelryk <sam@moodle.com>
Tue, 22 May 2012 02:21:47 +0000 (14:21 +1200)
mod/assign/gradingtable.php
mod/assign/lang/en/assign.php
mod/assign/locallib.php
mod/assign/module.js
mod/assign/renderer.php
mod/assign/styles.css

index a48eb0b..7d2bddc 100644 (file)
@@ -229,6 +229,8 @@ class assign_grading_table extends table_sql implements renderable {
      *
      * @param string $grade
      * @param boolean $editable
+     * @param int $userid The user id of the user this grade belongs to
+     * @param int $modified Timestamp showing when the grade was last modified
      * @return string The formatted grade
      */
     function display_grade($grade, $editable, $userid, $modified) {
@@ -236,7 +238,6 @@ class assign_grading_table extends table_sql implements renderable {
             return $grade;
         }
         $o = $this->assignment->display_grade($grade, $editable, $userid, $modified);
-
         return $o;
     }
 
@@ -342,7 +343,6 @@ class assign_grading_table extends table_sql implements renderable {
 
         $grade = $this->display_grade($row->grade, $this->quickgrading, $row->userid, $row->timemarked);
 
-
         //return $grade . $separator . $link;
         return $link . $separator . $grade;
     }
index 26ba6f4..ace9fab 100644 (file)
@@ -101,7 +101,7 @@ for <i>\'{$a->assignment}\'  at {$a->timeupdated}</i><br /><br />
 It is <a href="{$a->url}">available on the web site</a>.';
 $string['enabled'] = 'Enabled';
 $string['errornosubmissions'] = 'There are no submissions to download';
-$string['errorquickgradingnotcompatiblewithadvancedgrading'] = 'The grades were not saved because this assignment is currently using advanced grading';
+$string['errorquickgradingvsadvancedgrading'] = 'The grades were not saved because this assignment is currently using advanced grading';
 $string['errorrecordmodified'] = 'The grades were not saved because someone has modified one or more records more recently than when you loaded the page.';
 $string['feedbackcomments'] = 'Feedback comments';
 $string['feedback'] = 'Feedback';
index 45ec514..0e55f45 100644 (file)
@@ -889,6 +889,8 @@ class assign {
      *
      * @param mixed $grade int|null
      * @param boolean $editing Are we allowing changes to this grade?
+     * @param int $userid The user id the grade belongs to
+     * @param int $modified Timestamp from when the grade was last modified
      * @return string User-friendly representation of grade
      */
     public function display_grade($grade, $editing, $userid=0, $modified=0) {
@@ -896,10 +898,12 @@ class assign {
 
         static $scalegrades = array();
 
-        if ($this->get_instance()->grade >= 0) {    // Normal number
+        if ($this->get_instance()->grade >= 0) {
+            // Normal number
             if ($editing) {
                 $o = '<input type="text" name="quickgrade_' . $userid . '" value="' . $grade . '" size="6" maxlength="10" class="quickgrade"/>';
                 $o .= '&nbsp;/&nbsp;' . format_float($this->get_instance()->grade,2);
+                $o .= '<input type="hidden" name="grademodified_' . $userid . '" value="' . $modified . '"/>';
                 return $o;
             } else if ($grade == -1 || $grade === null) {
                 return '-';
@@ -907,7 +911,8 @@ class assign {
                 return format_float(($grade),2) .'&nbsp;/&nbsp;'. format_float($this->get_instance()->grade,2);
             }
 
-        } else {                                // Scale
+        } else {
+            // Scale
             if (empty($this->cache['scale'])) {
                 if ($scale = $DB->get_record('scale', array('id'=>-($this->get_instance()->grade)))) {
                     $this->cache['scale'] = make_menu_from_list($scale->scale);
@@ -2319,7 +2324,8 @@ class assign {
     /**
      * save quick grades
      *
-     * @return string - The result of the save operation
+     * @global moodle_database $DB
+     * @return string The result of the save operation
      */
     private function process_save_quick_grades() {
         global $USER, $DB, $CFG;
@@ -2331,36 +2337,37 @@ class assign {
         $gradingmanager = get_grading_manager($this->get_context(), 'mod_assign', 'submissions');
         $controller = $gradingmanager->get_active_controller();
         if (!empty($controller)) {
-            return get_string('errorquickgradingnotcompatiblewithadvancedgrading', 'assign');
+            return get_string('errorquickgradingvsadvancedgrading', 'assign');
         }
 
         $users = array();
         // first check all the last modified values
-        $len = strlen('grademodified_');
+        // Using POST is really unfortunate. A better solution needs to be found here. As we are looking for grades students we could
+        // gets a list of possible users and look for values based upon that.
         foreach ($_POST as $key => $value) {
-            if (substr($key, 0, $len) === 'grademodified_') {
+            if (preg_match('#^grademodified_(\d+)$#', $key, $matches)) {
                 // gather the userid, updated grade and last modified value
                 $record = new stdClass();
-                $record->userid = (int)substr($key, $len);
+                $record->userid = (int)$matches[1];
                 $record->grade = required_param('quickgrade_' . $record->userid, PARAM_INT);
-                $record->lastmodified = $value;
+                $record->lastmodified = clean_param($value, PARAM_INT);
                 $record->gradinginfo = grade_get_grades($this->get_course()->id, 'mod', 'assign', $this->get_instance()->id, array($record->userid));
                 $users[$record->userid] = $record;
             }
         }
-        list($userids, $useridparams) = $DB->get_in_or_equal(array_keys($users));
+        if (empty($users)) {
+            // Quick check to see whether we have any users to update and we don't
+            return get_string('quickgradingchangessaved', 'assign'); // Technical lie
+        }
 
+        list($userids, $params) = $DB->get_in_or_equal(array_keys($users), SQL_PARAMS_NAMED);
+        $params['assignment'] = $this->get_instance()->id;
         // check them all for currency
-        $currentgrades = $DB->get_recordset_sql('SELECT u.id as userid,
-                                                        g.grade as grade,
-                                                        g.timemodified as lastmodified
-                                                 FROM {user} u
-                                                 LEFT JOIN {assign_grades} g ON
-                                                            u.id = g.userid AND
-                                                            g.assignment = ? WHERE u.id ' .
-                                                 $userids,
-                                                 array_merge(array($this->get_instance()->id),
-                                                       $useridparams));
+        $sql = 'SELECT u.id as userid, g.grade as grade, g.timemodified as lastmodified
+                  FROM {user} u
+             LEFT JOIN {assign_grades} g ON u.id = g.userid AND g.assignment = :assignment
+                 WHERE u.id ' . $userids;
+        $currentgrades = $DB->get_recordset_sql($sql, $params);
 
         $modifiedusers = array();
         foreach ($currentgrades as $current) {
@@ -2396,6 +2403,7 @@ class assign {
             }
 
         }
+        $currentgrades->close();
 
         // ok - ready to process the updates
         foreach ($modifiedusers as $userid => $modified) {
@@ -2420,8 +2428,6 @@ class assign {
                 }
             }
 
-            $user = $DB->get_record('user', array('id' => $userid), '*', MUST_EXIST);
-
             $this->add_to_log('grade submission', $this->format_grade_for_log($grade));
         }
 
@@ -2442,10 +2448,7 @@ class assign {
         // Need submit permission to submit an assignment
         require_capability('mod/assign:grade', $this->context);
 
-
-
         $mform = new mod_assign_grading_options_form(null, array('cm'=>$this->get_course_module()->id, 'contextid'=>$this->context->id, 'userid'=>$USER->id, 'showquickgrading'=>false));
-
         if ($formdata = $mform->get_data()) {
             set_user_preference('assign_perpage', $formdata->perpage);
             set_user_preference('assign_filter', $formdata->filter);
index 841da27..d361b32 100644 (file)
@@ -101,34 +101,32 @@ M.mod_assign.init_grading_table = function(Y) {
 
 
         });
-        quickgrade = Y.all('.gradingtable .quickgrade');
+        var quickgrade = Y.all('.gradingtable .quickgrade');
         quickgrade.each(function(quick) {
             quick.on('change', function(e) {
-                parent = this.get('parentNode');
-                parent.addClass('quickgrademodified');
+                this.get('parentNode').addClass('quickgrademodified');
             });
         });
     });
 };
 
 M.mod_assign.check_dirty_quickgrading_form = function(e) {
+    if (!M.core_formchangechecker.get_form_dirty_state()) {
+        // the form is not dirty, so don't display any message
+        return;
+    }
 
-            if (!M.core_formchangechecker.get_form_dirty_state()) {
-                // the form is not dirty, so don't display any message
-                return;
-            }
-
-            // This is the error message that we'll show to browsers which support it
-            var warningmessage = 'There are unsaved quickgrading changes. Do you really wanto to leave this page?';
+    // This is the error message that we'll show to browsers which support it
+    var warningmessage = 'There are unsaved quickgrading changes. Do you really wanto to leave this page?';
 
-            // Most browsers are happy with the returnValue being set on the event
-            // But some browsers do not consistently pass the event
-            if (e) {
-                e.returnValue = warningmessage;
-            }
+    // Most browsers are happy with the returnValue being set on the event
+    // But some browsers do not consistently pass the event
+    if (e) {
+        e.returnValue = warningmessage;
+    }
 
-            // But some require it to be returned instead
-            return warningmessage;
+    // But some require it to be returned instead
+    return warningmessage;
 }
 M.mod_assign.init_grading_options = function(Y) {
     Y.use('node', function(Y) {
@@ -153,7 +151,7 @@ M.mod_assign.init_grading_options = function(Y) {
 // override the default dirty form behaviour to ignore any input with the class "ignoredirty"
 M.mod_assign.set_form_changed = M.core_formchangechecker.set_form_changed;
 M.core_formchangechecker.set_form_changed = function(e) {
-    target = e.currentTarget;
+    var target = e.currentTarget;
     if (!target.hasClass('ignoredirty')) {
         M.mod_assign.set_form_changed(e);
     }
index c08187e..9f57157 100644 (file)
@@ -92,15 +92,12 @@ class mod_assign_renderer extends plugin_renderer_base {
      * @return string
      */
     public function render_assign_quickgrading_result(assign_quickgrading_result $result) {
+        $url = new moodle_url('/mod/assign/view.php', array('id' => $result->coursemoduleid, 'action'=>'grading'));
+
         $o = '';
         $o .= $this->output->heading(get_string('quickgradingresult', 'assign'), 4);
-
         $o .= $this->output->notification($result->message);
-
-        $o .= $this->output->continue_button(new moodle_url('/mod/assign/view.php',
-                                                          array('id' => $result->coursemoduleid,
-                                                                'action'=>'grading')));
-
+        $o .= $this->output->continue_button($url);
         return $o;
     }
 
index 21445c6..11b8aa6 100644 (file)
@@ -127,5 +127,4 @@ div.earlysubmission {
 
 #page-mod-assign-view div.gradingtable tr .quickgrademodified {
     background-color: #FFCC99;
-}
-
+}
\ No newline at end of file