Created attachment 91774 [details] [review] Bug 23376: Clean up order recipt page This patchset switches from using DB lookups to using an order object for most things on orderreceive.pl It simplifies the script and makes minimal changes to the template To test: 1 - Place some orders and receive them 2 - Have orders with or without subscriptions attached 3 - Try with different AcqCreateItems settings 4 - Apply patch 5 - No behaviour should change 6 - Read code to ensure things make sense
Created attachment 91857 [details] [review] Bug 23376: Move AcqCreateItem logic to template
Created attachment 91858 [details] [review] Bug 23376: Remove unneeded 'count' parameter
When I try to apply this, I get the following 13:47 $ git bz apply 23376 Bug 23376 - Cleanup order receive page 91774 - Bug 23376: Clean up order recipt page 91857 - Bug 23376: Move AcqCreateItem logic to template 91858 - Bug 23376: Remove unneeded 'count' parameter Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 23376: Clean up order recipt page Using index info to reconstruct a base tree... M acqui/orderreceive.pl M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt Auto-merging acqui/orderreceive.pl CONFLICT (content): Merge conflict in acqui/orderreceive.pl error: Failed to merge in the changes. Patch failed at 0001 Bug 23376: Clean up order recipt page hint: Use 'git am --show-current-patch' to see the failed 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 /var/folders/97/nr6_vjn53dl2csvdgtcy_8f40000gp/T/Bug-23376-Clean-up-order-recipt-page-Q2iLmH.patch
Created attachment 110692 [details] [review] Bug 23376: Clean up order recipt page This patchset switches from using DB lookups to using an order object for most things on orderreceive.pl It simplifies the script and makes minimal changes to the template To test: 1 - Place some orders and receive them 2 - Have orders with or without subscriptions attached 3 - Try with different AcqCreateItems settings 4 - Apply patch 5 - No behaviour should change 6 - Read code to ensure things make sense Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Created attachment 110693 [details] [review] Bug 23376: Move AcqCreateItem logic to template Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Created attachment 110694 [details] [review] Bug 23376: Remove unneeded 'count' parameter Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
I took the liberty to rebase and signoff all at the same time. Nick, could you please check that rebase it's ok? Thanks
Heh, I harasses Agustin before testing, but yes, the rebase looks great, thank you!
Created attachment 111247 [details] [review] Bug 23376: Clean up order recipt page This patchset switches from using DB lookups to using an order object for most things on orderreceive.pl It simplifies the script and makes minimal changes to the template To test: 1 - Place some orders and receive them 2 - Have orders with or without subscriptions attached 3 - Try with different AcqCreateItems settings 4 - Apply patch 5 - No behaviour should change 6 - Read code to ensure things make sense Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111248 [details] [review] Bug 23376: Move AcqCreateItem logic to template Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111249 [details] [review] Bug 23376: Remove unneeded 'count' parameter Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111250 [details] [review] Bug 23376: (QA follow-up) Remove authnotrequired The rebased patchset subsequnetly re-introduced the now deprecated use of authnotrequired on staff client controllers. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nice bit of cleaning up, thanks. All works as expected still, no regressions found. Passing QA
Pushed to master for 20.11, thanks to everybody involved!
Nice one, Nick!
Created attachment 111378 [details] [review] Bug 23376: (follow-up) Fix subscription bug in orderreceive.tt When order has subscription, general order variable gets replaced by other order variable in FOR loop
(In reply to Agustín Moyano from comment #17) > Created attachment 111378 [details] [review] [review] > Bug 23376: (follow-up) Fix subscription bug in orderreceive.tt > > When order has subscription, general order variable gets replaced by other > order variable in FOR loop Pushed to master, thanks Agustin!
Created attachment 111750 [details] [review] Bug 23376: If a single item is returned we cannot count
(In reply to Nick Clemens from comment #19) > Created attachment 111750 [details] [review] [review] > Bug 23376: If a single item is returned we cannot count "order" is a Koha::Acquisition::Order and so order->items will return a Koha::Items set, should not then it be [% order.items.count %] instead?
I've tested some bugs with the patch as proposed, but not sure if there are side effects I have missed.
I've tried with count and it did not work (not sure why actually!)
(In reply to Nick Clemens from comment #19) > Created attachment 111750 [details] [review] [review] > Bug 23376: If a single item is returned we cannot count Patch pushed to master for 20.11.00, thanks Nick!
enhancement will not be backported to 20.05.x
We ran into a problem with our orders receive page and would like to know if anyone has run into same kind of issue. It seems if order.quantityreceived is more than 0 page orderreceive.pl/template orderreceive.tt dies on line 323 which reads [% IF ( order.items.count ) %]. In logs we have: "Template process failed: undef error - The method Koha::Item->count is not covered by tests!" Changing order.items.count to items.count doesn't produce error but isn't really helpful since now there's no value on input "Quantity received", just 0 (this could also be a separate problem).
(In reply to Emmi Takkinen from comment #25) > We ran into a problem with our orders receive page and would like to know if > anyone has run into same kind of issue. It seems if order.quantityreceived > is more than 0 page orderreceive.pl/template orderreceive.tt dies on line > 323 which reads [% IF ( order.items.count ) %]. In logs we have: > > "Template process failed: undef error - The method Koha::Item->count is not > covered by tests!" > > Changing order.items.count to items.count doesn't produce error but isn't > really helpful since now there's no value on input "Quantity received", just > 0 (this could also be a separate problem). It should be orders.items. It's certainly fixed on master since bug 29844. Weird that it has not been reported before.
(In reply to Jonathan Druart from comment #26) > (In reply to Emmi Takkinen from comment #25) > > We ran into a problem with our orders receive page and would like to know if > > anyone has run into same kind of issue. It seems if order.quantityreceived > > is more than 0 page orderreceive.pl/template orderreceive.tt dies on line > > 323 which reads [% IF ( order.items.count ) %]. In logs we have: > > > > "Template process failed: undef error - The method Koha::Item->count is not > > covered by tests!" > > > > Changing order.items.count to items.count doesn't produce error but isn't > > really helpful since now there's no value on input "Quantity received", just > > 0 (this could also be a separate problem). > > It should be orders.items. > It's certainly fixed on master since bug 29844. > > Weird that it has not been reported before. Yep, changing order.items to orders.items fixed error. I'll provide patch here.
Created attachment 131857 [details] Bug 23376: Fix template error on order receive page Order receive page dies on error 500 if orders quantityreceived is more than 0. The error logs reads: "Template process failed: undef error - The method Koha::Item->count is not covered by tests!" In template orderreceive.tt we have "order.items.count" not "orders.items.count" as it should be. Note! It seems that this error doesn't occur if template param edit is set as 1. To test: 1. Find order with quantityreceived value more than 0. 2. Try to receive this order. => Error 500 page is displayed. 3. Apply patch 4. Repeat steps 1 and 2. => Order receive page is displayed normally. Sponsored-by: Koha-Suomi Oy
(In reply to Emmi Takkinen from comment #28) > Created attachment 131857 [details] [review] > Bug 23376: Fix template error on order receive page > > Order receive page dies on error 500 if orders quantityreceived > is more than 0. The error logs reads: "Template process failed: undef > error - The method Koha::Item->count is not covered by tests!" > In template orderreceive.tt we have "order.items.count" not > "orders.items.count" as it should be. Note! It seems that this error > doesn't occur if template param edit is set as 1. > > To test: > 1. Find order with quantityreceived value more than 0. > 2. Try to receive this order. > => Error 500 page is displayed. > 3. Apply patch > 4. Repeat steps 1 and 2. > => Order receive page is displayed normally. > > Sponsored-by: Koha-Suomi Oy Please move this to a new bug, dependent on this one
Comment on attachment 131857 [details] Bug 23376: Fix template error on order receive page Created bug 30321 and obsoleted patch provided here.