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 [U+1F40B] b - with [U+1F32E] c - with [U+1F44D] 3 - Note the weight on each says '3' 4 - Click the tag to search, you get back all the records
Created attachment 82621 [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
Created attachment 82622 [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
Created attachment 82623 [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 [U+1F40B] b - with [U+1F32E] c - with [U+1F44D] - 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
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.
Created attachment 83717 [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>
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>
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>
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 [U+1F40B] b - with [U+1F32E] c - with [U+1F44D] - 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>
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>
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.
(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.
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?
The script I introduce doesn't have merge markers in it, it has some strings it prints in verbose mode.
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 => [U+1F32E] - 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
So we don't forget again: SET NAMES utf8mb4; will help with SQL.
(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 => [U+1F32E] > - 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.
Created attachment 85730 [details] [review] Bug 21846: (QA follow-up) Fix maintenance script path Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(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.
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?
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.
Created attachment 85741 [details] [review] Bug 21846: Simplify ugly double for loops
(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 :)
(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
*** Bug 21247 has been marked as a duplicate of this bug. ***
(In reply to Jonathan Druart from comment #22) > Created attachment 85741 [details] [review] [review] > Bug 21846: Simplify ugly double for loops C'mon!
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>
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>
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 [U+1F40B] b - with [U+1F32E] c - with [U+1F44D] - 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>
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>
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>
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>
Created attachment 85764 [details] [review] Bug 21846: Simplify ugly double for loops Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
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 ;)
(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.
(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.
(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 [U+1F40B] b - with [U+1F32E] c - with [U+1F44D] - 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
In my opinion we should fix bug 22420 first.
(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.
(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.
(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.
This looks solid to me and seems to work.. can this go to PQA Josef.. or are you waiting on some further feedback here?
I agree with Tomas, we should fix bug 22420 independently. This patchset works, code is OK and fixes the described bug, so setting PQA
Awesome work all! Pushed to master for 19.05
Pushed to 18.11.x for 18.11.04
not backporting to 18.05.x series
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".
(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.
bug 23056