Bug 17894 - Remove and Replace WriteOffFee
Summary: Remove and Replace WriteOffFee
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15894
  Show dependency treegraph
 
Reported: 2017-01-12 18:48 UTC by Kyle M Hall
Modified: 2017-12-07 22:17 UTC (History)
2 users (show)

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


Attachments
Bug 17894 - Update pay() and use it internally for WriteOffFee (5.91 KB, patch)
2017-01-12 18:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17894 - Add unit tests (2.17 KB, patch)
2017-01-12 18:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17894 - Remove and replace WriteOffFee (6.69 KB, patch)
2017-01-12 18:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17894 - Update pay() and use it internally for WriteOffFee (5.97 KB, patch)
2017-01-13 06:26 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17894 - Add unit tests (2.23 KB, patch)
2017-01-13 06:26 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17894 - Remove and replace WriteOffFee (6.76 KB, patch)
2017-01-13 06:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17894 - Update pay() and use it internally for WriteOffFee (6.02 KB, patch)
2017-01-19 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17894 - Add unit tests (2.28 KB, patch)
2017-01-19 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17894 - Remove and replace WriteOffFee (6.81 KB, patch)
2017-01-19 09:12 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 Kyle M Hall 2017-01-12 18:48:22 UTC
WriteOffFee is the last of the "payment" subroutines that need to be merged into Koha::Account::pay ( as a writeoff is really just a special case payment ).
Comment 1 Kyle M Hall 2017-01-12 18:57:29 UTC
Created attachment 58883 [details] [review]
Bug 17894 - Update pay() and use it internally for WriteOffFee
Comment 2 Kyle M Hall 2017-01-12 18:57:35 UTC
Created attachment 58884 [details] [review]
Bug 17894 - Add unit tests
Comment 3 Kyle M Hall 2017-01-12 18:57:38 UTC
Created attachment 58885 [details] [review]
Bug 17894 - Remove and replace WriteOffFee

WriteOffFee is the last of the "payment" subroutines that need to be
merged into Koha::Account::pay ( as a writeoff is really just type of
payment ).

Test Plan:
1) Apply this patch
2) Verify the writeoff, and writeoff all buttons still work
Comment 4 Josef Moravec 2017-01-13 06:26:36 UTC
Created attachment 58908 [details] [review]
[SIGNED-OFF] Bug 17894 - Update pay() and use it internally for WriteOffFee

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 5 Josef Moravec 2017-01-13 06:26:43 UTC
Created attachment 58909 [details] [review]
[SIGNED-OFF] Bug 17894 - Add unit tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 6 Josef Moravec 2017-01-13 06:26:49 UTC
Created attachment 58910 [details] [review]
[SIGNED-OFF] Bug 17894 - Remove and replace WriteOffFee

WriteOffFee is the last of the "payment" subroutines that need to be
merged into Koha::Account::pay ( as a writeoff is really just type of
payment ).

Test Plan:
1) Apply this patch
2) Verify the writeoff, and writeoff all buttons still work

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 7 Jonathan Druart 2017-01-17 08:56:32 UTC
Before this patch, the action logged contained the itemnumber, not now.
Would it make sense to keep it?
We have the accountlines_id, so we could get the itemnumber from it. But it could be handy to have it directly.
I am not sure this is relevant but I prefer to ask.
Comment 8 Kyle M Hall 2017-01-18 15:17:32 UTC
(In reply to Jonathan Druart from comment #7)
> Before this patch, the action logged contained the itemnumber, not now.
> Would it make sense to keep it?
> We have the accountlines_id, so we could get the itemnumber from it. But it
> could be handy to have it directly.
> I am not sure this is relevant but I prefer to ask.

After serious consideration, I think we should *not* be logging the itemnumber directly . This was actually making the logs messier. It is predicated on the assumption that a single writeoff action only affects a single fee. Basically, a "write off all" action was indistinguishable to making many individual writeoffs in the way it was coded. Now the writeoff all action works in a more sensible manner, it makes no sense to have an itemnumber key. For these reasons I recommend do not add itemnumber directly.
Comment 9 Jonathan Druart 2017-01-19 09:12:47 UTC
Created attachment 59211 [details] [review]
Bug 17894 - Update pay() and use it internally for WriteOffFee

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

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 10 Jonathan Druart 2017-01-19 09:12:52 UTC
Created attachment 59212 [details] [review]
Bug 17894 - Add unit tests

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

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 11 Jonathan Druart 2017-01-19 09:12:56 UTC
Created attachment 59213 [details] [review]
Bug 17894 - Remove and replace WriteOffFee

WriteOffFee is the last of the "payment" subroutines that need to be
merged into Koha::Account::pay ( as a writeoff is really just type of
payment ).

Test Plan:
1) Apply this patch
2) Verify the writeoff, and writeoff all buttons still work

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

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 12 Kyle M Hall 2017-01-19 11:15:10 UTC
Pushed to master for 17.05!
Comment 13 Katrin Fischer 2017-01-22 16:54:40 UTC
This won't get ported back to 16.11.x as it is an enhancement.