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

942 billing to default address #1424

Merged

Conversation

peterberkenbosch
Copy link
Contributor

update for #942 including specs changes.

The PR stalled, cherry-picked the commit and updated the spec suite to
reflect the changes.

Closes #942

@mtomov
Copy link
Contributor

mtomov commented Jun 19, 2017

The fix is really good, especially coupled with #1967 - just cherry-picked both, and about to deploy them to production. It is indeed great to have this usability improvement.

Thanks for the fix @yeonhoyoon , and taking it up afterwards @peterberkenbosch !

@mamhoff
Copy link
Contributor

mamhoff commented Jun 20, 2017

@tvdeyen I know you fixed this in a client project, do you agree this works well?

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Sweet

@tvdeyen
Copy link
Member

tvdeyen commented Jun 20, 2017

@peterberkenbosch there is a conflict, could you please resolve?

@mamhoff no, that wasn't me, but this change here is great

Set `bill_address` to default address, if user has one.

Currently when order goes to `address` state, only ship address gets filled in from a previously entered address. This is because `ship_address` is set to default address because `ship_address` is an alias for `default_address` in the `UserAddressBook` module.
https://github.com/solidusio/solidus/blob/master/core/app/models/concerns/spree/user_address_book.rb#L53
However bill_address is not an alias, so it is not prefilled and user has to enter it every time. Rather than making it into an alias, just try default_address in `assign_default_address` if user has one.
@peterberkenbosch peterberkenbosch force-pushed the 942-billing-to-default-address branch from 8fcc940 to fd1bf51 Compare June 20, 2017 07:06
@peterberkenbosch
Copy link
Contributor Author

@tvdeyen Fixed the conflict, when CI is green this is mergeable.

@peterberkenbosch
Copy link
Contributor Author

And it's not :), failing on the introduced spec even. Will investigate and fix.

@peterberkenbosch peterberkenbosch force-pushed the 942-billing-to-default-address branch from fd1bf51 to c650a18 Compare June 20, 2017 16:29
@@ -191,7 +191,7 @@ def find_transition(options = {})
transition[options[:from].to_sym] == options[:to].to_sym
end
end

Choose a reason for hiding this comment

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

Trailing whitespace detected.

update for solidusio#942 including specs changes.

The PR stalled, cherry-picked the commit and updated the spec suite to
reflect the changes.

Closes solidusio#942
@peterberkenbosch peterberkenbosch force-pushed the 942-billing-to-default-address branch from c650a18 to abaa73f Compare June 20, 2017 16:31
@peterberkenbosch
Copy link
Contributor Author

So, in #1954 we deprecated Spree::Order#assign_default_addresses! and introduced Order.new.assign_default_user_addresses, I have updated this PR accordingly.

@peterberkenbosch
Copy link
Contributor Author

@tvdeyen we are all green here!

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks, Peter!

@gmacdougall gmacdougall merged commit 68d0dda into solidusio:master Jun 28, 2017
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.

7 participants