Restore firstname lastname on address #6159
Conversation
- 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.
github-actions
bot
added
changelog:solidus_api
|
I'd like this to be optional. Either dependent on the store, or as a global configuration - something like store.separate_first_and_last_nameor Spree::Config.separate_first_and_last_nameThe way it's currently implemented, the store I work with that works perfectly fine with a single name field will have their Furthermore, I think The configuration can also be used to display / hide the |
|
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. |
@jarednorman @mamhoff 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 |
|
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.
|
@jarednorman @kennyadsl is that something you can get on board with? |
|
If we have both 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. |
|
Ok, so here's probably the easiest way to do it then:
Is that ok for everybody? |
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
*) required vs.
*) required and
*) required
I would prefer to use Oh, and maybe we should call the field "given_name"? 🤔 |
|
@tvdeyen I don't want to argue about naming schemes, buuuuuuuuuuuttttt:
Microsoft: first_name
I know you would :) but can we try to find something for newbies that is friendly. |
|
@jarednorman @tvdeyen @kennyadsl we would appreciate if you three find a consensus to work one. |
|
I'm ok with For the Spree::Config UI, I'm not in favor of that for a couple of reasons:
|
|
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 :) |
I can see that, but I think the option should be created upon installer to select either or make First Name + Last Name default. |
@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:
This is not a config that we should let people play with, unless we want to spend time fixing database inconsistencies.
Makes sense, but please let's make this in a new PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 ☔ View full report in Codecov by Sentry. |
|
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? |
|
@tvdeyen @kennyadsl @jarednorman 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. |
| @@ -138,5 +145,10 @@ def country_iso=(iso) | |||
| def country_iso | |||
| country && country.iso | |||
| end | |||
|
|
|||
| def validate_name | |||
There was a problem hiding this comment.
Can you please explain the reasoning behind this validation logic?
There was a problem hiding this comment.
@kennyadsl we are going life with the MVP this week, but we can definetly handle this down the road a week from now.
There was a problem hiding this comment.
@JustShah please take a look at this.
There was a problem hiding this comment.
Previously, we had validation for name. Since we’ve updated these attributes, I’ve introduced a new validation to ensure that users provide either a name or both a first name and a last name.
| expect(address).to be_valid | ||
| end | ||
|
|
||
| it 'does not add an error when both firstname and name are present' do |
There was a problem hiding this comment.
This looks like a database inconsistency due to a mixed approach (old and new ways of setting the name together), which shouldn't happen. What about preventing this from happening?
There was a problem hiding this comment.
We will look into this.
There was a problem hiding this comment.
@JustShah please take a look at this. As far as I remember we had a good reason, please try to elaborate about the logic behind overwriting name with first_name and last_name when present.
There was a problem hiding this comment.
Previously, users could add only their name. But now, since we also allow first_name and last_name, we provide more flexibility by letting name and first_name have different values.
- As a user, I can provide only first_name and last_name, and name will be automatically populated by concatenating both.
- As a user, if I do not provide either first_name or name, it will trigger a validation error.
- As a user, I have the flexibility to set name and first_name as different values.
This test case confirm that users can set first_name and name independently without any issues.
There was a problem hiding this comment.
You introduced two attributes (firstname and lastname) but are testing and validating only firstname and name. Why don't we test lastname anywhere? I would like us either
- only re-introduce firstname (treating name as lastname if firstname is present)
- or if we introduce both (firstname and lastname) we need to make sure we do never ever mix both approaches in order to make sure to not confuse future contributors and users.
All your tests below only ever test firstname. Never lastname. So it looks like you want to go with the firstname + name as lastname approach, but still you introduced a lastname attribute. That is very confusing and can lead to invalid data
Can you elaborate on why you took that approach?
|
@tvdeyen could you also take a look here? |
|
Hey @kennyadsl @JustShah answered everything, can you give this a try? |
| expect(address).to be_valid | ||
| end | ||
|
|
||
| it 'does not add an error when both firstname and name are present' do |
There was a problem hiding this comment.
You introduced two attributes (firstname and lastname) but are testing and validating only firstname and name. Why don't we test lastname anywhere? I would like us either
- only re-introduce firstname (treating name as lastname if firstname is present)
- or if we introduce both (firstname and lastname) we need to make sure we do never ever mix both approaches in order to make sure to not confuse future contributors and users.
All your tests below only ever test firstname. Never lastname. So it looks like you want to go with the firstname + name as lastname approach, but still you introduced a lastname attribute. That is very confusing and can lead to invalid data
Can you elaborate on why you took that approach?
| @@ -8,6 +8,8 @@ module Spree::Api | |||
| let!(:harry_address_attributes) do | |||
| { | |||
| 'name' => 'Harry Potter', | |||
| 'firstname' => 'Harry', | |||
There was a problem hiding this comment.
according to your validation this address record is now invalid. It can either have a name attribute or firstname and lastname. But never both.
This is just an example for what I saw in lot of your tests. We should make sure we never mix both approaches. This is why I am still in favor of just re-introducing firstname and using name as lastname if firstname is present. But never touch the address (and ignore the firstname) if name is already present.
| @@ -6,6 +6,16 @@ | |||
| <%= f.text_field :name, class: 'fullwidth' %> | |||
| </div> | |||
|
|
|||
| <div class="field <%= "#{type}-row" %>"> | |||
There was a problem hiding this comment.
we cannot just introduce the two attributes as additional fields in the forms. How inconsistent is that? We need a toggle (or some very clever logic) to either show the two separate fields or the name field. But never both. Still I think just re-introducing the firstname as an option (via a configuration) is our best bet and helps migrating data. Please elaborate why you just introduced the two additional fields. Maybe I am missing something?
|
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. |
|
We want to bring this upstream. I think we were going for what you said once about migrations:
The current process is:
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:
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:
|
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: