In some scenarios, it would be handy for consistency to just return an empty resultset (instead of undef, for example). Take the following example: sub current_item_level_holds { my ($self) = @_; my $items_rs = $self->_result->aqorders_items; my @item_numbers = $items_rs->get_column('itemnumber')->all; return unless @item_numbers; my $biblio = $self->biblio; return unless $biblio; return $biblio->current_holds->search( { itemnumber => { -in => \@item_numbers } } ); } if we wanted to always return a Koha::Holds iterator (as ->current_holds does) we could do: sub current_item_level_holds { my ($self) = @_; my $items_rs = $self->_result->aqorders_items; my @item_numbers = $items_rs->get_column('itemnumber')->all; my $biblio = $self->biblio; unless ( $biblio and @itemnumbers ) { return Koha::Holds->new->empty; } return $biblio->current_holds->search( { itemnumber => { -in => \@item_numbers } } ); }
Created attachment 103811 [details] [review] Bug 25296: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 103812 [details] [review] Bug 25296: Add a way to force an empty Koha::Objects resultset This patch adds a new ->empty method to Koha::Objects, that can be used to make the underlying DBIC resultset empty. This way, we can have consistency in our method's return values without the need to build a query that we know in advance that will be empty. No need to hit the DB at all. To test: 1. Apply this patches 2. Notice the tests cover what is expected 3. Run: $ kshell k$ prove t/db_dependent/Koha/Objects.t => SUCCESS: Tests pass! Yay! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
How to sign-off such a patch? And who has the knowledge/authority to do it? Reading the code and the reasoning behind this patch, I could say that it's clear and clean, but is it enough to sign-off?
Coming back from bug 25297, I have a question: Is it necessary to explicitly call: Koha::Objects->new->empty; When you assign a result: my $result = Koha::Holds->new; Don't you have already $result->count == 0 ? If this was the case, your code in bug 25297 could become something like that: my $result = Koha::Holds->new; my $biblio = $self->biblio; return $result unless $biblio; my $items_rs = $self->_result->aqorders_items; my @item_numbers = $items_rs->get_column('itemnumber')->all; return $result unless @item_numbers;
(In reply to Frédéric Demians from comment #4) > Coming back from bug 25297, I have a question: Is it necessary to explicitly > call: > > Koha::Objects->new->empty; > > When you assign a result: > > my $result = Koha::Holds->new; > > Don't you have already $result->count == 0 ? I wrote the regression tests for the behavior so you can play with other options. The short answer is Koha::Holds->new refers to the whole table rows.
My conversation with one of the DBIx::Class authors: [17:56:51] <tcohen> weird question: if I know a query is going to return and empty resultset in advance, is there a way/helper to avoid performing the query, but returning something resultset-ish for consistency? [17:57:17] <mst> tcohen: yes [17:57:23] <mst> tcohen: $rs->set_cache([]); [17:57:32] <tcohen> oh! [17:57:43] <mst> tcohen: not weird at all, I designed that feature in somewhere over ten years ago and if it doesn't work for you, please ask again :D
Created attachment 103860 [details] [review] Bug 25296: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 103861 [details] [review] Bug 25296: Add a way to force an empty Koha::Objects resultset This patch adds a new ->empty method to Koha::Objects, that can be used to make the underlying DBIC resultset empty. This way, we can have consistency in our method's return values without the need to build a query that we know in advance that will be empty. No need to hit the DB at all. To test: 1. Apply this patches 2. Notice the tests cover what is expected 3. Run: $ kshell k$ prove t/db_dependent/Koha/Objects.t => SUCCESS: Tests pass! Yay! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This makes a lot of sense in the context of bug 25297. Signing off as it works as expected and contains tests that pass. We will need clear coding guidelines for when it should and should not be used.
use Koha::Patrons; say Koha::Patrons->new->empty->count; say Koha::Patrons->empty->count; 0 53 How could we avoid that?
(In reply to Jonathan Druart from comment #10) > use Koha::Patrons; > say Koha::Patrons->new->empty->count; > say Koha::Patrons->empty->count; > > 0 > 53 > > > How could we avoid that? Do we need to?
Created attachment 103928 [details] [review] Bug 25296: Tests for uninstantiated behaviour Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 103929 [details] [review] Bug 25296: Make ->empty work for uninstantiated calls In OO we would usually instantiate the class and then interact with it like in: my $rs = Koha::Patrons->new; $rs->empty; Koha's practice is to call things like Koha::Patrons->search without instantiating the Koha::Patrons class. To keep consistency, this patch instantiates the resultset object on behalf of the caller if required. To test: 1. Apply the tests patch 2. Run: $ kshell $k prove t/db_dependent/Koha/Objects.t => FAIL: Tests fail because it is expected to have the class instantiated 3. Apply this patch 4. Repeat 2. => SUCCESS: Tests pass, instantiation happens implicitly. 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Jonathan Druart from comment #10) > use Koha::Patrons; > say Koha::Patrons->new->empty->count; > say Koha::Patrons->empty->count; > > 0 > 53 > > > How could we avoid that? Solved, you can now QA on this, please.
Created attachment 103934 [details] [review] Bug 25296: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 103935 [details] [review] Bug 25296: Add a way to force an empty Koha::Objects resultset This patch adds a new ->empty method to Koha::Objects, that can be used to make the underlying DBIC resultset empty. This way, we can have consistency in our method's return values without the need to build a query that we know in advance that will be empty. No need to hit the DB at all. To test: 1. Apply this patches 2. Notice the tests cover what is expected 3. Run: $ kshell k$ prove t/db_dependent/Koha/Objects.t => SUCCESS: Tests pass! Yay! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 103936 [details] [review] Bug 25296: Tests for uninstantiated behaviour Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 103937 [details] [review] Bug 25296: Make ->empty work for uninstantiated calls In OO we would usually instantiate the class and then interact with it like in: my $rs = Koha::Patrons->new; $rs->empty; Koha's practice is to call things like Koha::Patrons->search without instantiating the Koha::Patrons class. To keep consistency, this patch instantiates the resultset object on behalf of the caller if required. To test: 1. Apply the tests patch 2. Run: $ kshell $k prove t/db_dependent/Koha/Objects.t => FAIL: Tests fail because it is expected to have the class instantiated 3. Apply this patch 4. Repeat 2. => SUCCESS: Tests pass, instantiation happens implicitly. 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I am wondering if we should not force ->empty to be called only as a class method. my $patrons = Koha::Patrons->search; $patrons->empty; # Does not really make sense to me.
Nice work everyone! Pushed to master for 20.05
(In reply to Jonathan Druart from comment #19) > I am wondering if we should not force ->empty to be called only as a class > method. > > my $patrons = Koha::Patrons->search; > $patrons->empty; # Does not really make sense to me. It would be: my $patrons_rs = Koha::Patrons->new->empty; but I get the point. In the context of bug 25297, it really looks like an unnecessary extra line. I still feel uncomfortable with the magic instantiation that happens in ->search (and now in ->empty). my $rs = Koha::Things->new; $rs->search( $condition_1 ) if $condition_1; $rs->search( $condition_2 ) if $condition_2; while ( my $res = $rs->next ) {...} it is easier to understand (conceptually, for me) than my $rs = Koha::Things->search( $condition_1 ); $rs->search( $condition_2 ) if $condition_2; while ( my $res = $rs->next ) {...} even if you save one line of code (or a ->new on the same line even)... In this case I opted to not be rigid on my POV and implemented both behaviours, but I think it is worth discussing in a broader place, if you think some guideline needs to be generally adopted.
not backported to 19.11