Bug 36431

Summary: SIP2: Checkin does not show difference between WasTransfered and NeedsTransfer
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: SIP2Assignee: Marcel de Rooy <m.de.rooy>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: kyle
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 36431: Add screen message for NeedsTransfer when AutomaticItemReturn is off
Bug 36431: Unit tests
Bug 36431: Add screen message for NeedsTransfer when AutomaticItemReturn is off
Bug 36431: Add screen message for NeedsTransfer when AutomaticItemReturn is off
Bug 36431: Unit tests

Description Marcel de Rooy 2024-03-26 14:41:02 UTC
If you enable AutomaticItemReturn, SIP will trigger an eventual transfer to home branch when checking in. If disabled, it will not but the station receives an alert to notify the user.

But the SIP response for both cases is actually the same. The code shows this:

    if ($messages->{NeedsTransfer}) {
        $self->{item}->destination_loc($messages->{NeedsTransfer});
        $self->alert_type('04');            # send to other branch
    }
    if ($messages->{WasTransfered}) { # set into transit so tell unit
        $self->{item}->destination_loc($item->homebranch);
        $self->alert_type('04');            # send to other branch
    }

The destination location will be the same in both cases. And the alert number is the same too. How could the SIP station 'know' that the transfer is done already or still needs to?

Both SIP responses contains AO holding branch, AQ destination, CT destination and CV 04.
Comment 1 Marcel de Rooy 2024-03-26 14:52:58 UTC
As I understand, we cannot change the CV value. This SIP extension stands for "send to other branch".

But would it make sense to populate for instance the AF field [screen message] in the response to show that the transfer was already actually created (when AddReturn responded with WasTransfered) ?
Comment 2 Kyle M Hall (khall) 2024-03-26 17:31:10 UTC
(In reply to Marcel de Rooy from comment #1)
> As I understand, we cannot change the CV value. This SIP extension stands
> for "send to other branch".
> 
> But would it make sense to populate for instance the AF field [screen
> message] in the response to show that the transfer was already actually
> created (when AddReturn responded with WasTransfered) ?

CV is non-standard but is documented at https://help.oclc.org/Library_Management/Amlib/SIP2/SIP2_Manager_procedure/170Media_Types

01 - hold for this library
02 - hold for other branch
04 - send to other branch

In both of those code paths, there is indeed a real transfer. In the case of WasTransfered, it means a transfer was initiated, but the item has been checked in at the library the transfer originated from, so the destination and location in this case are not the same. Am I mistaken?
Comment 3 Marcel de Rooy 2024-03-27 12:36:17 UTC
(In reply to Kyle M Hall from comment #2)
> In both of those code paths, there is indeed a real transfer. In the case of
> WasTransfered, it means a transfer was initiated, but the item has been
> checked in at the library the transfer originated from, so the destination
> and location in this case are not the same. Am I mistaken?

We are not talking about the same situation. You describe a valid case where a transfer exists. But I am looking at the situation where the item is in another branch and has been checked out and checked in again. In that case WasTransfered can be set also (depending on AutomaticItemReturn):

Circulation L2519-
        if  (C4::Context->preference("AutomaticItemReturn"    ) or
            (C4::Context->preference("UseBranchTransferLimits") and
             ! IsBranchTransferAllowed($branch, $returnbranch, $item->$BranchTransferLimitsType )
           )) {
            ModItemTransfer($item->itemnumber, $branch, $returnbranch, $transfer_trigger, { skip_record_index => 1 });
            $messages->{'WasTransfered'} = $returnbranch;
            $messages->{'TransferTrigger'} = $transfer_trigger;
        } else {
            $messages->{'NeedsTransfer'} = $returnbranch;
            $messages->{'TransferTrigger'} = $transfer_trigger;
        }
Comment 4 Kyle M Hall (khall) 2024-03-27 16:41:40 UTC
> We are not talking about the same situation. You describe a valid case where
> a transfer exists. But I am looking at the situation where the item is in
> another branch and has been checked out and checked in again. In that case
> WasTransfered can be set also (depending on AutomaticItemReturn):

Would you be able to write up a test plan demonstrating the incorrect behavior and what should be happening instead?
Comment 5 Marcel de Rooy 2024-04-22 10:04:26 UTC
Created attachment 165258 [details] [review]
Bug 36431: Add screen message for NeedsTransfer when AutomaticItemReturn is off

Test plan:
Pick an item (without holds) with homebranch A and holdingbranch B.
(My SIP examples use RMA for A, NRD for B.)
Enable AutomaticItemReturn.
Check in this item at branch B via SIP.
* 09N20240422    10000020240422    160000APNRD|AONRD|ABperl01|
You should see something like:
* 101YNY20240422    105736AONRD|ABperl01|AQRMA|AJLearning Perl /|CSH|CTRMA|CV04|
Verify too that a transfer was started.
Now disable AutomaticItemReturn. Remove the transfer.
Check in this item at branch B via SIP.
You should see now something like:
101YNY20240422    105515AONRD|ABperl01|AQRMA|AJLearning Perl /|CSH|CTRMA|CV04|AFThis item must still be transferred to RMA branch.|
Verify that no transfer was started.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2024-04-22 10:04:28 UTC
Created attachment 165259 [details] [review]
Bug 36431: Unit tests

Test plan:
Run t/db_dependent/SIP/Message.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2024-04-22 10:05:48 UTC
(In reply to Kyle M Hall from comment #4)
> > We are not talking about the same situation. You describe a valid case where
> > a transfer exists. But I am looking at the situation where the item is in
> > another branch and has been checked out and checked in again. In that case
> > WasTransfered can be set also (depending on AutomaticItemReturn):
> 
> Would you be able to write up a test plan demonstrating the incorrect
> behavior and what should be happening instead?

See patch and test plan.
Chose to add the message to the case where AutomaticItemReturn is off (by far most installs have it enabled). So no change for them.
Comment 8 Kyle M Hall (khall) 2025-01-16 14:55:35 UTC
Created attachment 176670 [details] [review]
Bug 36431: Add screen message for NeedsTransfer when AutomaticItemReturn is off

Test plan:
Pick an item (without holds) with homebranch A and holdingbranch B.
(My SIP examples use RMA for A, NRD for B.)
Enable AutomaticItemReturn.
Check in this item at branch B via SIP.
* 09N20240422    10000020240422    160000APNRD|AONRD|ABperl01|
You should see something like:
* 101YNY20240422    105736AONRD|ABperl01|AQRMA|AJLearning Perl /|CSH|CTRMA|CV04|
Verify too that a transfer was started.
Now disable AutomaticItemReturn. Remove the transfer.
Check in this item at branch B via SIP.
You should see now something like:
101YNY20240422    105515AONRD|ABperl01|AQRMA|AJLearning Perl /|CSH|CTRMA|CV04|AFThis item must still be transferred to RMA branch.|
Verify that no transfer was started.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Marcel de Rooy 2025-01-16 14:58:22 UTC
(In reply to Kyle M Hall (khall) from comment #8)
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Great. Thanks!
Comment 10 Kyle M Hall (khall) 2025-01-16 15:10:48 UTC
Created attachment 176671 [details] [review]
Bug 36431: Add screen message for NeedsTransfer when AutomaticItemReturn is off

Test plan:
Pick an item (without holds) with homebranch A and holdingbranch B.
(My SIP examples use RMA for A, NRD for B.)
Enable AutomaticItemReturn.
Check in this item at branch B via SIP.
* 09N20240422    10000020240422    160000APNRD|AONRD|ABperl01|
You should see something like:
* 101YNY20240422    105736AONRD|ABperl01|AQRMA|AJLearning Perl /|CSH|CTRMA|CV04|
Verify too that a transfer was started.
Now disable AutomaticItemReturn. Remove the transfer.
Check in this item at branch B via SIP.
You should see now something like:
101YNY20240422    105515AONRD|ABperl01|AQRMA|AJLearning Perl /|CSH|CTRMA|CV04|AFThis item must still be transferred to RMA branch.|
Verify that no transfer was started.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall (khall) 2025-01-16 15:10:54 UTC
Created attachment 176672 [details] [review]
Bug 36431: Unit tests

Test plan:
Run t/db_dependent/SIP/Message.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>