Bug 21720 - Update C4::Circulation::AddIssuingCharge to use Koha::Account->add_debit
Summary: Update C4::Circulation::AddIssuingCharge 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: 21728 21999
Blocks: 21721
  Show dependency treegraph
 
Reported: 2018-10-30 09:27 UTC by Martin Renvoize
Modified: 2019-06-27 09:24 UTC (History)
3 users (show)

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


Attachments
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge (2.09 KB, patch)
2018-11-02 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge (2.23 KB, patch)
2018-11-30 13:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21720: Update Tests (2.17 KB, patch)
2018-11-30 13:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge (2.44 KB, patch)
2018-12-14 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21720: Update Tests (2.17 KB, patch)
2018-12-14 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge (2.49 KB, patch)
2019-01-22 08:08 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21720: Update Tests (2.23 KB, patch)
2019-01-22 08:08 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge (2.54 KB, patch)
2019-02-15 14:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21720: Update Tests (2.28 KB, patch)
2019-02-15 14:26 UTC, Tomás Cohen Arazi
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 09:27:24 UTC
All account handling should go via the Koha::Account class.
Comment 1 Martin Renvoize 2018-11-02 11:05:47 UTC
Created attachment 81882 [details] [review]
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge
Comment 2 Martin Renvoize 2018-11-30 13:36:12 UTC
Created attachment 82787 [details] [review]
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge
Comment 3 Martin Renvoize 2018-11-30 13:36:15 UTC
Created attachment 82788 [details] [review]
Bug 21720: Update Tests

AddIssuing charge is always called in void context, but the tests
checked that the return value was a Koha::Account::Offset and we now
return a Koha::Account::Line.

This patch changes the test to accept a Koha::Account::Line as returned,
adds a test to ensure a Koha::Account::Offset is also created and
corrects the logic that tests that only one accountline is added.
Comment 4 Martin Renvoize 2018-12-14 08:36:26 UTC
Created attachment 83199 [details] [review]
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge

Known Side Effect: Prior to this patch issuing charges were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_rent`.

Sponsored-by: PTFS Europe
Comment 5 Martin Renvoize 2018-12-14 08:36:29 UTC
Created attachment 83200 [details] [review]
Bug 21720: Update Tests

AddIssuing charge is always called in void context, but the tests
checked that the return value was a Koha::Account::Offset and we now
return a Koha::Account::Line.

This patch changes the test to accept a Koha::Account::Line as returned,
adds a test to ensure a Koha::Account::Offset is also created and
corrects the logic that tests that only one accountline is added.
Comment 6 Martin Renvoize 2018-12-16 09:35:31 UTC
QA Question: Regarding the side effect, this is reasonably easy to avert within the add_debit method but I'd like input as to whether the FinesLog should really be just about Fines or should include all Charges.. as it stands it's a bit of a mix of both.. this patch series turns it into more of the ChargesLog but upping the logging but it could alternatively go the other way and make what is recorded a bit more explicitly about fines and only fines.
Comment 7 Josef Moravec 2019-01-22 07:30:16 UTC
(In reply to Martin Renvoize from comment #6)
> QA Question: Regarding the side effect, this is reasonably easy to avert
> within the add_debit method but I'd like input as to whether the FinesLog
> should really be just about Fines or should include all Charges.. as it
> stands it's a bit of a mix of both.. this patch series turns it into more of
> the ChargesLog but upping the logging but it could alternatively go the
> other way and make what is recorded a bit more explicitly about fines and
> only fines.

I would prefer to log all charges, not only fines.
Comment 8 Josef Moravec 2019-01-22 08:08:47 UTC
Created attachment 84271 [details] [review]
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge

Known Side Effect: Prior to this patch issuing charges were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_rent`.

Sponsored-by: PTFS Europe

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 9 Josef Moravec 2019-01-22 08:08:50 UTC
Created attachment 84272 [details] [review]
Bug 21720: Update Tests

AddIssuing charge is always called in void context, but the tests
checked that the return value was a Koha::Account::Offset and we now
return a Koha::Account::Line.

This patch changes the test to accept a Koha::Account::Line as returned,
adds a test to ensure a Koha::Account::Offset is also created and
corrects the logic that tests that only one accountline is added.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Tomás Cohen Arazi 2019-02-15 14:26:46 UTC
Created attachment 85175 [details] [review]
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge

Known Side Effect: Prior to this patch issuing charges were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_rent`.

Sponsored-by: PTFS Europe

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2019-02-15 14:26:50 UTC
Created attachment 85176 [details] [review]
Bug 21720: Update Tests

AddIssuing charge is always called in void context, but the tests
checked that the return value was a Koha::Account::Offset and we now
return a Koha::Account::Line.

This patch changes the test to accept a Koha::Account::Line as returned,
adds a test to ensure a Koha::Account::Offset is also created and
corrects the logic that tests that only one accountline is added.

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

Pushed to master for 19.05
Comment 13 Martin Renvoize 2019-03-05 15:18:50 UTC
Enhancement will not be backported to the stable branch.