Make last names optional on Spree::Address

[jordan-brough]

For example, we're integrating with Amazon Payments right now and
Amazon doesn't require two names, which breaks this validation. And
it's hard to remove an ActiveRecord validation at the app level.

In chat @jhawthorn mentioned that it'd be nice to move to a single name field eventually, and also pointed to https://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/.

Some ideas I can think of:

• Removal: Remove the validation on lastname (the current state of this PR)
• Optional: Make the validation on lastname optional (probably defaulting to true for backwards-compatibility?)
• Validate full_name: Do validates_presence of :full_name instead of validating each field individually
• Combine: See if there is any progress we can make toward merging the firstname and lastname fields. Perhaps combine the DB fields into a new name field and have ruby methods for firstname and lastname (i.e. the opposite of what we have now)

Other thoughts?

[Sinetheta]

Spree::CreditCard already has a single name credit_card.rb.

When mailing things a "recipient line" is a single string for all shippers I can find, USPS.

I think that if an application wants to record first and last names, that's their business. It would be great is Solidus did not require them on addresses.

[jordan-brough]

Some other good points brought up in Solidus chat:

@peterberkenbosch mentioned that separate first & last names are nice for personalization, e.g. "Hello Mary Jo" instead of "Hello Mary Jo Smith".

@Sinetheta noted that personalization breaks down for people that don't fit the firstname/lastname mold, so maybe we shouldn't worry about it (at the Solidus level at least).

@jrochkind suggested that perhaps we don't needs names on Address at all for shipping purposes, as shipping services don't require names. And for things like company addresses there may not be a 'name' (or at least a name with first & last parts). He also mentioned that billing addresses may have different requirements.

@jhawthorn noted that Spree::CreditCard already has just a single name field.

About credit cards: We currently split credit card names apart for ActiveMerchant, but that whitespace-splitting seems to have worked well enough so far. Braintree has separate first/last name fields while Stripe has a single name field.

[tvdeyen]

I think we should not require last name to be present, but it's ok to store them separately. Once concatenated you can't split a name in first and last name anymore. Simple using split(" ") won't work (think of my full name).

So, I say 👍 for this PR

[gmacdougall]

I would prefer to leave this as is. Since the factories are designed to be included in other people's app's, some people may be relying upon 'Doe' to still be the last name. I don't expect it will be a major issue, but there's no reason we can't leave this as it was.

[jordan-brough]

@gmacdougall I can see what you mean, and I hate to break specs, but we'd rather break their specs than their app, right? E.g., we're trying this out on our own codebase and having the factory change helped us find a couple extra places where we weren't handling a possibly-nil last name. In general we want factories not to set attributes that aren't actually required and this attribute is no longer required right?

[jrochkind]

In general we want factories not to set attributes that aren't actually required

Is this true? I think of factories as creating 'typical' objects, not neccessarily minimal objects.

[jordan-brough]

I hope it's true. We don't follow that strictly but I think it makes for more robust tests if we try to make factories minimal by default. E.g. for this lastname scenario exactly -- you end up writing specs around attributes that the default factories provide but that aren't actually guaranteed to be there. It's easy to add additional factories that are more "filled out", but I think it's nice to make that explicit. Apps can also use FactoryGirl.modify if they'd like to have lastnames populated by default.

[jordan-brough]

@gmacdougall @jrochkind any further thoughts on this? We've been using this in our app for a while now and are happy with it.

[jrochkind]

I'm not sure what I think about the theory of factories being intended to make typical vs minimal objects -- perhaps there sometimes need to be mutiple versions of a factory to handle multiple cases (see product, with base_product, product, and then various even more complicated products), but I have no strong feelings either way about this particular change, it seems fine. Someone can later add a person_with_lots_of_stuff factory if needed.

[gmacdougall]

I'm not particularly bothered either way.

[jasonfb]

@jrochkind suggested that perhaps we don't needs names on Address at all for shipping purposes, as shipping services don't require names.

Which shipping services don't require names? This doesn't make sense to me, when we create Return labels on the FedEx api, I'm fairly certain it will hiccup if it doesn't have both a first & last name in there (as the FROM address... it even needs a phone number)

Also I can check with our 3PL but they probably need a first & last name too.

[jasonfb]

I think removing AR validations isn't as difficult as it is made out to be, it's just really counter-intuitive looking code.
here's what we have in our address decorator for removing the phone validation, works flawlessly

  _validators.reject!{ |key, value| key == :phone }
  _validate_callbacks.each do |callback|
    callback.raw_filter.attributes.reject! { |key| key == : phone } if callback.raw_filter.respond_to?(:attributes)
  end

[jasonfb]

I guess my over-arching feeling on the whole thing is 👎 because the whole industry already uses two separate for first and last name, nearly all our external providers are implemented this way, and as soon as you combine the name into a full name you immediately run the very real probably that someone, somewhere down the line is going to parse that full name when they need a first & last for some 3rd party website (we literally integrate with about 15 external providers, none of whom I think have consolidated first & last name fields in their systems)... and as soon as you write a parser for the names you are invariably going to get it wrong.

Whereas the use case of an external provider (as originally stated the impetus at the top of the PR) like Amazon that needs a single name field is obviously much easier and involves simply concatenating two strings with a simple space.

[jrochkind]

@jasonfb re removing validations: I guess I'd worry that relying on Rails internals like that is likely to break in future Rails versions, and require me to spend serious time doing detective work to figure out the 'new' undocumented/unsupported way to do it.

But I think you're right to identify the difficulty of removing validations as the key problem here. If there were a better/less-confusing/more-supported way to remove existing validations as a general tool, it would make Solidus model classes more customizable and reduce need for special purpose customizations or getting everything exactly right for every possible app. I wonder if Solidus could provide some generalized support for that, somehow.

[jordan-brough]

Re Validations: Ditto on what @jrochkind said. It would be sad to have that be the official method to remove the last name validation in Solidus. I think no-last-name is a perfectly reasonable scenario to support in a good way in Solidus core.

Personally I'm OK with leaving the fields separate, for now at least, but I do think we need to at least support nil last names.

[jordan-brough]

when we create Return labels on the FedEx api, I'm fairly certain it will hiccup if it doesn't have both a first & last name in there

@jasonfb do you have any example links for that? I tried looking around but in this fedex doc and this ruby gem I only see PersonName (like Amazon) and no first/last name fields. Which part of their API requires first & last names for addresses?

Likewise for UPS looking around in places like this all I see are things like Name, CompanyName, and AttentionName.

Do you have any examples of services that require separate first and last names on addresses and where an invalid whitespace split (e.g. "Mary" "Jo Smith" instead of "Mary Jo" "Smith") would cause a problem?

[jordan-brough]

Does anyone know of any examples of third party services that would require an address (not a user) to have both first and last names?

[jasonfb]

Will investigate

You are indeed correct, our FedEx code passes the full name, I was wrong

https://github.com/mackweldon/spree_shipping_labeler/blob/master/app/models/spree/address_decorator.rb

[jasonfb]

I guess the issue is also personal for me, because if you write your whitespace parser to include hyphens are white space, then my full name ("Jason Fleetwood-Boldt") becomes "Jason Fleetwood" and "Boldt" which I personally would not appreciate. (My entire life tell people looking things up alphabetically to look first under "F" and then under "B")

If, however, you write your whitespace parser to treat hyphens as characters, then I become "Jason" and "Fleetwood-Boldt" which is how I identify.

Then again, if I had followed the Spanish tradition (my father is from Chile), I would be "Jason Boldt Fleetwood" and "Fleetwood" would be my last name and "Boldt" would be my middle name.

I guess I wonder what the rational for this is--- is it to eliminate the idea of "First name" and "last name" conceptually, and let people use whatever name they feel represents themselves? (Which I get, on a social/ethical/cultural level).

In Vietnam and China (and other sinosphere countries, and if you're from the planet Bajor in the Star Trek universe-- yes I know I got really dorky there), family names come first followed by common, which even further supports your argument for single full name fields.

So although I started this post questioning the rationally perhaps I've talked myself into it simply on the cultural-sensitivity argument.

[jrochkind]

Yes, I think the main reason to eliminate them is that internationally not everyone has a 'first name' and a 'last name'. Some people only have one name. Some people have multiple names (including more than two), and it's not obvious which is the 'first name' and which is the 'last name', as in your Asian examples. Some people switch to 'given name' and 'family name' to try and avoid the Asian confusion, but this doesn't really avoid the confusion, names are just way more variable internationally than one from the U.S. might assume.

Also when it comes to shipping addresses especially -- aren't shipping addresses not uncommonly a company name followed by an address? Companies/organizations also don't tend to have 'first names' and 'last names'. There is a company field in the Spree::Address -- but what if my shipping address is simply "JRochkind LLC, 10 Maple Drive, etc"?

[jordan-brough]

It sounds like we have a pretty good consensus that this PR is OK, and there may be future improvements we can make (like combining first and last names)? Shall we go ahead and merge this?

[jordan-brough]

@gmacdougall per your latest comment are you 👍 on this PR as a whole?
@jhawthorn you had some thoughts on this iirc? Seem OK? I like the idea of combining first and last names but perhaps this can be a stepping stone to that?

[gmacdougall]

Yes. 👍

[mtomov]

And an interesting read on the very topic I came across just now

Caleb Thompson - Use One Field to Store Names or Addresses

[jhawthorn]

@jordan-brough Sorry. This caused tests to fail on master so I had to revert it.

66abad4
https://circleci.com/gh/solidusio/solidus/4433