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] New admin user edit page #5856

Conversation

MadelineCollier
Copy link
Contributor

@MadelineCollier MadelineCollier commented Sep 18, 2024

Summary

This PR is for issue: #5824

This migrates the users/:id/edit page to the new admin. It still relies on the old backend admin controller for the #update action as well as the other top level user tabs such as address, order history, and so on.

In the next couple PRs I will migrate the remaining tabs to the new interface, but for now I thought it best to keep the old interface for actions such as creating a new user, which we definitely want to still have working.

This PR does not handle the issue of permission dependent interface elements. In the old admin, some buttons would not be visible if the user lacked the permissions to action them. However it looks like that is currently standard for the entirety of the new admin as I don't see any permissions checks or anything like this anywhere in the new admin:

      <% if can?(:addresses, @user) %>
        <li class="<%= 'active' if current == :address %>">
          <%= link_to t("spree.admin.user.addresses"), spree.addresses_admin_user_path(@user) %>
        </li>
      <% end %>

If we are using a new helper or gem to manage this and I am just missing something definitely let me know which implementation I can reference! Otherwise for now I will put this up as is because handling if can? permissions checks across the whole of the admin is a bit out of scope for this PR.

In the next PRs I will migrate the other tabs of the user admin (address, order history etc) and will add the modal for user creation (although I may leave the user creation backend logic as is for now as it looks like the issue specifically states that the user invitation flow should be considered a separate task.

Again there weren't really designs for this so I just went for it and tried to keep to our existing components as much as possible. Hopefully it looks decent!

Screenshot 2024-09-18 at 10 24 18 PM

The attached video shows the functionality visually:

Screen.Recording.2024-09-18.at.9.29.12.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.

@github-actions github-actions bot added changelog:solidus_core Changes to the solidus_core gem changelog:solidus_admin labels Sep 18, 2024
@MadelineCollier MadelineCollier force-pushed the admin-user-creation-modification branch 3 times, most recently from 0dda58d to 9276214 Compare September 18, 2024 20:22
@MadelineCollier MadelineCollier marked this pull request as ready for review September 18, 2024 20:23
@MadelineCollier MadelineCollier requested a review from a team as a code owner September 18, 2024 20:23
@MadelineCollier MadelineCollier changed the title Add new users admin edit page [Admin] New admin user edit page Sep 18, 2024
@MadelineCollier
Copy link
Contributor Author

Looks like the promotions spec failures are consistent across all branches and unrelated to this PR

[Promotions GET #show when non admin should be unauthorized](https://app.circleci.com/pipelines/github/solidusio/solidus/6641/workflows/1a2097d4-4450-4d5e-974c-ceb2d39f7258/jobs/61557/tests#failed-test-0)
[Promotions GET #show when admin when finding by a promotion behaves like a JSON response should return JSON](https://app.circleci.com/pipelines/github/solidusio/solidus/6641/workflows/1a2097d4-4450-4d5e-974c-ceb2d39f7258/jobs/61557/tests#failed-test-1)
[Promotions GET #show when admin when finding by a promotion behaves like a JSON response should be ok](https://app.circleci.com/pipelines/github/solidusio/solidus/6641/workflows/1a2097d4-4450-4d5e-974c-ceb2d39f7258/jobs/61557/tests#failed-test-2)

@github-actions github-actions bot added the changelog:solidus_api Changes to the solidus_api gem label Sep 23, 2024
@MadelineCollier MadelineCollier force-pushed the admin-user-creation-modification branch from f107c75 to 451003c Compare September 23, 2024 11:04
Copy link

codecov bot commented Sep 23, 2024

Codecov Report

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

Project coverage is 89.24%. Comparing base (8182cb2) to head (ac8ee59).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
...p/components/solidus_admin/users/edit/component.rb 92.85% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5856   +/-   ##
=======================================
  Coverage   89.23%   89.24%           
=======================================
  Files         752      754    +2     
  Lines       17510    17533   +23     
=======================================
+ Hits        15625    15647   +22     
- Misses       1885     1886    +1     
Flag Coverage Δ
89.24% <95.83%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@MadelineCollier MadelineCollier force-pushed the admin-user-creation-modification branch from 451003c to 777a06d Compare September 23, 2024 11:25
@github-actions github-actions bot removed the changelog:solidus_api Changes to the solidus_api gem label Sep 23, 2024
@MadelineCollier
Copy link
Contributor Author

I have fixed the (unrelated) failing API promotions specs in this PR: #5859

@MadelineCollier MadelineCollier force-pushed the admin-user-creation-modification branch 3 times, most recently from 25a87fc to bdfe150 Compare September 23, 2024 14:14
@MadelineCollier MadelineCollier force-pushed the admin-user-creation-modification branch from bdfe150 to f7ea6b8 Compare September 30, 2024 17:06
This migrates the `users/:id/edit` page to the new admin. It still
relies on the old backend admin controller for the `#update` action as
well as the other top level user tabs such as address, order history,
and so on.

Co-authored-by: benjamin wil <[email protected]>
@MadelineCollier MadelineCollier force-pushed the admin-user-creation-modification branch from f7ea6b8 to ac8ee59 Compare September 30, 2024 17:16
@MadelineCollier MadelineCollier requested a review from a team October 1, 2024 12:57
@MadelineCollier MadelineCollier merged commit 3a92a45 into solidusio:main Oct 4, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants