Bug 39842

Summary: SIP current_location field is never sent
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: SIP2Assignee: Nick Clemens (kidclamp) <nick>
Status: Needs documenting --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: dcook, fridolin.somers, kyle, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 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
Bug 39842: Populate current location for SIP item with holdingbranch
Bug 39842: Populate current location for SIP item with holdingbranch

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!
Comment 17 Fridolin Somers 2025-10-03 13:31:53 UTC
Pushed to 24.11.x for 24.11.10
Comment 18 David Cook 2025-10-07 02:38:07 UTC
(In reply to Fridolin Somers from comment #17)
> Pushed to 24.11.x for 24.11.10

I wonder if we should've classified this one as an enhancement...

(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.

Trying 'item_information'
SEND: 1700120251007    023043AOCPL|AB39999000004151|ACkoha|
READ:
1810020120251007    023043AB39999000004151|AJUn chien andalou =|AQCPL|APFFL|CRNFIC|BGCPL|

Kiosks from FE Technologies use the item_information and look for AP to see if they need to do a branch transfer after a SIP checkin.

Overall, I think it's a very good thing that we're adding it into the SIP item_information response, as it provides SIP clients with more accurate data... it is possible that it could change the behaviour of some existing SIP clients in an unexpected way / at an unexpected time.

That's why we might not want to backport it.

But it's probably low risk, and it could provide people on older versions of Koha with a better SIP service. 

Anyway, just my 2 cents.
Comment 19 David Cook 2025-10-07 02:38:41 UTC
On a related note, does anyone have a "term1" user on their KTD? I couldn't get the test plan to work. I had to use the "koha" user.
Comment 20 Fridolin Somers 2025-10-07 11:38:28 UTC
OK thanks for the alert David
I revert from 24.11.x (not yet released) for behavior stability in LTS