https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #1 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> ---

From that bug:

If I check in an item where the issue id is already in old issues (I inserted this id for testing purposes), AddReturn correctly gives me back me a zero status and DataCorrupted message. However, SIP somehow ignores it. It returns a 10 with ok=1 while it should return a 10 with ok=0. The problem is in ILS.pm line 219 $circ->ok( ( $checked_in_ok && $item ) || ( $item && $item->{patron} ) ); In this specific case $circ->ok was already set to false in do_checkin just before that line. But now this line sets ok back to true. I guess this is a more fundamental problem. Because if AddReturn did set the ok flag to false for another reason, the same could happen. So I would propose to solve it on another report. I am not fully sure if we can just test if $circ->ok is still true before running this line 219, and leave it false when it is false. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 65304 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65304&action=edit Bug 18996: What about adding $circ->ok to the second clause ? Proposal needs feedback from preferably Colin. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Note: We also need the fix here from the last follow-up on 17680 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m, | |jonathan.druart@bugs.koha-c | |ommunity.org, | |katrin.fischer@bsz-bw.de, | |mtj@kohaaloha.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #6 from Colin Campbell <colin.campbell@ptfs-europe.com> --- The ok/not ok flag was never checked or trusted in the original sip code, its not alone, other calls to AddReturn seem to ignore the return. Instead it explicitly set ok as above. I'm wondering if adding the extra check would go against the spirit of bug 13411 which was driven my users' desire that checkins proceed as ok even although there are issues that would be thrown up at a staff terminal. The most common case people want to ignore is 'Book Not Issued' which from the borrowers perspective is not that distinguishable from 'Data Corruption' -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #8 from Colin Campbell <colin.campbell@ptfs-europe.com> --- (In reply to Katrin Fischer from comment #7)

If I understand correctly this bug would mean that the self check claims the book returned, but it's still on the patron's account.

We have some libraries that return all books at the circulation desk a second time to check for holds etc. - so those would be caught. But others might trust that alle the items are correctly returned and reshelve - so that could cause quite a problem if the item was later to be found on the shelves (did the patron "snuck" it in to avoid fines or did Koha mess up...?)

I think ideally we should alert of a problem in a way that makes the self check indicate to take the item to the circulation desk.

Yes the logic behind 13411 is that a checkin should be ok if there is no issue transaction. I think if there is an issue transaction, and we cant checkin then we must refer to the issue desk. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> ---

Yes the logic behind 13411 is that a checkin should be ok if there is no issue transaction. I think if there is an issue transaction, and we cant checkin then we must refer to the issue desk.

