StudentQuiz: Improve change sqq visibility code and fix change queston state is out-of-sync

[vuvanhieu143]

Hi @timhunt This issue is ready for peer review.

• I have removed the duplicated code and moved the change state logic change_question_state_api since it is just one service, I have renamed and moved it to local/external.
• I have renamed change_state_visibility to change_sq_question_visibility and centralized all the logic in studentquiz_question related to visibility in this function. Let us keep this function for other actions.
• Update the related state after we change the state in the database for the studentquiz_question object so it is not out o sync.
• There is a minor change in the tests/behat/pagination.feature. The test is not very stable, sometimes the test is failed, to make it more stable, I want to make sure the page loaded entirely before we click on the link I hope the code is more cleared and makes more sense now

[timhunt]

Thanks Hieu, the generally looks very good, I think it is doing the right thing, but I only think that after a lot of hard work to understand what the code is doing. So, most of the comments below are about altering the code to make it easier to understand:

1. For web service classes, the recommendation is to have the class name be the same as the service method name. Also, 'external' is an allowed second level namespace., so the class for service mod_studentquiz_set_state should be `mod_studentquiz\external\set_state (https://github.com/moodle/moodle/commit/1a1939ac29f26ba2972f47fcab5f0d7b0cbc43dc#diff-5b850491f6e95b557e525fc1dfc9591e3b7284b669edd331b746c96a66d17dc6 is a core change that gets it approximagely right)
2. Note, mod_studentquiz_set_state is not really right. It should be mod_studentquiz_update_question_state. Do we change it now? (See https://docs.moodle.org/dev/Web_service_API_functions#Naming_convention for the rules)
3. When moving an existing file, do not change the copyright, so I thnk it should stay as @author Huong Nguyen huongnv13@gmail.com / @copyright 2019 HSR (http://www.hsr.ch)
4. Given that this is a method on the studentquiz_question class, I think change_visibility would be a better name than change_sq_question_visibility. What do you think? Is it worth changing?
5. For that method, the argument names $type, $value do not really tell me what is gonig on. ... OK, having read some code, it seems that what is going on is weird. https://github.com/studentquiz/moodle-mod_studentquiz/blob/d3b0c72193e81311f1a62d62af8c0507248f53ce/classes/local/external/change_question_state_api.php#L73 Is now the time to sort this out, or do we just improve the comments? Given the weirdness, perhaps just leave the method name as change_state_visibility? (Yes, renaming the method causes the patch to be bigger than reqired.)
6. (Generally, rather than one method with a $type parameter to do one of three different things, it is rpobalby better to have three separate methods which each do one thing, but as I say, now might not be the time to fix this, unless you want to.)
7. I do not believe that "And I wait until the page is ready" has any effect in Behat. Moodle Behat does that automatically after every step. If it is not working reliably, check that all the JS code is correctly using M.util.js_pending / js_complete correctly. Probably best to do that in a separte issue.
8. studentquiz_question_test is very obfuscated. I cannot easily see what expected behaviour is being testd for. Did you try writing 4 separate test methods, each with a good name, to clearly test one thing. (Ovivously, add some helper methods to do the necssary setup, to avoid duplicate code.)

[vuvanhieu143]

Hi @timhunt , This is ready for peer-review again.

• I have moved the update_question_state to class/external.
• Revert the copy right(sorry about that)
• Separate the main function into three functions: change_delete_state, change_hidden_status, change_pin_status since DELETE and HIDE is not true state.
• Keep the name change_state_visibility.
• After reading the weirdness you point out, I found out the mod_studentquiz_state_notify is duplicated, so I remove it and call mod_studentquiz_event_notification_question directly.
• I revert the behat change since the test is not failing anymore.
• I have separate the unit test into three tests, I think this should cover enough cases.