Bug 21846 - Using emoji as tags doesn't discriminate between emoji when calculating weights or searching
Summary: Using emoji as tags doesn't discriminate between emoji when calculating weigh...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Josef Moravec
URL:
Keywords:
: 21247 (view as bug list)
Depends on:
Blocks: 23056
  Show dependency treegraph
 
Reported: 2018-11-15 20:40 UTC by Nick Clemens
Modified: 2020-06-04 20:32 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Please note, this patch fixes issues going forward. It includes a maintenance script to allow you to fix any possible existing cases. Please see bugzilla for details.
Version(s) released in:


Attachments
Bug 21846: Add Koha::Tags::Approval(s) and Koha::Tags::Index(es) (13.31 KB, patch)
2018-11-23 20:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21846: Regression tests for add_tag_approval (5.16 KB, patch)
2018-11-23 20:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21846: Make 'term' use utf8mb_bin collation on tags tables (4.27 KB, patch)
2018-11-23 20:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21846: (follow-up) Add maintenance script (5.06 KB, patch)
2019-01-08 16:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21846: Add Koha::Tags::Approval(s) and Koha::Tags::Index(es) (13.37 KB, patch)
2019-01-09 18:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21846: Regression tests for add_tag_approval (5.21 KB, patch)
2019-01-09 18:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21846: Make 'term' use utf8mb_bin collation on tags tables (4.32 KB, patch)
2019-01-09 18:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21846: (follow-up) Add maintenance script (5.10 KB, patch)
2019-01-09 18:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21846: (QA follow-up) Fix maintenance script path (1.15 KB, patch)
2019-02-26 18:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21846: Fix pod2usage return values (1.02 KB, patch)
2019-02-26 20:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21846: Simplify ugly double for loops (883 bytes, patch)
2019-02-26 20:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21846: Add Koha::Tags::Approval(s) and Koha::Tags::Index(es) (13.48 KB, patch)
2019-02-27 11:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21846: Regression tests for add_tag_approval (5.27 KB, patch)
2019-02-27 11:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21846: Make 'term' use utf8mb_bin collation on tags tables (4.39 KB, patch)
2019-02-27 11:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21846: (follow-up) Add maintenance script (5.17 KB, patch)
2019-02-27 11:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21846: (QA follow-up) Fix maintenance script path (1.20 KB, patch)
2019-02-27 11:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21846: Fix pod2usage return values (1.07 KB, patch)
2019-02-27 11:24 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21846: Simplify ugly double for loops (938 bytes, patch)
2019-02-27 11:24 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-11-15 20:40:55 UTC

    
Comment 1 Nick Clemens 2018-11-15 20:44:03 UTC
Tested on mariadb 10.1.26-0+deb9u1

To recreate:
1 - Enable tags and disable moderation (or plan to moderate and accept tags)
2 - Tag 3 records:
   a - with 🐋
   b - with 🌮
   c - with 👍
3 - Note the weight on each says '3'
4 - Click the tag to search, you get back all the records
Comment 2 Tomás Cohen Arazi 2018-11-23 20:04:10 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2018-11-23 20:04:15 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2018-11-23 20:04:19 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2018-11-23 20:09:35 UTC
I leave the patches I worked on today here.

The only thing that is missing IMO is re-calculating the weight and weight_total in tags_index and tags_approval. This could done easily by going through tags_all in the update process. I need to leave now, but I will implement it next week. The main issue can be tested anyways.
Comment 6 Tomás Cohen Arazi 2019-01-08 16:15:25 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2019-01-09 18:52:15 UTC
Created attachment 83759 [details] [review]
Bug 21846: Add Koha::Tags::Approval(s) and Koha::Tags::Index(es)

In order to avoid writing the tests using plain DBIC which would later
need to be replaced by a Koha::Object-based counterpart, I introduce
this stub classes.

Stub tests are added as well.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Tags.t \
          t/db_dependent/Koha/Tags/Approvals.t \
          t/db_dependent/Koha/Tags/Indexes.t
=> SUCCESS: Tests pass
- Sign off :-D

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Owen Leonard 2019-01-09 18:52:18 UTC
Created attachment 83760 [details] [review]
Bug 21846: Regression tests for add_tag_approval

Due to collation issues, add_tag_approval wrongly calculates
the weight for strings including extended UNICODE characters.

This patch introduces a test for this situation.

To test:
- Apply this patch on master
- Run:
  $ kshell
 k$ prove t/db_dependent/Tags.t
=> FAIL: Tests fail because all extended characters match the same

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Owen Leonard 2019-01-09 18:52:21 UTC
Created attachment 83761 [details] [review]
Bug 21846: Make 'term' use utf8mb_bin collation on tags tables

This patch makes the utf8mb4_bin collation preferred for comparing tags.
Otherwise suppolemental unicode characters all match.

To test:

- Enable tags and disable moderation (or plan to moderate and accept tags)
- Tag 3 records:
   a - with 🐋
   b - with 🌮
   c - with 👍
- Note the weight on each says '3'
- Click the tag to search, you get back all the records
- Apply the previous patches from this bug report
- Run:
  $ kshell
 k$ prove t/db_dependent/Tags.t
=> FAIL: Tests fail, related to counting stuffs
- Apply this patch and
(a) Run updatedatabase to upgrade the schema
- Run:
 k$ prove t/db_dependent/Tags.t
=> SUCCESS: Tests pass!
(b) reset_all to get a fresh DB using kohastructure.sql
- Run:
 k$ prove t/db_dependent/Tags.t
=> SUCCESS: Tests pass too!
- Sign off :-D

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Owen Leonard 2019-01-09 18:52:24 UTC
Created attachment 83762 [details] [review]
Bug 21846: (follow-up) Add maintenance script

This patch adds a maintenance script that generates the missing
tags_approval entries based on the tags_all table, and then recalculates
the weights for both tags_approval and tags_index tables.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Owen Leonard 2019-01-09 18:54:04 UTC
These patches worked as expected for me. I did get inconsistent results from 'prove t/db_dependent/Tags.t.' It sometimes passed and sometimes didn't.

The QA tool has some complaints, but I'm not sure if they're false positives. I'll leave it for QA to decide.
Comment 12 Tomás Cohen Arazi 2019-01-09 19:54:15 UTC
(In reply to Owen Leonard from comment #11)
> These patches worked as expected for me. I did get inconsistent results from
> 'prove t/db_dependent/Tags.t.' It sometimes passed and sometimes didn't.
> 
> The QA tool has some complaints, but I'm not sure if they're false
> positives. I'll leave it for QA to decide.

Owen, thanks for reporting that. The problem is the patchset isn't updating the DBIC schema files. You can do it by issuing the following command

 $ dbic

on the 'vagrant' user.

The @RM will provide those schema updates when pushing.
Comment 13 Katrin Fischer 2019-02-03 11:28:41 UTC
QA script:
 FAIL	misc/maintenance/fix_tags_weight.pl
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: merge marker (=======) (line 85)
		forbidden pattern: merge marker (=======) (line 124)
   OK	  git manipulation

Please fix!

I also noticed that you can only tag an item with any emoticon once. If I try to add whale and then later add smiley I always get told I can't add the same twice. Is this related?
Comment 14 Tomás Cohen Arazi 2019-02-25 14:52:56 UTC
The script I introduce doesn't have merge markers in it, it has some strings it prints in verbose mode.
Comment 15 Katrin Fischer 2019-02-25 19:27:10 UTC
Leaving this for tcohen to have another look:

What I did:
- Added taco to 2 separate records
- Applied patches
- updatedatabase - dbic - maint script
- Report: Added => 🌮
- Checked the OPAC
- Clicking on taco now finds the 2 correct records, other tags work as well
- But: tags table in the db is empty (should it be?)
- Tag cloud shows 2 tacos instead of one bigger taco
Comment 16 Katrin Fischer 2019-02-25 19:28:26 UTC
So we don't forget again: SET NAMES utf8mb4; will help with SQL.
Comment 17 Tomás Cohen Arazi 2019-02-26 18:39:10 UTC
(In reply to Katrin Fischer from comment #15)
> Leaving this for tcohen to have another look:
> 
> What I did:
> - Added taco to 2 separate records
> - Applied patches
> - updatedatabase - dbic - maint script
> - Report: Added => 🌮
> - Checked the OPAC
> - Clicking on taco now finds the 2 correct records, other tags work as well
> - But: tags table in the db is empty (should it be?)
> - Tag cloud shows 2 tacos instead of one bigger taco

Katrina, in order to test this, you better add a couple tags  on different biblios you can differentiate (both the biblios and the tags).

I did, and this is what I got:

https://snag.gy/VwboRH.jpg

The last two SELECT are after the maintenance script is run. So the source for truth is the 'tags_all' table. And, initially, the 'tags_approval' and 'tags_index' tables contain wrong calculated data, due to the fact that all emojis are considered the same.

The 'tags' table (which I hadn't had any idea about) is filled by misc/cronjobs/build_browser_and_cloud.pl. I would say leave that for another bug.
Comment 18 Tomás Cohen Arazi 2019-02-26 18:41:18 UTC
Created attachment 85730 [details] [review]
Bug 21846: (QA follow-up) Fix maintenance script path

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2019-02-26 19:16:15 UTC
(In reply to Katrin Fischer from comment #15)
> - But: tags table in the db is empty (should it be?)

The 'tags' table is used by a tool that is not related to the tags feature, but the subject cloud for french terms.

> - Tag cloud shows 2 tacos instead of one bigger taco

This seems to be a bug in master already, with or without emojis.
Comment 20 Jonathan Druart 2019-02-26 19:55:31 UTC
1. Maybe we should display the WARNING during the updatedb process only if really needed?

2. How do I test the maintenance script?

3. Why no module for tags_all?
Comment 21 Jonathan Druart 2019-02-26 20:03:01 UTC
Created attachment 85740 [details] [review]
Bug 21846: Fix pod2usage return values

According to existing pattern.
However I am not sure it makes sense to return an errno (and print to STDERR)
if --help is passed.
Comment 22 Jonathan Druart 2019-02-26 20:03:09 UTC
Created attachment 85741 [details] [review]
Bug 21846: Simplify ugly double for loops
Comment 23 Katrin Fischer 2019-02-27 00:47:09 UTC
(In reply to Tomás Cohen Arazi from comment #19)
> (In reply to Katrin Fischer from comment #15)
> > - But: tags table in the db is empty (should it be?)
> 
> The 'tags' table is used by a tool that is not related to the tags feature,
> but the subject cloud for french terms.

OK

> 
> > - Tag cloud shows 2 tacos instead of one bigger taco
> 
> This seems to be a bug in master already, with or without emojis.

You are right, this is a regression. I am not sure when it was introduced, but we need to fix it. The font should go bigger with increased use of a tag to get the normal cloud picture. I know for sure it still works in 17.11 but could not test further.

I also had added multiple tags according to Nicks test plan in comment 1. The taco was extra :)
Comment 24 Tomás Cohen Arazi 2019-02-27 01:39:47 UTC
(In reply to Katrin Fischer from comment #23)
> (In reply to Tomás Cohen Arazi from comment #19)
> > (In reply to Katrin Fischer from comment #15)
> > > - But: tags table in the db is empty (should it be?)
> > 
> > The 'tags' table is used by a tool that is not related to the tags feature,
> > but the subject cloud for french terms.
> 
> OK
> 
> > 
> > > - Tag cloud shows 2 tacos instead of one bigger taco
> > 
> > This seems to be a bug in master already, with or without emojis.
> 
> You are right, this is a regression. I am not sure when it was introduced,
> but we need to fix it. The font should go bigger with increased use of a tag
> to get the normal cloud picture. I know for sure it still works in 17.11 but
> could not test further.

I filled bug 22420, and Jonathan provided a fix already.

> I also had added multiple tags according to Nicks test plan in comment 1.
> The taco was extra :)

Ok
Comment 25 Tomás Cohen Arazi 2019-02-27 01:46:25 UTC
*** Bug 21247 has been marked as a duplicate of this bug. ***
Comment 26 Tomás Cohen Arazi 2019-02-27 10:46:46 UTC
(In reply to Jonathan Druart from comment #22)
> Created attachment 85741 [details] [review] [review]
> Bug 21846: Simplify ugly double for loops

C'mon!
Comment 27 Josef Moravec 2019-02-27 11:23:42 UTC
Created attachment 85758 [details] [review]
Bug 21846: Add Koha::Tags::Approval(s) and Koha::Tags::Index(es)

In order to avoid writing the tests using plain DBIC which would later
need to be replaced by a Koha::Object-based counterpart, I introduce
this stub classes.

Stub tests are added as well.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Tags.t \
          t/db_dependent/Koha/Tags/Approvals.t \
          t/db_dependent/Koha/Tags/Indexes.t
=> SUCCESS: Tests pass
- Sign off :-D

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 28 Josef Moravec 2019-02-27 11:23:46 UTC
Created attachment 85759 [details] [review]
Bug 21846: Regression tests for add_tag_approval

Due to collation issues, add_tag_approval wrongly calculates
the weight for strings including extended UNICODE characters.

This patch introduces a test for this situation.

To test:
- Apply this patch on master
- Run:
  $ kshell
 k$ prove t/db_dependent/Tags.t
=> FAIL: Tests fail because all extended characters match the same

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 29 Josef Moravec 2019-02-27 11:23:49 UTC
Created attachment 85760 [details] [review]
Bug 21846: Make 'term' use utf8mb_bin collation on tags tables

This patch makes the utf8mb4_bin collation preferred for comparing tags.
Otherwise suppolemental unicode characters all match.

To test:

- Enable tags and disable moderation (or plan to moderate and accept tags)
- Tag 3 records:
   a - with 🐋
   b - with 🌮
   c - with 👍
- Note the weight on each says '3'
- Click the tag to search, you get back all the records
- Apply the previous patches from this bug report
- Run:
  $ kshell
 k$ prove t/db_dependent/Tags.t
=> FAIL: Tests fail, related to counting stuffs
- Apply this patch and
(a) Run updatedatabase to upgrade the schema
- Run:
 k$ prove t/db_dependent/Tags.t
=> SUCCESS: Tests pass!
(b) reset_all to get a fresh DB using kohastructure.sql
- Run:
 k$ prove t/db_dependent/Tags.t
=> SUCCESS: Tests pass too!
- Sign off :-D

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 30 Josef Moravec 2019-02-27 11:23:53 UTC
Created attachment 85761 [details] [review]
Bug 21846: (follow-up) Add maintenance script

This patch adds a maintenance script that generates the missing
tags_approval entries based on the tags_all table, and then recalculates
the weights for both tags_approval and tags_index tables.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 31 Josef Moravec 2019-02-27 11:23:56 UTC
Created attachment 85762 [details] [review]
Bug 21846: (QA follow-up) Fix maintenance script path

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 32 Josef Moravec 2019-02-27 11:24:00 UTC
Created attachment 85763 [details] [review]
Bug 21846: Fix pod2usage return values

According to existing pattern.
However I am not sure it makes sense to return an errno (and print to STDERR)
if --help is passed.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 33 Josef Moravec 2019-02-27 11:24:04 UTC
Created attachment 85764 [details] [review]
Bug 21846: Simplify ugly double for loops

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 34 Josef Moravec 2019-02-27 11:25:33 UTC
The one QA tools complain is false positive here.

Tests are passing.

I tested maintance script by setting bad data in database manually and let the script fix it ;)
Comment 35 Jonathan Druart 2019-02-27 11:52:24 UTC
(In reply to Jonathan Druart from comment #20)
> 1. Maybe we should display the WARNING during the updatedb process only if
> really needed?
> 
> 2. How do I test the maintenance script?
> 
> 3. Why no module for tags_all?

This is without answer, setting back to SO.
Comment 36 Tomás Cohen Arazi 2019-02-27 11:53:21 UTC
(In reply to Jonathan Druart from comment #35)
> (In reply to Jonathan Druart from comment #20)
> > 1. Maybe we should display the WARNING during the updatedb process only if
> > really needed?
> > 
> > 2. How do I test the maintenance script?
> > 
> > 3. Why no module for tags_all?
> 
> This is without answer, setting back to SO.

There is Koha::Tags for that.
Comment 37 Tomás Cohen Arazi 2019-02-28 22:06:26 UTC
(In reply to Jonathan Druart from comment #20)
> 1. Maybe we should display the WARNING during the updatedb process only if
> really needed?
> 
> 2. How do I test the maintenance script?

To test:

- Enable tags and disable moderation (or plan to moderate and accept tags)
- Tag 3 records:
   a - with 🐋
   b - with 🌮
   c - with 👍
- Note the weight on each says '3'
- Click the tag to search, you get back all the records
- Apply the previous patches from this bug report
- Run:
  $ updatedatabase to upgrade the schema
  $ kshell
 k$ perl misc/maintenance/fix_tags_weight.pl
=> SUCCESS: the weights make sense
Comment 38 Jonathan Druart 2019-03-04 00:51:36 UTC
In my opinion we should fix bug 22420 first.
Comment 39 Tomás Cohen Arazi 2019-03-04 02:04:59 UTC
(In reply to Jonathan Druart from comment #38)
> In my opinion we should fix bug 22420 first.

No, 22420 won't be fixed if the weights are wrongly calculated, which is fixed here.
Comment 40 Jonathan Druart 2019-03-04 14:21:39 UTC
(In reply to Tomás Cohen Arazi from comment #39)
> (In reply to Jonathan Druart from comment #38)
> > In my opinion we should fix bug 22420 first.
> 
> No, 22420 won't be fixed if the weights are wrongly calculated, which is
> fixed here.

IIRC it affects any tags, not only emoji.
Comment 41 Tomás Cohen Arazi 2019-03-04 15:03:27 UTC
(In reply to Jonathan Druart from comment #40)
> (In reply to Tomás Cohen Arazi from comment #39)
> > (In reply to Jonathan Druart from comment #38)
> > > In my opinion we should fix bug 22420 first.
> > 
> > No, 22420 won't be fixed if the weights are wrongly calculated, which is
> > fixed here.
> 
> IIRC it affects any tags, not only emoji.

Yes, what I'm saying is that even though they are both about tags, they aren't really dependent as you propose.
Comment 42 Martin Renvoize 2019-03-11 19:02:16 UTC
This looks solid to me and seems to work.. can this go to PQA Josef.. or are you waiting on some further feedback here?
Comment 43 Josef Moravec 2019-03-11 20:19:13 UTC
I agree with Tomas, we should fix bug 22420 independently.

This patchset works, code is OK and fixes the described bug, so setting PQA
Comment 44 Nick Clemens 2019-03-15 12:47:26 UTC
Awesome work all!

Pushed to master for 19.05
Comment 45 Martin Renvoize 2019-03-19 10:04:01 UTC
Pushed to 18.11.x for 18.11.04
Comment 46 Lucas Gass 2019-03-26 23:27:05 UTC
not backporting to 18.05.x series
Comment 47 Michael Kuhn 2019-06-05 09:52:20 UTC
When updating from Koha 18.11 to Koha 19.05 the update process says:

Upgrade to 18.12.00.025 done (Bug 21846 - Using emoji as tags has broken weights)
WARNING: (Bug 21846) You need to manually run /usr/share/koha/intranet/cgi-bin/misc/maintenance/fix_tags_weight.pl to fix possible issues with tags.

But in fact the script "fix_tags_weight.pl" is located at "/usr/share/koha/bin/maintenance/fix_tags_weight.pl".
Comment 48 Tomás Cohen Arazi 2019-06-05 17:29:17 UTC
(In reply to Michael Kuhn from comment #47)
> When updating from Koha 18.11 to Koha 19.05 the update process says:
> 
> Upgrade to 18.12.00.025 done (Bug 21846 - Using emoji as tags has broken
> weights)
> WARNING: (Bug 21846) You need to manually run
> /usr/share/koha/intranet/cgi-bin/misc/maintenance/fix_tags_weight.pl to fix
> possible issues with tags.
> 
> But in fact the script "fix_tags_weight.pl" is located at
> "/usr/share/koha/bin/maintenance/fix_tags_weight.pl".

Thanks for reporting it. I will file a new bug for this.
Comment 49 Martin Renvoize 2019-06-13 16:07:59 UTC
bug 23056