We are refusing check-in atm, at the most helpful message is shown when you return on the circulation desk. So I think that's what we want, but I have no idea how it would translate into a SIP2 response. Can you help? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 65330 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65330&action=edit Bug 18996: Handle data corruption from old_issues at SIP checkin As per bug 18966 AddReturn returns false with a DataCorrupted message when the issue_id at hand already occurs in the old_issues table. This should be handled when returning an item via SIP too. SIP should not pretend that the checkin was successful, since this item needs special care. The following six different situations are handled: [1] An attempt to checkin an invalid barcode is handled as before. The ok flag is false; the screen message shows 'Invalid Item'. [2] We receive a DataCorrupted message: the alert type is set to the unused 98 code to indicate this new error condition. The ok flag is false; the screen message shows 'Checkin failed: data problem'. [3] The item checked in was not checked out AND the option checked_in_ok is active. The ok flag is set to true; no screen message. [4] The item checked in was not checked out AND the option checked_in_ok is not active. The ok flag is not changed (normally false); the screen message shows 'Item not checked out'. [5] (The regular checkin) The item was checked out and AddReturn returned true. The ok flag is true; no screen message. [6] ("Otherwise") The item was checked out, but AddReturn returned false. The ok flag is false; the screen message shows 'Checkin failed'. Note: Currently this case only refers to the Wrongbranch and withdrawn messages from AddReturn (where doreturn==0). Note: Situation 1 and 5 are unchanged. Behavior is slightly changed for situation 3 and 4; the option is only used when the item was not checked out. Situation 2 and situation 6 are changed. After bug 18966 SIP returned OK in case of data corruption (while checkin failed). In the remaining cases under [6] SIP also returned OK while checkin failed. Test plan: [1] Test all six cases listed above. In order to simulate data corruption insert the issue_id at hand in old_issues before you check in. In cases 3 and 4 you need to toggle the option in your SIPconfig.xml and restart the SIP server. Case 6 can be tested by checking out at branch A, adjusting the value of pref AllowReturnToBranch and checking in at branch B. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65304|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- The proposed patch now does not pretend that things are OK when the item is checked out and the checkin did fail for data corruption. It does the same for two other failing cases in AddReturn: Wrongbranch and withdrawn. Will add a test in Message.t on this or another report. But this patch should be tested now in SIP. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65330|0 |1 is obsolete| | --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 65336 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65336&action=edit Bug 18996: Handle data corruption from old_issues at SIP checkin As per bug 18966 AddReturn returns false with a DataCorrupted message when the issue_id at hand already occurs in the old_issues table. This should be handled when returning an item via SIP too. SIP should not pretend that the checkin was successful, since this item needs special care. The following six different situations are handled: [1] An attempt to checkin an invalid barcode is handled as before. The ok flag is false; the screen message shows 'Invalid Item'. [2] We receive a DataCorrupted message: the alert type is set to the unused 98 code to indicate this new error condition. The ok flag is false; the screen message shows 'Checkin failed: data problem'. [3] The item checked in was not checked out AND the option checked_in_ok is active. The ok flag is set to true; no screen message. [4] The item checked in was not checked out AND the option checked_in_ok is not active. The ok flag is not changed (normally false); the screen message shows 'Item not checked out'. [5] (The regular checkin) The item was checked out and AddReturn returned true. The ok flag is true; no screen message. [6] ("Otherwise") The item was checked out, but AddReturn returned false. The ok flag is false; the screen message shows 'Checkin failed'. Note: Currently this case only refers to the Wrongbranch and withdrawn messages from AddReturn (where doreturn==0). Note: Situation 1 and 5 are unchanged. Behavior is slightly changed for situation 3 and 4; the option is only used when the item was not checked out. Situation 2 and situation 6 are changed. After bug 18966 SIP returned OK in case of data corruption (while checkin failed). In the remaining cases under [6] SIP also returned OK while checkin failed. Test plan: [1] Test all six cases listed above. In order to simulate data corruption insert the issue_id at hand in old_issues before you check in. In cases 3 and 4 you need to toggle the option in your SIPconfig.xml and restart the SIP server. Case 6 can be tested by checking out at branch A, adjusting the value of pref AllowReturnToBranch and checking in at branch B. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 65393 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65393&action=edit Bug 18996: Resolve an uninitialized warning from ILS/Item.pm

From the syslog call on line 107/108. The field $item->{title} has not been initialized. When catching the warns in a test script with Test::Warn, I got: Use of uninitialized value $args[1] in sprintf at /usr/lib/x86_64-linux-gnu/perl/5.20/Sys/Syslog.pm line 423.

This simply resolves the warning. We might have a closer look to this title field on another report though. Test plan: Run Message.t from the next patch without this fix. It will fail since the additional warning makes warnings_like return false. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Raising Severity: SIP is fooling us. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 65418 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65418&action=edit Bug 18996: [QA Follow-up] Remove global variables from Message.t As per request of Colin in comment18, this patch makes the use of global variables in Message.t no longer needed. The three subtests are now completely independent and could well be moved to separate test scripts. Note: Strictly speaking, the use of global (package) variables could potentially introduce new bugs (e.g. if the value is modified outside the script). This seems not to be the case here, but we are safe now. Test plan: Run the test again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65336|0 |1 is obsolete| | --- Comment #21 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 65961 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65961&action=edit Handle data corruption from old issues at SIP checkin -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65394|0 |1 is obsolete| | --- Comment #23 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 65963 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65963&action=edit Add checkin subtest in SIP/Message.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #25 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Signed off patches - tested that normal checkins processed ok without side effects and that data corruption was reported as per intention if such errors were encountered -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Colin Campbell from comment #25)

Signed off patches - tested that normal checkins processed ok without side effects and that data corruption was reported as per intention if such errors were encountered

Thanks, Colin! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #29 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 66181 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66181&action=edit Bug 18996: Resolve an uninitialized warning from ILS/Item.pm

From the syslog call on line 107/108. The field $item->{title} has not been initialized. When catching the warns in a test script with Test::Warn, I got: Use of uninitialized value $args[1] in sprintf at /usr/lib/x86_64-linux-gnu/perl/5.20/Sys/Syslog.pm line 423.

