MDL-47064 Grades: Peer review cleanups
authorDamyon Wiese <damyon@moodle.com>
Wed, 24 Sep 2014 08:21:30 +0000 (16:21 +0800)
committerAdrian Greeve <adrian@moodle.com>
Fri, 3 Oct 2014 05:55:23 +0000 (13:55 +0800)
Changes include:
 * Search for existing items to reduce DB queries in grade_category::aggregate_grades
 * Comments improvements
 * Move brackets to be part of lang string
 * Convert aggregationhints to be a class variable instead of passing it around

Part of: MDL-46576

grade/report/lib.php
grade/report/user/lib.php
lang/en/grades.php
lib/grade/grade_category.php

index 28373c7..67ac4f6 100644 (file)
@@ -480,28 +480,28 @@ abstract class grade_report {
 
         //if the item definitely depends on a hidden item
         if (array_key_exists($course_item->id, $hiding_affected['altered']) ||
-            array_key_exists($course_item->id, $hiding_affected['alteredgrademin']) ||
-            array_key_exists($course_item->id, $hiding_affected['alteredgrademax']) ||
-            array_key_exists($course_item->id, $hiding_affected['alteredaggregationstatus']) ||
-            array_key_exists($course_item->id, $hiding_affected['alteredaggregationweight'])) { 
+                array_key_exists($course_item->id, $hiding_affected['alteredgrademin']) ||
+                array_key_exists($course_item->id, $hiding_affected['alteredgrademax']) ||
+                array_key_exists($course_item->id, $hiding_affected['alteredaggregationstatus']) ||
+                array_key_exists($course_item->id, $hiding_affected['alteredaggregationweight'])) { 
             if (!$this->showtotalsifcontainhidden[$courseid]) {
                 //hide the grade
                 $finalgrade = null;
             } else {
                 //use reprocessed marks that exclude hidden items
-                if (!empty($hiding_affected['altered'][$course_item->id])) {
+                if (isset($hiding_affected['altered'][$course_item->id])) {
                     $finalgrade = $hiding_affected['altered'][$course_item->id];
                 }
-                if (!empty($hiding_affected['alteredgrademin'][$course_item->id])) {
+                if (isset($hiding_affected['alteredgrademin'][$course_item->id])) {
                     $grademin = $hiding_affected['alteredgrademin'][$course_item->id];
                 }
-                if (!empty($hiding_affected['alteredgrademax'][$course_item->id])) {
+                if (isset($hiding_affected['alteredgrademax'][$course_item->id])) {
                     $grademax = $hiding_affected['alteredgrademax'][$course_item->id];
                 }
-                if (!empty($hiding_affected['alteredaggregationstatus'][$course_item->id])) {
+                if (isset($hiding_affected['alteredaggregationstatus'][$course_item->id])) {
                     $aggregationstatus = $hiding_affected['alteredaggregationstatus'][$course_item->id];
                 }
-                if (!empty($hiding_affected['alteredaggregationweight'][$course_item->id])) {
+                if (isset($hiding_affected['alteredaggregationweight'][$course_item->id])) {
                     $aggregationweight = $hiding_affected['alteredaggregationweight'][$course_item->id];
                 }
             }
index ce59ea7..7fe7d59 100644 (file)
@@ -176,6 +176,15 @@ class grade_report_user extends grade_report {
      */
     protected $viewasuser = false;
 
+    /**
+     * An array that collects the aggregationhints for every
+     * grade_item. The hints contain grade, grademin, grademax
+     * status, weight and parent.
+     *
+     * @var array
+     */
+    protected $aggregationhints = array();
+
     /**
      * Constructor. Sets local copies of user preferences and initialises grade_tree.
      * @param int $courseid
@@ -354,11 +363,8 @@ class grade_report_user extends grade_report {
      * Fill the table with data.
      *
      * @param $element - An array containing the table data for the current row.
-     * @param $aggregationhints - An array that collects the aggregationhints for every
-     *                            grade_item. The hints contain grade, grademin, grademax
-     *                            status, weight and parent.
      */
-    private function fill_table_recursive(&$element, &$aggregationhints = array()) {
+    private function fill_table_recursive(&$element) {
         global $DB, $CFG;
 
         $type = $element['type'];
@@ -494,7 +500,7 @@ class grade_report_user extends grade_report {
                         $data['weight']['content'] = format_float($hint['weight'] * 100.0, 2) . ' %';
                     }
                     if ($hint['status'] != 'used' && $hint['status'] != 'unknown') {
-                        $data['weight']['content'] .= '<br/>( ' . get_string('aggregationhint' . $hint['status'], 'grades') . ' )';
+                        $data['weight']['content'] .= '<br>' . get_string('aggregationhint' . $hint['status'], 'grades');
                     }
                 }
 
@@ -637,7 +643,7 @@ class grade_report_user extends grade_report {
                         $parent = $parent->load_parent_category();
                     }
                     $hint['parent'] = $parent->load_grade_item()->id;
-                    $aggregationhints[$grade_grade->itemid] = $hint;
+                    $this->aggregationhints[$grade_grade->itemid] = $hint;
                 }
             }
         }
@@ -664,7 +670,7 @@ class grade_report_user extends grade_report {
         /// Recursively iterate through all child elements
         if (isset($element['children'])) {
             foreach ($element['children'] as $key=>$child) {
-                $this->fill_table_recursive($element['children'][$key], $aggregationhints);
+                $this->fill_table_recursive($element['children'][$key]);
             }
         }
 
@@ -673,7 +679,7 @@ class grade_report_user extends grade_report {
         if ($this->showcontributiontocoursetotal && ($type == 'category' && $depth == 1)) {
             // We should have collected all the hints by now - walk the tree again and build the contributions column.
 
-            $this->fill_contributions_column($element, $aggregationhints);
+            $this->fill_contributions_column($element);
         }
     }
 
@@ -683,16 +689,13 @@ class grade_report_user extends grade_report {
      * grade_item.
      *
      * @param $element - An array containing the table data for the current row.
-     * @param $aggregationhints - An array that collects the aggregationhints for every
-     *                            grade_item. The hints contain grade, grademin, grademax
-     *                            status, weight and parent.
      */
-    public function fill_contributions_column($element, $aggregationhints) {
+    public function fill_contributions_column($element) {
 
-        /// Recursively iterate through all child elements
+        // Recursively iterate through all child elements.
         if (isset($element['children'])) {
             foreach ($element['children'] as $key=>$child) {
-                $this->fill_contributions_column($element['children'][$key], $aggregationhints);
+                $this->fill_contributions_column($element['children'][$key]);
             }
         } else if ($element['type'] == 'item') {
             // This is a grade item (We don't do this for categories or we would double count).
@@ -700,40 +703,38 @@ class grade_report_user extends grade_report {
             $itemid = $grade_object->id;
 
             // Ignore anything with no hint - e.g. a hidden row.
-            if (isset($aggregationhints[$itemid])) {
+            if (isset($this->aggregationhints[$itemid])) {
 
                 // Normalise the gradeval.
-                $graderange = $aggregationhints[$itemid]['grademax'] - $aggregationhints[$itemid]['grademin'];
-                $gradeval = ($aggregationhints[$itemid]['grade'] - $aggregationhints[$itemid]['grademin']) / $graderange;
+                $graderange = $this->aggregationhints[$itemid]['grademax'] - $this->aggregationhints[$itemid]['grademin'];
+                $gradeval = ($this->aggregationhints[$itemid]['grade'] - $this->aggregationhints[$itemid]['grademin']) / $graderange;
 
                 // Multiply the normalised value by the weight
                 // of all the categories higher in the tree.
-                $parent = $aggregationhints[$itemid]['parent'];
                 do {
-                    if (!is_null($aggregationhints[$itemid]['weight'])) {
-                        $gradeval *= $aggregationhints[$itemid]['weight'];
+                    if (!is_null($this->aggregationhints[$itemid]['weight'])) {
+                        $gradeval *= $this->aggregationhints[$itemid]['weight'];
                     }
 
                     // The second part of this if is to prevent infinite loops
                     // in case of crazy data.
-                    if (isset($aggregationhints[$itemid]['parent']) &&
-                        $aggregationhints[$itemid]['parent'] != $itemid) {
-                        $parent = $aggregationhints[$itemid]['parent'];
+                    if (isset($this->aggregationhints[$itemid]['parent']) &&
+                            $this->aggregationhints[$itemid]['parent'] != $itemid) {
+                        $parent = $this->aggregationhints[$itemid]['parent'];
                         $itemid = $parent;
                     } else {
-                        // We are at the top of the tree
+                        // We are at the top of the tree.
                         $parent = false;
                     }
                 } while ($parent);
                 // Finally multiply by the course grademax.
-                $gradeval *= $aggregationhints[$itemid]['grademax'];
+                $gradeval *= $this->aggregationhints[$itemid]['grademax'];
 
                 // Now we need to loop through the "built" table data and update the
                 // contributions column for the current row.
                 $header_row = "row_{$grade_object->id}_{$this->user->id}";
                 foreach ($this->tabledata as $key => $row) {
-                    if (isset($row['itemname']) &&
-                        ($row['itemname']['id'] == $header_row)) {
+                    if (isset($row['itemname']) && ($row['itemname']['id'] == $header_row)) {
                         // Found it - update the column.
                         $decimals = $grade_object->get_decimals();
                         $this->tabledata[$key]['contributiontocoursetotal']['content'] = format_float($gradeval, $decimals, true);
index 0327e91..f2853c2 100644 (file)
@@ -64,10 +64,10 @@ $string['aggregation_help'] = 'The aggregation determines how grades in a catego
 * Highest grade
 * Mode of grades - The grade that occurs the most frequently
 * Natural - The sum of all grade values scaled by weight';
-$string['aggregationhintnovalue'] = 'Empty';
-$string['aggregationhintdropped'] = 'Dropped';
-$string['aggregationhintexcluded'] = 'Excluded';
-$string['aggregationhintextra'] = 'Extra credit';
+$string['aggregationhintnovalue'] = '( Empty )';
+$string['aggregationhintdropped'] = '( Dropped )';
+$string['aggregationhintexcluded'] = '( Excluded )';
+$string['aggregationhintextra'] = '( Extra credit )';
 $string['aggregation_link'] = 'grade/aggregation';
 $string['aggregationcoef'] = 'Aggregation coefficient';
 $string['aggregationcoefextra'] = 'Extra credit'; // for the header of the table at Edit categories and items page
index d95e123..cd62e56 100644 (file)
@@ -564,7 +564,7 @@ class grade_category extends grade_object {
                                       $excluded,
                                       $grademinoverrides,
                                       $grademaxoverrides) {
-        global $CFG;
+        global $CFG, $DB;
 
         // Remember these so we can set flags on them to describe how they were used in the aggregation.
         $novalue = array();
@@ -601,16 +601,26 @@ class grade_category extends grade_object {
         // Make sure a grade_grade exists for every grade_item.
         // We need to do this so we can set the aggregationstatus
         // with a set_field call instead of checking if each one exists and creating/updating.
-        foreach ($items as $itemid => $gradeitem) {
-            $gradegrade = new grade_grade(array('itemid' => $gradeitem->id,
-                                                'userid' => $userid,
-                                                'rawgrademin' => $gradeitem->grademin,
-                                                'rawgrademax' => $gradeitem->grademax), false);
-            $gradegrade->grade_item = $this->grade_item;
-            if (!$gradegrade->fetch(array('itemid'=>$gradeitem->id, 'userid'=>$userid))) {
+        if (count($items) > 0) {
+            list($ggsql, $params) = $DB->get_in_or_equal(array_keys($items), SQL_PARAMS_NAMED, 'g');
+
+
+            $params['userid'] = $userid;
+            $sql = "SELECT itemid
+                      FROM {grade_grades}
+                     WHERE itemid $ggsql AND userid = :userid";
+            $existingitems = $DB->get_records_sql($sql, $params);
+
+            $notexisting = array_diff(array_keys($items), array_keys($existingitems));
+            foreach ($notexisting as $itemid) {
+                $gradeitem = $this->grade_item;
+                $gradegrade = new grade_grade(array('itemid' => $itemid,
+                                                    'userid' => $userid,
+                                                    'rawgrademin' => $gradeitem->grademin,
+                                                    'rawgrademax' => $gradeitem->grademax), false);
+                $gradegrade->grade_item = $gradeitem;
                 $gradegrade->insert('system');
             }
-
         }
 
         // if no grades calculation possible or grading not allowed clear final grade
@@ -632,7 +642,7 @@ class grade_category extends grade_object {
                 // If null, it means no grade.
                 if ($this->aggregateonlygraded) {
                     unset($grade_values[$itemid]);
-                    // Mark this item as dropped because it has no grade.
+                    // Mark this item as "excluded empty" because it has no grade.
                     $novalue[$itemid] = 0;
                     continue;
                 }
@@ -654,14 +664,13 @@ class grade_category extends grade_object {
             $grade_values[$itemid] = grade_grade::standardise_score($v, $usergrademin, $usergrademax, 0, 1);
         }
 
-        // Use min grade if grade missing for these types.
+        // For items with no value, and not excluded - either set their grade to 0 or exclude them.
         foreach ($items as $itemid=>$value) {
             if (!isset($grade_values[$itemid]) and !in_array($itemid, $excluded)) {
                 if (!$this->aggregateonlygraded) {
                     $grade_values[$itemid] = 0;
                 } else {
-                    // We are specifically marking these items that get dropped
-                    // because they are empty.
+                    // We are specifically marking these items as "excluded empty".
                     $novalue[$itemid] = 0;
                 }
             }
@@ -811,6 +820,10 @@ class grade_category extends grade_object {
      * @since Moodle 2.6.5, 2.7.2
      * @param array & $weights If provided, will be filled with the normalized weights
      *                         for each grade_item as used in the aggregation.
+     *                         Some rules for the weights are:
+     *                         1. The weights must add up to 1 (unless there are extra credit)
+     *                         2. The contributed points column must add up to the course
+     *                         final grade and this column is calculated from these weights.
      * @param array  $grademinoverrides User specific grademin values if different to the grade_item grademin (key is itemid)
      * @param array  $grademaxoverrides User specific grademax values if different to the grade_item grademax (key is itemid)
      * @return array containing values for: