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
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.
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
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.
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.
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.
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.
And certainly as well api/v1/swagger/paths/patrons_extended_attributes.yaml
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.
Yes, Oliver, I think it's the way to go. Asking for more opinions, Nick what do you think?
(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 :-)
Created attachment 144173 [details] [review] Bug 31381: Handle null attributes in list
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
(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?
(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
Created attachment 144188 [details] [review] Bug 31381: Add tests
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>
Created attachment 144197 [details] [review] Bug 31381: Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Works, makes sense, QA script happy. PQA
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>
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>
Pushed to master for 22.11. Nice work everyone, thanks!
Nice work! Pushed to 22.11.x