Bug 18242 - Move of checkouts to old_issues is not handled correctly
Summary: Move of checkouts to old_issues is not handled correctly
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 18185 (view as bug list)
Depends on:
Blocks: 18329 18651 18806
  Show dependency treegraph
 
Reported: 2017-03-09 18:52 UTC by Jonathan Druart
Modified: 2017-12-07 22:24 UTC (History)
8 users (show)

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


Attachments
Bug 18242: [SOLUTION 1]DB changes - remove PK on old_issues.issue_id (2.71 KB, patch)
2017-03-09 20:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18242: [SOLUTION 1]Schema changes (1.56 KB, patch)
2017-03-09 20:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18242: [SOLUTION 1]Remove primary key on old_issues.issue_id (2.37 KB, patch)
2017-03-09 20:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18242: [SOLUTION 2]Add tests (1.77 KB, patch)
2017-03-09 20:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18242: [SOLUTION 2]Handle correctly move to old_issues (4.86 KB, patch)
2017-03-09 20:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18242: [SOLUTION 2]Add tests (1.83 KB, patch)
2017-03-14 00:36 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 18242: [SOLUTION 2]Handle correctly move to old_issues (4.92 KB, patch)
2017-03-14 00:36 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 18242: [SOLUTION 2]Add tests (1.88 KB, patch)
2017-03-20 09:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18242: [SOLUTION 2]Handle correctly move to old_issues (4.97 KB, patch)
2017-03-20 10:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18242: 16.11.x adaptation - Old::Checkouts vs OldIssues (1.06 KB, patch)
2017-04-03 13:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18242: 16.05 version - [SOLUTION 2]Add tests (1.94 KB, patch)
2017-05-05 13:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18242: 16.05 version - [SOLUTION 2]Handle correctly move to old_issues (4.88 KB, patch)
2017-05-05 13:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18242: 16.05 version - 16.11.x adaptation - Old::Checkouts vs OldIssues (1.08 KB, patch)
2017-05-05 13:28 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-03-09 18:52:20 UTC
The table old_issues has a primary key defined on the issue_id column.
This issue_id comes from the issues table when an item is checked in.

In some case the value of issue_id already exists in the table

Basically this happens when an item is returned and mysqld is restarted: The auto increment value for issues.issue_id will be reset to MAX(issue_id)+1 (which is the value of the last entry of old_issues). See also the description of bug 18003 for more informations.
Comment 1 Jonathan Druart 2017-03-09 20:03:26 UTC
Created attachment 60969 [details] [review]
Bug 18242: [SOLUTION 1]DB changes - remove PK on old_issues.issue_id
Comment 2 Jonathan Druart 2017-03-09 20:03:31 UTC
Created attachment 60970 [details] [review]
Bug 18242: [SOLUTION 1]Schema changes
Comment 3 Jonathan Druart 2017-03-09 20:03:35 UTC
Created attachment 60971 [details] [review]
Bug 18242: [SOLUTION 1]Remove primary key on old_issues.issue_id

The table old_issues has a primary key defined on the issue_id column.
This issue_id comes from the issues table when an item is checked in.

In some case the value of issue_id already exists in the table

Basically this happens when an item is returned and mysqld is restarted:
The auto increment value for issues.issue_id will be reset to MAX(issue_id)+1
(which is the value of the last entry of old_issues). See also the description
of bug 18003 for more informations.

The easier solution seems to add a new column id as a primary key. The changes
to the codebase are very small.

However this will raise few problems:
- We can no longer assume that accountlines.issue_id==old_issues.id (I
guess it is problematic).
- Code in Koha::REST::V1::_object_ownership_by_checkout_id is wrong
assuming that old_issues.issuing_id is unique
Comment 4 Jonathan Druart 2017-03-09 20:04:16 UTC
Created attachment 60972 [details] [review]
Bug 18242: [SOLUTION 2]Add tests
Comment 5 Jonathan Druart 2017-03-09 20:04:21 UTC
Created attachment 60973 [details] [review]
Bug 18242: [SOLUTION 2]Handle correctly move to old_issues

The table old_issues has a primary key defined on the issue_id column.
This issue_id comes from the issues table when an item is checked in.

In some case the value of issue_id already exists in the table

Basically this happens when an item is returned and mysqld is restarted:
The auto increment value for issues.issue_id will be reset to
MAX(issue_id)+1 (which is the value of the last entry of old_issues).
See also the description of bug 18003 for more informations.

