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

Remove team when sole owner and remove projects #6721

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

laushinka
Copy link
Contributor

Description

  1. Removes user projects
  2. Removes teams when user is sole owner of the teams
  3. Removes projects of those sole-owned teams

Related Issue(s)

Fixes #6655

How to test

The testing is quite extensive.
Test 1:

  1. Create a team
  2. Add a member to that team
  3. Add a project to that team
  4. Set one team as owner, one as member
  5. Have the owner delete their user account. Expectation: team and project should be deleted.

Test 2:

  1. Redo steps 1-4 above.
  2. Have the member delete their user account. Expectation: team and project should not be deleted.

Test 3:

  1. Redo steps 1-3 above.
  2. Set both members as owners.
  3. Have one owner delete their user account. Expectation: team and project should not be deleted.

Test 4:

  1. Add project to the user.
  2. Delete the user. Expectation: project should be deleted, i.e. that project is available to be added again.

Release Notes

When a user deletes their account, own projects will be deleted and made available to be added again.
When the user is the sole owner of a team, that team and its projects will be deleted and made available to be added again.

Documentation

@laushinka laushinka force-pushed the laushinka/account-delete-6655 branch from 0ba0bf9 to c4e44d6 Compare November 16, 2021 11:03
@laushinka laushinka marked this pull request as ready for review November 16, 2021 11:11
@jankeromnes
Copy link
Contributor

jankeromnes commented Nov 17, 2021

Super cool, many thanks! Taking a look now 👀

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing this clean-up! I think this is going to be really helpful. ✨

I've left a few thoughts and comments in-line. Please have a look, and decide whether to accept or decline my suggestions. 🙂

Also, I tried running through your test protocol, but faced a few problems:

Test 1:

  1. Create a team

I logged in with GitHub and created a new team, and copied the invite URL.

  1. Add a member to that team

I copied the invite URL, then opened a private browser tab, logged in with GitLab (created a new Gitpod account, which I needed to unblock), then visited the invite URL to join as a member.

  1. Add a project to that team

I (perhaps sneakily) decided to add the projects as the new GitLab member (and not the original GitHub team owner). Doing this, I ran into #6743 (probably unrelated to this PR, but maybe this is where things went wrong)

  1. Set one team as owner, one as member

This was already the case (the original GitHub team creator was already the owner, and the new GitLab invited member was already a member) so no action needed.

  1. Have the owner delete their user account. Expectation: team and project should be deleted.

When I deleted the original GitHub user account, I found in my private window (where the GitLab member was still logged in) that the team still existed, now without any owner, and that also the projects still existed:

Screenshot 2021-11-17 at 11 19 49 Screenshot 2021-11-17 at 11 20 01

Maybe I did something wrong? Or maybe it's the problem I noticed in the SQL query (see below)

Happy to discuss and/or clarify and/or help! 😇

components/gitpod-db/src/typeorm/team-db-impl.ts Outdated Show resolved Hide resolved
components/gitpod-db/src/typeorm/team-db-impl.ts Outdated Show resolved Hide resolved
components/server/src/user/user-deletion-service.ts Outdated Show resolved Hide resolved
components/server/src/user/user-deletion-service.ts Outdated Show resolved Hide resolved
@laushinka
Copy link
Contributor Author

laushinka commented Nov 17, 2021

@jankeromnes Thank you for all the insights and for taking the time! I will go through them one by one 🤗

@laushinka laushinka force-pushed the laushinka/account-delete-6655 branch from 7d0484a to a00cf38 Compare November 17, 2021 15:32
@roboquat roboquat added size/L and removed size/M labels Nov 17, 2021
@laushinka
Copy link
Contributor Author

/hold

@laushinka laushinka force-pushed the laushinka/account-delete-6655 branch from a00cf38 to 4f49b77 Compare November 18, 2021 11:33
@codecov
Copy link

codecov bot commented Nov 18, 2021

Codecov Report

Merging #6721 (4f49b77) into main (158acc4) will decrease coverage by 1.72%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##            main   #6721      +/-   ##
========================================
- Coverage   7.89%   6.16%   -1.73%     
========================================
  Files         14      12       -2     
  Lines       1254    1086     -168     
========================================
- Hits          99      67      -32     
+ Misses      1152    1018     -134     
+ Partials       3       1       -2     
Flag Coverage Δ
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
installer-raw-app 6.16% <ø> (ø)

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

Impacted Files Coverage Δ
components/local-app/pkg/auth/auth.go
components/local-app/pkg/auth/pkce.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 158acc4...4f49b77. Read the comment docs.

@laushinka
Copy link
Contributor Author

laushinka commented Nov 18, 2021

@jankeromnes I've applied the changes and have tested different cases manually as well, including the one you mentioned above. Ready for another review 🙏🏽

@laushinka
Copy link
Contributor Author

/unhold

@jankeromnes
Copy link
Contributor

jankeromnes commented Nov 18, 2021

Many thanks Laurie! 🙏

Re-flagging myself for review so that I don't forget about this(!)

@jankeromnes jankeromnes self-requested a review November 18, 2021 15:57
Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfection! 🎯 ✨ Works as advertised!

Many thanks for teaching Teams & Projects to clean up after them 🧹 🙏

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 23629167ff47fd2535253cd2a7b2e8589de0d7c4

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jankeromnes

Associated issue: #6655

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit 5d751e3 into main Nov 18, 2021
@roboquat roboquat deleted the laushinka/account-delete-6655 branch November 18, 2021 17:22
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/L team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup teams and projects when an account is deleted
3 participants