Restore firstname lastname on address

[fthobe]

Summary

Description: This PR introduces updates to the address model, admin components, backend, API, and associated logic to handle firstname and lastname as separate fields, replacing the previous name field providing backwards compatibility to existing implementations of name. The updates aim to improve data handling and increase clarity when working with user and address-related data.

For the purpose of not breaking name and provide a smooth transistion if first and last name are used name is compiled with the concatenated value allowing the fields to coexist.

Changes: Admin: Updated admin components to use firstname and lastname fields separately in place of the name field. Backend: Modified backend logic to separate name into firstname and lastname, adjusting user and address forms, search functionality, and related views. API: Updated the API and its associated tests to manage firstname and lastname fields separately, replacing the name field. Core: Unignored the firstname and lastname fields in the address model. Introduced a set_full_name method for backward compatibility, concatenating firstname and lastname. Updated tests, factories, and locales to align with the new model structure.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

• [X] I agree that my PR will be published under the same license as Solidus.
• [X] I have written a thorough PR description.
• [X] I have kept my commits small and atomic.
• [X] I have used clear, explanatory commit messages.

The following are not always needed:

• 📖 I have updated the README to account for my changes.
• 📑 I have documented new code with YARD.
• 🛣️ I have opened a PR to update the guides.
• ✅ I have added automated tests to cover my changes.
• 📸 I have attached screenshots to demo visual changes.

Attribution: Author @JustShah Sponsor @gms-electronics

[mamhoff]

I'd like this to be optional. Either dependent on the store, or as a global configuration - something like

store.separate_first_and_last_name

or

Spree::Config.separate_first_and_last_name

The way it's currently implemented, the store I work with that works perfectly fine with a single name field will have their name field overwritten with empty strings, because there is no condition that stops the address model from overwriting name.

Furthermore, I think firstname can be optional. Shopify has implemented it like that, too.

The configuration can also be used to display / hide the name or firstname/lastname fields. Right now we have a situation where we can have conflicting fields, which I'm not a fan of.

[jarednorman]

Yeah, I'm with @mamhoff. There ware services out there that handle name as a unified field, which is preferable due to... reality. That said, a huge number of services that expect separate fields, and other reasons why separate fields are preferable for many orgs. Option to choose needs to remain, even if it's kind of annoying from a maintenance perspective. The alternative creates too much work for existing stores working with unified names.

[fthobe]

Yeah, I'm with @mamhoff. There ware services out there that handle name as a unified field, which is preferable due to... reality.

@jarednorman I know it's from a technical point of view challenging. I hope I outlined the reasoning in the tickets for anybodies satisfaction on the reasoning why this should be done. The current default configuration makes it very hard to reach compliance in multiple large European markets.

@mamhoff Actually as you can see also by the tests the name is only overwritten if fname and lname are not empty. We will roll another check on that.

Right now if not used first name and last name have no impact on name. This is a very aggressive stance, I'd love to call it a strongly opinionated default incentevize migrate back to these fields also because the pain of splitting those fields is much bigger than the pain of joining them. If that approach has no chance for majority we can put it behind a configuration flag, I wouldn't necessarily motivate people to fragment the DB structure that way but I can understand your point.

@tvdeyen @jarednorman @kennyadsl We will align this thing with the modus operandi you prefer, could you three decide what to do? If you could also check if anything is wrong with the code itself we might be able to upstream this in the fewest possible feedback cycles. Mayur did his best to align with quality requirements regarding the PR and commit.

[tvdeyen]

I am imagining a firstname attribute and use the current name attribute as either the full name or last name.

The first-name should not be mandatory by default and it should be probably hidden in forms.

That way we do not have three hard to align fields and reflect what already is the case in ie. Germany where most address forms only have the (last) name attribute mandatory.

All of this should be a - by default disabled - feature toggle (ie. enable_address_firstname). Maybe we even have a second toggle for showing / hiding the first-name field and validation separately.

[fthobe]

I am imagining a firstname attribute and use the current name attribute as either the full name or last name. ok, that's not stupid as we can reuse most of the stuff with least effort for everybody.

@jarednorman @kennyadsl is that something you can get on board with?

[kennyadsl]

If we have both firstname and name, I think we only need one preference: if the store wants the validation on the firstname. If they don't want to use it, they can still remove the field from the forms where it's displayed, or just leave it empty.

On a side note, it's the same option that Shopify offers:

I'm ok with this path, as soon as the solution is backward compatible.

[fthobe]

Ok, so here's probably the easiest way to do it then:

• we do not reintroduce last name
• we use name as last name
• we reintroduce first name
• we toggle a switch in admin to define the preference

Is that ok for everybody? @JustShah @tvdeyen @jarednorman

[tvdeyen]

[...] they can still remove the field from the forms where it's displayed, or just leave it empty.

This is true for the starter front end maybe, but not for admin forms. I suggest just displaying/hiding the field on the preference then.

If we keep the field and only require one of them, we will end with inconsistent data.

Examples

firstname	name*
	John Doe
Jane	Doe

*) required

vs.

firstname*	name*
John	Doe
Jane	Doe

*) required

and

name*
John Doe
Jane Doe

*) required

Ok, so here's probably the easiest way to do it then:

• we do not reintroduce last name
• we use name as last name
• we reintroduce first name
• we toggle a switch in admin to define the preference

I would prefer to use Spree::Config instead of a switch in the admin.

Oh, and maybe we should call the field "given_name"? 🤔

https://www.dictionary.com/browse/first%20name

[fthobe]

@tvdeyen I don't want to argue about naming schemes, buuuuuuuuuuuttttt:

Oh, and maybe we should call the field "given_name"? 🤔

Microsoft: first_name Google: first_name Shopify: first_name BigCommerce: first_name

I would prefer to use Spree::Config instead of a switch in the admin.

I know you would :) but can we try to find something for newbies that is friendly.

[fthobe]

@jarednorman @tvdeyen @kennyadsl we would appreciate if you three find a consensus to work one.

[kennyadsl]

I'm ok with first_name, and it should be automatically backward-compatible with the old implementation (for those who continued to use the split name), I guess.

For the Spree::Config UI, I'm not in favor of that for a couple of reasons:

• we have a lot of similar configs, why is this the only one with an admin UI?
• not having operators messing with some important configuration is good.
• setting a config is already the "newbies" interface in the Solidus world. I mean, if someone is in trouble changing a string in the initialier of their Rails application, Solidus is not the right place, and we are providing a service letting them know sooner rather than later.

[mamhoff]

I'd be very happy with just reintroducing an optional first name field.

I do think this should be configuration option to be set by store operators in code. Switching this option will lead to inconsistencies in address storing and display, and probably needs a very well-thought data migration. Letting admins do that is a recipe for a very annoying disaster :)

[fthobe]

For the Spree::Config UI, I'm not in favor of that for a couple of reasons:

• we have a lot of similar configs, why is this the only one with an admin UI?
• not having operators messing with some important configuration is good.
• setting a config is already the "newbies" interface in the Solidus world. I mean, if someone is in trouble changing a string in the initialier of their Rails application, Solidus is not the right place, and we are providing a service letting them know sooner rather than later.

I can see that, but I think the option should be created upon installer to select either or make First Name + Last Name default.

[fthobe]

I do think this should be configuration option to be set by store operators in code. Switching this option will lead to inconsistencies in address storing and display, and probably needs a very well-thought data migration. Letting admins do that is a recipe for a very annoying disaster :)

@mamhoff where do you see the particular issues with this migration? Could you elaborate what you have in mind?

[kennyadsl]

@fthobe

@mamhoff where do you see the particular issues with this migration? Could you elaborate what you have in mind?

