From 2e53372c37b2c1fccc2d4d99b0c3be77c2e717d3 Mon Sep 17 00:00:00 2001
From: skodak <skodak>
Date: Fri, 22 Jun 2007 18:33:32 +0000
Subject: [PATCH] MDL-10226 regrading of final grades improved, calculation
 cleanup before MDL-10231

---
 lib/grade/grade_category.php                  |  14 +--
 lib/grade/grade_item.php                      | 105 +++++++---------
 lib/gradelib.php                              | 112 +++++++++---------
 .../grade/simpletest/testgradeitem.php        |  12 +-
 4 files changed, 113 insertions(+), 130 deletions(-)

diff --git a/lib/grade/grade_category.php b/lib/grade/grade_category.php
index 74013768fe..f5f7e82f6f 100644
--- a/lib/grade/grade_category.php
+++ b/lib/grade/grade_category.php
@@ -304,15 +304,20 @@ class grade_category extends grade_object {
             return true;
         }
 
-        $result = true;
-
         $this->load_grade_item();
 
+        if ($this->grade_item->needsupdate) {
+            // this grade_item (and category) already needs update, no need to set it again here or in parent categories
+            return true;
+        }
+
         $paths = explode('/', $this->path);
 
         // Remove the first index, which is always empty
         unset($paths[0]);
 
+        $result = true;
+
         if (!empty($paths)) {
             $wheresql = '';
 
@@ -325,11 +330,6 @@ class grade_category extends grade_object {
 
         }
 
-        if (count($paths) == 1) {
-            // we are the top category - force recalculation of all formulas in course
-            $this->grade_item->force_recalculation();
-        }
-
         return $result;
     }
 
