Bug 22420 - Tag cloud feature broken
Summary: Tag cloud feature broken
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Tomás Cohen Arazi
QA Contact: Katrin Fischer
URL:
Whiteboard:
Keywords: rel_19_05_candidate
: 22766 (view as bug list)
Depends on: 14385
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-26 19:27 UTC by Tomás Cohen Arazi
Modified: 2019-06-08 19:41 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.06


Attachments
Bug 22420: Remove duplicated tags (1.61 KB, patch)
2019-02-26 20:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
[ALTERNATIVE] Bug 22420: Remove wrong calaculation on approved tags (2.63 KB, patch)
2019-02-27 10:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22420: Remove wrong calculation on approved tags (2.72 KB, patch)
2019-05-01 09:37 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2019-02-26 19:27:16 UTC
Steps to reproduce:
- Have the OPAC tags feature enabled
- Add a couple tags to different records, make sure some of them are used more than once
- Go to 'Tag cloud' in the OPAC
=> FAIL: the repeated tags are displayed the same size of the other ones, and repeated many times (one per use).

This was caused by bug 14385.
Comment 1 Tomás Cohen Arazi 2019-02-26 19:29:50 UTC
I'm sure it was caused by bug 14385 because it overwrites $results with something they calculate, and commmenting out the overwriting line fixes the problem.

324    }
325    #$results = \@filtered_results;
326    stratify_tags(10, $results); # work out the differents sizes for things
Comment 2 Jonathan Druart 2019-02-26 20:12:17 UTC
Created attachment 85748 [details] [review]
Bug 22420: Remove duplicated tags

Caused by
  commit e1b5fa657de843a177fb4bf57947a1376152d021
  Bug 14385: Squash of a lot of patches rebased

Test plan:
- Have the OPAC tags feature enabled
- Add a couple tags to different records, make sure some of them are
used more than once
- Go to 'Tag cloud' in the OPAC
=> the repeated tags must not be repeated many times.

[- Enjoy the lack of tests.]
Comment 3 Tomás Cohen Arazi 2019-02-27 09:59:21 UTC
(In reply to Jonathan Druart from comment #2)
> Created attachment 85748 [details] [review] [review]
> Bug 22420: Remove duplicated tags
> 
> Caused by
>   commit e1b5fa657de843a177fb4bf57947a1376152d021
>   Bug 14385: Squash of a lot of patches rebased
> 
> Test plan:
> - Have the OPAC tags feature enabled
> - Add a couple tags to different records, make sure some of them are
> used more than once
> - Go to 'Tag cloud' in the OPAC
> => the repeated tags must not be repeated many times.
> 
> [- Enjoy the lack of tests.]

This doesn't solve the problem.
Comment 4 Tomás Cohen Arazi 2019-02-27 10:23:55 UTC
Created attachment 85755 [details] [review]
[ALTERNATIVE] Bug 22420: Remove wrong calaculation on approved tags

Before the patch from bug 14385, the passed structure (that is returned
by get_approval_rows() looks like:

\ [
    [0] {
        approved           1,
        approved_by        51,
        approved_by_name   undef,
        date_approved      "2019-02-26 15:36:42",
        term               "another",
        weight_total       3
    },
    [1] {
    ...

After the introduced 'filtering', we loose the information about the
term weight!:

\ [
    [0] {
        approved               1,
        author                 "Heylin, Clinton.",
        biblionumber           1,
        borrowernumber         51,
        date_created           "2019-02-26 15:36:37",
        language               undef,
        subtitle               [],
        tag_id                 1,
        term                   "word",
        time_created_display   "15:36:37",
        title                  "E Street shuffle :",
        visible                1,
        XSLTBloc               "
            <<<BUNCH OF GENERATED HTML HERE>>>
    ...

The code even calls GetMarcBiblio. This needs to be reverted as it is
plain wrong. If it was worth removing *some* biblios from the
weight_total value, then this calculation should be done somewhere else,
without fetching the MARC data again.
Comment 5 Jonathan Druart 2019-02-27 13:13:43 UTC
What you are suggesting is temporary and must be fixed, right?
Comment 6 Tomás Cohen Arazi 2019-03-08 09:38:50 UTC
(In reply to Jonathan Druart from comment #5)
> What you are suggesting is temporary and must be fixed, right?

After looking at this and having feedback on koha-devel I can say the 'feature' of hiding tags for hidden bibliographic records (due to OpacHiddenItems modulo OpacHiddenExceptions) is broken altogether. i.e. The calculation is wrong anyway and the loop doesn't generate a suitable data structure either.

I prefer my approach to solve the immediate issue (no tags are hidden anyway, just wrongly displayed), and file a separate bug for the leakage bug.
Comment 7 Jonathan Druart 2019-04-26 23:52:12 UTC
Sending to NSO with your patch then ;)
Comment 8 Jonathan Druart 2019-04-30 18:29:16 UTC
*** Bug 22766 has been marked as a duplicate of this bug. ***
Comment 9 Jonathan Druart 2019-04-30 18:29:51 UTC
Upping severity.
Comment 10 Katrin Fischer 2019-05-01 09:31:29 UTC
Moving this to signed off as per this comment: bug 22766 comment 5
Comment 11 Katrin Fischer 2019-05-01 09:37:49 UTC
Created attachment 89163 [details] [review]
Bug 22420: Remove wrong calculation on approved tags

Before the patch from bug 14385, the passed structure (that is returned
by get_approval_rows() looks like:

\ [
    [0] {
        approved           1,
        approved_by        51,
        approved_by_name   undef,
        date_approved      "2019-02-26 15:36:42",
        term               "another",
        weight_total       3
    },
    [1] {
    ...

After the introduced 'filtering', we loose the information about the
term weight!:

\ [
    [0] {
        approved               1,
        author                 "Heylin, Clinton.",
        biblionumber           1,
        borrowernumber         51,
        date_created           "2019-02-26 15:36:37",
        language               undef,
        subtitle               [],
        tag_id                 1,
        term                   "word",
        time_created_display   "15:36:37",
        title                  "E Street shuffle :",
        visible                1,
        XSLTBloc               "
            <<<BUNCH OF GENERATED HTML HERE>>>
    ...

The code even calls GetMarcBiblio. This needs to be reverted as it is
plain wrong. If it was worth removing *some* biblios from the
weight_total value, then this calculation should be done somewhere else,
without fetching the MARC data again.

Signed-off-by: Claudio  <costalc@gmail.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2019-05-01 09:38:42 UTC
Fixes a major regression. I agree if filtering is needed, we should resolve that separately.
Comment 13 Nick Clemens 2019-05-07 11:44:42 UTC
Awesome work all!

Pushed to master for 19.05
Comment 14 Martin Renvoize 2019-05-08 14:16:12 UTC
Pushed to 18.11.x for 18.11.06
Comment 15 Lucas Gass 2019-05-24 17:41:31 UTC
missing dependencies, wont backport to 18.05.x