Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-39800

Course tags are being incorrectly weighted

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4, 2.4.5, 2.5.1
    • Fix Version/s: 2.6
    • Component/s: Tags
    • Labels:
    • Testing Instructions:
      Hide

      enable course tags
      create a course
      add the tag block to it
      add four different tags: fred, zoe, gamma, delta
      log on as a different user
      go to the same course and add two tags: fred and zoe
      log on as a different user
      go to the same course and add tag fred
      log on as a different user
      go to the same course and add tag fred

      Now you should have a tag cloud with fred 4 entries, zoe 2 entries, gamma 1 entry, delta 1 entry. You would expect "fred" to be biggest, then "zoe", then the other two. That's not the case though - delta and gamma are biggest.

      If you inspect the html you'll see Delta has class s20, Fred has class s80, gamma has class s20 and zoe has class s40.

      The correct html would have Fred with s20, Zoe with s10 and gamma and delta both with s5.

      Show
      enable course tags create a course add the tag block to it add four different tags: fred, zoe, gamma, delta log on as a different user go to the same course and add two tags: fred and zoe log on as a different user go to the same course and add tag fred log on as a different user go to the same course and add tag fred Now you should have a tag cloud with fred 4 entries, zoe 2 entries, gamma 1 entry, delta 1 entry. You would expect "fred" to be biggest, then "zoe", then the other two. That's not the case though - delta and gamma are biggest. If you inspect the html you'll see Delta has class s20, Fred has class s80, gamma has class s20 and zoe has class s40. The correct html would have Fred with s20, Zoe with s10 and gamma and delta both with s5.
    • Workaround:
      Hide

      Alter the theme for the sXX font sizes to cope with additional s values that are bigger.

      Show
      Alter the theme for the sXX font sizes to cope with additional s values that are bigger.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL_39800_squashed

      Description

      See https://moodle.org/mod/forum/discuss.php?d=229071

      The user has given her tag cloud HTML and screenshots of the differing display between the course home page and the "more tags" page.

      You can see from the html that the more frequently used tags have weights greater than 20 which do not then have corresponding styles in the base theme.

      The weight is calculated in tag_print_cloud assuming that the most popular tag is being supplied to the function first. In this user's case the least popular tag is being supplied first, hence the odd display.

      So, there are two problems to investigate further:

      a) why is the tag cloud sending the tags in the wrong order to the more page.

      b) would the sort by options of "alphabetical" and "date" ever work on that page anyway.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            jenny-gray Jenny Gray added a comment -

            I initially thought it was just the "more" page that was wrong, but actually its the main block as well. The reason is that they are both passing the tags to the tag cloud alphabetically not in order of priority. I've updated the issue's name to reflect this realisation.

            The offending code is at the end of coursetag_get_tags() where the result from the SQL query (which uses "ORDER BY count DESC, name ASC" is optionally re-sorted by date or alphabetic.

            The block uses alphabetic sort by default. Changing this to 'popularity' makes the weighting display correctly, but the tags are not displayed ordered by popularity. That's because the default tag_print_cloud function then orders the tags itself.

            This bug was therefore introduced during MDL-15471 which tidied up some other errors and swapped to using the default tag_print_cloud rather than the old coursetag_print_cloud which did not do any sorting. Bah - own goal and obviously something I completely overlooked last time round

            So here's the big question: Do we continue to support letting people re-order the tag clouds for course tags and implement options to pass the user choice through, or not?

            Note to self: (assuming I actually develop the fix - I'm off on holiday soon so not assigning this to me in case others get to it first)...The course tags block also shows in "all" mode on the site home page and in "my" mode on the my home page.

            Show
            jenny-gray Jenny Gray added a comment - I initially thought it was just the "more" page that was wrong, but actually its the main block as well. The reason is that they are both passing the tags to the tag cloud alphabetically not in order of priority. I've updated the issue's name to reflect this realisation. The offending code is at the end of coursetag_get_tags() where the result from the SQL query (which uses "ORDER BY count DESC, name ASC" is optionally re-sorted by date or alphabetic. The block uses alphabetic sort by default. Changing this to 'popularity' makes the weighting display correctly, but the tags are not displayed ordered by popularity. That's because the default tag_print_cloud function then orders the tags itself. This bug was therefore introduced during MDL-15471 which tidied up some other errors and swapped to using the default tag_print_cloud rather than the old coursetag_print_cloud which did not do any sorting. Bah - own goal and obviously something I completely overlooked last time round So here's the big question: Do we continue to support letting people re-order the tag clouds for course tags and implement options to pass the user choice through, or not? Note to self: (assuming I actually develop the fix - I'm off on holiday soon so not assigning this to me in case others get to it first)...The course tags block also shows in "all" mode on the site home page and in "my" mode on the my home page.
            Hide
            jenny-gray Jenny Gray added a comment -

            tag_print_cloud passes the tags through to tag_cloud_sort to sort either alphabetically or by whatever is defined in $CFG->tagsort which is not set in the admin screens. We could potentially use this config setting to make the more page work as expected.

            Show
            jenny-gray Jenny Gray added a comment - tag_print_cloud passes the tags through to tag_cloud_sort to sort either alphabetically or by whatever is defined in $CFG->tagsort which is not set in the admin screens. We could potentially use this config setting to make the more page work as expected.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            It seems to me that it should be displaying an initial tag cloud sorted by alphabet with weight emphasizing the tags, visually. And also allow users to sort it otherwise (and always be able to go back to the original "sort by alphabet").

            This way, we can gain to perspectives in one visual display.

            Show
            nadavkav Nadav Kavalerchik added a comment - It seems to me that it should be displaying an initial tag cloud sorted by alphabet with weight emphasizing the tags, visually. And also allow users to sort it otherwise (and always be able to go back to the original "sort by alphabet"). This way, we can gain to perspectives in one visual display.
            Hide
            jenny-gray Jenny Gray added a comment -

            Further community engagement in the issue of whether to allow user sorting or not has been requested in the original forum thread and via my blog http://ltsdevmusings.wordpress.com/2013/05/24/weighty-thoughts-about-tags/

            Thanks Nadav for responding!

            Note for HQ if you see this in triage - an integrators view would be welcome.

            Show
            jenny-gray Jenny Gray added a comment - Further community engagement in the issue of whether to allow user sorting or not has been requested in the original forum thread and via my blog http://ltsdevmusings.wordpress.com/2013/05/24/weighty-thoughts-about-tags/ Thanks Nadav for responding! Note for HQ if you see this in triage - an integrators view would be welcome.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            @Jenny, I recommend post this issue on the Moodle HQ forums as well.

            Show
            nadavkav Nadav Kavalerchik added a comment - @Jenny, I recommend post this issue on the Moodle HQ forums as well.
            Hide
            jenny-gray Jenny Gray added a comment -

            I've had other feedback here in the same vein as Nadav's so I think the best thing is to attempt to code a fix that retains the current functionality and get some peer review comments from the integration team.

            Show
            jenny-gray Jenny Gray added a comment - I've had other feedback here in the same vein as Nadav's so I think the best thing is to attempt to code a fix that retains the current functionality and get some peer review comments from the integration team.
            Hide
            jenny-gray Jenny Gray added a comment -

            In searching for uses of course tag functions, I've realised that the tag/coursetags_edit.php script (which you reach by clicking "edit my tags" in the tag block for a course) also exhibits the same problem.

            In fact, every use of coursetag_get_tags() has the same problem because it tries to sort and shouldn't. I've fixed this initially by using sort "popularity" as the passed in parameter, but it would be better to remove the parameter entirely to avoid any-one else using it and messing things up. I've done this in a separate commit c82c821 so that integrators can remove it easily if they disagree. I did consider deprecating it, but since its use would be broken I thought it better to remove it immediately.

            I noticed the same thing can be done for coursetag_get_all_tags() and while checking for uses of that, realised that we removed use of coursetag_sort() last time round but didn't remove the function itself, so I've stripped that out as well.

            Show
            jenny-gray Jenny Gray added a comment - In searching for uses of course tag functions, I've realised that the tag/coursetags_edit.php script (which you reach by clicking "edit my tags" in the tag block for a course) also exhibits the same problem. In fact, every use of coursetag_get_tags() has the same problem because it tries to sort and shouldn't. I've fixed this initially by using sort "popularity" as the passed in parameter, but it would be better to remove the parameter entirely to avoid any-one else using it and messing things up. I've done this in a separate commit c82c821 so that integrators can remove it easily if they disagree. I did consider deprecating it, but since its use would be broken I thought it better to remove it immediately. I noticed the same thing can be done for coursetag_get_all_tags() and while checking for uses of that, realised that we removed use of coursetag_sort() last time round but didn't remove the function itself, so I've stripped that out as well.
            Hide
            jenny-gray Jenny Gray added a comment -

            Peer review please? no documentation on API changes made yet until approach agreed.

            Show
            jenny-gray Jenny Gray added a comment - Peer review please? no documentation on API changes made yet until approach agreed.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Jenny,

            Many apologies for the delay in getting a review.

            I don't think we can accept changing the ordering of the parameters of the functions for backwards compatibility reasons. (Although as I think I said after the last course tags issue, i'm a little bit torn because the tags api doesn't seem like an especially solid thing that people are building upon.)

            The way we'd typically handle 'useless' parameters is to rename them $unused and update the phpdoc to say its no longer used. Then, if we wanted to get rid of the function in the future, we could replace it with a new one and deprecate the old one (http://docs.moodle.org/dev/Deprecation). That was peoples code will not immediately break.

            Having said that, I don't like the messing around with the $CFG->tagsort value, if we need to do that then it seems like the parameter still has a valid use?

            Show
            poltawski Dan Poltawski added a comment - Hi Jenny, Many apologies for the delay in getting a review. I don't think we can accept changing the ordering of the parameters of the functions for backwards compatibility reasons. (Although as I think I said after the last course tags issue, i'm a little bit torn because the tags api doesn't seem like an especially solid thing that people are building upon.) The way we'd typically handle 'useless' parameters is to rename them $unused and update the phpdoc to say its no longer used. Then, if we wanted to get rid of the function in the future, we could replace it with a new one and deprecate the old one ( http://docs.moodle.org/dev/Deprecation ). That was peoples code will not immediately break. Having said that, I don't like the messing around with the $CFG->tagsort value, if we need to do that then it seems like the parameter still has a valid use?
            Hide
            jenny-gray Jenny Gray added a comment -

            Hi Dan,

            No need to apologise - thanks for looking at it.

            I did the messing around with $CFG->tagsort to avoid changing the tag/locallib.php tag_cloud_sort() function which only uses that to define how to sort. If you agree, I could add a parameter to that function instead so that an alternative sort could be passed in.

            Then it would use either a) the passed in parameter or b) $CFG->tagsort if set or c) 'name' as the fall-back default.

            Thanks for explaining the approach for $unused, I will adopt that. I still think it is wrong to have a 'sort' parameter on get_tags, as the print_cloud function is always going to have to override any sort order to get the weightings displayed correctly. The 'sort' parameter should move to print_cloud, and for backward compatibility would accept null.

            If you are happy with that approach, I will re-code it?

            Show
            jenny-gray Jenny Gray added a comment - Hi Dan, No need to apologise - thanks for looking at it. I did the messing around with $CFG->tagsort to avoid changing the tag/locallib.php tag_cloud_sort() function which only uses that to define how to sort. If you agree, I could add a parameter to that function instead so that an alternative sort could be passed in. Then it would use either a) the passed in parameter or b) $CFG->tagsort if set or c) 'name' as the fall-back default. Thanks for explaining the approach for $unused, I will adopt that. I still think it is wrong to have a 'sort' parameter on get_tags, as the print_cloud function is always going to have to override any sort order to get the weightings displayed correctly. The 'sort' parameter should move to print_cloud, and for backward compatibility would accept null. If you are happy with that approach, I will re-code it?
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Jenny,

            Both your suggestions seem sensible to me, thanks!

            Show
            poltawski Dan Poltawski added a comment - Hi Jenny, Both your suggestions seem sensible to me, thanks!
            Hide
            jenny-gray Jenny Gray added a comment -

            Gah, realised why $CFG->tagsort is there in the first place. It is because the tag_cloud_sort() function is called by PHP function usort and so cannot take extra parameters. Using a global is the only way to get extra information in there.

            The alternative would be to have three different tag_cloud_sort() functions all of which were largely identical except for the property of the tag which they use to sort upon. And this would be more restrictive because if you ever wanted to sort on a different property of a tag, you'd have to add another function. Plus you'd need to retain the current function for backward compatibility, and have a new "ordering" function called from tag_print_cloud() to work out which of the sorting functions to call. That's an awful lot of code to avoid a config setting.

            I can't see an elegant way round this. I suspect $CFG->tagsort is the most pragmatic solution.

            Show
            jenny-gray Jenny Gray added a comment - Gah, realised why $CFG->tagsort is there in the first place. It is because the tag_cloud_sort() function is called by PHP function usort and so cannot take extra parameters. Using a global is the only way to get extra information in there. The alternative would be to have three different tag_cloud_sort() functions all of which were largely identical except for the property of the tag which they use to sort upon. And this would be more restrictive because if you ever wanted to sort on a different property of a tag, you'd have to add another function. Plus you'd need to retain the current function for backward compatibility, and have a new "ordering" function called from tag_print_cloud() to work out which of the sorting functions to call. That's an awful lot of code to avoid a config setting. I can't see an elegant way round this. I suspect $CFG->tagsort is the most pragmatic solution.
            Hide
            jenny-gray Jenny Gray added a comment -

            Response to first peer review: moved sorting into tag_cloud_print function, and swapped coursetag_get parameters for sorting to $unused. See previous comment, I do not believe it is sensible to rework to avoid $CFG->tagsort.

            I've updated upgrade.txt, assuming this will only go for 2.6 because of the API changes. Happy to create other branches if you think it is good for them though!

            Show
            jenny-gray Jenny Gray added a comment - Response to first peer review: moved sorting into tag_cloud_print function, and swapped coursetag_get parameters for sorting to $unused. See previous comment, I do not believe it is sensible to rework to avoid $CFG->tagsort. I've updated upgrade.txt, assuming this will only go for 2.6 because of the API changes. Happy to create other branches if you think it is good for them though!
            Hide
            poltawski Dan Poltawski added a comment -

            Sorry for the delay Jenny - I was away

            Show
            poltawski Dan Poltawski added a comment - Sorry for the delay Jenny - I was away
            Hide
            poltawski Dan Poltawski added a comment -

            Hmm ok - well I still think its a bit yuky and there are some possible solutions: http://stackoverflow.com/questions/8230538/pass-extra-parameters-to-usort-callback but it makes sense.

            The only issue now is that the upgrade.txt comment should be at the top of the file (newest release first)

            Show
            poltawski Dan Poltawski added a comment - Hmm ok - well I still think its a bit yuky and there are some possible solutions: http://stackoverflow.com/questions/8230538/pass-extra-parameters-to-usort-callback but it makes sense. The only issue now is that the upgrade.txt comment should be at the top of the file (newest release first)
            Hide
            jenny-gray Jenny Gray added a comment -

            Thanks for re-reviewing Dan. I have amended the final commit on the branch to get the text in upgrade.txt in the right order, so I think this is ready for integration now.

            I have re-based on current master but am now going away for a fortnight, so hopefully everything will be OK for you!

            Show
            jenny-gray Jenny Gray added a comment - Thanks for re-reviewing Dan. I have amended the final commit on the branch to get the text in upgrade.txt in the right order, so I think this is ready for integration now. I have re-based on current master but am now going away for a fortnight, so hopefully everything will be OK for you!
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Jenny,

            I am going to create a backport request for this - yes it changes APIs - but it is also a bug so we should consider it.

            Show
            damyon Damyon Wiese added a comment - Thanks Jenny, I am going to create a backport request for this - yes it changes APIs - but it is also a bug so we should consider it.
            Hide
            damyon Damyon Wiese added a comment -

            Integrated to master only for now.

            Note: I had to ammend your commit messages to meet our guidelines, which are:

            MDL-XXXX <componentname> <short message>

            I also squashed some commits as they seemed too verbose.

            Cheers, Damyon

            Show
            damyon Damyon Wiese added a comment - Integrated to master only for now. Note: I had to ammend your commit messages to meet our guidelines, which are: MDL-XXXX <componentname> <short message> I also squashed some commits as they seemed too verbose. Cheers, Damyon
            Hide
            abgreeve Adrian Greeve added a comment -

            Tested on the master integration branch.
            I found an error when entering in the same tag, but after looking at the code I can see that it is unrelated to the changes made here.
            Test passed.

            Show
            abgreeve Adrian Greeve added a comment - Tested on the master integration branch. I found an error when entering in the same tag, but after looking at the code I can see that it is unrelated to the changes made here. Test passed.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week.

            Hurray!

            Show
            damyon Damyon Wiese added a comment - Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week. Hurray!
            Hide
            jenny-gray Jenny Gray added a comment -

            Yay - lovely to come back from holiday and see this!

            Two questions (and offers of help)

            First, Adrian did you file the error you found when entering the same tag as an issue? I'd be happy to look at fixing that?

            Second, can I do anything to assist with the backport request?

            Show
            jenny-gray Jenny Gray added a comment - Yay - lovely to come back from holiday and see this! Two questions (and offers of help) First, Adrian did you file the error you found when entering the same tag as an issue? I'd be happy to look at fixing that? Second, can I do anything to assist with the backport request?
            Hide
            abgreeve Adrian Greeve added a comment -

            Yes, I did create an issue for the error that I found (MDL-41339). It is linked above.

            Show
            abgreeve Adrian Greeve added a comment - Yes, I did create an issue for the error that I found ( MDL-41339 ). It is linked above.
            Hide
            poltawski Dan Poltawski added a comment -

            Jenny - if you watch/assign yourself the backport request we'll comment on it when voted whether to accept ti.

            Show
            poltawski Dan Poltawski added a comment - Jenny - if you watch/assign yourself the backport request we'll comment on it when voted whether to accept ti.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  18/Nov/13