diff --git a/actions/allocate.php b/actions/allocate.php index d8eefa3..b3c3d20 100644 --- a/actions/allocate.php +++ b/actions/allocate.php @@ -63,8 +63,9 @@ // If a session variable holding perpage preference for the specific coursework is not set, set default value (10). if (!(isset($SESSION->allocate_perpage[$coursemoduleid]))) { - $SESSION->allocate_perpage[$coursemoduleid] = optional_param('per_page', $CFG->coursework_per_page, PARAM_INT); - $perpage = $SESSION->allocate_perpage[$coursemoduleid]; + $perpage = optional_param('per_page', 0, PARAM_INT); + $perpage = $perpage ?: (get_config('coursework', 'coursework_per_page') ?? 10); + $SESSION->allocate_perpage[$coursemoduleid] = $perpage; } else { $perpage = optional_param('per_page', $SESSION->allocate_perpage[$coursemoduleid], PARAM_INT); $SESSION->allocate_perpage[$coursemoduleid] = $perpage; @@ -75,8 +76,9 @@ $sorthow = optional_param('sorthow', '', PARAM_ALPHA); $options = compact('sortby', 'sorthow', 'perpage', 'page'); -// $_POST['allocatables'] comes as array of arrays which is not supported by optional_param_array, however we clean this later in process_data() function -$formdataarray = isset($_POST['allocatables']) ? $_POST['allocatables'] : []; +// Variable $_POST['allocatables'] comes as array of arrays which is not supported by optional_param_array. +// However, we clean this later in process_data() function. +$dirtyformdata = isset($_POST['allocatables']) ? $_POST['allocatables'] : []; require_login($course, true, $coursemodule); @@ -149,7 +151,7 @@ // Did we just get the form submitted to us? if ($formsavebutton) { $processor = new \mod_coursework\allocation\table\processor($coursework); - $processor->process_data($formdataarray); + $processor->process_data($dirtyformdata); $allocationsmanager->auto_generate_sample_set(); } diff --git a/classes/allocation/table/cell/data.php b/classes/allocation/table/cell/data.php index b9f86e2..3d99cd3 100644 --- a/classes/allocation/table/cell/data.php +++ b/classes/allocation/table/cell/data.php @@ -46,6 +46,26 @@ class data { */ private $stage; + /** + * Key in form data for allocation ID. + */ + const ALLOCATION_ID_KEY = 'allocation_id'; + + /** + * Key in form data for assessor ID. + */ + const ASSESSOR_ID_KEY = 'assessor_id'; + + /** + * Key in form data for "in set" status. + */ + const MODERATION_SET_KEY = 'in_set'; + + /** + * Key in form data for "pinned" status. + */ + const PINNED_KEY = 'pinned'; + /** * @param stage_base $stage * @param array $data @@ -60,10 +80,8 @@ public function __construct($stage, $data = []) { * @return mixed */ protected function preprocess_data() { - - $key = $this->assessor_id_key_name(); - if (array_key_exists($key, $this->data) && !empty($this->data[$key])) { - $assessor = user::find($this->data[$key]); + if (array_key_exists(self::ASSESSOR_ID_KEY, $this->data) && !empty($this->data[self::ASSESSOR_ID_KEY])) { + $assessor = user::find($this->data[self::ASSESSOR_ID_KEY]); if ($assessor && $this->stage->user_is_assessor($assessor)) { $this->assessor = $assessor; } @@ -88,36 +106,14 @@ public function has_assessor() { * @return bool */ public function allocatable_should_be_in_sampling(): bool { - $key = $this->moderation_set_key(); - return array_key_exists($key, $this->data) && $this->data[$key]; + return array_key_exists(self::MODERATION_SET_KEY, $this->data) + && $this->data[self::MODERATION_SET_KEY]; } /** * @return bool */ public function is_pinned(): bool { - $key = $this->pinned_key(); - return array_key_exists($key, $this->data) && $this->data[$key]; - } - - /** - * @return string - */ - private function assessor_id_key_name() { - return 'assessor_id'; - } - - /** - * @return string - */ - private function moderation_set_key() { - return 'in_set'; - } - - /** - * @return string - */ - private function pinned_key() { - return 'pinned'; + return array_key_exists(self::PINNED_KEY, $this->data) && $this->data[self::PINNED_KEY]; } } diff --git a/classes/allocation/table/processor.php b/classes/allocation/table/processor.php index d2811ff..40c51e3 100644 --- a/classes/allocation/table/processor.php +++ b/classes/allocation/table/processor.php @@ -50,10 +50,11 @@ public function __construct($coursework) { } /** - * @param array $tabledata + * Process form data received from /actions/allocate.php form. + * @param array $dirtyformdata */ - public function process_data($tabledata = []) { - $cleandata = $this->clean_data($tabledata); + public function process_data($dirtyformdata = []) { + $cleandata = $this->clean_data($dirtyformdata); $allocatables = $this->coursework->get_allocatables(); foreach ($allocatables as $allocatable) { @@ -79,57 +80,65 @@ private function get_row($allocatable) { /** * Sanitises the data, mostly making sure that we have ony valid student ids and valid stage identifiers. - * The stages will deal with sanitising the data for each cell. + * The stages will further deal with sanitising the data for each cell. * - * @param array $rawdata + * @param array $dirtyformdata * @return array */ - private function clean_data($rawdata) { - - // Data looks like this: - // $example_data = array( - // 4543 => array( // Student id - // 'assessor_1' => array( - // 'allocation_id' => 43, - // 'assessor_id' => 232, - // ), - // 'moderator_1' => array( - // 'allocation_id' => 46, - // 'assessor_id' => 235, - // 'in_set' => 1, - // ) - // ) - // ); + private function clean_data(array $dirtyformdata): array { + // Raw data looks like this - 4543 is student ID. + // $exampledata = [ + // 4543 => [ + // 'assessor_1' => ['allocation_id' => 43, 'assessor_id' => 232], + // 'moderator_1' => ['allocation_id' => 46, 'assessor_id' => 235, 'in_set' => 1], + // ], + // ]; + $allowedrowkeys = [ + \mod_coursework\allocation\table\cell\data::ALLOCATION_ID_KEY, + \mod_coursework\allocation\table\cell\data::ASSESSOR_ID_KEY, + \mod_coursework\allocation\table\cell\data::MODERATION_SET_KEY, + \mod_coursework\allocation\table\cell\data::PINNED_KEY, + ]; $cleandata = []; - foreach ($rawdata as $allocatableid => $datarrays) { + foreach ($dirtyformdata as $allocatableid => $dirtyrowsforuser) { - if (!$this->allocatable_id_is_valid($allocatableid)) { // Should be the id of a student. + // Should be the id of a student. + if (!$this->allocatable_id_is_valid(clean_param($allocatableid, PARAM_INT))) { continue; } - $cleandata[$allocatableid] = []; - - foreach ($this->coursework->marking_stages() as $stage) { + // Variable $rawdataforuser is expected to be an array of arrays. + $validstageindentifiers = array_keys($this->coursework->marking_stages()); + foreach ($dirtyrowsforuser as $stageidentifier => $dirtyrowforuser) { + if (!isset($validstageindentifiers, $stageidentifier)) { + throw new \invalid_parameter_exception("Invalid stage identifier $stageidentifier"); + } - if (array_key_exists($stage->identifier(), $datarrays)) { - $stagedata = $datarrays[$stage->identifier()]; - $cleandata[$allocatableid][$stage->identifier()] = $stagedata; + // Finally, check the keys and values in the cleaned row individually. + $keys = array_keys($dirtyrowforuser); + foreach ($keys as $key) { + if (!in_array($key, $allowedrowkeys)) { + throw new \invalid_parameter_exception("Invalid key $key"); + } + if ($dirtyrowforuser[$key] && !filter_var($dirtyrowforuser[$key], FILTER_SANITIZE_NUMBER_INT)) { + throw new \invalid_parameter_exception( + "Invalid value type for key '$key' - expected integer" + ); + } } + $cleandata[$allocatableid][$stageidentifier] = clean_param_array($dirtyrowforuser, PARAM_INT); } - /* if (array_key_exists('moderator', $datarrays)) { - $moderator_data = $datarrays['moderator']; - $clean_data[$allocatable_id]['moderator'] = $moderator_data; - }*/ } return $cleandata; } /** + * Is this a valid allocatable ID? * @param int $studentid * @return bool */ - private function allocatable_id_is_valid($studentid) { + private function allocatable_id_is_valid(int $studentid): bool { $allocatable = $this->get_allocatable_from_id($studentid); return $allocatable && $allocatable->is_valid_for_course($this->coursework->get_course()); } diff --git a/classes/controllers/submissions_controller.php b/classes/controllers/submissions_controller.php index 0fef2c7..4ad3828 100644 --- a/classes/controllers/submissions_controller.php +++ b/classes/controllers/submissions_controller.php @@ -387,13 +387,14 @@ private function terms_were_agreed_to() { } /** + * Is the coursework open? * @param coursework $coursework * @throws \coding_exception * @throws access_denied */ protected function check_coursework_is_open($coursework) { if (!$coursework->start_date_has_passed()) { - throw new access_denied($coursework, get_string('notstartedyet', 'mod_coursework', userdate($coursework->startdate))); + throw new \Exception(get_string('notstartedyet', 'mod_coursework', userdate($coursework->startdate))); } } diff --git a/view.php b/view.php index 851cd39..197bc55 100644 --- a/view.php +++ b/view.php @@ -93,10 +93,12 @@ $SESSION->page[$coursemoduleid] = $page; } -// If a session variable holding perpage preference for the specific coursework is not set, set default value (grab default value from global setting). +// If a session variable holding perpage preference for the specific coursework is not set, set default value. +// (Grab default value from plugin setting). if (!(isset($SESSION->perpage[$coursemoduleid]))) { - $SESSION->perpage[$coursemoduleid] = optional_param('per_page', $CFG->coursework_per_page, PARAM_INT); - $perpage = $SESSION->perpage[$coursemoduleid]; + $perpage = optional_param('per_page', 0, PARAM_INT); + $perpage = $perpage ?: (get_config('coursework', 'coursework_per_page') ?? 10); + $SESSION->perpage[$coursemoduleid] = $perpage; } else { $perpage = optional_param('per_page', $SESSION->perpage[$coursemoduleid], PARAM_INT); $SESSION->perpage[$coursemoduleid] = $perpage;