Description
Andrew Fuerste-Henry
2020-08-13 18:49:34 UTC
We have found that the overdues restriction is not automatically clearing even after using renew all on just two overdue items on an account. Created attachment 114165 [details] [review] Bug 26208: Perform batch checkin/renewals serially rather than asynchronously The issue here seems to be that when multiple requests hit at once they may not register that the renewal performed by the request should lift restrictions on the account. To mitigate this we can simply perform the renewals one after the other. To test: 1 - have multiple overdue items on one patron 2 - run overdues.pl with triggers set to generate a restriction 3 - renew all overdues with the Renew All button 4 - restriction is not removed even though patron no longer has overdue items 5 - Apply patch 6 - Make all items overdue again 7 - Click Renew All 8 - Items are renewed and restriction removed 9 - Checkout items to patron again (overdue or not) 10 - Click 'Select all' in checkin column 11 - Click 'Renew/Checkin selected items' 12 - Confirm checkin succeeds as before patches This sounds like an anti-feature to me. Nick's note on Bug 26457 says that renewals are silently failing. I'd say it would be better to make them fail loudly rather than making this work serially? (In reply to David Cook from comment #3) > This sounds like an anti-feature to me. > > Nick's note on Bug 26457 says that renewals are silently failing. I'd say it > would be better to make them fail loudly rather than making this work > serially? In these cases with restrictions there are no failures, just a race condition: # Remove any OVERDUES related debarment if the borrower has no overdues if ( $patron && $patron->is_debarred && ! $patron->has_overdues && @{ GetDebarments({ borrowernumber => $borrowernumber, type => 'OVERDUES' }) } ) { DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' }); } When all the transactions hit at once they are getting conflicting information and not removing the debarment, so all items renewed but restrictions stay I filed bug 27152, but currently this is a bug affecting many sites and this is a simple solution. There is a performance cost, but there will be with a bulk endpoint too (In reply to Nick Clemens from comment #4) > In these cases with restrictions there are no failures, just a race > condition: > # Remove any OVERDUES related debarment if the borrower has no > overdues > if ( $patron > && $patron->is_debarred > && ! $patron->has_overdues > && @{ GetDebarments({ borrowernumber => $borrowernumber, type => > 'OVERDUES' }) } > ) { > DelUniqueDebarment({ borrowernumber => $borrowernumber, type => > 'OVERDUES' }); > } > > When all the transactions hit at once they are getting conflicting > information and not removing the debarment, so all items renewed but > restrictions stay > Ok that's interesting. So the last 2 renewals for overdues will be happening around the same time, so $patron->has_overdues will be true for both. Yeah that's not good. Thanks for the clarification. > I filed bug 27152, but currently this is a bug affecting many sites and this > is a simple solution. There is a performance cost, but there will be with a > bulk endpoint too I would say that a bulk endpoint could actually have less of a performance cost, because with a bulk endpoint you could do less database I/O by fetching all the database data with 1 query. Although I guess with our use of DBIC we don't really do that as we currently prioritize convenience over performance. Anyway, food for thought. Nick, I am going through the test plan, and with the patch, the "renew all" does not remove the overdue restriction on the patron account. Created attachment 114241 [details] [review] Bug 26208: Perform batch checkin/renewals serially rather than asynchronously The issue here seems to be that when multiple requests hit at once they may not register that the renewal performed by the request should lift restrictions on the account. To mitigate this we can simply perform the renewals one after the other. To test: 1 - have multiple overdue items on one patron 2 - run overdues.pl with triggers set to generate a restriction 3 - renew all overdues with the Renew All button 4 - restriction is not removed even though patron no longer has overdue items 5 - Apply patch 6 - Make all items overdue again 7 - Click Renew All 8 - Items are renewed and restriction removed 9 - Checkout items to patron again (overdue or not) 10 - Click 'Select all' in checkin column 11 - Click 'Renew/Checkin selected items' 12 - Confirm checkin succeeds as before patches Created attachment 114440 [details] [review] Bug 26208: Perform batch checkin/renewals serially rather than asynchronously The issue here seems to be that when multiple requests hit at once they may not register that the renewal performed by the request should lift restrictions on the account. To mitigate this we can simply perform the renewals one after the other. To test: 1 - have multiple overdue items on one patron 2 - run overdues.pl with triggers set to generate a restriction 3 - renew all overdues with the Renew All button 4 - restriction is not removed even though patron no longer has overdue items 5 - Apply patch 6 - Make all items overdue again 7 - Click Renew All 8 - Items are renewed and restriction removed 9 - Checkout items to patron again (overdue or not) 10 - Click 'Select all' in checkin column 11 - Click 'Renew/Checkin selected items' 12 - Confirm checkin succeeds as before patches Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Hi, trying to do a second signoff.
overdue_notices.pl + the right trigger generated a restriction as expected.
But then
> 3 - renew all overdues with the Renew All button
> 4 - restriction is not removed even though patron no longer has overdue items
The restriction is removed without the patch being applied. No idea what when wrong, retried and same result. Database is just the fresh sample data.
(In reply to Victor Grousset/tuxayo from comment #9) > Hi, trying to do a second signoff. > > overdue_notices.pl + the right trigger generated a restriction as expected. > > But then > > > 3 - renew all overdues with the Renew All button > > 4 - restriction is not removed even though patron no longer has overdue items > > The restriction is removed without the patch being applied. No idea what > when wrong, retried and same result. Database is just the fresh sample data. Hi Victor, How many overdues did you have? Try with more? It's random failure from asynchronous renewals as far as I can tell *** Bug 27435 has been marked as a duplicate of this bug. *** Comment on attachment 114440 [details] [review] Bug 26208: Perform batch checkin/renewals serially rather than asynchronously Review of attachment 114440 [details] [review]: ----------------------------------------------------------------- Generally, I'm happy with this, I can't find any real regressions and it appears to resolve the issue. However, we introduce a few debugging lines which should really be removed before it can pass QA ::: koha-tmpl/intranet-tmpl/prog/js/checkouts.js @@ +182,5 @@ > + success: function( data ) { > + var id = "#renew_" + data.itemnumber; > + console.log( data); > + console.log( data.renew_okay); > + console.log( data.itemnumber); Can we removed/comment these console.log() statements? @@ +211,3 @@ > } > + console.log(content); > + console.log(id); and another couple... Created attachment 115681 [details] [review] Bug 26208: (follow-up) Remove debugging statements Created attachment 117162 [details] [review] Bug 26208: Perform batch checkin/renewals serially rather than asynchronously The issue here seems to be that when multiple requests hit at once they may not register that the renewal performed by the request should lift restrictions on the account. To mitigate this we can simply perform the renewals one after the other. To test: 1 - have multiple overdue items on one patron 2 - run overdues.pl with triggers set to generate a restriction 3 - renew all overdues with the Renew All button 4 - restriction is not removed even though patron no longer has overdue items 5 - Apply patch 6 - Make all items overdue again 7 - Click Renew All 8 - Items are renewed and restriction removed 9 - Checkout items to patron again (overdue or not) 10 - Click 'Select all' in checkin column 11 - Click 'Renew/Checkin selected items' 12 - Confirm checkin succeeds as before patches Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 117163 [details] [review] Bug 26208: (follow-up) Remove debugging statements Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Thanks Nick, Fixes issue and no regressions found. Passing QA Pushed to master for 21.05, thanks to everybody involved! On master, follow-up squashed with first patch Pushed to 20.11.x for 20.11.04 Pushed to 20.05.x for 20.05.10 Can't backport to 19.11.x: can't solve conflicts without risking messing up stuff. If there is an interest in having this backported, please submit a patch for 19.11. |