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