Since bug 9780 we are the ability to alert the librarian when a record which has been ordered will be deleted. Then the code to handle the alert has been copied to other catalogue/*detail.pl script, as the JS check is in the toolbar. This code is really unnecessary complicated and sometime simply not necessary. It could be replaced with Koha::Acquisition::Orders->search and make it more readable. (found it working on bug 23084, so writing it on top of it).
Created attachment 90824 [details] [review] Bug 23166: Replace the count of orders with Koha::Acq::Orders
Created attachment 90825 [details] [review] Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} We certainly faced 3 similar bugs due to this syntax: bug 23006, bug 22941 and bug 17526. To prevent other issues related to this syntax this patch suggests to replace them all in one go. Test plan: Confirm that the 2 syntaxes are similar Eyeball the patch and confirm that there is no typo!
Created attachment 90826 [details] [review] Bug 23166: Remove baskets_orders code This is never used.
Created attachment 90827 [details] [review] Bug 23166: Remove baskets_deletedorders code This is never used.
Created attachment 90828 [details] [review] Bug 23166: Replace the count of orders with Koha::Acq::Orders
Martin, any suggestions to make this even more readable? Maybe using filter_by? I will be waiting for a decision before applying the change to other *detail.pl scripts.
Created attachment 104365 [details] [review] Bug 23166: Remove baskets_orders code This is never used.
Created attachment 104366 [details] [review] Bug 23166: Remove baskets_deletedorders code This is never used.
Created attachment 104367 [details] [review] Bug 23166: Replace the count of orders with Koha::Acq::Orders
Created attachment 104368 [details] [review] Bug 23166: Remove filtering from controllers
Created attachment 104369 [details] [review] Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods
Created attachment 104370 [details] [review] Bug 23166: Call the methods from the .inc We do not longer need the order variables to be passed from the controllers, we can call the methods on the biblio object instead. There is something wrong with our ->search method and TT behaviours, it is hard to retrieve object list in a scalar context. If [% objects.method.count %] is called, objects.method will get the first object of the list and count will explode (Koha::Object->count does not exist) We need to force the call in a scalar context to retrieve an iterator and prevent to fetch all the objects (we could have called all then size, but it's not efficient) If adopted I will move the plugin on a separate bug report to ease backport
Test plan: Create several orders (on at least 2 bibliographic records) Cancel some Try to delete the bibliographic records => You should the JS alert about existing orders The delete button from the different views where the toolbar is displayed should be tested: - normal detail - ISBD detail - MARC detail - image viewer - labeled MARC - More detail No behaviour changes expected.
This is ready for testing (at least receive feedback on the different changes made by this patch set)
Created attachment 108991 [details] [review] Bug 23166: Remove baskets_orders code This is never used. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 108992 [details] [review] Bug 23166: Remove baskets_deletedorders code This is never used. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 108993 [details] [review] Bug 23166: Replace the count of orders with Koha::Acq::Orders Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 108994 [details] [review] Bug 23166: Remove filtering from controllers Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 108995 [details] [review] Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 108996 [details] [review] Bug 23166: Call the methods from the .inc We do not longer need the order variables to be passed from the controllers, we can call the methods on the biblio object instead. There is something wrong with our ->search method and TT behaviours, it is hard to retrieve object list in a scalar context. If [% objects.method.count %] is called, objects.method will get the first object of the list and count will explode (Koha::Object->count does not exist) We need to force the call in a scalar context to retrieve an iterator and prevent to fetch all the objects (we could have called all then size, but it's not efficient) If adopted I will move the plugin on a separate bug report to ease backport Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Qa tool has complaints: FAIL Koha/Template/Plugin/Context.pm FAIL pod coverage POD is missing for 'Scalar' FAIL koha-tmpl/intranet-tmpl/prog/en/includes/catalog-strings.inc FAIL valid_template Can't call method "filter_by_current" without a package or object reference at /kohadevbox/koha/Koha/Template/Plugin/Context.pm line 43.
Created attachment 108997 [details] [review] Bug 23166: Remove baskets_orders code This is never used. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 108998 [details] [review] Bug 23166: Remove baskets_deletedorders code This is never used. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 108999 [details] [review] Bug 23166: Replace the count of orders with Koha::Acq::Orders Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 109000 [details] [review] Bug 23166: Remove filtering from controllers Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 109001 [details] [review] Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 109002 [details] [review] Bug 23166: Call the methods from the .inc We do not longer need the order variables to be passed from the controllers, we can call the methods on the biblio object instead. There is something wrong with our ->search method and TT behaviours, it is hard to retrieve object list in a scalar context. If [% objects.method.count %] is called, objects.method will get the first object of the list and count will explode (Koha::Object->count does not exist) We need to force the call in a scalar context to retrieve an iterator and prevent to fetch all the objects (we could have called all then size, but it's not efficient) If adopted I will move the plugin on a separate bug report to ease backport Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
I totally missed running the qa tool, thanks for noticing Nick!
Created attachment 109019 [details] [review] Bug 23166: (QA follow-up) Add POD text
Created attachment 109020 [details] [review] Bug 23166: Remove baskets_orders code This is never used.
Created attachment 109021 [details] [review] Bug 23166: Remove baskets_deletedorders code This is never used.
Created attachment 109022 [details] [review] Bug 23166: Replace the count of orders with Koha::Acq::Orders
Created attachment 109023 [details] [review] Bug 23166: Remove filtering from controllers
Created attachment 109024 [details] [review] Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods
Created attachment 109025 [details] [review] Bug 23166: Call the methods from the .inc We do not longer need the order variables to be passed from the controllers, we can call the methods on the biblio object instead. There is something wrong with our ->search method and TT behaviours, it is hard to retrieve object list in a scalar context. If [% objects.method.count %] is called, objects.method will get the first object of the list and count will explode (Koha::Object->count does not exist) We need to force the call in a scalar context to retrieve an iterator and prevent to fetch all the objects (we could have called all then size, but it's not efficient) If adopted I will move the plugin on a separate bug report to ease backport
Created attachment 109026 [details] [review] Bug 23166: (QA follow-up) Add POD text
I attached a follow-up for the POD and removed the sign-offs given the other issue Nick pointed out still remains: > FAIL koha-tmpl/intranet-tmpl/prog/en/includes/catalog-strings.inc > FAIL valid_template > Can't call method "filter_by_current" without a package or object reference at /kohadevbox/koha/Koha/Template/Plugin/Context.pm line 43.
There seems to be nothing wrong with the code, it is just that the QA tool doesn't add the "orders" object because this is not being executed from the real .pl script, so therefore the $set variable is undef. I think we could say this is false positive then, right? Or if we want to prevent the QA tool failing maybe such a check can be added: sub Scalar { my ( $self, $set, $method ) = @_; + return undef unless $set; $set = $set->$method; return $set; } What do you think? I think it is safe to ignore this here and we could go ahead with the current patches.
(In reply to Joonas Kylmälä from comment #37) > I attached a follow-up for the POD and removed the sign-offs given the other > issue Nick pointed out still remains: > > > FAIL koha-tmpl/intranet-tmpl/prog/en/includes/catalog-strings.inc > > FAIL valid_template > > Can't call method "filter_by_current" without a package or object reference at /kohadevbox/koha/Koha/Template/Plugin/Context.pm line 43. Hi Joonas, we usually don't remove the sign-off lines - that's one reason we prefer follow-ups - because it allows to see the changes, but also because that means you don't change what has been signed off by someone.
Created attachment 109037 [details] [review] Bug 23166: Remove baskets_orders code This is never used. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 109038 [details] [review] Bug 23166: Remove baskets_deletedorders code This is never used. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 109039 [details] [review] Bug 23166: Replace the count of orders with Koha::Acq::Orders Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 109040 [details] [review] Bug 23166: Remove filtering from controllers Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 109041 [details] [review] Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 109042 [details] [review] Bug 23166: Call the methods from the .inc We do not longer need the order variables to be passed from the controllers, we can call the methods on the biblio object instead. There is something wrong with our ->search method and TT behaviours, it is hard to retrieve object list in a scalar context. If [% objects.method.count %] is called, objects.method will get the first object of the list and count will explode (Koha::Object->count does not exist) We need to force the call in a scalar context to retrieve an iterator and prevent to fetch all the objects (we could have called all then size, but it's not efficient) If adopted I will move the plugin on a separate bug report to ease backport Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 109043 [details] [review] Bug 23166: (QA follow-up) Add POD text Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 109044 [details] [review] Bug 23166: Remove baskets_orders code This is never used. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 109045 [details] [review] Bug 23166: Remove baskets_deletedorders code This is never used. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 109046 [details] [review] Bug 23166: Replace the count of orders with Koha::Acq::Orders Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 109047 [details] [review] Bug 23166: Remove filtering from controllers Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 109048 [details] [review] Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 109049 [details] [review] Bug 23166: Call the methods from the .inc We do not longer need the order variables to be passed from the controllers, we can call the methods on the biblio object instead. There is something wrong with our ->search method and TT behaviours, it is hard to retrieve object list in a scalar context. If [% objects.method.count %] is called, objects.method will get the first object of the list and count will explode (Koha::Object->count does not exist) We need to force the call in a scalar context to retrieve an iterator and prevent to fetch all the objects (we could have called all then size, but it's not efficient) If adopted I will move the plugin on a separate bug report to ease backport Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 109050 [details] [review] Bug 23166: (QA follow-up) Add POD text Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
(In reply to Katrin Fischer from comment #39) > we usually don't remove the sign-off lines - that's one reason we prefer > follow-ups - because it allows to see the changes, but also because that > means you don't change what has been signed off by someone. sorry for the mess! I was working on signing-off the patches the same time as Nick and hadn't noticed he had submitted his signed-off-by lines just some minutes earlier so when I ran git-bz attach with my patches it overrode his signed-off-by lines with mine. And then Nick had noticed the QA tool failure after his sign-off and put the bug to FQA state so I thought it would be best to wait for him to sign-off again, which also prompted for me to remove my own SO lines. Anyhow, Nick signed-off this now and I added my QA sign-off so ready for RM to merge :)
Sounds good and I can stop to stare at this because I am getting nowhere tonight :)
Created attachment 109058 [details] [review] Bug 23166: Early return if no set It prevents valid_template check to raise Can't call method "filter_by_current" without a package or object reference at /kohadevbox/koha/Koha/Template/Plugin/Context.pm line 49.
(In reply to Joonas Kylmälä from comment #38) > There seems to be nothing wrong with the code, it is just that the QA tool > doesn't add the "orders" object because this is not being executed from the > real .pl script, so therefore the $set variable is undef. I think we could > say this is false positive then, right? Or if we want to prevent the QA tool > failing maybe such a check can be added: > > sub Scalar { > my ( $self, $set, $method ) = @_; > + return undef unless $set; > $set = $set->$method; > return $set; > } > > What do you think? I think it is safe to ignore this here and we could go > ahead with the current patches. I've decided to fix it here, it should not hurt.
Created attachment 109059 [details] [review] Bug 23166: Early return if no set It prevents valid_template check to raise Can't call method "filter_by_current" without a package or object reference at /kohadevbox/koha/Koha/Template/Plugin/Context.pm line 49. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
(In reply to Jonathan Druart from comment #57) > I've decided to fix it here, it should not hurt. Ok, I added my SO for it in case you didn't push yet.
Pushed to master for 20.11, thanks to everybody involved!
enhancement will not be backported to 20.05.x