LASTATTEMPT quiz not using latest attempt to grade

https://github.com/tlevi/moodle/tree/mdl28226

This was hit for us in the regrade function; there is nothing specifying the ordering that quiz_calculate_best_grade must receive attempts in, and callers don't worry about it either.

If your DB picks an order other than ID or time for returning attempts (which of course is completly allowed), this bug will appear.

Thank you for diagnosing this, and working on a fix.

I'm afraid I don't like your fix. We should not tolerate lists of attempts that are sorted badly. The only place where we get badly sorted lists of attempts is in mod/quiz/report/overview/report.php, in the bit of code you fix with your change in MDL-28227. We should just add an order-by there.

Moodle 2.1 is definitely not affected by this. I am pretty sure 1.9 is not affected, based on a combination of my knowledge of the code, and also the fact that no one has reported a problem over the years.

Note that this fix follows on from MDL-28227.

Tony, please could you take a look and see whether it makes sense to you. Thanks, Tim.

Hi Tim,

I see where you're coming from there, but I think I disagree.
It comes down to the fact that calculate_best_grade does not meet it's contract, especially given the comments atm say:
@param array $attempts An array of all the attempts of the user at the quiz

There is no requirement about order stated here. Unit tests of pre/post conditions would fail, so it needs fixing there.
We have scripts to regrade (i.e quiz_attempts to quiz_grades only) that would hit this issue and some parts of moodle use this 'erronously' too, such as quiz_save_best_grade when not given attempts - this will call quiz_get_user_attempts and that does not order by timefinish asc.

Given all that, there seems little reason to have the DB do extra work (sorts or different plans) to satisfy an unstated requirement (bug) and the existing code loops over all elements anyway, so no performance implication.

Additionally, this issue was a seperate user report to MDL-28227 and so are the trigger conditions; it should not be blocked by that or otherwise related.

Sorry for the wall of text... what do you think?

Sorry, I meant to update the PHP doc comment on calculate_best_grade to say the array should be ordered, but I forgot. I will do that tomorrow.

This issue is only marked as blocked by MDL-28227 because they touch the same area of code, and so the patches will only apply in that order.

quiz_get_user_attempts does order the attempts - by attempt number, not time finish - but that is the same order.

If the function is to remain the same, when getting the last attempt grade, why does it need to loop over every single one instead of just grabbing the last element on the list?

I don't think it is reasonable to expect DB queries to return things in order; sure, most DBs do this by accident but we can't expect that to always be true.

Your proposed patch is an API change after-the-fact and requires an audit of all calling code to ensure there are no lurking bugs in other places.

It seems much more trivial to fix this at the source.

Any update on this one? Tim?

Well, you disagreed with my solution, so I did not submit it for integration before I went on holiday. I get back from holiday in about a week. I will look at this again then.

Oh ok

In my mind, the API for quiz_calculate_best_grade has always been clear, and this patch now fixes the docs to match how it is supposed to be. It also fixes the rather bizarre code.

It also fixes the underlying issue in 2.0.

I am giving you one more chance to peer review this and comment before I submit it for integration.

Documentation is still unclear; which is the correct order to give the attempts in? id, user, random?

There is no point whatsoever to calculate_best_grade if all it is going to do is give me the first or last element of the array; I'm sure callers are perfectly capable of doing that themselves.

Why not just obliterate this now useless function entirely...

Tim, it looks fine to me, in that the existing function is fixed and the docs more accurate.

The order to me looks OK too. "all the user's attempts at this quiz in order" makes sense to me.

+1 to integrate

Certainly the coeumentation is clear that I can pass attempts in ID order and expect correct grading...

Submitting for integration.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Integrated, thanks!

Test result: after re-arranging the order of entries in quiz-attempts, it still picked entry with the latest time and largest attempt number and used that as the basis of its calculation.

Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.