diff --git a/lib/grade/grade_item.php b/lib/grade/grade_item.php
index 59e169b5b8..141213980c 100644
--- a/lib/grade/grade_item.php
+++ b/lib/grade/grade_item.php
@@ -521,12 +521,15 @@ class grade_item extends grade_object {
 
         if ($this->is_locked()) {
             // locked grade items already have correct final grades
+            $this->needsupdate = false;
+            $this->update();
             return true;
         }
 
-        if ($calculation = $this->get_calculation()) {
-            if ($calculation->compute()) {
-                $this->force_regrade();
+        if ($this->is_calculated()) {
+            if ($this->calculation->compute()) {
+                $this->needsupdate = false;
+                $this->update();
                 return true;
             } else {
                 return array("Could not calculate grades for grade item id:".$this->id); // TODO: improve and localize
@@ -691,31 +694,11 @@ class grade_item extends grade_object {
         if ($category = $this->get_category()) {
             $category->force_regrading(); // we can ignore the result
 
-        } else {
-            $this->force_recalculation(); // recalculate all formulas - we do not know if any of them depends on this item
         }
 
-
         return $result;
     }
 
-    /**
-     * Force recalculation of all calculated grades in course.
-     */
-    function force_recalculation($courseid=null) {
-        if (empty($courseid)) {
-            $courseid = $this->courseid;
-        }
-        $grade_item = new grade_item(array('courseid'=>$courseid), false);
-        $grade_items = $grade_item->fetch_all_using_this();
-        foreach($grade_items as $gi) {
-            if ($gi->get_calculation()) {
-                $gi->needsupdate = true;
-                $gi->update();
-            }
-        }
-    }
-
     /**
      * Disassociates this item from its category parent(s). The object is then updated in DB.
      * @return boolean Success or Failure
@@ -791,67 +774,71 @@ class grade_item extends grade_object {
     }
 
     /**
-     * Returns this object's calculation.
-     * @param boolean $fetch Whether to fetch the value from the DB or not (false == just use the object's value)
-     * @return mixed $calculation Object if found, false otherwise.
+     * Checks if grade calculated. Returns this object's calculation.
+     * @return mixed $calculation Calculation object if exists, false otherwise.
      */
-    function get_calculation($nocache = false) {
+    function is_calculated() {
         if (is_null($this->calculation)) {
-            $nocache = true;
-        }
-
-        if ($nocache) {
             $this->calculation = grade_calculation::fetch('itemid', $this->id);
         }
 
-        return $this->calculation;
+        return !empty($this->calculation);
+    }
+
+    /**
+     * Returns calculation string if grade calculated.
+     * @return mixed string if calculation used, null if not
+     */
+    function get_calculation() {
+        if ($this->is_calculated()) {
+            return $this->calculation->calculation;
+        } else {
+            return NULL;
+        }
     }
 
     /**
      * Sets this item's calculation (creates it) if not yet set, or
      * updates it if already set (in the DB). If no calculation is given,
      * the calculation is removed.
-     * @param string $formula
-     * @return boolean
+     * @param string $formula string representation of formula used for calculation
+     * @return boolean success
      */
     function set_calculation($formula) {
-        // remove cached calculation object
+        // refresh cached calculation object
+        $this->calculation = null;
+
+        $result = true;
+
         if (empty($formula)) { // We are removing this calculation
-            if (!empty($this->id)) {
-                if ($grade_calculation = $this->get_calculation(true)) {
-                    $grade_calculation->delete();
-                }
+            if (!empty($this->id) and $this->is_calculated()) {
+                $this->calculation->delete();
+                $this->calculation = null; // remove cache
             }
-            $this->calculation = false; // cache no calculation present
-            $this->force_regrading();
-            return true;
 
         } else { // We are updating or creating the calculation entry in the DB
-            if ($grade_calculation = $this->get_calculation(true)) {
-                $grade_calculation->calculation = $formula;
-                if ($grade_calculation->update()) {
-                    $this->force_regrading();
-                    return true;
-                } else {
+            if ($this->is_calculated()) {
+                $this->calculation->calculation = $formula;
+                if (!$this->calculation->update()) {
                     $this->calculation = null; // remove cache
+                    $result = false;
                     debugging("Could not save the calculation in the database for this grade_item.");
-                    return false;
                 }
 
             } else {
-                $grade_calculation = new grade_calculation();
-                $grade_calculation->calculation = $formula;
-                $grade_calculation->itemid = $this->id;
-
-                if ($grade_calculation->insert()) {
-                    return true;
-                } else {
+                $grade_calculation = new grade_calculation(array('calculation'=>$formula, 'itemid'=>$this->id), false);
+                if (!$grade_calculation->insert()) {
                     $this->calculation = null; // remove cache
+                    $result = false;
                     debugging("Could not save the calculation in the database for this grade_item.");
-                    return false;
                 }
+                $this->calculation = $grade_calculation;
             }
         }
+
+        $this->force_regrading();
+
+        return $result;
     }
 
     /**
@@ -979,7 +966,7 @@ class grade_item extends grade_object {
             return array();
         }
 
-        if ($calculation = $this->get_calculation()) {
+        if ($calculation = $this->is_calculated()) {
             return $calculation->dependson();
 
         } else if ($this->itemtype == 'category') {
@@ -1030,7 +1017,7 @@ class grade_item extends grade_object {
     function update_raw_grade($userid, $rawgrade=false, $howmodified='manual', $note=NULL, $feedback=false, $feedbackformat=FORMAT_MOODLE) {
 
         // calculated grades can not be updated
-        if ($this->get_calculation()) {
+        if ($this->is_calculated()) {
             return false;
         }
 
diff --git a/lib/gradelib.php b/lib/gradelib.php
index da01ed0f01..13ebbc867e 100644
--- a/lib/gradelib.php
+++ b/lib/gradelib.php
@@ -116,6 +116,15 @@ function grade_update($source, $courseid, $itemtype, $itemmodule, $iteminstance,
         $params = compact('courseid', 'itemtype', 'itemmodule', 'iteminstance', 'itemnumber');
         if ($itemdetails) {
             $itemdetails = (array)$itemdetails;
+
+            // grademin and grademax ignored when scale specified            
+            if (array_key_exists('scaleid', $itemdetails)) {
+                if ($itemdetails['scaleid']) {
+                    unset($itemdetails['grademin']);
+                    unset($itemdetails['grademax']);
+                }
+            }
+
             foreach ($itemdetails as $k=>$v) {
                 if (!in_array($k, $allowed)) {
                     // ignore it
@@ -320,52 +329,66 @@ function grade_get_items($courseid, $itemtype=NULL, $itemmodule=NULL, $iteminsta
  * Updates all final grades in course.
  *
  * @param int $courseid
+ * @param boolean $regradeall force regrading of all items
+ *
  * @return boolean true if ok, array of errors if problems found
  */
-function grade_update_final_grades($courseid) {
-    $errors = array();
-    $grade_item = new grade_item();
-    $grade_item->courseid = $courseid;
-    if (!$grade_items = $grade_item->fetch_all_using_this()) {
-        return true;
-    }
+function grade_update_final_grades($courseid, $regradeall=false) {
 
-    $needsupdate = false;
-    $calculated = false;
-    foreach ($grade_items as $gid=>$gitem) {
-        $grade_item =& $grade_items[$gid];
-        if ($grade_item->needsupdate) {
-            $needsupdate = true;
-        }
-        if ($grade_item->get_calculation()) {
-            $calculated = true;
-        }
+    if ($regradeall) {
+        set_field('grade_items', 'needsupdate', 1, 'courseid', $courseid);
     }
 
-    // no update needed
-    if (!$needsupdate) {
+    $grade_item = new grade_item(array('courseid'=>$courseid), false);
+
+    if (!$grade_items = $grade_item->fetch_all_using_this()) {
         return true;
     }
 
-    // the easy way
-    if (!$calculated) {
+    if (!$regradeall) {
+        $needsupdate = false;
+        $calculated = false;
         foreach ($grade_items as $gid=>$gitem) {
             $grade_item =& $grade_items[$gid];
             if ($grade_item->needsupdate) {
-                $result = $grade_item->update_final_grades();
-                if ($result !== true) {
-                    $errors = array_merge($errors, $result);
-                }
+                $needsupdate = true;
+            }
+            if ($grade_item->is_calculated()) {
+                $calculated = true;
             }
         }
-
-        if (count($errors) == 0) {
+    
+        if (!$needsupdate) {
+            // no update needed
             return true;
-        } else {
-            return $errors;
+    
+        } else if ($calculated) {
+            // flag all calculated grade items with needsupdate
+            // we want to make sure all are ok, this can be improved later with proper dependency calculation
+            foreach ($grade_items as $gid=>$gitem) {
+                $grade_item =& $grade_items[$gid];
+                if (!$grade_item->is_calculated()) {
+                    continue;
+                }
+                $grade_item->update_from_db(); // make sure we have current data, it might have been updated in this loop already
+                if (!$grade_item->needsupdate) {
+                    //force recalculation and forced update of all parents
+                    $grade_item->force_regrading();
+                }
+            } 
+    
+            // again make sure all date is up-to-date - the needsupdate flag might have changed
+            foreach ($grade_items as $gid=>$gitem) {
+                $grade_item =& $grade_items[$gid];
+                $grade_item->update_from_db();
+                unset($grade_item->category);
+            }
         }
     }
 
+
+    $errors = array();
+
     // now the hard way with calculated grade_items or categories
     $finalitems = array();
     $finalids = array();
@@ -373,41 +396,16 @@ function grade_update_final_grades($courseid) {
         $count = 0;
         foreach ($grade_items as $gid=>$gitem) {
             $grade_item =& $grade_items[$gid];
-            if (!$grade_item->needsupdate and $grade_item->itemtype!='category' and !$grade_item->get_calculation()) {
+            if (!$grade_item->needsupdate) {
                 $finalitems[$gid] = $grade_item;
                 $finalids[] = $gid;
                 unset($grade_items[$gid]);
                 continue;
             }
 
+            //do we have all data for finalizing of this item?
             $dependson = $grade_item->dependson();
 
-            //are we dealing with category with no calculated items?
-            // we can not trust the needsupdate flag because category might contain calculated items
-            if ($grade_item->itemtype=='category' and !$grade_item->needsupdate) {
-                $forceupdate = false;
-                foreach ($dependson as $childid) {
-                    if (in_array($childid, $finalids)) {
-                        $child = $finalitems[$childid];
-                    } else {
-                        $child = $grade_items[$childid];
-                    }
-                    if ($child->itemtype == 'category' or $child->get_calculation()) {
-                        $forceupdate = true;
-                    }
-                }
-
-                if ($forceupdate) {
-                    $grade_item->force_regrading();
-                } else {
-                    $finalitems[$gid] = $grade_item;
-                    $finalids[] = $gid;
-                    unset($grade_items[$gid]);
-                    continue;
-                }
-            }
-
-            //do we have all data for this item?
             $doupdate = true;
             foreach ($dependson as $did) {
                 if (!in_array($did, $finalids)) {
@@ -430,7 +428,7 @@ function grade_update_final_grades($courseid) {
 
         if ($count == 0) {
             foreach($grade_items as $grade_item) {
-                $errors[] = 'Probably circular reference in grade_item id:'.$grade_item->id; // TODO: localize
+                $errors[] = 'Probably circular reference or broken calculation formula in grade_item id:'.$grade_item->id; // TODO: localize
             }
             break;
         }
diff --git a/lib/simpletest/grade/simpletest/testgradeitem.php b/lib/simpletest/grade/simpletest/testgradeitem.php
index 03485d3d56..f8cc1e6155 100755
--- a/lib/simpletest/grade/simpletest/testgradeitem.php
+++ b/lib/simpletest/grade/simpletest/testgradeitem.php
@@ -232,22 +232,20 @@ class grade_item_test extends grade_test {
         $this->assertEqual($this->grade_grades[0]->finalgrade, $final_grade->finalgrade);
     }
 
-    function test_grade_item_get_calculation() {
+    function test_grade_item_is_calculated() {
         $grade_item = new grade_item($this->grade_items[1]);
-        $this->assertTrue(method_exists($grade_item, 'get_calculation'));
-        $grade_calculation = $grade_item->get_calculation();
-
-        $this->assertEqual($this->grade_calculations[0]->id, $grade_calculation->id);
+        $this->assertTrue(method_exists($grade_item, 'is_calculated'));
+        $this->assertTrue($grade_item->is_calculated());
     }
 
     function test_grade_item_set_calculation() {
 /*        $grade_item = new grade_item($this->grade_items[1]);
         $this->assertTrue(method_exists($grade_item, 'set_calculation'));
-        $this->assertTrue(method_exists($grade_item, 'get_calculation'));
+        $this->assertTrue(method_exists($grade_item, 'is_calculated'));
 
         $calculation = '=SUM([unittestgradeitem1], [unittestgradeitem3])';
         $grade_item->set_calculation($calculation);
-        $new_calculation = $grade_item->get_calculation();
+        $new_calculation = $grade_item->is_calculated();
 
         $this->assertEqual($calculation, $new_calculation->calculation);
 */    }
-- 
2.39.5