Bug 23376 - Cleanup order receive page code
Summary: Cleanup order receive page code
Status: Pushed to master
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 21811 26730
  Show dependency treegraph
 
Reported: 2019-07-25 19:49 UTC by Nick Clemens
Modified: 2020-11-09 20:37 UTC (History)
5 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

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