Solidus: RFC: Remove firstname and lastname from adresses

Created on 19 Jun 2019  路  9Comments  路  Source: solidusio/solidus

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.

Discussion

Most helpful comment

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

cc @mamhoff

All 9 comments

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.

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

I also am in favor of this.

Here Here

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?

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.

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.

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

cc @mamhoff

@tvdeyen that would be great, thanks!

Was this page helpful?
0 / 5 - 0 ratings

Related issues

graygilmore picture graygilmore  路  5Comments

vfonic picture vfonic  路  4Comments

jhawthorn picture jhawthorn  路  4Comments

ericsaupe picture ericsaupe  路  4Comments

brchristian picture brchristian  路  5Comments