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

[Admin] Add new users admin addresses page #5865

Conversation

MadelineCollier
Copy link
Contributor

@MadelineCollier MadelineCollier commented Oct 7, 2024

Summary

This PR is for issue #5824.

This migrates the users/:id/addresses page to the new admin. Instead of squashing everything into one controller action with an if request.put? wrapper (like the legacy backend controller did), I chose to break this new admin into two actions separated by their methods and responsibilities.

The old admin page did not have anything in the way of validations or error messages, it would simply fail to update the address and leave it as is if the user provided invalid values. (While also showing a message saying that the update was successful?)

This page improves upon that flow, adding validations to the address forms when their values are invalid and preventing the update, but since we are submitting an update against the user and not the address directly, we still need to do some extra work to run the validations and generate errors for the form (rather than silently failing to update like the legacy back-end controller used to do).

Screenshots:

Here is an example of the old admin's lack of validation:

Screen.Recording.2024-10-04.at.4.34.28.PM.mov

Here is the new admin with new validation:

Screen.Recording.2024-10-09.at.12.45.18.PM.mov

Here is a video to show off the new page more generally:

Screen.Recording.2024-10-09.at.12.50.14.PM.mov

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.

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 97.77778% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.26%. Comparing base (3a92a45) to head (53f4b28).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ponents/solidus_admin/users/addresses/component.rb 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5865      +/-   ##
==========================================
+ Coverage   89.24%   89.26%   +0.02%     
==========================================
  Files         754      755       +1     
  Lines       17533    17575      +42     
==========================================
+ Hits        15647    15689      +42     
  Misses       1886     1886              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thank you! I would like to be on a par with the current backend.

@tvdeyen tvdeyen added this to the 4.4 milestone Oct 9, 2024
This migrates the `users/:id/addresses` page to the new admin. Instead
of squashing everything into one controller action with an `if
request.put?` wrapper (like the legacy backend controller did), I chose
to break this new admin into two actions separated by their methods and
responsibilities.

The old admin page did not have anything in the way of validations or
error messages, it would simply fail to update the address and leave it
as is if the user provided invalid values. (While also showing a message
saying that the update was successful?)

This page improves upon that flow, adding validations to the address
forms when their values are invalid and preventing the update, but
since we are submitting an update against the user and not the address
directly, we still need to do some extra work to run the validations and
generate errors for the form (rather than silently failing to update
like the legacy back-end controller used to do).
@MadelineCollier
Copy link
Contributor Author

Thank you! I would like to be on a par with the current backend.

Hey @tvdeyen I have reworked the validations somewhat, have a look at the updated controller, component, and attached videos :)

@MadelineCollier MadelineCollier requested a review from a team October 9, 2024 10:55
@MadelineCollier
Copy link
Contributor Author

And regarding your earlier questions about the preference :address_requires_phone, it looks like the reason that the sandbox does not require a phone in specs (despite that being the default) is because of the inclusion of gem "solidus_paypal_commerce_platform", "~> 1.0".

[3] pry> @address.require_phone?
=> false

[4] pry> $ @address.require_phone?

From: ..../sandbox/vendor/bundle/ruby/3.1.0/gems/solidus_paypal_commerce_platform-1.0.3/app/decorators/models/solidus_paypal_commerce_platform/spree/address_decorator.rb:6:
Owner: SolidusPaypalCommercePlatform::Spree::AddressDecorator
Visibility: public
Signature: require_phone?()
Number of lines: 3

def require_phone?
  super && false
end

[5] pry> Spree::Config[:address_requires_phone]
=> true

@tvdeyen
Copy link
Member

tvdeyen commented Oct 9, 2024

Hey @tvdeyen I have reworked the validations somewhat, have a look at the updated controller, component, and attached videos :)

Thanks.

And regarding your earlier questions about the preference :address_requires_phone, it looks like the reason that the sandbox does not require a phone in specs (despite that being the default) is because of the inclusion of gem "solidus_paypal_commerce_platform", "~> 1.0".

[3] pry> @address.require_phone?
=> false

[4] pry> $ @address.require_phone?

From: ..../sandbox/vendor/bundle/ruby/3.1.0/gems/solidus_paypal_commerce_platform-1.0.3/app/decorators/models/solidus_paypal_commerce_platform/spree/address_decorator.rb:6:
Owner: SolidusPaypalCommercePlatform::Spree::AddressDecorator
Visibility: public
Signature: require_phone?()
Number of lines: 3

def require_phone?
  super && false
end

[5] pry> Spree::Config[:address_requires_phone]
=> true

That's defiantly interesting, but I was talking about the Spree::Config[:address_requires_state] or more precisely country.states_required setting and how the current admin address form (in the order edit) handles showing and hiding the state select respecting that setting.

Another thing I thought about is to show the whole users address book, not just the current ship and bill addresses. But maybe that's something we could tackle in a sub sequent PR.

@MadelineCollier MadelineCollier changed the title Add new users admin addresses page [Admin] Add new users admin addresses page Oct 10, 2024
@MadelineCollier
Copy link
Contributor Author

Spree::Config[:address_requires_state]

Apologies for my misunderstanding, I was looking at the line above the one you highlighted in the preferences config!

Currently this new user address admin page has the same functionality as every other location that is using the addresses component:

(In these examples, the Faroe Islands are set to states_required: false. And Spree::Config[:address_requires_state] is set to true.)

edit order billing address:

Screen.Recording.2024-10-10.at.1.39.21.PM.mov

users admin address:

Screen.Recording.2024-10-10.at.1.40.16.PM.mov

legacy backend admin address:

Screen.Recording.2024-10-10.at.1.37.33.PM.mov

As far as I can tell this is on par with the other new admin pages and while not exactly the same as the old admin (which just changed the field from a dropdown to a regular text input) it still seems roughly equivalent. Can you let me know exactly what is missing and where it should be added to this PR?

@kennyadsl
Copy link
Member

@MadelineCollier I think the difference is that when there are no states associated with a specific country, but the state is required, we should allow people to add the state manually, with the input text, which I'm not seeing in the new admin, looking at the screens you posted.

@MadelineCollier
Copy link
Contributor Author

@MadelineCollier I think the difference is that when there are no states associated with a specific country, but the state is required, we should allow people to add the state manually, with the input text, which I'm not seeing in the new admin, looking at the screens you posted.

Ah ok I see what you mean, I can add this for sure. If I add it to the address component it should correct the issue across the board.

@MadelineCollier
Copy link
Contributor Author

MadelineCollier commented Oct 11, 2024

I am going to merge this one as the changes to the address component made more sense conceptually as a separate commit and separate PR (since they affect all areas of the admin using the address component, and not just this page). That new PR is now open here: #5871 and in my tests it has fixed the issue :)

@MadelineCollier MadelineCollier merged commit 489e527 into solidusio:main Oct 11, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants