This does not include support for more module types. I intend to add support for the 'view' completion condition to most modules. That will be done after the patch is checked in. Similarly, I think there are some missing help files, I will add these.

The patch does not include binaries; there are some new icons (as described in the design) which I will check in. At present the icons are produced by me, I hope to replace them with graphic-designed ones if appropriate.

As requested I have added unit testing and modified the system so that it is object-oriented (sort of) and therefore easier to test. I have also added a manual test procedure (.txt file) that can be carried out by QA staff.

The unit test completes successfully. At present, the manual test procedure cannot be completed because there is a bug in quiz that prevents you submitting a quiz. If you skip the quiz-related parts, it works.

I realise this patch is very long (168KB) - precisely for that reason I'd like to check it in asap (perhaps even if there are minor problems) as keeping it merged may be a challenge.

[I'm not twiddling my thumbs, there's plenty of local stuff to do - just that if I don't do the availability bit soonish, I'll have to mess about / argue a bit to get it scheduled later.]

If nobody is available say to get it reviewed within say a week from now at most, then I would suggest that I get Tim to review the code and, after that, check it in - we can still make changes and undo things once it is in CVS.

I just did a quick-quick review... to see how it's looking. Some things:

1) Have seen some INT(9) in the patch. Shouldn't them be INT(10) ?
2) stupid.php (I hope it wasn't a easter egg for reviewers!) :-D
3) I suppose that all the .js in the patch is optional, correct?
4) Those exceptions in forum/lib.php forum_get_completion_state() and so on.... I think we need final decision. I'd continue using print_error() for now, until decided.
5) Didn't we talked about to do completion conditions pluggable? Sure I'm wrong and I dreamed it, but.
6) In backup & restore... are completiongradeitemnumbers recoded and so? I didn't find it.
7) I like it (tests/freeze/plugin_supports...).

my +1 for this (one more vote would be perfect). Great code! B-)

1) I normally use INT(9) for small numbers that I am expecting to be <100 as it saves space etc (on 32-bit systems anyway). Is that bad? I can make them INT(10) if you like. All the Moodle IDs are or should be INT(10).

2) oops

3) correct - the JS is used mainly for students to tick checkboxes, if you don't have JS then this works also (goes to another http, redirects back).

4) OK, those exceptions were temporary anyway (don't use proper moodle exception class, because it doesn't/didn't exist). What I'll do is change all the exceptions to call a temporary, deprecated completion_throw_exception function that actually just calls print_error for now. Then we can search/replace the calls to that function with real throw whateverexception once exceptions are agreed.

5) We did but I thought it made the API too complicated and wasn't helpful for module implementors mainly because it wasn't really going to be possible to do it independent of other module code. I think Martin was ok with that (he was reading those emails).

6) Gradeitemnumbers don't need recoding because it's an index number that begins with 0 (in fact it is basically always 0) and not an id.

7) yay

I really, really to commit this stuff on Monday at latest as I am running out ot time allocated to this project. So if somebody can give that other +1 (even if it's just based on eloy's...)

2-3-4-6-7)

5) sniff, sniff (np, anyway).

Here it's one more vote from Step Pilgrim: +1 :-D

Ciao

Not going to resolve the bug quite yet as I want to add completion support (at least 'view') to some other Moodle modules.

2/ grade_grade::notify_changed()
if(!empty($COURSE)) { $course=$COURSE; } else { $this->load_grade_item(); $course=get_record('course','id',$grade_item->courseid); }

this is incorrect, you have to test for $COURSE->id == $grade_item->courseid, not empty($COURSE); $COURSE always exists, you might get another course

3/ {$CFG->prefix}modules is not used anymore - use {moduels} instead

4/ grade_grade::notify_changed()
$cm=$DB->get_record_sql(", would get_coursemodule_from_instance() do the same?

5/ grade_grade::notify_changed()
$course=get_record('course','id',$grade_item->courseid);
grrrrrrrrr

6/ contructors should use __contruct() - not my idea

maybe we could return null is module does not know and false if 100% not supported, it might come handy later I think

2/ Fixed (also $grade_item was wrong, should have been $this->grade_item)

3/ Fixed here and in other parts of my checkin.

4/ Yes. Fixed.

5/ Ooops. Why didn't that show a warning or something? I have debugging on and my test procedure includes this code... maybe it redirected too quick to see...?... anyway, fixed.

6/ Oh? OK, fixed.

switch role: yes but not relevant (the completion icons do not depend on role, all roles see them)

login as: didn't work, but does now (I just made it clear the cache when it 'reads' data if userid has changed)

7/ Fixed (sorry did not notice this in port to 2.0)

8/ Agreed, could be useful if we need any 'default-true' behaviour or something. changed in forum/quiz/moodlelib function/phpdoc

also I moved constants to top of moodlelib, that was just an oversight.

does this validate?

10/ yes it is intentional (note csv_quote function that uses textlib to convert to ascii). I know this is crap but Excel did not at all like csv files in utf-8 and excel is what most people will open them in...

11/ there is no ajax in progress report?

old xmldb syntax

11/ Moodle uses ajax to mean 'or any advanced javascript', I didn't know that. I have changed it to use ajaxenabled (it still requires Firefox for svg support, this is done in the ajaxenabled check now). I left in the screenreader check as well because I don't think the SVG text is probably screenreadable.

12/ fixed (I reran that upgrade, seems ok)

I think this part of the task is finished now (hm maybe the setting should be moved from experimental at some point - but other than that), so resolving fixed, I will use separate bugs if there are other problems with it.

http://liris.cnrs.fr/~amille/

I am very interest about what you do, and I hope to contribute in your work, and also use what you do to ameliorate my works. At the moment I develop a tool which connect to Moodle, collect data, and then analyse learners activities...

my Email is: tarek.djouad@liris.cnrs.fr
....

Even if there was a working patch it wouldn't be safe to install for anyone who isn't comfortable with developing new Moodle code. And if you are comfortable, it would probably be just as easy to backport it yourself based on the standard-Moodle-2.0 patch included here rather than try to convert the OU-Moodle-1.9 patch.

Should anyone create a working 1.9 patch in that way, do post here. (And I guess if you really really want a copy of my OU Moodle 1.9 patch to work from, despite me telling you it'll be useless, ask.)