I've had a look at our current code and it looks the proposed solution is already part of a FIXME:

sub non_issues_charges {
    my ($self) = @_;

    #NOTE: With bug 23049 these preferences could be moved to being attached
    #to individual debit types to give more flexability and specificity.
    my @not_fines;
    push @not_fines, 'RESERVE'
      unless C4::Context->preference('HoldsInNoissuesCharge');
    push @not_fines, ( 'RENT', 'RENT_DAILY', 'RENT_RENEW', 'RENT_DAILY_RENEW' )
      unless C4::Context->preference('RentalsInNoissuesCharge');
    unless ( C4::Context->preference('ManInvInNoissuesCharge') ) {
        my @man_inv = Koha::Account::DebitTypes->search({ is_system => 0 })->get_column('code');
        push @not_fines, @man_inv;
    }

But looking at the code I also realized another thing 

* ManInvInNoissuesCharge = All manually invoiceable debit types

This is wrong, it's not only the manually invoiceable ones per the checkbox you can set, but all manually added debit types, which makes this even less flexible than we had initially thought. 

* ManInvInNoissuesCharge = All manually added debit types, all but the system internal ones

Created attachment 145080 [details] [review]
Bug 32450: Noissuescharge debit type exclusions

Currently the debit types to be excluded from the noissuescharge syspref are hardcoded in non_issues_charges which gives no flexibility for selecting which debit types should be included. This patch amends the subrout
ine to use a database flag to identify which debit types should be included. It also adds a column to the table in the Debit Types area of System Preferences which shows which debit types are included. The ability to
edit all debit types has been added rather than just the non-system ones and the flag to include/exclude the debit type from noissuescharge can be changed by clicking that edit button.

Test plan:
1) Choose a patron and add some fines to this patron that have different debit_types
2) Navigate to system preferences and observe that currently you can only amend the noissuescharge included debit types using three preferences: ManInvInNoissuesCharge, RentalsInNoissuesCharge, HoldsInNoissuesCharge
3) Apply both commits attached to this bug
4) Navigate as above and observe that these three system preferences are now gone
5) Navigate to Debit Types in System Preferences, the table should have a column called No issues charge that shows whether a debit_type is Included or Not included
6) Click the edit button and there should be a checkbox for Included in noissuescharge
7) Change some of the debit_types using this option and observe that the patron you added fines to will either be blocked from checkouts or able to checkout depending on which debit_types you include and the value of
these fines.

Created attachment 145081 [details] [review]
Bug 32450: Update db to add noissuescharge flag and remove sysprefs

This commit updates the database to add a new flag to show which debit_types are included in noissuescharge. It also deletes the current sysprefs that are hardcoded for this as these are now redundant.

Test plan:
1) Choose a patron and add some fines to this patron that have different debit_types
2) Navigate to system preferences and observe that currently you can only amend the noissuescharge included debit types using three preferences: ManInvInNoissuesCharge, RentalsInNoissuesCharge, HoldsInNoissuesCharge
3) Apply both commits attached to this bug
4) Navigate as above and observe that these three system preferences are now gone
5) Navigate to Debit Types in System Preferences, the table should have a column called No issues charge that shows whether a debit_type is Included or Not included
6) Click the edit button and there should be a checkbox for Included in noissuescharge
7) Change some of the debit_types using this option and observe that the patron you added fines to will either be blocked from checkouts or able to checkout depending on which debit_types you include and the value of
these fines.

Created attachment 145127 [details] [review]
Bug 32450: Noissuescharge debit type exclusions

Currently the debit types to be excluded from the noissuescharge syspref are hardcoded in non_issues_charges which gives no flexibility for selecting which debit types should be included. This patch amends the subrout
ine to use a database flag to identify which debit types should be included. It also adds a column to the table in the Debit Types area of System Preferences which shows which debit types are included. The ability to
edit all debit types has been added rather than just the non-system ones and the flag to include/exclude the debit type from noissuescharge can be changed by clicking that edit button.