In this solution the change is done at code level instead of DB
structure: If old_issues.issue_id already exists before moving from
the issues table, the issue_id is updated (not on cascade for
accountlines.issue_id, should it?) before the move.
Comment 6 Chris Cormack 2017-03-14 00:36:16 UTC
Created attachment 61056 [details] [review]
Bug 18242: [SOLUTION 2]Add tests

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 7 Chris Cormack 2017-03-14 00:36:45 UTC
Created attachment 61057 [details] [review]
Bug 18242: [SOLUTION 2]Handle correctly move to old_issues

The table old_issues has a primary key defined on the issue_id column.
This issue_id comes from the issues table when an item is checked in.

In some case the value of issue_id already exists in the table

Basically this happens when an item is returned and mysqld is restarted:
The auto increment value for issues.issue_id will be reset to
MAX(issue_id)+1 (which is the value of the last entry of old_issues).
See also the description of bug 18003 for more informations.

In this solution the change is done at code level instead of DB
structure: If old_issues.issue_id already exists before moving from
the issues table, the issue_id is updated (not on cascade for
accountlines.issue_id, should it?) before the move.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 8 Jonathan Druart 2017-03-15 19:48:47 UTC
*** Bug 18185 has been marked as a duplicate of this bug. ***
Comment 9 Nick Clemens 2017-03-20 09:59:55 UTC
Created attachment 61254 [details] [review]
Bug 18242: [SOLUTION 2]Add tests

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens 2017-03-20 10:00:01 UTC
Created attachment 61255 [details] [review]
Bug 18242: [SOLUTION 2]Handle correctly move to old_issues

The table old_issues has a primary key defined on the issue_id column.
This issue_id comes from the issues table when an item is checked in.

In some case the value of issue_id already exists in the table

Basically this happens when an item is returned and mysqld is restarted:
The auto increment value for issues.issue_id will be reset to
MAX(issue_id)+1 (which is the value of the last entry of old_issues).
See also the description of bug 18003 for more informations.

In this solution the change is done at code level instead of DB
structure: If old_issues.issue_id already exists before moving from
the issues table, the issue_id is updated (not on cascade for
accountlines.issue_id, should it?) before the move.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens 2017-03-20 10:01:24 UTC
This works in all testing I tried, it does seem that if things get out of sync the ids will be updated for most returns and never get back into sync, however, I  can't think of any reason issue_id needs to be static so seems fine
Comment 12 Brendan Gallagher 2017-03-23 08:41:50 UTC
Pushed to Master - Should be in the 17.05 release.  Thanks!
Comment 13 Katrin Fischer 2017-03-26 20:25:48 UTC
Patches don't appear to work for 16.11.x:

#   Failed test 'No tests run for subtest "Handle ids duplication"'
#   at t/db_dependent/Circulation/Returns.t line 289.
Can't locate object method "find" via package "Koha::Old::Checkouts" (perhaps you forgot to load "Koha::Old::Checkouts"?) at t/db_dependent/Circulation/Returns.t line 287.

If this is needed, please provide patches for 16.11.x!
Comment 14 Jonathan Druart 2017-03-27 14:01:40 UTC
For 16.11.x you just need to replace Koha::Old::Checkout with Koha::OldIssue (bug 17796).
Comment 15 Katrin Fischer 2017-04-02 16:41:50 UTC
Jonathan, could you please provide patches for 16.11.x?
Comment 16 Jonathan Druart 2017-04-03 13:07:53 UTC
Created attachment 61794 [details] [review]
Bug 18242: 16.11.x adaptation - Old::Checkouts vs OldIssues
Comment 17 Fred 2017-04-06 03:34:56 UTC
Please I have problems in the loans, I deleted the circulation history, please in what files should I make the changes ?. What change should I make in the database ?. I have version 16.11.06
Comment 18 Katrin Fischer 2017-04-10 21:41:06 UTC
These patches have been pushed to 16.11.x and will be in 16.11.07.

Thx for the follow-up, Jonahtan!
Comment 19 Mason James 2017-05-05 09:39:40 UTC
Hi, the atches don't appear to work for 16.05.x:
If this is needed, please provide patches for 16.05.x

-----------
mason@xen1:~/g/k/16.05.x$ git bz apply 18242
Bug 18242 - Remove primary key on old_issues.issue_id

61254 - Bug 18242: [SOLUTION 2]Add tests
61255 - Bug 18242: [SOLUTION 2]Handle correctly move to old_issues
61794 - Bug 18242: 16.11.x adaptation - Old::Checkouts vs OldIssues

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 18242: [SOLUTION 2]Add tests
Using index info to reconstruct a base tree...
M       t/db_dependent/Circulation/Returns.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Circulation/Returns.t
CONFLICT (content): Merge conflict in t/db_dependent/Circulation/Returns.t
Failed to merge in the changes.
Patch failed at 0001 Bug 18242: [SOLUTION 2]Add tests
The copy of the patch that failed is found in:
   /home/mason/g/k/16.05.x/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-18242-SOLUTION-2Add-tests-uyNJwm.patch
