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

feat(dashboard): add customers domain #6093

Merged
merged 6 commits into from
Jan 16, 2024
Merged

Conversation

kasperkristensen
Copy link
Contributor

@kasperkristensen kasperkristensen commented Jan 15, 2024

What

  • Adds customers domain with the following pages: list overview, details, edit, create, update password.

Note
Some things are in-complete and will be followed up in future PRs:

  • Table loading state
    • Awaiting design, will implement for all Tables once ready.
  • Table filters
    • Will implement in separate PR and add to all table that uses queries that support it
  • Customer group section on the details page
    • There is currently no good way to fetch customer groups related to a customer without overfetching, eg. expanding groups and fetching potentially 100s of groups in a single query. Will add section, with the ability to bulk remove and add from groups once this can be none in a less expensive way.
  • Metadata
    • Will add this across admin in a separate PR

CLOSES CORE-1647

@kasperkristensen kasperkristensen requested a review from a team as a code owner January 15, 2024 16:59
Copy link

changeset-bot bot commented Jan 15, 2024

⚠️ No Changeset found

Latest commit: 8bb20e8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jan 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2024 11:39am
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2024 11:39am
docs-ui ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2024 11:39am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2024 11:39am

@olivermrbl
Copy link
Contributor

very nit: Any chance we can have the copy to clipboard icon in-line instead of under?
image

@olivermrbl
Copy link
Contributor

olivermrbl commented Jan 16, 2024

thought: Should we leave this out in the first iteration? I could imagine the flow of resetting customers' passwords would involve more steps e.g. sending out an email.

image

@olivermrbl
Copy link
Contributor

todo: I can't exit the "double cancel conf." flow:

CleanShot.2024-01-16.at.10.14.31.mp4

@olivermrbl
Copy link
Contributor

todo: I can't update customers with a registered account, because we pass the email:

CleanShot.2024-01-16.at.10.16.19.mp4

@olivermrbl
Copy link
Contributor

question: The navbar is off on mobile, but not sure if this will be addressed in a separate PR. I know we agreed not to put effort into a responsive design, but thought I'd call it out:
CleanShot 2024-01-16 at 10 17 48

@kasperkristensen
Copy link
Contributor Author

kasperkristensen commented Jan 16, 2024

question: The navbar is off on mobile, but not sure if this will be addressed in a separate PR. I know we agreed not to put effort into a responsive design, but thought I'd call it out: CleanShot 2024-01-16 at 10 17 48

This will be handled in a separate PR, Ludvig has done a responsive design for the topbar, just haven't gotten around to implementing it.

very nit: Any chance we can have the copy to clipboard icon in-line instead of under? image

Yes, the JSON view is very placeholder right now will fix it in a separate PR

thought: Should we leave this out in the first iteration? I could imagine the flow of resetting customers' passwords would involve more steps e.g. sending out an email.

image

I agree, which is why I also added the confirmation that stresses that you need to communicate this to the customer. I included it since I would argue this is a pretty essential CM feature so even though it is not that well fleshed out in Medusa it might still make sense to include it as it. Just to prevent store owners from having to do direct DB changes to help customers that struggle to reset their password.

@olivermrbl
Copy link
Contributor

olivermrbl commented Jan 16, 2024

I included it since I would argue this is a pretty essential CM feature so even though it is not that well fleshed out in Medusa it might still make sense to include it as it.

I agree it's an essential feature. But as long as we don't have a sophisticated RBAC, an email flow, or something similar, I would argue it could cause more harm than good, as anyone with access to admin can update other customers' passwords.

The fact that you need to do it in the DB right now is actually perhaps a good layer of security until something else is in place. And since this is not supported in our admin right now, we won't be robbing existing users' of a feature.

Update: fwiw, I checked with Shopify and Magento, and they both eliminated the feature in their recent versions in favor of an email flow

@kasperkristensen kasperkristensen merged commit 6315a61 into develop Jan 16, 2024
16 checks passed
@kasperkristensen kasperkristensen deleted the feat/3.0-customers branch January 16, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants