Bug 16223 - Automatically remove any borrower debarments after a payment
Summary: Automatically remove any borrower debarments after a payment
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Emmi Takkinen
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-07 11:38 UTC by Lari Taskula
Modified: 2022-04-03 13:20 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16223: Automatically remove any borrower debarments after a payment (12.00 KB, patch)
2016-04-08 12:43 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16223: Enable regex match for rules of debarment removals (4.18 KB, patch)
2016-04-08 13:49 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16223: Automatically remove any borrower debarments after a payment (10.70 KB, patch)
2020-02-27 11:24 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 16223: Enable regex match for rules of debarment (4.14 KB, patch)
2020-02-27 11:25 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 16223: Automatically remove any borrower debarments after a payment (10.80 KB, patch)
2020-05-06 06:52 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 16223: Enable regex match for rules of debarment removals (4.16 KB, patch)
2020-05-06 06:53 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 16223: Automatically remove any borrower debarments after a payment (11.02 KB, patch)
2020-12-09 07:53 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 16223: Enable regex match for rules of debarment removals (4.26 KB, patch)
2020-12-09 07:54 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 26940: Take empty strings into account (1.29 KB, text/plain)
2021-03-19 12:54 UTC, ByWater Sandboxes
Details
Bug 16223: Enable regex match for rules of debarment removals (4.29 KB, patch)
2021-03-22 08:00 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 16223: Automatically remove any borrower debarments after a payment (11.10 KB, patch)
2021-03-22 08:03 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 16223: Enable regex match for rules of debarment removals (4.35 KB, patch)
2021-03-22 08:03 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 16223: Automatically remove any borrower debarments after a payment (11.21 KB, patch)
2021-07-12 05:56 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 16223: Enable regex match for rules of debarment removals (4.35 KB, patch)
2021-07-12 05:57 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 16223: Fix QA issues (4.31 KB, patch)
2021-11-03 13:27 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 16223: Call DelDebarmentsAfterPayment from Koha::Account::Line::apply (4.59 KB, patch)
2021-11-25 12:36 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 16223: Automatically remove any borrower debarments after a payment (11.03 KB, patch)
2022-03-28 06:16 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 16223: Enable regex match for rules of debarment removals (4.35 KB, patch)
2022-03-28 06:17 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 16223: Fix QA issues (4.31 KB, patch)
2022-03-28 06:17 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 16223: Call DelDebarmentsAfterPayment from Koha::Account::Line::apply (4.56 KB, patch)
2022-03-28 06:18 UTC, Emmi Takkinen
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2016-04-07 11:38:15 UTC
Some libraries debar Borrowers at the end of the year for having unpaid fines, like in Bug 15157. Currently librarians have to manually remove this type of debarments after Borrower has paid his/her fines. Add a system preference to define debarments that should be automatically removed after a payment is made, and add functionality to actually remove the defined debarments from Borrower. Also let libraries to define the amount of outstanding fines after payment after which the debarment will be removed.
Comment 1 Lari Taskula 2016-04-08 12:43:45 UTC
Created attachment 50078 [details] [review]
Bug 16223: Automatically remove any borrower debarments after a payment

Some libraries debar Patrons at the end of the year for having unpaid fines,
like in Bug 15157. Currently librarians have to manually remove this type of
debarments after Patron has paid his/her fines. Add a system preference to
define debarments that should be automatically removed after a payment is made,
and add functionality to actually remove the defined debarments from Patron.
Also let libraries to define the amount of outstanding fines after payment
after which the debarment will be removed.

This patch introduces a system preference DebarmentsToLiftAfterPayment, which
allows libraries to define rules for removing debarments after paying fines.
The system preference uses YAML and is defined as follows:

Debarment with this comments will be removed:
  outstanding: 5

Which means that if a Patron has a debarment "Debarment with this comment will
be removed", and he pays his fines and charges until his outstanding fees are
equal or less than 5.00, this debarment will be lifted. The parameter outstanding
is optional - if not given, the debarment will be lifted until Patron has paid
all of his outstanding fees (in other words, equal to "outstanding: 0").

To test:
1. Set a debarment to a Patron
2. Set a fine for Patron
3. Define rule(s) for removing debarment(s) in system preference
   DebarmentsToLiftAfterPayment
4. Pay the fine you set in step 2
5. Note that the debarment is now lifted
Comment 2 Lari Taskula 2016-04-08 13:49:40 UTC
Created attachment 50084 [details] [review]
Bug 16223: Enable regex match for rules of debarment removals

This patch adds possibility to use regex to match debarments with dynamic comment.
Comment 3 Emmi Takkinen 2019-10-22 11:13:58 UTC
Tried to test this, got error:
error: short SHA1 bb92094 is ambiguous
hint: The candidates are:
hint:   bb92094b8c blob
hint:   bb92094f25 blob
error: sha1 information is lacking or useless (t/db_dependent/Patron/Borrower_Debarments.t).
error: could not build fake ancestor
Comment 4 Lari Taskula 2019-10-22 11:24:06 UTC
(In reply to Emmi Takkinen from comment #3)
> Tried to test this, got error:
> error: short SHA1 bb92094 is ambiguous
> hint: The candidates are:
> hint:   bb92094b8c blob
> hint:   bb92094f25 blob
> error: sha1 information is lacking or useless
> (t/db_dependent/Patron/Borrower_Debarments.t).
> error: could not build fake ancestor

Do you see this after your error:

> Patch failed at 0001 Bug 16223: Automatically remove any borrower debarments after a payment
> Use 'git am --show-current-patch' to see the failed patch
> When you have resolved this problem run "git bz apply --continue".
> If you would prefer to skip this patch, instead run "git bz apply --skip".
> To restore the original branch and stop patching run "git bz apply --abort".
> Patch left in /tmp/Bug-16223-Automatically-remove-any-borrower-debarm-EzxzUo.patch

Git fails to merge changes and is unable to continue. You need to continue manually. 

You can do the following:
1. git apply --reject /tmp/Bug-16223-Automatically-remove-any-borrower-debarm-EzxzUo.patch
2. It generates .rej file for every original file that failed, e.g. C4/Accounts.pm.rej
3. In this .rej file you can see the changes that are not yet applied
4. Merge the changes manually (by copypasting the lines to appropriate locations, if they are still valid)
Comment 5 Katrin Fischer 2019-11-24 12:48:54 UTC
Should this be "patch doesn't apply"?
Comment 6 Emmi Takkinen 2020-02-27 11:24:14 UTC
Created attachment 99697 [details] [review]
Bug 16223: Automatically remove any borrower debarments  after a payment

Rebased patch.
Comment 7 Emmi Takkinen 2020-02-27 11:25:03 UTC
Created attachment 99698 [details] [review]
Bug 16223: Enable regex match for rules of debarment

Rebased patch.
Comment 8 Emmi Takkinen 2020-05-06 06:52:42 UTC
Created attachment 104399 [details] [review]
Bug 16223: Automatically remove any borrower debarments  after a payment

Rebased patch.
Comment 9 Emmi Takkinen 2020-05-06 06:53:28 UTC
Created attachment 104400 [details] [review]
Bug 16223: Enable regex match for rules of debarment removals

Rebased patch.
Comment 10 Emmi Takkinen 2020-12-09 07:53:15 UTC
Created attachment 114272 [details] [review]
Bug 16223: Automatically remove any borrower debarments after a payment

Some libraries debar Patrons at the end of the year for having unpaid fines,
like in Bug 15157. Currently librarians have to manually remove this type of
debarments after Patron has paid his/her fines. Add a system preference to
define debarments that should be automatically removed after a payment is made,
and add functionality to actually remove the defined debarments from Patron.
Also let libraries to define the amount of outstanding fines after payment
after which the debarment will be removed.

This patch introduces a system preference DebarmentsToLiftAfterPayment, which
allows libraries to define rules for removing debarments after paying fines.
The system preference uses YAML and is defined as follows:

Debarment with this comments will be removed:
  outstanding: 5

Which means that if a Patron has a debarment "Debarment with this comment will
be removed", and he pays his fines and charges until his outstanding fees are
equal or less than 5.00, this debarment will be lifted. The parameter outstanding
is optional - if not given, the debarment will be lifted until Patron has paid
all of his outstanding fees (in other words, equal to "outstanding: 0").

To test:
1. Set a debarment to a Patron
2. Set a fine for Patron
3. Define rule(s) for removing debarment(s) in system preference
   DebarmentsToLiftAfterPayment
4. Pay the fine you set in step 2
5. Note that the debarment is now lifted

Also prove t/db_dependent/Patron/Borrower_Debarments.t

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 11 Emmi Takkinen 2020-12-09 07:54:01 UTC
Created attachment 114273 [details] [review]
Bug 16223: Enable regex match for rules of debarment removals

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 12 Emmi Takkinen 2020-12-09 07:55:18 UTC
Rebased patches and removed dependency to bug 14698.
Comment 13 ByWater Sandboxes 2021-03-19 12:54:25 UTC
Created attachment 118526 [details]
Bug 26940: Take empty strings into account

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>
Comment 14 Katrin Fischer 2021-03-21 13:45:51 UTC
Switching this one to Signed off as well noticing the sandbox patches.
Comment 15 Emmi Takkinen 2021-03-22 06:41:17 UTC
Comment on attachment 118526 [details]
Bug 26940: Take empty strings into account

It seems one patch from bug 26940 has somehow slipped here. Actual patches still need sign off. Obsoleted patch and changing status back to Needs sign off. Katariina could you sign off again?
Comment 16 ByWater Sandboxes 2021-03-22 08:00:24 UTC
Created attachment 118572 [details] [review]
Bug 16223: Enable regex match for rules of debarment removals

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>
Comment 17 ByWater Sandboxes 2021-03-22 08:03:07 UTC
Created attachment 118574 [details] [review]
Bug 16223: Automatically remove any borrower debarments after a payment

Some libraries debar Patrons at the end of the year for having unpaid fines,
like in Bug 15157. Currently librarians have to manually remove this type of
debarments after Patron has paid his/her fines. Add a system preference to
define debarments that should be automatically removed after a payment is made,
and add functionality to actually remove the defined debarments from Patron.
Also let libraries to define the amount of outstanding fines after payment
after which the debarment will be removed.

This patch introduces a system preference DebarmentsToLiftAfterPayment, which
allows libraries to define rules for removing debarments after paying fines.
The system preference uses YAML and is defined as follows:

Debarment with this comments will be removed:
  outstanding: 5

Which means that if a Patron has a debarment "Debarment with this comment will
be removed", and he pays his fines and charges until his outstanding fees are
equal or less than 5.00, this debarment will be lifted. The parameter outstanding
is optional - if not given, the debarment will be lifted until Patron has paid
all of his outstanding fees (in other words, equal to "outstanding: 0").

To test:
1. Set a debarment to a Patron
2. Set a fine for Patron
3. Define rule(s) for removing debarment(s) in system preference
   DebarmentsToLiftAfterPayment
4. Pay the fine you set in step 2
5. Note that the debarment is now lifted

Also prove t/db_dependent/Patron/Borrower_Debarments.t

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>
Comment 18 ByWater Sandboxes 2021-03-22 08:03:09 UTC
Created attachment 118575 [details] [review]
Bug 16223: Enable regex match for rules of debarment removals

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>
Comment 19 Emmi Takkinen 2021-03-22 10:42:40 UTC
Setting back to signed offed.
Comment 20 Emmi Takkinen 2021-07-12 05:56:31 UTC
Created attachment 122768 [details] [review]
Bug 16223: Automatically remove any borrower debarments after a payment

Some libraries debar Patrons at the end of the year for having unpaid fines,
like in Bug 15157. Currently librarians have to manually remove this type of
debarments after Patron has paid his/her fines. Add a system preference to
define debarments that should be automatically removed after a payment is made,
and add functionality to actually remove the defined debarments from Patron.
Also let libraries to define the amount of outstanding fines after payment
after which the debarment will be removed.

This patch introduces a system preference DebarmentsToLiftAfterPayment, which
allows libraries to define rules for removing debarments after paying fines.
The system preference uses YAML and is defined as follows:

Debarment with this comments will be removed:
  outstanding: 5

Which means that if a Patron has a debarment "Debarment with this comment will
be removed", and he pays his fines and charges until his outstanding fees are
equal or less than 5.00, this debarment will be lifted. The parameter outstanding
is optional - if not given, the debarment will be lifted until Patron has paid
all of his outstanding fees (in other words, equal to "outstanding: 0").

To test:
1. Set a debarment to a Patron
2. Set a fine for Patron
3. Define rule(s) for removing debarment(s) in system preference
   DebarmentsToLiftAfterPayment
4. Pay the fine you set in step 2
5. Note that the debarment is now lifted

Also prove t/db_dependent/Patron/Borrower_Debarments.t

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>
Comment 21 Emmi Takkinen 2021-07-12 05:57:04 UTC
Created attachment 122769 [details] [review]
Bug 16223: Enable regex match for rules of debarment removals

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>
Comment 22 Emmi Takkinen 2021-07-20 12:43:54 UTC
Oops, forgot to set this back to "Needs Signoff"...
Comment 23 Emmi Takkinen 2021-10-19 05:32:18 UTC
This is actually already signed off.
Comment 24 Katrin Fischer 2021-11-01 13:04:36 UTC
Hi Emmi,

my apply script shows a bit of ending whitespace and the QA test tools show an error I haven't seen before:

1) QA tools error

 FAIL	Koha/Patron/Debarments.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  pod coverage
   OK	  spelling
   FAIL	  valid
		Inconsistent hierarchy during C3 merge of class 'Koha::Patron::Debarments':
			current merge results [
				Koha::Patron::Debarments,
			]
			merging failed on 'Exporter' 

