Bug 36431 - SIP2: Checkin does not show difference between WasTransfered and NeedsTransfer
Summary: SIP2: Checkin does not show difference between WasTransfered and NeedsTransfer
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-03-26 14:41 UTC by Marcel de Rooy
Modified: 2024-04-22 10:05 UTC (History)
1 user (show)

See Also:
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 (1.83 KB, patch)
2024-04-22 10:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36431: Unit tests (2.36 KB, patch)
2024-04-22 10:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.