Moodle

Informal list of subtasks:

1. Refactor existing code into subplugins with minimal changes..
2. Make quizaccess_plagiarismcheckbox contrib plugin to drive further changes.
3. Verify subplugin DB tables get created.
4. A way to get a new field on the quiz settings form.
5. Save the setting when the form is submitted.
6. Load the setting when the quiz is re-edited.
7. Load the setting when a quiz attempt is started.
8. Refactor password check into renderer and to be pluggable.
9. Clean up secure window initialisation.

10. Separate out unit tests.
11. Move lang strings?

Done so far: 1. 2. 3. 10.

Work is proceeding on this branch: https://github.com/timhunt/moodle/commits/MDL-29627

12. handle rule instance creation so that MDL-13592 would be fixable with pure contrib code.

4. 5. and 6. done.

7. and 9. done. 9. involved major untangling! So, what is left is:

8. Refactor password check into renderer and to be pluggable.
11. Move lang strings?
12. handle rule instance creation so that MDL-13592 would be fixable with pure contrib code. Also cope with password rule tracking $canpreview on construct.

New one

13. mod_quiz_renderer::view_table logic needs to be moved to view.php.

12. Done.

New one todo:

14. replace numerical constants for $quiz->popup with string constants.

14. done.

https://github.com/timhunt/moodle-local_codechecker/commits/getstring is a nice tool for 11.

8. Done.
11. Done.

13. I will deal with later. MDL-29699

So this should now be ready for code review.

Colin, Mahmoud, please could one (or both) of you review these changes for me.

diff-stat currently looks like
67 files changed, 3680 insertions, 1828 deletions

Drat! I forgot a bit: When you backup and restore a quiz, any quizaccess settings need to be backed up and restored too.

OK, backup and restore now implemented (further commits on the same branches). So, once again I think this is ready for review.

1/ the changes in lib/moodlelib.php look ok to me
2/ the standard plugin info is missing in lib/pluginlib.php, method plugin_manager::standard_plugins_list(), right?
3/ ping David if it gets into integration, he may need to manually register new subplugin types in AMOS
4/ I do not understand the quiz internals much, but the more plugins the better imo, so my +1

Ciao

1. The Moodlelib change is actually MDL-29625 which is getting integrated this week. Eloy made a few good suggestions so what got integrated is slightly better than what is in this branch.

2. Doh! I forgot about this. Will fix tomorrow.

3. Also, thanks for reminding me. (Hopefully AMOS uses get_plugin_types, or pluginlib.php, or something, so it all magically works.)

4. Thanks.

2. Done now.

3. David, I just added you as a watcher. Can you comment? Thanks. (You might also like to review the last commit in my branch.)

Hopefully AMOS uses get_plugin_types, or pluginlib.php, or something, so it all magically works.

Unfortunately not yet because AMOS tracks all Moodle branches and it seemed to be pretty difficult to get the information from all 1.x and 2.0 branch when AMOS was developed. So as a temporary solution, there is an XML file https://github.com/moodlehq/moodle-local_amos/blob/master/plugins.xml that aggregates the information about standard plugins from all 2.x branches. That is suboptimal solution, of course. It would help if we had such a list in a XML file in the core for each branch so that AMOS could just concatenate them. That is, instead of having the standard plugins whitelisted in PHP code, have the white-list in a separate XML file parse-able by external tools (like AMOS). For now, I just have to copy the information from the 16330ed788e422a4bbf7306f47668a1e396d11e7 into that file in AMOS.

From my point of view, the branch integrates new plugin type correctly (I guess Tim actually testes and saw the rules listed at the Plugins overview page well) and detected strings are MOV'ed into the subplugin which is great. +1

Oh I just realized that the AMOS issue was actually regarding plugintype, not the list of standard plugins. Well, that should work blindly. Of course AMOS can't execute master code to recognize the subplugintype. But it just blindly expects that any file modified in moodle.git with a path like */lang/en/xxx.php is a strings file for the component xxx. So it should register it.

There was a bug that prevented new admintools detected but that one should be fixed. So this is actually a good opportunity to test it. I will leave AMOS untouched and will see what happens

The changes in lib/moodlelib.php looks fine and the function get_plugin_list_with_file() is very useful

The calss quiz_access_manager provides lots of helpful methods.

In mod/quiz/accessmanager.php, in method make_review_link(), instead using an else branch I would just return whatever is returned in else branch at the bottom of this method.

In mod/quiz/attemptlib.php file, in method cannot_review_message(), instead of:
if () { return ...; } else if { return ...; }
} else { return ...; }

I would use something like
if () { return ...; }
}
if { return ...; }
return ...;

In this instance I do not see any differences in performance, but I think it is easier to read the code.

Overall, very useful changes for developers.

I agree my code style with if statements is inconsistent. I actually think that which style is best depends on the particular situation. That is, I would code like

if (test) {
    return possibility1;
} else {
    return possibility2;
}

but I would write

if (test) {
    return exceptional_or_null_case;
}
return normal_case;

Anyway, I looked again, and decided not to make any further changes at this stage.

Submitting for integration. Thanks, Mahmoud, Petr for your reviews.

Developer docs for writing one of these sub-plugins: http://docs.moodle.org/dev/Quiz_access_rules.

Nice one, reviewing...

btw, hate when you add a lot of "noise" about changing wrapping without further changes, grrr.

Some comments, surely none (but 5, tiny detail) preventing integration:

1) In various places you are "accumulating" into keyed arrays different things (browser_security_choices, sql_params, settings...) and I think it's ok, but if for some circumstance sort of reuse (in brand-new plugin implementation) of the same keys happens. Perhaps it would be safer to add some unique prefix to every element or perform some check looking for duplicates, or keep those elements under its rule boundaries, not using them to populate external arrays... I know this is hardly going to happen but seems possible.

