Bug 12021

Summary: SIP2 checkin should alert on transfer and use CT for return branch
Product: Koha Reporter: Benjamin Rokseth <benjamin.rokseth>
Component: SIP2Assignee: Benjamin Rokseth <benjamin.rokseth>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: boutrosboutrosboutros, colin.campbell, julian.maurice, katrin.fischer, kyle, m.de.rooy, martin.renvoize, mtj, veron
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 12021 - SIP2 checkin should alert on transfer and use CT for return branch
[SIGNED-OFF] Bug 12021 - SIP2 checkin should alert on transfer and use CT for return branch
Bug 12021 - SIP2 checkin should alert on transfer and use CT for return branch
Bug 12021 - SIP2 checkin should alert on transfer and use CT for return branch
Bug 12021 - SIP2 checkin should alert on transfer and use CT for return branch
Bug 12021 - SIP2 checkin should alert on transfer and use CT for return branch

Description Benjamin Rokseth 2014-04-01 11:56:51 UTC
According to SIP2 specs for checkin:
  AO is where the transaction took place,
  AQ is the "permanent location", and
  CT is the "destination location" (i.e., pickup lib for a hold or target lib for a transfer).

A SIP2 checkin should respect circulation rules and alert (CV04 - send to other branch) with destination location in CT.
Comment 1 Benjamin Rokseth 2014-04-01 12:13:46 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2014-04-23 14:18:25 UTC
Created attachment 27488 [details] [review]
[SIGNED-OFF] Bug 12021 - SIP2 checkin should alert on transfer and use CT for return branch

This small patch corrects the SIP2 checkin response if branch transfer is needed.
It depends on bug 7981 and also removes an obsolete alert check in SIP2.

Test plan:
 - apply bug 7981
 - make sure syspref AutomaticItemReturn is false
 - check 'Return policy' in 'Default checkout, hold and return policy'
 - make a SIP2 checkout and checkin from a branch different than item's home branch.
 - verify that SIP2 response gives alert CV04 (=send to different branch)
 - also verify that response field CT gives correct branch according to 'Return policy'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 Martin Renvoize 2014-08-11 09:44:15 UTC
QA Note: In the code you comment that the return from checkin can't be trusted.  If this is the case, why not fix the code at the source of the issue rather than adding to technical debt?

Having said that, it looks as though your code 'should' work. I've not tested it yet, and am reluctant to until there's some explanation of what your expecting Checkin to return.

Could you please open a bug, or link to an existing bug, with a clear explanation of what this comment means?
Comment 4 Benjamin Rokseth 2014-08-11 10:19:37 UTC
Actually, my patch removes the comment you mentioned and the "reproduced alert logic" as they were unintelligible and probably some residue. There was no bug reference or any other.

My patch only corrects the returned destination_loc in the SIP response to reflect the Checkin response $messages (NeedsTransfer, Wrongbranch, WrongTransfer). This was never working in the original code as it was not appended to the returned objects {item}, but only to self.
Comment 5 Colin Campbell 2014-08-26 13:10:18 UTC
To clarify the original report SIP2 specifies AO as institution_code it is not necessarily tied to a branch (The distributed config example explicitly has an example where it appears not to be). Using it as location is a good idea but we cannot assume that. Also CT is not part the Sip 2.0 standard so we should be aware some Sip devices may not read it (one used to look for the branch in screen message). 3M introduced it as part of one of their extension book capture pieces of hardware. It is listed in Sip 3. 
At present CT is returned if a transit is required for a hold. It would be good to generalize it across other transits
The test plan should confirm that the present behaviour ( CT returned for a hold ) still works.

At present though this is blocked on its dependency on 7981
Comment 6 Colin Campbell 2015-02-24 14:29:13 UTC
The dependency on 7981 appears to be bogus. Applying the patch caused the CT and CV fields to be set appropriately for transits where they were previously missing. I'll try and test fully and sign off if ok
Comment 7 Colin Campbell 2015-03-02 15:43:23 UTC
I think there may be an error in the logic behind this patch. It includes a testplan that starts with switching off automaticitemreturn, but if you have it set on you want these fields set so that self checks place returns in correct reshelving bins
Comment 8 Benjamin Rokseth 2015-03-03 14:09:38 UTC
I might be mistaken, but AutomaticItemReturn triggers automatic transfer and does not alert. Meaning logic to handle return messages/etc. is handled elsewhere/manually and is not notified by SIP. At least this seems to be the case. Whether that is good is another question, but changing that would require another patch.
Comment 9 Benjamin Rokseth 2015-08-19 09:09:17 UTC
Any news on this? bug 7981 is pushed to master, so there is no more dependencies.

I still believe the AutomaticItemReturn syspref should be disabled, as turning it on would disable any alerts. this is from the docs, http://manual.koha-community.org/3.20/en/patscirc.html: 

The various Return Policies have the following effects:

    Item returns home: The item will prompt the librarian to transfer the item to its home library

        If the AutomaticItemReturn preference is set to automatically transfer the items home, then a prompt will not appear

I am positive this is also the case with SIP2: if AutomaticItemReturn is enabled, then SIP2 return will not alert.

This bug corrects an obvious bug which can be seen in the code:
https://github.com/Koha-Community/Koha/blob/master/C4/SIP/ILS/Transaction/Checkin.pm#L82-L119

where CT message should be appended to item
$self->{item}->destination_loc

