Bug 21720

Summary: Update C4::Circulation::AddIssuingCharge 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, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21678
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 21728, 21999    
Bug Blocks: 21721    
Attachments: Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge
Bug 21720: Update Tests
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge
Bug 21720: Update Tests
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge
Bug 21720: Update Tests
Bug 21720: Use Koha::Account->add_debit in AddIssuingCharge
Bug 21720: Update Tests

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 (kidclamp) 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.