Test plan:
1) Choose a patron and add some fines to this patron that have different debit_types
2) Navigate to system preferences and observe that currently you can only amend the noissuescharge included debit types using three preferences: ManInvInNoissuesCharge, RentalsInNoissuesCharge, HoldsInNoissuesCharge
3) Apply both commits attached to this bug
4) Navigate as above and observe that these three system preferences are now gone
5) Navigate to Debit Types in System Preferences, the table should have a column called No issues charge that shows whether a debit_type is Included or Not included
6) Click the edit button and there should be a checkbox for Included in noissuescharge
7) Change some of the debit_types using this option and observe that the patron you added fines to will either be blocked from checkouts or able to checkout depending on which debit_types you include and the value of
these fines.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Created attachment 145128 [details] [review]
Bug 32450: Update db to add noissuescharge flag and remove sysprefs

This commit updates the database to add a new flag to show which debit_types are included in noissuescharge. It also deletes the current sysprefs that are hardcoded for this as these are now redundant.

Test plan:
1) Choose a patron and add some fines to this patron that have different debit_types
2) Navigate to system preferences and observe that currently you can only amend the noissuescharge included debit types using three preferences: ManInvInNoissuesCharge, RentalsInNoissuesCharge, HoldsInNoissuesCharge
3) Apply both commits attached to this bug
4) Navigate as above and observe that these three system preferences are now gone
5) Navigate to Debit Types in System Preferences, the table should have a column called No issues charge that shows whether a debit_type is Included or Not included
6) Click the edit button and there should be a checkbox for Included in noissuescharge
7) Change some of the debit_types using this option and observe that the patron you added fines to will either be blocked from checkouts or able to checkout depending on which debit_types you include and the value of
these fines.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Comment on attachment 145127 [details] [review]
Bug 32450: Noissuescharge debit type exclusions

Review of attachment 145127 [details] [review]:
-----------------------------------------------------------------

::: Koha/Account.pm
@@ +711,5 @@
>      my ($self) = @_;
>  
> +    my @fines;
> +    my $dbh=C4::Context->dbh;
> +    my $sth = $dbh->prepare("SELECT code FROM account_debit_types WHERE no_issues_charge = 1");

Sorry Matt, this will need to be converted to a DBIC style lookup rather than using a handle.. we dis-sallow direct SQL in the Koha:: namespace.

https://metacpan.org/pod/DBIx::Class::ResultSet#search is a good starting place for how this works.. especially the WHERE clause link from it.

tl:dr;  It'll be something along the lines

my $blocking_types => Koha::Debit::Types->search({ no_issues_charge => 1 }, { columns => 'code' });

Then the ->lines->search could use it inline.

$self->lines->search(
  {
    debit_type_code => { '-in' => $blocking_types }
  }
);

This so pseudo code untested ;P

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/debit_types.tt
@@ +220,2 @@
>                                          <td class="actions">
> +                                            [% IF !debit_type.archived %]

Hmm, I wonder if at least some of the fields in system level types need to be marked as read only.

Comment on attachment 145128 [details] [review]
Bug 32450: Update db to add noissuescharge flag and remove sysprefs

Review of attachment 145128 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/atomicupdate/bug_32450-add_debit_type_flag.pl
@@ +6,5 @@
> +    up => sub {
> +        my ($args) = @_;
> +        my ($dbh, $out) = @$args{qw(dbh out)};
> +
> +        if( !column_exists( 'account_debit_types', 'no_issues_charge' ) ) {

I'd be tempted to name this 'blocks_issue' or 'prevents_issue' or something as opposed to 'no_issues_charge'.

@@ +15,5 @@
> +          say $out "Added column 'account_debit_types.no_issues_charge'";
> +        }
> +
> +        $dbh->do(q{
> +            DELETE FROM systempreferences WHERE variable = 'ManInvInNoissuesCharge'

We'll need an extra step in here I'm afraid.. We're not accounting for the current state of the system preference at upgrade time.

You'll want to check the current value of ManInvInNoissuesCharge and set the boolean flag for the appropriate, prior to this patch hard coded, set of debit types based on it.

@@ +19,5 @@
> +            DELETE FROM systempreferences WHERE variable = 'ManInvInNoissuesCharge'
> +        });
> +
> +        $dbh->do(q{
> +            DELETE FROM systempreferences WHERE variable = 'RentalsInNoissuesCharge'

As above

@@ +22,5 @@
> +        $dbh->do(q{
> +            DELETE FROM systempreferences WHERE variable = 'RentalsInNoissuesCharge'
> +        });
> +        $dbh->do(q{
> +            DELETE FROM systempreferences WHERE variable = 'HoldsInNoissuesCharge'

As above

Created attachment 145171 [details] [review]
Bug 32450: Update db to add noissuescharge flag and remove sysprefs

This commit updates the database to add a new flag to show which debit_types are included in noissuescharge. It also deletes the current sysprefs that are hardcoded for this as these are now redundant.

Test plan:
1) Choose a patron and add some fines to this patron that have different debit_types
2) Navigate to system preferences and observe that currently you can only amend the noissuescharge included debit types using three preferences: ManInvInNoissuesCharge, RentalsInNoissuesCharge, HoldsInNoissuesCharge
3) Apply both commits attached to this bug
4) Navigate as above and observe that these three system preferences are now gone
5) Navigate to Debit Types in System Preferences, the table should have a column called No issues charge that shows whether a debit_type is Included or Not included
6) Click the edit button and there should be a checkbox for Included in noissuescharge
7) Change some of the debit_types using this option and observe that the patron you added fines to will either be blocked from checkouts or able to checkout depending on which debit_types you include and the value of
these fines.

