https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65151 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65151&action=edit Bug 18966: Do not deal with duplicate issue_id on checkin Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables. For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table. That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242. The goal was to prevent data lost. Data lost may happens in this case: Check an item out (issue_id = 1) Check an item in (issue_id = 1) Restart MySQL (reset auto increment for issue_id to 1) Check an item out (issue_id = 1) Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails. Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues. That has been fixed using a transaction. This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected. A good way to fix that would have been to LOCK the tables: 1- Start a transaction 2- LOCK the table to make sure nobody will read id and avoid race conditions 3- Move the content from one table to the other, dealing with ids 4- UNLOCK the table 5- Commit the transaction But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table). Finally the solution implemented is not acceptable for several reasons: - batch checkins may fail - issue_id will always stay out of sync (between issues and old_issues) See 18651 comment 66. Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to: 1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup => It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions, 2- Display a warning on the checkin page and link to a solution/explanation 3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix 4- Display a warning on the about page for corrupted data (see bug 18931) 5- Write and make available a maintenance script to fix corrupted data (TODO LATER) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 65155 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65155&action=edit [16.11.x] Bug 18966: Do not deal with duplicate issue_id on checkin -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |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=18966 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #6 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to David Cook from comment #5)

Do you have a test plan for testers to follow for this one?

Check an item out (issue_id = 1) Check an item in (issue_id = 1) Restart MySQL (reset auto increment for issue_id to 1) Check an item out (issue_id = 1) Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails. For starters, instead of BOOM we should see the new warning ;) Please note the dependencies.

=> It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions,

Hm, I think it can be more than one, depending on how many items you check out and return immediately before the restart (check-out 3, check-in 3, restart mysql, PK is behind by 3) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- For another approach, a few unpolished patches on 18970 based on reverting 18242 and not keeping the renumbering (taking some possible data loss on older issues for granted). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65151|0 |1 is obsolete| | --- Comment #9 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 65203 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65203&action=edit Bug 18966: Do not deal with duplicate issue_id on checkin Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables. For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table. That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242. The goal was to prevent data lost. Data lost may happens in this case: Check an item out (issue_id = 1) Check an item in (issue_id = 1) Restart MySQL (reset auto increment for issue_id to 1) Check an item out (issue_id = 1) Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails. Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues. That has been fixed using a transaction. This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected. A good way to fix that would have been to LOCK the tables: 1- Start a transaction 2- LOCK the table to make sure nobody will read id and avoid race conditions 3- Move the content from one table to the other, dealing with ids 4- UNLOCK the table 5- Commit the transaction But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table). Finally the solution implemented is not acceptable for several reasons: - batch checkins may fail - issue_id will always stay out of sync (between issues and old_issues) See 18651 comment 66. Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to: 1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup => It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions, 2- Display a warning on the checkin page and link to a solution/explanation 3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix 4- Display a warning on the about page for corrupted data (see bug 18931) 5- Write and make available a maintenance script to fix corrupted data (TODO LATER) 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=18966 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65155|0 |1 is obsolete| | --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 65205 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65205&action=edit (16.11.x) Bug 18966: Do not deal with duplicate issue_id on checkin Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables. For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table. That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242. The goal was to prevent data lost. Data lost may happens in this case: Check an item out (issue_id = 1) Check an item in (issue_id = 1) Restart MySQL (reset auto increment for issue_id to 1) Check an item out (issue_id = 1) Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails. Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues. That has been fixed using a transaction. This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected. A good way to fix that would have been to LOCK the tables: 1- Start a transaction 2- LOCK the table to make sure nobody will read id and avoid race conditions 3- Move the content from one table to the other, dealing with ids 4- UNLOCK the table 5- Commit the transaction But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table). Finally the solution implemented is not acceptable for several reasons: - batch checkins may fail - issue_id will always stay out of sync (between issues and old_issues) See 18651 comment 66. Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to: 1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup => It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions, 2- Display a warning on the checkin page and link to a solution/explanation 3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix 4- Display a warning on the about page for corrupted data (see bug 18931) 5- Write and make available a maintenance script to fix corrupted data (TODO LATER) 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=18966 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65156|0 |1 is obsolete| | --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 65207 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65207&action=edit (16.05.x) Bug 18966: Do not deal with duplicate issue_id on checkin Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables. For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table. That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242. The goal was to prevent data lost. Data lost may happens in this case: Check an item out (issue_id = 1) Check an item in (issue_id = 1) Restart MySQL (reset auto increment for issue_id to 1) Check an item out (issue_id = 1) Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails. Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues. That has been fixed using a transaction. This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected. A good way to fix that would have been to LOCK the tables: 1- Start a transaction 2- LOCK the table to make sure nobody will read id and avoid race conditions 3- Move the content from one table to the other, dealing with ids 4- UNLOCK the table 5- Commit the transaction But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table). Finally the solution implemented is not acceptable for several reasons: - batch checkins may fail - issue_id will always stay out of sync (between issues and old_issues) See 18651 comment 66. Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to: 1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup => It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions, 2- Display a warning on the checkin page and link to a solution/explanation 3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix 4- Display a warning on the about page for corrupted data (see bug 18931) 5- Write and make available a maintenance script to fix corrupted data (TODO LATER) 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=18966 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #8)

(In reply to David Cook from comment #5)

...

Do you have a test plan for testers to follow for this one?

Nope, you should only test this if you understand the problem and followed the discussions on the previous bug reports.

I understand the problem, but super time poor, so was hoping for some easy copy/pasting. Just me being lazy I guess :p. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 --- Comment #17 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #15)

For these patches we need people strongly involved in the testing process. We (and especially I) lost a lot of time because the previous tries have not been tested widely enough. If you follow a test plan that I have written it will be nearly useless to validate it (since I already tested it when I wrote the patch). You will need to think about something I forgot.

By that token, every test plan is useless. Typically, I'll follow a test plan, and during the process I may try something else and see a problem with the patch. I'm really invested in fixing this bug. It constantly affects us, and while we have local workarounds, it drives me crazy. It's embarrassing constantly telling people that there is a problem with Koha that we can't immediately fix. But I suppose if it bothers me that much... maybe I should involve myself more in the testing process. I have to go buy some new headphones right now, but when I return, I will look at this again, as this is perhaps my biggest pet peeve about Koha at the moment. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #65203|0 |1 is obsolete| | --- Comment #19 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 65272 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65272&action=edit Bug 18966: Do not deal with duplicate issue_id on checkin Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables. For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table. That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242. The goal was to prevent data lost. Data lost may happens in this case: Check an item out (issue_id = 1) Check an item in (issue_id = 1) Restart MySQL (reset auto increment for issue_id to 1) Check an item out (issue_id = 1) Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails. Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues. That has been fixed using a transaction. This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected. A good way to fix that would have been to LOCK the tables: 1- Start a transaction 2- LOCK the table to make sure nobody will read id and avoid race conditions 3- Move the content from one table to the other, dealing with ids 4- UNLOCK the table 5- Commit the transaction But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table). Finally the solution implemented is not acceptable for several reasons: - batch checkins may fail - issue_id will always stay out of sync (between issues and old_issues) See 18651 comment 66. Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to: 1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup => It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions, 2- Display a warning on the checkin page and link to a solution/explanation 3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix 4- Display a warning on the about page for corrupted data (see bug 18931) 5- Write and make available a maintenance script to fix corrupted data (TODO LATER) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #20 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=18966 --- Comment #22 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- (In reply to Fridolin SOMERS from comment #21)

Checking for 17.05.x :

UT passes but there is a new warn "Violation of unique constraint in OldIssue". Only when old_issues already contains datas, so no problem.

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 --- Comment #24 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Fridolin SOMERS from comment #23)

Just, we may use this error message in SIP serveur : C4/SIP/ILS/Transaction/Checkin.pm ?

What do you mean? I do not think we should display a specific message in this case. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 --- Comment #26 from Mason James <mtj@kohaaloha.com> --- Created attachment 65294 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65294&action=edit [16.11.x] Bug 18966: fix unneeded INSERT in AddReturn() -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 Bug 18966 depends on bug 18651, which changed state. Bug 18651 Summary: Move of checkouts is still not correctly handled https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Stable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |colin.campbell@ptfs-europe. | |com Blocks| |18996 --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #24)

(In reply to Fridolin SOMERS from comment #23)

...

Just, we may use this error message in SIP serveur : C4/SIP/ILS/Transaction/Checkin.pm ?

What do you mean? I do not think we should display a specific message in this case.

It seems that we still have a problem here. 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. @Colin: Could you have a look too ? Opened bug 18996 for this *issue*. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18996 [Bug 18996] SIP sets ok flag to true for refused checkin for data corruption -- You are receiving this mail because: You are watching all bug changes.

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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 --- Comment #32 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Looks like the "update the fines" part should be removed (it should not hurt, no side-effects expected); 2177 # Update the fines 2178 $dbh->do(q|UPDATE accountlines SET issue_id = ? WHERE issue_id = ?|, undef, $old_checkout->issue_id, $issue->issue_id); -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 --- Comment #34 from Colin Campbell <colin.campbell@ptfs-europe.com> --- (In reply to Jonathan Druart from comment #32)

Looks like the "update the fines" part should be removed (it should not hurt, no side-effects expected);

2177 # Update the fines 2178 $dbh->do(q|UPDATE accountlines SET issue_id = ? WHERE issue_id = ?|, undef, $old_checkout->issue_id, $issue->issue_id);

Yes its a hangover from the stage when the code was changing issue_id in issues, its now at best a waste of cpu cycles -- You are receiving this mail because: You are watching all bug changes.