2) All files are © Open University but mod/quiz/accessmanager.php that is © Tim

3) The dots in comments, lol, I'm about to ignite one war against them

4) On upgrade... won't be easy to make '-' the DB default and then only update old popup = 1 and 2 ? And keep the default enabled? I don't think leaving potentially some '[unknownvalue]' there is a good idea.

5) Also on upgrade step 2011100604 isn't the switch...case statement missing breaks?

6) Once again, plz avoid mixing unrelated wrapping changes with the fix it really makes things harder here. It's complex enough without all those changes, hehe.

7) Trying to execute 'mod/quiz/accessrule' unit tests ends with error (guess you need to include some missing stuff):

Fatal error: Class 'quiz' not found in /mod/quiz/accessrule/delaybetweenattempts/simpletest/testrule.php on line 49

I think it looks interesting and cuasi-perfect (although I don't like your coding style at all, pffff. :-P). Of course, there are a lot of code I don't understand but it is correct from a coding POV, fair enough. That and the abundance of unit tests aim me to integrate this after hearing from you above the points above, thanks!

Ciao

1) There is a weak convention that the keys start with the plugin name. That is true for browser_security choices, for example. See, also, the PHP docs on quiz_access_rule_base::get_settings_sql. However it is not strongly enforced, which may give useful flexibility one day. As you say, the chance of a real name collision happening is almost zero.

2) accessmanager.php was written in 2008 while I was working at Moodle HQ. Therefore, not (c) OU.

3) Grrr! I disagree, as we just discussed in HQ chat. Sentences should end in a full stop.

4) I disagree. I want the NOT NULL constraint to catch the bug if the code forgets to set the field. I also don't want to silently overwrite weird custom values in the DB. I want people who have written custom code hacks to know that the code changes have broken them. So, I will not make any change here.

5) Oops! Fixed.

6) I think ultimately I care more that the code ends up clean for the future, than how difficult the change is to review today. However, I will try to be kinder to integrators in the future.

7) Oops. I always just run all the tests in mod/quiz, since it is fast. I will make sure that the tests are independent. Fixed.

Right. One more commit added to the branch.

Integrated thanks! All unit tests are now executable separately and passing.

the commit from here is proving to break quiz>edit settings in integration, heres a patch for consideration (Tim's review too) :
https://github.com/nebgor/moodle/compare/integration_master...MDL-29627
https://github.com/nebgor/moodle/commit/bd3ce6c4a3ad07cb6bec9046cbb4bd731b03596d

Sorry, Apu, but have you read this bug? Which commit from here, there are lots.

Anyway, I'm taking a look at this now.

Admittedly i didn't read this entire bug, i didn't have the time but i hope this helps :
dd70d561 mod/quiz/accessmanager.php (Tim Hunt 2011-10-05 15:54:57 +0100 187) list($sql, $params) = self::get_load_sql($quizid, $rules, '');

Fixing workflow state.

As commented by Apu above.

New commit on the branch that should fix this:

https://github.com/timhunt/moodle/commit/e4df79dd9ce2f0a81075aafcaaa6c446387c6204

I was, of course, only testing with my quizaccess_honestycheck plugin installed. I should have remembered to test without.

Please integrate my patch, and then put this back into testing for more thorough scrutiny. Thanks.

New commit added and issue reset to testing phase.

I ran into some problems testing this. They're not significant, but they should be addressed. The problems were functional ones relating to restrictions rather than error messages.

1. When I turned off JavaScript before starting a quiz with a time limit I was able to complete and submit answers. This is not a problem, but according to the documentation, I should not have received marks for the answers provided after the time limit. I set a 1min time limit. After a few minutes I submitted my answers. My attempt was marked and received marks. The Overdue time displayed was incorrect. I'll attach a screenshot. I tried this with 10min also and found a similar result.

2. I was not able to get the IP address restriction to work. I entered the IP address of the machine I was testing on and a partial address also, but it would not allow me to start the test, based on the IP address, until this setting was removed.

1. is a completely separate issue that I am aware of, but there is no very easy solution. (Non-easy solution coming in 2.3).

The overdue mis-display should be trivial to fix. It is another separate issue that is unaffected to the changes for this bug, so please file a separate MDL and I will fix it for next week.

2. Please can you test last week's weekly build. I am sure I used this rule in my own testing and it worked for me (at least as well as it worked in the past). The changes here don't change any of the existing functionality, they just refactor it. So, before I spend ages diving into the code, please can you quickly check that the IP stuff was working on your system last week.

The IP address restriction seems to behave the same in an older 2.1 version, so these changes aren't introducing this problem.

The message given is "This quiz is only accessible from certain locations, and this computer is not on the allowed list."

back to waiting for testing (as requested by Michael)

Passing as issues found during testing will be dealt with through new issues.

I ran some testing with the IP address issue. I got the same result with demo.moodle.net.

I ran the getremoteaddr() function and printed the result inside quizaccess_ipaddress() to see what address it was seeing.

On my local machine it was the server IP address 127.0.0.1, which is not what I expected. Adding that as an address in the IP address restriction field worked, so at least the restriction checking works.

I'm going to have to leave this with you. I'm off to the GSOC Mentor Summit tomorrow. I'm happy to run further tests.

Thanks for the extra testing Michael.

I don't see complaints about this working coming in via the quiz forum, so I think it is working for most people. Therefore, I will ignore this until we get more bug reports.

I tried this again from home without the complications of NAT and it worked. I may have got the HQ subnet incorrect, but I'll try it again when I get back.

Many thanks for all the hard work. This is now part of Moodle, your favorite LMS.

Closing as fixed, ciao

We already have these tests.