Created attachment 145172 [details] [review]
Bug 32450: Noissuescharge debit type exclusions

Currently the debit types to be excluded from the noissuescharge syspref are hardcoded in non_issues_charges which gives no flexibility for selecting which debit types should be included. This patch amends the subrout
ine to use a database flag to identify which debit types should be included. It also adds a column to the table in the Debit Types area of System Preferences which shows which debit types are included. The ability to
edit all debit types has been added rather than just the non-system ones and the flag to include/exclude the debit type from noissuescharge can be changed by clicking that edit button.

Test plan:
1) Choose a patron and add some fines to this patron that have different debit_types
2) Navigate to system preferences and observe that currently you can only amend the noissuescharge included debit types using three preferences: ManInvInNoissuesCharge, RentalsInNoissuesCharge, HoldsInNoissuesCharge
3) Apply both commits attached to this bug
4) Navigate as above and observe that these three system preferences are now gone
5) Navigate to Debit Types in System Preferences, the table should have a column called No issues charge that shows whether a debit_type is Included or Not included
6) Click the edit button and there should be a checkbox for Included in noissuescharge
7) Change some of the debit_types using this option and observe that the patron you added fines to will either be blocked from checkouts or able to checkout depending on which debit_types you include and the value of
these fines.

Created attachment 145189 [details] [review]
Bug 32450: Update db to add noissuescharge flag and remove sysprefs

This commit updates the database to add a new flag to show which debit_types are
included in noissuescharge. It also deletes the current sysprefs that are
hardcoded for this as these are now redundant.

Test plan:
1) Choose a patron and add some fines to this patron that have different debit_types
2) Navigate to system preferences and observe that currently you can only amend the
   noissuescharge included debit types using three preferences: ManInvInNoissuesCharge,
   RentalsInNoissuesCharge, HoldsInNoissuesCharge
3) Apply both commits attached to this bug
4) Navigate as above and observe that these three system preferences are now gone
5) Navigate to Debit Types in System Preferences, the table should have a column
   called No issues charge that shows whether a debit_type is Included or Not included
6) Click the edit button and there should be a checkbox for Included in noissuescharge
7) Change some of the debit_types using this option and observe that the patron you
   added fines to will either be blocked from checkouts or able to checkout depending
   on which debit_types you include and the value of these fines.

Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Created attachment 145190 [details] [review]
Bug 32450: DBIC Schema

Created attachment 145191 [details] [review]
Bug 32450: Noissuescharge debit type exclusions

Currently the debit types to be excluded from the noissuescharge syspref are
hardcoded in non_issues_charges which gives no flexibility for selecting which
debit types should be included. This patch amends the subroutine to use a
database flag to identify which debit types should be included. It also adds a
column to the table in the Debit Types area of System Preferences which shows
which debit types are included. The ability to edit all debit types has been
added rather than just the non-system ones and the flag to include/exclude the
debit type from noissuescharge can be changed by clicking that edit button.

