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

Restore firstname lastname on address #6159

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

fthobe
Copy link
Contributor

@fthobe fthobe commented Feb 24, 2025

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:

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.

- Unignored `firstname` and `lastname` field in the address model.
- Updated validations, attributes, and views to handle 'firstname' and 'lastname' separately.
- Modified tests, factories, and locales to align with the new structure.
- Introduced a 'set_full_name' method callback to concatenate 'firstname' and 'lastname' for backward compatibility.
Update the address handling logic in the APIs and tests to for 'firstname' and 'lastname' fields, instead of using a single 'name' field.
- Update the backend logic to separate 'name', 'firstname' and 'lastname' for better data handling.
- Adjusted user and address forms, search functionality, and related views to use the new fields.
Update the address handling logic in the admin components and tests for 'firstname' and 'lastname' fields,
instead of using a single 'name' field.
@fthobe fthobe requested a review from a team as a code owner February 24, 2025 16:28
@github-actions github-actions bot added changelog:solidus_api Changes to the solidus_api gem changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem changelog:solidus_sample Changes to the solidus_sample gem changelog:solidus_admin labels Feb 24, 2025
@mamhoff
Copy link
Contributor

mamhoff commented Feb 25, 2025

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

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

fthobe commented Feb 26, 2025

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

tvdeyen commented Feb 27, 2025 via email

@fthobe
Copy link
Contributor Author

fthobe commented Feb 27, 2025

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

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:

Screenshot 2025-02-27 at 15 50 38@2x

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

@fthobe
Copy link
Contributor Author

fthobe commented Feb 27, 2025

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

tvdeyen commented Feb 27, 2025

[...] 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
Copy link
Contributor Author

fthobe commented Feb 27, 2025

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_admin changelog:solidus_api Changes to the solidus_api gem changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem changelog:solidus_sample Changes to the solidus_sample gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants