Moodle
  1. Moodle
  2. MDL-28226

LASTATTEMPT quiz not using latest attempt to grade

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      Testing is most needed in 2.0:

      1. Create a quiz set to grading method first, or last, attempt.
      2. Do lots of attempts as one student.
      3. Try to persuade your database to return records in an arbitrary order. I think Postgres does this after you have deleted enough rows from a table and vacuummed.
      4. Regrade the quiz, and make sure the final grade is computed correctly.

      Show
      Testing is most needed in 2.0: 1. Create a quiz set to grading method first, or last, attempt. 2. Do lots of attempts as one student. 3. Try to persuade your database to return records in an arbitrary order. I think Postgres does this after you have deleted enough rows from a table and vacuummed. 4. Regrade the quiz, and make sure the final grade is computed correctly.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      17917

      Description

      The quiz grade calculation in LASTATTEMPT mode uses the last on the list of attempts it is given, not the last by data.

      This is resolved by checking the attempt is newer than the best so far, so latest attempts are used regardless of order.

      Patch incoming with github link

        Issue Links

          Activity

          Show
          Tony Levi added a comment - https://github.com/tlevi/moodle/tree/mdl28226
          Hide
          Tony Levi added a comment -

          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.

          Show
          Tony Levi added a comment - 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.
          Hide
          Tim Hunt added a comment -

          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.

          Show
          Tim Hunt added a comment - 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.
          Hide
          Tim Hunt added a comment -

          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.

          Show
          Tim Hunt added a comment - 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.
          Hide
          Tony Levi added a comment -

          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?

          Show
          Tony Levi added a comment - 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?
          Hide
          Tim Hunt added a comment -

          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.

          Show
          Tim Hunt added a comment - 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.
          Hide
          Adam Olley added a comment -

          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?

          Show
          Adam Olley added a comment - 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?
          Hide
          Tony Levi added a comment -

          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.

          Show
          Tony Levi added a comment - 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.
          Hide
          Tony Levi added a comment -

          Any update on this one? Tim?

          Show
          Tony Levi added a comment - Any update on this one? Tim?
          Hide
          Tim Hunt added a comment -

          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.

          Show
          Tim Hunt added a comment - 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.
          Hide
          Tony Levi added a comment -

          Oh ok

          Show
          Tony Levi added a comment - Oh ok
          Hide
          Tim Hunt added a comment -

          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.

          Show
          Tim Hunt added a comment - 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.
          Hide
          Tony Levi added a comment -

          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...

          Show
          Tony Levi added a comment - 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...
          Hide
          Martin Dougiamas added a comment -

          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

          Show
          Martin Dougiamas added a comment - 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
          Hide
          Tony Levi added a comment -

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

          Show
          Tony Levi added a comment - Certainly the coeumentation is clear that I can pass attempts in ID order and expect correct grading...
          Hide
          Tim Hunt added a comment -

          Submitting for integration.

          Show
          Tim Hunt added a comment - Submitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Michael de Raadt added a comment -

          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.

          Show
          Michael de Raadt added a comment - 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.
          Hide
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: