Closed HuongNV13 closed 4 years ago
I think this is almost exactly right. Just a few minor things to fix before this gets merged:
I am guessing that the window.onbeforeunload = null; bit is to aviod problems from core_formchangechecker if the report options have been changed. I think that the 'right' way to do that is if (typeof M.core_formchangechecker !== 'undefined') { M.core_formchangechecker.set_form_submitted(); }
The redirect at the end of processresponses.php should probably be changed to add the lastchanged parameter (even if it will not work until Pull #6 has been merged).
It is not an issue for this PBI, but when it is used in the report table, get_user_details has a performance issue. \core_user::get_user($userid); is a database query, and context_module::instance($cmid) is a database query. You should not do that in the loop over talbe rows. It woudl be better to pass in $user, rather than $userid, where $user is a stdClass with all the required fields set. Similarly, pass in $context instead of $cmid.
Other two commits: absolutely fine. Thanks.
And, thanks for splitting it into separate commits. That made the review easier.
Hi @timhunt Thanks for your comment
I already fixed all of them Also, this branch included point 16 in Pull #6
Please help me to review it again
Thanks,
Thanks Huong. Merged now.
Just one thing: $cm is a really, really bad name for a variable holding a context. In Moodle, a $cm is a very well-known thing. So using that vaiable name for something else is very confusing.
Rather than delay this issue any more, I just added a commit to rename the variable. I hope that was OK.
Hi @timhunt, Please help me to review it
Including in this branch:
The code was tested done by our tester
Thanks,