Test plan:
1) Choose a patron and add some fines to this patron that have different debit_types
2) Navigate to system preferences and observe that currently you can only amend
   the noissuescharge included debit types using three preferences:
   ManInvInNoissuesCharge, RentalsInNoissuesCharge, HoldsInNoissuesCharge
3) Apply both commits attached to this bug
4) Navigate as above and observe that these three system preferences are now gone
5) Navigate to Debit Types in System Preferences, the table should have a column
   called No issues charge that shows whether a debit_type is Included or Not
   included
6) Click the edit button and there should be a checkbox for Included in
   noissuescharge
7) Change some of the debit_types using this option and observe that the patron
   you added fines to will either be blocked from checkouts or able to checkout
   depending on which debit_types you include and the value of these fines.

Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

I restored the signoff lines and split the dbic build out of the first patch.. it's customary to commit that one separately as it's the most likely one to clash on rebases.

Code is looking good now, but I think we need the Unit tests updating..

Calls to 'non_issues_charges' appear to be present in db_dependent/Accounts.t and db_dependent/Koha/Patron.t.

It's worth running them, and perhaps the other accounts related tests in ktd to see if they still pass Matt.. and maybe worth looking to check that your change is now covered in those tests too ;P

Created attachment 145194 [details] [review]
Bug 32450: Unit tests updated

Unit tests for the non_issues_charges function have been updated to remove the system preferences and test based on the new database flags

Test plan:
1) Apply all patches
2) In the kshell, run prove -v t/db_dependent/Accounts.t
3) All tests should pass

Created attachment 145198 [details] [review]
Bug 32450: Add boolean to DBIC schema

Adding boolean flag to DBIC schema. Test plan as per previous commits

 FAIL   installer/data/mysql/atomicupdate/bug_32450-add_debit_type_flag.pl
   FAIL   file permissions
                File must have the exec flag

 FAIL   t/db_dependent/Accounts.t
   FAIL   valid
                "my" variable $total masks earlier declaration in same scope
                "my" variable $non_issues_charges masks earlier declaration in same scope
                "my" variable $other_charges masks earlier declaration in same scope

__PACKAGE__->add_columns(
    '+is_system' => { is_boolean => 1 }
);

__PACKAGE__->add_columns(
    "+can_be_sold" => { is_boolean => 1 }
);

__PACKAGE__->add_columns(
    "+can_be_invoiced" => { is_boolean => 1 }
);

__PACKAGE__->add_columns(
    "+restricts_checkouts" => { is_boolean => 1 }
);


Can you merge those in one statement btw ?

Created attachment 147563 [details] [review]
Bug 32450: (QA follow-up) Fix QA test errors

Exec flag added to atomic update file, trailing whitespaces removed
Unit tests patched to remove duplicate variable names

Looking here again

Created attachment 149653 [details] [review]
Bug 32450: Update db to add noissuescharge flag and remove sysprefs

This commit updates the database to add a new flag to show which debit_types are
included in noissuescharge. It also deletes the current sysprefs that are
hardcoded for this as these are now redundant.

Test plan:
1) Choose a patron and add some fines to this patron that have different debit_types
2) Navigate to system preferences and observe that currently you can only amend the
   noissuescharge included debit types using three preferences: ManInvInNoissuesCharge,
   RentalsInNoissuesCharge, HoldsInNoissuesCharge
3) Apply both commits attached to this bug
4) Navigate as above and observe that these three system preferences are now gone
5) Navigate to Debit Types in System Preferences, the table should have a column
   called No issues charge that shows whether a debit_type is Included or Not included
6) Click the edit button and there should be a checkbox for Included in noissuescharge
7) Change some of the debit_types using this option and observe that the patron you
   added fines to will either be blocked from checkouts or able to checkout depending
   on which debit_types you include and the value of these fines.

Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149654 [details] [review]
Bug 32450: DBIC Schema

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149655 [details] [review]
Bug 32450: Noissuescharge debit type exclusions

Currently the debit types to be excluded from the noissuescharge syspref are
hardcoded in non_issues_charges which gives no flexibility for selecting which
debit types should be included. This patch amends the subroutine to use a
database flag to identify which debit types should be included. It also adds a
column to the table in the Debit Types area of System Preferences which shows
which debit types are included. The ability to edit all debit types has been
added rather than just the non-system ones and the flag to include/exclude the
debit type from noissuescharge can be changed by clicking that edit button.

