Moodle
  1. Moodle
  2. MDL-28684

QE2 upgrade fails on bogus second start state when questiontype is 'multichoice'

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.2
    • Fix Version/s: 2.1.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      This bug can only be tested in a particular data state, which cannot be produced in the UI for all I know.

      You need to start from a Moodle 1.9 installation. This installation must contain a quiz with a multichoice question included. In an attempt on that question, there must be a non-start state (i.e., not the first state in the attempt) with empty answer, for example: "8917,8918,8919,8920:"

      Run the 1.9 -> 2.1 migration routine on this data. Verify that the particular attempt gets converted, and conversion does not fail with an error message.

      To make this more transparent, I have attached a piece of unit test code generated with the QE upgrade helper. I'm not sure whether you use unit tests in integration testing, but it should make clear what the state of the data is.

      Show
      This bug can only be tested in a particular data state, which cannot be produced in the UI for all I know. You need to start from a Moodle 1.9 installation. This installation must contain a quiz with a multichoice question included. In an attempt on that question, there must be a non-start state (i.e., not the first state in the attempt) with empty answer, for example: "8917,8918,8919,8920:" Run the 1.9 -> 2.1 migration routine on this data. Verify that the particular attempt gets converted, and conversion does not fail with an error message. To make this more transparent, I have attached a piece of unit test code generated with the QE upgrade helper. I'm not sure whether you use unit tests in integration testing, but it should make clear what the state of the data is.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      18470

      Description

      I ran into the following error message during the QE2 upgrade from Moodle 1.9 to 2.1

      "Coding error detected, it must be fixed by a programmer: Two inconsistent open states for question session 36362."

      Situation in the data: The affected question is a multichoice question. The question session turned out to have two start states (event=0), with sequence numbers 0 and 2. The answer in the second one was empty, with the answer string being "8917,8918,8919,8920:" .

      The code deals with these bogus start states with empty answers, in question_behaviour_attempt_updater::process0(). However, for determining whether the answer of state is empty, it checks whether the answer is an empty string. This is incorrect in the case of multichoice questions, where the answer string internally records the order of answers. In this case, the code should check whether the answer string ends in ":".

      Proposed fix will follow shortly.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          That will work, but it seems a bit ugly. We should not really be referring to specific question types there.

          The nice solution would refactor this so that it calls a new method $this->qtypeupdater->is_blank_answer($state); - default implementation return $state->answer == ''; override that in qtype_multichoice_qe2_attempt_updater.

          Would you be able to make that change? Thanks.

          Show
          Tim Hunt added a comment - That will work, but it seems a bit ugly. We should not really be referring to specific question types there. The nice solution would refactor this so that it calls a new method $this->qtypeupdater->is_blank_answer($state); - default implementation return $state->answer == ''; override that in qtype_multichoice_qe2_attempt_updater. Would you be able to make that change? Thanks.
          Hide
          Henning Bostelmann added a comment -

          Will do, although there might be a bit of delay. (I've integration tested the patch as proposed in our local environment, so we'll go live with that.)

          Show
          Henning Bostelmann added a comment - Will do, although there might be a bit of delay. (I've integration tested the patch as proposed in our local environment, so we'll go live with that.)
          Hide
          Henning Bostelmann added a comment -

          New implementation of the fix, as discussed. Tested on a copy of my live data. Note the modified branch names.

          Show
          Henning Bostelmann added a comment - New implementation of the fix, as discussed. Tested on a copy of my live data. Note the modified branch names.
          Hide
          Tim Hunt added a comment -

          Nice one! Thanks for revising the patch. I am now submitting this for integration.

          Show
          Tim Hunt added a comment - Nice one! Thanks for revising the patch. I am now submitting this 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 -

          This has been integrated, many thanks!

          Note the commit contained illegal whitespace, usually we reject such commits. This time I've added extra commit fixing it, but please, try to check that next time. TIA!

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated, many thanks! Note the commit contained illegal whitespace, usually we reject such commits. This time I've added extra commit fixing it, but please, try to check that next time. TIA!
          Hide
          Aparup Banerjee added a comment -

          yes, please provide some testing instructions

          Show
          Aparup Banerjee added a comment - yes, please provide some testing instructions
          Hide
          Henning Bostelmann added a comment -

          More detailed testing instructions included. I admit that it's a bit complicated; it's a migration related error.

          Show
          Henning Bostelmann added a comment - More detailed testing instructions included. I admit that it's a bit complicated; it's a migration related error.
          Hide
          Tim Hunt added a comment -

          Doh!, we did not even run the existing unit tests, which showed the new code was wrong.

          Fixes at
          https://github.com/timhunt/moodle/compare/master...MDL-28684
          https://github.com/timhunt/moodle/compare/MOODLE_21_STABLE...MDL-28684_21

          You should be able to merge those branches in to get the fix.

          Show
          Tim Hunt added a comment - Doh!, we did not even run the existing unit tests, which showed the new code was wrong. Fixes at https://github.com/timhunt/moodle/compare/master...MDL-28684 https://github.com/timhunt/moodle/compare/MOODLE_21_STABLE...MDL-28684_21 You should be able to merge those branches in to get the fix.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          merged, thanks! Now tests are passing ok.

          Show
          Eloy Lafuente (stronk7) added a comment - merged, thanks! Now tests are passing ok.
          Hide
          Henning Bostelmann added a comment -

          Tim , something seems incorrect about this commit you just made:

          https://github.com/timhunt/moodle/commit/e9f707d38a9bc5b43a9beb63b905e423223b5246

          Perhaps this passes the unit test, but it surely doesn't have the desired effect. The variable $a is now undefined.

          Show
          Henning Bostelmann added a comment - Tim , something seems incorrect about this commit you just made: https://github.com/timhunt/moodle/commit/e9f707d38a9bc5b43a9beb63b905e423223b5246 Perhaps this passes the unit test, but it surely doesn't have the desired effect. The variable $a is now undefined.
          Hide
          Aparup Banerjee added a comment -

          stopping testing, seems its still being worked on.

          Show
          Aparup Banerjee added a comment - stopping testing, seems its still being worked on.
          Hide
          Tim Hunt added a comment -

          Drat! how did we miss that. The $a should be $state->answer.

          Show
          Tim Hunt added a comment - Drat! how did we miss that. The $a should be $state->answer.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Wow, yes (I completely missed that too on review). Also how the unittests do not show the notice about undefined $a.

          Adding commits to set proper $state->answer... @ 21_STABLE and master.

          Show
          Eloy Lafuente (stronk7) added a comment - Wow, yes (I completely missed that too on review). Also how the unittests do not show the notice about undefined $a. Adding commits to set proper $state->answer... @ 21_STABLE and master.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Added:

          master: http://git.moodle.org/gw?p=integration.git;a=commit;h=6459abdc9e3774594faa1b963a5d75a66d62120e

          (and cherry-picked to 21_STABLE)

          Running tests again...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Added: master: http://git.moodle.org/gw?p=integration.git;a=commit;h=6459abdc9e3774594faa1b963a5d75a66d62120e (and cherry-picked to 21_STABLE) Running tests again...ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Aha, found why unittest didn't return any notice and the cause is that the second part of the offending line is never executed along unittests run. So, with:

          return empty($state->answer) || substr($state->answer, -1) == ':';
          

          1) We are not covering the second condition in unittests. Ideally we should.
          2) I tried swapping the condition and then got the notice, so no mystery 1) is the cause for the "missing notice"

          Summary: code has been fixed, unit tests (incomplete) continue passing. This should be tested again.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Aha, found why unittest didn't return any notice and the cause is that the second part of the offending line is never executed along unittests run. So, with: return empty($state->answer) || substr($state->answer, -1) == ':'; 1) We are not covering the second condition in unittests. Ideally we should. 2) I tried swapping the condition and then got the notice, so no mystery 1) is the cause for the "missing notice" Summary: code has been fixed, unit tests (incomplete) continue passing. This should be tested again.
          Hide
          Aparup Banerjee added a comment -

          i've added a few of those states mentioned in test instructions to the db, this fix does work for me.

          ps : not too sure of any other variations to the states, but this works

          Show
          Aparup Banerjee added a comment - i've added a few of those states mentioned in test instructions to the db, this fix does work for me. ps : not too sure of any other variations to the states, but this works
          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:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: