Bug 31381 - Searching patrons by letter broken when using non-mandatory extended attributes
Summary: Searching patrons by letter broken when using non-mandatory extended attributes
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 32331
  Show dependency treegraph
 
Reported: 2022-08-17 10:43 UTC by Oliver Behnke
Modified: 2023-06-08 22:32 UTC (History)
6 users (show)

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


Attachments
Bug 31381: Handle null attributes in list (789 bytes, patch)
2022-11-22 19:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31381: Add tests (1.80 KB, patch)
2022-11-23 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31381: Handle null attributes in list (1.38 KB, patch)
2022-11-23 10:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31381: Add tests (1.86 KB, patch)
2022-11-23 10:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31381: Handle null attributes in list (1.43 KB, patch)
2022-11-30 18:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31381: Add tests (1.91 KB, patch)
2022-11-30 18:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Behnke 2022-08-17 10:43:49 UTC
Hi,

searching for patrons by letter like this returns an empty list:

/cgi-bin/koha/members/members-home.pl?firstletter=A

I find the following in intranet-error.log:

[cgi:error] AH01215: [warn] OpenAPI >>> GET api/v1/patrons [{"message":"Expected string - got null.","path":"\\/body\\/10\\/extended_attributes\\/0\\/value"},{"message":"Expected string - got null.","path":"\\/body\\/14\\/extended_attributes\\/0\\/value"}]: /usr/share/koha/api/v1/app.pl, referer: /cgi-bin/koha/members/members-home.pl?firstletter=A

We have defined a single extended attribute which isn't mandatory, so it might not exist for some patrons. If I interpret "10" and "14" as representing borrower_attributes.borrowernumber (table.attribute) values then "10" does indeed lack an entry, but "14" has one. Not a smoking gun yet...

Maybe this is a regression due to this?
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27857

Cheers
Comment 1 Oliver Behnke 2022-08-17 10:48:56 UTC
Note: the empty list isn't returned for all letters, so I presume that some patron DB entries break the list creation, probably those with the extended attribute "missing". I'll try to confirm that.
Comment 2 Oliver Behnke 2022-08-17 11:00:04 UTC
Gotcha! It's not the borrowers with the extended attribute "missing" but rather those which (existing) borrower_attributes.attribute is NULL.

I'd argue that a NULL value should be treated like a missing attribute or like an empty string. I doubt it's intended to cause the error above and the subsequent patron list issue.

Hope this helps
Comment 3 Owen Leonard 2022-08-17 11:17:27 UTC
I can reproduce this problem in master by setting a patron attribute on the patron edit screen and then editing the values in the database to be NULL.
Comment 4 Oliver Behnke 2022-08-17 11:55:09 UTC
I can also confirm that editing/saving an affected patron's NULL attribute  via the intranet effectively removes it from borrower_attributes. Doing so for all affected patrons of a given letter (here "A") fixes the patron list issue for that letter.
Comment 5 Jonathan Druart 2022-08-17 12:54:06 UTC
The REST API spec is not expecting NULL/undef for an attribute's value. As the UI is not allowing undef I am not sure this is a bug.
Comment 6 Jonathan Druart 2022-08-17 12:56:56 UTC
If we need to fix this, we could apply this:

diff --git a/api/v1/swagger/definitions/patron_extended_attribute.yaml b/api/v1/swagger/definitions/patron_extended_attribute.yaml
index 84343d258b1..adb42944b80 100644
--- a/api/v1/swagger/definitions/patron_extended_attribute.yaml
+++ b/api/v1/swagger/definitions/patron_extended_attribute.yaml
@@ -6,7 +6,9 @@ properties:
     type: integer
   type:
     description: Extended attribute type
-    type: string
+    type:
+      - string
+      - "null"
   value:
     description: Extended attribute value
     type:


And adjust the tests.
Comment 7 Jonathan Druart 2022-08-17 12:57:23 UTC
And certainly as well api/v1/swagger/paths/patrons_extended_attributes.yaml
Comment 8 Oliver Behnke 2022-08-18 06:02:25 UTC
If you decide against allowing/preparing for NULL values then the DB schema should probably be changed to define borrower_attributes.attribute as not nullable. This way whatever tries to do that would be caught right away, without causing subsequent errors down the line. Ideally this would also allow to find the root cause.
Comment 9 Jonathan Druart 2022-08-18 08:21:32 UTC
Yes, Oliver, I think it's the way to go.

Asking for more opinions, Nick what do you think?
Comment 10 Nick Clemens 2022-08-26 17:08:49 UTC
(In reply to Jonathan Druart from comment #9)
> Yes, Oliver, I think it's the way to go.
> 
> Asking for more opinions, Nick what do you think?

Asking Tomas :-)
Comment 11 Nick Clemens 2022-11-22 19:22:02 UTC
Created attachment 144173 [details] [review]
Bug 31381: Handle null attributes in list
Comment 12 Nick Clemens 2022-11-22 19:23:34 UTC
This is affecting stables and preventing patron searching, we can handle it via data as well, but we should patch this to prevent failures and then forbid null attributes at the DB level on another bug
Comment 13 Jonathan Druart 2022-11-22 20:12:13 UTC
(In reply to Jonathan Druart from comment #6)
> And adjust the tests.

(In reply to Jonathan Druart from comment #7)
> And certainly as well api/v1/swagger/paths/patrons_extended_attributes.yaml

What about that, Nick?
Comment 14 Nick Clemens 2022-11-22 20:13:49 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to Jonathan Druart from comment #6)
> > And adjust the tests.
> 
> (In reply to Jonathan Druart from comment #7)
> > And certainly as well api/v1/swagger/paths/patrons_extended_attributes.yaml
> 
> What about that, Nick?

The tests pass - I can add a test that a null value doesn't fail

For the second, I don't think we should add support to set attributes as null - we should prevent it wholly on another bug
Comment 15 Jonathan Druart 2022-11-23 08:30:56 UTC
Created attachment 144188 [details] [review]
Bug 31381: Add tests
Comment 16 Nick Clemens 2022-11-23 10:56:41 UTC
Created attachment 144196 [details] [review]
Bug 31381: Handle null attributes in list

This patch allows null values to be returned in patron attributes
to prevent a crash when searching patrons

To test:
1 - Add a NULL attribute to a borrower, in sample data, Edna Acosta
    sudo koha-mysql kohadev
    INSERT INTO borrower_attributes (borrowernumber,code,attribute) VALUES (5,'SHOW_BCODE',NULL);
2 - Browse to 'Home->Patrons'
    http://localhost:8081/cgi-bin/koha/members/members-home.pl
3 - Click 'Browse by last name: A'
4 - Datatables error:
    Expected string - got null
5 - Apply patch
6 - Restart all
7 - Browse by last name: A
8 - Success!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2022-11-23 10:56:45 UTC
Created attachment 144197 [details] [review]
Bug 31381: Add tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Tomás Cohen Arazi 2022-11-30 18:31:03 UTC
Works, makes sense, QA script happy. PQA
Comment 19 Tomás Cohen Arazi 2022-11-30 18:32:55 UTC
Created attachment 144351 [details] [review]
Bug 31381: Handle null attributes in list

This patch allows null values to be returned in patron attributes
to prevent a crash when searching patrons

To test:
1 - Add a NULL attribute to a borrower, in sample data, Edna Acosta
    sudo koha-mysql kohadev
    INSERT INTO borrower_attributes (borrowernumber,code,attribute) VALUES (5,'SHOW_BCODE',NULL);
2 - Browse to 'Home->Patrons'
    http://localhost:8081/cgi-bin/koha/members/members-home.pl
3 - Click 'Browse by last name: A'
4 - Datatables error:
    Expected string - got null
5 - Apply patch
6 - Restart all
7 - Browse by last name: A
8 - Success!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2022-11-30 18:33:15 UTC
Created attachment 144352 [details] [review]
Bug 31381: Add tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2022-11-30 18:59:32 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 22 Jacob O'Mara 2022-12-02 10:41:31 UTC
Nice work! Pushed to 22.11.x