Bug 24815 - Koha::Cash::Register relations should return sets not undef
Summary: Koha::Cash::Register relations should return sets not undef
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 23321 23355 24817
Blocks: 13985
  Show dependency treegraph
 
Reported: 2020-03-05 16:09 UTC by Martin Renvoize
Modified: 2020-11-30 21:48 UTC (History)
5 users (show)

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


Attachments
Bug 24815: Correct return values of Koha::Cash::Register relations (2.29 KB, patch)
2020-03-05 16:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24815: Correct return values of Koha::Cash::Register relations (1.96 KB, patch)
2020-03-06 09:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24815: Add additional tests (3.97 KB, patch)
2020-03-06 09:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24815: Correct return values of Koha::Cash::Register relations (1.96 KB, patch)
2020-03-26 20:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24815: Add additional tests (3.96 KB, patch)
2020-03-26 20:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24815: Correct return values of Koha::Cash::Register relations (2.01 KB, patch)
2020-04-23 10:33 UTC, David Nind
Details | Diff | Splinter Review
Bug 24815: Add additional tests (4.01 KB, patch)
2020-04-23 10:33 UTC, David Nind
Details | Diff | Splinter Review
Bug 24815: Correct return values of Koha::Cash::Register relations (2.01 KB, patch)
2020-04-27 13:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24815: Add additional tests (3.99 KB, patch)
2020-04-27 13:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24815: (follow-up) Rebase correction (1.14 KB, patch)
2020-04-27 13:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24815: Correct return values of Koha::Cash::Register relations (2.08 KB, patch)
2020-04-27 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24815: Add additional tests (3.75 KB, patch)
2020-04-27 13:22 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 Martin Renvoize 2020-03-05 16:09:16 UTC
The cash register class contains a bad pattern of returning 'undef' for relations instead of an empty resultset.
Comment 1 Martin Renvoize 2020-03-05 16:13:14 UTC
Created attachment 100203 [details] [review]
Bug 24815: Correct return values of Koha::Cash::Register relations

This patch correct all cases of return undef to instead return an empty
resultset.
Comment 2 Jonathan Druart 2020-03-05 16:58:24 UTC
I will double check tomorrow but I think the following one is not correct:

@@ -86,7 +84,6 @@ sub last_cashup {
         { order_by => { '-desc' => [ 'timestamp', 'id' ] }, rows => 1 }
     )->single;
 
-    return unless $rs;
     return Koha::Cash::Register::Action->_new_from_dbic($rs);
 }
Comment 3 Tomás Cohen Arazi 2020-03-05 18:53:46 UTC
(In reply to Jonathan Druart from comment #2)
> I will double check tomorrow but I think the following one is not correct:
> 
> @@ -86,7 +84,6 @@ sub last_cashup {
>          { order_by => { '-desc' => [ 'timestamp', 'id' ] }, rows => 1 }
>      )->single;
>  
> -    return unless $rs;
>      return Koha::Cash::Register::Action->_new_from_dbic($rs);
>  }

You are right. We return undef on the singular case
Comment 4 Martin Renvoize 2020-03-06 09:04:15 UTC
Created attachment 100219 [details] [review]
Bug 24815: Correct return values of Koha::Cash::Register relations

This patch correct all cases of return undef to instead return an empty
resultset.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2020-03-06 09:04:18 UTC
Created attachment 100220 [details] [review]
Bug 24815: Add additional tests
Comment 6 Jon Knight 2020-03-11 18:09:24 UTC
Trying it on an up-to-date koha-testing-docker with a clean branch off the current origin/master the tests don't apply:

$ git bz apply  24815
Bug 24815 - Koha::Cash::Register relations should return sets not undef

100219 - Bug 24815: Correct return values of Koha::Cash::Register relations
100220 - Bug 24815: Add additional tests

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 24815: Correct return values of Koha::Cash::Register relations
Applying: Bug 24815: Add additional tests
Using index info to reconstruct a base tree...
M	t/db_dependent/Koha/Cash/Register.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Cash/Register.t
CONFLICT (content): Merge conflict in t/db_dependent/Koha/Cash/Register.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 24815: Add additional tests
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-24815-Add-additional-tests-XWf3eu.patch
Comment 7 Martin Renvoize 2020-03-26 20:42:01 UTC
Created attachment 101907 [details] [review]
Bug 24815: Correct return values of Koha::Cash::Register relations

This patch correct all cases of return undef to instead return an empty
resultset.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-03-26 20:42:04 UTC
Created attachment 101908 [details] [review]
Bug 24815: Add additional tests
Comment 9 Martin Renvoize 2020-03-26 20:42:46 UTC
Rebased, thanks for yesting Jon
Comment 10 David Nind 2020-04-23 10:33:51 UTC
Created attachment 103559 [details] [review]
Bug 24815: Correct return values of Koha::Cash::Register relations

This patch correct all cases of return undef to instead return an empty
resultset.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2020-04-23 10:33:54 UTC
Created attachment 103560 [details] [review]
Bug 24815: Add additional tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2020-04-23 10:37:41 UTC
Tested by:

1) Running tests before and after patch applied (prove t/db_dependent/Koha/Cash/Register.t) and made sure they pass.
2) Setting up a cash register, adding items for purchase, adding some transactions, running a cashup, searching for previous transactions <-- all seemed to work as expected.

Hope that this is sufficent for testing!
Comment 13 Jonathan Druart 2020-04-24 08:14:05 UTC
Comment on attachment 103560 [details] [review]
Bug 24815: Add additional tests

Review of attachment 103560 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Koha/Cash/Register.t
@@ +249,5 @@
> +        # add_cashup should not happen simultaneously with any other action
> +        # that results in an accountline attached to the same cash register.
> +        # In testing, we need to sleep for a second after each action that
> +        # adds to the database. (We cannot use Time::Fake as timestamps are
> +        # being added at the DB level, not in perl.

Hum, double check this, it has been removed previously.
Comment 14 Martin Renvoize 2020-04-27 13:11:20 UTC
Created attachment 103786 [details] [review]
Bug 24815: Correct return values of Koha::Cash::Register relations

This patch correct all cases of return undef to instead return an empty
resultset.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 Martin Renvoize 2020-04-27 13:11:23 UTC
Created attachment 103787 [details] [review]
Bug 24815: Add additional tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 16 Martin Renvoize 2020-04-27 13:11:27 UTC
Created attachment 103788 [details] [review]
Bug 24815: (follow-up) Rebase correction
Comment 17 Martin Renvoize 2020-04-27 13:12:54 UTC
You are indeed correct, it came back through a bad rebase.. corrected in the followup
Comment 18 Jonathan Druart 2020-04-27 13:22:17 UTC
Created attachment 103790 [details] [review]
Bug 24815: Correct return values of Koha::Cash::Register relations

This patch correct all cases of return undef to instead return an empty
resultset.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2020-04-27 13:22:20 UTC
Created attachment 103791 [details] [review]
Bug 24815: Add additional tests

Signed-off-by: David Nind <david@davidnind.com>

Bug 24815: (follow-up) Rebase correction

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Martin Renvoize 2020-04-29 15:25:09 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 21 Joy Nelson 2020-05-08 22:03:56 UTC
not backported to 19.11.x