Replace order simulator with something simpler #82
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seand7565
force-pushed
the
fix_order_simulator
branch
4 times, most recently
from
eeea785 to
1978e5d
Compare
seand7565
changed the title
elia
requested changes
Aug 3, 2020
Comment on lines
11
to
41
| @order.transaction do | ||
| build_address(params[:address]) | ||
| @order.ensure_updated_shipments | ||
| @order.email = "[email protected]" unless @order.email | ||
| @order.contents.advance | ||
| @paypal_order = SolidusPaypalCommercePlatform::PaypalOrder.new(@order).to_replace_json | ||
| raise ActiveRecord::Rollback | ||
| end | ||
|
|
||
| render json: @paypal_order, status: :ok | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def build_address(address) | ||
| country = ::Spree::Country.find_by(iso: address[:country_code]) | ||
| # Also adds fake information for a few fields, so validations can run | ||
| new_ship_address = ::Spree::Address.new( | ||
| city: address[:city], | ||
| state: country.states.find_by(abbr: address[:state]), | ||
| state_name: address[:state], | ||
| zipcode: address[:postal_code], | ||
| country: country, | ||
| address1: "123 Fake St.", | ||
| phone: "123456789", | ||
| firstname: "Fake" | ||
| ) | ||
|
|
||
| @order.update(ship_address: new_ship_address) | ||
| end | ||
|
|
There was a problem hiding this comment.
What about moving most of this to an external object and keep the controller a little slender?
Ideally the action should look more or less like this:
@paypal_order = SolidusPaypalCommercePlatform::PaypalOrder.new(@order)
render json: @paypal_order.to_replace_json(address: params[:address])There was a problem hiding this comment.
I moved the address update functionality to SPCP::PayPalAddress, since its responsibility is to format paypal addresses to Spree addresses. How does that look?
We were updating only the address when PayPal returns the users address, but we also need to make sure that the shipments are relevant to the new address also. This makes sure the shipments are regenerated when the address changes.
Previously when we got a partial address from PayPal, we would dup the order, and use it in its unsaved state to simulate the new address, so nothing would be actually changed or added to the DB. However, this does not play well with a lot of Solidus methods, like promotion activation (because it calls `create!` which doesn't work with unsaved objects). It was also becoming a bit too complex. This PR moves to simplify all of that by just wrapping the address change in an ActiveRecord transaction and rolling it back after we get the data we need to send back to PayPal. In essence, this will change the orders ship_address, generate new shipments, and format all of that to be sent to PayPal, and then undo all of the actions it just took to generate that data, leaving the order untouched at the end.
seand7565
force-pushed
the
fix_order_simulator
branch
from
1978e5d to
451ea98
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.