Martin is basically reinforcing what I said here:

not having operators messing with some important configuration is good.

This is not a config that we should let people play with, unless we want to spend time fixing database inconsistencies.

---

I think the option should be created upon installer

Makes sense, but please let's make this in a new PR.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.67%. Comparing base (7205ff0) to head (53e1733). Report is 60 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6159      +/-   ##
==========================================
+ Coverage   86.56%   88.67%   +2.10%     
==========================================
  Files         512      833     +321     
  Lines       11838    18077    +6239     
==========================================
+ Hits        10248    16029    +5781     
- Misses       1590     2048     +458     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[fthobe]

On my eternal journey to get crucified here :D

So the current setup in this PR does not force the use of First_name last_name as name is only overwritten if first_name and last_name are used. That was intentionally build that way by us to allow maximum flexibility and least possible migration pain.

I have limited capacity to change that a migration from name to the good old ways makes anybody struggle as the problem of splitting is what it is. The advantage of this solution is, it ain't break anything. I am not a big fan of making name the last_name as I believe it's jumping around a circle and creates semantic issues around naming in this system.

Is anybody willing to take this one as it is behind a setting or do you three insist on making name last_name?

[fthobe]

@tvdeyen @kennyadsl @jarednorman Could one of you please write down clear direction where this heading? I know this is again being perceived as one of my crusades, but there's no good solution other than migrating back to this. It was a technically wise, practically not entirely well made decision with good spirits in mind. Let's correct it, all biting the sour apple for having first_name and last_name. I am sure we will all survive with a deprecation plan.

I know, I think we all know about falsehoods programmers believe about names, nobody likes to do this, but for the sake of living in a practical world and trying to do this without drama and 70 pages, can we please get out of this with you three making a practical decision. first_name & name is semantically confusing, this PR contains no obligation for anybody to use this fields and I am sure @chaimann can find a wonderful way to hide them in backend.

Please provide a way to make this happen, as in all honesty, the initial change shouldn't have happened and I strongly believe we all know that. As @elia also said on the issue, there's obvious demand for this otherwise the extension wouldn't exist.

We could wrap this up with #6169 and #6168 and we are like 80% there on the compliance issues. Imprint with legal notes and addresses are still missing, but we are almost there to be EU fit.

[fthobe]

@tvdeyen could you also take a look here?

[fthobe]

Hey @kennyadsl @JustShah answered everything, can you give this a try?

[tvdeyen]

Just reading through the old comments and just realized that none of the requests have been addressed yet. Why are we requested to re-review the PR if it still is in this state?

We have a clear path above and it has not been addressed. Please read the comments and adjust accordingly. Not rushing, but please only re-request a review if it's done. We have limited resources and this is just a waste of time.

If you don't want to continue working in this or think the approach we want to go with is wrong then please close this PR and let's continue elaborating in a discussion. That's fine as well.

[fthobe]

We want to bring this upstream. Thomas, it's not a complex task, but it can become breaking quickly (also by accident). Having you all three looking over that for one feedback cycle adds a lot of value and allows us to align better with your ideas.

I think we were going for what you said once about migrations:

• introduce new columns
• for a couple of versions Write value in both
• deprecate old columns
• delete old columns

The current process is:

• if first_name and last_name are written, they are concatenated and written into name (which allows a deprecation to take place without issues) as we will have to adjust multiple placed (Paypal, shipstation,...)
• We can surely adjust tests here to cover also last_name

A configuration was not made yet to verify the approach first, I'd go with backend as the way it's currently made had no potential to break anything (as name is always written), but i can see how alternative approaches might be technically more appealing, but also more aggressive.

We have three options here:

1. configuration in backend (rejected by core)
2. configuration in file and option in installer

We are going to go with a config at that point.

We didn't want to waste your time, it's a feedback cycle to figure out what you all want. I hope I answered all questions and we will add:

• Configuration to enable / disable
• tests for last_name