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][Users] Introduce user creation and modification capability #5824

Open
MadelineCollier opened this issue Aug 13, 2024 · 7 comments
Open
Assignees

Comments

@MadelineCollier
Copy link
Contributor

MadelineCollier commented Aug 13, 2024

Update

This issue has grown in scope to encompass the entirety of the user admin's sub-menus since those didn't have their own standalone tickets.
Image

Description

Develop the functionality to add and edit users through the admin panel, providing fields for key user details and the ability to assign roles.

Note: The user invitation feature will be implemented in a separated task

Features

  • Form for entering user details (name, email, role).
  • Validation and user creation logic

Figma Design

Storyboard


Design details

Image

@MadelineCollier MadelineCollier self-assigned this Aug 13, 2024
@MadelineCollier MadelineCollier converted this from a draft issue Aug 13, 2024
@MadelineCollier MadelineCollier moved this from Todo to In Progress in Solidus Admin Aug 22, 2024
@MadelineCollier
Copy link
Contributor Author

MadelineCollier commented Aug 22, 2024

Image

@kennyadsl @adammathys do we want to add a name column to the spree_users table via a migration in core to accommodate this design? Or should we just leave it at email for now? I know it's a pretty common customization for stores to just add firstname/lastname/name to Spree::User themselves.

Same deal with the profile picture? Add it as an attachment on the model in core? I just want to make sure I'm not making too many assumptions based on the design, but if we want to 100% match the designs regardless of what we currently have for columns in core I can keep that in mind for future issues and just add whatever we need to adhere to them.

@kennyadsl
Copy link
Member

kennyadsl commented Aug 22, 2024

Good questions, someone has to make a decision here.

It's pretty common to have this info on the user table indeed. So I guess we should do it.
On the other side, it's not strictly related to this feature. I mean, I imagine the name will be initially populated with a migration that copy the name from the last order's shipping address, and we could also add this later in isolation.

For the attachment, my preference is to start without it, it has been never requested to us if I recall correctly. Some other options might be using gravatar for now? I can definitely live without it and I'm sure there's other stuff with more priority.

That said, I'm ok with having @MadelineCollier having a final word here. One way or the other it will work and it will be an improvement.

@MadelineCollier
Copy link
Contributor Author

MadelineCollier commented Sep 13, 2024

Progress update from this week, still some kinks to iron out before it can go to PR. There weren't any designs aside from the "invite new user" flow so I just freestyled it with our existing page layouts and components. Hope it looks okay!

Screen.Recording.2024-09-13.at.6.14.02.PM.mov

I am noticing that there are a lot of spots in the old admin that look like this:

      <% 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 %>

I don't see any similar occurrences in the new admin, and I am wondering if we have an existing plan for how we want to handle CanCan stuff in our new components. For now I am just # @todo do this later -ing. Any thoughts or suggestions?

@kennyadsl
Copy link
Member

@MadelineCollier nice! We should definitely address the missing CanCan conditional code. @rainerdema @elia do you have any context why this is missing in the new admin? Were we planning to add it later?

@MadelineCollier
Copy link
Contributor Author

Hey @rainerdema (separate issue from the above) I have been wondering about re-using the component('ui/forms/address') in the new admin users/addresses page.

The old admin version looks like this:

Screenshot 2024-10-01 at 4 20 58 PM

The only roadblock for reusing the component('ui/forms/address') seems to be that the component is maybe a bit old/possibly only functional within the context of the orders admin? I am not totally sure what's up with it but it doesn't pass a form object to the text_field.
Screenshot 2024-10-01 at 4 21 39 PM

Is there a reason it's not passing the form here? Or is this code just a bit older and missing out on the new options that text_field has now?

Every other occurrence that I could find of component("ui/forms/field").text_field seems pass f as the form object.

Screenshot 2024-10-01 at 4 22 26 PM

Hoping to get some context from you before I make any changes that might break something in the orders admin :)

@elia
Copy link
Member

elia commented Oct 3, 2024

@MadelineCollier nice! We should definitely address the missing CanCan conditional code. @rainerdema @elia do you have any context why this is missing in the new admin? Were we planning to add it later?

@kennyadsl @MadelineCollier I think we were planning to add them later in some way, there were a few changes that we wanted to make to make permissions more understandable and manageable from the UI. I guess that's an unsolved problem, and we should find the best trade off between the components being self contained and also know about permissions.

I think probably the easiest separation would be to avoid any permissions checks within /ui components, and only allow the checks when building a whole page.

@elia
Copy link
Member

elia commented Oct 3, 2024

The only roadblock for reusing the component('ui/forms/address') seems to be that the component is maybe a bit old/possibly only functional within the context of the orders admin? I am not totally sure what's up with it but it doesn't pass a form object to the text_field.

@MadelineCollier I think you're right that's an older version, but should be easy enough to pass the form object name prefix to it and make it work outside of the order. The order surely needs to control that prefix to separate billing and shipping addresses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

No branches or pull requests

3 participants