Could you please check this out?

I've also done a first code review:

2) Please always use INSERT IGNORE when adding system preferences. This helps to keep them idempotent.

+    $dbh->do("INSERT INTO systempreferences (variable, value, options, explanation, type) VALUES ('DebarmentsToLiftAfterPayment', '', '', 'Lift these debarments after Borrower has paid his/her fees', 'textarea')");

3) Gender neutral code comments etc.

In English, you can use they/them/their to make your text gender neutral (wish it was that easy in German...)

Like here:
Lift these debarments after Borrower has paid his/her fees

You can say:
Lift these debarments after patron has paid their charges.

See: https://wiki.koha-community.org/wiki/Terminology
- prefer patron instead of borrower
- prefer charges instead of fines/fees (not added yet, but changed in GUI)

Similar for the written out pref text in the .yml file.

4) Unit tests pass :)
Comment 25 Emmi Takkinen 2021-11-02 09:38:59 UTC
Thanks Katrin for feedback! Points 2 and 3 are easy fix, but I don't really know what that QA tool error means or how to fix it (seems like similar error occurred in bug 23271, something about circular dependency). I'll look more into it, if someone knows how to fix this feel free to do so.
Comment 26 Katrin Fischer 2021-11-02 13:38:43 UTC
Adding Joubu und Tomas, maybe one of them can help with this one:

 FAIL	Koha/Patron/Debarments.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  pod coverage
   OK	  spelling
   FAIL	  valid
		Inconsistent hierarchy during C3 merge of class 'Koha::Patron::Debarments':
			current merge results [
				Koha::Patron::Debarments,
			]
			merging failed on 'Exporter'
Comment 27 Jonathan Druart 2021-11-02 16:20:20 UTC
(In reply to Katrin Fischer from comment #26)
> Adding Joubu und Tomas, maybe one of them can help with this one:
> 
>  FAIL	Koha/Patron/Debarments.pm
>    OK	  critic
>    OK	  forbidden patterns
>    OK	  git manipulation
>    OK	  pod
>    OK	  pod coverage
>    OK	  spelling
>    FAIL	  valid
> 		Inconsistent hierarchy during C3 merge of class 'Koha::Patron::Debarments':
> 			current merge results [
> 				Koha::Patron::Debarments,
> 			]
> 			merging failed on 'Exporter'

Ignore it if it does not break the UI or tests. It should be fixed by bug 29395.
Comment 28 Emmi Takkinen 2021-11-03 13:27:28 UTC
Created attachment 127252 [details] [review]
Bug 16223: Fix QA issues

To test repeat original test plan and
prove t/db_dependent/Patron/Borrower_Debarments.t

Sponsored-by: Koha-Suomi Oy
Comment 29 Martin Renvoize 2021-11-03 16:18:10 UTC
Sorry guys.. this is a fail for me.

I think you need to move the function call from Koha::Account::pay to Koha::Account::Line::apply.. that way you'll catch any and all calls to apply payments (where you have it now, you'll miss any calls directly to apply, or calls to payin_amount.. and you'll miss cases where there's any additional outstanding credit to be applied from the payment).

Also, when autorenewal of items upon payment was added (a similar feature in my opinion).. we required that the user was given feedback of the change.. I'd love to see that here too.

I realise this is an old development being worked through.. but at least the first issue needs resolving prior to push in my opinion.. though I'd love to see the second dealt with.. I'd personally be OK with that being in a follow-up bug.

Failed QA
Comment 30 Emmi Takkinen 2021-11-24 11:22:05 UTC
(In reply to Martin Renvoize from comment #29)
> Also, when autorenewal of items upon payment was added (a similar feature in
> my opinion).. we required that the user was given feedback of the change..
> I'd love to see that here too.
> 

I tried to find which feature you mean with this but alas, I failed :D Could you point me the bug number of this so I could take a look?
Comment 31 Martin Renvoize 2021-11-25 05:52:16 UTC
It's bug 23051
Comment 32 Emmi Takkinen 2021-11-25 12:36:11 UTC
Created attachment 128005 [details] [review]
Bug 16223: Call DelDebarmentsAfterPayment from Koha::Account::Line::apply

Calling function from Koha::Account::pay missed several payment cases.
Moving it to Koha::Account::Line::apply should cover all payments. Also
reworked tests no just to test DelDebarmentsAfterPayment but to test
if debartments are lifted after pay.

To test repeat test plan from previous patches.
Also prove t/db_dependent/Patron/Borrower_Debartments.t

Sponsored-by: Koha-Suomi Oy
Comment 33 Katrin Fischer 2022-03-26 23:56:46 UTC
Hi Emmi, patches no longer apply, could you please rebase?

The script notes some trailing whitespace also, maybe worth checking. 

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 16223: Automatically remove any borrower debarments after a payment
.git/rebase-apply/patch:28: trailing whitespace.
    
.git/rebase-apply/patch:160: trailing whitespace.
     - 
warning: 2 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	Koha/Account.pm
M	Koha/Patron/Debarments.pm
M	installer/data/mysql/mandatory/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
M	t/db_dependent/Patron/Borrower_Debarments.t
.git/rebase-apply/patch:28: trailing whitespace.
    
.git/rebase-apply/patch:160: trailing whitespace.
     - 
warning: 2 lines applied after fixing whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Patron/Borrower_Debarments.t
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
Auto-merging installer/data/mysql/mandatory/sysprefs.sql
Auto-merging Koha/Patron/Debarments.pm
CONFLICT (content): Merge conflict in Koha/Patron/Debarments.pm
Auto-merging Koha/Account.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 16223: Automatically remove any borrower debarments after a payment
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-16223-Automatically-remove-any-borrower-debarm-iuWbtz.p
Comment 34 Emmi Takkinen 2022-03-28 06:16:44 UTC
Created attachment 132284 [details] [review]
Bug 16223: Automatically remove any borrower debarments after a payment

Some libraries debar Patrons at the end of the year for having unpaid fines,
like in Bug 15157. Currently librarians have to manually remove this type of
debarments after Patron has paid his/her fines. Add a system preference to
define debarments that should be automatically removed after a payment is made,
and add functionality to actually remove the defined debarments from Patron.
Also let libraries to define the amount of outstanding fines after payment
after which the debarment will be removed.

This patch introduces a system preference DebarmentsToLiftAfterPayment, which
allows libraries to define rules for removing debarments after paying fines.
The system preference uses YAML and is defined as follows:

Debarment with this comments will be removed:
  outstanding: 5

Which means that if a Patron has a debarment "Debarment with this comment will
be removed", and he pays his fines and charges until his outstanding fees are
equal or less than 5.00, this debarment will be lifted. The parameter outstanding
is optional - if not given, the debarment will be lifted until Patron has paid
all of his outstanding fees (in other words, equal to "outstanding: 0").

To test:
1. Set a debarment to a Patron
2. Set a fine for Patron
3. Define rule(s) for removing debarment(s) in system preference
   DebarmentsToLiftAfterPayment
4. Pay the fine you set in step 2
5. Note that the debarment is now lifted

Also prove t/db_dependent/Patron/Borrower_Debarments.t

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>
Comment 35 Emmi Takkinen 2022-03-28 06:17:16 UTC
Created attachment 132285 [details] [review]
Bug 16223: Enable regex match for rules of debarment removals

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>

Signed-off-by: Katariina Hanhisalo <katariina.hanhisalo@xamk.fi>
Comment 36 Emmi Takkinen 2022-03-28 06:17:41 UTC
Created attachment 132286 [details] [review]
Bug 16223: Fix QA issues

To test repeat original test plan and
prove t/db_dependent/Patron/Borrower_Debarments.t

Sponsored-by: Koha-Suomi Oy
Comment 37 Emmi Takkinen 2022-03-28 06:18:10 UTC
Created attachment 132287 [details] [review]
Bug 16223: Call DelDebarmentsAfterPayment from  Koha::Account::Line::apply

Calling function from Koha::Account::pay missed several payment cases.
Moving it to Koha::Account::Line::apply should cover all payments. Also
reworked tests no just to test DelDebarmentsAfterPayment but to test
if debartments are lifted after pay.

To test repeat test plan from previous patches.
Also prove t/db_dependent/Patron/Borrower_Debartments.t

Sponsored-by: Koha-Suomi Oy
Comment 38 Emmi Takkinen 2022-03-28 06:20:20 UTC
Rebased patches. Couldn't find trailing whitespaces after applying patches, but found a extra =cut on last patch.
Comment 40 Katrin Fischer 2022-04-03 13:20:05 UTC
Hi Emmi, 

thanks for the rebase, starting to dig a little deeper into this now:

1) DelDebarmentsAfterPayment 

