Bug 34077 - writeoff_debts without --confirm doesn't show which accountline records it would have been written off
Summary: writeoff_debts without --confirm doesn't show which accountline records it wo...
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Julian Maurice
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 27049 28995 35074
Blocks:
  Show dependency treegraph
 
Reported: 2023-06-21 14:07 UTC by Caroline Cyr La Rose
Modified: 2024-06-04 15:58 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 34077: Update output to match POD (1.43 KB, patch)
2023-10-17 11:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34077: Update output to match POD (1.66 KB, patch)
2023-12-05 16:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34077: Update output to match POD (1.72 KB, patch)
2023-12-18 21:43 UTC, Caroline Cyr La Rose
Details | Diff | Splinter Review
Bug 34077: Update output to match POD (1.73 KB, patch)
2024-02-06 12:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34077: Only print verbose output when asked for (1.57 KB, patch)
2024-02-06 12:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34077: Tidy the script (4.12 KB, patch)
2024-02-06 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34077: Improve documentation and error messages of writeoff_debts.pl (3.32 KB, patch)
2024-02-16 08:39 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 34077: Allow to use --category-code as the only filter option (980 bytes, patch)
2024-02-16 08:39 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 34077: Improve documentation and error messages of writeoff_debts.pl (3.37 KB, patch)
2024-06-04 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34077: Allow to use --category-code as the only filter option (1.02 KB, patch)
2024-06-04 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.
Comment 18 Martin Renvoize 2024-06-04 15:58:09 UTC
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>
Comment 19 Martin Renvoize 2024-06-04 15:58:12 UTC
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>
Comment 20 Martin Renvoize 2024-06-04 15:58:45 UTC
Thanks for taking over here Julian.. I was a bit burnt out.

I appreciate the help.

Passing QA