Question Engine 2:Review 2011-05

Basic References

Repository: git://github.com/timhunt/Moodle-Question-Engine-2.git (qe2_wip branch)
Docs: https://docs.moodle.org/en/Development:Question_Engine_2
Tracker: http://tracker.moodle.org/browse/MDL-20636
Forums: http://moodle.org/mod/forum/view.php?id=737

Intro and Organization

This is aimed to be a (more or less complete) review of the new Question Engine for Moodle 2.1, trying to analyze (the huge) Tim's work under different perspectives (coding, DB structures, security...) in some separated iterations.

The list below shows each one of steps to follow in order to complete the review process. Note that Q&A rounds can happen at any stage.

Read all the Docs
Read the MDL
Create Oracle DB for that (stronk7_qereview) (yes, it's the best testing env, there is some serious rationale about that)
Look @ DB structures and installation.
Review QE2 itself, understand tests
Look for DB uses
Look for Quiz changes related to QE2 support
Review all requests, security, caps, user-introduced info specially.
Upgrade (both quiz and engine)
Backup & Restore (specially BC).
Functional tests
Review & consider upgrade helper local plugin

The review results will be disclosed here, Q&A will happen in Moodle forums, and any task/bug found will be reported in the Tracker. Everything in this document will be numbered and cross linked for easier reference everywhere.

Said that, let's go! Yay!

Installation / upgrade

A1: behaviors missing version / requires? And should have enable/disable/delete interface in admin->plugins ? whitelist them?
A2: problem on local plugins showing again and again? Need to define them as Standard? whitelist them?
A3: Silly: quiz_report => quiz_reports rationale? Note I agree plural is better, but there are references to quiz_report into the statistics report upgrade code. And that table is renamed by quiz upgrade… anybody upgrading from 1.9 will crash?
A4: Compare all the question & quiz tables after trying these paths between 2.1 (installed) and upgraded.

This has been done @ qe_integration and now structure matches 100%, but the changes need review @ MDL-20636

A5: Table qtype_essay_options ? Is that correct? Shouldn't be question_essay_options? I've fixed the query a bit, but not the name.
A6: Upgrade: Why a lot of (quiz) upgrade steps are conditionally executed based on: if ($dbman->field_exists('quiz', 'review')). What happens if the fields is not there? Seems strange.
A7: Do we have protection for timeouts / progress on potentially LONG steps? convert_all_quiz_attempts() seems ok, but… needed in more places?

Backup and restore

B1: Silly: Surely get_qtype_restorer() can be protected
B2: Silly: backup and restore. Why "response/variable" ? Shouldn't be "responses/response" or "variables/variable" ?
B3: convert 1.9 => 2.0 (quiz & qtypes)
B4: restore 2.0 => 2.1 (quiz & qtypes)
B5: Support for missing type on 1.9/2.0/2.1 => 2.1 restore
B6: Do we need qbehavior plugin support in B & R? If so, where should it have "hook" points?

Documentation

C1: Teacher / admin documentation missing, explaining behaviors, qtypes, quiz, qeupgradehelper local extension...

Unit tests

D1: running /question tests returns: 74/75 test cases complete: 2194 passes, 0 fails and 0 exceptions. Which one is the missing test case? Also coverage information seems really incomplete. Need to add the corresponding "include/exclude coverage settings to get better coverage?.
D2: running mod/quiz passes also ok, but coverage also needs better definition. Same for lib/simpletest/testquestionlib.php
D3: something goes really wrong here running ALL tests (MySQL), with dozens of errors on question/engine/simpletest/testdatalib.php with continuous errors like:

qubaid_condition_test / ▶ test_qubaid_list_one_join
Equal expectation fails at character 111 with [SELECT qa.id, qa.maxmark FROM {question_attempts} qa WHERE qa.questionusageid = :qubaid8 AND qa.slot = :slot] and [SELECT qa.id, qa.maxmark FROM {question_attempts} qa WHERE qa.questionusageid = :qubaid212 AND qa.slot = :slot] at [/Users/stronk7/git_moodle/moodle/question/engine/simpletest/testdatalib.php line 45]
Fail: question/engine/simpletest/testdatalib.php / ▶ qubaid_condition_test / ▶ test_qubaid_list_one_join
Equal expectation fails as key list [qubaid8, slot] does not match key list [qubaid212, slot] at [/Users/stronk7/git_moodle/moodle/question/engine/simpletest/testdatalib.php line 49]

And then tons of debugging information about "open box at" and "close box at" + some "Coding error detected, it must be fixed by a programmer".

Curiously running question/engine/simpletest/testdatalib.php alone reports 16 passes, 0 fails a 0 exceptions. So something else is causing that test to fail.

D4: run all unit tests under all DBs
D5: Some problems with trailing spaces and unit tests caused by html2text(). Those tests should be independent of the converter behavior.

This has been fixed @ qe2_integration by adding method to bring html2text independency. Was tempted to implement own assertEqual(), but finally decided to leave that unmodified and be more explicit.

QA tests

E1: We need really complex upgrades to be run under all DBs, using all qtypes and configurations missing questions and all sort of (BIG) stuff.
E2: Both qtypes/qbehaviors and quiz must be fully tested with/without JS.

Integration

F1: On integration, must we move all the upgrade steps (core mainly, not needed for types/quiz) to current dates? And also all the $plugin->requires to current week.
F2: On integration, must we respect current history? Any special interest? NP here at all, just asking.
F3: qeupgradehelper must go to 20_STABLE too, if it's a pre-helper.. should be there IMO. Yes?

Questions

G1: What happens with randomsamatch? Delete? Move all uses to missingtype on upgrade? All the rest are fully supported?
G2: After chat I got the idea (incorrect after looking deep) about random not being anymore real qtype but just one functionality in the quiz module (quiz picks), but they continue being created in question banks? Do we really need them there? If this happens, why is this different from the randomsamatch (deleted/unsupported) case?
G3: Shouldn't we have some way to specify which qtypes (and behaviors too, perhaps) can be used (supported) by one module? If some day we want lesson (or survey) using all that question engine machinery it's possible to imagine some types not being suitable for all modules or also, some behaviors being exclusive, or all the grading stuff being completely optional or different visualization per module… just to name some potential differences. Needs thinking, not now surely, but.
G4: Old question_states/sessions… shouldn't be 100% out? At least on install? Does the option to avoid upgrading all attempts prevent its deletion on upgrade? Or do we want there for 2.0 => 2.1 restore?

Core

H1: Switching qtype and mod order, I think it's ok. Can anybody imagine one situation with this leading to problem? Isn't this much the chicken and the egg problem? This change should be commented in code to avoid future modifications and, perhaps, a two/three-stage upgrade (pre/post) should be introduced to avoid problems like this in 2.2 or 2.3
H2: And, of course, do we need qbehavior also to be before mod? Seems to be the same case.
H3: Anything to mark as deprecate in current implementation (planned to be out on 2.2/2.3…) ? For example "xxx_question_list_instances()". (git diff master...qe2_wip | grep legacy)

Some details

I1: Silly: QTYPE occurrences
I2: Saw this notice:

You should set baseurl when using flexible_table.
line 481 of /lib/tablelib.php: call to debugging()
line 198 of /admin/qtypes.php: call to flexible_table->setup()

I3: Silly: question/upgrade.php: Personally don't like that name (as far as is used by standard db/ stuff)
I4: admin_setting_question_behaviour() shouldn't be quiz only? There are other settings, also potentially of application to other modules (the review one, and that is in quiz/settingslib.php instead)
I5: Silly: lib/questionlib.php or question/lib.php ?
I6: Usability (simple): One thing always seems incorrect here is why, when editing one question bank (within one quiz activity), the default category goes to course category and not to module category. Same happens when editing one quiz, always default to course cat.
I7: Reflexion: question/engine/bank.php has a few static members. While it's ok 99.99% of the time… we should provide sort of "resetters" for them or so. Or are we 100% sure the config ($questionconfig) or other bits aren't going to change ever along the request under any circumstance? Same for "$loadedbehaviours" in question_engine class?
I8: Reflexion: Bitwise operations are so, so, so unreadable, at least in my (limited) mind. I need pen and paper to solve them, grrr. And quiz is really plagued!
I9: Reflexion: Make 2.3 to require 2.2 so we can "cut" there again the upgrade scripts.
I10: Offtopic: Plz stop using that bloody line-wrapper in your IDE. It has messed all the quiz/core/qtype upgrade.php scripts, it should NOT being "fixing" old code at all. Really makes comparison, reviewing and patching way more difficult.
I11: To avoid being forgotten (unrelated): http://tracker.moodle.org/browse/MDL-16094?focusedCommentId=112034&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-112034
~~I12~~: Some whitespace detected here and there.

fixed

Conclusions

Z1: We need all the points in Section A (Installation and Upgrading) and section F (Integration), fixed/decided before integration. The rest can arrive later.
Z2: Let's strike references above as they get fixed/decided and commented inline.
Z3: +1 to put it in asap. It is pretty cool code, brilliant job! (awesome, congrats :-P)