Bug 34077

Summary: writeoff_debts without --confirm doesn't show which accountline records it would have been written off
Product: Koha Reporter: Caroline Cyr La Rose <caroline.cyr-la-rose>
Component: Command-line UtilitiesAssignee: Julian Maurice <julian.maurice>
Status: Needs Signoff --- QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: david, julian.maurice, m.de.rooy, martin.renvoize, philippe.blouin, robin
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28995
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35996
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 27049, 28995, 35074    
Bug Blocks:    
Attachments: Bug 34077: Update output to match POD
Bug 34077: Update output to match POD
Bug 34077: Update output to match POD
Bug 34077: Update output to match POD
Bug 34077: Only print verbose output when asked for
Bug 34077: Tidy the script
Bug 34077: Improve documentation and error messages of writeoff_debts.pl
Bug 34077: Allow to use --category-code as the only filter option

Description Caroline Cyr La Rose 2023-06-21 14:07:35 UTC
The documentation in writeoff_debts says that if we omit --confirm, "the script will only report on the accountline records it would have been written off".

However, while testing bug 28995, I tried to see which accountline would be written off, and omitting the --confirm flag only shows the help.

I ran

./misc/cronjobs/writeoff_debts.pl --added-after 2023-06-20

and it showed the help.

When I ran

./misc/cronjobs/writeoff_debts.pl --added-after 2023-06-20 --confirm

it wrote off the charge correctly.
Comment 1 Martin Renvoize 2023-10-17 11:51:01 UTC
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.
Comment 2 David Nind 2023-11-16 20:33:18 UTC
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.
Comment 3 David Nind 2023-12-03 22:40:27 UTC
Changing status to Failed QA - see comment#2
Comment 4 Martin Renvoize 2023-12-05 16:49:38 UTC
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.
Comment 5 Martin Renvoize 2023-12-05 16:50:26 UTC
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.
Comment 6 David Nind 2023-12-05 22:50:27 UTC
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
Comment 7 Caroline Cyr La Rose 2023-12-18 21:43:33 UTC
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>
Comment 8 Julian Maurice 2023-12-21 12:51:03 UTC
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 ?
Comment 9 Marcel de Rooy 2024-01-19 10:43:35 UTC
Needs feedback. Previous comment sounds good to me.
Comment 10 Martin Renvoize 2024-02-06 12:16:16 UTC
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>
Comment 11 Martin Renvoize 2024-02-06 12:16:19 UTC
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.
Comment 12 Martin Renvoize 2024-02-06 12:17:16 UTC
Follow up added to take account of Julians comment.. He's right.. we should really only output verbose if it's specifically asked for.
Comment 13 Martin Renvoize 2024-02-06 12:22:32 UTC
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.
Comment 14 Julian Maurice 2024-02-06 12:53:36 UTC
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 ?)
Comment 15 Martin Renvoize 2024-02-06 15:02:28 UTC
I'm out, this was meant to be a simple consistency fix
Comment 16 Julian Maurice 2024-02-16 08:39:13 UTC
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
Comment 17 Julian Maurice 2024-02-16 08:39:16 UTC
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.