|
A patch for head to include course tagging extension to the tags block.
The latest patch for HEAD/2.0 created 9 January 08, includes one minor adjustment over earlier patch.
Thanks for the patch, looking at it now....
1/ oh, this in modolelib.php is not good:
/// Delete course tags require_once($CFG->dirroot.'/blocks/tags/coursetagslib.php'); coursetag_delete_course_tags($course->id); if ($showfeedback) { notify("$strdeleted course tags"); } code in moodlelib.php must not depend on blocks, I think that coursetagslib.php could be somewhere in course/lib.php instead. 2/ the html code in block is not xthml strict - incorrect single quotes, disabled attribute 3/ hardcoded lang strings: "Submit" in form, js warnings 4/ hardcoded siteid: courseid = optional_param('courseid', 1, PARAM_INT); 5/ no assignments to $COURSE, it must be done by require_login()! 6/ not compatible with multilang - use format_string on $course->fullname before printing it 7/ images should use pixpath (only works for standard images though) to allow theme tweaks 8/ it must work without js - does it? 9/ coding style: {} after if required In fact I do not like the access control+coding style in core tags code at all, I complained but the SoC student did not listen. The main tag related code should be fixed/refactored/improved/rewritten first IMO. I do like the idea of course tags! Please keep working on that. >The main tag related code should be fixed/refactored/improved/rewritten first IMO. I'm guessing the SoC student had the same problem we do - no time! I couldn't commit us to refactoring the whole thing I'm afraid, so if every-one is agreed that that is necessary, then the course tagging will have to wait for some-one else to get round to it. Or we commit course tagging now and the whole thing gets refactored later. The other points should be relatively quick to fix though. I don't seem to be able to actually assign this bug to John, but it'll be him that does the actual work (as indeed he has done up to now). Jenny Thanks.
I would personally prefer to refactor core first and commit this later. Many thanks Petr for reviewing the last patch so quickly.
I have created a new patch which has corrected all the issues you raised. On the question of whether the code works without javascript, the answer is yes and no. The block input autocomplete is functional, if not slick, without js. The block footer links will not work without js. The block footer links could be changed to make a new hit on the server each time (and this would avoid issues with js disabled browsers), but I felt that the light weight js was worth considering fully in this patch before going down that route. Of course the main tag code has not been refactored, and as Jenny has mentioned, unfortunately I cannot put time into that at the moment. Does anyone else have an opinion on Petr's suggestion? Here's a new patch, to prevent conflicts, as I commited the fix for
Thanks for the new patch and fixing that error in the tagging system. May I enquire whether you are likely to be working on a review of the access control+coding style in core tags code as outlined by Petr above?
It is the end of January, a month on from initial patch submission, and I am under some pressure to either commit or abandon this code.
This is a 'last call' for comments on this course tagging extension to the main tagging system. While Petr has raised a request for a review of access control and coding style on the current tagging system, there appears to be no other reason for not committing this course tagging extension code. I suggest that in the absence of any further comment I will commit this code on Wednesday 6th Feb 08. I am sorry, we are all working hard to get 1.9.0 as good as possible before the release. The new development phase towards 2.0 did not start yet, we are trying to keep the differences between 1.9 and HEAD as minimal as possible, because new features in HEAD make the merging much, much harder, not to mention the problems with database upgrade scripts.
My personal opinion is that the tags code is not good enough and that it should be improved first. The reason why I am telling this is that it is often me who ends up fixing/rewriting stuff like this - for example groups+groupings, user bulk operations and upload, administration menu, course reset, upgrade code, recent activity, statistics just to name a few from 1.9 branch only I was thinking about improving of the tags code in 1.9.1, any changes in HEAD would only make it harder. Please keep it here as a patch for now. Mathieu has started looking at cleaning up the tags API already (yesterday) WRT
Please find a new patch for adding course tagging to moodle 2.0 (HEAD) as at 1st April 08. Any comments welcome of course!
Mathieu, we need to examine this properly together when you get back.
Also consider how it works together with http://docs.moodle.org/en/Student_projects/Blog_improvements I've added a global configuration for the tags block to 'turn on' (or off) the course tagging bit in the block - sorry its taken me so long to get round to adding this. I've not added a bump to the version numbers this time, so the database upgrade in the code will not occur automatically. If anyone needs help with this please ask.
Please see two screen shots; of the course home page with the coursetagging added to the tags block, and the tags page with courses added.
I started updating the most recent patch to head... you can find it in file coursetags_june_08.zip. This includes changes to use the new dmllib, and there are still broken queries, but it's party usable.
coursetags_20080613.txt contains a working patch (completed migration to new database functions). To be discussed next Monday, hopefully to be checked-in soon after. This patch contains a database change to table tag_instance.
Thanks Mathieu for your work in you last patch.
I've tested it and found just a couple of minor bits (an odd </div> I'd left in for no good reason, and two entries of the same code in a couple of places!). I've also noticed that the patch process on my Eclipse is screwing up the arrow_left.gif image (the file exists, but its definately not an image that will display). So I've done another patch, coursetags_20080613_a.txt, to correct the code - and I will have to email you the image even though it is supposed to be in the patch. I would be very happy for this code to be committed, if it meets with your approval in the discussions on Monday. (Files changed over the last patch are I thought the DML conversion was completed, but there are still unconverted calls in a few files. This will need to be done before this patch get committed..
Well spotted - I've updated coursetagslib.php with this latest patch.
Another change in tag lib.php this morning -> another patch coursetags_20080613_a.txt (but this time I've been more careful with the XMLDB changes as well).
Ok, here's a new patch (in file ..._20060619.txt). This is probably not very different from 0618 (yesterday), but as I did some cleanup in the code, here's a new one.. just in case. I removed the old files (those I sent anyway) from this issue, as they are not useful anymore ...
Committed to CVS head.
We can now work on fixing the smaller integration issues... I created issue MDL-15471 for that purpose. I'm closing this right now, so that it get reviewed (and hopefully closed) tomorrow. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sub-task
Resolved
Critical
 |
|
Any comments would be gratefully received.
Please also see the forum discussion http://moodle.org/mod/forum/discuss.php?d=78985
, and http://moodle.org/mod/forum/discuss.php?d=79067
.
PS. The patch also covers the very minor
MDL-12565bug.