Bug 31061 - OPACSuppress behavior with Elasticsearch
Summary: OPACSuppress behavior with Elasticsearch
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact:
URL:
Keywords:
Depends on: 23676
Blocks:
  Show dependency treegraph
 
Reported: 2022-06-28 19:42 UTC by Fridolin Somers
Modified: 2023-12-28 20:43 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 31061: Fix test suite (1.61 KB, patch)
2022-06-28 20:21 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 31061: Change type of suppress in Elasticsearch mappings (1.48 KB, patch)
2022-06-28 20:21 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 31061: OPACSuppress does not work properly with Elasticsearch (3.09 KB, patch)
2022-06-28 20:21 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 31061: OPACSuppress behavior with Elasticsearch (3.92 KB, patch)
2022-07-01 08:40 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2022-06-28 19:42:38 UTC
OPACSuppress is system preference to hide records in OPAC search by adding a limit in search query.

With Zebra, this limit is : not Suppress=1
https://git.koha-community.org/Koha-community/Koha/src/commit/244b847a08bf61a747a1e4be4eed86cc776eabd6/Koha/SearchEngine/Zebra/QueryBuilder.pm#L83
With Elasticsearch this limit is currently quite different : suppress:false
https://git.koha-community.org/Koha-community/Koha/src/commit/244b847a08bf61a747a1e4be4eed86cc776eabd6/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm#L286

Main problem is that in default framework we define Suppress search field on 942$n with authorized values YES_NO.
This generates records with 1 for YES and 0 for NO.
But a boolean for Elasticsearch is not a perl boolean.
So only missing subfield means false :
https://www.elastic.co/guide/en/elasticsearch/reference/6.8/boolean.html
Records with 0 are hidden !

Another side-effect :
We whould like to have several MARC fields with 'suppress' search field.
To distinguish several causes of hidden at OPAC.
Any of those fields having 1 must hide at OPAC.
So syntax 'NOT(suppress:1)' is more correct.

I propose we change to have the same behavior as Zebra.
Change 'suppress' search field to not remove boolean type.
Change QueryBuilder to use 'NOT(suppress:1)'.
Comment 1 Fridolin Somers 2022-06-28 20:21:50 UTC
Created attachment 136695 [details] [review]
Bug 31061: Fix test suite
Comment 2 Fridolin Somers 2022-06-28 20:21:54 UTC
Created attachment 136696 [details] [review]
Bug 31061: Change type of suppress in Elasticsearch mappings
Comment 3 Fridolin Somers 2022-06-28 20:21:57 UTC
Created attachment 136697 [details] [review]
Bug 31061: OPACSuppress does not work properly with Elasticsearch

OPACSuppress is system preference to hide records in OPAC search by adding a limit in search query.

With Zebra, this limit is : not Suppress=1
https://git.koha-community.org/Koha-community/Koha/src/commit/244b847a08bf61a747a1e4be4eed86cc776eabd6/Koha/SearchEngine/Zebra/QueryBuilder.pm#L83
With Elasticsearch this limit is currently quite different : suppress:false
https://git.koha-community.org/Koha-community/Koha/src/commit/244b847a08bf61a747a1e4be4eed86cc776eabd6/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm#L286

Main problem is that in default framework we define Suppress search field on 942$n with authorized values YES_NO.
This generates records with 1 for YES and 0 for NO.
But a boolean for Elasticsearch is not a perl boolean.
So only missing subfield means false :
https://www.elastic.co/guide/en/elasticsearch/reference/6.8/boolean.html
Records with 0 are hidden !

Another side-effect :
We whould like to have several MARC fields with 'suppress' search field.
To distinguish several causes of hidden at OPAC.
Any of those fields having 1 must hide at OPAC.
So syntax 'NOT(suppress:1)' is more correct.

I propose we change to have the same behavior as Zebra.
Change 'suppress' search field to not remove boolean type.
Change QueryBuilder to use 'NOT(suppress:1)'.

Test plan :
1) Use search engine Elasticsearch
2) Disable system preference OpacSuppression
3) Search in OPAC for all records : '*:*'
     => Note how many results
4) Enable system preference OpacSuppression
5) Edit a biblio record with 942$n set to YES
6) Search in OPAC for all records : '*:*'
     => Note how many results, you should have 1 less than 3)
7) Edit a biblio record with 942$n set to NO
8) Search in OPAC for all records : '*:*'
     => Note how many results, you should have same number as 3)
9) Edit a biblio record to remove 942$n
10) Search in OPAC for all records : '*:*'
     => Note how many results, you should have same number as 3)
