Bug 21747

Summary: Update C4::Overdues::UpdateFine to use Koha::Account->add_debit and Koha::Account::Line->adjust
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: andrewfh, jonathan.druart, josef.moravec, katrin.fischer, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17138
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 21002, 21727, 21753    
Bug Blocks: 21722    
Attachments: Bug 21747: Use Koha::Account:: routines in UpdateFine
Bug 21747: Use Koha::Account:: routines in UpdateFine
Bug 21747: Use Koha::Account:: routines in UpdateFine
Bug 21747: Use Koha::Account:: routines in UpdateFine
Bug 21747: Use Koha::Account:: routines in UpdateFine
Bug 21747: (follow-up) Intelligent rename of offsets
Bug 21747: Use Koha::Account:: routines in UpdateFine
Bug 21747: (follow-up) Intelligent rename of offsets
Bug 21747: Use Koha::Account:: routines in UpdateFine
Bug 21747: (follow-up) Intelligent rename of offsets
Bug 21747: Use Koha::Account:: routines in UpdateFine
Bug 21747: (follow-up) Intelligent rename of offsets
Bug 21747: Use Koha::Account:: routines in UpdateFine
Bug 21747: (follow-up) Intelligent rename of offsets
Bug 21747: Insert fine_increase and fine_decrease for new installs

Description Martin Renvoize 2018-11-01 14:09:49 UTC
All account handling should go via the Koha::Account class.
Comment 1 Martin Renvoize 2018-11-01 20:15:05 UTC
*** Bug 21748 has been marked as a duplicate of this bug. ***
Comment 2 Martin Renvoize 2018-11-02 11:29:26 UTC
Created attachment 81890 [details] [review]
Bug 21747: Use Koha::Account:: routines in UpdateFine

Set to use Koha::Account->add_debit and Koha::Account::Line->adjust
Comment 3 Josef Moravec 2018-12-12 15:21:47 UTC
Created attachment 83130 [details] [review]
Bug 21747: Use Koha::Account:: routines in UpdateFine

Set to use Koha::Account->add_debit and Koha::Account::Line->adjust

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 4 Martin Renvoize 2018-12-14 16:59:06 UTC
Created attachment 83234 [details] [review]
Bug 21747: Use Koha::Account:: routines in UpdateFine

Set to use Koha::Account->add_debit and Koha::Account::Line->adjust

Known Side Effect: Prior to this patch fines did not set an 'action' in
the FineLog when FinsLog is enabled. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_fine`.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 5 Martin Renvoize 2018-12-15 12:38:58 UTC
Created attachment 83252 [details] [review]
Bug 21747: Use Koha::Account:: routines in UpdateFine

Set to use Koha::Account->add_debit and Koha::Account::Line->adjust

Known Side Effect: The format of the FinesLog, if enabled, is changed
after this patch. Prior to this patch the $actionname was left undefined
and the $infos field contained the string:

`"due=".$due."  amount=".$amount." itemnumber=".$itemnum`

After this patch, the logs are more consistent with other FINES logs,
with an $actionname of 'CREATE' or 'UPDATE' and the $infos field
containing a Dumper of fine data.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 6 Tomás Cohen Arazi 2019-01-18 14:53:19 UTC
(In reply to Martin Renvoize from comment #5)
> Created attachment 83252 [details] [review] [review]
> Bug 21747: Use Koha::Account:: routines in UpdateFine
> 
> Set to use Koha::Account->add_debit and Koha::Account::Line->adjust
> 
> Known Side Effect: The format of the FinesLog, if enabled, is changed
> after this patch. Prior to this patch the $actionname was left undefined
> and the $infos field contained the string:
> 
> `"due=".$due."  amount=".$amount." itemnumber=".$itemnum`
> 
> After this patch, the logs are more consistent with other FINES logs,
> with an $actionname of 'CREATE' or 'UPDATE' and the $infos field
> containing a Dumper of fine data.
> 
> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

I think we need to consider the backdated returns use case, and have another offset for those, as fine_increment doesn't look right.
Comment 7 Tomás Cohen Arazi 2019-02-01 11:36:14 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> 
> I think we need to consider the backdated returns use case, and have another
> offset for those, as fine_increment doesn't look right.

What I meant here is that FU (Fine Update) should be splitted into 'fine_increment' and 'fine_decrement'. Not all 'fine_increment'. And in the case of backdated returns/dropbox mode we could pass a specific offset_type to ->adjust.
Comment 8 Katrin Fischer 2019-02-03 09:21:27 UTC
Moving to failed QA as of Tomas last comments.
Comment 9 Martin Renvoize 2019-02-04 14:40:46 UTC
Created attachment 84725 [details] [review]
Bug 21747: Use Koha::Account:: routines in UpdateFine

Set to use Koha::Account->add_debit and Koha::Account::Line->adjust

Known Side Effect: The format of the FinesLog, if enabled, is changed
after this patch. Prior to this patch the $actionname was left undefined
and the $infos field contained the string:

`"due=".$due."  amount=".$amount." itemnumber=".$itemnum`

After this patch, the logs are more consistent with other FINES logs,
with an $actionname of 'CREATE' or 'UPDATE' and the $infos field
containing a Dumper of fine data.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Martin Renvoize 2019-02-04 14:40:50 UTC
Created attachment 84726 [details] [review]
Bug 21747: (follow-up) Intelligent rename of offsets

This patch intelligently renames the account_offset types for updateing
fines from `Fine Update` to `fine_increment` and `fine_decrement`
depending on the sign of the calculated difference of the adjustment.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2019-02-06 08:28:34 UTC
Created attachment 84807 [details] [review]
Bug 21747: Use Koha::Account:: routines in UpdateFine

Set to use Koha::Account->add_debit and Koha::Account::Line->adjust

Known Side Effect: The format of the FinesLog, if enabled, is changed
after this patch. Prior to this patch the $actionname was left undefined
and the $infos field contained the string:

`"due=".$due."  amount=".$amount." itemnumber=".$itemnum`

After this patch, the logs are more consistent with other FINES logs,
with an $actionname of 'CREATE' or 'UPDATE' and the $infos field
containing a Dumper of fine data.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Martin Renvoize 2019-02-06 08:28:37 UTC
Created attachment 84808 [details] [review]
Bug 21747: (follow-up) Intelligent rename of offsets

This patch intelligently renames the account_offset types for updateing
fines from `Fine Update` to `fine_increment` and `fine_decrement`
depending on the sign of the calculated difference of the adjustment.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-02-06 11:58:48 UTC
Created attachment 84810 [details] [review]
Bug 21747: Use Koha::Account:: routines in UpdateFine

Set to use Koha::Account->add_debit and Koha::Account::Line->adjust

Known Side Effect: The format of the FinesLog, if enabled, is changed
after this patch. Prior to this patch the $actionname was left undefined
and the $infos field contained the string:

`"due=".$due."  amount=".$amount." itemnumber=".$itemnum`

After this patch, the logs are more consistent with other FINES logs,
with an $actionname of 'CREATE' or 'UPDATE' and the $infos field
containing a Dumper of fine data.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Martin Renvoize 2019-02-06 11:58:51 UTC
Created attachment 84811 [details] [review]
Bug 21747: (follow-up) Intelligent rename of offsets

This patch intelligently renames the account_offset types for updateing
fines from `Fine Update` to `fine_increment` and `fine_decrement`
depending on the sign of the calculated difference of the adjustment.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2019-02-06 12:01:40 UTC
I've implemented the follow-up as suggested now so that we use 'fine_update' as the 'type' to pass to the method and then 'fine_increment' or 'fine_decrement' for the two cases in the account_offsets table..

I'm not 100% sure 'increment' and 'decrement' are the clearest terms whilst we're here.. they may be clearer as 'increase' and 'decrease'... just a thought.
Comment 16 Martin Renvoize 2019-02-06 13:38:19 UTC
Created attachment 84816 [details] [review]
Bug 21747: Use Koha::Account:: routines in UpdateFine

Set to use Koha::Account->add_debit and Koha::Account::Line->adjust

Known Side Effect: The format of the FinesLog, if enabled, is changed
after this patch. Prior to this patch the $actionname was left undefined
and the $infos field contained the string:

`"due=".$due."  amount=".$amount." itemnumber=".$itemnum`

After this patch, the logs are more consistent with other FINES logs,
with an $actionname of 'CREATE' or 'UPDATE' and the $infos field
containing a Dumper of fine data.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 17 Martin Renvoize 2019-02-06 13:38:23 UTC
Created attachment 84817 [details] [review]
Bug 21747: (follow-up) Intelligent rename of offsets

This patch intelligently renames the account_offset types for updateing
fines from `Fine Update` to `fine_increment` and `fine_decrement`
depending on the sign of the calculated difference of the adjustment.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2019-02-06 13:39:02 UTC
Updated to use _increase and _decrease as Katrin agreed they're clearer terms for the average user.
Comment 19 Tomás Cohen Arazi 2019-02-15 15:16:05 UTC
Created attachment 85179 [details] [review]
Bug 21747: Use Koha::Account:: routines in UpdateFine

Set to use Koha::Account->add_debit and Koha::Account::Line->adjust

Known Side Effect: The format of the FinesLog, if enabled, is changed
after this patch. Prior to this patch the $actionname was left undefined
and the $infos field contained the string:

`"due=".$due."  amount=".$amount." itemnumber=".$itemnum`

After this patch, the logs are more consistent with other FINES logs,
with an $actionname of 'CREATE' or 'UPDATE' and the $infos field
containing a Dumper of fine data.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2019-02-15 15:16:08 UTC
Created attachment 85180 [details] [review]
Bug 21747: (follow-up) Intelligent rename of offsets

This patch intelligently renames the account_offset types for updateing
fines from `Fine Update` to `fine_increment` and `fine_decrement`
depending on the sign of the calculated difference of the adjustment.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Nick Clemens 2019-02-27 14:16:21 UTC
Awesome work all!

Pushed to master for 19.05
Comment 22 Jonathan Druart 2019-02-27 19:06:09 UTC
Created attachment 85795 [details] [review]
Bug 21747: Insert fine_increase and fine_decrease for new installs

This fixes Circulation.t
koha_1       | DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`account_offsets`, CONSTRAINT `account_offsets_ibfk_t` FOREIGN KEY (`type`) REFERENCES `account_offset_types` (`type`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `account_offsets` ( `amount`, `debit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=1, 1='22', 2='fine_increase'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Nick Clemens 2019-02-27 20:51:51 UTC
(In reply to Jonathan Druart from comment #22)
> Created attachment 85795 [details] [review] [review]
> Bug 21747: Insert fine_increase and fine_decrease for new installs
> 
> This fixes Circulation.t
> koha_1       | DBD::mysql::st execute failed: Cannot add or update a child
> row: a foreign key constraint fails (`koha_kohadev`.`account_offsets`,
> CONSTRAINT `account_offsets_ibfk_t` FOREIGN KEY (`type`) REFERENCES
> `account_offset_types` (`type`) ON DELETE CASCADE ON UPDATE CASCADE) [for
> Statement "INSERT INTO `account_offsets` ( `amount`, `debit_id`, `type`)
> VALUES ( ?, ?, ? )" with ParamValues: 0=1, 1='22', 2='fine_increase'] at
> /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Follow up pushed to master
Comment 24 Martin Renvoize 2019-03-01 11:42:58 UTC
Enhancement, will not be backported to 18.11.x series.
Comment 25 Martin Renvoize 2019-03-11 11:42:03 UTC
*** Bug 22374 has been marked as a duplicate of this bug. ***
Comment 26 Martin Renvoize 2020-01-30 13:09:13 UTC
*** Bug 24431 has been marked as a duplicate of this bug. ***