On systems with a decent amount of fines it seems the fines cron can cause issues for patrons. The problem seems to be: 1 - Fines job runs and selects overdues 2 - Patron with an overdue returns books, fine is updated, item is checked in 3 - Fines job iterates to the item that was checked in during step 2, sees there is no accruing fine, generates a new accruing fine I am not sure what the best solution here is, possibly we should confirm the issue before generating the fine in the cronjob?
Hi Nick, makes sense to me, not sure how that would impact performance.
Did you managed to confirm this one Nick? I'm wondering if it's now 'gone away' or still needs investigating.
I seem to have run into this also. I'm running a modified 21.05.05. I have, since noticing the duplicates, deleted all the duplicates, and changed my systems crontab so that daily tasks will run before circulation starts. I've looked at the code in cronjobs/fines.pl and circ/returns.pl. It seems that AddReturn calls _FixOverduesOnReturn which flips the status to 'RETURNED'. Then fines.pl calls UpdateFine, which can't find the fine because the status has been flipped, so it add a new one. I'm think the best course of action would be to have UpdateFine return if the fine has been flipped to 'RETURNED' and 'CalculateFinesOnReturn' is on, since AddReturn will have already updated the fine in that case. Though that still doesn't over the edge-case of 'CalculateFinesOnReturn' being off, since AddReturn still calls _FixOverduesOnReturn either way. With 'CalculateFinesOnReturn' off, when the book gets returned before fines.pl gets to it, UpdateFine still won't find it because _FixOverduesOnReturn flipped the status, but AddReturn didn't call _CalculateAndUpdateFine. The duplicate will get added, but the amount on the original fine wasn't updated. So maybe the best course of action is to have UpdateFine not care about status when looking for the fine, and then only adjust the fine is it's 'UNRETURNED' or 'CalculateFinesOnReturn' is off. (In addition to the check for amount changing.)
Created attachment 127816 [details] [review] Proposed fix
This looks good.. My one query is around the `|| !C4::Context->preference('CalculateFinesOnReturn')` introduction.. I'm not sure if that's the behaviour we want/expect.. should we still increment a fine after return here? I realise, it's to do with the job being long running and so the patron has 'got away with it' as it were.. but we could be introducing questions if the patron returns their item, pays their fine and then finds they have a bit more to pay after the fact because the calculations were still running? Otherwise, I think this is a great little improvement.
Created attachment 127851 [details] [review] Bug 18855: Don't create duplicate overdue fines if an issue is returned If cronjobs/fines.pl is running during circulation hours, then an issue may be considered for having it's overdue fine updated after the issue has been returned and it's fine status flipped from 'UNRETURNED' to 'RETURNED'. In this case UpdateFine will create a duplicate fine because it can't find the specific accountline for the overdue and unreturned fine. This changes UpdateFine to not care about status, so the accountline will be found, until it comes to adjusting the fine. At this point if the fine is 'UNRETURNED' or 'CalculateFinesOnReturn' is off (the book was returned and it's overdue fine status flipped, but the amount wasn't updated), then go ahead and adjust the fine amount. Test plan: 1. find an overdue fine near the end of the list of overdue fines that cronjobs/fines.pl will be considering. 2. start cronjobs/fines.pl. 3. immediately check in the overdue book. 4. once fines.pl is finished observe that a duplicate overdue fine has been created on the patrons account. 5. apply patch. 6. repeat 1 - 4 and observe that the duplicate fine was not created. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 127852 [details] [review] Bug 18855: (follow-up) Restore CalculateFinesOnReturn behaviour This patch removes the check for a disabled 'CalculateFinesOnReturn' to restore the behaviour prior to this patch series. If a patron has returned the item during the fines cron run, they may well have been charged a reduced fine at check-in time. However, I feel that the fine should not then increment/decrement behind the scenes after the check-in. This is to reduce confusion for end users and librarians alike.
Signing off.. leaving to QA/RM to decide whether to keep my follow-up or not.
I am totally in agreement with you on this. (In reply to Martin Renvoize from comment #7) > Created attachment 127852 [details] [review] [review] > Bug 18855: (follow-up) Restore CalculateFinesOnReturn behaviour > > This patch removes the check for a disabled 'CalculateFinesOnReturn' > to restore the behaviour prior to this patch series. > > If a patron has returned the item during the fines cron run, they may > well have been charged a reduced fine at check-in time. However, I feel > that the fine should not then increment/decrement behind the scenes > after the check-in. This is to reduce confusion for end users and > librarians alike.
Can you provide tests, please?
(In reply to Jonathan Druart from comment #10) > Can you provide tests, please? Looking through the db_dependent/Overdues.t file I notice that Koha expects a new fine to be generated if an overdue book is renewed and then becomes overdue again. This complicates things since a book renewed just after the cronjob starts could look like it was renewed previously and became overdue again. Now I'm trying to think of how to tell the difference between a book renewed a couple weeks ago (for example) and a book renewed after the cronjob started. Does the cronjob/fines.pl script need to pull the fines status again just before updating the fine to make sure the status hasn't changed, instead of changing how C4::Overdues::UpdateFine() handles status? But then we'd have to make the cronjob lock the database at the point of refreshing the status and updating the fine; otherwise we've only reduced the window on the race condition (to nearly nil). Or is the very small chance of a race condition good enough? (I don't think it is, but I don't like locking the database during operating hours either.) I'm going to chose the locking option, and make a new patch to do that. (Bonus, I don't have to write a test case ;) )
Created attachment 128187 [details] [review] Proposed fix in cronjobs/fines.pl This changes cronjobs/fines.pl instead of C4/Overdues.pm Test plan: 1. find an overdue fine near the end of the list of overdue fines that cronjobs/fines.pl will be considering. 2. start cronjobs/fines.pl. 3. immediately check in or renew the overdue book. 4. once fines.pl is finished observe that a duplicate overdue fine has been created on the patrons account. 5. apply patch. 6. repeat 1 - 4 and observe that the duplicate fine was not created.
Using a table lock seems like a crude solution. Why not use a transaction?
(In reply to Andreas Jonsson from comment #13) > Using a table lock seems like a crude solution. > > Why not use a transaction? I agree that the table lock is pretty ugly. Honestly, I haven't tested that it solves the problem either. Think of it, it probably creates a problem where the accountlines table is locked during circulation. (I just found out that foreign key tables also get locked, which compounds this problem.) But a transaction would not prevent multiple threads from changing the same row, which is what I was trying to catch there. I'd be fine with going back to Martin's patches. (But a test would still have to be written. ;) ) Besides, I'm not even using the table locking patch in production here. I've moved on from this, so I'll have to dig in a bit to remember what all is going on, to figure out how much of a problem the 'renewed is still overdue so new fine' case is. This keys on the issue_id, but maybe that can be changed to key on the accountlines_id instead; that would be a better solution to this problem I think. I'll dig in a bit and see if I can get this to key on the accountlines_id instead.
I think I figured out why the code here was the way it was. It has to do with the possibility of a book being renewed and then becoming overdue again. The way the code was there would be a second overdue fine added to the account lines for this second instance of the issue becoming overdue. With the patches to dropping the status check the second overdue fine would never get entered since the previous overdue fine would be found instead. And the table locking patch doesn't lock the table soon enough to fix the duplication problem. (Which is besides the fact that locking in a long running process would totally kill the circulation desk.) Which brings me back to using accountlines_id to find the overdue fine instead of issue_id. We need to either make sure the fines cron job doesn't run during circulation hours, or find and use the accountlines_id when pulling the list of overdues. Those are the best solutions I can think of, either run the script when things aren't going to change, or make sure the script can find the right account line in case things change so we don't have a duplicate. So, I need to either patch cronjobs/fines.pl and C4/Overdues.pm to use accountlines_id, or patch the cron files to specify when the fines cron job runs (move some jobs from cron.daily to cron.d). I prefer patching fines.pl and C4/Overdues.pm, since I can't be sure how long the cron jobs would run, and some should finish before others start. Options and alternate solutions welcome.
Created attachment 131304 [details] [review] Have the fines cronjob check the issue before updateing the fine Here is another possible solution to this bug. Have the fines cron job check if the issue due date has changed before updating the fine. If the due date changed while the script was processing other issues, then we'll pass on it this time. That way duplicates can be avoided, and issues that become overdue again will get the second fine as they should (on the next run if it was renewed during this run).
I am not sure yet I understand the issue. Can you please add a test plan to your commit message? If the item was renewed and became overdue a second time, my expectation would be that the fine process 'starts over' with a new accountlines entry and starting to calculate 'from 0'.
Here's the test plan from the patch file: Test plan: 1. find an overdue fine near the end of the list of overdue fines that cronjobs/fines.pl will be considering. 2. start cronjobs/fines.pl. 3. immediately check in the overdue book. 4. once fines.pl is finished observe that a duplicate overdue fine has been created on the patrons account. 5. apply patch. 6. repeat 1 - 4 and observe that the duplicate fine was not created. Re: your comment (I replied to the email, but don't see it here): Yes, that would be normal operation. The fine for the issue is identified by being unreturned. If a book is renewed it's fine is returned, and if it becomes overdue again the UpdateFine call will create the new one. The bug is if an overdue book is returned or renewed while the script is running, but before the script has processed that issue, then it's fine will be returned, but the script still called UpdateFine, so another fine is created since it couldn't find the one that was just changed to returned. I've changed the script to check the issue before calling UpdateFine, and skip the call this time if the issue (due date) changed to avoid the duplicate fine. There is still a very narrow window where UpdateFine gets called and at the same time the fine is changed to returned, but I think that chance is small enough to overlook. The alternate would be to lock the tables before calling UpdateFine, but that would slow down the circulation desk while the fines script runs, which would probably bring complaints.
Created attachment 131961 [details] [review] Bug 18855: Move the condition at the top of the iteration - skip the CalcFine call - use Koha::Checkout->is_overdue
Your patch felt a bit dirty at first but I don't have anything else to suggest actually. I've added a follow-up patch to tidy a bit the code around your changes. I am wondering about the "$counted" variable. When do we need to increment it? In my understanding it's correct with my patch (and was not before) but I am not 100% sure.
(In reply to Jonathan Druart from comment #20) > Your patch felt a bit dirty at first but I don't have anything else to > suggest actually. > I've added a follow-up patch to tidy a bit the code around your changes. > > I am wondering about the "$counted" variable. When do we need to increment > it? In my understanding it's correct with my patch (and was not before) but > I am not 100% sure. I'd say you are right about the $counted variable. You're follow up looks good. I prefer having the 'has the issue changed' check be closer to UpdateFine(), so that the chance of a race condition is as small as possible. But I could let that slide.
Hi all, I'm curious to know the usefulness of having the fines.pl cron run during library operation hours... If anyone can enlighten me on this, I would be very grateful. I was able to recreate the problem on master, and I created a detailed test plan, if you're interested (see below). However, the patches don't apply on the current master, so I was not able to test them. Here is my detailed test plan (I know it's a lot but I figured that I already wrote it, so might as well share it!) 0. Preliminary settings 0.1. finesMode system preference must be set to Calculate and charge 0.2. There must be a circulation rule that will charge fines (beware of bug 32271) 0.3. In Tools > Calendar, today must not be a holiday 1. Make a lot of overdue checkouts - I used the batch checkout feature, but if your system already has a lot of overdue checkouts, you can skip to step 2 1.1. Enable batch checkouts 1.1.1. Go to Administration > Global system preferences 1.1.2. Search for BatchCheckouts 1.1.3. Set BatchCheckouts to Allow 1.1.4. Select all categories in BatchCheckoutsValidCategories 1.1.5. Click "Save all Circulation preferences" 1.2. Get a list of barcodes 1.2.1. Go to Reports 1.2.2. Click "Create from SQL" 1.2.3. Give the report a name 1.2.4. For the SQL query, enter SELECT barcode FROM items WHERE onloan IS NULL LIMIT 60; 1.2.5. Click "Save report" 1.2.6. Click "Run report" 1.2.7. Click "Download" > "Tab separated text" 1.3. Go to a patron's file 1.3.1. Go to Patrons 1.3.2. Click on "Search" 1.3.3. Click on a patron's name 1.4. Do a batch checkout with a due date in the past 1.4.1. Click on the "Batch check out" tab on the left 1.4.2. In "Use a file", click "Choose file" 1.4.3. Choose the file downloaded from the report 1.4.4. In "Hard due date", choose a date in the past 1.4.5. Click "Check out" 1.4.6. Click "Checkout or renew" 2. Find the last issue in the database 2.1. In the database (or in reports), type the following query SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode, items.itemlost, items.replacementprice, items.biblionumber FROM issues LEFT JOIN items USING (itemnumber) WHERE date_due < NOW() \G; 2.2. Copy the barcode from the last entry 3. Set up so that you can run fines.pl and check in the item at the same time (or very close to the same time) 3.1. In Koha, click the "Check in" option in the search bar at the top of the page 3.2. Paste the barcode in the search bar BUT DO NOT PRESS ENTER OR THE ARROW RIGHT AWAY 3.3. In a terminal, enter the fines.pl command ./misc/cronjobs/fines.pl 3.4. Execute the command and immediately click on the arrow in the staff interface to check in the item 4. Check the patron's fines 4.1. Click on the patron's name in the check in screen 4.2. Go to the Accounting tab on the left 4.3. In the search box just above the table, paste in the returned item's barcode --> There are two fines, one Fine (Accruing) and one Fine (Returned) for the same item at the same time
(In reply to Caroline Cyr La Rose from comment #22) > Hi all, > I'm curious to know the usefulness of having the fines.pl cron run during > library operation hours... If anyone can enlighten me on this, I would be > very grateful. > The fines.pl is part of the daily cron job, which starts around 2:30 on my system. (as per /etc/crontab) By the time the daily cron job gets done with fines it's after 6, which is when some of my libraries open (I'm running Koha for a School District). It's probably not intended to be run during operation hours, it just happens to for me. I'll rebase the patch for current master and upload it.
Created attachment 144083 [details] [review] Bug 18855: Don't create duplicate overdue fines if an issue is returned [rebased Nov. 13, 2022] [includes Jonathan's followup] Test plan (by Caroline): 0. Preliminary settings 0.1. finesMode system preference must be set to Calculate and charge 0.2. There must be a circulation rule that will charge fines (beware of bug 32271) 0.3. In Tools > Calendar, today must not be a holiday 1. Make a lot of overdue checkouts - I used the batch checkout feature, but if your system already has a lot of overdue checkouts, you can skip to step 2 1.1. Enable batch checkouts 1.1.1. Go to Administration > Global system preferences 1.1.2. Search for BatchCheckouts 1.1.3. Set BatchCheckouts to Allow 1.1.4. Select all categories in BatchCheckoutsValidCategories 1.1.5. Click "Save all Circulation preferences" 1.2. Get a list of barcodes 1.2.1. Go to Reports 1.2.2. Click "Create from SQL" 1.2.3. Give the report a name 1.2.4. For the SQL query, enter SELECT barcode FROM items WHERE onloan IS NULL LIMIT 60; 1.2.5. Click "Save report" 1.2.6. Click "Run report" 1.2.7. Click "Download" > "Tab separated text" 1.3. Go to a patron's file 1.3.1. Go to Patrons 1.3.2. Click on "Search" 1.3.3. Click on a patron's name 1.4. Do a batch checkout with a due date in the past 1.4.1. Click on the "Batch check out" tab on the left 1.4.2. In "Use a file", click "Choose file" 1.4.3. Choose the file downloaded from the report 1.4.4. In "Hard due date", choose a date in the past 1.4.5. Click "Check out" 1.4.6. Click "Checkout or renew" 2. Find the last issue in the database 2.1. In the database (or in reports), type the following query SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode, items.itemlost, items.replacementprice, items.biblionumber FROM issues LEFT JOIN items USING (itemnumber) WHERE date_due < NOW() \G; 2.2. Copy the barcode from the last entry 3. Set up so that you can run fines.pl and check in the item at the same time (or very close to the same time) 3.1. In Koha, click the "Check in" option in the search bar at the top of the page 3.2. Paste the barcode in the search bar BUT DO NOT PRESS ENTER OR THE ARROW RIGHT AWAY 3.3. In a terminal, enter the fines.pl command ./misc/cronjobs/fines.pl 3.4. Execute the command and immediately click on the arrow in the staff interface to check in the item 4. Check the patron's fines 4.1. Click on the patron's name in the check in screen 4.2. Go to the Accounting tab on the left 4.3. In the search box just above the table, paste in the returned item's barcode --> There are two fines, one Fine (Accruing) and one Fine (Returned) for the same item at the same time
Created attachment 144084 [details] [review] Bug 18855: Don't create duplicate overdue fines if an issue is returned This includes Jonathan's followup If cronjobs/fines.pl is running during circulation hours, then an issue may be considered for having it's overdue fine updated after the issue has been returned and it's fine status flipped from 'UNRETURNED' to 'RETURNED'. In this case UpdateFine will create a duplicate fine because it can't find the specific accountline for the (formerly) overdue issue. This changes cronjobs/fines.pl to double check the issue before updating the fine. If the issue has changed between starting the script and updating the fine, then the script will skip it. There is a small amount of time between the check and calling UpdateFine where the issue can be changed and this problem can reoccure. The chance of that happening is so small that it's probably fine to leave as is. It is also possible that the fine won't be updated because the issue was returned. In this case the fine payed by the patron will be lower, but that is better then the patron finding later that there is more to a fine they thought they had paid all of. Test plan (by Caroline): 0. Preliminary settings 0.1. finesMode system preference must be set to Calculate and charge 0.2. There must be a circulation rule that will charge fines (beware of bug 32271) 0.3. In Tools > Calendar, today must not be a holiday 1. Make a lot of overdue checkouts - I used the batch checkout feature, but if your system already has a lot of overdue checkouts, you can skip to step 2 1.1. Enable batch checkouts 1.1.1. Go to Administration > Global system preferences 1.1.2. Search for BatchCheckouts 1.1.3. Set BatchCheckouts to Allow 1.1.4. Select all categories in BatchCheckoutsValidCategories 1.1.5. Click "Save all Circulation preferences" 1.2. Get a list of barcodes 1.2.1. Go to Reports 1.2.2. Click "Create from SQL" 1.2.3. Give the report a name 1.2.4. For the SQL query, enter SELECT barcode FROM items WHERE onloan IS NULL LIMIT 60; 1.2.5. Click "Save report" 1.2.6. Click "Run report" 1.2.7. Click "Download" > "Tab separated text" 1.3. Go to a patron's file 1.3.1. Go to Patrons 1.3.2. Click on "Search" 1.3.3. Click on a patron's name 1.4. Do a batch checkout with a due date in the past 1.4.1. Click on the "Batch check out" tab on the left 1.4.2. In "Use a file", click "Choose file" 1.4.3. Choose the file downloaded from the report 1.4.4. In "Hard due date", choose a date in the past 1.4.5. Click "Check out" 1.4.6. Click "Checkout or renew" 2. Find the last issue in the database 2.1. In the database (or in reports), type the following query SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode, items.itemlost, items.replacementprice, items.biblionumber FROM issues LEFT JOIN items USING (itemnumber) WHERE date_due < NOW() \G; 2.2. Copy the barcode from the last entry 3. Set up so that you can run fines.pl and check in the item at the same time (or very close to the same time) 3.1. In Koha, click the "Check in" option in the search bar at the top of the page 3.2. Paste the barcode in the search bar BUT DO NOT PRESS ENTER OR THE ARROW RIGHT AWAY 3.3. In a terminal, enter the fines.pl command ./misc/cronjobs/fines.pl 3.4. Execute the command and immediately click on the arrow in the staff interface to check in the item 4. Check the patron's fines 4.1. Click on the patron's name in the check in screen 4.2. Go to the Accounting tab on the left 4.3. In the search box just above the table, paste in the returned item's barcode --> Without the patch, there are two fines, one Fine (Accruing) and one Fine (Returned) for the same item at the same time --> With the patch, there is only one fine, Fine (Returned) Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Wow, thanks for the quick rebase Michael! I tested and I only have one fine, Fine (Returned), now.
(In reply to Caroline Cyr La Rose from comment #26) > Wow, thanks for the quick rebase Michael! > > I tested and I only have one fine, Fine (Returned), now. You're welcome. Thanks for the sign off.
I'm a bit concerned by the amount of database queries this change is going to cause. I wonder if instead we should change the query that updates the fine to only do so if the checkout is still overdue. That would solve the problem without producing additional queries. The other option would be to enable this feature via a command line option ( defaulting to off to keep the current behavior ).
The problem comes because the script can't find the right account line, because the fine changed status to returned after the script started but before the script got to that issue. I don't think we can avoid the extra database queries because the database is change elsewhere while the fines script is running. I could add a command line option, but that creates the situation where a library may have this problem, and in looking for a solution they find the command line option. Wouldn't they wonder why the option didn't default to on to avoid the problem? Anyway, I'll go ahead and add the command line option. I want to get this moving.
Created attachment 145496 [details] [review] Bug 18855: Don't create duplicate overdue fines if an issue is returned This includes Jonathan's followup and the command line switch suggested by QA If cronjobs/fines.pl is running during circulation hours, then an issue may be considered for having it's overdue fine updated after the issue has been returned and it's fine status flipped from 'UNRETURNED' to 'RETURNED'. In this case UpdateFine will create a duplicate fine because it can't find the specific accountline for the (formerly) overdue issue. This changes cronjobs/fines.pl to double check the issue before updating the fine. If the issue has changed between starting the script and updating the fine, then the script will skip it. There is a small amount of time between the check and calling UpdateFine where the issue can be changed and this problem can reoccure. The chance of that happening is so small that it's probably fine to leave as is. It is also possible that the fine won't be updated because the issue was returned. In this case the fine payed by the patron will be lower, but that is better then the patron finding later that there is more to a fine they thought they had paid all of. Test plan (by Caroline): 0. Preliminary settings 0.1. finesMode system preference must be set to Calculate and charge 0.2. There must be a circulation rule that will charge fines (beware of bug 32271) 0.3. In Tools > Calendar, today must not be a holiday 1. Make a lot of overdue checkouts - I used the batch checkout feature, but if your system already has a lot of overdue checkouts, you can skip to step 2 1.1. Enable batch checkouts 1.1.1. Go to Administration > Global system preferences 1.1.2. Search for BatchCheckouts 1.1.3. Set BatchCheckouts to Allow 1.1.4. Select all categories in BatchCheckoutsValidCategories 1.1.5. Click "Save all Circulation preferences" 1.2. Get a list of barcodes 1.2.1. Go to Reports 1.2.2. Click "Create from SQL" 1.2.3. Give the report a name 1.2.4. For the SQL query, enter SELECT barcode FROM items WHERE onloan IS NULL LIMIT 60; 1.2.5. Click "Save report" 1.2.6. Click "Run report" 1.2.7. Click "Download" > "Tab separated text" 1.3. Go to a patron's file 1.3.1. Go to Patrons 1.3.2. Click on "Search" 1.3.3. Click on a patron's name 1.4. Do a batch checkout with a due date in the past 1.4.1. Click on the "Batch check out" tab on the left 1.4.2. In "Use a file", click "Choose file" 1.4.3. Choose the file downloaded from the report 1.4.4. In "Hard due date", choose a date in the past 1.4.5. Click "Check out" 1.4.6. Click "Checkout or renew" 2. Find the last issue in the database 2.1. In the database (or in reports), type the following query SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode, items.itemlost, items.replacementprice, items.biblionumber FROM issues LEFT JOIN items USING (itemnumber) WHERE date_due < NOW() \G; 2.2. Copy the barcode from the last entry 3. Set up so that you can run fines.pl and check in the item at the same time (or very close to the same time) 3.1. In Koha, click the "Check in" option in the search bar at the top of the page 3.2. Paste the barcode in the search bar BUT DO NOT PRESS ENTER OR THE ARROW RIGHT AWAY 3.3. In a terminal, enter the fines.pl command ./misc/cronjobs/fines.pl 3.4. Execute the command and immediately click on the arrow in the staff interface to check in the item 4. Check the patron's fines 4.1. Click on the patron's name in the check in screen 4.2. Go to the Accounting tab on the left 4.3. In the search box just above the table, paste in the returned item's barcode --> Without the patch, there are two fines, one Fine (Accruing) and one Fine (Returned) for the same item at the same time --> With the patch, there is only one fine, Fine (Returned)
Created attachment 151647 [details] [review] Bug 18855: Don't create duplicate overdue fines if an issue is returned This includes Jonathan's followup If cronjobs/fines.pl is running during circulation hours, then an issue may be considered for having it's overdue fine updated after the issue has been returned and it's fine status flipped from 'UNRETURNED' to 'RETURNED'. In this case UpdateFine will create a duplicate fine because it can't find the specific accountline for the (formerly) overdue issue. This changes cronjobs/fines.pl to double check the issue before updating the fine. If the issue has changed between starting the script and updating the fine, then the script will skip it. There is a small amount of time between the check and calling UpdateFine where the issue can be changed and this problem can reoccure. The chance of that happening is so small that it's probably fine to leave as is. It is also possible that the fine won't be updated because the issue was returned. In this case the fine payed by the patron will be lower, but that is better then the patron finding later that there is more to a fine they thought they had paid all of. Test plan (by Caroline): 0. Preliminary settings 0.1. finesMode system preference must be set to Calculate and charge 0.2. There must be a circulation rule that will charge fines (beware of bug 32271) 0.3. In Tools > Calendar, today must not be a holiday 1. Make a lot of overdue checkouts - I used the batch checkout feature, but if your system already has a lot of overdue checkouts, you can skip to step 2 1.1. Enable batch checkouts 1.1.1. Go to Administration > Global system preferences 1.1.2. Search for BatchCheckouts 1.1.3. Set BatchCheckouts to Allow 1.1.4. Select all categories in BatchCheckoutsValidCategories 1.1.5. Click "Save all Circulation preferences" 1.2. Get a list of barcodes 1.2.1. Go to Reports 1.2.2. Click "Create from SQL" 1.2.3. Give the report a name 1.2.4. For the SQL query, enter SELECT barcode FROM items WHERE onloan IS NULL LIMIT 60; 1.2.5. Click "Save report" 1.2.6. Click "Run report" 1.2.7. Click "Download" > "Tab separated text" 1.3. Go to a patron's file 1.3.1. Go to Patrons 1.3.2. Click on "Search" 1.3.3. Click on a patron's name 1.4. Do a batch checkout with a due date in the past 1.4.1. Click on the "Batch check out" tab on the left 1.4.2. In "Use a file", click "Choose file" 1.4.3. Choose the file downloaded from the report 1.4.4. In "Hard due date", choose a date in the past 1.4.5. Click "Check out" 1.4.6. Click "Checkout or renew" 2. Find the last issue in the database 2.1. In the database (or in reports), type the following query SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode, items.itemlost, items.replacementprice, items.biblionumber FROM issues LEFT JOIN items USING (itemnumber) WHERE date_due < NOW() \G; 2.2. Copy the barcode from the last entry 3. Set up so that you can run fines.pl and check in the item at the same time (or very close to the same time) 3.1. In Koha, click the "Check in" option in the search bar at the top of the page 3.2. Paste the barcode in the search bar BUT DO NOT PRESS ENTER OR THE ARROW RIGHT AWAY 3.3. In a terminal, enter the fines.pl command ./misc/cronjobs/fines.pl 3.4. Execute the command and immediately click on the arrow in the staff interface to check in the item 4. Check the patron's fines 4.1. Click on the patron's name in the check in screen 4.2. Go to the Accounting tab on the left 4.3. In the search box just above the table, paste in the returned item's barcode --> Without the patch, there are two fines, one Fine (Accruing) and one Fine (Returned) for the same item at the same time --> With the patch, there is only one fine, Fine (Returned) Signed-off-by: Sam Lau <samalau@gmail.com>
QA: Looking here
Some observations: It is very good to add the parameter. That will make people more comfortable. But you check twice and the first time without. Could we remove the first check? And just depend on the last one (that has the parameter test)? The time between those two checks will be limited. After we did ++$counted; you still do a next in the second check, should we decrement counted again? Minor thing, bit picky..
Created attachment 152907 [details] [review] Bug 18855: Don't create duplicate overdue fines if an issue is returned [rebase 2023-06-25] [ QA followup: I removed the first due date check, but left the issue object check. Seemed like the right thing to do. I added a $counted decrement since the iteration is getting skipped before the fine is updated ] This includes Jonathan's followup If cronjobs/fines.pl is running during circulation hours, then an issue may be considered for having it's overdue fine updated after the issue has been returned and it's fine status flipped from 'UNRETURNED' to 'RETURNED'. In this case UpdateFine will create a duplicate fine because it can't find the specific accountline for the (formerly) overdue issue. This changes cronjobs/fines.pl to double check the issue before updating the fine. If the issue has changed between starting the script and updating the fine, then the script will skip it. There is a small amount of time between the check and calling UpdateFine where the issue can be changed and this problem can reoccure. The chance of that happening is so small that it's probably fine to leave as is. It is also possible that the fine won't be updated because the issue was returned. In this case the fine payed by the patron will be lower, but that is better then the patron finding later that there is more to a fine they thought they had paid all of. Test plan (by Caroline): 0. Preliminary settings 0.1. finesMode system preference must be set to Calculate and charge 0.2. There must be a circulation rule that will charge fines (beware of bug 32271) 0.3. In Tools > Calendar, today must not be a holiday 1. Make a lot of overdue checkouts - I used the batch checkout feature, but if your system already has a lot of overdue checkouts, you can skip to step 2 1.1. Enable batch checkouts 1.1.1. Go to Administration > Global system preferences 1.1.2. Search for BatchCheckouts 1.1.3. Set BatchCheckouts to Allow 1.1.4. Select all categories in BatchCheckoutsValidCategories 1.1.5. Click "Save all Circulation preferences" 1.2. Get a list of barcodes 1.2.1. Go to Reports 1.2.2. Click "Create from SQL" 1.2.3. Give the report a name 1.2.4. For the SQL query, enter SELECT barcode FROM items WHERE onloan IS NULL LIMIT 60; 1.2.5. Click "Save report" 1.2.6. Click "Run report" 1.2.7. Click "Download" > "Tab separated text" 1.3. Go to a patron's file 1.3.1. Go to Patrons 1.3.2. Click on "Search" 1.3.3. Click on a patron's name 1.4. Do a batch checkout with a due date in the past 1.4.1. Click on the "Batch check out" tab on the left 1.4.2. In "Use a file", click "Choose file" 1.4.3. Choose the file downloaded from the report 1.4.4. In "Hard due date", choose a date in the past 1.4.5. Click "Check out" 1.4.6. Click "Checkout or renew" 2. Find the last issue in the database 2.1. In the database (or in reports), type the following query SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode, items.itemlost, items.replacementprice, items.biblionumber FROM issues LEFT JOIN items USING (itemnumber) WHERE date_due < NOW() \G; 2.2. Copy the barcode from the last entry 3. Set up so that you can run fines.pl and check in the item at the same time (or very close to the same time) 3.1. In Koha, click the "Check in" option in the search bar at the top of the page 3.2. Paste the barcode in the search bar BUT DO NOT PRESS ENTER OR THE ARROW RIGHT AWAY 3.3. In a terminal, enter the fines.pl command ./misc/cronjobs/fines.pl 3.4. Execute the command and immediately click on the arrow in the staff interface to check in the item 4. Check the patron's fines 4.1. Click on the patron's name in the check in screen 4.2. Go to the Accounting tab on the left 4.3. In the search box just above the table, paste in the returned item's barcode --> Without the patch, there are two fines, one Fine (Accruing) and one Fine (Returned) for the same item at the same time --> With the patch, there is only one fine, Fine (Returned)
Why not remove: + # if the issue changed before the script got to it, then pass on it. + my $issue = Koha::Checkouts->find({ issue_id => $overdue->{issue_id} }); + next if ( ! $issue ); + + my $datedue = dt_from_string( $overdue->{date_due} ); + next unless $issue->is_overdue( $datedue ); And keep: - my $datedue = dt_from_string( $overdue->{date_due} ); - if ( DateTime->compare( $datedue, $today ) == 1 ) { - next; # not overdue - }
Created attachment 152987 [details] [review] Bug 18855: Don't create duplicate overdue fines if an issue is returned [ rebased 2023-07-03 to include suggestion from Marcel de Rooy and previous QA comments skip a database call by not instantiating the issue object twice ] This includes Jonathan's followup If cronjobs/fines.pl is running during circulation hours, then an issue may be considered for having it's overdue fine updated after the issue has been returned and it's fine status flipped from 'UNRETURNED' to 'RETURNED'. In this case UpdateFine will create a duplicate fine because it can't find the specific accountline for the (formerly) overdue issue. This changes cronjobs/fines.pl to double check the issue before updating the fine. If the issue has changed between starting the script and updating the fine, then the script will skip it. There is a small amount of time between the check and calling UpdateFine where the issue can be changed and this problem can reoccure. The chance of that happening is so small that it's probably fine to leave as is. It is also possible that the fine won't be updated because the issue was returned. In this case the fine payed by the patron will be lower, but that is better then the patron finding later that there is more to a fine they thought they had paid all of. Test plan (by Caroline): 0. Preliminary settings 0.1. finesMode system preference must be set to Calculate and charge 0.2. There must be a circulation rule that will charge fines (beware of bug 32271) 0.3. In Tools > Calendar, today must not be a holiday 1. Make a lot of overdue checkouts - I used the batch checkout feature, but if your system already has a lot of overdue checkouts, you can skip to step 2 1.1. Enable batch checkouts 1.1.1. Go to Administration > Global system preferences 1.1.2. Search for BatchCheckouts 1.1.3. Set BatchCheckouts to Allow 1.1.4. Select all categories in BatchCheckoutsValidCategories 1.1.5. Click "Save all Circulation preferences" 1.2. Get a list of barcodes 1.2.1. Go to Reports 1.2.2. Click "Create from SQL" 1.2.3. Give the report a name 1.2.4. For the SQL query, enter SELECT barcode FROM items WHERE onloan IS NULL LIMIT 60; 1.2.5. Click "Save report" 1.2.6. Click "Run report" 1.2.7. Click "Download" > "Tab separated text" 1.3. Go to a patron's file 1.3.1. Go to Patrons 1.3.2. Click on "Search" 1.3.3. Click on a patron's name 1.4. Do a batch checkout with a due date in the past 1.4.1. Click on the "Batch check out" tab on the left 1.4.2. In "Use a file", click "Choose file" 1.4.3. Choose the file downloaded from the report 1.4.4. In "Hard due date", choose a date in the past 1.4.5. Click "Check out" 1.4.6. Click "Checkout or renew" 2. Find the last issue in the database 2.1. In the database (or in reports), type the following query SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode, items.itemlost, items.replacementprice, items.biblionumber FROM issues LEFT JOIN items USING (itemnumber) WHERE date_due < NOW() \G; 2.2. Copy the barcode from the last entry 3. Set up so that you can run fines.pl and check in the item at the same time (or very close to the same time) 3.1. In Koha, click the "Check in" option in the search bar at the top of the page 3.2. Paste the barcode in the search bar BUT DO NOT PRESS ENTER OR THE ARROW RIGHT AWAY 3.3. In a terminal, enter the fines.pl command ./misc/cronjobs/fines.pl 3.4. Execute the command and immediately click on the arrow in the staff interface to check in the item 4. Check the patron's fines 4.1. Click on the patron's name in the check in screen 4.2. Go to the Accounting tab on the left 4.3. In the search box just above the table, paste in the returned item's barcode --> Without the patch, there are two fines, one Fine (Accruing) and one Fine (Returned) for the same item at the same time --> With the patch, there is only one fine, Fine (Returned)
Created attachment 152991 [details] [review] Bug 18855: Don't create duplicate overdue fines if an issue is returned This includes Jonathan's followup If cronjobs/fines.pl is running during circulation hours, then an issue may be considered for having it's overdue fine updated after the issue has been returned and it's fine status flipped from 'UNRETURNED' to 'RETURNED'. In this case UpdateFine will create a duplicate fine because it can't find the specific accountline for the (formerly) overdue issue. This changes cronjobs/fines.pl to double check the issue before updating the fine. If the issue has changed between starting the script and updating the fine, then the script will skip it. There is a small amount of time between the check and calling UpdateFine where the issue can be changed and this problem can reoccure. The chance of that happening is so small that it's probably fine to leave as is. It is also possible that the fine won't be updated because the issue was returned. In this case the fine payed by the patron will be lower, but that is better then the patron finding later that there is more to a fine they thought they had paid all of. Test plan (by Caroline): 0. Preliminary settings 0.1. finesMode system preference must be set to Calculate and charge 0.2. There must be a circulation rule that will charge fines (beware of bug 32271) 0.3. In Tools > Calendar, today must not be a holiday 1. Make a lot of overdue checkouts - I used the batch checkout feature, but if your system already has a lot of overdue checkouts, you can skip to step 2 1.1. Enable batch checkouts 1.1.1. Go to Administration > Global system preferences 1.1.2. Search for BatchCheckouts 1.1.3. Set BatchCheckouts to Allow 1.1.4. Select all categories in BatchCheckoutsValidCategories 1.1.5. Click "Save all Circulation preferences" 1.2. Get a list of barcodes 1.2.1. Go to Reports 1.2.2. Click "Create from SQL" 1.2.3. Give the report a name 1.2.4. For the SQL query, enter SELECT barcode FROM items WHERE onloan IS NULL LIMIT 60; 1.2.5. Click "Save report" 1.2.6. Click "Run report" 1.2.7. Click "Download" > "Tab separated text" 1.3. Go to a patron's file 1.3.1. Go to Patrons 1.3.2. Click on "Search" 1.3.3. Click on a patron's name 1.4. Do a batch checkout with a due date in the past 1.4.1. Click on the "Batch check out" tab on the left 1.4.2. In "Use a file", click "Choose file" 1.4.3. Choose the file downloaded from the report 1.4.4. In "Hard due date", choose a date in the past 1.4.5. Click "Check out" 1.4.6. Click "Checkout or renew" 2. Find the last issue in the database 2.1. In the database (or in reports), type the following query SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode, items.itemlost, items.replacementprice, items.biblionumber FROM issues LEFT JOIN items USING (itemnumber) WHERE date_due < NOW() \G; 2.2. Copy the barcode from the last entry 3. Set up so that you can run fines.pl and check in the item at the same time (or very close to the same time) 3.1. In Koha, click the "Check in" option in the search bar at the top of the page 3.2. Paste the barcode in the search bar BUT DO NOT PRESS ENTER OR THE ARROW RIGHT AWAY 3.3. In a terminal, enter the fines.pl command ./misc/cronjobs/fines.pl 3.4. Execute the command and immediately click on the arrow in the staff interface to check in the item 4. Check the patron's fines 4.1. Click on the patron's name in the check in screen 4.2. Go to the Accounting tab on the left 4.3. In the search box just above the table, paste in the returned item's barcode --> Without the patch, there are two fines, one Fine (Accruing) and one Fine (Returned) for the same item at the same time --> With the patch, there is only one fine, Fine (Returned) Signed-off-by: Sam Lau <samalau@gmail.com>
Created attachment 153012 [details] [review] Bug 18855: Don't create duplicate overdue fines if an issue is returned This includes Jonathan's followup If cronjobs/fines.pl is running during circulation hours, then an issue may be considered for having it's overdue fine updated after the issue has been returned and it's fine status flipped from 'UNRETURNED' to 'RETURNED'. In this case UpdateFine will create a duplicate fine because it can't find the specific accountline for the (formerly) overdue issue. This changes cronjobs/fines.pl to double check the issue before updating the fine. If the issue has changed between starting the script and updating the fine, then the script will skip it. There is a small amount of time between the check and calling UpdateFine where the issue can be changed and this problem can reoccure. The chance of that happening is so small that it's probably fine to leave as is. It is also possible that the fine won't be updated because the issue was returned. In this case the fine payed by the patron will be lower, but that is better then the patron finding later that there is more to a fine they thought they had paid all of. Test plan (by Caroline): 0. Preliminary settings 0.1. finesMode system preference must be set to Calculate and charge 0.2. There must be a circulation rule that will charge fines (beware of bug 32271) 0.3. In Tools > Calendar, today must not be a holiday 1. Make a lot of overdue checkouts - I used the batch checkout feature, but if your system already has a lot of overdue checkouts, you can skip to step 2 1.1. Enable batch checkouts 1.1.1. Go to Administration > Global system preferences 1.1.2. Search for BatchCheckouts 1.1.3. Set BatchCheckouts to Allow 1.1.4. Select all categories in BatchCheckoutsValidCategories 1.1.5. Click "Save all Circulation preferences" 1.2. Get a list of barcodes 1.2.1. Go to Reports 1.2.2. Click "Create from SQL" 1.2.3. Give the report a name 1.2.4. For the SQL query, enter SELECT barcode FROM items WHERE onloan IS NULL LIMIT 60; 1.2.5. Click "Save report" 1.2.6. Click "Run report" 1.2.7. Click "Download" > "Tab separated text" 1.3. Go to a patron's file 1.3.1. Go to Patrons 1.3.2. Click on "Search" 1.3.3. Click on a patron's name 1.4. Do a batch checkout with a due date in the past 1.4.1. Click on the "Batch check out" tab on the left 1.4.2. In "Use a file", click "Choose file" 1.4.3. Choose the file downloaded from the report 1.4.4. In "Hard due date", choose a date in the past 1.4.5. Click "Check out" 1.4.6. Click "Checkout or renew" 2. Find the last issue in the database 2.1. In the database (or in reports), type the following query SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode, items.itemlost, items.replacementprice, items.biblionumber FROM issues LEFT JOIN items USING (itemnumber) WHERE date_due < NOW() \G; 2.2. Copy the barcode from the last entry 3. Set up so that you can run fines.pl and check in the item at the same time (or very close to the same time) 3.1. In Koha, click the "Check in" option in the search bar at the top of the page 3.2. Paste the barcode in the search bar BUT DO NOT PRESS ENTER OR THE ARROW RIGHT AWAY 3.3. In a terminal, enter the fines.pl command ./misc/cronjobs/fines.pl 3.4. Execute the command and immediately click on the arrow in the staff interface to check in the item 4. Check the patron's fines 4.1. Click on the patron's name in the check in screen 4.2. Go to the Accounting tab on the left 4.3. In the search box just above the table, paste in the returned item's barcode --> Without the patch, there are two fines, one Fine (Accruing) and one Fine (Returned) for the same item at the same time --> With the patch, there is only one fine, Fine (Returned) Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 23.11. Nice work everyone, thanks!
Thanks for all the hard work! Pushed to 23.05.x for the next release
Nice work everyone! Pushed to oldstable for 22.11.x