Bug 32450

Summary: Make it possible to exclude debit types from charges counted for circulation restriction (noissuecharge)
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: Fines and feesAssignee: Matt Blenkinsop <matt.blenkinsop>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: caroline.cyr-la-rose, katrin.fischer, m.de.rooy, martin.renvoize, matt.blenkinsop, michaela.sieber
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Caroline Cyr La Rose Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/692
Text to go in the release notes:
**Sponsored by** *PTFS Europe* This enhancement allows a user to select which debit types should be included in deciding whether a patron should be restricted from checkouts (`noissuescharge` system preference). Three existing system preferences have been deleted (`ManInvInNoissuesCharge`, `RentalsInNoissuesCharge`, `HoldsInNoissuesCharge`) and the management of the debit types now sits in the Debit Types area of system preferences. The user can edit each debit type and select whether it should be included in the `noissuescharge` calculation, giving users much more flexibility over restrictions.
Version(s) released in:
23.05.00
Bug Depends on:    
Bug Blocks: 33264, 33787    
Attachments: Bug 32450: Noissuescharge debit type exclusions
Bug 32450: Update db to add noissuescharge flag and remove sysprefs
Bug 32450: Noissuescharge debit type exclusions
Bug 32450: Update db to add noissuescharge flag and remove sysprefs
Bug 32450: Update db to add noissuescharge flag and remove sysprefs
Bug 32450: Noissuescharge debit type exclusions
Bug 32450: Update db to add noissuescharge flag and remove sysprefs
Bug 32450: DBIC Schema
Bug 32450: Noissuescharge debit type exclusions
Bug 32450: Unit tests updated
Bug 32450: Add boolean to DBIC schema
Bug 32450: (QA follow-up) Fix QA test errors
Bug 32450: Update db to add noissuescharge flag and remove sysprefs
Bug 32450: DBIC Schema
Bug 32450: Noissuescharge debit type exclusions
Bug 32450: Unit tests updated
Bug 32450: Add boolean to DBIC schema
Bug 32450: (QA follow-up) Fix QA test errors
Bug 32450: (QA follow-up) Polishing atomic update
Bug 32450: (QA follow-up) Update UsageStats for removed prefs
Bug 32450: (QA follow-up) Fix failing tests
Bug 32450: (QA follow-up) Change wording in debit types table
Bug 32450: Update db to add noissuescharge flag and remove sysprefs
Bug 32450: DBIC Schema
Bug 32450: Noissuescharge debit type exclusions
Bug 32450: Unit tests updated
Bug 32450: Add boolean to DBIC schema
Bug 32450: (QA follow-up) Fix QA test errors
Bug 32450: (QA follow-up) Polishing atomic update
Bug 32450: (QA follow-up) Update UsageStats for removed prefs
Bug 32450: (QA follow-up) Fix failing tests
Bug 32450: (QA follow-up) Change wording in debit types table
Bug 32450: (QA follow-up) Fix SIP/Transaction.t

Description Katrin Fischer 2022-12-12 14:11:14 UTC
We do have several permissions that are about what is counted for noissuecharge, the dynamic restriction for circulation.

* ManInvInNoissuesCharge = All manual invoiceable debit types
* RentalsInNoissuesCharge = Exclude hold charges
* HoldsInNoissuesCharge = Exclude rental charges

This all comes from a time when Koha's debit types were hardcoded, but had no 'home' in the database. Now that they have their own database table we could move them to a flag for each debit type.

This was triggered by a discussion by making a certain debit type not count towards the circulation restriction, but our current system is to inflexible in that regard.
Comment 1 Katrin Fischer 2022-12-12 15:40:44 UTC
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
Comment 2 Matt Blenkinsop 2023-01-06 13:52:10 UTC
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.
Comment 3 Matt Blenkinsop 2023-01-06 13:54:51 UTC
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.
Comment 4 ByWater Sandboxes 2023-01-07 10:18:42 UTC
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>
Comment 5 ByWater Sandboxes 2023-01-07 10:18:45 UTC
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 6 Martin Renvoize 2023-01-09 11:27:29 UTC
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 7 Martin Renvoize 2023-01-09 11:32:16 UTC
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
Comment 8 Matt Blenkinsop 2023-01-10 12:39:34 UTC
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.
Comment 9 Matt Blenkinsop 2023-01-10 12:52:46 UTC
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.
Comment 10 Martin Renvoize 2023-01-10 14:51:53 UTC
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>
Comment 11 Martin Renvoize 2023-01-10 14:51:57 UTC
Created attachment 145190 [details] [review]
Bug 32450: DBIC Schema
Comment 12 Martin Renvoize 2023-01-10 14:52:02 UTC
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>
Comment 13 Martin Renvoize 2023-01-10 14:54:43 UTC
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
Comment 14 Matt Blenkinsop 2023-01-10 16:03:45 UTC
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
Comment 15 Matt Blenkinsop 2023-01-10 16:34:14 UTC
Created attachment 145198 [details] [review]
Bug 32450: Add boolean to DBIC schema

