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.

Sign up for GitHub

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

Jump to bottom

Use Address name attribute in views and APIs #3524

Merged
merged 6 commits into from
Mar 6, 2020
Merged

Use Address name attribute in views and APIs #3524

merged 6 commits into from
Mar 6, 2020

Conversation

filippoliverani
Copy link
Contributor

@filippoliverani filippoliverani commented Feb 25, 2020 •
edited
Loading

Description
This PR is a step to solve #3234 and a continuation of #3458

The PR allows to start using the new Spree::Address#name attribute instead of firstname and lastname by adding a show_address_deprecated_attributes preference and providing alternate HTML and JSON views.
The old attributes are not deprecated yet, but this PR discourages their usage and enables name usage everywhere.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

@filippoliverani filippoliverani marked this pull request as ready for review February 25, 2020 14:19
ericsaupe
ericsaupe approved these changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@ericsaupe ericsaupe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

kennyadsl
kennyadsl reviewed Feb 28, 2020
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff again, but I left some other notes. Let me know what do you think!

frontend/spec/features/checkout_spec.rb Outdated Show resolved Hide resolved
frontend/spec/controllers/spree/checkout_controller_spec.rb Show resolved Hide resolved
backend/spec/controllers/spree/admin/users_controller_spec.rb Outdated Show resolved Hide resolved
backend/app/views/spree/admin/orders/index.html.erb Outdated Show resolved Hide resolved
backend/spec/features/admin/orders/customer_details_spec.rb Outdated Show resolved Hide resolved
api/app/helpers/spree/api/api_helpers.rb Outdated Show resolved Hide resolved
backend/app/controllers/spree/admin/search_controller.rb Outdated Show resolved Hide resolved
@filippoliverani filippoliverani force-pushed the switch-to-address-name branch from e446ac9 to 4fd8b3b Compare March 2, 2020 09:45
@filippoliverani filippoliverani force-pushed the switch-to-address-name branch from 4fd8b3b to 6bb63de Compare March 4, 2020 09:39
kennyadsl
kennyadsl approved these changes Mar 4, 2020
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @filippoliverani. This thing is becoming a reality!

@filippoliverani
Copy link
Contributor Author

Thanks, @filippoliverani. This thing is becoming a reality!

Thank you for your help @kennyadsl! 💪

@kennyadsl
Copy link
Member

@filippoliverani can you check the conflict here. I can't recall merging something in that file lately.. 🤔

@filippoliverani
Add preference to drive new Address name adoption
2130097
@filippoliverani
Add validation errors to correct Address name field
adaaa4c
@filippoliverani
Enable Address name usage in api
3244eec 
Favor name attribute usage over firstname and lastname.
To avoid internally relying on deprecated code for backwards
compatibility, deprecated fields are rendered in API responses only
when `show_deprecated_attributes` preference is set to true.
@filippoliverani
Enable Address name usage in frontend
0f1f15b 
Favor name attribute usage over firstname and lastname attributes.
To avoid internally relying on deprecated code for backwards
compatibility, deprecated fields are rendered in HTML views instead of
name field only when `show_deprecated_attributes` preference is set to
true.
@filippoliverani
Enable Address name usage in backend
bd32a88 
Favor name attribute usage in backend over firstname and lastname.
To avoid internally relying on deprecated code for backwards
compatibility, deprecated fields are rendered in HTML views and API
responses instead of name field only when `show_deprecated_attributes`
preference is set to true.
@filippoliverani
Add Address name attribute to developer guide
8b43d28
@filippoliverani filippoliverani force-pushed the switch-to-address-name branch from 6bb63de to 8b43d28 Compare March 6, 2020 08:52
@filippoliverani
Copy link
Contributor Author

@filippoliverani can you check the conflict here. I can't recall merging something in that file lately.. 🤔
Generators has been renamed recently, rebased 👍