Test plan:
1) Choose a patron and add some fines to this patron that have different debit_types
2) Navigate to system preferences and observe that currently you can only amend
   the noissuescharge included debit types using three preferences:
   ManInvInNoissuesCharge, RentalsInNoissuesCharge, HoldsInNoissuesCharge
3) Apply both commits attached to this bug
4) Navigate as above and observe that these three system preferences are now gone
5) Navigate to Debit Types in System Preferences, the table should have a column
   called No issues charge that shows whether a debit_type is Included or Not
   included
6) Click the edit button and there should be a checkbox for Included in
   noissuescharge
7) Change some of the debit_types using this option and observe that the patron
   you added fines to will either be blocked from checkouts or able to checkout
   depending on which debit_types you include and the value of these fines.

Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149656 [details] [review]
Bug 32450: Unit tests updated

Unit tests for the non_issues_charges function have been updated to remove the system preferences and test based on the new database flags

Test plan:
1) Apply all patches
2) In the kshell, run prove -v t/db_dependent/Accounts.t
3) All tests should pass

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149657 [details] [review]
Bug 32450: Add boolean to DBIC schema

Adding boolean flag to DBIC schema. Test plan as per previous commits

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149658 [details] [review]
Bug 32450: (QA follow-up) Fix QA test errors

Exec flag added to atomic update file, trailing whitespaces removed
Unit tests patched to remove duplicate variable names

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149659 [details] [review]
Bug 32450: (QA follow-up) Polishing atomic update

Database comment.
Variable names.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149660 [details] [review]
Bug 32450: (QA follow-up) Update UsageStats for removed prefs

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

This looks quite good. But still needs some attention. I didnt finish up yet, but will post some findings already.
Please give attention first to the failing tests.

TODO Fix t/db_dependent/SIP/Patron.t
# Subtest: NoIssuesChargeGuarantees tests
    1..6
    not ok 1 - Only patron's fines are reported in total
    #   Failed test 'Only patron's fines are reported in total'
    #   at t/db_dependent/SIP/Patron.t line 315.
    #          got: '0'
    #     expected: '11'
    not ok 2 - Guarantor blocked
    #   Failed test 'Guarantor blocked'
    #   at t/db_dependent/SIP/Patron.t line 316.
    not ok 3 - Screen message includes related fines total
    #   Failed test 'Screen message includes related fines total'
    #   at t/db_dependent/SIP/Patron.t line 317.
    #                   'Greetings from Koha. yauSBUttb'
    #     doesn't match '(?^u:Patron blocked by fines \(11\.11\) on guaranteed accounts)'
    ok 4 - Guarantee only fines correctly counted
    ok 5 - Guarantee not blocked by guarantor fines
    ok 6 - Screen message does not include blocked message
    # Looks like you failed 3 tests of 6.
not ok 9 - NoIssuesChargeGuarantees tests
#   Failed test 'NoIssuesChargeGuarantees tests'
#   at t/db_dependent/SIP/Patron.t line 326.

TODO t/db_dependent/SIP/Transaction.t
# Looks like you failed 3 tests of 17.

Preliminary QA Comments
Bit confusing as to naming.
+        my @renewals = ('RENT', 'RENT_DAILY', 'RENT_RENEW', 'RENT_DAILY_RENEW');
+                    if($_ eq 'RentalsInNoissuesCharge') { push @debit_types_to_update, @renewals};
Will fix in a follow-up.

+                    if($_ eq 'ManInvInNoissuesCharge') { push @debit_types_to_update, @manual};
Perl allows you to do: statement if condition; for these kind of lines.
When we are using { }, we normally add newlines etc.
No blocker

This is a theory thing. But if you run it multiple times, the field has been inserted already and the DEFAULT 1 has been applied. The sysprefs have been deleted, so saying 'default value has been applied' is not true anymore. They are not reset again or so.
No blocker, just noting.

Wording is a bit less intuitive. The column name is No issues charge (which is already obscure). And then we could have the value Not included. Twice, no and not, might not be very clear. No blocker, could be improved. Or even should be, I am breaking my head every time ;)