Comment 20 Jonathan Druart 2017-05-05 13:28:12 UTC
Created attachment 63168 [details] [review]
Bug 18242: 16.05 version - [SOLUTION 2]Add tests
Comment 21 Jonathan Druart 2017-05-05 13:28:16 UTC
Created attachment 63169 [details] [review]
Bug 18242: 16.05 version - [SOLUTION 2]Handle correctly move to old_issues

The table old_issues has a primary key defined on the issue_id column.
This issue_id comes from the issues table when an item is checked in.

In some case the value of issue_id already exists in the table

Basically this happens when an item is returned and mysqld is restarted:
The auto increment value for issues.issue_id will be reset to
MAX(issue_id)+1 (which is the value of the last entry of old_issues).
See also the description of bug 18003 for more informations.

In this solution the change is done at code level instead of DB
structure: If old_issues.issue_id already exists before moving from
the issues table, the issue_id is updated (not on cascade for
accountlines.issue_id, should it?) before the move.
Comment 22 Jonathan Druart 2017-05-05 13:28:20 UTC
Created attachment 63170 [details] [review]
Bug 18242: 16.05 version - 16.11.x adaptation - Old::Checkouts vs OldIssues
Comment 23 Jonathan Druart 2017-05-05 13:29:07 UTC
Last 3 patches are for 16.05.x
I DID NOT TEST THEM!
If you want them for 16.05.x, please test.
Comment 24 Mason James 2017-05-08 02:21:52 UTC
(In reply to Jonathan Druart from comment #23)
> Last 3 patches are for 16.05.x
> I DID NOT TEST THEM!
> If you want them for 16.05.x, please test.

thanks Jonathan, the patchset looks to be working as expected  on 16.05.x :)
Comment 25 Mason James 2017-05-08 02:22:25 UTC
Pushed to 16.05.x, for 16.05.12 release
Comment 26 Jonathan Druart 2017-05-22 17:33:34 UTC
Updating the bug report title, we did not remove the PK on old_issues.
Comment 27 Jonathan Druart 2017-05-22 17:48:15 UTC
(In reply to Jonathan Druart from comment #21)
> In this solution the change is done at code level instead of DB
> structure: If old_issues.issue_id already exists before moving from
> the issues table, the issue_id is updated (not on cascade for
> accountlines.issue_id, should it?) before the move.

See bug 18651 for a follow-up
Comment 28 Katrin Fischer 2017-06-27 22:08:49 UTC
Tests fail for me with and without this patch:
 
1..26
ok 1 - connect to intranet
ok 2 - login to staff client
ok 3 - load main page
ok 4 - open tools module
Link not found at t/db_dependent/www/batch.t line 69.
# Looks like your test exited with 255 just after 4.
Comment 29 Pongtawat 2017-07-28 07:12:51 UTC
This patch cause a big problem for me in 16.11.

The way the new issue_id are calculated/updated didn't take into consideration the case that 2 checkouts for the same item happen at the same time. While it is very unlikely, I think it shouldn't cause a problem.

In our case, our Self-return vendor make a mistake and issue 2 SIP checkin messages at the same time. One of them work properly, but the other one incorrectly move the latest row in issues into old_issues. This cause the item to lost it checkout status (almost).

Probably we should backport #18966 to oldstable too?
Comment 30 Pongtawat 2017-07-28 07:19:44 UTC
Oh, I just notice that 16.11.10 was just released and it includes #18966 too :)
Comment 31 Tomás Cohen Arazi 2017-07-28 11:58:43 UTC
(In reply to Pongtawat from comment #30)
> Oh, I just notice that 16.11.10 was just released and it includes #18966 too
> :)

\o/
Comment 32 Sophie MEYNIEUX 2017-08-24 14:06:49 UTC
This patch does not always  work for 16.11

When you change issue_id with max(issue_id)+1 from old_issues but try to insert it into issues, you may have a primary key conflict.

my $new_issue_id = $dbh->selectrow_array(q|SELECT MAX(issue_id)+1 FROM old_issues|);
    $dbh->do(
        q|UPDATE issues SET issue_id = ? WHERE issue_id = ?|,
        undef, $new_issue_id, $issue_id
    );
Comment 33 Jonathan Druart 2017-08-25 12:51:28 UTC
(In reply to Sophie MEYNIEUX from comment #32)
> This patch does not always  work for 16.11

Which versions of 16.11.x?
You should read the description of bug 18966.