According to coding guidelines the methods in Koha namespace should always be snake case.

2) Terminology

debarment --> restriction

In general, and that possibly applies to above as well, we should use the term "restrictions". See: bug 5268 for an explanation.

borrower --> patron

I know you can't change all spots as the table is called borrowers_debarments, but I think it would be good to change whereever possible.

Example:
Lift these debarments after Borrower has paid their charges >
Lift these restrictions after a patron has paid their charges

See also: https://wiki.koha-community.org/wiki/Terminology

3) Number of tests

The number of tests needs to be updated to 37 for tests to pass :)

4) Functionality

A thing I stumbled on is that this works on the restriction comment and not the type of restriction. I believe this is done, because we can't have different types of restrictions currently and this makes it more flexible. But I am not sure if it will not also create issues.

Currently the comment can only be set, if the type of restriction is MANUAL, for DISCHARGE it will be empty, possibly for OVERDUES and SUSPENSION as well. We cannot edit restrictions to add a certain comment or fix a typo.

I recently reviewed bug 23681 and I think this goes in a more maintainable direction. It will allow to define restriction types in a new table with some being Koha-internal and others being added in addition. 

And what you do with the YAML pref could then be moved to be columns in the new table, which would make it easy to query, easy to handle for staff, etc. and remove any issues with variations of spelling/whitespace by having a list to pick from.

I know this is the older dev, but maybe have a look at bug 23681?

It's currently waiting for some more work, but I think it would be still achievable for the next version, especially with a little help.