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>

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?

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.

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

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

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

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.

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

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

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.

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>

Trivial rebase iirc

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.

(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

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>

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>

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>

Pushed to master for 17.05, thanks Ben!

This patch has been pushed to 16.11.x and will be in 16.11.07.

Pushed to 16.05.x, for 16.05.12 release

Pushed to 3.22.x for 3.22.21