Bug 34077

Created attachment 157230 [details] [review]
Bug 34077: Update output to match POD

This patch adds a check on !$confirm such that we enable verbose output
should the confirm flag not be passed. This adheres with what the
documentation already states.

I had a go at testing.

Just to clarify, in case I got something wrong:

1. With --confirm it should do what it is supposed to do (it did for me)

2. Without --confirm it should show what it would change (this didn't work as expected for me - see 3 and 4)

3. Without --confirm and with only one option, the help is shown, for example: ./misc/cronjobs/writeoff_debts.pl --added-after 2023-06-20

4. Without --confirm with two or more options, it will show what would change, for example: ./misc/cronjobs/writeoff_debts.pl --added-after 2023-06-20 -v

This is after the patch was applied and transactions set up and date of one transaction changed to earlier.

Changing status to Failed QA - see comment#2

Created attachment 159598 [details] [review]
Bug 34077: Update output to match POD

This patch adds a check on !$confirm such that we enable verbose output
should the confirm flag not be passed. This adheres with what the
documentation already states.

The logic was flawed, thanks for testing David.

I've corrected it now, and also added a little extra detail into the output as it was missing some of the more recently added parameters.

I'm now getting an error when I confirm. Am I testing this correctly?

 ./misc/cronjobs/writeoff_debts.pl --added_after 2023-12-04 --category-code PT --confirm
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'type' at row 1 at /kohadevbox/koha/Koha/Object.pm line 170
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'type' at row 1 at /kohadevbox/koha/Koha/Object.pm line 170

or

./misc/cronjobs/writeoff_debts.pl --added_after 2023-12-04 --confirm
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'type' at row 1 at /kohadevbox/koha/Koha/Object.pm line 170
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'type' at row 1 at /kohadevbox/koha/Koha/Object.pm line 170


This is the test plan I used:

1. Add two fees in a patron account (I used Mary Burton, borrower number = 49):
   1.1. Go into a patron record > Accounting tab
   1.2. Go to the Create a manual invoice tab
   1.3. Fill out the form (use manual fee)
   1.4. Click 'Save'
   1.5. Redo steps 1.2 to 1.4 to add a second fee
   
2. Manually change the date for one transaction in the database:
   2.1. There should be two transactions in the accountlines and account_offsets tables (koha-mysql kohadev + select * from accountlines; + select * from account_offsets;)
   2.2. Update the date for one of the fees using the accountlines_id:
        update accountlines set date = '2023-06-01 09:34:05' where accountlines_id = 1;
        update account_offsets set created_on = '2023-06-01 09:34:05' where debit_id = '1';
   => If you look at the patron record in the staff interface, you should now have two fees with different date: one fee on 1 June 2023 and one with today's date (the account_offset is also modified - not sure if that last part is necessary).

3. Run writeoff_debts.pl with various options to make sure it works as expected (to use --added_after, the date should be after the first fee but before the second):
   3.1. Without any options it should return the help: ./misc/cronjobs/writeoff_debts.pl
   3.2. Without --confirm and one option it should show what it would change: ./misc/cronjobs/writeoff_debts.pl --added_after 2023-12-04 
      ==> With --confirm we would write off 1 debts added after 2023-12-04
          Accountline 2 will be written of
   3.3. Without --confirm and with two or more options - it should show what it would change: ./misc/cronjobs/writeoff_debts.pl --added-after 2023-06-20 --category-code PT
      ==> With --confirm we would write off 1 debts of type MANUAL added after 2023-06-20
          Accountline 2 will be written off
   3.4. With --confirm it should correctly write off one amount: ./misc/cronjobs/writeoff_debts.pl --added_after 2023-12-04 --confirm

4. Check the patron record
   ==> The fee with the date after should be written off

Created attachment 159987 [details] [review]
Bug 34077: Update output to match POD

This patch adds a check on !$confirm such that we enable verbose output
should the confirm flag not be passed. This adheres with what the
documentation already states.

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>

This causes a small usability issue IMO:

`writeoff_debts.pl` and
`writeoff_debts.pl -v` have the same verbosity level, but

`writeoff_debts.pl -c` and
`writeoff_debts.pl -c -v` have different verbosity level

One might run `writeoff_debts.pl` first, be happy with the output and just add -c to confirm, only to be disappointed that there is no output now.

What about doing the opposite ? Instead of updating the script to match the POD, update the POD to match the script behavior and say that -v is required if -c is absent ?

Needs feedback. Previous comment sounds good to me.

Created attachment 161772 [details] [review]
Bug 34077: Update output to match POD

This patch adds a check on !$confirm such that we enable verbose output
should the confirm flag not be passed. This adheres with what the
documentation already states.

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>

Created attachment 161773 [details] [review]
Bug 34077: Only print verbose output when asked for

This patch updates the logic such that we only trigger verbose when
verbose is explicitly asked for.

Follow up added to take account of Julians comment.. He's right.. we should really only output verbose if it's specifically asked for.

Created attachment 161774 [details] [review]
Bug 34077: Tidy the script

Minimal changes were needed to bring the script up to date with current
perltidy requirements. I opted to do a full tidy rather than just tidy
the blocks changed.

With the followup we can now execute the script without -v nor -c, which prints nothing and does nothing. Not only useless, it may also give the impression that something has been done without errors. Failing QA for that.
I think the current behavior in master is ok (-v is required if -c is absent) and only the documentation needs to be fixed.

On the other hand, requiring a "verbose" flag is unusual for a CLI. So maybe one of the following alternatives is better (or both):
- Set default verbosity to 1, and add a `-q, --quiet` flag to decrease verbosity
- Add a `-n, --dry-run` flag and require exclusively one of -n or -c
(maybe best in an another bug report ?)

I'm out, this was meant to be a simple consistency fix

Created attachment 162234 [details] [review]
Bug 34077: Improve documentation and error messages of writeoff_debts.pl

* Change the SYNOPSIS to better describe the different ways to use the
  script
* Only show the SYNOPSIS when options used are wrong (unknown option,
  no filter options, or neither -c nor -v)
* Show the options details only with --help
* Clarify the fact that -v is required when -c is not supplied in the
  description of both options
* Print a specific error message for the following cases:
  * no filters options
  * neither -c nor -v was supplied

Created attachment 162235 [details] [review]
Bug 34077: Allow to use --category-code as the only filter option

--category-code was not checked in the "at least one filter option"
check but it is clearly a filter option.

Created attachment 167403 [details] [review]
Bug 34077: Improve documentation and error messages of writeoff_debts.pl

* Change the SYNOPSIS to better describe the different ways to use the
  script
* Only show the SYNOPSIS when options used are wrong (unknown option,
  no filter options, or neither -c nor -v)
* Show the options details only with --help
* Clarify the fact that -v is required when -c is not supplied in the
  description of both options
* Print a specific error message for the following cases:
  * no filters options
  * neither -c nor -v was supplied

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167404 [details] [review]
Bug 34077: Allow to use --category-code as the only filter option

--category-code was not checked in the "at least one filter option"
check but it is clearly a filter option.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Thanks for taking over here Julian.. I was a bit burnt out.

I appreciate the help.

Passing QA

Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant

Backported to 24.05.x for upcoming 24.05.02

Pushed to 23.11.x for 23.11.07

Apologies for bringing this one up again! I was trying to write a release note and was testing the script.

I don't think the script is working as it should:
1. I couldn't get it to work when using the --confirm option.
2. Using the --category-code option - which one is it?:
   - The second patch title says "Allow to use --category-code as the only filter option"
   - The help text says: 
     . Please note that --category-code must be accompanied by another filter - the script will not run if this is the only filter provided.
     . This option cannot be used alone, it must be combined with another filter.

Maybe another change has affected this one?

If not, I'll create another bug.

1. Add three manual fees to a patron's account with a patron category of PT.

2. Run the script with these options (all the options I tried with --verbose work as expected):
   - misc/cronjobs/writeoff_debts.pl --verbose --category-code=PT 
   - misc/cronjobs/writeoff_debts.pl --verbose --type=MANUAL
   - misc/cronjobs/writeoff_debts.pl --verbose --category-code=PT --added-before=2024-08-02
   - misc/cronjobs/writeoff_debts.pl --verbose --category-code=PT --added-after=2024-07-01

   Results: You get a message similar to this showing what will happen (this is for --category-code=PT --verbose)
   Attempting to write off 3 debts
   Accountline 1 will be written off
   Accountline 2 will be written off
   Accountline 3 will be written off

3. Run the script examples in step 2 again with the --confirm option instead of the --verbose option. This doesn't work as expected for any combination I tried, and you get output similar to this:

   misc/cronjobs/writeoff_debts.pl --confirm --category-code=PT --added-before=2024-08-02
   DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'type' at row 1 at /kohadevbox/koha/Koha/Object.pm line 172
   DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'type' at row 1 at /kohadevbox/koha/Koha/Object.pm line 172

It looks like some data is "too long" for a field it#s being moved to. Wondering if you tested on a new database or if there might be any weird data from testbuilder around?

Not backporting to 23.05.x unless requested