@kennyadsl kennyadsl merged commit 7c202de into solidusio:master Mar 6, 2020
spaghetticode added a commit to solidusio/solidus_stripe that referenced this pull request Mar 10, 2020
@spaghetticode
Fill name fields according to Solidus preference
a6d87e4 
Since solidusio/solidus#3524 was merged,
we need to verify if we're using the single "Name" field or the
previous first/last name combination.
spaghetticode added a commit to spaghetticode/solidus_stripe that referenced this pull request Mar 10, 2020
@spaghetticode
Fill name fields according to Solidus preference
7c161ce 
Since solidusio/solidus#3524 was merged,
we need to verify if we're using the single "Name" field or the
previous first/last name combination.
spaghetticode added a commit to spaghetticode/solidus_stripe that referenced this pull request Mar 10, 2020
@spaghetticode
Fill name fields according to Solidus preference
cf68be3 
Since solidusio/solidus#3524 was merged,
we need to verify if we're using the single "Name" field or the
previous first/last name combination.
@kennyadsl kennyadsl mentioned this pull request Mar 16, 2020
Merged
@filippoliverani filippoliverani mentioned this pull request Apr 10, 2020
Merged
4 tasks
stem added a commit to stem/solidus_product_assembly that referenced this pull request Apr 18, 2020
@stem
Fix failing spec since Solidus address format change
4a538b8 
Because of addresses' firstname & lastname deprecations in solidusio/solidus#3584,
solidusio/solidus#3524 and probably other PRs, the inputs where deleted
in favor of a single `name` input.
This is only on master for now, so it should be in 2.11 or 3.0.
@stem stem mentioned this pull request Apr 18, 2020
Merged
stem added a commit to stem/solidus_product_assembly that referenced this pull request Apr 18, 2020
@stem
Fix failing spec since Solidus address format change
cff67e8 
Because of addresses' firstname & lastname deprecations in solidusio/solidus#3584,
solidusio/solidus#3524 and probably other PRs, the inputs where deleted
in favor of a single `name` input.
This is only on master for now, so it should be in 2.11 or 3.0.
stem added a commit to stem/solidus_product_assembly that referenced this pull request Apr 18, 2020
@stem
Fix failing spec since Solidus address format change
6de20f4 
Because of addresses' firstname & lastname deprecations in solidusio/solidus#3584,
solidusio/solidus#3524 and probably other PRs, the inputs where deleted
in favor of a single `name` input.
This is only on master for now, so it should be in 2.11 or 3.0.
aldesantis pushed a commit to stem/solidus_product_assembly that referenced this pull request Apr 25, 2020
@stem @aldesantis
Fix failing spec since Solidus address format change
b507969 
Because of addresses' firstname & lastname deprecations in solidusio/solidus#3584,
solidusio/solidus#3524 and probably other PRs, the inputs where deleted
in favor of a single `name` input.
This is only on master for now, so it should be in 2.11 or 3.0.
filippoliverani added a commit to filippoliverani/solidus_tax_cloud that referenced this pull request May 15, 2020
@filippoliverani
Fill address names combined when needed
9a7d770 
In solidusio/solidus#3524 we introduced a new preference that allows
to use the combined version of firstname/lastname.
This commit updates checkout feature specs to reflect this preference.
Once the split version will be deprecated and removed, we can
probably also revert this commit and use the combined version only.
@filippoliverani filippoliverani mentioned this pull request May 15, 2020
Merged
aldesantis pushed a commit to filippoliverani/solidus_tax_cloud that referenced this pull request May 16, 2020
@filippoliverani @aldesantis
Fill address names combined when needed
b379751 
In solidusio/solidus#3524 we introduced a new preference that allows
to use the combined version of firstname/lastname.
This commit updates checkout feature specs to reflect this preference.
Once the split version will be deprecated and removed, we can
probably also revert this commit and use the combined version only.
filippoliverani added a commit to solidusio-contrib/solidus_handling_fees that referenced this pull request May 22, 2020
@filippoliverani
Fill-in combined address names in specs when needed
cb04657 
In solidusio/solidus#3524 we introduced a new preference that allows
to use the combined version of firstname/lastname.
This commit updates checkout feature specs to reflect this preference.
Once the split version will be deprecated and removed, we can
probably also revert this commit and use the combined version only.
@filippoliverani filippoliverani mentioned this pull request May 22, 2020
Merged
tvdeyen
tvdeyen reviewed Dec 16, 2021
View reviewed changes
core/lib/spree/testing_support/dummy_app.rb
@@ -103,6 +103,7 @@ class Application < ::Rails::Application
Spree.config do |config|
config.mails_from = "[email protected]"
config.raise_with_invalid_currency = false
config.use_combined_first_and_last_name_in_address = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been false, because we still tested v2.11 here, that has this set to false (see above)
That way specs are misleadingly green and issues like #4094 didn't came up in the specs for v2.11

I am proposing to revert this in v2.11 branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@tvdeyen tvdeyen tvdeyen left review comments

@ericsaupe ericsaupe ericsaupe approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@filippoliverani @kennyadsl @tvdeyen @ericsaupe @houndci-bot