solidusio / solidus

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

RFC: Remove firstname and lastname from adresses #3234

Closed tvdeyen closed 3 years ago

tvdeyen commented 5 years ago

The problem

Names :)

Seriously, read https://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/

Also issues like https://github.com/solidusio/solidus_paypal_braintree/issues/226 happen

The proposal

We should add a name column to Spree::Address then deprecate and later remove firstname and lastname.

ericsaupe commented 5 years ago

I'm all for this. It simplifies a lot of the edge cases surrounding names and how to display them. I can see use cases where they want a bit more of a restricted naming experience a gem can be created to handle those cases.

jacobherrington commented 5 years ago

I have nothing against this -- I think it's a great change.

kennyadsl commented 5 years ago

I also am in favor of this.

skukx commented 4 years ago

Here Here

filippoliverani commented 4 years ago

I really like this change and I started to work on it in #3447 but with the help of @kennyadsl I realized it was too much for a single PR.

I'm tring to split the work into four different steps:

  1. add name virtual attribute which still relies on firstname and lastname (#3465 and #3458)
  2. use name in views e api responses instead of lastname and firstname if an ad hoc show_address_deprecated_attributes preference is set to true
  3. add name column to Adress db table, add a rake data migration task and keep attributes in sync during updates
  4. deprecate show_address_deprecated_attributes preference, firstname, lastname and full_name

What do you think?

kennyadsl commented 3 years ago

We now have the codebase using the combined name everywhere but this is still not done since we don't have a name database attribute and we can't set the name attribute in a clean way, for example when using address.update_attributes(name: 'Something').

At the current stage we see 3 options to move this forward:

Option 1:

Override how Rails sets attributes and instead of using its own column, split name into the two db fields that we currently have.

Option 2:

Add name to the database and provide a fallback when name is not set. The job to migrate can be performed async.

Option 3:

Add name to the database and provide a migration/rake task that combines all the old firstname and lastname into `name. Can take forever on old stores with a lot of records.

kennyadsl commented 3 years ago

We discussed this during the core team, we'd go with option 2 (adding a fallback when name is not present) and creating a migration that has multiple options and each store can select the best one based on their needs. We did something similar here that we can replicate.

The handlers we were thinking of are:

tvdeyen commented 3 years ago

We might be able to help with the migration since we already done that for one of our clients.

cc @mamhoff

kennyadsl commented 3 years ago

@tvdeyen that would be great, thanks!