-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Remove deprecated name-related Address fields #3820
Remove deprecated name-related Address fields #3820
Conversation
968ff73
to
051edf7
Compare
8668f44
to
1ba0224
Compare
1ba0224
to
df7004c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @filippoliverani!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@filippoliverani can you please rebase with the recent changes in address.rb
?
Otherwise, it looks good to me 👍
how about we keep the first & lastname and add |
df7004c
to
2c7fac4
Compare
@spaghetticode I have rebased this PR |
@peterberkenbosch @kennyadsl @spaghetticode
An important principle behind the whole refactoring is that a name in an address doesn't have to map to a person name, I'm not sure we should encourage users to think otherwise. Moreover, splitting a name in multiple components make sense only for some kind of names and for some locales, Basacamp's name_of_person works only for english names unfortunately. I think that, in general, developers should add their opinionated or locale-dependent way of saving persons' names to their User models. Address names instead can be more flexible and future-proof if kept as generic as possible. What do you think? |
As already suggested by someone else (maybe @elia), I also think we should have a Also, we are still on time to keep both code paths ( |
If we want to keep |
No wait, I didn't mean to revert what we did up to now 😅 , I meant (and I'm not even sure it's possible) to keep allowing both paths based on the |
I'm late to the party around the naming, I don't really understand or see the need to make this generic tbh. What I see right now personally is that it is causing more problems then it's solving. I do like the idea of moving the person name (firstname, lastname, mabye even middlename) to the user. That seems to be semanticly more correct. However this will cause some issues on what to do when a user wants to ship to someone else. What do we put on the shipping information? We do need a name there. Just a generic name field on address does not make sense to me tbh. Again, I'm late to the party, I might have missed the initial discussion around that (reading up on that right now). Either way, it's easy enough to manually override this or make a tiny extension that does what I need :) Just a bit confused about the need. Feel free to ignore the above, the PR itself looks good to me. |
And now I do.. 👍 any specific needs should be implemented by users indeed. |
Description
Ref #3816 and #3234
This PR removes deprecated public-accessible
Address
name fields and their aliases:full_name
firstname
first_name
lastname
last_name
.Address
name
attribute is still interally persisted on the DB as two separatefirstname
andlastname
columms, in a future PR we can move them to a newname
column and provide a rake task to help migrate existing data.Checklist:
[] I have added tests to cover this change (if needed)