search

solidusio / solidus

🛒 Solidus, the open-source eCommerce framework for industry trailblazers.
https://solidus.io
Other
5.03k stars 1.29k forks source link

Remove transition notice from name splitter #3627

Closed seand7565 closed 2 years ago

seand7565 commented 4 years ago

https://github.com/solidusio/solidus/blob/afd7f5b3bc1f3701012b7932725941aa772e04f8/core/app/models/spree/address/name.rb indicates that the name splitter will be removed at some point in the future, however this is a feature that is useful for a number of reasons - for instance, PayPal expects a given & surname for billing addresses. We should keep the splitting functionality, and rephrase the comments so they reflect that the functionality will stay with the platform.

(this issue has changed given the comments below - here's the previous copy for historical purposes)

Is your feature request related to a problem? Please describe. There are a few cases in which splitting a name into parts - like first_name and last_name - is desirable. In my case, PayPal expects a given & surname, rather than one fullname. https://developer.paypal.com/docs/api/orders/v2/#definition-payer.name

Splitting names is problematic for a few reasons (see #3234 ), but necessary when working with APIs that haven't adopted the same standards that we have.

Describe the solution you'd like I think we should implement a name splitter class to handle splitting names in these cases. We could define a simple splitting method:

"Sean PH Denny".split(" ",2) 
#["Sean", "PH Denny"]

which could be easily swapped out with a custom splitter, perhaps to conform to local naming conventions.

This should be pretty simple to build out, but I would love to hear some opinions on this!

elia commented 4 years ago

The article cited by @tvdeyen in #3234 (https://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/) is a sure prediction that given any system the probability they got something wrong is almost 100% 😅

I can work on this if that's fine for the core team 👍

kennyadsl commented 4 years ago

I think it's already in core, we can just use https://github.com/solidusio/solidus/blob/afd7f5b3bc1f3701012b7932725941aa772e04f8/core/app/models/spree/address/name.rb

elia commented 4 years ago

I think it's already in core

Great! Maybe I'll update the comment to avoid giving the impression this will go away once the transition is completed:

https://github.com/solidusio/solidus/blob/afd7f5b3bc1f3701012b7932725941aa772e04f8/core/app/models/spree/address/name.rb#L5-L6

jarednorman commented 4 years ago

Yeah, I suspect many stores will need to interface with external systems that make different assumptions about names, so it should remain useful.

kennyadsl commented 2 years ago

Closing this, should be addressed.