Bug 23376 - Cleanup order receive page code
Summary: Cleanup order receive page code
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 8179 30321 21811 26730 27671 28627 28881
  Show dependency treegraph
 
Reported: 2019-07-25 19:49 UTC by Nick Clemens
Modified: 2022-03-21 06:51 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 23376: Clean up order recipt page (22.77 KB, patch)
2019-07-25 19:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23376: Move AcqCreateItem logic to template (6.49 KB, patch)
2019-07-29 20:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23376: Remove unneeded 'count' parameter (1.47 KB, patch)
2019-07-29 20:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23376: Clean up order recipt page (23.06 KB, patch)
2020-09-24 18:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23376: Move AcqCreateItem logic to template (7.99 KB, patch)
2020-09-24 18:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23376: Remove unneeded 'count' parameter (1.51 KB, patch)
2020-09-24 18:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23376: Clean up order recipt page (23.12 KB, patch)
2020-10-05 15:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23376: Move AcqCreateItem logic to template (8.05 KB, patch)
2020-10-05 15:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23376: Remove unneeded 'count' parameter (1.57 KB, patch)
2020-10-05 15:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23376: (QA follow-up) Remove authnotrequired (998 bytes, patch)
2020-10-05 15:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23376: (follow-up) Fix subscription bug in orderreceive.tt (4.15 KB, patch)
2020-10-08 14:38 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23376: If a single item is returned we cannot count (956 bytes, patch)
2020-10-15 14:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23376: Fix template error on order receive page (2.07 KB, text/plain)
2022-03-18 07:21 UTC, Emmi Takkinen
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-07-25 19:49:03 UTC

    
Comment 1 Nick Clemens 2019-07-25 19:52:54 UTC
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
Comment 2 Nick Clemens 2019-07-29 20:06:13 UTC
Created attachment 91857 [details] [review]
Bug 23376: Move AcqCreateItem logic to template
Comment 3 Nick Clemens 2019-07-29 20:11:27 UTC
Created attachment 91858 [details] [review]
Bug 23376: Remove unneeded 'count' parameter
Comment 4 Agustín Moyano 2020-09-24 16:54:52 UTC
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
Comment 5 Agustín Moyano 2020-09-24 18:45:43 UTC
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>
Comment 6 Agustín Moyano 2020-09-24 18:45:47 UTC
Created attachment 110693 [details] [review]
Bug 23376: Move AcqCreateItem logic to template

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 7 Agustín Moyano 2020-09-24 18:45:52 UTC
Created attachment 110694 [details] [review]
Bug 23376: Remove unneeded 'count' parameter

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 8 Agustín Moyano 2020-09-24 18:47:06 UTC
I took the liberty to rebase and signoff all at the same time.

Nick, could you please check that rebase it's ok?

Thanks
Comment 9 Nick Clemens 2020-09-25 11:35:56 UTC
Heh, I harasses Agustin before testing, but yes, the rebase looks great, thank you!
Comment 10 Martin Renvoize 2020-10-05 15:48:39 UTC
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>
Comment 11 Martin Renvoize 2020-10-05 15:48:43 UTC
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>
Comment 12 Martin Renvoize 2020-10-05 15:48:47 UTC
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>
Comment 13 Martin Renvoize 2020-10-05 15:48:53 UTC
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>
Comment 14 Martin Renvoize 2020-10-05 15:49:49 UTC
Nice bit of cleaning up, thanks.

All works as expected still, no regressions found.

Passing QA
Comment 15 Jonathan Druart 2020-10-08 14:20:40 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 16 Jonathan Druart 2020-10-08 14:22:58 UTC
Nice one, Nick!
Comment 17 Agustín Moyano 2020-10-08 14:38:00 UTC
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
Comment 18 Jonathan Druart 2020-10-08 14:44:51 UTC
(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!
Comment 19 Nick Clemens 2020-10-15 14:51:52 UTC
Created attachment 111750 [details] [review]
Bug 23376: If a single item is returned we cannot count
Comment 20 Jonathan Druart 2020-10-20 15:51:31 UTC
(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?
Comment 21 Katrin Fischer 2020-10-20 15:54:32 UTC
I've tested some bugs with the patch as proposed, but not sure if there are side effects I have missed.
Comment 22 Jonathan Druart 2020-10-20 16:04:04 UTC
I've tried with count and it did not work (not sure why actually!)
Comment 23 Jonathan Druart 2020-10-20 16:04:24 UTC
(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!
Comment 24 Lucas Gass 2020-11-09 20:37:50 UTC
enhancement will not be backported to 20.05.x
Comment 25 Emmi Takkinen 2022-03-17 13:13:20 UTC
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).
Comment 26 Jonathan Druart 2022-03-17 14:26:23 UTC
(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.
Comment 27 Emmi Takkinen 2022-03-18 06:59:50 UTC
(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.
Comment 28 Emmi Takkinen 2022-03-18 07:21:41 UTC
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
Comment 29 Nick Clemens 2022-03-18 14:33:23 UTC
(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 30 Emmi Takkinen 2022-03-21 06:51:49 UTC
Comment on attachment 131857 [details]
Bug 23376: Fix template error on order receive page

Created bug 30321 and obsoleted patch provided here.