If I set the lost status for an item to 1 (Lost), then when viewing the record in the staff interface, it sits on "Processing". To replicate: 1. Find any record in the staff interface with an existing item and add a new item. 2. Note that when you select the "Normal" view, the holdings section lists the existing item and the new one that you created. 3. Edit the item you created and change "1 - Lost status" to "Lost". 4. Select the "Normal" view for the record again and note that a dialog box appears saying "Processing" and the holdings table does not list any items.
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.