Bug 27580 - NULL values not correctly handled in Koha::Items->filter_by_visible_in_opac
Summary: NULL values not correctly handled in Koha::Items->filter_by_visible_in_opac
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 24254
Blocks:
  Show dependency treegraph
 
Reported: 2021-01-29 19:39 UTC by Tomás Cohen Arazi
Modified: 2022-06-06 20:25 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.03,20.05.09,19.11.15


Attachments
Bug 27580: Regression tests (4.82 KB, patch)
2021-01-29 19:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27580: Handle NULL values correctly in Koha::Items->filter_by_visible_in_opac (1.27 KB, patch)
2021-01-29 19:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27580: Regression tests (4.89 KB, patch)
2021-01-29 20:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 27580: Handle NULL values correctly in Koha::Items->filter_by_visible_in_opac (1.33 KB, patch)
2021-01-29 20:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 27580: Regression tests (4.94 KB, patch)
2021-01-31 12:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27580: Handle NULL values correctly in Koha::Items->filter_by_visible_in_opac (1.39 KB, patch)
2021-01-31 12:48 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 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