Bug 29757 - Add filter_by_non_reversible/reversible methods for offsets
Summary: Add filter_by_non_reversible/reversible methods for offsets
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: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 28656 29759
  Show dependency treegraph
 
Reported: 2021-12-22 18:05 UTC by Tomás Cohen Arazi
Modified: 2022-12-12 21:24 UTC (History)
3 users (show)

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


Attachments
Bug 29757: Unit tests (2.06 KB, patch)
2021-12-22 19:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29757: Add filters for reversable offsets (2.11 KB, patch)
2021-12-22 19:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29757: (QA follow-up) POD fixes (3.85 KB, patch)
2021-12-22 19:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29757: Unit tests (2.12 KB, patch)
2022-01-07 13:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29757: Add filters for reversable offsets (2.17 KB, patch)
2022-01-07 13:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29757: (QA follow-up) POD fixes (3.91 KB, patch)
2022-01-07 13:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29757: Unit tests (2.18 KB, patch)
2022-01-07 14:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29757: Add filters for reversable offsets (2.23 KB, patch)
2022-01-07 14:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29757: (QA follow-up) POD fixes (3.97 KB, patch)
2022-01-07 14:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29757: (QA follow-up) Remove abs from the test (1.16 KB, patch)
2022-01-11 14:33 UTC, Marcel de Rooy
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 2021-12-22 18:05:16 UTC
Taking this methods out of bug 28656 for reusing them, with Martin's approval.
Comment 1 Tomás Cohen Arazi 2021-12-22 19:55:55 UTC
Created attachment 128865 [details] [review]
Bug 29757: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-12-22 19:56:01 UTC
Created attachment 128866 [details] [review]
Bug 29757: Add filters for reversable offsets

This patch adds filtering methods for (non)reversable offsets.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Account/Offsets.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2021-12-22 19:56:05 UTC
Created attachment 128867 [details] [review]
Bug 29757: (QA follow-up) POD fixes

This patch adds POD, fixes the existing one in the file, and also
renames reversable => reversible as required by the QA scripts.

If this name change is problematic, I can revert it and resubmit my
follow-up.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Kyle M Hall 2022-01-07 13:07:27 UTC
Created attachment 129150 [details] [review]
Bug 29757: Unit tests

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Kyle M Hall 2022-01-07 13:07:51 UTC
Created attachment 129151 [details] [review]
Bug 29757: Add filters for reversable offsets

This patch adds filtering methods for (non)reversable offsets.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Account/Offsets.t
=> SUCCESS: Tests pass!
3. Sign off :-D

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Kyle M Hall 2022-01-07 13:07:54 UTC
Created attachment 129152 [details] [review]
Bug 29757: (QA follow-up) POD fixes

This patch adds POD, fixes the existing one in the file, and also
renames reversable => reversible as required by the QA scripts.

If this name change is problematic, I can revert it and resubmit my
follow-up.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Martin Renvoize 2022-01-07 14:28:41 UTC
Created attachment 129178 [details] [review]
Bug 29757: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2022-01-07 14:28:45 UTC
Created attachment 129179 [details] [review]
Bug 29757: Add filters for reversable offsets

This patch adds filtering methods for (non)reversable offsets.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Account/Offsets.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2022-01-07 14:28:49 UTC
Created attachment 129180 [details] [review]
Bug 29757: (QA follow-up) POD fixes

This patch adds POD, fixes the existing one in the file, and also
renames reversable => reversible as required by the QA scripts.

If this name change is problematic, I can revert it and resubmit my
follow-up.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2022-01-07 14:29:13 UTC
All looks good to me, adding my accounts hat stamp here too.
Comment 11 Marcel de Rooy 2022-01-10 12:49:41 UTC
> Bug 29757: Add filters for reversable offsets

https://whichiscorrect.com/reversible-or-reversable/
Comment 12 Tomás Cohen Arazi 2022-01-10 13:06:10 UTC
(In reply to Marcel de Rooy from comment #11)
> > Bug 29757: Add filters for reversable offsets
> 
> https://whichiscorrect.com/reversible-or-reversable/

Right, the original implementation used reversable. I think I forgot to correct the commit subject
Comment 13 Marcel de Rooy 2022-01-10 15:22:14 UTC
Still looking here a bit.
Comment 14 Marcel de Rooy 2022-01-11 14:02:29 UTC
What I still have trouble with here most, is that the two filter methods have a generic name but they relate to debits.

We could use:
my $total_reversible = $debit->debit_offsets->filter_by_reversible->total;

But what if we want to filter credits?
my $total_reversible = $credit->credit_offsets->filter_by_reversible->total;
That would make no sense; the code refers to credit_id and amount < 0.

So, in other words, does the name really reflect what we are doing?
Comment 15 Tomás Cohen Arazi 2022-01-11 14:05:46 UTC
(In reply to Marcel de Rooy from comment #14)
> What I still have trouble with here most, is that the two filter methods
> have a generic name but they relate to debits.
> 
> We could use:
> my $total_reversible = $debit->debit_offsets->filter_by_reversible->total;
> 
> But what if we want to filter credits?
> my $total_reversible = $credit->credit_offsets->filter_by_reversible->total;
> That would make no sense; the code refers to credit_id and amount < 0.
> 
> So, in other words, does the name really reflect what we are doing?

I agree.

Martin and I have been talking about subclassing Koha::Acount::Line(s) so we have proper classes for debits and credits. In that case, we could move those methods there. I'm fine with a method name change. I ran out of ideas, though.
Comment 16 Marcel de Rooy 2022-01-11 14:33:05 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2022-01-11 14:33:29 UTC Comment hidden (obsolete)
Comment 18 Fridolin Somers 2022-01-22 07:17:45 UTC
The only difference between the two methods is "-not_in".
Could be make a private method called by those two to avoid duplicate code ?
Comment 19 Marcel de Rooy 2022-01-24 07:08:53 UTC
(In reply to Fridolin Somers from comment #18)
> The only difference between the two methods is "-not_in".
> Could be make a private method called by those two to avoid duplicate code ?

Good idea.
Comment 20 Fridolin Somers 2022-01-28 20:39:34 UTC
Martin, on it ?
Comment 21 Tomás Cohen Arazi 2022-01-29 01:56:20 UTC
(In reply to Fridolin Somers from comment #18)
> The only difference between the two methods is "-not_in".
> Could be make a private method called by those two to avoid duplicate code ?

I generally agree, but it is a really small query... And generalizing it might end up obfuscating the code with not much gain.
Comment 22 Fridolin Somers 2022-01-29 06:27:26 UTC
OK gain is really small.
Comment 23 Fridolin Somers 2022-01-29 07:54:14 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