Similar for sub non_issues_charges. Already existed. But non should better be no here, I guess. Leaving as-is.

Remove prefs from C4/UsageStats.pm. See follow-up.

Created attachment 149858 [details] [review]
Bug 32450: (QA follow-up) Fix failing tests

This patch fixes the failing tests in t/db_dependent/SIP/Patron.t

Test plan:
Run prove -v t/db_dependent/SIP/Patron.t and all tests should pass

Created attachment 149859 [details] [review]
Bug 32450: (QA follow-up) Change wording in debit types table

This patch changes confusing wording in the debit types table. Column heading No issues charge has been changed to Blocks checkouts? and the column data is now simply Yes/No rather than Included/Not included

Test plan:
1) Apply patch
2) Navigate to debit types in administration
3) The column in the table should exist as described above

Thanks Matt. Revisiting this one now.

 prove Circulation.t Circulation/NoIssuesChargeGuarantees.t Circulation/Returns.t Circulation/_CalculateAndUpdateFine.t Circulation/issue.t Koha/Account.t Koha/Account/Line.t Koha/Account/Lines.t Koha/Account/Offsets.t Koha/Cash/Register.t Koha/Cash/Register/Cashup.t Koha/Charges/Sales.t Koha/Checkouts.t Koha/Items.t Koha/Patron.t Koha/Plugins/Account_hooks.t Letters/TemplateToolkit.t Members.t Overdues.t Reserves.t SIP/Patron.t api/v1/patrons_accounts.t
Circulation.t ........................... 62/66 Use of uninitialized value $rentalCharge in numeric gt (>) at /usr/share/koha/C4/Circulation.pm line 1120.
Use of uninitialized value $rentalCharge in numeric gt (>) at /usr/share/koha/C4/Circulation.pm line 1120.
Use of uninitialized value $rentalCharge in numeric gt (>) at /usr/share/koha/C4/Circulation.pm line 1120.
Use of uninitialized value $rentalCharge in numeric gt (>) at /usr/share/koha/C4/Circulation.pm line 1120.
Circulation.t ........................... ok
Circulation/NoIssuesChargeGuarantees.t .. ok
Circulation/Returns.t ................... ok
Circulation/_CalculateAndUpdateFine.t ... ok
Circulation/issue.t ..................... ok
Koha/Account.t .......................... ok
Koha/Account/Line.t ..................... ok
Koha/Account/Lines.t .................... ok
Koha/Account/Offsets.t .................. ok
Koha/Cash/Register.t .................... ok
Koha/Cash/Register/Cashup.t ............. ok
Koha/Charges/Sales.t .................... ok
Koha/Checkouts.t ........................ ok
Koha/Items.t ............................ ok
Koha/Patron.t ........................... ok
Koha/Plugins/Account_hooks.t ............ ok
Letters/TemplateToolkit.t ............... ok
Members.t ............................... ok
Overdues.t .............................. ok
Reserves.t .............................. 8/77 No reserves HOLD_CANCELLATION letter transported by email at /usr/share/koha/C4/Letters.pm line 588.
Reserves.t .............................. ok
SIP/Patron.t ............................ ok
api/v1/patrons_accounts.t ............... ok
All tests successful.
Files=22, Tests=422, 216 wallclock secs ( 0.85 usr  0.12 sys + 167.92 cusr 25.45 csys = 194.34 CPU)
Result: PASS

Looks promising :)

git grep -l Accountline
Accounts.t
ILSDI_Services.t
Koha/Cash/Register.t
Koha/Patrons.t
Patrons.t
SIP/Patron.t
SIP/Transaction.t
api/v1/patrons_accounts.t

They pass too.

