https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 --- Comment #1 from Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> --- Created attachment 113939 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113939&action=edit Bug 27080 - Add --statute-barred option to cleanup_database.pl to purge all fees where the item has been returned and the fee is older than the given number of days. According to Swedish law, fees become statute-barred after 3 years. With this option, we can use cleanup_database.pl to purge those from accountlines. NOTE: This commit also fixes indentations. You can use the -w option with git to ignore space differences. To test: 1) You will need a record in the accountlines table WHERE status = 'REMOVED' AND amountoutstanding > 0. 2) Run ./misc/cronjobs/cleanup_database.pl --statute-barred DAYS Where DAYS is 1 or greater. This will delete records from accountlines WHERE date is DAYS day(s) or older. 3) Confirm that records have been deleted. Sponsored-by: Lunds Universitetsbibliotek -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Change sponsored?|--- |Sponsored -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@helsinki.fi Status|Needs Signoff |Failed QA --- Comment #3 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Hi, I spotted few things to improve: 1) Instead of doing RAW SQL use Koha Koha::Account::Line objects and ->delete. 2) Move code somewhere else than C4: https://wiki.koha-community.org/wiki/Coding_Guidelines#DEPR3:_C4_deprecated_... – unfortunately I don't have good suggestions as to what could be better place right now. 3) Split the formatting changes of the patch to its own patch in order to ease reviewing the changes and give us more confidence that the whitespace change won't accidentally do any logic changes, also it eases reverting and usage of git in general. 4) Add unit test for the new function I must say I don't know what those RETURNED accountlines are about so regarding whether this feature is something we want or if it should be implemented in some other way I cannot say, maybe I can research this topic later. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 --- Comment #5 from Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> --- Look at the patch and see that I have added the functionality for purge_statute_barred_fees to be exactly as purge_zero_balance_fees that is already there. If the changes you suggest need to be done, they need to be done on a deeper level than this patch. That would increase the scope of this. I have adapted this to what is available, in precise alignment with what is already there. If C4 is deprecated, then there needs to be an alternative. Again, it increases the scope. The whitespace changes are strictly in the help section, to align all text that is already there, in alignment with what is already there. Also, I copy the Patch title from the top of this page, which is: Bug 27080 - Add --statute-barred option to cleanup_database.pl to purge all fees where the item has been returned and the fee is older than the given number of days. This patch introduces functionality which is required by Swedish law, so it will be applicable for all libraries using Koha in Sweden. The RETURNED is about items that have been returned. In Sweden, by law, fees are statute-barred after 3 years for books which have been returned. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 --- Comment #7 from Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> --- (In reply to Joonas Kylmälä from comment #6)

(In reply to Timothy Alexis Vass from comment #5)

...

Look at the patch and see that I have added the functionality for purge_statute_barred_fees to be exactly as purge_zero_balance_fees that is already there.

If the changes you suggest need to be done, they need to be done on a deeper level than this patch. That would increase the scope of this.

I have adapted this to what is available, in precise alignment with what is already there. If C4 is deprecated, then there needs to be an alternative. Again, it increases the scope.

Yes, a bit more work, please look first if there is already a module for this under Koha/ directory, if not create a new module.

So the scope becomes rewriting the entire C4/Accounts.pm module as a new module? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 --- Comment #9 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Timothy Alexis Vass from comment #7)

So the scope becomes rewriting the entire C4/Accounts.pm module as a new module?

No, just focus on the parts that matter for this bug. It would be welcome, but not necessary, to refactor the already existing related functions. In this case it would be only purge_zero_balance_fees() and that would need to be moved to some module in Koha namespace. Then the new function purge_statute_barred_fees() could be added in later patch to go to this same module. Or you can also go the easier route, which is to add only purge_statute_barred_fees() to a module in Koha namespace. Then in the future we can tidy up C4/Accounts.pm a bit by moving purge_zero_balance_fees() also to the module in Koha namespace. Let me know if you get stuck somewhere or need clarifications on the requested improvements. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #11 from David Cook <dcook@prosentient.com.au> --- Can you elaborate on what "statute-barred" means? At a glance, it doesn't mean anything obvious in English. At a stretch, I'd guess that it means something isn't allowed by law (ie statute), but it's not clear what's not allowed. There might not be a problem with the wording per se, but we might want to add some more description about the meaning of the wording, since it's not clear. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 --- Comment #13 from David Cook <dcook@prosentient.com.au> --- (In reply to Katrin Fischer from comment #12)

I was wondering about the term too, but the translations to German make sense and it appears to be an existing English term :) It's a legal term meaning kind of "expired". There has so much time passed, that the fee has "expired". In German: verjährt.

