Note:

If you want to create a new page for developers, you should create it on the Moodle Developer Resource site.

Question Engine 2:Review 2011-05

From MoodleDocs

Basic References

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.

  1. Read all the Docs
  2. Read the MDL
  3. Create Oracle DB for that (stronk7_qereview) (yes, it's the best testing env, there is some serious rationale about that)
  4. Look @ DB structures and installation.
  5. Review QE2 itself, understand tests
  6. Look for DB uses
  7. Look for Quiz changes related to QE2 support
  8. Review all requests, security, caps, user-introduced info specially.
  9. Upgrade (both quiz and engine)
  10. Backup & Restore (specially BC).
  11. Functional tests
  12. 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?
enable/disable/delete UI is MDL-27490. Many Moodle plugins without DB or capabilities do not bother with version.php files. I can, however, add them if required.--Tim Hunt 22:53, 6 June 2011 (WST)
  • 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.
Done https://github.com/timhunt/moodle/commit/6e56da8347eaa881841c8194b1d8db1dc4a154d6 --Tim Hunt 00:25, 7 June 2011 (WST)
  • 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)
I have reviewed Eloy's DB changes, and they all look good to me.
  • 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
Done (commit 030fba8)--Tim Hunt 00:31, 7 June 2011 (WST)
  • B2: Silly: backup and restore. Why "response/variable" ? Shouldn't be "responses/response" or "variables/variable" ?
I thought response/variable most accurately described what the data was.--Tim Hunt 00:31, 7 June 2011 (WST)
  • B3: convert 1.9 => 2.0 (quiz & qtypes)
Surely this is part of the 1.9 -> 2.0 upgrade project, not our problem here.--Tim Hunt 00:31, 7 June 2011 (WST)
  • B4: restore 2.0 => 2.1 (quiz & qtypes)
This is MDL-27639.--Tim Hunt 00:31, 7 June 2011 (WST)
  • B5: Support for missing type on 1.9/2.0/2.1 => 2.1 restore
This needs to be tested. It should work, but I would not be surprised if it was broken.--Tim Hunt 00:31, 7 June 2011 (WST)
  • B6: Do we need qbehavior plugin support in B & R? If so, where should it have "hook" points?
I think no. It is currently part of the design that qbehaviours do not have their own DB tables. I would need careful convincing that it was necessary before I allowed it.--Tim Hunt 00:31, 7 June 2011 (WST)

Documentation

  • C1: Teacher / admin documentation missing, explaining behaviors, qtypes, quiz, qeupgradehelper local extension...
Correct. I am hoping for volunteers to help write it (Tomaz, Helen, ...) I am happy to answer any questions from people trying to write docs, but I think my time is better spent on fixing bugs at the moment.--Tim Hunt 00:32, 7 June 2011 (WST)

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?.
Reviewing coverage would be good, however, it is a bit tricky because the qbehaviours and qtypes form a sort-of 2D matrix, so some of the qtype tests are in qbehaviours, and vice versa. All one can really say is that the tests in /question should cover 100% of the library code in /question. It is hard to break it down, but we could probably do better than we are now. I need to read and apply Unit_tests#Code_coverage_analysis. I made MDL-27739 for this.--Tim Hunt 02:04, 7 June 2011 (WST)
  • D2: running mod/quiz passes also ok, but coverage also needs better definition. Same for lib/simpletest/testquestionlib.php
As for D1. Also covered by MDL-27739.--Tim Hunt 02:04, 7 June 2011 (WST)
  • 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.
I think this was broken by the change that made get_in_or_equal use static $counter = 1; That is really bad for unit testing. It should use a class variable at least. I thought I had done a work-around. I don't understand why this breaks only on MySQL.--Tim Hunt 02:04, 7 June 2011 (WST)
  • D4: run all unit tests under all DBs
I cannot easily do this. Also, almost all unit tests are DB-independatent, so I do not expect this to find much. However, if you can do this for me, then it is worth doing.--Tim Hunt 02:04, 7 June 2011 (WST)
  • 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)
I made a slightly different fix using a new compare_qas method https://github.com/timhunt/moodle/commit/4c5743343fc01185cd95ea296954e52134ea9e40 --Tim Hunt 02:04, 7 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.
I started writing Question_Engine_2:Testing to help people know what to test. I have not yet written the upgrade section.--Tim Hunt 00:40, 7 June 2011 (WST)
  • E2: Both qtypes/qbehaviors and quiz must be fully tested with/without JS.
I believe everything works fine with, and without, JS, except for the multianswer qtype, which has requried JS since forever. 2.1 is no worse than 2.0 here.--Tim Hunt 00:40, 7 June 2011 (WST)

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?
I think we should ask some typical Moodle admins. I do not have strong opinions myself.--Tim Hunt 00:34, 7 June 2011 (WST)
Requested MD for some feedback from admins / partners

