Bug 21728 - Update C4::Reserves::ChargeReserveFee to use Koha::Account->add_debit
Summary: Update C4::Reserves::ChargeReserveFee to use Koha::Account->add_debit
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 21722
Blocks: 21720
  Show dependency treegraph
 
Reported: 2018-10-30 19:04 UTC by Martin Renvoize
Modified: 2019-06-27 09:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee (1.57 KB, patch)
2018-11-01 11:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee (1.57 KB, patch)
2018-11-02 10:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee (1.81 KB, patch)
2018-11-30 19:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21728 - Add 'Reserve Fee' to the account_offset_types (1.11 KB, patch)
2018-11-30 19:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee (1.87 KB, patch)
2019-01-22 09:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21728: Add 'Reserve Fee' to the account_offset_types (1.17 KB, patch)
2019-01-22 09:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21728: Use Koha::Account->add_debit in ChargeReserveFee (1.92 KB, patch)
2019-02-15 14:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21728: Add 'Reserve Fee' to the account_offset_types (1.21 KB, patch)
2019-02-15 14:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21728: (RM follow-up) Add 'Reserve Fee' for new installs (761 bytes, patch)
2019-03-04 20:27 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.