Comment 4 Caroline Cyr La Rose 2022-06-30 13:30:02 UTC
I must not be completely awake, but I cannot reproduce the original "problematic" behaviour.

Without applying the patch, using ES, the number of records fluctuates according to 942$n being Yes (1) or No (0) and OpacSuppression being on or off.

There are 3 records with 1 in 942$n
With OpacSuppression = 405
Without OpacSuppression = 408
Comment 5 Fridolin Somers 2022-07-01 07:11:27 UTC
Ah indeed seems to be a code that convert 'perl boolean' value into true/false :
https://git.koha-community.org/Koha-community/Koha/src/commit/6c315c164dbcf3e05fcfe4a3110b824ab3ac162c/Koha/SearchEngine/Elasticsearch.pm#L950

I need to test again
Comment 6 Fridolin Somers 2022-07-01 08:26:18 UTC
Ahhhhh now that I've retested with KTD, indeed it works.
Maybe my search was too faster after unhiding record with 942$n=0.

> But a boolean for Elasticsearch is not a perl boolean.
Ouf its is thanks to a trick in the code.

I will change patches in order to only change to : NOT(suppress:true)
Comment 7 Fridolin Somers 2022-07-01 08:40:30 UTC
Created attachment 136799 [details] [review]
Bug 31061: OPACSuppress behavior with Elasticsearch

OPACSuppress is system preference to hide records in OPAC search by adding a limit in search query.

With Zebra, this limit is : not Suppress=1
https://git.koha-community.org/Koha-community/Koha/src/commit/244b847a08bf61a747a1e4be4eed86cc776eabd6/Koha/SearchEngine/Zebra/QueryBuilder.pm#L83
With Elasticsearch this limit is currently quite different : suppress:false
https://git.koha-community.org/Koha-community/Koha/src/commit/244b847a08bf61a747a1e4be4eed86cc776eabd6/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm#L286

We whould like to have several MARC fields with 'suppress' search field.
To distinguish several causes of hidden at OPAC.
Any of those fields having 1 must hide at OPAC.
Syntax 'NOT(suppress:true)' does that, like with Zebra.

Test plan :
1) Use search engine Elasticsearch
2) Disable system preference OpacSuppression
3) Search in OPAC for all records : '*:*'
     => Note how many results
4) Enable system preference OpacSuppression
5) Edit a biblio record with 942$n set to YES
6) Search in OPAC for all records : '*:*'
     => Note how many results, you should have 1 less than 3)
7) Edit a biblio record with 942$n set to NO
8) Search in OPAC for all records : '*:*'
     => Note how many results, you should have same number as 3)
9) Edit a biblio record to remove 942$n
10) Search in OPAC for all records : '*:*'
     => Note how many results, you should have same number as 3)
Comment 8 David Nind 2022-10-30 00:58:50 UTC
I had a go at testing using KTD and the sample data.

I couldn't replicate the issue before the patch was applied, and there were no differences in the expected results with 943$n set to Yes, No and removed.

I've set out what I did below.

Before the patch was applied
============================

Using Elasticsearch, index rebuilt (koha-elasticsearch --rebuild -d -b -a kohadev)
OpacSuppression = Don't hide: 435 records
OpacSuppression = Hide: 433 records
Records that are suppressed already in the sample data = 2: biblionumber 232 and 234 (used report to find https://wiki.koha-community.org/wiki/SQL_Reports_Library#Bibs_Suppressed_in_OPAC)
Recorded edited with OpacSuppression = Hide: 126 (Intermediate Perl) 
- 942$n = Yes: records = 432 (as expected) (435 - 2 records already hidden - 1 new record)
- 942$n = No: records = 433 (as expected)
- 942$n = removed: records = 433 (as expected)

Tests pass: prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t


After the patch is applied
==========================

Using Elasticsearch, index rebuilt (koha-elasticsearch --rebuild -d -b -a kohadev)
OpacSuppression = Don't hide: 435 records
OpacSuppression = Hide: 433 records
Records that are suppressed already in the sample data = 2: biblionumber 232 and 234 (used report to find https://wiki.koha-community.org/wiki/SQL_Reports_Library#Bibs_Suppressed_in_OPAC)
Recorded edited with OpacSuppression = Hide: 126 (Intermediate Perl) 
- 942$n = Yes: records = 432 (as expected) (435 - 2 records already hidden - 1 new record)
- 942$n = No: records = 433 (as expected)
- 942$n = removed: records = 433 (as expected)

Tests pass: prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
Comment 9 Fridolin Somers 2022-12-22 07:09:35 UTC
> We whould like to have several MARC fields with 'suppress' search field.
We abandonned this idea
Lets keep the actual code ;)