The cash register class contains a bad pattern of returning 'undef' for relations instead of an empty resultset.
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.
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); }
(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
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>
Created attachment 100220 [details] [review] Bug 24815: Add additional tests
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
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>
Created attachment 101908 [details] [review] Bug 24815: Add additional tests
Rebased, thanks for yesting Jon
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>
Created attachment 103560 [details] [review] Bug 24815: Add additional tests Signed-off-by: David Nind <david@davidnind.com>
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 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.
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>
Created attachment 103787 [details] [review] Bug 24815: Add additional tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 103788 [details] [review] Bug 24815: (follow-up) Rebase correction
You are indeed correct, it came back through a bad rebase.. corrected in the followup
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>
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>
Nice work everyone! Pushed to master for 20.05
not backported to 19.11.x