Question Engine 2:Review 2011-05
From MoodleDocs
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: behaviours 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?
- I guess we need to fix this, but can we assign it to david, since he wrote that bit of the upgrade screens.--Tim Hunt 22:53, 6 June 2011 (WST)
- 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?
- I don't think that following the coding guidelines is silly. Before 2.0 was finished, Petr said that in future we would only support upgrade from one major version to the next. That is, 1.9 -> 2.0 -> 2.1 is supported, but 1.9 -> 2.1 is not supported. If that information is incorrect, then we need to fix quiz_statistics upgrade, which is easy. Just make direct upgrade 1.9 -> 2.1 use proper quiz_reports table name from the start.
- 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 --Eloy Lafuente (stronk7) 16:44, 6 June 2011 (WST)
- --Tim Hunt 22:53, 6 June 2011 (WST)
- 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.
- This is correct. In Moodle, all table names should start with the frankenstyle name of the plugin that owns them. So, only the core question tables should start question_, the qtype tables should start qtype_. Most of the existing qtypes get this wrong, but I think we should get it right in future, so I used the right name for the new essay table. In 2.2 I may consider fixing the table names for all the other core qtypes, but I decided not to do that in 2.1.--Tim Hunt 22:53, 6 June 2011 (WST)
- 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.
- This is for the benefit of users who installed old versions of my qe2_wip branch before I wrote the upgrade code. It does not harm upgrade from 2.0, so I think it is safer to leave it. However, if you want to remove it, we can.--Tim Hunt 22:53, 6 June 2011 (WST)
- A7: Do we have protection for timeouts / progress on potentially LONG steps? convert_all_quiz_attempts() seems ok, but… needed in more places?
- It should do, everything worked when we ran the upgrade at the OU (our 1.9 -> 1.9 + question engine). However, my upgrade code is trying to use the progress-bar class, and that seems to be broken.--Tim Hunt 22:53, 6 June 2011 (WST)
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. --Eloy Lafuente (stronk7) 16:45, 6 June 2011 (WST)
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.
- Done, review. Seems to be working ok here --Eloy Lafuente (stronk7) 16:44, 6 June 2011 (WST)
F2: On integration, must we respect current history? Any special interest? NP here at all, just asking.
- Forget, the whole history will be preserved --Eloy Lafuente (stronk7) 16:44, 6 June 2011 (WST)
- 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 --Eloy Lafuente (stronk7) 16:45, 6 June 2011 (WST)
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)
