Details

    • Type: Sub-task
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Attachments

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

            Issue Links

              Activity

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

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

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

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

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

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

              Show
              skodak Petr Skoda added a comment - Thanks for the patch, looking at it now....
              Hide
              skodak Petr Skoda 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
              skodak Petr Skoda 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 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 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
              skodak Petr Skoda added a comment -

              Thanks.

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

              Show
              skodak Petr Skoda added a comment - Thanks. I would personally prefer to refactor core first and commit this later.
              Hide
              jb23347 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
              jb23347 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
              scyrma Mathieu Petit-Clair added a comment -

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

              Show
              scyrma Mathieu Petit-Clair added a comment - Here's a new patch, to prevent conflicts, as I commited the fix for MDL-12565 .
              Hide
              jb23347 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
              jb23347 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
              jb23347 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
              jb23347 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              dougiamas 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
              dougiamas 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
              jb23347 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
              jb23347 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
              dougiamas 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
              dougiamas 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
              jb23347 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
              jb23347 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
              jb23347 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
              jb23347 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
              scyrma 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
              scyrma 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
              scyrma 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
              scyrma 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
              jb23347 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
              jb23347 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
              scyrma 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
              scyrma 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
              jb23347 John Beedell added a comment -

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

              Show
              jb23347 John Beedell added a comment - Well spotted - I've updated coursetagslib.php with this latest patch.
              Hide
              jb23347 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
              jb23347 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
              scyrma 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
              scyrma 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
              scyrma 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
              scyrma 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:
                    Fix Release Date:
                    24/Nov/10