Screen for a staff member to view an in-progress attempt that print well

[HuongNV13]

Hi @timhunt ,

Please help me to review it

The code was tested done by our tester

Thanks,

[timhunt]

Thanks Huong this is looking generally very good, but there are some things that are not right yet. (Also, I have commented on some minor coding style things I noticed.)

1. I think the logic for $displayoptions in render_question_attempt_content is wrong. We definitely don't want edit links here, so I think you should just be calling $attemptobj->get_display_options. Looking at this commit makes me realise that this is a detail I need to discuss with Chris and Paul Johnson, but the changes you make (e.g. forcing all the review options on) is definitely wrong. If we do anything with these review sheets, it will be to print them to send back to the student. So, for now, just call $attemptobj->get_display_options and use the result (you can keep the bit about hiding flags and manual comment link.) If that is not right, we will fix this bit later.
2. I found where you copied the if ($slot != $originalslot) { code from, and the person who wrote that core quiz code (== me) should have written comments to explain what it was for. However, it is not required in this report. You can delete that if statement.
3. Do not call $quba = question_engine::load_questions_usage_by_activity($attemptobj->get_uniqueid()); inside the loop over slots. That is a performance disaster! Also, you don't need to do it. The usage has already been loaded inside $attemptobj. Use $attemptobj->get_question_attempt($slot)->render_question(). (Just check what $quba->render_question and do the same).
4. It is really annoying that it is necessary to duplicate code from review.php into prepare_summary_attempt_information, but I cannot see any option. However, please add a comment making it clear that this is copied code, and where it was copied from. (That helps maintenance.)
5. Modern Moodle coding style does not use locallib.php. You should use a class in the classes folder, e.g. quiz_answersheets_utils::prepare_summary_attempt_information()
6. Not very important, but I think current good practice is to put the renderer class in classes/output/renderer.php.
7. I think that general convention for Moodle scripts that get loaded in the browser is that there should be no in the name (I guess because in URLs is a problem. It gets confused with the typical underlining of links). So, I think that the script should be attemptsheet.php.
8. The way tablelib works, it should not be necsessary to define function other_cols like you did (https://github.com/moodleou/moodle-quiz_answersheets/commit/5c6dcec0bd7b235db9035c361039f5c16f724b20#diff-8b71bf0cae824544cc3b3fc74290eac2R86). If the name of the col_attemptsheet method matches the column name attempt_sheet, then it will be called automatically. (So, rename the method to col_attempt_sheet and delete the other_cols method.
9. I don't think we should call $attemptobj->check_review_capability(); in attempt_sheet.php. We should just do the require_capability('quiz/answersheets:view' check.
10. Some picky points about the code in attempt_sheet.php. $url is defnied on line 32, and then not used until like 45. Would be better to move the definion of URL closer to where it is used.
11. Also, it seems strange to render the whole page into $output (which puts the whole HTML into RAM. Why not just echo everything in order here?
12. In the case of a not-finished attempt, do we need some bits of the summary table - in particular the user's name?

Thanks for working on this. If any of the above is not clear, I am happy to discuss.

[timhunt]

Hi Huong? I can see that you pushed another commit. Does that mean that this is ready for review again?

Also, please could you rebase on latest master? Thanks.

[HuongNV13]

Hi Tim, We are just done the local test again after the fix. Please help me to review it again

About point 12: For in-progress attempt, I will show the summary table with only Fullname. If any information that we need in future, we can change it

Thanks,

[timhunt]

Very nearly perfect. I am afraid I spotted two small things that are not quite right.

3. I think you need to set $displayoptions->context before calling $attemptobj->get_question_attempt($slot)->render(). That is the one thing that question_usage_by_activity::render does that otherwise will be missed.

and

5. If you are puttling the class in the quiz_answersheets namespace, then better to just call it utils. Otherwise the full name is quiz_answersheets\quiz_answersheets_utils. Better to have quiz_answersheets\utils.

If you can fix those, then this can be merged into master tomorrow (or today, your time!)

I also noticed one more thing, which it might be worth fixing as part of the next task you do. (I think don't do this now because we want to get the current task finished, and it nearly is):

FUTURE. It occurs to me that in modern Moodle coding style, it would be better to put classes like quiz_answersheets_table.php inside the classes folder, so they can be auto-loaded.

[timhunt]

In case it is not obvious, I think that the reason 3) is necessary - if you want to verify it in testing, is one of two things:

• It might be to do with quetions with images added to the qestion text.
• Or it may be questions (specifically and Essay question where the student is allowed to attach files as part of their response).

Handling the attached files will probably break if context is not set.

[HuongNV13]

Hi Tim. Thanks for your comment. I pushed another commit. Please help me to review it again

Thanks,

[timhunt]

Great! Thanks Huong.