[Bug 32472] New: The method Koha::Item->count is not covered by tests
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 Bug ID: 32472 Summary: The method Koha::Item->count is not covered by tests 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: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org This is only possible under 21.11 (before bug Bug 29844 - Remove uses of wantarray in Koha::Objects) in templates where constructions like items.count etc are used on passed objects. Template Toolkit does evaluate these expressions in LIST context while we probably expected scalar context as when chaining in Perl. This particular crash comes from acqui/orderreceive.pl (and template): [% IF ( order.items.count ) %] In this case we should either use the Scalar plugin or - simply - use size here from TT itself. Note that this could be applied to master too since it is probably cheaper to stay in TT than call Koha::Objects/DBI here. -- 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=32472 --- Comment #1 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #0)
Note that this could be applied to master too since it is probably cheaper to stay in TT than call Koha::Objects/DBI here.
No, it cannot ! The resultset is one. So always true.. -- 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=32472 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |arthur.suzuki@biblibre.com Severity|enhancement |major Summary|The method |[21.11] The method |Koha::Item->count is not |Koha::Item->count is not |covered by tests |covered by tests Version|master |21.11 --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Quite interesting to see how many occurrences are really affected. If you pass items with say $items = Koha::Items->search, you will be fine since you passed items as a scalar to the template. So a lot of [plurals].count are just fine. But what about order.items.count? Generally this comes from passing order as a scalar but the next part "items" is evaluated in list context in TT. So those might be wrong.. A git grep like git grep -E "\w+\.\w+\.count " will give such candidates. (Note that you could also call other methods than count..] This gives me just a bunch for opac templates in 21.11. The ones I checked seem to be fine: item.ratings.count comes from $issue->{ratings} so scalar context. But quite a bit more for intranet, but most are in a specific context: club enrollments, return claims, credit type library limits, course reserves, stock rotation quota. Just mentioning a few: includes/cat-toolbar.inc: [% ELSIF ( biblio.subscriptions.count ) %] includes/clubs-table.inc: [% c.club_enrollments.count | html %] modules/acqui/showorder.tt: [% order.claims.count | html %] modules/cataloguing/moveitem.tt: [% IF from_biblio.items.count == 0 && CAN_user_editcatalogue_edit_catalogue %]modules/members/discharge.tt: [% IF patron.holds.count %] etc -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> ---
includes/cat-toolbar.inc: [% ELSIF ( biblio.subscriptions.count ) %] This one is fine. return $self->{_subscriptions} (scalar)
modules/cataloguing/moveitem.tt: [% IF from_biblio.items.count == 0 && CAN_user_editcatalogue_edit_catalogue %] It is a scalar too: my $items_rs = $self->_result->items;
club enrollments too: return scalar Koha::Club::Enrollments->search( { club_id => $self->id(), date_canceled => undef } ); return_claims: my $return_claims = $self->_result->return_claims_borrowernumbers; (SCALAR) FINE claims FINE my $claims_rs = $self->_result->aqorders_claims; So it is not so bad as it looked. But sub items in Koha/Acquisition/Order.pm is a bad example: with return Koha::Items->search({ itemnumber => \@itemnumbers }); So we could git grep on git grep -E "return Koha::\w+->search" in the Koha directory. See next comment. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- git grep -E "return Koha::\w+->search" Acquisition/Bookseller.pm: return Koha::Subscriptions->search( { aqbooksellerid => $self->id } ); => sub subscriptions This could be LIST context. So needs adjustment in 21.11 ? Acquisition/Order.pm: return Koha::Items->search({ itemnumber => \@itemnumbers }); => see previous comment WRONG ArticleRequests.pm: return Koha::ArticleRequests->search_limited($params); ArticleRequests.pm: return Koha::ArticleRequests->search_limited( $params ); ArticleRequests.pm: return Koha::ArticleRequests->search_limited( $params ); ArticleRequests.pm: return Koha::ArticleRequests->search_limited( $params ); ArticleRequests.pm: return Koha::ArticleRequests->search_limited( $params ); => sub requested, pending, processing, completed, canceled => article-requests.tt: Pending (<span id="ar_pending_count">[% article_requests_pending.count | html %]</span>) => article-requests.tt: Processing (<span id="ar_processing_count">[% article_requests_processing.count | html %]</span>) => Are passed with scalar to template. FINE OK Biblio.pm: return Koha::Libraries->search( => sub pickup_locations looks WRONG but no direct template calls? OK Biblio.pm: return Koha::Checkouts->search( { "item.biblionumber" => $self->id }, => sub current_checkouts and ALSO old_checkouts NO template calls OK Biblio.pm: return Koha::Items->search( { itemnumber => { -in => \@itemnumbers } } ); => host_items looks wrong NO template_calls OK Biblios.pm: return Koha::Libraries->search( => pickup_locations SEE above OK Holds.pm: return Koha::Items->search( => get_items_that_can_fill, waiting, unfilled are candidates / No template calls found TODO Illrequest.pm: return Koha::AuthorisedValues->search( OK ItemType.pm: return Koha::ItemTypes->search_with_localization({ parent_type => $self->itemtype }); => children_with_localization OK Library.pm: return Koha::Libraries->search({ branchcode => { '-in' => [ keys %seen ] } }); => get_hold_libraries, validate_hold_sibling TODO Library/Group.pm: return Koha::Libraries->search( TODO SMS/Provider.pm: return Koha::Patrons->search( { sms_provider_id => $self->id } )->count(); TODO SMTP/Server.pm: return Koha::Libraries->search( { branchcode => { -in => \@library_ids } } ); => libraries OK Subscription.pm: return Koha::Patrons->search({ borrowernumber => {-in => \@borrowernumbers } }); * subscribers OK Template/Plugin/AuthorisedValues.pm: return Koha::AuthorisedValues->search_with_library_limits( * GetAuthValueDropbox, GetDescriptionsByKohaField TODO Template/Plugin/Desks.pm: return Koha::Desks->search(); OK Template/Plugin/ItemTypes.pm: return Koha::ItemTypes->search_with_localization->unblessed; TODO Util/StockRotation.pm: return Koha::Libraries->search( OK Virtualshelves.pm: return Koha::Virtualshelves->search( * get_shelves_containing_record -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Preliminary conclusion: Luckily only found a few occurrences that really seem to be wrong in 21.11. The combination of weird TT list context evaluation of chained expressions, passing objects to templates and potentially returning lists instead of a result set (before bug 29844) based on wantarray is a minefield. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Acquisition/Bookseller.pm: return Koha::Subscriptions->search( { aqbooksellerid => $self->id } ); => sub subscriptions This could be LIST context. So needs adjustment in 21.11 ? This one is also fine, since we do not call it in templates. We call biblio.subscriptions but not on a bookseller. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emmi.takkinen@koha-suomi.fi -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 144609 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144609&action=edit Bug 32472: Fix wrong POD for bookseller->subscriptions You should not call this in list context anymore after bug 29844. Test plan: Check pod of Bookseller. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- 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=32472 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 144610 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144610&action=edit Bug 32472: [21.11] Force scalar context on acqorder->items This resolves a crash like The method Koha::Item->count is not covered by tests! on the template construction [% IF ( order.items.count ) %] in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt. Test plan (21.11): Receive an order line under this condition: quantityreceived>0 with items attached to the order line. Note: Should be possible, comes from production logs. Look for list context on ->items called from acqorder. The result of git grep "\->items" is too wide but we could theoretically miss an occurence with git grep "order.*\->items". With order.* we do catch $new_order_object->items.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- 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=32472 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|[21.11] The method |[21.11 CRASH] The method |Koha::Item->count is not |Koha::Item->count is not |covered by tests |covered by tests -- 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=32472 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |m.de.rooy@rijksmuseum.nl |ity.org | -- 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=32472 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29844 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144609|0 |1 is obsolete| | --- Comment #9 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Created attachment 144860 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144860&action=edit Bug 32472: Fix wrong POD for bookseller->subscriptions You should not call this in list context anymore after bug 29844. Test plan: Check pod of Bookseller. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144610|0 |1 is obsolete| | --- Comment #10 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Created attachment 144861 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144861&action=edit Bug 32472: [21.11] Force scalar context on acqorder->items This resolves a crash like The method Koha::Item->count is not covered by tests! on the template construction [% IF ( order.items.count ) %] in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt. Test plan (21.11): Receive an order line under this condition: quantityreceived>0 with items attached to the order line. Note: Should be possible, comes from production logs. Look for list context on ->items called from acqorder. The result of git grep "\->items" is too wide but we could theoretically miss an occurence with git grep "order.*\->items". With order.* we do catch $new_order_object->items.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Look at both patches, they are extremely trivial. There is a signoff from Emmi (thx for that!). Moving to the attention of the RM. The second patch is not needed on master or 22.x.x. Marked for 21.11 :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Trivial POD fix pushed to master. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Version(s)| |21.11.18 released in| | Status|Passed QA |RESOLVED --- Comment #13 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- applied to 21.11.x for 21.11.18 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 Jacob O'Mara <jacob.omara@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jacob.omara@ptfs-europe.com --- Comment #14 from Jacob O'Mara <jacob.omara@ptfs-europe.com> --- POD fix pushed to stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 Jacob O'Mara <jacob.omara@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|21.11.18 |21.11.18,22.11.04 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32472 --- Comment #15 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- *** Bug 30321 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org