Bug 20997

Summary: Add Koha::Account::Line::apply
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, josef.moravec, katrin.fischer, kyle, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18580
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 20978    
Bug Blocks: 20944, 21607, 21896    
Attachments: Bug 20997: Add unit tests for Koha::Account::Line::apply
Bug 20997: Add Koha::Account::Line::apply method
Bug 20997: Add unit tests for Koha::Account::Line::apply
Bug 20997: Add Koha::Account::Line::apply method
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method
Bug 20997: Add unit tests for Koha::Account::Line::apply
Bug 20997: Add Koha::Account::Line::apply method
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method
Bug 20997: Add new offset type
Bug 20997: ->apply should be applied to Koha::Account::Lines
Bug 20997: (follow-up) Minor POD fixes
Bug 20997: ->apply should be applied to Koha::Account::Lines
Bug 20997: (follow-up) Minor POD fixes
Bug 20997: Add unit tests for Koha::Account::Line::apply
Bug 20997: Add Koha::Account::Line::apply method
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method
Bug 20997: Add new offset type
Bug 20997: ->apply should be applied to Koha::Account::Lines
Bug 20997: (follow-up) Minor POD fixes
Bug 20997: (QA follow-up) POD fixes and offset type consistency
Bug 20997: Add unit tests for Koha::Account::Line::apply
Bug 20997: Add Koha::Account::Line::apply method
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method
Bug 20997: Add new offset type
Bug 20997: ->apply should be applied to Koha::Account::Lines
Bug 20997: (follow-up) Minor POD fixes
Bug 20997: (QA follow-up) POD fixes and offset type consistency
Bug 20997: Replace missing credit_applied with 'Credit Applied'

Description Tomás Cohen Arazi 2018-06-26 14:51:15 UTC
There should be a method to apply a credit to a debit.
Comment 1 Tomás Cohen Arazi 2018-06-26 14:56:18 UTC
Created attachment 76451 [details] [review]
Bug 20997: Add unit tests for Koha::Account::Line::apply

This patch adds unit tests for the mentioned method.

To test:
- Apply this patch
- Run:
  $ khell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> FAIL: The method is not already implemented!
Comment 2 Tomás Cohen Arazi 2018-06-26 14:56:30 UTC
Created attachment 76452 [details] [review]
Bug 20997: Add Koha::Account::Line::apply method

This patch implements the ->apply() method for account lines.
It will only work on credit lines, and raise an exception otherwise.

It implements ->is_credit() as well.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> SUCCESS: Tests pass!
- Check all use cases are covered by the tests
- Sign off :-D
Comment 3 Josef Moravec 2018-06-26 16:59:43 UTC
Created attachment 76459 [details] [review]
Bug 20997: Add unit tests for Koha::Account::Line::apply

This patch adds unit tests for the mentioned method.

To test:
- Apply this patch
- Run:
  $ khell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> FAIL: The method is not already implemented!

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 4 Josef Moravec 2018-06-26 16:59:51 UTC
Created attachment 76460 [details] [review]
Bug 20997: Add Koha::Account::Line::apply method

This patch implements the ->apply() method for account lines.
It will only work on credit lines, and raise an exception otherwise.

It implements ->is_credit() as well.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> SUCCESS: Tests pass!
- Check all use cases are covered by the tests
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 5 Josef Moravec 2018-06-26 17:13:44 UTC
Created attachment 76461 [details] [review]
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method

Test plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Account/Lines.t
=> Test should pass
Comment 6 Tomás Cohen Arazi 2018-06-26 17:24:41 UTC
Created attachment 76462 [details] [review]
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method

Test plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Account/Lines.t
=> Test should pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Kyle M Hall 2018-06-26 17:38:12 UTC
Created attachment 76463 [details] [review]
Bug 20997: Add unit tests for Koha::Account::Line::apply

This patch adds unit tests for the mentioned method.

To test:
- Apply this patch
- Run:
  $ khell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> FAIL: The method is not already implemented!

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2018-06-26 17:38:20 UTC
Created attachment 76464 [details] [review]
Bug 20997: Add Koha::Account::Line::apply method

This patch implements the ->apply() method for account lines.
It will only work on credit lines, and raise an exception otherwise.

It implements ->is_credit() as well.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> SUCCESS: Tests pass!
- Check all use cases are covered by the tests
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Kyle M Hall 2018-06-26 17:38:23 UTC
Created attachment 76465 [details] [review]
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method

Test plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Account/Lines.t
=> Test should pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Tomás Cohen Arazi 2018-06-28 19:00:43 UTC
Created attachment 76549 [details] [review]
Bug 20997: Add new offset type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2018-06-28 19:00:47 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2018-06-28 19:00:51 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2018-07-03 15:30:21 UTC
Created attachment 76654 [details] [review]
Bug 20997: ->apply should be applied to Koha::Account::Lines

This patch makes ->apply get passed an object set class instead of
single account lines.

The change is fully covered by tests.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2018-07-03 15:30:36 UTC
Created attachment 76655 [details] [review]
Bug 20997: (follow-up) Minor POD fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2018-07-03 15:32:06 UTC
Testers: I've rebased so it applies after some dependencies got changes. I also fixed the Koha::Account::Lines param move and tests.
Comment 16 Jonathan Druart 2018-07-23 18:49:26 UTC
CONFLICT (content): Merge conflict in t/db_dependent/Koha/Account/Lines.t
Comment 17 Tomás Cohen Arazi 2018-07-23 18:56:44 UTC
Created attachment 77194 [details] [review]
Bug 20997: Add unit tests for Koha::Account::Line::apply

This patch adds unit tests for the mentioned method.

To test:
- Apply this patch
- Run:
  $ khell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> FAIL: The method is not already implemented!

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Tomás Cohen Arazi 2018-07-23 18:56:50 UTC
Created attachment 77195 [details] [review]
Bug 20997: Add Koha::Account::Line::apply method

This patch implements the ->apply() method for account lines.
It will only work on credit lines, and raise an exception otherwise.

It implements ->is_credit() as well.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> SUCCESS: Tests pass!
- Check all use cases are covered by the tests
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Tomás Cohen Arazi 2018-07-23 18:56:56 UTC
Created attachment 77196 [details] [review]
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method

Test plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Account/Lines.t
=> Test should pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Tomás Cohen Arazi 2018-07-23 18:57:03 UTC
Created attachment 77197 [details] [review]
Bug 20997: Add new offset type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2018-07-23 18:57:09 UTC
Created attachment 77198 [details] [review]
Bug 20997: ->apply should be applied to Koha::Account::Lines

This patch makes ->apply get passed an object set class instead of
single account lines.

The change is fully covered by tests.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2018-07-23 18:57:17 UTC
Created attachment 77199 [details] [review]
Bug 20997: (follow-up) Minor POD fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Jonathan Druart 2018-07-23 19:38:45 UTC
Just minor (not blocker), reading the code:

1. In Koha::Account::Line->apply
+    my $offset_type = $params->{offset_type} // 'credit_applied';

Should not we raise an exception if the offset_type is not known?

I would also list the possible values in the POD (or at list tell it comes from 
account_offset_types)

2. I think we should follow the pattern "Credit Applied", instead of switching to snake_case "credit_applied" only for this new one.

3. is_debit, is_credit - what about amount == 0?
Comment 24 Tomás Cohen Arazi 2018-07-24 02:06:04 UTC
(In reply to Jonathan Druart from comment #23)
> Just minor (not blocker), reading the code:
> 
> 1. In Koha::Account::Line->apply
> +    my $offset_type = $params->{offset_type} // 'credit_applied';
> 
> Should not we raise an exception if the offset_type is not known?

This method applies a credit to debits. The idea was to make it simple to use, with documented defaults. There are two use cases in which I'm sure offset_type will not be 'credit_applied':
- Payments: if we rewrite ->pay using this, then the offset type would be 'payment' to keep the current behaviour.
- Write-offs: if we are writing off a debit, then 'writeoff' should be used.

IThe work collecting the existing hardcoded options is already done so should be easy. If you consider it a blocker, I'll do it as a follow-up. Otherwise, I will happily do it on a separate bug.

> I would also list the possible values in the POD (or at list tell it comes
> from 
> account_offset_types)

I agree.

> 2. I think we should follow the pattern "Credit Applied", instead of
> switching to snake_case "credit_applied" only for this new one.

Well, I wanted to deal with switching to snake_case all of them on a separate bug, and I expected Katrin to fail this one for this case issue. I can fix it as you like if you're QAing it.

> 3. is_debit, is_credit - what about amount == 0?

I only added is_credit he, blame Josef. Seriously, an account line with amount = 0 only happens when a credit is void. And void credits are not credits as we handle them. If we need methods for finding void credits specifically we could add them, but that'd be another bug, when there's a use case for that.
Comment 25 Tomás Cohen Arazi 2018-07-27 17:57:24 UTC
Created attachment 77310 [details] [review]
Bug 20997: (QA follow-up) POD fixes and offset type consistency

This patch addresses minor issues highlighted by QA.
- The POD is enhanced based on the proposal in bug 11983.
- The introduced offset type value is made consistent with the rest of
them.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Jonathan Druart 2018-07-31 19:47:08 UTC
Created attachment 77349 [details] [review]
Bug 20997: Add unit tests for Koha::Account::Line::apply

This patch adds unit tests for the mentioned method.

To test:
- Apply this patch
- Run:
  $ khell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> FAIL: The method is not already implemented!

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2018-07-31 19:47:12 UTC
Created attachment 77350 [details] [review]
Bug 20997: Add Koha::Account::Line::apply method

This patch implements the ->apply() method for account lines.
It will only work on credit lines, and raise an exception otherwise.

It implements ->is_credit() as well.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> SUCCESS: Tests pass!
- Check all use cases are covered by the tests
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2018-07-31 19:47:17 UTC
Created attachment 77351 [details] [review]
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method

Test plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Account/Lines.t
=> Test should pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2018-07-31 19:47:21 UTC
Created attachment 77352 [details] [review]
Bug 20997: Add new offset type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2018-07-31 19:47:25 UTC
Created attachment 77353 [details] [review]
Bug 20997: ->apply should be applied to Koha::Account::Lines

This patch makes ->apply get passed an object set class instead of
single account lines.

The change is fully covered by tests.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Jonathan Druart 2018-07-31 19:47:30 UTC
Created attachment 77354 [details] [review]
Bug 20997: (follow-up) Minor POD fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 32 Jonathan Druart 2018-07-31 19:47:34 UTC
Created attachment 77355 [details] [review]
Bug 20997: (QA follow-up) POD fixes and offset type consistency

This patch addresses minor issues highlighted by QA.
- The POD is enhanced based on the proposal in bug 11983.
- The introduced offset type value is made consistent with the rest of
them.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 33 Jonathan Druart 2018-07-31 19:47:38 UTC
Created attachment 77356 [details] [review]
Bug 20997: Replace missing credit_applied with 'Credit Applied'

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 34 Nick Clemens 2018-08-10 12:57:23 UTC
Awesome work all!

Pushed to master for 18.11
Comment 35 Martin Renvoize 2018-08-13 08:33:23 UTC
I'm reserving judgement on whether to backport this one until 20944 is pushed. My gut feeling is that its dependency bug contains too internal api change and is not justified by the benefits backporting 20944 would bring.