Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Tags
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      33726

      Description

      This will extend the 1.9 tagging features (tagging users and blogs) to allow users to tag courses. See http://openlearn.open.ac.uk for a demo of roughly how it will work (post Jan - not live just as I write!)

      Essentially the tag block will also cover course tags, and the tag display will have an extra section listing course tags with a display similar to the course search screen.

      This will also add the ability to toggle the view of tags to "my tags", "official tags", "community tags" and "all tags" in the block for better personalisation.

      1. 11992coursetagpatch.txt
        102 kB
        John Beedell
      2. 11992-coursetagspatch.txt
        104 kB
        Mathieu Petit-Clair
      3. coursetags_20080613_a.txt
        85 kB
        John Beedell
      4. coursetags_20080613_a.txt
        85 kB
        John Beedell
      5. coursetags_20080613_a.txt
        80 kB
        John Beedell
      6. coursetags_20080619.txt
        82 kB
        Mathieu Petit-Clair
      7. coursetags_may08.txt
        88 kB
        John Beedell
      8. coursetagspatch.txt
        87 kB
        John Beedell
      9. coursetagspatch.txt
        84 kB
        John Beedell
      10. coursetagspatch.txt
        105 kB
        John Beedell
      1. coursetags.jpg
        32 kB
      2. screenshot-1.jpg
        32 kB

        Issue Links

          Activity

          Hide
          John Beedell added a comment -

          I am considering committing the course tagging feature to head in January, so I am attaching a patch created today for the adventurous to try out.

          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-12565 bug.

          Show
          John Beedell added a comment - I am considering committing the course tagging feature to head in January, so I am attaching a patch created today for the adventurous to try out. 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-12565 bug.
          Hide
          John Beedell added a comment -

          A patch for head to include course tagging extension to the tags block.

          Show
          John Beedell added a comment - A patch for head to include course tagging extension to the tags block.
          Hide
          John Beedell added a comment -

          The latest patch for HEAD/2.0 created 9 January 08, includes one minor adjustment over earlier patch.

          Show
          John Beedell added a comment - The latest patch for HEAD/2.0 created 9 January 08, includes one minor adjustment over earlier patch.
          Hide
          Petr Škoda added a comment -

          Thanks for the patch, looking at it now....

          Show
          Petr Škoda added a comment - Thanks for the patch, looking at it now....
          Hide
          Petr Škoda added a comment -

          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.

          Show
          Petr Škoda added a comment - 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.
          Hide
          Jenny Gray added a comment -

          >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

          Show
          Jenny Gray added a comment - >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
          Hide
          Petr Škoda added a comment -

          Thanks.

          I would personally prefer to refactor core first and commit this later.

          Show
          Petr Škoda added a comment - Thanks. I would personally prefer to refactor core first and commit this later.
          Hide
          John Beedell added a comment -

          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?

          Show
          John Beedell added a comment - 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?
          Hide
          Mathieu Petit-Clair added a comment -

          Here's a new patch, to prevent conflicts, as I commited the fix for MDL-12565.

          Show
          Mathieu Petit-Clair added a comment - Here's a new patch, to prevent conflicts, as I commited the fix for MDL-12565 .
          Hide
          John Beedell added a comment -

          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?

          Show
          John Beedell added a comment - 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?
          Hide
          John Beedell added a comment -

          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.

          Show
          John Beedell added a comment - 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.
          Hide
          Petr Škoda added a comment -

          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.

          Show
          Petr Škoda added a comment - 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.
          Hide
          Martin Dougiamas added a comment -

          Mathieu has started looking at cleaning up the tags API already (yesterday) WRT

          • the dual functions of the parameters (id or tag) and
          • clarifying the external API of tag_xxxx functions
          Show
          Martin Dougiamas added a comment - Mathieu has started looking at cleaning up the tags API already (yesterday) WRT the dual functions of the parameters (id or tag) and clarifying the external API of tag_xxxx functions
          Hide
          John Beedell added a comment -

          Please find a new patch for adding course tagging to moodle 2.0 (HEAD) as at 1st April 08. Any comments welcome of course!

          Show
          John Beedell added a comment - Please find a new patch for adding course tagging to moodle 2.0 (HEAD) as at 1st April 08. Any comments welcome of course!
          Hide
          Martin Dougiamas added a comment -

          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

          Show
          Martin Dougiamas added a comment - 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
          Hide
          John Beedell added a comment -

          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.

          Show
          John Beedell added a comment - 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.
          Hide
          John Beedell added a comment -

          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.

          Show
          John Beedell added a comment - 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.
          Hide
          Mathieu Petit-Clair added a comment -

          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.

          Show
          Mathieu Petit-Clair added a comment - 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.
          Hide
          Mathieu Petit-Clair added a comment -

          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.

          Show
          Mathieu Petit-Clair added a comment - 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.
          Hide
          John Beedell added a comment -

          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
          blocks/tags/block_tags.php
          lang/en_utf8/tag.php
          lib/moodlelib.php
          tag/index.php, coursetagsedit.php, coursetagslib.php)

          Show
          John Beedell added a comment - 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 blocks/tags/block_tags.php lang/en_utf8/tag.php lib/moodlelib.php tag/index.php, coursetagsedit.php, coursetagslib.php)
          Hide
          Mathieu Petit-Clair added a comment -

          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..

          Show
          Mathieu Petit-Clair added a comment - 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..
          Hide
          John Beedell added a comment -

          Well spotted - I've updated coursetagslib.php with this latest patch.

          Show
          John Beedell added a comment - Well spotted - I've updated coursetagslib.php with this latest patch.
          Hide
          John Beedell added a comment -

          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).

          Show
          John Beedell added a comment - 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).
          Hide
          Mathieu Petit-Clair added a comment -

          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 ...

          Show
          Mathieu Petit-Clair added a comment - 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 ...
          Hide
          Mathieu Petit-Clair added a comment -

          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.

          Show
          Mathieu Petit-Clair added a comment - 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.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: