Bug 24815

Summary: Koha::Cash::Register relations should return sets not undef
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: normal    
Priority: P5 - low CC: 1joynelson, david, J.P.Knight, jonathan.druart, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on: 23321, 23355, 24817    
Bug Blocks: 13985    
Attachments: Bug 24815: Correct return values of Koha::Cash::Register relations
Bug 24815: Correct return values of Koha::Cash::Register relations
Bug 24815: Add additional tests
Bug 24815: Correct return values of Koha::Cash::Register relations
Bug 24815: Add additional tests
Bug 24815: Correct return values of Koha::Cash::Register relations
Bug 24815: Add additional tests
Bug 24815: Correct return values of Koha::Cash::Register relations
Bug 24815: Add additional tests
Bug 24815: (follow-up) Rebase correction
Bug 24815: Correct return values of Koha::Cash::Register relations
Bug 24815: Add additional tests

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