Bug 18996 - SIP sets ok flag to true for refused checkin for data corruption
Summary: SIP sets ok flag to true for refused checkin for data corruption
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 18966 17680
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-28 09:27 UTC by Marcel de Rooy
Modified: 2017-10-20 08:26 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18996: Handle DataCorrupted message in SIP checkin (985 bytes, patch)
2017-07-28 09:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18996: What about adding $circ->ok to the second clause ? (1000 bytes, patch)
2017-07-28 09:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18996: Handle data corruption from old_issues at SIP checkin (5.23 KB, patch)
2017-07-31 07:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18996: Handle data corruption from old_issues at SIP checkin (5.26 KB, patch)
2017-07-31 11:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18996: Resolve an uninitialized warning from ILS/Item.pm (1.26 KB, patch)
2017-08-01 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18996: Add checkin subtest in SIP/Message.t (9.31 KB, patch)
2017-08-01 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18996: [QA Follow-up] Remove global variables from Message.t (7.44 KB, patch)
2017-08-02 11:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Handle data corruption from old issues at SIP checkin (5.30 KB, patch)
2017-08-14 16:44 UTC, Colin Campbell
Details | Diff | Splinter Review
Resolve an uninitialized warning from ILS/Item.pm (1.30 KB, patch)
2017-08-14 16:45 UTC, Colin Campbell
Details | Diff | Splinter Review
Add checkin subtest in SIP/Message.t (9.35 KB, patch)
2017-08-14 16:47 UTC, Colin Campbell
Details | Diff | Splinter Review
QA Remove global variables from patch (7.48 KB, patch)
2017-08-14 16:48 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 18996: Handle data corruption from old_issues at SIP checkin (5.34 KB, patch)
2017-08-18 15:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18996: Resolve an uninitialized warning from ILS/Item.pm (1.34 KB, patch)
2017-08-18 15:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18996: Add checkin subtest in SIP/Message.t (9.39 KB, patch)
2017-08-18 15:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18996: [QA Follow-up] Remove global variables from Message.t (7.52 KB, patch)
2017-08-18 15:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18996 [QA Followup] - Fix some minor whitespace issues (2.31 KB, patch)
2017-08-18 15:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18996: [16.11.x] Handle data corruption from old_issues at SIP checkin (5.36 KB, patch)
2017-09-11 19:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18996: [16.11.x] Resolve an uninitialized warning from ILS/Item.pm (1.36 KB, patch)
2017-09-11 19:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18996: [16.11.x] Add checkin subtest in SIP/Message.t (9.43 KB, patch)
2017-09-11 19:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18996: [16.11.x] [QA Follow-up] Remove global variables from Message.t (7.54 KB, patch)
2017-09-11 19:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18996: [16.11.x] use Koha::OldIssues instead of Koha::Old::Checkouts (1.65 KB, patch)
2017-09-11 19:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18996: (followup) Fix tests count [16.11.x] (977 bytes, patch)
2017-10-09 19:22 UTC, Tomás Cohen Arazi
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 2017-07-28 09:27:18 UTC
See bug 18966
Comment 1 Marcel de Rooy 2017-07-28 09:30:32 UTC
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.
Comment 2 Marcel de Rooy 2017-07-28 09:37:13 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-07-28 09:39:45 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2017-07-28 09:42:05 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2017-07-28 09:44:33 UTC Comment hidden (obsolete)
Comment 6 Colin Campbell 2017-07-28 10:40:14 UTC
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'
Comment 7 Katrin Fischer 2017-07-28 10:54:41 UTC
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.
Comment 8 Colin Campbell 2017-07-28 11:42:00 UTC
(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.
Comment 9 Katrin Fischer 2017-07-28 14:46:04 UTC
> 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?
Comment 10 Marcel de Rooy 2017-07-28 15:17:05 UTC
(In reply to Katrin Fischer from comment #9)
> > 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?

Will submit another proposal here.
Comment 11 Marcel de Rooy 2017-07-31 07:30:39 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2017-07-31 07:36:31 UTC
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.
Comment 13 Marcel de Rooy 2017-07-31 11:53:06 UTC
Created attachment 65336 [details] [review]
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>
Comment 14 Marcel de Rooy 2017-07-31 11:55:13 UTC
Tiny change: remove uninitialized warn on alert_type and reworded syslog message for case [6].
Comment 15 Marcel de Rooy 2017-08-01 12:35:24 UTC
Created attachment 65393 [details] [review]
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>
Comment 16 Marcel de Rooy 2017-08-01 12:35:29 UTC
Created attachment 65394 [details] [review]
Bug 18996: 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>
Comment 17 Marcel de Rooy 2017-08-01 14:18:15 UTC
Raising Severity: SIP is fooling us.
Comment 18 Colin Campbell 2017-08-01 16:02:27 UTC
(In reply to Marcel de Rooy from comment #15)
> Created attachment 65393 [details] [review] [review]
> 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>

(In reply to Marcel de Rooy from comment #16)
> Created attachment 65394 [details] [review] [review]
> Bug 18996: 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>

unnecessarily making lexical variables package variables is potentially introducing bugs. If you want to do it, it should not really be slipped in as part of another patch, but have its own rationale
Comment 19 Marcel de Rooy 2017-08-02 11:26:34 UTC
Created attachment 65418 [details] [review]
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>
Comment 20 Marcel de Rooy 2017-08-02 11:27:13 UTC
(In reply to Colin Campbell from comment #18)
> unnecessarily making lexical variables package variables is potentially
> introducing bugs. If you want to do it, it should not really be slipped in
> as part of another patch, but have its own rationale

Added a follow-up to address your concerns. Thx for testing.
Comment 21 Colin Campbell 2017-08-14 16:44:39 UTC
Created attachment 65961 [details] [review]
Handle data corruption from old issues at SIP checkin
Comment 22 Colin Campbell 2017-08-14 16:45:53 UTC
Created attachment 65962 [details] [review]
Resolve an uninitialized warning from ILS/Item.pm
Comment 23 Colin Campbell 2017-08-14 16:47:07 UTC
Created attachment 65963 [details] [review]
Add checkin subtest in SIP/Message.t
Comment 24 Colin Campbell 2017-08-14 16:48:27 UTC
Created attachment 65964 [details] [review]
QA Remove global variables from patch
Comment 25 Colin Campbell 2017-08-14 16:50:54 UTC
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
Comment 26 Marcel de Rooy 2017-08-14 17:25:47 UTC
(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!
Comment 27 Katrin Fischer 2017-08-15 15:50:08 UTC
Thx Colin!
Comment 28 Kyle M Hall 2017-08-18 15:06:58 UTC
Created attachment 66180 [details] [review]
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>
Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall 2017-08-18 15:07:06 UTC
Created attachment 66181 [details] [review]
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>
Comment 30 Kyle M Hall 2017-08-18 15:07:10 UTC
Created attachment 66182 [details] [review]
Bug 18996: 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>
Comment 31 Kyle M Hall 2017-08-18 15:07:13 UTC
Created attachment 66183 [details] [review]
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>
Comment 32 Kyle M Hall 2017-08-18 15:07:16 UTC
Created attachment 66184 [details] [review]
Bug 18996 [QA Followup] - Fix some minor whitespace issues

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Jonathan Druart 2017-08-20 11:32:05 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 34 Fridolin SOMERS 2017-09-06 12:23:02 UTC
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.
Comment 35 Marcel de Rooy 2017-09-06 13:03:00 UTC
(In reply to Fridolin SOMERS from comment #34)
> 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.

Thats fine. You dont need 17680 in 17.05
Comment 36 Katrin Fischer 2017-09-10 08:27:30 UTC
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
Comment 37 Katrin Fischer 2017-09-10 08:32:11 UTC
I've tried to fix the tests replacing the newer by the old modules, but getting stuck here:

    # Data corrupted: add same issue_id to old_issues
    Koha::OldIssues->new({ issue_id => $issue->issue_id })->store;

The method store is not covered by tests at t/db_dependent/SIP/Message.t line 312.
    not ok 15 - DBIx error on duplicate issue_id
    #   Failed test 'DBIx error on duplicate issue_id'
    #   at t/db_dependent/SIP/Message.t line 316.
    # didn't find a warning
    # expected to find warning: (?^u:Duplicate entry)
    # expected to find warning: (?^u:data corrupted)
    not ok 16 - OK flag is false when we encounter data corruption in old_issues
    #   Failed test 'OK flag is false when we encounter data corruption in old_issues'
    #   at t/db_dependent/SIP/Message.t line 318.
    #          got: '1'
    #     expected: '0'
    not ok 17 - Alert flag is set
    #   Failed test 'Alert flag is set'
    #   at t/db_dependent/SIP/Message.t line 319.
    #          got: 'N'
    #     expected: 'Y'
    not ok 18 - Code AF not found in '|AOz7vpbEBO2|ABNZd|AQz7vpbEBO2|AJNVjlgNa_|AAgHWDOyBTR|CRmibXFk|CSd0dkNUeiPLgDfs5ZS8tKmTiko63dH74rAf5ikn_Jg2QXE6TEmks381qyPoSUvy5ZORcYPHtyT_BiUpH7vTR2KdHjc_TNDNaFvZtMsvaP_qDOQLqP8ZWfJMGFKWO1dd6VaXt1kCqgU1zqOmoPEcox92XXWlL54V0df04DoOGpaGq2aChxvamLvEPhEgLDsbLhw34EI28uO9DfKFYDtPpVteffxIy7qHyFZHgkPNthjLMNAeDYSZZP9Y|'?
    #   Failed test 'Code AF not found in '|AOz7vpbEBO2|ABNZd|AQz7vpbEBO2|AJNVjlgNa_|AAgHWDOyBTR|CRmibXFk|CSd0dkNUeiPLgDfs5ZS8tKmTiko63dH74rAf5ikn_Jg2QXE6TEmks381qyPoSUvy5ZORcYPHtyT_BiUpH7vTR2KdHjc_TNDNaFvZtMsvaP_qDOQLqP8ZWfJMGFKWO1dd6VaXt1kCqgU1zqOmoPEcox92XXWlL54V0df04DoOGpaGq2aChxvamLvEPhEgLDsbLhw34EI28uO9DfKFYDtPpVteffxIy7qHyFZHgkPNthjLMNAeDYSZZP9Y|'?'
    #   at t/db_dependent/SIP/Message.t line 364.
    #          got: '0'
    #     expected: '1'
    not ok 19 - OK flag is true when we checkin after removing the duplicate
    #   Failed test 'OK flag is true when we checkin after removing the duplicate'
    #   at t/db_dependent/SIP/Message.t line 327.
    #          got: '0'
    #     expected: '1'
    not ok 20 - Alert flag is not set
    #   Failed test 'Alert flag is not set'
    #   at t/db_dependent/SIP/Message.t line 328.
    #          got: 'Y'
    #     expected: 'N'
    ok 21 - Issue record is gone now
    # Looks like you failed 6 tests of 21.
Comment 38 Jonathan Druart 2017-09-11 19:10:55 UTC
Created attachment 67055 [details] [review]
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>
Comment 39 Jonathan Druart 2017-09-11 19:11:00 UTC
Created attachment 67056 [details] [review]
Bug 18996: [16.11.x] 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>
Comment 40 Jonathan Druart 2017-09-11 19:11:06 UTC
Created attachment 67057 [details] [review]
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>
Comment 41 Jonathan Druart 2017-09-11 19:11:11 UTC
Created attachment 67058 [details] [review]
Bug 18996: [16.11.x] [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>
Comment 42 Jonathan Druart 2017-09-11 19:11:16 UTC
Created attachment 67059 [details] [review]
Bug 18996: [16.11.x] use Koha::OldIssues instead of Koha::Old::Checkouts
Comment 43 Jonathan Druart 2017-09-11 19:11:43 UTC
I have rebased and adapt the patches for 16.11.x, but did not test them!
Comment 44 Katrin Fischer 2017-09-11 23:00:18 UTC
I am not good with SIP2 testing, can someone help?
Comment 45 Katrin Fischer 2017-09-15 22:50:42 UTC
Hi Jonathan, thx, tests all work now. Pushing.

These patches have been pushed to 16.11.x and will be in 16.11.12.
Comment 46 Tomás Cohen Arazi 2017-10-09 19:22:07 UTC
Created attachment 67849 [details] [review]
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>
Comment 47 Tomás Cohen Arazi 2017-10-09 19:22:57 UTC
Ping Katrina, last followup is needed.
Comment 48 Katrin Fischer 2017-10-14 12:47:12 UTC
Follow-up has been pushed to 16.11.x and will be in 16.11.13.

Thx Tomas!