Allow a staff member can create an attempt for a student who does not have one in-progress

[HuongNV13]

Hi @timhunt,

This is Lam, the new member of Green Team. He has done the development for this ticket.

Please help me to review it

The code was tested done by our tester

Thanks,

[timhunt]

Sorry, lots of comments.

1. Thinking about how you approched this: do we really need a custom modal just to do what is essentially a modal_save_cancel. (Where you can customise the label text.) I think there would be less code to maintian if you used the standard widget.

2. Please use promises, not nested callbacks (e.g. for handlnig the result of the Ajax call).

3. Mostly you should not need explicit error-hadlind code, becuase I think that Moodle default error handling for Ajax requests is fine.

For some good example code to copy, have a look at the deleteItem method in blocks/dashboardboxes/amd/src/customise.js,or search the code for other uses of ModalFactory.types.SAVE_CANCEL.

4. Again, you don't need your own big try/catch block in quiz_answersheets_external::create_attempt. The Moodle web service framework handles exeptions thrown, and reports them properly. (For exampel, your code throws away the stack trace of the exception.)

5. Also, rather than returning special values in the external funtion, it would probably be clearer to throw the error conditions as exceptions.

6. The way you have implemeted col_create_attempt, the full text of the 'create_attempt_modal_description' and other variables on the in every . This is very inefficient. Really, the data attributes on each row should only have the data that is different.

7. Don't manually parse $CFG->showuseridentity. Use get_extra_user_fields. (And, preferably call it once, not in the loop over table-rows.)

8. Do not define a pre-built service just for one web service function. Except in very special situations, you never want to define a service.

9. Don't hard-code $fullname = $row->firstname . ' ' . $row->lastname;. Use fullname($row).

Minor points:

10. calling the main JavaScript object 'app' is not a common idiom. I thin most OU code calls it 't' (sort-of short for 'this'.)

11. I know it was necessary to duplciate standard quiz code into external because MDL-66633 has only been integrated into Moodle 3.8, but please add a comment in the code here, to say that this code can be simplified when we move to Moodle 3.8.

12. Concatenating strings like get_string('create_attempt_modal_description', 'quiz_answersheets', $fullname . ' '. $useridentifytext) is normally wrong. The langauge string should probably be 'Are you sure you want to create a quiz attempt for {$a->name} ({$a->extrainfo})?'. (Although the case when $a->extrainfo is empty and you don't want the brackts makes the rule un-clear here.)

13. Worth adding a Behat test that the Cancel button in the dialogue really Cancels.

14. Non-editing teacher should not have quiz/answersheets:createattempt by default.

Requirements clarification:

15. We almost certainly should not do the standard permissions check ($accessmanager->prevent_new_attempt($numattempts, $lastattempt);) for creating attempt. For example staff may need to use this tool to create and print a quiz attempt for one student before the quiz opens to other students. (And later they may need to input responses received through the mail after the official close date.)

16. Becuase of what this report is for, the default option should probably be "Attempts from": "enrolled users who have, or have not, attempted the quiz"

Random comment:

17. It seems really odd that we have to implement get_user_extras_text() function. Surely something like that should exist in Moodle, but I can't find it.

[lamtranb]

Hi @timhunt, The code was reviewed by Mr Thinh Pham, and it was tested by our tester. Please help me to review it. Thanks.

[timhunt]

I can see you have fixed a lot of the more serious problems form the previous review, which is good.

A) However, there is one serious problem that remains. Each page in moodle is supposed to require O(1) database queries. Your code is O(n) in the number of table rows. In fact, if I counted right, it is 7*n. Some of our courses have hundreds of students. This is a problem.

B) A few of the minor comments from before have not been addressed. E.g. 6., 11.

[timhunt]

Note: it should be the case that adding &perfinfo=yesplease to the URL of any page puts useful performance info in the page footer, but this seems to be broken in the new OU theme.

[timhunt]

My suggestion for how to compute whether we should show the 'Create Attempt' button as part of the main DB query is https://github.com/moodleou/moodle-quiz_answersheets/commit/wip-start-attempt.

Obviously, there is an argument against this approach: it is duplicating logic from standard Moodle into our plugin, and doing it in scary SQL. However, I think that is unavoidable if we want reasonable performace, which we do. Also, the logic has not changed in core since overrides were added in 2010 (MDL-16478) so the maintenance risk of this duplicated logic is low.

Also, be warned, I have not tested this properly.

Hope that is helpful.

[lamtranb]

Hi @timhunt Thank you very much for your review and your suggestion. Sorry for my mistake. I have tested your suggestion and it's working perfectly. I have updated my code as below:

• Comment 6: I have created a private array variable and check whenever it has userid as key, then the code will get message from its value. If not, then push data into array.
• Comment 11: I have duplicated the code for that function with a @todo comment.

Please help me to review again and hope you happy with these changes. Many thanks.

[timhunt]

Great! Thank you for persisting with this. Code merged into master now.