Thanks for the clarification, Katrin. It does appear to be an English legal term, and that it typically refers to debts. I love learning new things. Maybe --statue-barred-fees or --statute-barred-debts might be a little more explicit? Before Katrin explained the term to me, I probably could've intuited the meaning of "statue-barred-fees" or "statute-barred-debts", but I was confused what "thing" that "statute-barred" would refer to without my newfound knowledge. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Add --statute-barred option |Add --statute-barred-fees |to cleanup_database.pl to |option to |purge all fees where the |cleanup_database.pl to |item has been returned and |purge all fees where the |the fee is older than the |item has been returned and |given number of days. |the fee is older than the | |given number of days. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 --- Comment #16 from Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> --- (In reply to David Cook from comment #15)

(In reply to Timothy Alexis Vass from comment #14)

...

I will make it --statue-barred-fees. The fees are literally statute-barred.

Thanks for humouring me, Timothy. I know I am being pedantic, but I really do think it will be helpful.

I agree David. It definitely is more obvious and that is definitely important. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 --- Comment #18 from Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> --- (In reply to Timothy Alexis Vass from comment #17)

NOTE: This commit also fixes indentations. You can use the -w option with git to ignore space differences.

I removed the whitespace changes. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #19 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Hi, - can you explain why only RETURNED fees? It would probably good to have this explanation in the help documentation too. Some other fees that might need to be removed also could be RENEWED and LOST? Please see installer/data/mysql/mandatory/account_debit_types.sql, although it doesn't contain every type of fee Koha generates currently. - Depending on your answer to the above question, would it make sense to make the name of this option something more generic, like --unpaid-fees? - The commit title is still too long, see comment #4. - The code is now not in a function, i.e. cannot be tested. Would something like Koha::Account::Lines::statute_barred (depending on the scope of this function maybe a different name) sound good to you? Then you can call ->delete or ->count on those and the code will look super cool in the script, wouldn't it? :) - The whitespace changes would still be nice to have, just in a separate patch. Tip: add it as last in the series so any rebase conflicts will be easier to resolve when you only need to redo the whitespace patch and not also the feature patch. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114322|0 |1 is obsolete| | --- Comment #22 from Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> --- Created attachment 114323 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114323&action=edit Bug 27080: Add --statute-barred-fees option to cleanup_database.pl. According to Swedish law, fees become statute-barred after 3 years if the material has been returned. With this option, we can use cleanup_database.pl to purge those from accountlines. To test: 1) You will need at least one record in the accountlines table WHERE status = 'RETURNED' AND amountoutstanding > 0. 2) Run ./misc/cronjobs/cleanup_database.pl --statute-barred-fees DAYS Where DAYS is 1 or greater. This will delete records from accountlines WHERE date is DAYS day(s) or older. 3) Confirm that records have been deleted. Sponsored-by: Lunds Universitetsbibliotek -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 --- Comment #23 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Timothy Alexis Vass from comment #20)

(In reply to Joonas Kylmälä from comment #19)

In the other situations "Renewed" and "Lost", it is different. Then the debt has been handed over to "Kronofogdemyndigheten", which is the Swedish Enforcement Agency.

What about RESERVE_EXPIRED fees? In Finland those could become statute barred depending on whether the library reminds the customer about the fees: <https://www.kirjastot.fi/forum/sakot-vanhenevat-kolmessa-vuodessa-135cf5>. I think also RENEWED fees could become statute barred in Finland in the case where the item was just lost for 1 day and patron went to library then immediately and wanted to renew the book, and has been renewing it ever since so the status is still RENEWED 3 years later.

...

- Depending on your answer to the above question, would it make sense to make the name of this option something more generic, like --unpaid-fees?

The only thing that makes sense is to call it --statute-barred-fees because that is exactly what it is. They are not --unpaid-fees, they are not --expired-fees, they are not --old-fees, they are fees that have been barred according to ruling statutes.

The problem with using statute barred term is that the legal definition and usage of this option might vary by the region/country and library (if they have different processes in place that prevent some things getting statute barred like in the Finnish example above). How will the user know which country's definition is being used? It is not good idea to do one statute-barred for each country/region because it will make the program complex by having to adapt to each case.

...

- The code is now not in a function, i.e. cannot be tested. Would something like Koha::Account::Lines::statute_barred (depending on the scope of this function maybe a different name) sound good to you? Then you can call ->delete or ->count on those and the code will look super cool in the script, wouldn't it? :)

Easy to maintain, simple code is preferred. I would suggest removing all this insane complexity and do database maintenance in the database, such as: CREATE EVENT cleanup_statute_barred_fees ON SCHEDULE EVERY 1 DAY DO DELETE FROM accountlines WHERE date < date_sub(curdate(), INTERVAL 1095 DAY) AND status = 'RETURNED' AND amountoutstanding > 0

Quite seriously, if you want to make it redundnantly complex because you find that interesting, you are free to make it happen.

My goal is the same, easy to maintain and simple code. Easy to maintain for me means we have tests, then any modifications to the search method can be done without testing manually all the places where it is being used. Maybe it is a bit overkill, what do others think? Other benefits of using a function is that now we can document the statute debarred fines in a standard way in the perldoc that goes along with it. As for simple, I find Koha::Account::Lines::statute_barred->delete simpler/easier to understand than the call the search with multiple params, the same goes with the statute_barred function definition: I will find it easier to understand the reasons behind RETURNED, amountoutstanding > 0 when these are documented right next to the lines of code in question either using variables or the perldoc. When this is in a function we can also reuse this code in other places in the future. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 --- Comment #25 from Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> --- You do whatever you like, we drop any further efforts on this. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #26 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- OK, it's time for someone to put a pragmatic hat on here.. and it appears that person is me today. Whilst I agree with allot of what Joonas is saying, and he's really only trying to help by offering constructive criticism, I think this patch is reasonable and with the polishing that has already taken place I think it's reasonable to expect it to progress into Koha and as such I will take a look at testing myself with a view to signing off. Explanation of Joonas's views: Koha is a massive project with countless users worldwide and huge numbers of developers contributing to it. As a general rule, if you don't have a unit test covering a change you are making you cannot expect that feature to not get broken by future development. (This is NOT koha specific, a massive number of projects out there require a unit test as part of every/any submission). Joonas was merely making suggestions on how best to submit the code allowing for such unit tests. Yes, in an ideal world, C4:: would be gone and Koha:: would be fully unit tested and have fully replaced it with cleaner more modern code. Unfortunately, Koha is a victim of it's own success, having grown organically. We are lacking tests in many places, but things are always improving.. and it takes time to migrate everything (and most of that time is unfunded!) Koha is also a huge international project, so the terms are important and so is the flexibility of features... I've been caught by that one a number of times myself, where a feature is obvious to me, but I quickly learn it's not at all clear to my counterparts elsewhere or that for it to work for them it needs a little tweaking. I tend to try and code with that in mind these days and make the foundations as flexible as I can so others can build atop them. For example with this feature I personally would love to see it all controlled by system preferences.. i.e. the list of what constitutes a fee that may be statute-barred, and the timeings.. then simply passing a binary flag to the script would be required to enable it. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27080 --- Comment #30 from Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> --- We have discussed the statute-barred fines further in our team. To clean up statute-barred fees would require the system to know how such are defined. This would need to be defined specifically for each library. For our purpose, these would the answers specific to Lund University Library:

1) Do you not want to limit by "debit_type_code => 'OVERDUE'" to ensure you only catch overdue fines? We only want those with debit_type_code => 'OVERDUE' 2) Limiting to "status => 'RETURNED'" will mean you miss accruing fines, fines fixed at the point of the lose of the item being reported and others We only want those with status => 'RETURNED' 3) Is there any difference between a partially paid and completely unpaid fee in the legal context that needs taking care of? They all need to be deleted. Hmm.. should this really be a delete rather than a recorded write-off... They need to be deleted. You may want to also take care of offsets here. The offsets also need to be deleted.

- It could be that most libraries would prefer setting the balance to 0 rather than deleting them. - It could be that a few libraries would want different status and debit_type_code criteria. - There could be other requirements which are relevant to other libraries than ours. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.