feat(admin): Add button to remove UserEmail #94072
Conversation
leedongwei
commented
Jun 23, 2025
leedongwei
commented
github-actions
bot
added
the
Scope: Frontend
| const primary = user.email; | ||
| const emails = user.emails ? user.emails : [{email: primary}]; | ||
|
|
||
| const handleRemoveEmail = async (email: string) => { |
There was a problem hiding this comment.
Can you use useMutation() instead? It uses react query and is the ✨ new way ✨
https://develop.sentry.dev/frontend/network-requests/#mutations-postputdelete-requests
| {data.email !== primary && ( | ||
| <Confirm | ||
| message={`Are you sure you want to remove the email ${data.email}?`} | ||
| onConfirm={() => handleRemoveEmail(data.email)} |
There was a problem hiding this comment.
If you use the useMutation hook, you can use mutateAsync here to keep the async functionality
| name: 'Addresses', | ||
| content: ({Panel}) => <UserEmails Panel={Panel} user={user} />, | ||
| content: ({Panel}) => ( | ||
| <UserEmails Panel={Panel} user={user} onUserUpdate={refetchUser} /> |
There was a problem hiding this comment.
You can also directly modify the cache after this succeeds if you want the list to immediately update instead of having a delay: https://develop.sentry.dev/frontend/network-requests/#updating-your-query-data
ameliahsu
left a comment
ameliahsu
left a comment
There was a problem hiding this comment.
mainly nits about testing
|
|
||
| expect(screen.getByText('primary@example.com')).toBeInTheDocument(); | ||
| expect(screen.getByText('secondary@example.com')).toBeInTheDocument(); | ||
| expect(screen.getByText('— primary')).toBeInTheDocument(); |
There was a problem hiding this comment.
you can use textWithMarkupMatcher to match text within multiple nodes, to make sure that the correct email is being labeled as primary
| render(<UserEmails Panel={mockPanel} user={mockUser} />); | ||
|
|
||
| // Should show remove button for secondary email | ||
| expect(screen.getByTestId('remove-email-button')).toBeInTheDocument(); |
There was a problem hiding this comment.
should use screen.getByRole() instead of using test id
| > | ||
| <Button | ||
| data-test-id="remove-email-button" | ||
| priority="danger" |
There was a problem hiding this comment.
not sure if there's a design pattern for this but i would just make the priority="default" for the buttons in the table, this is a random example i found from settings/account/identities/
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