not 
$self->destination_loc
Comment 10 Benjamin Rokseth 2017-01-10 21:23:41 UTC
reverting the 'in discussion' status as the discussion died.

The patch is still valid as the initial SIP2 checkin code is flawed. Alert type CV04 (transfer with no holds) needs to be accompanied with CT destination branch. This patch fixes this in the cases where permanent location is not the right return branch (AddReturn returns messages WrongBranch, WrongTransfer or NeedsTransfer)

If AutomaticItemReturn is disabled, return branch is given as value of NeedsTransfer, so test plan is still valid

before patch
1) Switch off AutomaticItemReturn
2) Checkin item at a different branch than home or holding branch
3) observe CV04 but no CT field in SIP response

after patch
repeat 1-2
3) observe CT branch according to circulation policies in preferences
4) checkin item at wrong branch, observe again CV04 and CT with correct branch

even if it was originally signed off it might need an extra pair of eyes
Comment 11 Marcel de Rooy 2017-03-17 09:00:37 UTC
Still applies. Saw the CV04 but not the CT (protocol issue). A branch transfer was created for checkin at another location.
Will come back here later.
Comment 12 Marcel de Rooy 2017-03-17 09:01:15 UTC
Created attachment 61200 [details] [review]
Bug 12021 - SIP2 checkin should alert on transfer and use CT for return branch

This small patch corrects the SIP2 checkin response if branch transfer is needed.
It depends on bug 7981 and also removes an obsolete alert check in SIP2.

Test plan:
 - apply bug 7981
 - make sure syspref AutomaticItemReturn is false
 - check 'Return policy' in 'Default checkout, hold and return policy'
 - make a SIP2 checkout and checkin from a branch different than item's home branch.
 - verify that SIP2 response gives alert CV04 (=send to different branch)
 - also verify that response field CT gives correct branch according to 'Return policy'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Marcel de Rooy 2017-03-17 09:02:31 UTC
Trivial rebase iirc
Comment 14 Marcel de Rooy 2017-03-17 16:24:00 UTC
Please look at 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->destination_loc($iteminformation->{homebranch});
        $self->alert_type('04');            # send to other branch
    }

You changed NeedsTransfer, but you did not touch WasTransfered here.
That is the reason why I saw the CV04 without CT.
Please change and set back to Needs Signoff. A fresh signoff would not be bad.
Comment 15 Benjamin Rokseth 2017-03-24 14:02:57 UTC
(In reply to Marcel de Rooy from comment #14)
> Please look at 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->destination_loc($iteminformation->{homebranch});
>         $self->alert_type('04');            # send to other branch
>     }
> 
> You changed NeedsTransfer, but you did not touch WasTransfered here.
> That is the reason why I saw the CV04 without CT.
> Please change and set back to Needs Signoff. A fresh signoff would not be
> bad.

this must be a lost commit, will make a new patch and set back to Needs Signoff
Comment 16 Benjamin Rokseth 2017-03-27 08:40:54 UTC
Created attachment 61640 [details] [review]
Bug 12021 - SIP2 checkin should alert on transfer and use CT for return branch

This small patch corrects the SIP2 checkin response if branch transfer is needed.
It depends on bug 7981 and also removes an obsolete alert check in SIP2.

Test plan:
 - apply bug 7981
 - make sure syspref AutomaticItemReturn is false
 - check 'Return policy' in 'Default checkout, hold and return policy'
 - make a SIP2 checkout and checkin from a branch different than item's home branch.
 - verify that SIP2 response gives alert CV04 (=send to different branch)
 - also verify that response field CT gives correct branch according to 'Return policy'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Marc Véron 2017-04-11 18:52:41 UTC
Created attachment 62060 [details] [review]
Bug 12021 - SIP2 checkin should alert on transfer and use CT for return branch

This small patch corrects the SIP2 checkin response if branch transfer is needed.
It depends on bug 7981 and also removes an obsolete alert check in SIP2.

Test plan:
 - apply bug 7981
 - make sure syspref AutomaticItemReturn is false
 - check 'Return policy' in 'Default checkout, hold and return policy'
 - make a SIP2 checkout and checkin from a branch different than item's home branch.
 - verify that SIP2 response gives alert CV04 (=send to different branch)
 - also verify that response field CT gives correct branch according to 'Return policy'

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

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 18 Marcel de Rooy 2017-04-20 12:23:42 UTC
Created attachment 62466 [details] [review]
Bug 12021 - SIP2 checkin should alert on transfer and use CT for return branch

This small patch corrects the SIP2 checkin response if branch transfer is needed.
It depends on bug 7981 and also removes an obsolete alert check in SIP2.

Test plan:
 - apply bug 7981
 - make sure syspref AutomaticItemReturn is false
 - check 'Return policy' in 'Default checkout, hold and return policy'
 - make a SIP2 checkout and checkin from a branch different than item's home branch.
 - verify that SIP2 response gives alert CV04 (=send to different branch)
 - also verify that response field CT gives correct branch according to 'Return policy'

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

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Kyle M Hall 2017-04-21 14:29:30 UTC
Pushed to master for 17.05, thanks Ben!
Comment 20 Katrin Fischer 2017-04-21 22:40:25 UTC
This patch has been pushed to 16.11.x and will be in 16.11.07.
Comment 21 Mason James 2017-05-03 04:39:08 UTC
Pushed to 16.05.x, for 16.05.12 release
Comment 22 Julian Maurice 2017-05-10 09:52:27 UTC
Pushed to 3.22.x for 3.22.21