Bug 39842 - SIP current_location field is never sent
Summary: SIP current_location field is never sent
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-05-06 12:48 UTC by Nick Clemens (kidclamp)
Modified: 2025-05-16 15:20 UTC (History)
3 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 39842: Populate current location for SIP item with holdingbranch (1.55 KB, patch)
2025-05-06 12:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 39842: Populate current location for SIP item with holdingbranch (1.60 KB, patch)
2025-05-06 20:11 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 39842: Populate current location for SIP item with holdingbranch (1.67 KB, patch)
2025-05-08 12:43 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2025-05-06 12:48:41 UTC
We have AP => FID_CURRENT_LOCATION and that points to current_location in SIP items, however, we never populate that field so it is never returned in SIP responses.

Some SIP machines would like to be able to find hold items and list where they are waiting.
Comment 1 Nick Clemens (kidclamp) 2025-05-06 12:51:55 UTC
Created attachment 181969 [details] [review]
Bug 39842: Populate current location for SIP item with holdingbranch

This patch simply adds the holdingbranch to the SIP item as current_location

To test:
1 - Find an item and check it in at a different location than its homebranch
2 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -t CR -m item_information --item 39999000004151
3 - Note no AP field is returned
4 - Apply patch, restart all
5 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -t CR -m item_information --item 39999000004151
6 - You see the current holdingbranch in the AP field
7 - Return the item to its homebranch
8 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -t CR -m item_information --item 39999000004151
9 - Confirm home and holding branches are the same now
Comment 2 Roman Dolny 2025-05-06 20:11:55 UTC
Created attachment 181994 [details] [review]
Bug 39842: Populate current location for SIP item with holdingbranch

This patch simply adds the holdingbranch to the SIP item as current_location

To test:
1 - Find an item and check it in at a different location than its homebranch
2 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -t CR -m item_information --item 39999000004151
3 - Note no AP field is returned
4 - Apply patch, restart all
5 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -t CR -m item_information --item 39999000004151
6 - You see the current holdingbranch in the AP field
7 - Return the item to its homebranch
8 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -t CR -m item_information --item 39999000004151
9 - Confirm home and holding branches are the same now

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 3 David Cook 2025-05-07 06:15:48 UTC
We bumped into this one the other day as well. The vendor of the SIP client machine was able to work around it, but I was thinking this might be a good idea...

We'll need to check the unit tests for this one.
Comment 4 David Cook 2025-05-07 06:17:08 UTC
I wonder a bit if this will break anything for existing machines... hopefully not. 

Sometimes I wish we had feature flags or transitional system preferences that we could temporarily use to turn off a new feature until a fix can be developed.
Comment 5 Martin Renvoize (ashimema) 2025-05-08 12:38:02 UTC
Any chance of adding a unit test here Nick?
Comment 6 Martin Renvoize (ashimema) 2025-05-08 12:43:09 UTC
Created attachment 182078 [details] [review]
Bug 39842: Populate current location for SIP item with holdingbranch

This patch simply adds the holdingbranch to the SIP item as current_location

To test:
1 - Find an item and check it in at a different location than its homebranch
2 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -t CR -m item_information --item 39999000004151
3 - Note no AP field is returned
4 - Apply patch, restart all
5 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -t CR -m item_information --item 39999000004151
6 - You see the current holdingbranch in the AP field
7 - Return the item to its homebranch
8 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -t CR -m item_information --item 39999000004151
9 - Confirm home and holding branches are the same now

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 7 Martin Renvoize (ashimema) 2025-05-08 12:47:05 UTC
I decided we don't need to push for a unit test here.. none of the other fields are tested and it's a trivial patch.
Comment 8 David Cook 2025-05-09 00:38:03 UTC
(In reply to Martin Renvoize (ashimema) from comment #7)
> I decided we don't need to push for a unit test here.. none of the other
> fields are tested and it's a trivial patch.

That's good to know about the test. I was going to check through them but ran out of time.

I'm a bit anxious about this one, but it does seem like a sensible change. When I was talking to the hardware vendor, I couldn't figure out why on earth we're not sending AP. It was very reasonable that they wanted AP. 

I wonder a bit if we'll see unexpected consequences as a result of this sensible change though. 

I suppose worst case it gets reverted, and best case it just works. 

I think I rather see it go in than get blocked, so thumbs up from me I think.
Comment 9 Katrin Fischer 2025-05-09 06:31:26 UTC
Decided to push, but adding Kyle to get another opinion.
Comment 10 Katrin Fischer 2025-05-09 07:14:45 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 11 Kyle M Hall (khall) 2025-05-12 12:24:55 UTC
This patch is missing unit tests and I'm concerned this current location will be incorrect during checkins that trigger a transfer to a different branch.
Comment 12 David Cook 2025-05-13 02:20:46 UTC
(In reply to Kyle M Hall (khall) from comment #11)
> This patch is missing unit tests and I'm concerned this current location
> will be incorrect during checkins that trigger a transfer to a different
> branch.

+1
Comment 13 Katrin Fischer 2025-05-13 06:25:48 UTC
Do you suggest a revert or could we make this configurable?
Comment 14 Nick Clemens (kidclamp) 2025-05-13 16:31:05 UTC
(In reply to Kyle M Hall (khall) from comment #11)
> This patch is missing unit tests and I'm concerned this current location
> will be incorrect during checkins that trigger a transfer to a different
> branch.

The field is only sent during item_information requests - so the information won't be changing during the request, so I don't understand the concern here.

I agree we could expand tests to cover this on a new bug
Comment 15 Kyle M Hall (khall) 2025-05-14 11:06:21 UTC
> The field is only sent during item_information requests - so the information
> won't be changing during the request, so I don't understand the concern here.

Agreed. The field is used in the checkin *request*, not the response, so my concerns are invalid.
Comment 16 Katrin Fischer 2025-05-16 15:20:48 UTC
(In reply to Kyle M Hall (khall) from comment #15)
> > The field is only sent during item_information requests - so the information
> > won't be changing during the request, so I don't understand the concern here.
> 
> Agreed. The field is used in the checkin *request*, not the response, so my
> concerns are invalid.

Thanks!