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.
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.