Moodle
  1. Moodle
  2. MDL-31226

Incorrect question state "not yet answered" in summary of progressive attempts when attempts build on previous ones

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.4.6, 2.5.2, 2.6
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      Pre-requirements:
      Create a quiz and two questions:
      Quiz setting 'Attempts allowed': unlimited
      Quiz setting 'Each attempt builds on the last': yes

      Scenario 1:

      1. Start the first attempt.
      2. Make sure that initially displayed status for each question is "Not yet answered".
      3. Enter a response to each question.
      4. Make sure that summary page show "Answer saved" for both questions.
      5. Submit.
      6. Re-attempt.
      7. Notice that displayed questions status is "Not changed since last attempt"
      8. Modify one of the answers, another one leave the same.
      9. Make sure that summary page show "Answer saved" for modified question and "Not changed since last attempt" for the one that was left unchanged.
      10. Submit.

      Scenario 2:

      1. Start the first attempt.
      2. Make sure that initially displayed status for each question is "Not yet answered".
      3. Do not enter response to each question, just click "Next".
      4. Make sure that summary page show "Not yet answered" for both questions.
      5. Submit.
      6. Re-attempt.
      7. Notice that displayed questions status is "Not yet answered"
      8. Modify one of the answers, another one leave the same.
      9. Make sure that summary page show "Answer saved" for modified question and "Not yet answered" for the one that was left unchanged.
      10. Submit.
      11. Re-attempt.
      12. Notice that displayed questions status is "Not yet answered" for the one you did not touch last time, and "Not changed since last attempt" for the one you modified.
      13. Modify answer you did not touch last time.
      14. Make sure that summary page show "Answer saved" for modified question and "Not changed since last attempt" for the one that was left unchanged.
      15. Submit.
      Show
      Pre-requirements: Create a quiz and two questions: Quiz setting 'Attempts allowed': unlimited Quiz setting 'Each attempt builds on the last': yes Scenario 1: Start the first attempt. Make sure that initially displayed status for each question is "Not yet answered". Enter a response to each question. Make sure that summary page show "Answer saved" for both questions. Submit. Re-attempt. Notice that displayed questions status is "Not changed since last attempt" Modify one of the answers, another one leave the same. Make sure that summary page show "Answer saved" for modified question and "Not changed since last attempt" for the one that was left unchanged. Submit. Scenario 2: Start the first attempt. Make sure that initially displayed status for each question is "Not yet answered". Do not enter response to each question, just click "Next". Make sure that summary page show "Not yet answered" for both questions. Submit. Re-attempt. Notice that displayed questions status is "Not yet answered" Modify one of the answers, another one leave the same. Make sure that summary page show "Answer saved" for modified question and "Not yet answered" for the one that was left unchanged. Submit. Re-attempt. Notice that displayed questions status is "Not yet answered" for the one you did not touch last time, and "Not changed since last attempt" for the one you modified. Modify answer you did not touch last time. Make sure that summary page show "Answer saved" for modified question and "Not changed since last attempt" for the one that was left unchanged. Submit.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull 2.4 Branch:
      MDL-31226-MOODLE_24_STABLE
    • Pull 2.5 Branch:
      MDL-31226-MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-31226-master
    • Rank:
      37681

      Description

      With multiple attempts such that new attempts continue (build on) the previous ones, the answers from the previous attempt are transferred to the new attempt. In the new attempt users will change only the incorrect ones and leave the correct untouched. The latter questions will appear in the attempt summary as 'not yet answered' which confuses new users and annoys experienced ones.

      The question state in the mulitple-attempts-building-on-previous mode should distinguish questions which are not answered at all and question which are answered even if the answer was entered in some previous attempt.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Basically, what needs to be done is that the question status message needs to use different text in this specific situation (quiz in attemptonlast mode, question was answered the first time around, answer not yet changes). The only difficulty is that attemptonlast is a quiz concept, whereas the status string is generated in the question engine. So, it will be tricky (though possible) to achieve this.

          If we are working on this, what would be the ideal behaviour? When starting the second attempt, should we distinguish between questions that the student got correct during the first attempt, from those that they got wrong. For example, we could start questions that were right in the first attempt in the 'complete' state, and start the others in the 'answer not changed' state. Thoughts?

          Show
          Tim Hunt added a comment - Basically, what needs to be done is that the question status message needs to use different text in this specific situation (quiz in attemptonlast mode, question was answered the first time around, answer not yet changes). The only difficulty is that attemptonlast is a quiz concept, whereas the status string is generated in the question engine. So, it will be tricky (though possible) to achieve this. If we are working on this, what would be the ideal behaviour? When starting the second attempt, should we distinguish between questions that the student got correct during the first attempt, from those that they got wrong. For example, we could start questions that were right in the first attempt in the 'complete' state, and start the others in the 'answer not changed' state. Thoughts?
          Hide
          Itamar Tzadok added a comment -

          Yes, that would be the ideal behaviour. The multiple-attempts-building-on-previous is essentially an adaptive mode that is meant to allow learners to improve by correcting mistakes, but not in the question level (as in the designated adaptive mode). So, correct answers from previous attempt should be indicated and even frozen if possible as there should be no reason to change a correct answer and freezing it may prevent innocent mistakes and the accompanying complaints and accommodations.

          Show
          Itamar Tzadok added a comment - Yes, that would be the ideal behaviour. The multiple-attempts-building-on-previous is essentially an adaptive mode that is meant to allow learners to improve by correcting mistakes, but not in the question level (as in the designated adaptive mode). So, correct answers from previous attempt should be indicated and even frozen if possible as there should be no reason to change a correct answer and freezing it may prevent innocent mistakes and the accompanying complaints and accommodations.
          Hide
          Joseph Rézeau added a comment -

          I agree with Itamar's views on this.
          About the idea of "freezing" previous correct answers:

          • the mark (points, score) for previously entered correct answers should be frozen
          • but the answer field(s) for such questions should not be frozen; in adaptive/learning mode, it does make sense for students to try out more than one "correct" answer, especially for the short answer and related question types.
          Show
          Joseph Rézeau added a comment - I agree with Itamar's views on this. About the idea of "freezing" previous correct answers: the mark (points, score) for previously entered correct answers should be frozen but the answer field(s) for such questions should not be frozen; in adaptive/learning mode, it does make sense for students to try out more than one "correct" answer, especially for the short answer and related question types.
          Hide
          Itamar Tzadok added a comment -

          Insofar as the second point is directed at practice and does not serve assessment it should probably be implemented as a distinct practice (preview of sort) mode in the quiz level (or a "tiny-quiz" plugin). Not only that saving an incorrect answer as the last response for a question that is already marked correct is confusing and potentially counterproductive especially when the responses are reviewed later, it also inflates the database with redundant response history. In a practice activity we can discard the data or most of it when the activity is terminated.

          Show
          Itamar Tzadok added a comment - Insofar as the second point is directed at practice and does not serve assessment it should probably be implemented as a distinct practice (preview of sort) mode in the quiz level (or a "tiny-quiz" plugin). Not only that saving an incorrect answer as the last response for a question that is already marked correct is confusing and potentially counterproductive especially when the responses are reviewed later, it also inflates the database with redundant response history. In a practice activity we can discard the data or most of it when the activity is terminated.
          Hide
          Nicholas Koeppen added a comment -

          Hello,

          I'm curious where this issue stands. Our students are having difficulty understanding the review status messages resulting in support calls from users fearing they can't submit the following quiz attempts. What can I do to help implement this fix. Thank you.

          Show
          Nicholas Koeppen added a comment - Hello, I'm curious where this issue stands. Our students are having difficulty understanding the review status messages resulting in support calls from users fearing they can't submit the following quiz attempts. What can I do to help implement this fix. Thank you.
          Hide
          Tim Hunt added a comment -

          Yo devise a solution, you need to think about:

          • Quizzes with and without "Each attempt builds on last" enabled.
          • All possible question behaviours.
          • All possible states that a question attempt might be in.

          And come up with and implementation of question_behaviour::get_state_string that does the right thing in all circumstances.

          Note that the current implementation does the right thing in almost every circumstance, so you are much more likely to break something than you are to fix things.

          Also note that you are not allowed to refer to anything quiz-specific in the question code.

          And, your fix needs to be comprehensible to whoever has to review it. (That means me.) Probably the way to achieve that, and to prove that you have not caused any regressions, is to make sure there is a complete set of unit tests for the get_state_string method.

          So, it is an interesting problem, and if you have time to give it the thought it deserves, that would be great, because I am unlikely to have time to tackle it any time soon.

          Show
          Tim Hunt added a comment - Yo devise a solution, you need to think about: Quizzes with and without "Each attempt builds on last" enabled. All possible question behaviours. All possible states that a question attempt might be in. And come up with and implementation of question_behaviour::get_state_string that does the right thing in all circumstances. Note that the current implementation does the right thing in almost every circumstance, so you are much more likely to break something than you are to fix things. Also note that you are not allowed to refer to anything quiz-specific in the question code. And, your fix needs to be comprehensible to whoever has to review it. (That means me.) Probably the way to achieve that, and to prove that you have not caused any regressions, is to make sure there is a complete set of unit tests for the get_state_string method. So, it is an interesting problem, and if you have time to give it the thought it deserves, that would be great, because I am unlikely to have time to tackle it any time soon.
          Hide
          Tim Hunt added a comment -

          This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

          For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

          Show
          Tim Hunt added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
          Hide
          Katharine Ebbs added a comment -

          Dear Tim,
          Can you please advise who is assigned to do this fix? We have a lot of people sitting exams in mid August and we would like to upgrade to Moodle 2.4 andhave the fix done. Depending on time it would take and cost we could maybe help support financially? If any other organisations are interested in sharing the cost we would be interested. Best Regards
          Katharine Ebbs

          Show
          Katharine Ebbs added a comment - Dear Tim, Can you please advise who is assigned to do this fix? We have a lot of people sitting exams in mid August and we would like to upgrade to Moodle 2.4 andhave the fix done. Depending on time it would take and cost we could maybe help support financially? If any other organisations are interested in sharing the cost we would be interested. Best Regards Katharine Ebbs
          Hide
          Tim Hunt added a comment -

          It is not assigned to anyone to fix. (See the bit saying Assignee: Unassigned). The issue is neither time nor cost, necessarily. See my previous comments.

          Show
          Tim Hunt added a comment - It is not assigned to anyone to fix. (See the bit saying Assignee: Unassigned). The issue is neither time nor cost, necessarily. See my previous comments.
          Hide
          Katharine Ebbs added a comment -

          Can you tell us anything that can support us here? Any estimate of how long it would take? We have the choice of staying with 1.9 which is no longer supported by Moodle or upgrading where we use the reattempt button to assist users who have computer/internet difficulties in the multiple exam centres we use. The way the summary table looks at the moment on a reattempt would freak exam candidates out.

          Show
          Katharine Ebbs added a comment - Can you tell us anything that can support us here? Any estimate of how long it would take? We have the choice of staying with 1.9 which is no longer supported by Moodle or upgrading where we use the reattempt button to assist users who have computer/internet difficulties in the multiple exam centres we use. The way the summary table looks at the moment on a reattempt would freak exam candidates out.
          Hide
          Tim Hunt added a comment -

          To estimate, I would need to know how to solve this, and if I knew that, I probably would already just have done it myself. Try http://moodle.com/partners/ for commercial Moodle support.

          You could also ask in the quiz forum: https://moodle.org/mod/forum/view.php?id=737 "we use the reattempt button to assist users who have computer/internet difficulties in the multiple exam centres" seems odd to me. There is probably a better way, but this is not the place to discuss it.

          Show
          Tim Hunt added a comment - To estimate, I would need to know how to solve this, and if I knew that, I probably would already just have done it myself. Try http://moodle.com/partners/ for commercial Moodle support. You could also ask in the quiz forum: https://moodle.org/mod/forum/view.php?id=737 "we use the reattempt button to assist users who have computer/internet difficulties in the multiple exam centres" seems odd to me. There is probably a better way, but this is not the place to discuss it.
          Hide
          Ruslan Kabalin added a comment -

          We had a conversation with Tim regarding this. The problem is mainly that every new attempt is started with hard-coded state "todo", thus the state remain as "Not yes answered". I ended up adding a new state for "notchanged" and use it when the attempt is based on previous one. Modifying the output of existing todo state based on number of attempts is also possible, but seems dirtier to me. The fix seems make output behave correctly in cases I tired (first attempt, second and more attempt, interrupted first attempt, interrupted second attempt), but more extensive testing is required.

          Show
          Ruslan Kabalin added a comment - We had a conversation with Tim regarding this. The problem is mainly that every new attempt is started with hard-coded state "todo", thus the state remain as "Not yes answered". I ended up adding a new state for "notchanged" and use it when the attempt is based on previous one. Modifying the output of existing todo state based on number of attempts is also possible, but seems dirtier to me. The fix seems make output behave correctly in cases I tired (first attempt, second and more attempt, interrupted first attempt, interrupted second attempt), but more extensive testing is required.
          Hide
          Tim Hunt added a comment -

          No unit tests.

          No testing instructions. (The testing instructions above were written when the bug was submitted, and do not reflect what really needs to be done to verify that the patch is correct and does not cause any regressions.)

          The new question state is not necessary, nor is it desirable. (I may be wrong about this, but a new state requires extensive argument to justify it, which is lacking.)

          If you add a new question state, you need to document it in the upgrade.txt file. You also need to add testing instructions to cover every place that could possibly break as a result.

          Show
          Tim Hunt added a comment - No unit tests. No testing instructions. (The testing instructions above were written when the bug was submitted, and do not reflect what really needs to be done to verify that the patch is correct and does not cause any regressions.) The new question state is not necessary, nor is it desirable. (I may be wrong about this, but a new state requires extensive argument to justify it, which is lacking.) If you add a new question state, you need to document it in the upgrade.txt file. You also need to add testing instructions to cover every place that could possibly break as a result.
          Hide
          Ruslan Kabalin added a comment -

          Thanks for criticism Tim I submitted the patch for initial review (yes/no/what to change) just to figure out if you and other devs approve this way of resolving the issue. I will do the remaining bits before submitting for integration.

          Regarding new question state, I think this is the cleanest way of resolving the issue. Another approach could be determining the number of attempts within get_state_class of question_state_todo class, or adding a class variable that will be used as flag whether the question has been submitted (in which case it will need to be updated on each instantiation). But all this makes the code complicated and more difficult to support in future. If you already have good infrastructure for question states in place, I do not understand why I should avoid using it? I do not have a deep understanding of the whole question implementation, may be there are good reasons not adding new states, if so, please explain.

          I do not see upgrade.txt within question/engine directory, do I need to create new?

          Show
          Ruslan Kabalin added a comment - Thanks for criticism Tim I submitted the patch for initial review (yes/no/what to change) just to figure out if you and other devs approve this way of resolving the issue. I will do the remaining bits before submitting for integration. Regarding new question state, I think this is the cleanest way of resolving the issue. Another approach could be determining the number of attempts within get_state_class of question_state_todo class, or adding a class variable that will be used as flag whether the question has been submitted (in which case it will need to be updated on each instantiation). But all this makes the code complicated and more difficult to support in future. If you already have good infrastructure for question states in place, I do not understand why I should avoid using it? I do not have a deep understanding of the whole question implementation, may be there are good reasons not adding new states, if so, please explain. I do not see upgrade.txt within question/engine directory, do I need to create new?
          Hide
          Tim Hunt added a comment -

          In a sense, question states form a languages for communication between the question engine, and other code that uses the question engine. Adding a new word to a language does not necessarily improve communication. It only improves communcation if the cost of teaching everyone the new work is less than that extra expressiveness gained from the ne word.

          Or, to put it another way, adding a state breaks backwards compatibility. We don't do that unless it is really necessry. Here it is not necessary. We have discussed how to implement this without adding a new state.

          Show
          Tim Hunt added a comment - In a sense, question states form a languages for communication between the question engine, and other code that uses the question engine. Adding a new word to a language does not necessarily improve communication. It only improves communcation if the cost of teaching everyone the new work is less than that extra expressiveness gained from the ne word. Or, to put it another way, adding a state breaks backwards compatibility. We don't do that unless it is really necessry. Here it is not necessary. We have discussed how to implement this without adding a new state.
          Hide
          Ruslan Kabalin added a comment -

          Thanks Tim for your comments. I have modified the patch accordingly. Unit tests have been modified and language output validation has been added. Testing instruction has been modified.

          Show
          Ruslan Kabalin added a comment - Thanks Tim for your comments. I have modified the patch accordingly. Unit tests have been modified and language output validation has been added. Testing instruction has been modified.
          Hide
          Ruslan Kabalin added a comment -

          Will modify unittest in a moment.

          Show
          Ruslan Kabalin added a comment - Will modify unittest in a moment.
          Hide
          Ruslan Kabalin added a comment -

          Finally... ready for revision.

          Show
          Ruslan Kabalin added a comment - Finally... ready for revision.
          Hide
          Tim Hunt added a comment -

          The code is now almost right. Just some minor issues, of which ony 3 is serious.

          1. First line of the commit comment is just too long. (Look at the commit on github.)

          2. https://github.com/lucisgit/moodle/commit/6f4474b86c8b25155c1f3b77868c053ed0717a53#L3R580 this is not the right way to check what the state is. You should use

          $this->get_state() == question_state::$complete

          3. https://github.com/lucisgit/moodle/commit/6f4474b86c8b25155c1f3b77868c053ed0717a53#L3L890 This is wrong, in terms of setting the state. To see the problem, use these steps to reproduce:

          a. Make an "Each attempt builds on last" quiz with one multiplechoice question.
          b. make a first attempt, do not answer the question.
          c. start a second attempt.

          with your code, it will be shown as "Not changed since last attempt". It should be shown as "Not answered".

          4. Given that you got 3. wrong, I think you should add a unit test specifically for this case.

          To solve 3. ... this was too difficult to explain in words. I think you need something like https://github.com/timhunt/moodle/commit/MDL-31226. Note that I have not tested this. I am assuming you will squash those changes into your single commit.

          Show
          Tim Hunt added a comment - The code is now almost right. Just some minor issues, of which ony 3 is serious. 1. First line of the commit comment is just too long. (Look at the commit on github.) 2. https://github.com/lucisgit/moodle/commit/6f4474b86c8b25155c1f3b77868c053ed0717a53#L3R580 this is not the right way to check what the state is. You should use $this->get_state() == question_state::$complete 3. https://github.com/lucisgit/moodle/commit/6f4474b86c8b25155c1f3b77868c053ed0717a53#L3L890 This is wrong, in terms of setting the state. To see the problem, use these steps to reproduce: a. Make an "Each attempt builds on last" quiz with one multiplechoice question. b. make a first attempt, do not answer the question. c. start a second attempt. with your code, it will be shown as "Not changed since last attempt". It should be shown as "Not answered". 4. Given that you got 3. wrong, I think you should add a unit test specifically for this case. To solve 3. ... this was too difficult to explain in words. I think you need something like https://github.com/timhunt/moodle/commit/MDL-31226 . Note that I have not tested this. I am assuming you will squash those changes into your single commit.
          Hide
          Ruslan Kabalin added a comment -

          Thanks a lot for review, Tim. I have addressed the issues you listed and squashed your code with some very minor modification. I have also added a test for the scenario you mentioned and tested the code with and without patch you suggested (test failed without it).

          Show
          Ruslan Kabalin added a comment - Thanks a lot for review, Tim. I have addressed the issues you listed and squashed your code with some very minor modification. I have also added a test for the scenario you mentioned and tested the code with and without patch you suggested (test failed without it).
          Hide
          Ruslan Kabalin added a comment -

          Testing instruction has been modified, additional scenario that covers previous revision outcomes was added.

          Show
          Ruslan Kabalin added a comment - Testing instruction has been modified, additional scenario that covers previous revision outcomes was added.
          Hide
          Tim Hunt added a comment -

          [Y] Syntax
          [Y] Whitespace
          [Y] Output
          [Y] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Yay! Nice work Ruslan. I guess you now just need to back-port this before it is submtited for integration.

          Show
          Tim Hunt added a comment - [Y] Syntax [Y] Whitespace [Y] Output [Y] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Yay! Nice work Ruslan. I guess you now just need to back-port this before it is submtited for integration.
          Hide
          Ruslan Kabalin added a comment -
          Show
          Ruslan Kabalin added a comment - Related forum discussion: https://moodle.org/mod/forum/discuss.php?d=233390
          Hide
          Ruslan Kabalin added a comment - - edited

          Unit tests has been run on the corresponding stable branches as well. Ready for integration.

          Show
          Ruslan Kabalin added a comment - - edited Unit tests has been run on the corresponding stable branches as well. Ready for integration.
          Hide
          Marina Glancy added a comment -

          Thanks a lot everybody, this has been integrated in 2.4, 2.5 and master and now ready for testing. 12 votes, 15 watchers - well done!

          Ruslan, special congratulations on passing though Tim's peer review

          Show
          Marina Glancy added a comment - Thanks a lot everybody, this has been integrated in 2.4, 2.5 and master and now ready for testing. 12 votes, 15 watchers - well done! Ruslan, special congratulations on passing though Tim's peer review
          Hide
          Ruslan Kabalin added a comment -

          Ruslan, special congratulations on passing though Tim's peer review

          Thanks Marina, we need a special badge for this in Moodle

          Show
          Ruslan Kabalin added a comment - Ruslan, special congratulations on passing though Tim's peer review Thanks Marina, we need a special badge for this in Moodle
          Hide
          Mark Nelson added a comment -

          Hi Ruslan, works as expected. Thanks for the easy to follow testing instructions and your contribution to Moodle.

          Show
          Mark Nelson added a comment - Hi Ruslan, works as expected. Thanks for the easy to follow testing instructions and your contribution to Moodle.
          Hide
          Sam Hemelryk added a comment -

          Huzzah, your code made it into Moodle. Perhaps now things are ever so slightly better!

          "The ship can't take this much pressure. Sometimes it falls apart just sitting in the hangar."
          ~ Professor Farnsworth

          Show
          Sam Hemelryk added a comment - Huzzah, your code made it into Moodle. Perhaps now things are ever so slightly better! "The ship can't take this much pressure. Sometimes it falls apart just sitting in the hangar." ~ Professor Farnsworth

            People

            • Votes:
              12 Vote for this issue
              Watchers:
              18 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: