Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StudentQuiz: Must not refer to GET or POST in code #667798 #454

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

StudentQuiz: Must not refer to GET or POST in code #667798 #454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 67 additions & 2 deletions classes/question/bank/studentquiz_bank_view.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,71 @@ private function set_filter_form_fields($anonymize = true) {
* but the moodle filter form can only process POST, so we need to copy them there.
*/
private function set_filter_post_data() {
$_POST = $_GET;
$params = [];
$paramdata = ['mform_isexpanded_id_filtertab' => optional_param('mform_isexpanded_id_filtertab', null, PARAM_TEXT),
'cmid' => optional_param('cmid', null, PARAM_TEXT),
'id' => optional_param('id', null, PARAM_TEXT),
'qperpage' => optional_param('qperpage', null, PARAM_TEXT),
'sesskey' => optional_param('sesskey', null, PARAM_TEXT),
'_qf__mod_studentquiz_question_bank_filter_form' =>
optional_param('_qf__mod_studentquiz_question_bank_filter_form', null, PARAM_TEXT),
'mform_showmore_id_filtertab' => optional_param('mform_showmore_id_filtertab', null, PARAM_TEXT),
'onlynew' => optional_param('onlynew', null, PARAM_TEXT),
'only_new_state' => optional_param('only_new_state', null, PARAM_TEXT),
'only_approved_state' => optional_param('only_approved_state', null, PARAM_TEXT),
'only_disapproved_state' => optional_param('only_disapproved_state', null, PARAM_TEXT),
'only_changed_state' => optional_param('only_changed_state', null, PARAM_TEXT),
'only_reviewable_state' => optional_param('only_reviewable_state', null, PARAM_TEXT),
'onlygood' => optional_param('onlygood', null, PARAM_TEXT),
'onlymine' => optional_param('onlymine', null, PARAM_TEXT),
'onlydifficultforme' => optional_param('onlydifficultforme', null, PARAM_TEXT),
'onlydifficult' => optional_param('onlydifficult', null, PARAM_TEXT),
'tagarray_op' => optional_param('tagarray_op', null, PARAM_TEXT),
'tagarray' => optional_param('tagarray', null, PARAM_TEXT),
'state' => optional_param('state', null, PARAM_TEXT),
'publiccomment_op' => optional_param('publiccomment_op', null, PARAM_TEXT),
'publiccomment' => optional_param('publiccomment', null, PARAM_TEXT),
'name_op' => optional_param('name_op', null, PARAM_TEXT),
'name' => optional_param('name', null, PARAM_TEXT),
'questiontext_op' => optional_param('questiontext_op', null, PARAM_TEXT),
'questiontext' => optional_param('questiontext', null, PARAM_TEXT),
'firstname_op' => optional_param('firstname_op', null, PARAM_TEXT),
'firstname' => optional_param('firstname', null, PARAM_TEXT),
'lastname_op' => optional_param('lastname_op', null, PARAM_TEXT),
'lastname' => optional_param('lastname', null, PARAM_TEXT),
'timecreated_sdt' => optional_param_array('timecreated_sdt', null, PARAM_TEXT),
'timecreated_edt' => optional_param_array('timecreated_edt', null, PARAM_TEXT),
'lastanswercorrect' => optional_param('lastanswercorrect', null, PARAM_TEXT),
'myrate_op' => optional_param('myrate_op', null, PARAM_TEXT),
'myrate' => optional_param('myrate', null, PARAM_TEXT),
'rate' => optional_param('rate', null, PARAM_TEXT),
'myattempts_op' => optional_param('myattempts_op', null, PARAM_TEXT),
'myattempts' => optional_param('myattempts', null, PARAM_TEXT),
'mydifficulty_op' => optional_param('mydifficulty_op', null, PARAM_TEXT),
'mydifficulty' => optional_param('mydifficulty', null, PARAM_TEXT),
'difficultylevel_op' => optional_param('difficultylevel_op', null, PARAM_TEXT),
'difficultylevel' => optional_param('difficultylevel', null, PARAM_TEXT),
'rate_op' => optional_param('rate_op', null, PARAM_TEXT),
'submitbutton' => optional_param('submitbutton', null, PARAM_TEXT),
'cat' => optional_param('cat', null, PARAM_TEXT),
'startquiz' => optional_param('startquiz', null, PARAM_TEXT),
'useFilter' => optional_param('useFilter', false, PARAM_BOOL),
];

// Remove redundant data.
foreach ($paramdata as $key => $param) {
if (!is_null($param)) {
$params[$key] = $param;
}
}

if (optional_param('submitbutton', null, PARAM_TEXT) === 'Filter'
&& !optional_param('useFilter', false, PARAM_BOOL)) {
$params['useFilter'] = true;
$params = \mod_studentquiz\utils::flat_url_data($params);
$url = new \moodle_url('/mod/studentquiz/view.php', $params);
redirect($url);
}
}