Questions

  • G1: What happens with randomsamatch? Delete? Move all uses to missingtype on upgrade? All the rest are fully supported?
MDL-27414 exists to track this.--Tim Hunt 01:44, 7 June 2011 (WST)
  • 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?
The point is that random qtype does not do anything at quiz attempt time (there is no quetsion.php file/class). It is replaced by a real question at the time that the quiz_attempt / quba is created. The difference with randomsamatch is that it would have to have a question.php file/class that would have to randomise and load other questions from the DB during the attempt.--Tim Hunt 01:44, 7 June 2011 (WST)
  • 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.
There is a natural place to add this (methods on the question_bank or question_engine classes) if we ever need it. Until we need it, and know exactly what we do need, I would rather not make up methods based on guesses of what we might need in the future.--Tim Hunt 01:44, 7 June 2011 (WST)
  • 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?
This is because of Petr's rule that install.xml and result of upgrade.php must match 100%. Therefore, if we want to keep those tables after upgrade (we do, useful insurance, and it allows the qeupgradehelper reset / redo upgrade feature to work - not needed for restore) then we must create them on new install. We can remove them in 2.x.

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
mods use qtypes, qtypes are really an extension of core, so it makes sense to do them first. I cannot imagine any problem arising.--Tim Hunt 00:47, 7 June 2011 (WST)
  • H2: And, of course, do we need qbehavior also to be before mod? Seems to be the same case.
qbehaviour plugins should not have db tables (or anything else), so it does not matter where they are in the order.--Tim Hunt 00:47, 7 June 2011 (WST)
  • 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)
Good question, but I think I marked everything deprecated things as I went along. There are no xxx_question_list_instances functions anywhere in Moodle 2.1.--Tim Hunt 00:47, 7 June 2011 (WST)

Some details

  • I1: Silly: QTYPE occurrences
Removed some in commit 70d2481.--Tim Hunt 01:28, 7 June 2011 (WST)
  • 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()
Fixed d9eb954--Tim Hunt 01:28, 7 June 2011 (WST)
Another 'silly' question to consider later, should this be admin/qtypes/php, or question/qtypeadmin.php?--Tim Hunt 01:28, 7 June 2011 (WST)
  • I3: Silly: question/upgrade.php: Personally don't like that name (as far as is used by standard db/ stuff)
That has been there since before 1.9. What would you suggest as a better name?--Tim Hunt 01:28, 7 June 2011 (WST)
  • 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)
No. In future it will be used by other modules. I think the review one is more quiz-specific. I would not encourage other modules to use bitfields like this.--Tim Hunt 01:28, 7 June 2011 (WST)
  • I5: Silly: lib/questionlib.php or question/lib.php ?
I agree, but as Martin. He is the one who likes lib/groupslib.php, lib/gradeslib.php, etc. I think we should not change this now.--Tim Hunt 01:28, 7 June 2011 (WST)
  • 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.
This is been like this for a long time, and I think now is not the time to change it. (I have plans to change the question bank as my next big change).--Tim Hunt 01:28, 7 June 2011 (WST)
  • 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?
Easy to add resetters later if there is ever a need. Until there is a need, I will leave it.--Tim Hunt 01:28, 7 June 2011 (WST)
  • 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!
Yes, but there are fewer in 2.1 than in 2.0, and without any bitwise operations, we would need 28 DB columns where currently we have 7 reviewxxx columns.--Tim Hunt 01:28, 7 June 2011 (WST)
  • I9: Reflexion: Make 2.3 to require 2.2 so we can "cut" there again the upgrade scripts.
I think this decision is not really related to QE2.--Tim Hunt 01:28, 7 June 2011 (WST)
  • 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.
Actually, I did that manually in response to local/codechecker. Actually, it makes it much easier to read the code if you always wrap before null/XMLDB_NOTNULL. However I agree it makes the diff impossible to read. Sorry.--Tim Hunt 01:28, 7 June 2011 (WST)
Reported and fixed as MDL-27737--Tim Hunt 01:28, 7 June 2011 (WST)
  • I12: Some whitespace detected here and there.
fixed --Eloy Lafuente (stronk7) 16:45, 6 June 2011 (WST)
Grrrrr! I can't believe I did not catch this myself--Tim Hunt 01:28, 7 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)

Responses from Tim

Eloy,

I think I have added comments to all the points above (apart from the ones you had already done).

I have not added strike to the things I think are dealt with. I was hoping that you would review all the comments I made above (and code changes, new MDL issues where appropriate) and add strike-out to the ones you are happy with.

The latest code has been pushed to https://github.com/timhunt/moodle/commits/MDL-20636_master_new_question_engine (and I updated the links above).

I am about to go home now, but will probably look at this later today and tomorrow morning.

Tim.

Retrieved from "https://docs.moodle.org/dev/index.php?title=Question_Engine_2:Review_2011-05&oldid=24362"
Powered by MediaWiki