-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Laushinka/allow users to delete 5066 #5966
Conversation
6ecdb57
to
b144324
Compare
/werft run 👍 started the job as gitpod-build-laushinka-allow-users-to-delete-5066.29 |
@gtsiolis @JanKoehnlein I just realized that members should be removed as well. I will push another commit and meanwhile hold the PR. |
/hold |
@laushinka let us know when this is ready for review! |
/unhold |
@gtsiolis @JanKoehnlein Ready now. Just finished testing the member removal. Also just fyi I'm unable to log into Slack. |
Looking at this now! 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this happening, @laushinka! 🧁
So much looking forward for being able to delete teams in staging and production. 🙌
Left some comments below! Let me know what you think.
components/dashboard/src/Menu.tsx
Outdated
}, | ||
{ | ||
title: 'Settings', | ||
link: `/t/${team.slug}/settings`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue(blocking): Although permissions are still not clear for team owners and members, it makes sense to allow only team owners to delete a team, right? Cross-posting WIP documentation from https://github.com/gitpod-io/website/pull/842 for visibility. 👀
suggestion: Since settings currently include only one action to delete a team, what do you think of hiding the settings tab for non-owners? Cc @svenefftinge because #5066 (comment) and relevant discussion (internal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gtsiolis You're right, this makes sense!
<PageWithSubMenu subMenu={settingsMenu} title='General' subtitle='Manage general team settings.'> | ||
<h3>Delete Team</h3> | ||
<p className="text-base text-gray-500 pb-4">Deleting this team will also remove all associated data with this team, including projects and workspaces. Deleted teams cannot be restored!</p> | ||
<button className="danger secondary" onClick={() => setModal(true)}>Delete Account</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: This returns an error in console and so feedback to the user when they are have member permissions, not owner permission, which also lacks visual feedback for the user. Hiding the project settings page for members (non-owners) could resolve this issue. WDYT? ❓
<PageWithSubMenu subMenu={settingsMenu} title='General' subtitle='Manage general team settings.'> | ||
<h3>Delete Team</h3> | ||
<p className="text-base text-gray-500 pb-4">Deleting this team will also remove all associated data with this team, including projects and workspaces. Deleted teams cannot be restored!</p> | ||
<button className="danger secondary" onClick={() => setModal(true)}>Delete Account</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: This removes the team entry in the team scope selector but
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still an issue. Minor and non-blocking. Added follow up #6016.
<PageWithSubMenu subMenu={settingsMenu} title='General' subtitle='Manage general team settings.'> | ||
<h3>Delete Team</h3> | ||
<p className="text-base text-gray-500 pb-4">Deleting this team will also remove all associated data with this team, including projects and workspaces. Deleted teams cannot be restored!</p> | ||
<button className="danger secondary" onClick={() => setModal(true)}>Delete Account</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: When deleting a team a user is redirected back to /account
. What do you think of redirecting back to the starting page of the user, /workspaces
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was torn re where to redirect and was looking forward to your input. It can be a bit disorienting to delete a team and then suddenly seeing another settings page, so I agree with redirecting back to /workspaces.
@gtsiolis It's ready for another review! |
/werft run 👍 started the job as gitpod-build-laushinka-allow-users-to-delete-5066.36 |
9e0c55b
to
c12fa27
Compare
db2a679
to
5737672
Compare
/werft run 👍 started the job as gitpod-build-laushinka-allow-users-to-delete-5066.42 |
/werft run 👍 started the job as gitpod-build-laushinka-allow-users-to-delete-5066.43 |
/lgtm |
LGTM label has been added. Git tree hash: 0da431ffac03389cf65df56ab5b1f9883826580b
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JanKoehnlein Associated issue: #5066 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks @laushinka @JanKoehnlein! 🏀 Just took another look at this and noticed the issue in #5966 (comment) is still present. Added a follow-up issue in #6016 since this has been merged as this seems to be minor and an edge case, see #5966 (comment). |
Description
Related Issue(s)
Fixes #5066
How to test
Release Notes