-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Restore firstname lastname on address #6159
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
base: main
Are you sure you want to change the base?
Changes from all commits
c856918
2ea3d5b
b2bd848
53e1733
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,28 +129,32 @@ | |
| # Invalid submission | ||
| within("form.ship_address") do | ||
| fill_in "Name", with: "" | ||
| fill_in "First Name", with: "" | ||
| fill_in "Street Address", with: "" | ||
| click_on "Update" | ||
| end | ||
| expect(page).to have_content("can't be blank").twice | ||
| expect(page).to have_content("can't be blank").thrice | ||
|
|
||
| # Valid submission | ||
| within("form.bill_address") do | ||
| fill_in "Name", with: "Galadriel" | ||
| fill_in "First Name", with: "Galadriel" | ||
| click_on "Update" | ||
| end | ||
| expect(page).to have_content("Billing Address has been successfully updated.") | ||
|
|
||
| # Valid submission | ||
| within("form.ship_address") do | ||
| fill_in "Name", with: "Elrond" | ||
| fill_in "First Name", with: "Elrond" | ||
| click_on "Update" | ||
| end | ||
| expect(page).to have_content("Shipping Address has been successfully updated.") | ||
|
|
||
| # Cancel submission | ||
| within("form.bill_address") do | ||
| fill_in "Name", with: "Smeagol" | ||
| fill_in "First Name", with: "Smeagol" | ||
| click_on "Cancel" | ||
| end | ||
| expect(page).to have_content("Users / [email protected] / Addresses") | ||
|
|
@@ -159,6 +163,8 @@ | |
| # The address forms weirdly only have values rather than actual text on the page. | ||
| expect(page).to have_field("user[bill_address_attributes][name]", with: "Galadriel") | ||
| expect(page).to have_field("user[ship_address_attributes][name]", with: "Elrond") | ||
| expect(page).to have_field("user[bill_address_attributes][firstname]", with: "Galadriel") | ||
| expect(page).to have_field("user[ship_address_attributes][firstname]", with: "Elrond") | ||
| end | ||
| end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,8 @@ module Spree::Api | |
| email: "[email protected]", | ||
| bill_address_attributes: { | ||
| name: 'First Last', | ||
| firstname: 'First', | ||
| lastname: 'Last', | ||
| address1: '1 Test Rd', | ||
| city: 'City', | ||
| country_id: country.id, | ||
|
|
@@ -60,6 +62,8 @@ module Spree::Api | |
| }, | ||
| ship_address_attributes: { | ||
| name: 'First Last', | ||
| firstname: 'First', | ||
| lastname: 'Last', | ||
| address1: '1 Test Rd', | ||
| city: 'City', | ||
| country_id: country.id, | ||
|
|
@@ -84,6 +88,8 @@ module Spree::Api | |
| email: "[email protected]", | ||
| bill_address_attributes: { | ||
| name: 'First Last', | ||
| firstname: 'First', | ||
| lastname: 'Last', | ||
| address1: '1 Test Rd', | ||
| city: 'City', | ||
| country_id: country.id, | ||
|
|
@@ -93,6 +99,8 @@ module Spree::Api | |
| }, | ||
| ship_address_attributes: { | ||
| name: 'First Last', | ||
| firstname: 'First', | ||
| lastname: 'Last', | ||
| address1: '1 Test Rd', | ||
| city: 'City', | ||
| country_id: country.id, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,16 @@ | |
| <%= f.text_field :name, class: 'fullwidth' %> | ||
| </div> | ||
|
|
||
| <div class="field <%= "#{type}-row" %>"> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| <%= f.label :firstname %> | ||
| <%= f.text_field :firstname, class: 'fullwidth' %> | ||
| </div> | ||
|
|
||
| <div class="field <%= "#{type}-row" %>"> | ||
| <%= f.label :lastname %> | ||
| <%= f.text_field :lastname, class: 'fullwidth' %> | ||
| </div> | ||
|
|
||
| <% if Spree::Config[:company] %> | ||
| <div class="field <%= "#{type}-row" %>"> | ||
| <%= f.label :company %> | ||
|
|
||
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.
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.