Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: 2.1.1, 2.2
-
Fix Version/s: 2.1.2
-
Component/s: Questions
-
Labels:
-
Testing Instructions:
-
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.
Gliffy Diagrams
Attachments
Issue Links
- has been marked as being related by
-
-
- Closed
-
Activity
- All
- Comments
- Work Log
- History
- Activity
- Links Hierarchy
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.)
New implementation of the fix, as discussed. Tested on a copy of my live data. Note the modified branch names.
Nice one! Thanks for revising the patch. I am now submitting this 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 
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!
yes, please provide some testing instructions
More detailed testing instructions included. I admit that it's a bit complicated; it's a migration related error.
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.
merged, thanks! Now tests are passing ok.
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.
stopping testing, seems its still being worked on.
Drat! how did we miss that. The $a should be $state->answer.
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.
Added:
master: http://git.moodle.org/gw?p=integration.git;a=commit;h=6459abdc9e3774594faa1b963a5d75a66d62120e
(and cherry-picked to 21_STABLE)
Running tests again...ciao
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.
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
Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.
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.