Bug 29757

Summary: Add filter_by_non_reversible/reversible methods for offsets
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize (ashimema) <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, kyle, m.de.rooy
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00
Circulation function:
Bug Depends on:    
Bug Blocks: 28656, 29759    
Attachments: Bug 29757: Unit tests
Bug 29757: Add filters for reversable offsets
Bug 29757: (QA follow-up) POD fixes
Bug 29757: Unit tests
Bug 29757: Add filters for reversable offsets
Bug 29757: (QA follow-up) POD fixes
Bug 29757: Unit tests
Bug 29757: Add filters for reversable offsets
Bug 29757: (QA follow-up) POD fixes
Bug 29757: (QA follow-up) Remove abs from the test

Description Tomás Cohen Arazi (tcohen) 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 (tcohen) 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 (tcohen) 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 (tcohen) 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 (khall) 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 (khall) 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 (khall) 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 (ashimema) 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 (ashimema) 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 (ashimema) 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 (ashimema) 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 (tcohen) 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 (tcohen) 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 (tcohen) 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 ﷐[U+1F984]﷑