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 Upgrade] Fixed inexistent order#shipping_method= in several specs #2680

Merged
merged 7 commits into from
Sep 24, 2018
Merged

[Spree Upgrade] Fixed inexistent order#shipping_method= in several specs #2680

merged 7 commits into from
Sep 24, 2018

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Sep 11, 2018 •
edited
Loading

Closes #2662

Instead of writing an adapter (as in #2669 ), the best solution here is to fix all specs that need this method. Production code may actually not require to set shipping_method like this.

Test

Acc Criteira: no "undefined method order.shipping_method=" errors in specs

@luisramos0 luisramos0 changed the base branch from master to 2-0-stable September 11, 2018 22:02
@luisramos0 luisramos0 self-assigned this Sep 11, 2018
@mkllnk
Copy link
Member

mkllnk commented Sep 13, 2018

@luisramos0 May I ask why you create WIP pull requests?

@luisramos0
Copy link
Contributor Author

:-)
The first valid reason is that I need early feedback. Not the case here.
The second not-so-valid reason is that I spent some time without success trying to run semaphore builds on my branches. The PR is the easier option to run the build against a branch.

@mkllnk
Copy link
Member

mkllnk commented Sep 14, 2018 •
edited
Loading

@luisramos0 Have you seen my documentation of configuring Semaphore? https://github.com/openfoodfoundation/openfoodnetwork/wiki/Continuous-Integration#semaphore-ci

IMHO that's a downside of Semaphore, upside of Travis that the config is in the code already.

@luisramos0
Copy link
Contributor Author

ah, nice, I'll give it a try now. thanks for that!

@luisramos0 luisramos0 changed the title [Spree Upgrade] WIP - Fixes serializers shipping_method_id [Spree Upgrade] WIP - Replaces shipping_method_id in basic_enterprise_serializer Sep 14, 2018
This was referenced Sep 14, 2018
Closed
Closed
@luisramos0 luisramos0 changed the title [Spree Upgrade] WIP - Replaces shipping_method_id in basic_enterprise_serializer [Spree Upgrade] WIP - Sep 16, 2018
@luisramos0 luisramos0 changed the title [Spree Upgrade] WIP - [Spree Upgrade] WIP - Fix inexistent order#shipping_method= in several specs Sep 16, 2018
@luisramos0 luisramos0 force-pushed the 2-0-stable-x branch 4 times, most recently from 3e3823c to bff672e Compare September 16, 2018 23:41
@luisramos0
Copy link
Contributor Author

I have gone through all the specs mentioned in issue #2662. This is ready for review.

@luisramos0 luisramos0 removed the pr-wip label Sep 16, 2018
@luisramos0
Added new factory for shipping_method_with trait delivery to fix orde…
93ac99b 
…r.shipping_method issue in features/admin/orders_spec
@luisramos0
Fixed controllers/spree/admin/orders/customer_details_controller_spec:
7b6e482 
- fixed order creation to use order.shipments instead of order.shipping_method
- adapted test to new spree 2 controller logic (shipments page is gone since 67f568914988bcc0a1fc520d15ed6444a6d12824 and redirect logic changed on e9cde1b4d570dd4f7f979ac71a58d6f3f342ebb4)
@luisramos0 luisramos0 changed the title [Spree Upgrade] WIP - Fix inexistent order#shipping_method= in several specs [Spree Upgrade] - Fix inexistent order#shipping_method= in several specs Sep 16, 2018
@luisramos0 luisramos0 changed the title [Spree Upgrade] - Fix inexistent order#shipping_method= in several specs [Spree Upgrade] - Fixed inexistent order#shipping_method= in several specs Sep 16, 2018
@luisramos0
Moved shipment and shipping_method factories with flat rate and shipp…
fbd2d96 
…ing fees to traits
@luisramos0
In features/admin/reports_spec, fixed setting shipping_method in orde…
38c2f52 
…r, this is now done through order shipments
@luisramos0
Fixed query in order_cycle_management_report and fixed the respective…
7565666 
… spec by setting shipping_method in order through shipments
@luisramos0
Fixed setting shipping_method in the order through order.shipments in…
c66b611 
… proxy_order_spec, lib/open_food_network/customers_report_spec and features/admin/shipping_methods_spec
@luisramos0
In paypal_spec, fixed setting shipping_method in order, this is now d…
843cd44 
…one through order shipments
@luisramos0 luisramos0 changed the title [Spree Upgrade] - Fixed inexistent order#shipping_method= in several specs [Spree Upgrade] Fixed inexistent order#shipping_method= in several specs Sep 16, 2018
@HugsDaniel HugsDaniel mentioned this pull request Sep 21, 2018
Merged
This was referenced Sep 21, 2018
Merged
Closed
mkllnk
mkllnk approved these changes Sep 23, 2018
View reviewed changes
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Looks good.

Off topic: The Spree upgrade seems to make a lot of things more complicated, because we haven't found the right way yet to bridge the OFN domain with Spree's domain. We will have to do some refactoring once it's all green.

@luisramos0
Copy link
Contributor Author

yes, we need to continuously refactor. do let me know if you are thinking of some specific refactoring, in some cases we may just do it now.
Also, we should create refactoring issues as we go. Specially for the important ones.
For example, I have just created #2763.

@luisramos0
Copy link
Contributor Author

2 approvals, moving to ready to go.

@sauloperez
Copy link
Contributor

sauloperez commented Sep 24, 2018 •
edited
Loading

Off topic: The Spree upgrade seems to make a lot of things more complicated, because we haven't found the right way yet to bridge the OFN domain with Spree's domain. We will have to do some refactoring once it's all green.

I totally understand your point and I believe we're getting better at that but, I think the tight coupling to Spree's DB schema won't be easy to overcome.

@luisramos0 luisramos0 merged commit 0eb2854 into openfoodfoundation:2-0-stable Sep 24, 2018
@luisramos0 luisramos0 deleted the 2-0-stable-x branch September 24, 2018 14:08
@luisramos0 luisramos0 mentioned this pull request Oct 30, 2018
Merged
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