Add name to Spree::Address #3458

filippoliverani · 2019-12-16T09:06:27Z

Description
This PR proposes a first step to solve #3234 .

The main idea is to add a new name attribute to Spree::Address and to deprecate firstname, lastname and full_name usage while keeping full backward compatibility.

This PR adds a virtual attribute name that replaces full_name and favors reading form it when accessing Address data. It uses a Spree::Address::Name value object to handle name parsing during deprecation period.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have added tests to cover this change (if needed)

core/app/models/spree/order.rb

kennyadsl

Thanks, Filippo. I left some comments, let me know your thoughts.

Also, I've found this code into ActiveMerchant. Curious how they do the opposite to split the full name into different parts, do you prefer this way for a specific reason?

Still not sure about the Rubocop fixes merged in the PR. I'd love to keep them but these PRs are very critical and I'd focus their content on the changes only.

core/app/models/spree/address.rb

core/spec/lib/spree/name_spec.rb

filippoliverani · 2019-12-16T16:21:41Z

@kennyadsl Thanks for the feedback!

Also, I've found this code into ActiveMerchant. Curious how they do the opposite to split the full name into different parts, do you prefer this way for a specific reason?

I just extracted the code used in Spree::CreditCard#first_name to avoid reinventing the wheel, no preferences about one style or the other.

Still not sure about the Rubocop fixes merged in the PR. I'd love to keep them but these PRs are very critical and I'd focus their content on the changes only.

They seemed small enough to me but I can keep only the ones enforced by Hound to reduce changes.

core/lib/spree/name.rb

spaghetticode

@filippoliverani thank you for working on this, I left some minor comments, I think this is a great start!

frontend/app/views/spree/checkout/payment/_gateway.html.erb

core/app/models/spree/address.rb

core/lib/spree/name.rb

core/app/models/spree/order.rb

filippoliverani · 2019-12-20T10:28:49Z

@spaghetticode Thank you for your feedback!

backend/app/views/spree/admin/shared/_address.html.erb

spaghetticode

@filippoliverani thank you! Still one small nitpick, otherwise looks good to me 👍

backend/app/views/spree/admin/shared/_address.html.erb

tvdeyen

Thanks so much. This is a great change and very good work on the backwards compatibility 👏

I do think we should deprecate first_name and last_name as well.

Again, awesome work 🎉

core/app/models/spree/address/name.rb

tvdeyen · 2020-01-16T10:29:28Z

core/app/models/spree/credit_card.rb

@@ -163,7 +163,7 @@ def has_payment_profile?
    #   the object to ActiveMerchant.
    # @return [String] the first name on this credit card
    def first_name
-      name.to_s.split(/[[:space:]]/, 2)[0]
+      Spree::Address::Name.new(name).first_name


We should deprecate this method and move the logic into solidus_gateway, respectively into the payment method extensions. Having ActiveMerchant related logic in core seems odd.

I think this could be part of another PR since there are also other (unrelated) methods that we should deprecate under the same motivation. I'd say to keep this as it's minimum since it's already an important/critical change.

tvdeyen · 2020-01-16T10:29:40Z

core/app/models/spree/credit_card.rb

@@ -172,7 +172,7 @@ def first_name
    #   the object to ActiveMerchant.
    # @return [String] the last name on this credit card
    def last_name
-      name.to_s.split(/[[:space:]]/, 2)[1]
+      Spree::Address::Name.new(name).last_name


Ditto deprecate this method.

filippoliverani

Thanks so much. This is a great change and very good work on the backwards compatibility 👏

I do think we should deprecate first_name and last_name as well.

Again, awesome work 🎉

Thank you @tvdeyen 🙏

The PR does not include deprecation yet to keep the amount of code changed at minimum. I was thinking to split the whole work into 4-5 PRs to ease the review work, this is a proposed roadmap #3234 (comment).

This is the first non-destructive step to have a unified name attribute. Spree::Name class is needed to perfortm the conversions that will ease the transition to name field.

The logic for splitting a name and concatenating name components should be centralized in a single class.

kennyadsl

Thanks, @filippoliverani. This is awesome!

houndci-bot reviewed Dec 16, 2019

View reviewed changes

core/app/models/spree/order.rb Outdated Show resolved Hide resolved

filippoliverani changed the title ~~Add name to address #1~~ Add name to Spree::Address Dec 16, 2019

filippoliverani force-pushed the add-name-to-address-#1 branch from db38029 to ce67527 Compare December 16, 2019 09:32

filippoliverani marked this pull request as ready for review December 16, 2019 10:22

kennyadsl reviewed Dec 16, 2019

View reviewed changes

filippoliverani mentioned this pull request Dec 16, 2019

RFC: Remove firstname and lastname from adresses #3234

Closed

kennyadsl reviewed Dec 17, 2019

View reviewed changes

core/lib/spree/name.rb Outdated Show resolved Hide resolved

jacobherrington added the Code Review Needed label Dec 18, 2019

spaghetticode reviewed Dec 20, 2019

View reviewed changes

frontend/app/views/spree/checkout/payment/_gateway.html.erb Outdated Show resolved Hide resolved

core/app/models/spree/address.rb Outdated Show resolved Hide resolved

core/lib/spree/name.rb Outdated Show resolved Hide resolved

core/app/models/spree/order.rb Outdated Show resolved Hide resolved

filippoliverani force-pushed the add-name-to-address-#1 branch from ce67527 to 93da764 Compare January 9, 2020 15:30

houndci-bot reviewed Jan 9, 2020

View reviewed changes

backend/app/views/spree/admin/shared/_address.html.erb Outdated Show resolved Hide resolved

backend/app/views/spree/admin/shared/_address.html.erb Outdated Show resolved Hide resolved

backend/app/views/spree/admin/shared/_address.html.erb Outdated Show resolved Hide resolved

filippoliverani mentioned this pull request Jan 10, 2020

Add name to Spree::Address nebulab/solidus#30

Closed

spaghetticode approved these changes Jan 13, 2020

View reviewed changes

backend/app/views/spree/admin/shared/_address.html.erb Outdated Show resolved Hide resolved

filippoliverani force-pushed the add-name-to-address-#1 branch from 52e2efd to 3b59c0c Compare January 15, 2020 12:04

filippoliverani requested review from kennyadsl and tvdeyen January 16, 2020 10:12

tvdeyen reviewed Jan 16, 2020

View reviewed changes

filippoliverani commented Jan 17, 2020

View reviewed changes

filippoliverani added 3 commits January 17, 2020 09:22

Add name to Spree::Address

682b439

This is the first non-destructive step to have a unified name attribute. Spree::Name class is needed to perfortm the conversions that will ease the transition to name field.

Use Spree::Address::Name to handle credit card names

991083e

The logic for splitting a name and concatenating name components should be centralized in a single class.

Fix Erb Lint violations in Address partial

8220390

filippoliverani force-pushed the add-name-to-address-#1 branch from 3b59c0c to 8220390 Compare January 17, 2020 08:43

kennyadsl approved these changes Jan 20, 2020

View reviewed changes

kennyadsl merged commit d0d1e3d into solidusio:master Jan 20, 2020

filippoliverani mentioned this pull request Feb 25, 2020

Use Address name attribute in views and APIs #3524

Merged

5 tasks

brchristian mentioned this pull request Jan 29, 2021

Maintain compatibility with Solidus 3.0 Address#name solidusio/solidus_stripe#100

Closed

forkata mentioned this pull request Feb 5, 2021

Fix tests and allow support for extension to be used on Solidus 3 solidusio-contrib/solidus_gdpr#36

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add name to Spree::Address #3458

Add name to Spree::Address #3458

filippoliverani commented Dec 16, 2019 •

edited

Loading

kennyadsl left a comment

filippoliverani commented Dec 16, 2019

spaghetticode left a comment

filippoliverani commented Dec 20, 2019

spaghetticode left a comment

tvdeyen left a comment

tvdeyen Jan 16, 2020

kennyadsl Jan 16, 2020

tvdeyen Jan 16, 2020

filippoliverani left a comment •

edited

Loading

kennyadsl left a comment

Add name to Spree::Address #3458

Add name to Spree::Address #3458

Conversation

filippoliverani commented Dec 16, 2019 • edited Loading

kennyadsl left a comment

Choose a reason for hiding this comment

filippoliverani commented Dec 16, 2019

spaghetticode left a comment

Choose a reason for hiding this comment

filippoliverani commented Dec 20, 2019

spaghetticode left a comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

tvdeyen Jan 16, 2020

Choose a reason for hiding this comment

kennyadsl Jan 16, 2020

Choose a reason for hiding this comment

tvdeyen Jan 16, 2020

Choose a reason for hiding this comment

filippoliverani left a comment • edited Loading

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

filippoliverani commented Dec 16, 2019 •

edited

Loading

filippoliverani left a comment •

edited

Loading