Adding boolean flag to DBIC schema. Test plan as per previous commits
Comment 16 Marcel de Rooy 2023-02-24 10:43:32 UTC
 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
Comment 17 Marcel de Rooy 2023-02-24 10:43:49 UTC
__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 ?
Comment 18 Matt Blenkinsop 2023-03-01 14:58:35 UTC
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
Comment 19 Marcel de Rooy 2023-04-14 08:01:22 UTC
Looking here again
Comment 20 Marcel de Rooy 2023-04-14 09:52:23 UTC
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>
Comment 21 Marcel de Rooy 2023-04-14 09:52:26 UTC
Created attachment 149654 [details] [review]
Bug 32450: DBIC Schema

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2023-04-14 09:52:28 UTC
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>
Comment 23 Marcel de Rooy 2023-04-14 09:52:31 UTC
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>
Comment 24 Marcel de Rooy 2023-04-14 09:52:33 UTC
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>
Comment 25 Marcel de Rooy 2023-04-14 09:52:35 UTC
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>
Comment 26 Marcel de Rooy 2023-04-14 09:52:38 UTC
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>
Comment 27 Marcel de Rooy 2023-04-14 09:52:40 UTC
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>
Comment 28 Marcel de Rooy 2023-04-14 09:55:51 UTC
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.
Comment 29 Marcel de Rooy 2023-04-14 09:56:03 UTC
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.
Comment 30 Marcel de Rooy 2023-04-14 09:56:39 UTC
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.
Comment 31 Matt Blenkinsop 2023-04-19 10:34:01 UTC
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
Comment 32 Matt Blenkinsop 2023-04-19 10:34:04 UTC
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
Comment 33 Marcel de Rooy 2023-04-19 11:40:35 UTC
Thanks Matt. Revisiting this one now.
Comment 34 Marcel de Rooy 2023-04-19 12:35:10 UTC
 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 :)
Comment 35 Marcel de Rooy 2023-04-19 12:39:01 UTC
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.
Comment 36 Marcel de Rooy 2023-04-19 12:41:09 UTC
(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.
Comment 37 Marcel de Rooy 2023-04-19 12:42:11 UTC
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>
Comment 38 Marcel de Rooy 2023-04-19 12:42:14 UTC
Created attachment 149873 [details] [review]
Bug 32450: DBIC Schema

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Marcel de Rooy 2023-04-19 12:42:16 UTC
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>
Comment 40 Marcel de Rooy 2023-04-19 12:42:19 UTC
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>
Comment 41 Marcel de Rooy 2023-04-19 12:42:21 UTC
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>
Comment 42 Marcel de Rooy 2023-04-19 12:42:24 UTC
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>
Comment 43 Marcel de Rooy 2023-04-19 12:42:27 UTC
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>
Comment 44 Marcel de Rooy 2023-04-19 12:42:29 UTC
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>
Comment 45 Marcel de Rooy 2023-04-19 12:42:32 UTC
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>
Comment 46 Marcel de Rooy 2023-04-19 12:42:34 UTC
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>
Comment 47 Marcel de Rooy 2023-04-19 12:42:37 UTC
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>
Comment 48 Martin Renvoize 2023-04-19 14:31:34 UTC
Awesome guys, thanks for this.. great to see it worked through and the teamwork that happened here :)
Comment 49 Katrin Fischer 2023-04-21 06:42:08 UTC
Also very glad to see this has reached PQA, thanks to all involved!
Comment 50 Tomás Cohen Arazi 2023-05-17 13:25:36 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 51 Matt Blenkinsop 2023-06-02 16:20:22 UTC
Enhancement - not backporting to 22.11.x