[Bug 25296] New: Add a way to force an empty Koha::Objects resultset
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Bug ID: 25296 Summary: Add a way to force an empty Koha::Objects resultset Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org 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 } } ); } -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | CC| |jonathan.druart@bugs.koha-c | |ommunity.org, | |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |agustinmoyano@theke.io -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 103811 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103811&action=edit Bug 25296: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 103812 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103812&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |25297 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25297 [Bug 25297] Consistent return value in K::A::Order->current_item_level_holds -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |frederic@tamil.fr --- Comment #3 from Frédéric Demians <frederic@tamil.fr> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #4 from Frédéric Demians <frederic@tamil.fr> --- 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; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103811|0 |1 is obsolete| | --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 103860 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103860&action=edit Bug 25296: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103812|0 |1 is obsolete| | --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 103861 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103861&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #9 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_20_05_candidate, | |RM_priority -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- use Koha::Patrons; say Koha::Patrons->new->empty->count; say Koha::Patrons->empty->count; 0 53 How could we avoid that? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 103928 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103928&action=edit Bug 25296: Tests for uninstantiated behaviour Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 103929 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103929&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103860|0 |1 is obsolete| | Attachment #103861|0 |1 is obsolete| | Attachment #103928|0 |1 is obsolete| | Attachment #103929|0 |1 is obsolete| | --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 103934 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103934&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 103935 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103935&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 103936 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103936&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 103937 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103937&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #19 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |jonathan.druart@bugs.koha-c |y.org |ommunity.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |20.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 --- Comment #21 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_20_05_candidate, | |RM_priority | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25296 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joy@bywatersolutions.com --- Comment #22 from Joy Nelson <joy@bywatersolutions.com> --- not backported to 19.11 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org