|
History
| Log In
|
|
|
|
Thanks so much! Will review soon (right after the freeze)!
Great - thanks. I can't start work on the second part (controlling activity availability) until this one is in, so the sooner the better, and good luck with the freeze. :)
[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.] Any chance of progress on this now that 1.9.2 is out? I'm concerned about the speed of other changes which may cause merge problems if I don't get this code in soon.
I am not able to start another task during the next 14 days, sorry :-(
OK. Perhaps somebody else could review it?
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. Hi Sam,
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-) Thanks Eloy!
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...) 1) Ah, for numbers up to 100 all you need is INT(3) then (if to save space is the objective, the number is the number of digits in xmldb slang).
2-3-4-6-7) (y) 5) sniff, sniff (np, anyway). Here it's one more vote from Step Pilgrim: +1 :-D Ciao :-) I have now committed this revised patch (attached) to Moodle HEAD, plus the required icon files which aren't in the patch on account of being binary.
Not going to resolve the bug quite yet as I want to add completion support (at least 'view') to some other Moodle modules. 1/ grade_grade::notify_changed()
the global $restore is a hack, what is we have some other $restore global elsewhere? 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;-) btw, does it work with "login as" and "switch role"?
7/ mod/forum/mod_form.php
function get_data($slashed=true) { magic quotes related code and params were already removed 8/ forum_supports($feature)
maybe we could return null is module does not know and false if 100% not supported, it might come handy later I think 1/ agreed to leave with TODO as we don't have a better solution yet
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. 9/ some view.php
+ print '<script type="text/javascript" src="completion.js"></script>'. + '<script type="text/javascript">completion_strsaved="'.get_string('saved','completion').'"</script>'; + } does this validate? 10/ oops, this is not intentional, right?
header('Content-Type: text/csv; charset=ISO-8859-1'); from progress report 11/ progress report does not obey ajax enable switch
9/ that's in course/view.php and yes it validates.
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? 12/
/moodle/mod/forum/db/upgrade.php old xmldb syntax 10/ I have changed progress report to offer two flavours of csv, proper csv (UTF8-) and Excel CSV (UTF16LE with byte-order mark and tab instead of comma)
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) Over the past few days I have made several tweaks based on bug and usability reports from our testing here (we are about to release our 1.9 version of this code to students). These mostly don't change the way it works; the most significant was to add a help button alongside the first progress icon that appears to students. Quite important otherwise they might not know what the icons are for!
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. Hi,
I am PhD student at the University of Lyon1 (France), and I work with Pr. Alain Mille (http://liris.cnrs.fr/~amille/). In our work, we are interest to use traces issue from learning platforms to study collaborative activities between learners. We use tracking's engineering based on models, and we use Moodle database to collect and modelling traces. 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 .... |
|||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Sub-task
Resolved
Minor
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.