(In reply to Matt Blenkinsop from comment #32)
> Created attachment 149859 [details] [review] [review]
> Bug 32450: (QA follow-up) Change wording in debit types table
> 
> This patch changes confusing wording in the debit types table. Column
> heading No issues charge has been changed to Blocks checkouts? and the
> column data is now simply Yes/No rather than Included/Not included

Yes, I think that is clearer indeed.

Created attachment 149872 [details] [review]
Bug 32450: Update db to add noissuescharge flag and remove sysprefs

This commit updates the database to add a new flag to show which debit_types are
included in noissuescharge. It also deletes the current sysprefs that are
hardcoded for this as these are now redundant.

Test plan:
1) Choose a patron and add some fines to this patron that have different debit_types
2) Navigate to system preferences and observe that currently you can only amend the
   noissuescharge included debit types using three preferences: ManInvInNoissuesCharge,
   RentalsInNoissuesCharge, HoldsInNoissuesCharge
3) Apply both commits attached to this bug
4) Navigate as above and observe that these three system preferences are now gone
5) Navigate to Debit Types in System Preferences, the table should have a column
   called No issues charge that shows whether a debit_type is Included or Not included
6) Click the edit button and there should be a checkbox for Included in noissuescharge
7) Change some of the debit_types using this option and observe that the patron you
   added fines to will either be blocked from checkouts or able to checkout depending
   on which debit_types you include and the value of these fines.

Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149873 [details] [review]
Bug 32450: DBIC Schema

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149874 [details] [review]
Bug 32450: Noissuescharge debit type exclusions

Currently the debit types to be excluded from the noissuescharge syspref are
hardcoded in non_issues_charges which gives no flexibility for selecting which
debit types should be included. This patch amends the subroutine to use a
database flag to identify which debit types should be included. It also adds a
column to the table in the Debit Types area of System Preferences which shows
which debit types are included. The ability to edit all debit types has been
added rather than just the non-system ones and the flag to include/exclude the
debit type from noissuescharge can be changed by clicking that edit button.

Test plan:
1) Choose a patron and add some fines to this patron that have different debit_types
2) Navigate to system preferences and observe that currently you can only amend
   the noissuescharge included debit types using three preferences:
   ManInvInNoissuesCharge, RentalsInNoissuesCharge, HoldsInNoissuesCharge
3) Apply both commits attached to this bug
4) Navigate as above and observe that these three system preferences are now gone
5) Navigate to Debit Types in System Preferences, the table should have a column
   called No issues charge that shows whether a debit_type is Included or Not
   included
6) Click the edit button and there should be a checkbox for Included in
   noissuescharge
7) Change some of the debit_types using this option and observe that the patron
   you added fines to will either be blocked from checkouts or able to checkout
   depending on which debit_types you include and the value of these fines.

Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149875 [details] [review]
Bug 32450: Unit tests updated

Unit tests for the non_issues_charges function have been updated to remove the system preferences and test based on the new database flags

Test plan:
1) Apply all patches
2) In the kshell, run prove -v t/db_dependent/Accounts.t
3) All tests should pass

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149876 [details] [review]
Bug 32450: Add boolean to DBIC schema

Adding boolean flag to DBIC schema. Test plan as per previous commits

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149877 [details] [review]
Bug 32450: (QA follow-up) Fix QA test errors

Exec flag added to atomic update file, trailing whitespaces removed
Unit tests patched to remove duplicate variable names

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149878 [details] [review]
Bug 32450: (QA follow-up) Polishing atomic update

Database comment.
Variable names.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149879 [details] [review]
Bug 32450: (QA follow-up) Update UsageStats for removed prefs

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149880 [details] [review]
Bug 32450: (QA follow-up) Fix failing tests

This patch fixes the failing tests in t/db_dependent/SIP/Patron.t

Test plan:
Run prove -v t/db_dependent/SIP/Patron.t and all tests should pass

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149881 [details] [review]
Bug 32450: (QA follow-up) Change wording in debit types table

This patch changes confusing wording in the debit types table. Column heading No issues charge has been changed to Blocks checkouts? and the column data is now simply Yes/No rather than Included/Not included

Test plan:
1) Apply patch
2) Navigate to debit types in administration
3) The column in the table should exist as described above

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 149882 [details] [review]
Bug 32450: (QA follow-up) Fix SIP/Transaction.t

You forgot this one, Matt. Comment29 :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Awesome guys, thanks for this.. great to see it worked through and the teamwork that happened here :)

Also very glad to see this has reached PQA, thanks to all involved!

Pushed to master for 23.05.

Nice work everyone, thanks!

Enhancement - not backporting to 22.11.x