Summary: | Staff interface detail page item table lookup fails when item has lost status but no claims returned | ||
---|---|---|---|
Product: | Koha | Reporter: | David Nind <david> |
Component: | Staff interface | Assignee: | David Cook <dcook> |
Status: | RESOLVED FIXED | QA Contact: | Katrin Fischer <katrin.fischer> |
Severity: | major | ||
Priority: | P5 - low | CC: | dcook, gmcharlt, jonathan.druart, lucas, matt.blenkinsop, phil |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
24.11.00
|
|
Circulation function: | |||
Bug Depends on: | 27919, 33568 | ||
Bug Blocks: | |||
Attachments: |
Bug 38248: Fix condition when item has no return_claims in API response
Bug 38248: Fix condition when item has no return_claims in API response Bug 38248: Do not deal with return claims if pref is not set Bug 38248: Do not deal with return claims if pref is not set |
Description
David Nind
2024-10-23 19:55:21 UTC
If you set "ClaimReturnedLostValue" to "Lost", you get a different error as well. I get a JS alert() that says: 400: Bad Request. Not in enum list: <with a long list that I'm not going to write out> -- I'm guessing I might just need to rebuild my API files... (In reply to David Cook from comment #1) > If you set "ClaimReturnedLostValue" to "Lost", you get a different error as > well. > > I get a JS alert() that says: > > 400: Bad Request. > Not in enum list: <with a long list that I'm not going to write out> > > -- > > I'm guessing I might just need to rebuild my API files... Yep, rebuild the swagger spec and restarted, and now it's fine. This should be an easy fix... Created attachment 173241 [details] [review] Bug 38248: Fix condition when item has no return_claims in API response This change just fixes a condition to not break when an item has no return_claims in an API response. Test plan: 0) Apply the patch and koha-plack --restart kohadev NOTE: You may need to rebuild your swagger spec since bug 27919 was pushed redocly bundle --ext json api/v1/swagger/swagger.yaml \ --output api/v1/swagger/swagger_bundle.json 1) Go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29 2) Create an item with a status of "Lost" 3) Go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29 4) Note that the item table loads without a problem 5) In system preferences, set the ClaimReturnedLostValue syspref to any value 6) Checkout an item to a patron 7) Click "Claim returned" and make the claim 8) Go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29 9) Note that the item table loads without a problem and "(Claimed returned") appears for the item that was claimed returned Created attachment 173250 [details] [review] Bug 38248: Fix condition when item has no return_claims in API response This change just fixes a condition to not break when an item has no return_claims in an API response. Test plan: 0) Apply the patch and koha-plack --restart kohadev NOTE: You may need to rebuild your swagger spec since bug 27919 was pushed redocly bundle --ext json api/v1/swagger/swagger.yaml \ --output api/v1/swagger/swagger_bundle.json 1) Go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29 2) Create an item with a status of "Lost" 3) Go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29 4) Note that the item table loads without a problem 5) In system preferences, set the ClaimReturnedLostValue syspref to any value 6) Checkout an item to a patron 7) Click "Claim returned" and make the claim 8) Go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29 9) Note that the item table loads without a problem and "(Claimed returned") appears for the item that was claimed returned Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Simple change and tested it. PQA. Pushed for 24.11! Well done everyone, thank you! I am wondering if the condition is correct. What would make sense IMO is to test for ClaimReturnedLostValue We should have row.return_claims set if the pref is set 248 [% IF Koha.Preference('ClaimReturnedLostValue') %] 249 embed.push('return_claims'); 250 [% END %] (In reply to Jonathan Druart from comment #7) > I am wondering if the condition is correct. What would make sense IMO is to > test for ClaimReturnedLostValue > > We should have row.return_claims set if the pref is set > > 248 [% IF Koha.Preference('ClaimReturnedLostValue') %] > 249 embed.push('return_claims'); > 250 [% END %] We now have claimed returns items with LOST values other than the ClaimReturendLostValue. (See bug 27919) So this is why this needed to be adjusted. Then remove the embed condition? rows.return_claims won't be set if the pref is not set. Should be on bug 27919 maybe. (In reply to Jonathan Druart from comment #9) > rows.return_claims won't be set if the pref is not set. row.return_claims not being set is the whole problem resolved by this patch (In reply to David Cook from comment #10) > (In reply to Jonathan Druart from comment #9) > > rows.return_claims won't be set if the pref is not set. > > row.return_claims not being set is the whole problem resolved by this patch Look at the code: 248 [% IF Koha.Preference('ClaimReturnedLostValue') %] 249 embed.push('return_claims'); 250 [% END %] 497 const hasReturnClaims = row.return_claims && row.return_claims.filter(rc => !rc.resolution).length > 0 ? true : false This means that if the pref is not set, row.return_claims won't be set (the key will not exist). If the pref is set, row.return_claims will be an array (eventually empty). A correct logic would be surround the 'hasReturnClaims' code with test on ClaimReturnedLostValue. if ( row.lost_status ) { let lost_lib = av_lost.get(row.lost_status.toString()) || _("Unavailable (lost or missing"); [% IF Koha.Preference('ClaimReturnedLostValue') %] const hasReturnClaims = row.return_claims.filter(rc => !rc.resolution).length > 0 ? true : false nodes += '<span class="lost">%s</span>'.format(escape_str(lost_lib)); if(hasReturnClaims) { nodes += '<span class="claimed_returned">' + _("(Claimed returned)") + '</span>'; } [% END %] } *BUT* (In reply to Katrin Fischer from comment #8) > (In reply to Jonathan Druart from comment #7) > > I am wondering if the condition is correct. What would make sense IMO is to > > test for ClaimReturnedLostValue > > > > We should have row.return_claims set if the pref is set > > > > 248 [% IF Koha.Preference('ClaimReturnedLostValue') %] > > 249 embed.push('return_claims'); > > 250 [% END %] > > We now have claimed returns items with LOST values other than the > ClaimReturendLostValue. (See bug 27919) So this is why this needed to be > adjusted. Then following the above, there is a flaw in the logic. If this is correct we should always embed return_claims, and remove the IF l.248 If you have the time to deal with it, I'd say go for it. Personally, I have bigger fish to fry... (In reply to David Cook from comment #12) > If you have the time to deal with it, I'd say go for it. Personally, I have > bigger fish to fry... Do you think I enjoy dealing with those silly things? Created attachment 173704 [details] [review] Bug 38248: Do not deal with return claims if pref is not set If the pref is not set we don't embed return_claims and so row.return_claims will never be set. When the pref is on, row.return_claims should always be an array. Test plan: Same as previous patch (In reply to Jonathan Druart from comment #14) > Created attachment 173704 [details] [review] [review] > Bug 38248: Do not deal with return claims if pref is not set > > If the pref is not set we don't embed return_claims and so > row.return_claims will never be set. > When the pref is on, row.return_claims should always be an array. > > Test plan: > Same as previous patch Requesting Matt's signoff, please. Created attachment 173708 [details] [review] Bug 38248: Do not deal with return claims if pref is not set If the pref is not set we don't embed return_claims and so row.return_claims will never be set. When the pref is on, row.return_claims should always be an array. Test plan: Same as previous patch Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Good catch, patch works as advertised and the logic makes sense Follow-up Bug 38248: Do not deal with return claims if pref is not set grabbed for push to main. Missing 24.05.x dependencies, no backport. |