This simply resolves the warning. We might have a closer look to this title field on another report though. Test plan: Run Message.t from the next patch without this fix. It will fail since the additional warning makes warnings_like return false. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #31 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 66183 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66183&action=edit Bug 18996: [QA Follow-up] Remove global variables from Message.t As per request of Colin in comment18, this patch makes the use of global variables in Message.t no longer needed. The three subtests are now completely independent and could well be moved to separate test scripts. Note: Strictly speaking, the use of global (package) variables could potentially introduce new bugs (e.g. if the value is modified outside the script). This seems not to be the case here, but we are safe now. Test plan: Run the test again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #33 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 17.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable --- Comment #34 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Pushed to 17.05.x, will be in 17.05.04 Bug 18996 [QA Followup] - Fix some minor whitespace issues : This patch did not apply but it was trivial to write it manually since its only spaces/tabs changes. This bud depends on Bug 17680 which is not in 17.05.x but I had no conflicts and Koha::Checkouts exists in 17.05.x. UT t/db_dependent/SIP/Message.t runs well. I encourage you to validate and contact me if there is a problem. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #36 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Sadly for 16.11 things are not going as smoothly. I applied all 5 patches cherry-picked from Fridolin. The last patch had some conflicts, but I resolved them and the git diff -w doesn't show a difference now. Tests fail: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ perl t/db_dependent/SIP/Message.t 1..3 Can't locate Koha/Old/Checkouts.pm in @INC (you may need to install the Koha::Old::Checkouts module) (@INC contains: /home/vagrant/kohaclone /home/vagrant/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.20.2 /usr/local/share/perl/5.20.2 /usr/lib/x86_64-linux-gnu/perl5/5.20 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.20 /usr/share/perl/5.20 /usr/local/lib/site_perl .) at t/db_dependent/SIP/Message.t line 37. BEGIN failed--compilation aborted at t/db_dependent/SIP/Message.t line 37. 16.11.x still has Koha/OldIssues.pm -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #38 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 67055 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=67055&action=edit Bug 18996: [16.11.x] Handle data corruption from old_issues at SIP checkin As per bug 18966 AddReturn returns false with a DataCorrupted message when the issue_id at hand already occurs in the old_issues table. This should be handled when returning an item via SIP too. SIP should not pretend that the checkin was successful, since this item needs special care. The following six different situations are handled: [1] An attempt to checkin an invalid barcode is handled as before. The ok flag is false; the screen message shows 'Invalid Item'. [2] We receive a DataCorrupted message: the alert type is set to the unused 98 code to indicate this new error condition. The ok flag is false; the screen message shows 'Checkin failed: data problem'. [3] The item checked in was not checked out AND the option checked_in_ok is active. The ok flag is set to true; no screen message. [4] The item checked in was not checked out AND the option checked_in_ok is not active. The ok flag is not changed (normally false); the screen message shows 'Item not checked out'. [5] (The regular checkin) The item was checked out and AddReturn returned true. The ok flag is true; no screen message. [6] ("Otherwise") The item was checked out, but AddReturn returned false. The ok flag is false; the screen message shows 'Checkin failed'. Note: Currently this case only refers to the Wrongbranch and withdrawn messages from AddReturn (where doreturn==0). Note: Situation 1 and 5 are unchanged. Behavior is slightly changed for situation 3 and 4; the option is only used when the item was not checked out. Situation 2 and situation 6 are changed. After bug 18966 SIP returned OK in case of data corruption (while checkin failed). In the remaining cases under [6] SIP also returned OK while checkin failed. Test plan: [1] Test all six cases listed above. In order to simulate data corruption insert the issue_id at hand in old_issues before you check in. In cases 3 and 4 you need to toggle the option in your SIPconfig.xml and restart the SIP server. Case 6 can be tested by checking out at branch A, adjusting the value of pref AllowReturnToBranch and checking in at branch B. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #40 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 67057 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=67057&action=edit Bug 18996: [16.11.x] Add checkin subtest in SIP/Message.t Variable $branch was not used. Promoted some global vars to our. Shared the branchcode between all three subtests now. The third subtest contains all six cases mentioned in the first patch. Test plan: Run t/db_dependent/SIP/Message.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #42 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 67059 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=67059&action=edit Bug 18996: [16.11.x] use Koha::OldIssues instead of Koha::Old::Checkouts -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #44 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I am not good with SIP2 testing, can someone help? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #46 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 67849 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=67849&action=edit Bug 18996: (followup) Fix tests count [16.11.x] This patch fixes a wrong tests count introduced on merging. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #48 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Follow-up has been pushed to 16.11.x and will be in 16.11.13. Thx Tomas! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #49 from Mason James <mtj@kohaaloha.com> --- Pushed to 16.05.x, for 16.05.17 release -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 --- Comment #51 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Anna from comment #50)

Can it be applied to 17.05.06? Thanks!

It's part of 17.05.x since 17.05.04. -- You are receiving this mail because: You are watching all bug changes.