Dont assign distributor address to pickups #1642
Closed
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
|
not shown in interface but I think it might affect reports and maybe invoices, order interface etc? I’d be a bit cautious about thinking this has no implications anywhere
… On 27 Jun 2017, at 7:52 AM, Pau Pérez Fabregat ***@***.***> wrote:
This is one of the two parts of the Order model that on the way of the Spree 2.0 upgrade as stated in https://community.openfoodnetwork.org/t/order-model-step7/979 <https://community.openfoodnetwork.org/t/order-model-step7/979>.
We assign the distributor's address to the orders that are picked up (not delivered) but setting the name and phone number from the billing address. By inspecting the ceckout's UI as well the as the rails views, this is not shown anywhere, so there's no need to assign it in the first place.
Current behaviour
<https://user-images.githubusercontent.com/762088/27561920-685da4e6-5aca-11e7-962b-840cbf53d954.png>
With this PR
<https://user-images.githubusercontent.com/762088/27561944-86b17698-5aca-11e7-85b9-14e2e90c39a1.png>
You can view, comment on, or merge this pull request online at:
#1642 <#1642>
Commit Summary
Do not assign distributor's address to pickups
Remove assignment in assignment expression
File Changes
M app/controllers/checkout_controller.rb <https://github.com/openfoodfoundation/openfoodnetwork/pull/1642/files#diff-0> (8)
M app/models/spree/order_decorator.rb <https://github.com/openfoodfoundation/openfoodnetwork/pull/1642/files#diff-1> (19)
Patch Links:
https://github.com/openfoodfoundation/openfoodnetwork/pull/1642.patch <https://github.com/openfoodfoundation/openfoodnetwork/pull/1642.patch>
https://github.com/openfoodfoundation/openfoodnetwork/pull/1642.diff <https://github.com/openfoodfoundation/openfoodnetwork/pull/1642.diff>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#1642>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACxryFBaMsCujxatdtLRA4FohC-WfIK-ks5sICgogaJpZM4OF5un>.
|
When the shipping method is of pickup type (also referred as "collection") we don't show the order's ship address in the UI. There's no point on setting any ship_address in that case then.
sauloperez
force-pushed
the
dont-assign-distributor-address-to-pickups
branch
from
dbbfbec to
670851d
Compare
|
By looking at the uses of |
|
@sauloperez there might be some intereference as well with the issue @ltrls is working on #928, he will put his PR in progress on github today so that you can understand what he has done and see together if there is an impact. |
They encapsulate the abstractions of delivery and pickup shipping methods scattered throughout the code.
sauloperez
force-pushed
the
dont-assign-distributor-address-to-pickups
branch
from
8c16928 to
c304240
Compare
enricostano
reviewed
Jun 27, 2017
| order.should_receive(:ship_address=) | ||
| controller.send(:clear_ship_address) | ||
| shipping_method = build(:shipping_method) | ||
| shipping_method_instance = Pickup.new(order) |
There was a problem hiding this comment.
Do we really need this?
There was a problem hiding this comment.
Not really, the var is not used besides as return value of the stub
sauloperez
force-pushed
the
dont-assign-distributor-address-to-pickups
branch
from
c304240 to
2f54eb3
Compare
|
@myriamboure there is no interference so far, but athough doesn't look like there will be, since it's still WIP I can't tell for sure. |
|
We'll address the problems Part of the refactor introduced here is now in #1669. |
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.
This is one of the two parts of the
Ordermodel that get on the way of the Spree 2.0 upgrade as stated in https://community.openfoodnetwork.org/t/order-model-step7/979. TL;DR;#shipping_methodis removed fromOrderin that version.So far when saving an order we assign the distributor's address to the orders that are picked up (not delivered) but setting the name and phone number from the billing address.
By inspecting the ceckout's UI as well the as the rails views, this is not shown anywhere, so there's no need to assign it in the first place.
Current behaviour
With this PR
We also had the need to refactor shipping methos by introducing
Delivery,Pickupanddeliverty?.While reading the codebase we constantly find ourselves "computing"
the actual meaning og require_ship_addressin our heads to understand whether we're talking about a delivery or a pickup. This refactor makes the itent behind all this logic a bit more obvious, which makes the spree upgrade a bit easier.