Details

    • Testing Instructions:
      Hide

      1. switch course tagging on in the tag block settings
      2. create two courses A and B.
      3. Add the tag block to the site home page and configure it to display throughout the site.
      4. Add tags 'one' and 'two' to course A, and 'two' and 'three' to course B. Do this by visiting the course, and typing each tag in turn in the tag block text entry field and pressing return. Note that as you start to type in the box, matching existing tags should appear for your re-use (this isn't autocomplete though, just a suggestion).
      5. Make tags 'one' 'two' and 'three' official thorugh the tag editing screen.
      6. Add tags 'four' and 'five' to course A, and 'five' and 'six' to course B. Leave these tags as default.
      7. Go to the site home page, you should see all 6 tags in the block. Click on a tag to view the tag's page showing what is tagged with that word.
      8. Edit the block settings and change the tagtype dropdown to 'official'. Save and recheck that only official tags are shown on the site home page in the block.
      9. Repeat 8 for default tags only.
      10. Log in as a different user, create a few more tags. Go to the 'my home' page and ensure that only tags you've just created (not one to six) are displayed.
      11. Log back in as admin, change the permissions on the authenticated user role so that that role cannot create or flag tags.
      12. Log back in as an ordinary user. Ensure that you can no longer add tags in a course or flag tags as inappropriate on the tag page.

      Show
      1. switch course tagging on in the tag block settings 2. create two courses A and B. 3. Add the tag block to the site home page and configure it to display throughout the site. 4. Add tags 'one' and 'two' to course A, and 'two' and 'three' to course B. Do this by visiting the course, and typing each tag in turn in the tag block text entry field and pressing return. Note that as you start to type in the box, matching existing tags should appear for your re-use (this isn't autocomplete though, just a suggestion). 5. Make tags 'one' 'two' and 'three' official thorugh the tag editing screen. 6. Add tags 'four' and 'five' to course A, and 'five' and 'six' to course B. Leave these tags as default. 7. Go to the site home page, you should see all 6 tags in the block. Click on a tag to view the tag's page showing what is tagged with that word. 8. Edit the block settings and change the tagtype dropdown to 'official'. Save and recheck that only official tags are shown on the site home page in the block. 9. Repeat 8 for default tags only. 10. Log in as a different user, create a few more tags. Go to the 'my home' page and ensure that only tags you've just created (not one to six) are displayed. 11. Log back in as admin, change the permissions on the authenticated user role so that that role cannot create or flag tags. 12. Log back in as an ordinary user. Ensure that you can no longer add tags in a course or flag tags as inappropriate on the tag page.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-15471
    • Rank:
      4135

      Description

      Following MDL-11992 commit, we will need to improve the integration of the initial course tagging patch with the rest of the tagging system. Among things that need to be improved:
      1) use of "hardcoded" font-style - course tagging uses dynamic css font-size plus: better mathematical distribution, minus: harder to theme) while tags uses s?? classes -,
      2) the choice of "tag type" in the block (all, mine, course, official, community)
      3) push back into /tag/lib.php as much common code as possible

      1. simplertags.diff
        14 kB
        Jenny Gray
      2. simplertags.diff
        7 kB
        Jenny Gray

        Activity

        Hide
        Mathieu Petit-Clair added a comment -

        Known issues:

        • it's still possible to have multiple blocks: looking at the code, it appears that it's a feature of blocklib that you can't make a "multiple instances" block become a "unique instance" block. I don't know if it's really useful to have multiple tags block on a given page.
        • the block displaying the course tags uses dynamically calculated font-size instead of css classes: both ways have advantages, we'll need to discuss it a bit more
        • it's possible to choose which "types" of tags to display in the block (using javascript links): we need to come up with a better way to present these (eg. "community tags" are a bit OU-specific)
        • help files are ongoing editing in MDL-15385 and will be committed when they're ready
        Show
        Mathieu Petit-Clair added a comment - Known issues: it's still possible to have multiple blocks: looking at the code, it appears that it's a feature of blocklib that you can't make a "multiple instances" block become a "unique instance" block. I don't know if it's really useful to have multiple tags block on a given page. the block displaying the course tags uses dynamically calculated font-size instead of css classes: both ways have advantages, we'll need to discuss it a bit more it's possible to choose which "types" of tags to display in the block (using javascript links): we need to come up with a better way to present these (eg. "community tags" are a bit OU-specific) help files are ongoing editing in MDL-15385 and will be committed when they're ready
        Hide
        Jenny Gray added a comment -

        I was tidying up some other work and realised that there's no hook to delete course tags when the course is deleted.

        Currently we handle this from within local/lib.php local_delete_course(). But that's not been committed to Moodle 2.0 as part of the course tagging, so there I don't think course tags are cleaned up at all

        I think there are two options: it might be possible to add a handler for the 'course_deleted' event, or we should add to the remove_course_contents() function in moodlelib.php

        What do you think?

        Show
        Jenny Gray added a comment - I was tidying up some other work and realised that there's no hook to delete course tags when the course is deleted. Currently we handle this from within local/lib.php local_delete_course(). But that's not been committed to Moodle 2.0 as part of the course tagging, so there I don't think course tags are cleaned up at all I think there are two options: it might be possible to add a handler for the 'course_deleted' event, or we should add to the remove_course_contents() function in moodlelib.php What do you think?
        Hide
        Jenny Gray added a comment -

        BTW just realised, following some usability testing, we dropped the "all,mine, course, official, community" links at the bottom of the block and made a few other minor alterations so it takes up less space.

        I attach a patch which I think shows what we've done (but I've knocked it together very quickly, so I hope it works!!)

        Show
        Jenny Gray added a comment - BTW just realised, following some usability testing, we dropped the "all,mine, course, official, community" links at the bottom of the block and made a few other minor alterations so it takes up less space. I attach a patch which I think shows what we've done (but I've knocked it together very quickly, so I hope it works!!)
        Hide
        John Beedell added a comment -

        Jenny, the delete course tags is already in moodlelib.php remove_course_contents() in 2.0 code, but I agree that it would probably be better to add it to the events system in the long run.

        Show
        John Beedell added a comment - Jenny, the delete course tags is already in moodlelib.php remove_course_contents() in 2.0 code, but I agree that it would probably be better to add it to the events system in the long run.
        Hide
        Jenny Gray added a comment -

        I've updated the patch that shows our simpler interface because I found some left-overs that were causing a significant performance hit if your tag table is large.

        I also recommend adding an index on tag for name,id to make the query that supports the auto-complete on the "add a tag" box more efficient.

        Show
        Jenny Gray added a comment - I've updated the patch that shows our simpler interface because I found some left-overs that were causing a significant performance hit if your tag table is large. I also recommend adding an index on tag for name,id to make the query that supports the auto-complete on the "add a tag" box more efficient.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        So the only open thing in this issue is about to create one "name, id" index in the tag table?

        Show
        Eloy Lafuente (stronk7) added a comment - So the only open thing in this issue is about to create one "name, id" index in the tag table?
        Hide
        Jenny Gray added a comment -

        I'm just coming back to this after a very long break and I don't think the attached patch ever did make it into the code base. I think this work is from around the time when Mathieu left HQ.

        I have noticed a number of issues with the course tagging as they currently stand, and would like to offer a branch for integration soon. I plan to resolve the following:

        • improved performance of the block display as per patch above
        • use tag_print_cloud rather than custom coursetag_print_cloud
        • improvement to wording for the tag-type choice, and admin ability to suppress it entirely & pick default tag cloud to display
        • add the name,id index on tag table
          • To make ‘flag as inappropriate’ functionality respond to a (new) capability check so that it can be disabled where no-one is monitoring flags.

        I have explicitly ignored the comment about multiple instances of the same block, as this does not relate to course tagging.

        Show
        Jenny Gray added a comment - I'm just coming back to this after a very long break and I don't think the attached patch ever did make it into the code base. I think this work is from around the time when Mathieu left HQ. I have noticed a number of issues with the course tagging as they currently stand, and would like to offer a branch for integration soon. I plan to resolve the following: improved performance of the block display as per patch above use tag_print_cloud rather than custom coursetag_print_cloud improvement to wording for the tag-type choice, and admin ability to suppress it entirely & pick default tag cloud to display add the name,id index on tag table • To make ‘flag as inappropriate’ functionality respond to a (new) capability check so that it can be disabled where no-one is monitoring flags. I have explicitly ignored the comment about multiple instances of the same block, as this does not relate to course tagging.
        Hide
        Jenny Gray added a comment -

        Probably also a good idea to remove commented out code that is suspiciously OU only e.g coursetag_get_official_keywords() which MDL-33901 has noted can be done without and rss related code (which we're not using any more).

        I'll do a lot of simplifying the code to hopefully make it more readable too. Seemd like there were masses of extra variables that aren't varying at runtime.

        As a result of improving the performance of the block as per the patch above, the links have been removed from the bottom of the block to toggle display between different tag clouds. They weren't working anyway (as noted above) so I hope this is acceptable. It also negates the need to improve their wording and provide an admin option to suppress them.

        Show
        Jenny Gray added a comment - Probably also a good idea to remove commented out code that is suspiciously OU only e.g coursetag_get_official_keywords() which MDL-33901 has noted can be done without and rss related code (which we're not using any more). I'll do a lot of simplifying the code to hopefully make it more readable too. Seemd like there were masses of extra variables that aren't varying at runtime. As a result of improving the performance of the block as per the patch above, the links have been removed from the bottom of the block to toggle display between different tag clouds. They weren't working anyway (as noted above) so I hope this is acceptable. It also negates the need to improve their wording and provide an admin option to suppress them.
        Hide
        Jenny Gray added a comment -

        Ug this code was a mess. I'm sure it shouldn't really be checking the system context for whether a user can add tags in a course. Changing to the course context.

        Show
        Jenny Gray added a comment - Ug this code was a mess. I'm sure it shouldn't really be checking the system context for whether a user can add tags in a course. Changing to the course context.
        Hide
        Jenny Gray added a comment -

        Prior to submitting for integration I'd like to get some-one to glance over this and see if you're reasonably happy.

        The previous comments in the issue show the work that has been done, but it is easiest to simply look at the code now rather than view a diff from the previous.

        For testing - enable course tagging, add a few tags in a few courses and make some of the tags official. Add the block on the site home, my and course home pages and check it displays sensible stuff!

        I've only done the work on master for now, but I'd appreciate feedback on whether to submit it for 2.3 as well?

        Show
        Jenny Gray added a comment - Prior to submitting for integration I'd like to get some-one to glance over this and see if you're reasonably happy. The previous comments in the issue show the work that has been done, but it is easiest to simply look at the code now rather than view a diff from the previous. For testing - enable course tagging, add a few tags in a few courses and make some of the tags official. Add the block on the site home, my and course home pages and check it displays sensible stuff! I've only done the work on master for now, but I'd appreciate feedback on whether to submit it for 2.3 as well?
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Note from MDL-33901: it would be great if you use this issue to also take rid of the commented out stuff that was talked there.

        Show
        Eloy Lafuente (stronk7) added a comment - Note from MDL-33901 : it would be great if you use this issue to also take rid of the commented out stuff that was talked there.
        Hide
        Jenny Gray added a comment -

        Submitting for integration now because I'm hoping it'll make the cut for 2.4. Apologies for not waiting for a peer reviewer, but hopefully this can speed the process?

        Show
        Jenny Gray added a comment - Submitting for integration now because I'm hoping it'll make the cut for 2.4. Apologies for not waiting for a peer reviewer, but hopefully this can speed the process?
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Oh Jenny,

        we cannot integrate that branch right now:

        1) it contains a bunch of local merges, we don't use to accept them (perhaps rebasing all your commits is the way to go). Squashing as much as possible (no problem integrating multiple commits, but better if related ones are mixed together).

        2) commit messages must, all them, make reference to the MDL-XXXX they belong to.

        3) there are apparently unrelated commits: ff82f48d24cf3c5843af4f44f6dc3c238d3e79a9 (calendar?).

        4) there are duplicate commits? 0e4a181 and 912dbca

        I've not gone deeply, sorry. If you need any support for cleaning the candidate branch, ask for it. And don't worry, we are still 4w away from freeze. We can!

        Show
        Eloy Lafuente (stronk7) added a comment - Oh Jenny, we cannot integrate that branch right now: 1) it contains a bunch of local merges, we don't use to accept them (perhaps rebasing all your commits is the way to go). Squashing as much as possible (no problem integrating multiple commits, but better if related ones are mixed together). 2) commit messages must, all them, make reference to the MDL-XXXX they belong to. 3) there are apparently unrelated commits: ff82f48d24cf3c5843af4f44f6dc3c238d3e79a9 (calendar?). 4) there are duplicate commits? 0e4a181 and 912dbca I've not gone deeply, sorry. If you need any support for cleaning the candidate branch, ask for it. And don't worry, we are still 4w away from freeze. We can!
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Jenny Gray added a comment -

        Is there any documentation about what you expect of a submitted branch? I always seem to screw something up. So sorry for wasting your time.

        Any how, I got a bit worried about my clone repository, so I dumped it and started again as there wasn't anything else useful in it. The new branch has a single commit with all my changes on it so it should be simpler for you.

        Show
        Jenny Gray added a comment - Is there any documentation about what you expect of a submitted branch? I always seem to screw something up. So sorry for wasting your time. Any how, I got a bit worried about my clone repository, so I dumped it and started again as there wasn't anything else useful in it. The new branch has a single commit with all my changes on it so it should be simpler for you.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        When you have one feature/bugfix branch living for long periods it's recommended to keep it updated via rebase instead of merge.

        So, if your branch is wip-MDL-15471 (branched from master), in order to get latest master changes each week incorporated... you should do something like:

        git checkout wip-MDL-15471
        git rebase master
        

        And that will cause all the new commits present in master (that you have pulled from upstream) to be added to wip-MDL-15471 and then, your own commits replayed on top of them. Of course that only can be done with "wip" branches, because it breaks history. But that's the way we use to do with feature/bugfix branches. In fact the automated message tells "You may wish to rebase...".

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - When you have one feature/bugfix branch living for long periods it's recommended to keep it updated via rebase instead of merge. So, if your branch is wip- MDL-15471 (branched from master), in order to get latest master changes each week incorporated... you should do something like: git checkout wip-MDL-15471 git rebase master And that will cause all the new commits present in master (that you have pulled from upstream) to be added to wip- MDL-15471 and then, your own commits replayed on top of them. Of course that only can be done with "wip" branches, because it breaks history. But that's the way we use to do with feature/bugfix branches. In fact the automated message tells "You may wish to rebase...". Ciao
        Hide
        Dan Poltawski added a comment -

        Hi Jenny,

        Sorry, but i'm going to have to reopen this. The single patch doesn't cherry-pick cleanly onto master, and your branch includes a 'merge commit' which seemingly includes all the core changes in the commit. See this:
        https://github.com/jennymgray/moodle/commit/85da63537aa14f503a3f25323b07e96f02b5f5f0

        Your patch needs to be rebased onto the latest top of moodle master, so the only diff which gets applied is your changes.

        I'm afraid i'm a bit pressed for time right now, so can't go in as much detail as i'd like. But perhaps try the developer chat to explain this better than i'm doing.

        thanks,
        Dan

        Show
        Dan Poltawski added a comment - Hi Jenny, Sorry, but i'm going to have to reopen this. The single patch doesn't cherry-pick cleanly onto master, and your branch includes a 'merge commit' which seemingly includes all the core changes in the commit. See this: https://github.com/jennymgray/moodle/commit/85da63537aa14f503a3f25323b07e96f02b5f5f0 Your patch needs to be rebased onto the latest top of moodle master, so the only diff which gets applied is your changes. I'm afraid i'm a bit pressed for time right now, so can't go in as much detail as i'd like. But perhaps try the developer chat to explain this better than i'm doing. thanks, Dan
        Hide
        Dan Poltawski added a comment -

        Oh, I also forgot to comment: It'd be much easier for us to accept this change if the testing instructions are moe extensive (or at least guide the tester through a small path testing the areas required).

        Show
        Dan Poltawski added a comment - Oh, I also forgot to comment: It'd be much easier for us to accept this change if the testing instructions are moe extensive (or at least guide the tester through a small path testing the areas required).
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Jenny Gray added a comment -

        Eloy - thanks for the rebase instructions, I've never worked out how to do it before and make it work! I'll give them a go later.

        Dan - I'd love a better explanation about how Moodle HQ processes differ from OU ones with regard to integrating completed work (we would accept what I sent you). Perhaps a subject for a coffee break at HackFest in October? If we want more OU developers offering core code, it would make sense to align our processes so we don't have to learn two.

        Testing instructions updated:

        1. switch course tagging on in the tag block settings
        2. create two courses A and B.
        3. Add the tag block to the site home page and configure it to display throughout the site.
        4. Add tags 'one' and 'two' to course A, and 'two' and 'three' to course B. Do this by visiting the course, and typing each tag in turn in the tag block text entry field and pressing return. Note that as you start to type in the box, matching existing tags should appear for your re-use (this isn't autocomplete though, just a suggestion).
        5. Make tags 'one' 'two' and 'three' official thorugh the tag editing screen.
        6. Add tags 'four' and 'five' to course A, and 'five' and 'six' to course B. Leave these tags as default.
        7. Go to the site home page, you should see all 6 tags in the block. Click on a tag to view the tag's page showing what is tagged with that word.
        8. Edit the block settings and change the tagtype dropdown to 'official'. Save and recheck that only official tags are shown on the site home page in the block.
        9. Repeat 8 for default tags only.
        10. Log in as a different user, create a few more tags. Go to the 'my home' page and ensure that only tags you've just created (not one to six) are displayed.
        11. Log back in as admin, change the permissions on the authenticated user role so that that role cannot create or flag tags.
        12. Log back in as an ordinary user. Ensure that you can no longer add tags in a course or flag tags as inappropriate on the tag page.

        Show
        Jenny Gray added a comment - Eloy - thanks for the rebase instructions, I've never worked out how to do it before and make it work! I'll give them a go later. Dan - I'd love a better explanation about how Moodle HQ processes differ from OU ones with regard to integrating completed work (we would accept what I sent you). Perhaps a subject for a coffee break at HackFest in October? If we want more OU developers offering core code, it would make sense to align our processes so we don't have to learn two. Testing instructions updated: 1. switch course tagging on in the tag block settings 2. create two courses A and B. 3. Add the tag block to the site home page and configure it to display throughout the site. 4. Add tags 'one' and 'two' to course A, and 'two' and 'three' to course B. Do this by visiting the course, and typing each tag in turn in the tag block text entry field and pressing return. Note that as you start to type in the box, matching existing tags should appear for your re-use (this isn't autocomplete though, just a suggestion). 5. Make tags 'one' 'two' and 'three' official thorugh the tag editing screen. 6. Add tags 'four' and 'five' to course A, and 'five' and 'six' to course B. Leave these tags as default. 7. Go to the site home page, you should see all 6 tags in the block. Click on a tag to view the tag's page showing what is tagged with that word. 8. Edit the block settings and change the tagtype dropdown to 'official'. Save and recheck that only official tags are shown on the site home page in the block. 9. Repeat 8 for default tags only. 10. Log in as a different user, create a few more tags. Go to the 'my home' page and ensure that only tags you've just created (not one to six) are displayed. 11. Log back in as admin, change the permissions on the authenticated user role so that that role cannot create or flag tags. 12. Log back in as an ordinary user. Ensure that you can no longer add tags in a course or flag tags as inappropriate on the tag page.
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Dan Poltawski added a comment -

        Hi Jenny,

        With regard to tag_print_cloud(), our usual process would not to change the arguments like that (and in some cases we have unusued arguments for backwards compatibility). Also we are trying to warn develooers about functions going away using a deprecation process: http://docs.moodle.org/dev/Deprecation

        However, in the case of this issue, the old tags things were not well maintained, and we've looked for any users in contrib and can't find them, so i'm inclined to let this land despite making these backward incompatible changes.

        I'm just looking for more integrator +1/input on this before doing so.

        Show
        Dan Poltawski added a comment - Hi Jenny, With regard to tag_print_cloud(), our usual process would not to change the arguments like that (and in some cases we have unusued arguments for backwards compatibility). Also we are trying to warn develooers about functions going away using a deprecation process: http://docs.moodle.org/dev/Deprecation However, in the case of this issue, the old tags things were not well maintained, and we've looked for any users in contrib and can't find them, so i'm inclined to let this land despite making these backward incompatible changes. I'm just looking for more integrator +1/input on this before doing so.
        Hide
        Jenny Gray added a comment -

        Thanks Dan - that extra argument was the thing I was most worried about offering. To be truly backward compatible (though less easy to read), as noted in the TODO comment I could move the parameter to the end of the list i.e. tag_print_cloud($nr_of_tags=150,$return=false,$tagset=null).

        Not sure I understand your comment about deprecation? This function isn't marked as deprecated.

        Show
        Jenny Gray added a comment - Thanks Dan - that extra argument was the thing I was most worried about offering. To be truly backward compatible (though less easy to read), as noted in the TODO comment I could move the parameter to the end of the list i.e. tag_print_cloud($nr_of_tags=150,$return=false,$tagset=null). Not sure I understand your comment about deprecation? This function isn't marked as deprecated.
        Hide
        Aparup Banerjee added a comment -

        seems fine from a glance (+0.6) at the changes to tag_print_cloud() for master only. (plus as Dan mentioned, checked about uses in contrib and nothing)

        ps: Jenny, normally we would endeavour to separate code clean up, adding boiler plates - adding 'public' etc within a separate commit to changes that are more logically different i.e.: changing function(ality),arguments etc.

        Show
        Aparup Banerjee added a comment - seems fine from a glance (+0.6) at the changes to tag_print_cloud() for master only. (plus as Dan mentioned, checked about uses in contrib and nothing) ps: Jenny, normally we would endeavour to separate code clean up, adding boiler plates - adding 'public' etc within a separate commit to changes that are more logically different i.e.: changing function(ality),arguments etc.
        Hide
        Sam Hemelryk added a comment -

        Hi guys,

        I've looked over this issue quickly.
        It gets a +1 from, passively, providing Dan you are happy to finish it up as you mentioned.
        It'd be great to hit our normal deprecation targets but as mentioned its not a commonly used API, is way outdated and as we know there are no uses in contrib = fine.
        The following are the 'limited' notes I've made:

        1. Typo innappropriate
        2. lib/db/install.xml whitespace nl missing that would make eloy cry + version bump not made.
        3. upgrade file has version number mismatch but I'd bet you'd be fixing that as a conflict anyway.
        4. Still a few obvious coding style things to clean up.

        I'd also agree with Dan's thoughts this needs to be well summarised and documented in an upgrade file and probably in docs somewhere.
        I also think that the functions that have been removed should be put back in as stubs that just throw a debugging notice and return default... just in case anyone out there has used them, however unlikely.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Hi guys, I've looked over this issue quickly. It gets a +1 from, passively, providing Dan you are happy to finish it up as you mentioned. It'd be great to hit our normal deprecation targets but as mentioned its not a commonly used API, is way outdated and as we know there are no uses in contrib = fine. The following are the 'limited' notes I've made: Typo innappropriate lib/db/install.xml whitespace nl missing that would make eloy cry + version bump not made. upgrade file has version number mismatch but I'd bet you'd be fixing that as a conflict anyway. Still a few obvious coding style things to clean up. I'd also agree with Dan's thoughts this needs to be well summarised and documented in an upgrade file and probably in docs somewhere. I also think that the functions that have been removed should be put back in as stubs that just throw a debugging notice and return default... just in case anyone out there has used them, however unlikely. Many thanks Sam
        Hide
        Dan Poltawski added a comment -

        Thanks guys, i've integrated this now.

        I did some commits on top to address some of those issues.

        I was adding the function stubs to lib/depreceatedlib.php, but in the end I didn't, because the functionality is different and the other tags changes are significant to affect anyone doing anything with these tags.

        This is unfortunate if anyone is doing things with tags, but, we've checked contrib and found nothing. The cleanup is, I think worth it.

        Show
        Dan Poltawski added a comment - Thanks guys, i've integrated this now. I did some commits on top to address some of those issues. I was adding the function stubs to lib/depreceatedlib.php, but in the end I didn't, because the functionality is different and the other tags changes are significant to affect anyone doing anything with these tags. This is unfortunate if anyone is doing things with tags, but, we've checked contrib and found nothing. The cleanup is, I think worth it.
        Hide
        Dan Poltawski added a comment - - edited

        Edit: problem moved to MDL-34573 which caused the regression.

        Show
        Dan Poltawski added a comment - - edited Edit: problem moved to MDL-34573 which caused the regression.
        Hide
        Dan Poltawski added a comment -

        Hi Jenny,

        Not sure I understand this step:
        "10. Log in as a different user, create a few more tags. Go to the 'my home' page and ensure that only tags you've just created (not one to six) are displayed."

        I'm not sure how to add my own tag, i've added an interest to my profile, but that isn't showing up in my moodle.

        Show
        Dan Poltawski added a comment - Hi Jenny, Not sure I understand this step: "10. Log in as a different user, create a few more tags. Go to the 'my home' page and ensure that only tags you've just created (not one to six) are displayed." I'm not sure how to add my own tag, i've added an interest to my profile, but that isn't showing up in my moodle.
        Hide
        Jenny Gray added a comment -

        Wow! Thanks all for the work on this. I was expecting you to bounce it back at me with a list of things to fix!!

        Dan - on the /my page, the Tag block should show you course tags that you've added. So if you repeat step 4 with some of the same and some different tag words, you should see these tags appear in the block on /my rather than the same tags as at the site home page or on a course home page. Sorry for the lack of clarity.

        Show
        Jenny Gray added a comment - Wow! Thanks all for the work on this. I was expecting you to bounce it back at me with a list of things to fix!! Dan - on the /my page, the Tag block should show you course tags that you've added. So if you repeat step 4 with some of the same and some different tag words, you should see these tags appear in the block on /my rather than the same tags as at the site home page or on a course home page. Sorry for the lack of clarity.
        Hide
        Dan Poltawski added a comment -

        Ah, indeed.

        It looks good.

        Show
        Dan Poltawski added a comment - Ah, indeed. It looks good.
        Hide
        Dan Poltawski added a comment -

        Congratulations, you've done it!

        Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc

        Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

        Show
        Dan Poltawski added a comment - Congratulations, you've done it! Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: