Course tags improvements

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

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?

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

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.

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.

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

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.

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.

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.

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?

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.

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?

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

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!

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

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.

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

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

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

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

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

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.

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

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.

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.

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.

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

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.

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

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.

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.

Ah, indeed.

It looks good.

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.