Bug 26208 - Overdues restrictions not consistently removed when renewing multiple items at once
Summary: Overdues restrictions not consistently removed when renewing multiple items a...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical with 1 vote (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
: 27435 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-08-13 18:49 UTC by Andrew Fuerste-Henry
Modified: 2021-03-22 03:08 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04,20.05.10


Attachments
Bug 26208: Perform batch checkin/renewals serially rather than asynchronously (6.97 KB, patch)
2020-12-03 19:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26208: Perform batch checkin/renewals serially rather than asynchronously (6.97 KB, patch)
2020-12-07 19:05 UTC, Kelly McElligott
Details | Diff | Splinter Review
Bug 26208: Perform batch checkin/renewals serially rather than asynchronously (7.03 KB, patch)
2020-12-16 14:47 UTC, Kelly McElligott
Details | Diff | Splinter Review
Bug 26208: (follow-up) Remove debugging statements (1.27 KB, patch)
2021-01-22 12:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26208: Perform batch checkin/renewals serially rather than asynchronously (7.09 KB, patch)
2021-02-22 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26208: (follow-up) Remove debugging statements (1.33 KB, patch)
2021-02-22 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2020-08-13 18:49:34 UTC
To recreate:
- have multiple overdue items on one patron
- run overdues.pl with triggers set to generate a restriction
- renew all overdues with the Renew All button
- restriction is not removed even though patron no longer has overdue items

I did not get this error with 5 overdues, but I did get it with 10. Seems like it's due to some messy sequencing when sending a bunch of renewals at once, so it may be inconsistent to recreate.
Comment 1 Amber Wilde 2020-09-10 17:08:50 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.
Comment 2 Nick Clemens 2020-12-03 19:17:53 UTC Comment hidden (obsolete)
Comment 3 David Cook 2020-12-04 02:11:03 UTC
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?
Comment 4 Nick Clemens 2020-12-04 11:38:30 UTC
(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
Comment 5 David Cook 2020-12-06 22:37:10 UTC
(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.
Comment 6 Kelly McElligott 2020-12-07 18:12:36 UTC
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.
Comment 7 Kelly McElligott 2020-12-07 19:05:27 UTC Comment hidden (obsolete)
Comment 8 Kelly McElligott 2020-12-16 14:47:29 UTC
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>
Comment 9 Victor Grousset/tuxayo 2021-01-09 23:28:22 UTC
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.
Comment 10 Nick Clemens 2021-01-11 12:34:04 UTC
(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
Comment 11 Jonathan Druart 2021-01-18 14:37:45 UTC
*** Bug 27435 has been marked as a duplicate of this bug. ***
Comment 12 Martin Renvoize 2021-01-22 12:09:26 UTC
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...
Comment 13 Nick Clemens 2021-01-22 12:23:28 UTC
Created attachment 115681 [details] [review]
Bug 26208: (follow-up) Remove debugging statements
Comment 14 Martin Renvoize 2021-02-22 16:08:38 UTC
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>
Comment 15 Martin Renvoize 2021-02-22 16:08:41 UTC
Created attachment 117163 [details] [review]
Bug 26208: (follow-up) Remove debugging statements

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2021-02-22 16:09:17 UTC
Thanks Nick,

Fixes issue and no regressions found.

Passing QA
Comment 17 Jonathan Druart 2021-02-23 12:21:24 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 18 Fridolin Somers 2021-02-25 11:00:00 UTC
On master, follow-up squashed with first patch
Comment 19 Fridolin Somers 2021-02-25 11:13:49 UTC
Pushed to 20.11.x for 20.11.04
Comment 20 Andrew Fuerste-Henry 2021-03-05 17:47:18 UTC
Pushed to 20.05.x for 20.05.10
Comment 21 Victor Grousset/tuxayo 2021-03-22 03:08:35 UTC
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.