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.

Sign up for GitHub

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

Jump to bottom

[Spree 2 Upgrade] - Added adapted order.shipping_method #2654

Merged
merged 4 commits into from
Sep 11, 2018
Merged

[Spree 2 Upgrade] - Added adapted order.shipping_method #2654

merged 4 commits into from
Sep 11, 2018

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Sep 6, 2018 •
edited
Loading

What? Why?

Part of #2009

WIP: This is the quick fix, it still needs cleaning up, docs and tests.

What should we test?

More specs fixed.

Release notes

Changelog Category: Fixed
Adapted to spree 2 multi shipments by adding adapted shipping method to order.

How is this related to the Spree upgrade?

This is part fo the spree upgrade.

@luisramos0 luisramos0 self-assigned this Sep 6, 2018
@luisramos0 luisramos0 force-pushed the 2-0-ship-method branch 2 times, most recently from 3f4935f to a082478 Compare September 7, 2018 11:08
@luisramos0 luisramos0 mentioned this pull request Sep 8, 2018
Closed
@luisramos0 luisramos0 changed the title WIP - Added adapted order.shipping_method [Spree 2 Upgrade] WIP - Added adapted order.shipping_method Sep 8, 2018
@luisramos0 luisramos0 mentioned this pull request Sep 8, 2018
Closed
@luisramos0
Copy link
Contributor Author

luisramos0 commented Sep 9, 2018 •
edited
Loading

hey @sauloperez @mkllnk @HugsDaniel
Before I proceed here I'd like to confirm the approach with you guys.
Would you agree I should use the same approach here as in #2638 VariantStock?
I'd create something like a OrderShippingMethodConcern with this adapted version of shipping_method and shipping_method= (see shipping_method= here).

I think that would be the best approach. If you guys agree, I can create that class and write some unit tests and docs for it.
Thanks!

@luisramos0 luisramos0 mentioned this pull request Sep 9, 2018
Closed
@luisramos0 luisramos0 changed the title [Spree 2 Upgrade] WIP - Added adapted order.shipping_method [Spree 2 Upgrade] - Added adapted order.shipping_method Sep 9, 2018
@luisramos0 luisramos0 removed the pr-wip label Sep 9, 2018
@luisramos0
Copy link
Contributor Author

Moving to "Code Review" to get feedback. Will move back to "In Dev" once feedback is gathered and decisions taken.

@mkllnk
Copy link
Member

mkllnk commented Sep 9, 2018

I like the separation in a concern. The decorator is too big anyway and I don't see any downside.

sauloperez
sauloperez reviewed Sep 10, 2018
View reviewed changes
app/models/spree/order_decorator.rb Outdated
# Thus, this method returns the shipping method of the first and only shipment in the order
def shipping_method
return if shipments.empty?
shipments.first.shipping_method
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should enforce this at DB level as we did in #2638.

@sauloperez
Copy link
Contributor

Agree @luisramos0

@luisramos0
Copy link
Contributor Author

all right, thanks for the feedback!

@luisramos0 luisramos0 changed the title [Spree 2 Upgrade] - Added adapted order.shipping_method [Spree 2 Upgrade] - WIP - Added adapted order.shipping_method Sep 10, 2018
@luisramos0 luisramos0 changed the title [Spree 2 Upgrade] - WIP - Added adapted order.shipping_method [Spree 2 Upgrade] - Added adapted order.shipping_method Sep 10, 2018
@luisramos0 luisramos0 removed the pr-wip label Sep 10, 2018
@luisramos0
Copy link
Contributor Author

This is ready for review with new concern, unit tests, docs and db constraint.

@luisramos0
Moved order.shipping_method to the OrderShippingMethod concern
3577f37
@luisramos0
Added DB uniqueness constraint on order_id to shipments
c00e6a5 
By forbidding more than a row per order in the spree_shipments table we ensure all orders have no more than one shipment associated
@luisramos0
Copy link
Contributor Author

I have copied the approach from #2638 See this comment:
#2638 (comment)

sauloperez
sauloperez reviewed Sep 11, 2018
View reviewed changes
db/migrate/20180910155506_add_uniqueness_of_order_id_to_spree_shipments.rb
remove_index :spree_shipments, :order_id
add_index :spree_shipments, :order_id, unique: true
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sauloperez sauloperez merged commit 288f685 into openfoodfoundation:2-0-stable Sep 11, 2018
@sauloperez
Copy link
Contributor

I couldn't stop myself @luisramos0 :trollface: . I hope there was nothing missing.

@luisramos0
Copy link
Contributor Author

ahaha, awesome, lets do this 🎉

@luisramos0 luisramos0 deleted the 2-0-ship-method branch September 11, 2018 10:09
@luisramos0
Copy link
Contributor Author

Marking this one as part of #2009, but not closing it.

@luisramos0
Copy link
Contributor Author

I have changed my mind. I am closing #2009.
I have opened #2685 to fix the order_spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkllnk mkllnk mkllnk approved these changes

@sauloperez sauloperez sauloperez approved these changes

Assignees

@luisramos0 luisramos0

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@luisramos0 @mkllnk @sauloperez