Bug 27580

Summary: NULL values not correctly handled in Koha::Items->filter_by_visible_in_opac
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: Pushed to oldoldstable --- QA Contact: Katrin Fischer <katrin.fischer>
Severity: major    
Priority: P5 - low CC: andrew, caroline.cyr-la-rose, fridolin.somers, nick, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.03,20.05.09,19.11.15
Bug Depends on: 24254    
Bug Blocks:    
Attachments: Bug 27580: Regression tests
Bug 27580: Handle NULL values correctly in Koha::Items->filter_by_visible_in_opac
Bug 27580: Regression tests
Bug 27580: Handle NULL values correctly in Koha::Items->filter_by_visible_in_opac
Bug 27580: Regression tests
Bug 27580: Handle NULL values correctly in Koha::Items->filter_by_visible_in_opac

Description Tomás Cohen Arazi 2021-01-29 19:39:48 UTC
As pointed by @andrew on bug 15448, NULL values are not handled correctly here, because NOT IN <LIST> considers NULL to be included in <LIST>:

MariaDB [koha_kohadev]> SELECT itemnumber,copynumber FROM items WHERE itemnumber=1;
+------------+------------+
| itemnumber | copynumber |
+------------+------------+
|          1 | NULL       |
+------------+------------+
1 row in set (0.00 sec)

MariaDB [koha_kohadev]> SELECT itemnumber,copynumber FROM items WHERE itemnumber=1 AND copynumber NOT IN (2);
Empty set (0.00 sec)

MariaDB [koha_kohadev]> UPDATE items SET copynumber=1 WHERE itemnumber=1;
Query OK, 1 row affected (0.00 sec)
Rows matched: 1  Changed: 1  Warnings: 0

MariaDB [koha_kohadev]> SELECT itemnumber,copynumber FROM items WHERE itemnumber=1 AND copynumber NOT IN (2);
+------------+------------+
| itemnumber | copynumber |
+------------+------------+
|          1 | 1          |
+------------+------------+
1 row in set (0.00 sec)
Comment 1 Tomás Cohen Arazi 2021-01-29 19:46:54 UTC
Created attachment 116076 [details] [review]
Bug 27580: Regression tests

This test makes all the generated sample items have undef copynumber,
and all rules are added copynumber => [ 2 ] on the tests as
suggested by Andrew on his failing example.

The tests are expected to pass. i.e. copynumber is clearly not equal to
2, so those items shouldn't be hidden.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> FAIL: Tests fail! undef is considered in [ 2 ]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-01-29 19:46:58 UTC
Created attachment 116077 [details] [review]
Bug 27580: Handle NULL values correctly in Koha::Items->filter_by_visible_in_opac

This patch fixes the internally used query so it doesn't consider NULL
values as IN the set.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> FAIL: Tests fail :-/
3. Apply this patch
4. Repeat 2
=> SUCCESS: tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Andrew Fuerste-Henry 2021-01-29 20:20:27 UTC
Created attachment 116080 [details] [review]
Bug 27580: Regression tests

This test makes all the generated sample items have undef copynumber,
and all rules are added copynumber => [ 2 ] on the tests as
suggested by Andrew on his failing example.

The tests are expected to pass. i.e. copynumber is clearly not equal to
2, so those items shouldn't be hidden.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> FAIL: Tests fail! undef is considered in [ 2 ]

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 4 Andrew Fuerste-Henry 2021-01-29 20:20:30 UTC
Created attachment 116081 [details] [review]
Bug 27580: Handle NULL values correctly in Koha::Items->filter_by_visible_in_opac

This patch fixes the internally used query so it doesn't consider NULL
values as IN the set.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> FAIL: Tests fail :-/
3. Apply this patch
4. Repeat 2
=> SUCCESS: tests pass!
5. Sign off :-D

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 5 Katrin Fischer 2021-01-31 12:48:51 UTC
Created attachment 116094 [details] [review]
Bug 27580: Regression tests

This test makes all the generated sample items have undef copynumber,
and all rules are added copynumber => [ 2 ] on the tests as
suggested by Andrew on his failing example.

The tests are expected to pass. i.e. copynumber is clearly not equal to
2, so those items shouldn't be hidden.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> FAIL: Tests fail! undef is considered in [ 2 ]

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Katrin Fischer 2021-01-31 12:48:55 UTC
Created attachment 116095 [details] [review]
Bug 27580: Handle NULL values correctly in Koha::Items->filter_by_visible_in_opac

This patch fixes the internally used query so it doesn't consider NULL
values as IN the set.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> FAIL: Tests fail :-/
3. Apply this patch
4. Repeat 2
=> SUCCESS: tests pass!
5. Sign off :-D

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Jonathan Druart 2021-02-01 16:27:34 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 8 Fridolin Somers 2021-02-05 15:36:54 UTC
Pushed to 20.11.x for 20.11.03
Comment 9 Andrew Fuerste-Henry 2021-02-08 20:31:51 UTC
Pushed to 20.05.x for 20.05.09
Comment 10 Victor Grousset/tuxayo 2021-02-10 00:56:41 UTC
Backported: Pushed to 19.11.x branch for 19.11.15