Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimise Shipment#to_package #6466

Merged

Conversation

Matt-Yorkley
Copy link
Contributor

@Matt-Yorkley Matt-Yorkley commented Dec 8, 2020

What? Why?

This performance improvement is added in a Spree 2.2 commit to reduce the amount of processing done in larger orders. Spotted when looking at adjustments changes in 2-2-stable.

See: spree/spree@ab01b1e for details.

What should we test?

The #to_package method is used when checking different shipping rates for an order, and when calculating some adjustments. Maybe sanity check switching shipping methods at checkout and placing an order (with lots of items in the order, not just one)?

Release notes

Changelog Category: Technical changes

Backported a performance improvement for Shipment#to_package

This is done in a later Spree commit to reduce the amount of processing done in larger orders.

See: spree/spree@ab01b1e
@Matt-Yorkley Matt-Yorkley self-assigned this Dec 13, 2020
@Matt-Yorkley Matt-Yorkley marked this pull request as ready for review January 7, 2021 00:14
@filipefurtad0 filipefurtad0 self-assigned this Jan 14, 2021
@filipefurtad0 filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Jan 14, 2021
@sauloperez
Copy link
Contributor

Maybe sanity check switching shipping methods at checkout and placing an order (with lots of items in the order, not just one)?

@Matt-Yorkley would you mind checking if that's automated already?

@filipefurtad0
Copy link
Contributor

I've got this one @sauloperez 👍
This is the next chunk to be removem on manual release testing steps. This is the right opportunity to do it, so I'll test manually only what in not covered in specs.

Thanks for bringing this up!

@Matt-Yorkley
Copy link
Contributor Author

I think there are various specs that already cover this in different ways, I just thought a quick sanity-check might be a good idea just to make extra sure. It's a fairly simple change, but the #to_package logic is something we really don't want to break...

@filipefurtad0 filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-fr staging.coopcircuits.fr pr-staged-es and removed pr-staged-fr staging.coopcircuits.fr pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-es labels Jan 19, 2021
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Jan 20, 2021

Hey @Matt-Yorkley ,

I've made a brief sanity check here:

  • creating a shipping method with a fee, as a hub manager
  • placed an order with 40 line items in the shopfront (customer), selecting the newly created shipping method
  • in the BO, edited the order and:
  • switched between payment methods -> for 40 items, the page took around 10 sec. to load (before this PR it took more or less the same time -> maybe this is an inappropriate test for performance, or more line items are needed in the order?)

Met pre-existing issues: #6300 and #2881

Also met this UI-related: #6707

There are plenty of scenarios covered on the spec spec/features/admin/shipping_methods_spec.rb. I have some comments and questions, but they are not related to this PR, and as such maybe best discussed under #6693.

Anyway, I think this PR is good to go.

@filipefurtad0 filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 20, 2021
@Matt-Yorkley
Copy link
Contributor Author

Cool. I think at the point you switched in the backoffice, the rates have already been calculated, so it won't be faster at that specific point.

@andrewpbrett andrewpbrett merged commit 1d4fa29 into openfoodfoundation:master Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants