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.

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

removal of usage of global variables #87

Closed
wants to merge 3 commits into from

Conversation

dionysius
Copy link
Member

@dionysius dionysius commented Jan 27, 2019

From MDL-62822

classes/question/bank/question_bank_filter.php
Any reason for using $_POST variables directly on set_defaults()? This is not recommended at all.

classes/question/bank/studentquiz_bank_view.php
Another usages of $_GET and $_POST directly, pretty sure this will require some refactoring.

  • removed all usage of $_ params where needed
  • during the process detected, that most affected code is not necessary at all, so removed
  • category is found by default category of the current context
  • From the view.php: if cmid is passed via 'id', 'cmid' is getting set - this place is probably best for that need

This PR needs most likely also a good manual test of the filter! Please have a look into that too.

@dionysius dionysius changed the title Mdl 62822 nointernalaccesstopost removal of usage of global variables Jan 27, 2019
The step updateSubmission() in https://github.com/frankkoch/moodle-mod_studentquiz/pull/87/files#diff-6351ea0fef416e5c6111a6f05462ef18L127 was unneccessary, hence moodleform is capable to process the submission by itself. But it
wants a valid session, which we have to fake in the unittests.
@dionysius
Copy link
Member Author

Update: fixed unit-test for filtering questions

@muqiuq muqiuq closed this Feb 3, 2019
@dionysius dionysius deleted the MDL-62822-nointernalaccesstopost branch March 10, 2019 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants