Summary: | SIP current_location field is never sent | ||
---|---|---|---|
Product: | Koha | Reporter: | Nick Clemens (kidclamp) <nick> |
Component: | SIP2 | Assignee: | Nick Clemens (kidclamp) <nick> |
Status: | Pushed to main --- | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | normal | ||
Priority: | P5 - low | CC: | dcook, kyle, martin.renvoize |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
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
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
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 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> 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. 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. Any chance of adding a unit test here Nick? 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> 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. (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. Decided to push, but adding Kyle to get another opinion. Pushed for 25.05! Well done everyone, thank you! 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. (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 Do you suggest a revert or could we make this configurable? (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 > 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.
(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! |