Bug 21728

Summary: Update C4::Reserves::ChargeReserveFee to use Koha::Account->add_debit
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: josef.moravec, katrin.fischer, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 21722    
Bug Blocks: 21720    
Attachments: Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee
Bug 21728 - Add 'Reserve Fee' to the account_offset_types
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee
Bug 21728: Add 'Reserve Fee' to the account_offset_types
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee
Bug 21728: Add 'Reserve Fee' to the account_offset_types
Bug 21728: (RM follow-up) Add 'Reserve Fee' for new installs

Description Martin Renvoize 2018-10-30 19:04:07 UTC
All account handling should go via the Koha::Account class.
Comment 1 Martin Renvoize 2018-11-01 11:12:08 UTC
Created attachment 81791 [details] [review]
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee
Comment 2 Martin Renvoize 2018-11-02 10:58:54 UTC
Created attachment 81881 [details] [review]
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee
Comment 3 Martin Renvoize 2018-11-30 19:48:00 UTC
Created attachment 82808 [details] [review]
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee
Comment 4 Martin Renvoize 2018-11-30 19:48:05 UTC
Created attachment 82809 [details] [review]
Bug 21728 - Add 'Reserve Fee' to the account_offset_types
Comment 5 Martin Renvoize 2018-11-30 19:50:42 UTC
Test plan:

1) Apply patches
2) Run updatedatabase.pl
3) Check that t/db_dependant/Reserves.t still passes
4) Check that t/db_dependant/Reservers/GetReserveFee.t still passes
Comment 6 Josef Moravec 2019-01-22 09:26:21 UTC
Created attachment 84278 [details] [review]
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 7 Josef Moravec 2019-01-22 09:26:24 UTC
Created attachment 84279 [details] [review]
Bug 21728: Add 'Reserve Fee' to the account_offset_types

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 8 Katrin Fischer 2019-01-22 14:48:50 UTC
Can we please fix terminology?

The first patch adds "Reserve charge" (Translatability?)
The second adds "Reserve fee". 

Not sure if it should be charge or fee, but it should be hold! :)
Comment 9 Martin Renvoize 2019-02-04 12:58:45 UTC
Hi Katrin...

So the first patch doesn't actually 'add' a new term but rather keeps the existing terminology.. and you are entirely correct that it's not translatable as it's stored in the database as data.  I personally would like to ditch the 'description' field entirely at some point and rather use code and then translatable descriptions at the template level further down the call path.. But I didn't want to introduce too many changes in one go (these descriptions may already be in use for reports for example).

The second patch is a 'code' and I thought matched current guidelines.. I can happily rename that one to `Hold Fee` though.. would that make sense?
Comment 10 Katrin Fischer 2019-02-04 16:39:03 UTC
(In reply to Martin Renvoize from comment #9)
> Hi Katrin...
> 
> So the first patch doesn't actually 'add' a new term but rather keeps the
> existing terminology.. and you are entirely correct that it's not
> translatable as it's stored in the database as data.  I personally would
> like to ditch the 'description' field entirely at some point and rather use
> code and then translatable descriptions at the template level further down
> the call path.. But I didn't want to introduce too many changes in one go
> (these descriptions may already be in use for reports for example).

My mistake here was that I thought it introduced the "code" - it doesn't. So I am fine with continuing with clean-up later. A lot of times it's better to get things right from the beginning, because things get much harder once people start using things (like you said with reports)

Description is less heavily used than it was in 3.0 or 3.2 these days as there was already an attempt to move it into templates. We should continue that and clean up all remaining (see bug 21953 in that context too)

> The second patch is a 'code' and I thought matched current guidelines.. I
> can happily rename that one to `Hold Fee` though.. would that make sense?

Guidelines say: terminology as GUI = so reserve should be hold and then there is capitalization... but current codes don't follow coding guidelines in some cases already - I will leave it up to you and Tomas. But as we are trying to get things right for API, maybe switching to Hold if easily possible would be good.
Comment 11 Tomás Cohen Arazi 2019-02-15 14:09:45 UTC
Created attachment 85171 [details] [review]
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2019-02-15 14:09:51 UTC
Created attachment 85172 [details] [review]
Bug 21728: Add 'Reserve Fee' to the account_offset_types

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Nick Clemens 2019-03-04 19:24:26 UTC
Awesome work all!

Pushed to master for 19.05
Comment 14 Nick Clemens 2019-03-04 20:27:01 UTC
Created attachment 86032 [details] [review]
Bug 21728: (RM follow-up) Add 'Reserve Fee' for new installs
Comment 15 Martin Renvoize 2019-03-05 15:18:13 UTC
Enhancement will not be backported to stable branch.