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

RFC: Remove firstname and lastname from adresses #3234

Closed
tvdeyen opened this issue Jun 19, 2019 · 9 comments
Closed

RFC: Remove firstname and lastname from adresses #3234

tvdeyen opened this issue Jun 19, 2019 · 9 comments

Comments

@tvdeyen
Copy link
Member

tvdeyen commented Jun 19, 2019

The problem

Names :)

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

Also issues like solidusio/solidus_paypal_braintree#226 happen

The proposal

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

@ericsaupe
Copy link
Contributor

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
Copy link
Contributor

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

@kennyadsl
Copy link
Member

kennyadsl commented Jul 10, 2019

I also am in favor of this.

@skukx
Copy link
Contributor

skukx commented Oct 24, 2019

Here Here

@filippoliverani
Copy link
Contributor

filippoliverani commented Dec 16, 2019

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 (Refactor Spree::Address value_attributes #3465 and Add name to Spree::Address #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
Copy link
Member

kennyadsl commented Jan 19, 2021

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
Copy link
Member

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:

  • RaiseException (default): it will tell people that this thing needs their attention.
  • MigrateData: it combines firstname and lastname (with SQL?) and moves the result into name.
  • DoNothing: it allows people to keep the current database structure, they can re-add firstname and lastname in the codebase to keep using the old way of setting names or maybe they can execute the migration in the background later if they are worried about the migration to be too time-consuming for their database size.

@tvdeyen
Copy link
Member Author

tvdeyen commented Jan 21, 2021

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

cc @mamhoff

@kennyadsl
Copy link
Member

@tvdeyen that would be great, thanks!

MinasMazar pushed a commit to solidusio-contrib/solidus_importer that referenced this issue Feb 5, 2021
This is using the new SolidusSupport helper method to determine when
using one or the other version.

Refs.
- [Solidus RFC](solidusio/solidus#3234)
- [Solidus 3.0 Extensions Hackathon](solidusio/solidus#3911)
MinasMazar pushed a commit to solidusio-contrib/solidus_importer that referenced this issue Feb 5, 2021
This is using the new SolidusSupport helper method to determine when
using one or the other version.

Refs.
- [Solidus RFC](solidusio/solidus#3234)
- [Solidus 3.0 Extensions Hackathon](solidusio/solidus#3911)
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

No branches or pull requests

6 participants