Taking this methods out of bug 28656 for reusing them, with Martin's approval.
Created attachment 128865 [details] [review] Bug 29757: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
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>
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>
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>
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>
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>
All looks good to me, adding my accounts hat stamp here too.
> Bug 29757: Add filters for reversable offsets https://whichiscorrect.com/reversible-or-reversable/
(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
Still looking here a bit.
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?
(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.
Created attachment 129296 [details] [review] Bug 29757: (QA follow-up) Remove abs from the test Obviously abs has two sides, and we could allow wrong results. We can be more precise here. Trivial exercise. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment on attachment 129296 [details] [review] Bug 29757: (QA follow-up) Remove abs from the test Oops
The only difference between the two methods is "-not_in". Could be make a private method called by those two to avoid duplicate code ?
(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.
Martin, on it ?
(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.
OK gain is really small.
Pushed to master for 22.05, thanks to everybody involved [U+1F984]