Moodle

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

Details

  • Type: Bug Bug
  • Status: Closed 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:
  • Pull Master Diff URL:

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

has been marked as being related by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-34385 When upgrading from 1.9.17 to 2.2.3, the quiz upgrade has question session errors

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Aparup Banerjee added a comment -

yes, please provide some testing instructions

Show
Aparup Banerjee added a comment - yes, please provide some testing instructions
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: