From: David Mudrak <david.mudrak@gmail.com>
Date: Mon, 4 Jan 2010 17:43:02 +0000 (+0000)
Subject: Edit grading form refactoring
X-Git-Url: http://git.mjollnir.org/gw?a=commitdiff_plain;h=a8389d652b2e0de3fdfad9069801000a2685d976;p=moodle.git

Edit grading form refactoring

The previous desing was too "base-class oriented". I have realized that
strategies have to deal with loading and saving dimension definitions in
their own way. Getting rid of all db<->form field mappings as it would
work for very simple strategies only.
---

diff --git a/mod/workshop/editgradingform.php b/mod/workshop/editgradingform.php
index 8f3914d7e2..c7710dbd1f 100644
--- a/mod/workshop/editgradingform.php
+++ b/mod/workshop/editgradingform.php
@@ -64,21 +64,21 @@ if (file_exists($strategylib)) {
 $classname = 'workshop_' . $workshop->strategy . '_strategy';
 $strategy = new $classname($workshop);
 
-// load the dimensions from the database
-$dimensions = $strategy->load_dimensions();
+// load the assessment form definition from the database
+// this must be called before get_edit_strategy_form() where we have to know
+// the number of repeating fieldsets
+$formdata = $strategy->load_grading_form();
 
 // load the form to edit the grading strategy dimensions
-$mform = $strategy->get_edit_strategy_form($selfurl, count($dimensions));
+$mform = $strategy->get_edit_strategy_form($selfurl);
 
 // initialize form data
-$formdata = new stdClass;
-$formdata->dimensions = $dimensions;
 $mform->set_data($formdata);
 
 if ($mform->is_cancelled()) {
     redirect($returnurl);
 } elseif ($data = $mform->get_data()) {
-    $strategy->save_dimensions($data);
+    $strategy->save_grading_form($data);
     if (isset($data->saveandclose)) {
         redirect($returnurl);
     } else {
diff --git a/mod/workshop/grading/accumulative/gradingform.php b/mod/workshop/grading/accumulative/gradingform.php
index 269e9dabbc..8011374c3c 100644
--- a/mod/workshop/grading/accumulative/gradingform.php
+++ b/mod/workshop/grading/accumulative/gradingform.php
@@ -68,7 +68,7 @@ class workshop_edit_accumulative_strategy_form extends workshop_edit_strategy_fo
 
         $numofdimensionstoadd   = 2;
         $numofinitialdimensions = 3;
-        $numofdisplaydimensions = max($this->nocurrentdims + $numofdimensionstoadd, $numofinitialdimensions);
+        $numofdisplaydimensions = max($this->strategy->get_number_of_dimensions() + $numofdimensionstoadd, $numofinitialdimensions);
         $this->repeat_elements($repeated, $numofdisplaydimensions,  $repeatedoptions, 'numofdimensions', 'adddimensions', $numofdimensionstoadd, get_string('addmoredimensionsaccumulative', 'workshop', $numofdimensionstoadd));
     }
 
diff --git a/mod/workshop/grading/accumulative/simpletest/teststrategy.php b/mod/workshop/grading/accumulative/simpletest/teststrategy.php
index 0ffcf7f5d6..180f704115 100644
--- a/mod/workshop/grading/accumulative/simpletest/teststrategy.php
+++ b/mod/workshop/grading/accumulative/simpletest/teststrategy.php
@@ -35,9 +35,14 @@ require_once($CFG->dirroot . '/mod/workshop/grading/accumulative/strategy.php');
  */
 class testable_workshop_accumulative_strategy extends workshop_accumulative_strategy {
 
-    public function cook_form_data($raw) {
-        return parent::cook_form_data($raw);
+    public function _cook_dimension_records(array $raw) {
+        return parent::_cook_dimension_records($raw);
     }
+
+    public function _cook_form_data(stdClass $raw) {
+        return parent::_cook_form_data($raw);
+    }
+
 }
 
 
@@ -49,8 +54,11 @@ class workshop_accumulative_strategy_test extends UnitTestCase {
     /** instance of the strategy logic class being tested */
     protected $strategy;
 
-    /** this emulates data returned by get_data() of a submitted strategy edit form */
-    protected $rawdata;
+    /** this emulates dimensions data returned by get_data() of a submitted strategy edit form */
+    protected $rawform;
+
+    /** this emulates dimensions data stored in database to be loaded into strategy edit form */
+    protected $rawdb;
 
     /** setup testing environment */
     public function setUp() {
@@ -63,24 +71,69 @@ class workshop_accumulative_strategy_test extends UnitTestCase {
 
         // emulate a form with 5 dimensions. The first three are already in DB, the forth is new and the
         // fifth is left empty
-        $this->rawdata = new stdClass;
-        $this->rawdata->workshopid          = 42;
-        $this->rawdata->strategy            = 'accumulative';
-        $this->rawdata->numofdimensions     = 5;
-        $this->rawdata->dimensionid         = array(0 => 3,       1=> 2,         2 => 1,       3 => 0,       4 => 0);
-        $this->rawdata->description         = array(0 => 'First', 1 => 'Second', 2 => 'Third', 3 => 'Forth', 4 => '');
-        $this->rawdata->descriptionformat   = array(0 => 1,       1 => 1,        2 => 1,       3 => 1,       4 => 1);
-        $this->rawdata->grade               = array(0 => 10,      1 => 5,        2 => 5,       3 => 2,       4 => 10);
-        $this->rawdata->weight              = array(0 => 1,       1 => 1,        2 => 2,       3 => 2,       4 => 1);
+        $this->rawform = new stdClass;
+        $this->rawform->workshopid          = 42;
+        $this->rawform->strategy            = 'accumulative';
+        $this->rawform->numofdimensions     = 5;
+        $this->rawform->dimensionid         = array(0 => 3,       1=> 2,         2 => 1,       3 => 0,       4 => 0);
+        $this->rawform->description         = array(0 => 'First', 1 => 'Second', 2 => 'Third', 3 => 'Forth', 4 => '');
+        $this->rawform->descriptionformat   = array(0 => 1,       1 => 1,        2 => 1,       3 => 1,       4 => 1);
+        $this->rawform->grade               = array(0 => 10,      1 => 5,        2 => 5,       3 => 2,       4 => 10);
+        $this->rawform->weight              = array(0 => 1,       1 => 1,        2 => 2,       3 => 2,       4 => 1);
+
+        // emulate two assessment dimensions being stored in database
+        $this->rawdb = array();
+        $this->rawdb[3] = new stdClass;
+        $this->rawdb[3]->id                 = 3;
+        $this->rawdb[3]->workshopid         = 42;
+        $this->rawdb[3]->sort               = 1;
+        $this->rawdb[3]->description        = 'First';
+        $this->rawdb[3]->descriptionformat  = 1;
+        $this->rawdb[3]->grade              = 20;
+        $this->rawdb[3]->weight             = 16;
+
+        $this->rawdb[7] = new stdClass;
+        $this->rawdb[7]->id                 = 7;
+        $this->rawdb[7]->workshopid         = 42;
+        $this->rawdb[7]->sort               = 2;
+        $this->rawdb[7]->description        = 'Second';
+        $this->rawdb[7]->descriptionformat  = 1;
+        $this->rawdb[7]->grade              = 10;
+        $this->rawdb[7]->weight             = 1;
+
     }
 
+
     public function tearDown() {
         $this->strategy = null;
+        $this->rawform  = null;
+        $this->rawdb    = null;
     }
 
+    
+    public function test_cook_dimension_records() {
+        
+        $cooked = $this->strategy->_cook_dimension_records($this->rawdb);
+        $this->assertIsA($cooked, 'stdClass');
+        $cooked = (array)$cooked;
+
+        $this->assertEqual($cooked['dimensionid[0]'], 3);
+        $this->assertEqual($cooked['description[0]'], 'First');
+        $this->assertEqual($cooked['descriptionformat[0]'], 1);
+        $this->assertEqual($cooked['grade[0]'], 20);
+        $this->assertEqual($cooked['weight[0]'], 16);
+
+        $this->assertEqual($cooked['dimensionid[1]'], 7);
+        $this->assertEqual($cooked['description[1]'], 'Second');
+        $this->assertEqual($cooked['descriptionformat[1]'], 1);
+        $this->assertEqual($cooked['grade[1]'], 10);
+        $this->assertEqual($cooked['weight[1]'], 1);
+    }
+
+    
     public function test_cook_form_data() {
 
-        $cooked = $this->strategy->cook_form_data($this->rawdata);
+        $cooked = $this->strategy->_cook_form_data($this->rawform);
         $this->assertIsA($cooked, 'Array');
         $this->assertEqual($cooked[2], (object)array(
                             'id'                => 1,
diff --git a/mod/workshop/grading/accumulative/strategy.php b/mod/workshop/grading/accumulative/strategy.php
index d08660986c..15d56db7b4 100644
--- a/mod/workshop/grading/accumulative/strategy.php
+++ b/mod/workshop/grading/accumulative/strategy.php
@@ -32,21 +32,111 @@ require_once(dirname(dirname(__FILE__)) . '/strategy.php'); // parent class
 /**
  * Accumulative grading strategy logic.
  */
-class workshop_accumulative_strategy extends workshop_strategy {
+class workshop_accumulative_strategy extends workshop_base_strategy {
+
+    public function load_grading_form() {
+        global $DB;
+
+        $dims = $DB->get_records('workshop_forms_' . $this->name, array('workshopid' => $this->workshop->id), 'sort');
+        $this->nodimensions = count($dims);
+        return $this->_cook_dimension_records($dims);
+    }
+
+
+    /**
+     * Transpones the dimension data from DB so the assessment form editor can be populated by set_data
+     *
+     * Called internally from load_grading_form(). Could be private but keeping protected
+     * for unit testing purposes.
+     * 
+     * @param array $raw Array of raw dimension records as fetched by get_record()
+     * @return array Array of fields data to be used by the mform set_data
+     */
+    protected function _cook_dimension_records(array $raw) {
+
+        $formdata = array();
+        $key = 0;
+        foreach ($raw as $dimension) {
+            $formdata['dimensionid[' . $key . ']']       = $dimension->id;
+            $formdata['description[' . $key . ']']       = $dimension->description;
+            $formdata['descriptionformat[' . $key . ']'] = $dimension->descriptionformat;
+            $formdata['grade[' . $key . ']']             = $dimension->grade;
+            $formdata['weight[' . $key . ']']            = $dimension->weight;
+            $key++;
+        }
+        return (object)$formdata;
+    }
+
 
     /**
-     * Mapping of the db fields to the form fields for every dimension of assessment
+     * Save the assessment dimensions into database
      *
-     * @return array Array ['field_db_name' => 'field_form_name']
+     * Saves data into the main strategy form table. If the record->id is null or zero,
+     * new record is created. If the record->id is not empty, the existing record is updated. Records with
+     * empty 'description' field are removed from database.
+     * The passed data object are the raw data returned by the get_data().
+     *
+     * @uses $DB
+     * @param object $data Raw data returned by the dimension editor form
+     * @access public
+     * @return void
      */
-    public function map_dimension_fieldnames() {
-        return array(
-                'id'                => 'dimensionid',
-                'description'       => 'description',
-                'descriptionformat' => 'descriptionformat',
-                'grade'             => 'grade',
-                'weight'            => 'weight',
-            );
+    public function save_grading_form(stdClass $data) {
+        global $DB;
+
+        if (!isset($data->strategyname) || ($data->strategyname != $this->name)) {
+            // the workshop strategy has changed since the form was opened for editing
+            throw new moodle_exception('strategyhaschanged', 'workshop');
+        }
+
+        $data = $this->_cook_form_data($data);
+        $todelete = array();
+        foreach ($data as $record) {
+            if (empty($record->description)) {
+                if (!empty($record->id)) {
+                    // existing record with empty description - to be deleted
+                    $todelete[] = $record->id;
+                }
+                continue;
+            }
+            if (empty($record->id)) {
+                // new field
+                $record->id = $DB->insert_record('workshop_forms_' . $this->name, $record);
+            } else {
+                // exiting field
+                $DB->update_record('workshop_forms_' . $this->name, $record);
+            }
+        }
+        $DB->delete_records_list('workshop_forms_' . $this->name, 'id', $todelete);
+    }
+
+
+    /**
+     * Prepares data returned by mform so they can be saved into database
+     *
+     * It automatically adds some columns into every record. The sorting is 
+     * done by the order of the returned array and starts with 1.
+     * Called internally from save_grading_form() only. Could be private but
+     * keeping protected for unit testing purposes.
+     * 
+     * @param object $raw Raw data returned by mform
+     * @return array Array of objects to be inserted/updated in DB
+     */
+    protected function _cook_form_data(stdClass $raw) {
+
+        $cook = array();
+
+        for ($k = 0; $k < $raw->numofdimensions; $k++) {
+            $cook[$k]                    = new stdClass();
+            $cook[$k]->id                = isset($raw->dimensionid[$k]) ? $raw->dimensionid[$k] : null;
+            $cook[$k]->workshopid        = $this->workshop->id;
+            $cook[$k]->sort              = $k + 1;
+            $cook[$k]->description       = isset($raw->description[$k]) ? $raw->description[$k] : null;
+            $cook[$k]->descriptionformat = FORMAT_HTML;
+            $cook[$k]->grade             = isset($raw->grade[$k]) ? $raw->grade[$k] : null;
+            $cook[$k]->weight            = isset($raw->weight[$k]) ? $raw->weight[$k] : null;
+        }
+        return $cook;
     }
 
 
diff --git a/mod/workshop/grading/gradingform.php b/mod/workshop/grading/gradingform.php
index 3e28b892f4..36dc6613fe 100644
--- a/mod/workshop/grading/gradingform.php
+++ b/mod/workshop/grading/gradingform.php
@@ -43,9 +43,6 @@ class workshop_edit_strategy_form extends moodleform {
     /** strategy logic instance that this class is editor of */ 
     protected $strategy;
 
-    /** number of current dimensions loaded from DB */
-    protected $nocurrentdims;
-
     /**
      * Add the fields that are common for all grading strategies.
      *
@@ -61,8 +58,7 @@ class workshop_edit_strategy_form extends moodleform {
         global $CFG;
 
         $mform = $this->_form;
-        $this->strategy         = $this->_customdata['strategy'];
-        $this->nocurrentdims    = $this->_customdata['nocurrentdims']; 
+        $this->strategy = $this->_customdata['strategy'];
 
         $mform->addElement('hidden', 'strategyname', $this->strategy->name);
 
@@ -91,33 +87,4 @@ class workshop_edit_strategy_form extends moodleform {
         // By default, do nothing.
     }
 
-
-    /**
-     * Set the form data before it is displayed
-     *
-     * Strategy plugins should provide the list of fields to be mapped from 
-     * DB record to the form fields in their map_dimension_fieldnames() method
-     * 
-     * @param object $formdata Should contain the array $formdata->dimensions
-     * @access public
-     * @return void
-     */
-    public function set_data($formdata) {
-
-        if (is_array($formdata->dimensions) && !empty($formdata->dimensions)) {
-            // $formdata->dimensions must be array of dimension records loaded from database
-            $key = 0;
-            $default_values = array();
-            foreach ($formdata->dimensions as $dimension) {
-                foreach ($this->strategy->map_dimension_fieldnames() as $fielddbname => $fieldformname) {
-                    $default_values[$fieldformname . '[' . $key . ']'] = $dimension->$fielddbname;
-                }
-                $key++;
-            }
-            $formdata = (object)((array)$formdata + $default_values);
-        }
-        parent::set_data($formdata);
-    }
-
-
 }
diff --git a/mod/workshop/grading/strategy.php b/mod/workshop/grading/strategy.php
index 35d45435f8..f769ee5734 100644
--- a/mod/workshop/grading/strategy.php
+++ b/mod/workshop/grading/strategy.php
@@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die();
 /**
  * Strategy interface defines all methods that strategy subplugins has to implemens
  */
-interface workshop_strategy_interface {
+interface workshop_strategy {
 
     /**
      * Factory method returning an instance of an assessment form editor class
@@ -38,47 +38,59 @@ interface workshop_strategy_interface {
      * dimensions that will be passed by set_data() must be already known here becase the
      * definition() of the form has to know the number and it is called before set_data().
      *
-     * @param string $actionurl URL of the action handler script
-     * @param int $nocurrentdims Number of current dimensions to be set later by set_data()
+     * @param string $actionurl URL of the action handler script, defaults to auto detect
      * @access public
      * @return object The instance of the assessment form editor class
      */
-    public function get_edit_strategy_form($actionurl, $nocurrentdims=0);
+    public function get_edit_strategy_form($actionurl=null);
 
 
     /**
-     * Load the assessment dimensions from database
+     * Load the assessment dimensions and other grading form elements
      * 
      * Assessment dimension (also know as assessment element) represents one aspect or criterion 
      * to be evaluated. Each dimension consists of a set of form fields. Strategy-specific information
      * are saved in workshop_forms_{strategyname} tables.
+     * The returned object is passed to the mform set_data() method.
      *
      * @access public
-     * @return array Array of database records
+     * @return object Object representing the form fields values
      */
-    public function load_dimensions();
+    public function load_grading_form();
 
 
     /**
-     * Save the assessment dimensions into database
+     * Save the assessment dimensions and other grading form elements
      *
      * Assessment dimension (also know as assessment element) represents one aspect or criterion 
      * to be evaluated. Each dimension consists of a set of form fields. Strategy-specific information
      * are saved in workshop_forms_{strategyname} tables.
      *
      * @access public
+     * @param object $data Raw data as returned by the form editor
      * @return void
      */
-    public function save_dimensions($data);
+    public function save_grading_form(stdClass $data);
+
+
+    /**
+     * Return the number of assessment dimensions defined in the instance of the strategy
+     * 
+     * @return int Zero or positive integer
+     */
+    public function get_number_of_dimensions();
+
 }
 
+
+
 /**
  * Base class for grading strategy logic
  *
  * This base class implements the default behaviour that should be suitable for the most
  * of simple grading strategies.
  */
-class workshop_strategy implements workshop_strategy_interface {
+abstract class workshop_base_strategy implements workshop_strategy {
 
     /** the name of the strategy */
     public $name;
@@ -86,6 +98,9 @@ class workshop_strategy implements workshop_strategy_interface {
     /** the parent workshop instance */
     protected $workshop;
 
+    /** number of dimensions defined in database */
+    protected $nodimensions;
+
     /**
      * Constructor 
      * 
@@ -95,8 +110,9 @@ class workshop_strategy implements workshop_strategy_interface {
      */
     public function __construct($workshop) {
 
-        $this->name     = $workshop->strategy;
-        $this->workshop = $workshop;
+        $this->name         = $workshop->strategy;
+        $this->workshop     = $workshop;
+        $this->nodimensions = null;
     }
 
 
@@ -105,8 +121,10 @@ class workshop_strategy implements workshop_strategy_interface {
      *
      * By default, the class is defined in grading/{strategy}/gradingform.php and is named
      * workshop_edit_{strategy}_strategy_form
+     *
+     * @param $actionurl URL of form handler, defaults to auto detect the current url
      */
-    public function get_edit_strategy_form($actionurl, $nocurrentdims=0) {
+    public function get_edit_strategy_form($actionurl=null) {
         global $CFG;    // needed because the included files use it
     
         $strategyform = dirname(__FILE__) . '/' . $this->name . '/gradingform.php';
@@ -120,8 +138,6 @@ class workshop_strategy implements workshop_strategy_interface {
         $customdata = new stdClass;
         $customdata = array(
                         'strategy'      => $this,
-                        'nocurrentdims' => $nocurrentdims,
-
                         );
         $attributes = array('class' => 'editstrategyform');
 
@@ -131,92 +147,13 @@ class workshop_strategy implements workshop_strategy_interface {
 
 
     /**
-     * Load the assessment dimensions from database
+     * By default, the number of loaded dimensions is set by load_grading_form() 
      * 
-     * This base method just fetches all relevant records from the main strategy form table.
-     *
-     * @uses $DB
      * @access public
      * @return void
      */
-    public function load_dimensions() {
-        global $DB;
-
-        return $DB->get_records('workshop_forms_' . $this->name, array('workshopid' => $this->workshop->id), 'sort');
-    }
-
-
-    /**
-     * Save the assessment dimensions into database
-     *
-     * This base method saves data into the main strategy form table. If the record->id is null or zero,
-     * new record is created. If the record->id is not empty, the existing record is updated. Records with
-     * empty 'description' field are not saved.
-     * The passed data object are the raw data returned by the get_data(). They must be cooked here.
-     *
-     * @uses $DB
-     * @param object $data Raw data returned by the dimension editor form
-     * @access public
-     * @return void
-     */
-    public function save_dimensions($data) {
-        global $DB;
-        
-        if (!isset($data->strategyname) || ($data->strategyname != $this->name)) {
-            // the workshop strategy has changed since the form was opened for editing
-            throw new moodle_exception('strategyhaschanged', 'workshop');
-        }
-
-        $data = $this->cook_form_data($data);
-
-        foreach ($data as $record) {
-            if (empty($record->description)) {
-                continue;
-            }
-            if (empty($record->id)) {
-                // new field
-                $record->id = $DB->insert_record('workshop_forms_' . $this->name, $record);
-            } else {
-                // exiting field
-                $DB->update_record('workshop_forms_' . $this->name, $record);
-            }
-        }
-    }
-
-
-    /**
-     * The default implementation transposes the returned structure
-     *
-     * It automatically adds some columns into every record.
-     * The sorting is done by the order of the returned array and starts with 1.
-     * 
-     * @param object $raw 
-     * @return void
-     */
-    protected function cook_form_data($raw) {
-
-        $cook = array();
-        foreach (array_flip($this->map_dimension_fieldnames()) as $formfield => $dbfield) {
-            for ($k = 0; $k < $raw->numofdimensions; $k++) {
-                $cook[$k]->{$dbfield}   = isset($raw->{$formfield}[$k]) ? $raw->{$formfield}[$k] : null;
-                $cook[$k]->descriptionformat    = FORMAT_HTML;
-                $cook[$k]->sort                 = $k + 1;
-                $cook[$k]->workshopid           = $this->workshop->id;
-            }
-        }
-        return $cook;
-    }
-
-
-    /**
-     * Return the mapping of the db fields to the form fields for every assessment dimension
-     *
-     * This must be public because it is also used by the dimensions editor class.
-     * 
-     * @return array Array ['field_db_name' => 'field_form_name']
-     */
-    public function map_dimension_fieldnames() {
-        return array();
+    public function get_number_of_dimensions() {
+        return $this->nodimensions;
     }
 
 
diff --git a/mod/workshop/submission.php b/mod/workshop/submission.php
index f30e6f5d73..a1c06b40ae 100644
--- a/mod/workshop/submission.php
+++ b/mod/workshop/submission.php
@@ -85,7 +85,7 @@ $mform = new workshop_submission_form(null, array('current' => $submission, 'cm'
                                                  'dataoptions' => $dataoptions, 'attachmentoptions'=>$attachmentoptions));
 
 if ($mform->is_cancelled()){
-    die();
+    die(); // todo
     if ($id){
         redirect("view.php?id=$cm->id");
     } else {