There should be a method to apply a credit to a debit.
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!
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
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>
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>
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
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>
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>
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>
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>
Created attachment 76549 [details] [review] Bug 20997: Add new offset type Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 76550 [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>
Created attachment 76551 [details] [review] Bug 20997: (follow-up) Minor POD fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
Created attachment 76655 [details] [review] Bug 20997: (follow-up) Minor POD fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Testers: I've rebased so it applies after some dependencies got changes. I also fixed the Koha::Account::Lines param move and tests.
CONFLICT (content): Merge conflict in t/db_dependent/Koha/Account/Lines.t
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>
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>
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>
Created attachment 77197 [details] [review] Bug 20997: Add new offset type Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
Created attachment 77199 [details] [review] Bug 20997: (follow-up) Minor POD fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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?
(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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
Awesome work all! Pushed to master for 18.11
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.