Bug 20997 - Add Koha::Account::Line::apply
Summary: Add Koha::Account::Line::apply
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 20978
Blocks: 20944 21607 21896
  Show dependency treegraph
 
Reported: 2018-06-26 14:51 UTC by Tomás Cohen Arazi
Modified: 2020-06-04 20:32 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 20997: Add unit tests for Koha::Account::Line::apply (6.70 KB, patch)
2018-06-26 14:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: Add Koha::Account::Line::apply method (5.50 KB, patch)
2018-06-26 14:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: Add unit tests for Koha::Account::Line::apply (6.75 KB, patch)
2018-06-26 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20997: Add Koha::Account::Line::apply method (5.56 KB, patch)
2018-06-26 16:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method (2.21 KB, patch)
2018-06-26 17:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method (2.27 KB, patch)
2018-06-26 17:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: Add unit tests for Koha::Account::Line::apply (6.83 KB, patch)
2018-06-26 17:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20997: Add Koha::Account::Line::apply method (5.64 KB, patch)
2018-06-26 17:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method (2.35 KB, patch)
2018-06-26 17:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20997: Add new offset type (2.09 KB, patch)
2018-06-28 19:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: ->apply should be applied to Koha::Account::Lines (7.87 KB, patch)
2018-06-28 19:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: (follow-up) Minor POD fixes (1.24 KB, patch)
2018-06-28 19:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: ->apply should be applied to Koha::Account::Lines (9.46 KB, patch)
2018-07-03 15:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: (follow-up) Minor POD fixes (1.03 KB, patch)
2018-07-03 15:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: Add unit tests for Koha::Account::Line::apply (4.80 KB, patch)
2018-07-23 18:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: Add Koha::Account::Line::apply method (5.61 KB, patch)
2018-07-23 18:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method (2.35 KB, patch)
2018-07-23 18:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: Add new offset type (2.09 KB, patch)
2018-07-23 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: ->apply should be applied to Koha::Account::Lines (9.46 KB, patch)
2018-07-23 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: (follow-up) Minor POD fixes (1.03 KB, patch)
2018-07-23 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: (QA follow-up) POD fixes and offset type consistency (3.53 KB, patch)
2018-07-27 17:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20997: Add unit tests for Koha::Account::Line::apply (4.88 KB, patch)
2018-07-31 19:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20997: Add Koha::Account::Line::apply method (5.70 KB, patch)
2018-07-31 19:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20997: (follow-up) Add Koha::Account::Line->is_debit method (2.42 KB, patch)
2018-07-31 19:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20997: Add new offset type (2.18 KB, patch)
2018-07-31 19:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20997: ->apply should be applied to Koha::Account::Lines (9.55 KB, patch)
2018-07-31 19:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20997: (follow-up) Minor POD fixes (1.10 KB, patch)
2018-07-31 19:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20997: (QA follow-up) POD fixes and offset type consistency (3.60 KB, patch)
2018-07-31 19:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20997: Replace missing credit_applied with 'Credit Applied' (1.03 KB, patch)
2018-07-31 19:47 UTC, Jonathan Druart
Details | Diff | Splinter Review

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