/**
Expand All @@ -566,7 +630,8 @@ private function initialize_filter_form($pageurl) {
$this->filterform = new \mod_studentquiz_question_bank_filter_form(
$this->fields,
$pageurl->out(false),
array_merge(['cmid' => $this->cm->id], $this->pagevars)
array_merge(['cmid' => $this->cm->id], $this->pagevars),
'get'
);
}

Expand Down
46 changes: 41 additions & 5 deletions classes/utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,19 +406,24 @@ public static function moodle_version_is(string $operator, string $version): boo
/**
* We hide 'All participants' option in group mode. It doesn't make sense to display question of all groups together,
* and it makes confusing in reports. If the group = 0, NULL or an invalid group,
* we force to chose first available group by default.
* we force to choose first available group by default.
*
* @param stdClass $cm Course module class.
* @param \stdClass $cm Course module class.
* @param moodle_url $url The report url.
*/
public static function set_default_group($cm) {
public static function set_default_group(\stdClass $cm, moodle_url $url): void {
global $USER;

$allowedgroups = groups_get_activity_allowed_groups($cm, $USER->id);
if ($allowedgroups && !groups_get_activity_group($cm, true, $allowedgroups)) {
// Although the UI show that the first group is selected, the param 'group' is not set,
// so the groups_get_activity_group() will return wrong value. We have to set it in $_GET to prevent the
// problem when user go to the student quiz in the first time.
$_GET['group'] = reset($allowedgroups)->id;
// problem when usergroups_get_activity_group go to the student quiz in the first time.
if (!optional_param('group', -1, PARAM_INT)) {
$group = reset($allowedgroups)->id;
$url->param('group', $group);
redirect($url->out());
}
}
}

Expand Down Expand Up @@ -804,4 +809,35 @@ public static function save_rate(\stdClass $data): void {
$DB->update_record('studentquiz_rate', $data);
}
}

/**
* Flat URL data before set to url.
* Sometimes, users may need to filter questions based on their creation date.
* The URL parameters, stored in an array as timecreated_sdt and timecreated_edt,
* contain values for year, month, day, and enabled.
* Before setting these values as URL parameters, we need to flatten it.
* e.g:
* - With these data input:
* ['timecreated_sdt' => ['day' = '11', 'month' => '11', 'year' => '2023', 'enabled' => '1]]
* - The output will be:
* ['timecreated_sdt[day]' => '11', 'timecreated_sdt[month]' => '11', timecreated_sdt[year]' => '11', 'enabled' => '1'];
*
* @param array $data URL data need to convert.
* @return array Flat parameters.
*/
public static function flat_url_data(array $data): array {
$params = [];
foreach ($data as $key => $value) {
if ($key === 'timecreated_sdt' || $key === 'timecreated_edt') {
foreach ($value as $index => $time) {
$paramname = $key . '[' . $index . ']';
$params[$paramname] = $time;
}
} else {
$params[$key] = $value;
}
}

return $params;
}
}
27 changes: 24 additions & 3 deletions reportlib.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,9 @@ public function get_useractivitystats() {
/**
* Constructor assuming we already have the necessary data loaded.
* @param int $cmid course_module id
* @param string $type Report type view, rank or stat. Default: view.
*/
public function __construct($cmid) {
public function __construct(int $cmid, string $type = 'view') {
global $DB, $USER;
if (!$this->cm = get_coursemodule_from_id('studentquiz', $cmid)) {
throw new mod_studentquiz_view_exception($this, 'invalidcoursemodule');
Expand All @@ -189,7 +190,19 @@ public function __construct($cmid) {
$this->userid = $USER->id;
$this->availablequestions = mod_studentquiz_count_questions($cmid);

\mod_studentquiz\utils::set_default_group($this->cm);
switch ($type) {
case 'view':
$url = $this->get_view_url();
break;
case 'stat':
$url = $this->get_stat_url();
break;
default:
$url = $this->get_rank_url();
break;
}

\mod_studentquiz\utils::set_default_group($this->cm, $url);
$this->groupid = groups_get_activity_group($this->cm, true);

// Check to see if any roles setup has been changed since we last synced the capabilities.
Expand Down Expand Up @@ -225,7 +238,15 @@ public function get_rank_url() {
* @return array
*/
public function get_urlview_data() {
return array('cmid' => $this->cm->id);
return ['cmid' => $this->cm->id, 'group' => optional_param('group', -1, PARAM_INT)];
}

/**
* Get quiz report url
* @return moodle_url
*/
public function get_view_url() {
return new moodle_url('/mod/studentquiz/view.php', $this->get_urlview_data());
}

/**
Expand Down
2 changes: 1 addition & 1 deletion reportrank.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
$cmid = required_param('cmid', PARAM_INT);
}

$report = new mod_studentquiz_report($cmid);
$report = new mod_studentquiz_report($cmid, 'rank');
$cm = $report->get_coursemodule();
require_login($report->get_course(), false, $cm);
$context = $report->get_context();
Expand Down
2 changes: 1 addition & 1 deletion reportstat.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
$cmid = required_param('cmid', PARAM_INT);
}

$report = new mod_studentquiz_report($cmid);
$report = new mod_studentquiz_report($cmid, 'stat');
$cm = $report->get_coursemodule();

require_login($report->get_course(), false, $cm);
Expand Down
1 change: 1 addition & 0 deletions tests/bank_view_test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ public function test_questionbank_filter_question_name() {

$this->set_filter(QUESTION_NAME_FILTER, 'Question 1');
$this->set_filter(QUESTION_NAME_OP_FILTER, '0');
$this->set_filter('useFilter', true);

// Hard coded.
$questionbank = $this->run_questionbank();
Expand Down
41 changes: 41 additions & 0 deletions tests/utils_test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,4 +172,45 @@ public function test_ensure_question_version_status_is_correct(string $status, b
$this->assertEquals(question_version_status::QUESTION_STATUS_READY, $newquestionversion->status);
}

/**
* Test flat_url_data function.
*
* @covers \mod_studentquiz\utils::flat_url_data
*/
public function test_flat_url_data(): void {
$this->resetAfterTest();
$params = [
'cmid' => '81',
'cat' => '533,140',
'qperpage' => '20',
'timecreated_sdt' => [
'day' => '11',
'month' => '11',
'year' => '2022',
'enabled' => '1',
],
'timecreated_edt' => [
'day' => '11',
'month' => '11',
'year' => '2023',
'enabled' => '1',
]
];

$expectedparams = [
'cmid' => '81',
'cat' => '533,140',
'qperpage' => '20',
'timecreated_sdt[day]' => '11',
'timecreated_sdt[month]' => '11',
'timecreated_sdt[year]' => '2022',
'timecreated_sdt[enabled]' => '1',
'timecreated_edt[day]' => '11',
'timecreated_edt[month]' => '11',
'timecreated_edt[year]' => '2023',
'timecreated_edt[enabled]' => '1',
];

$this->assertEquals($expectedparams, utils::flat_url_data($params));
}
}
2 changes: 2 additions & 0 deletions tests/viewlib_test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,11 @@ protected function setUp(): void {

$this->cm = get_coursemodule_from_id('studentquiz', $studentquiz->cmid);
$context = \context_module::instance($this->cm->id);
$category = question_get_default_category($context->id);

// Some internal moodle functions (e.g. question_edit_setup()) require the cmid to be found in $_xxx['cmid'].
$_GET['cmid'] = $this->cm->id;
$_GET['cat'] = $category->id . ',' . $context->id;

// Satisfy codechecker: $course $cm $studentquiz $userid.
$report = new \mod_studentquiz_report($this->cm->id);
Expand Down
12 changes: 6 additions & 6 deletions view.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,18 @@
$cmid = required_param('id', PARAM_INT);
// Some internal moodle functions (e.g. question_edit_setup()) require the cmid to be found in $_xxx['cmid'],
// but moodle allows to view a mod page with parameter id in place of cmid.
$_GET['cmid'] = $cmid;
$url = new moodle_url('/mod/studentquiz/view.php', ['cmid' => $cmid, 'id' => $cmid]);
redirect($url);
}

// TODO: make course-, context- and login-check in a better starting class (not magically hidden in "report").
// And when doing that, offer course, context and studentquiz object over it, which all following actions can use.
$report = new mod_studentquiz_report($cmid);
$report = new mod_studentquiz_report($cmid, 'view');
require_login($report->get_course(), false, $report->get_coursemodule());

$course = $report->get_course();
$context = $report->get_context();
$cm = $report->get_coursemodule();

$studentquiz = mod_studentquiz_load_studentquiz($cmid, $context->id);

// If for some weired reason a studentquiz is not aggregated yet, now would be a moment to do so.
Expand All @@ -56,14 +56,14 @@
// Load view.
$view = new mod_studentquiz_view($course, $context, $cm, $studentquiz, $USER->id, $report);
$baseurl = $view->get_questionbank()->base_url();

// Redirect if we have received valid data.
// Usually we should use submitted_data(), but since we have two forms merged and exchanging their values
// using GET params, we can't use that.
if (!empty($_GET)) {

if (!empty($_REQUEST)) {
if (optional_param('startquiz', null, PARAM_BOOL)) {
require_sesskey();
if ($ids = mod_studentquiz_helper_get_ids_by_raw_submit(fix_utf8($_GET))) {
if ($ids = mod_studentquiz_helper_get_ids_by_raw_submit(fix_utf8($_REQUEST))) {
if ($attempt = mod_studentquiz_generate_attempt($ids, $studentquiz, $USER->id)) {
$questionusage = question_engine::load_questions_usage_by_activity($attempt->questionusageid);
$baseurl->remove_params('startquiz');
Expand Down
20 changes: 10 additions & 10 deletions viewlib.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,21 +123,21 @@ public function __construct($course, $context, $cm, $studentquiz, $userid, $repo
* Loads the question custom bank view.
*/
private function load_questionbank() {
$_POST['cat'] = $this->get_category_id() . ',' . $this->get_context_id();
$params = $_GET;
$params = $_REQUEST;
if (!optional_param('cat', null, PARAM_SEQUENCE)) {
$params['cat'] = $this->get_category_id() . ',' . $this->get_context_id();
$params = \mod_studentquiz\utils::flat_url_data($params);
$url = new \moodle_url('/mod/studentquiz/view.php', $params);
redirect($url);
}
// Get edit question link setup.
list($thispageurl, $contexts, $cmid, $cm, $module, $pagevars)
[$thispageurl, $contexts, $cmid, $cm, $module, $pagevars]
= question_edit_setup('questions', '/mod/studentquiz/view.php', true);
$pagevars['qperpage'] = optional_param('qperpage', \mod_studentquiz\utils::DEFAULT_QUESTIONS_PER_PAGE, PARAM_INT);
$pagevars['showall'] = optional_param('showall', false, PARAM_BOOL);
$pagevars['cat'] = $this->get_category_id() . ',' . $this->get_context_id();
$this->pageurl = new moodle_url($thispageurl);
foreach ($params as $key => $value) {
if ($key == 'timecreated_sdt' || $key == 'timecreated_edt') {
$value = http_build_query($value);
}
$thispageurl->param($key, $value);
}
$urlparams = \mod_studentquiz\utils::flat_url_data($params);
$this->pageurl = new moodle_url($thispageurl, $urlparams);
// Trigger notification if user got returned from the question edit form.
// TODO: Shouldn't this be somewhere outside of load_questionbank(), as this is clearly not relevant for showing the
// question bank?
Expand Down