Make delete or downgrade team owners scale #1029
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fisx
force-pushed
the
fisx/delete-or-downgrade-team-owners
branch
from
3943f40
to
3e2e815
Compare
fisx
force-pushed
the
akshaymankar/count-team-members
branch
from
4a3872f
to
01a114c
Compare
fisx
force-pushed
the
fisx/delete-or-downgrade-team-owners
branch
3 times, most recently
from
bf6271c
to
2826c3a
Compare
fisx
force-pushed
the
fisx/delete-or-downgrade-team-owners
branch
from
290e230
to
4eb788d
Compare
mheinzel
force-pushed
the
fisx/delete-or-downgrade-team-owners
branch
from
4eb788d
to
66f4200
Compare
"aeson half-roundtrip" was broken because it relied on ordering of fields, which is not guaranteed in JSON. There were also two redundant "aeson roundtrip" tests, so I removed one.
mheinzel
reviewed
Apr 8, 2020
services/brig/src/Brig/API/User.hs
Outdated
| @@ -745,20 +744,10 @@ deleteUser uid pwd = do | |||
| Nothing -> throwE DeleteUserInvalid | |||
| Just a -> case accountStatus a of | |||
| Deleted -> return Nothing | |||
| Suspended -> ensureNotOnlyOwner account >> go a | |||
| Active -> ensureNotOnlyOwner account >> go a | |||
| Suspended -> go a | |||
There was a problem hiding this comment.
we might still want to prevent owners from deleting themselves. Ending up with an owner without email address is not a big deal, but no owner at all seems problematic.
Comment on lines
121
to
122
| \ an owner. You must own the team to do that, and you must\ | ||
| \ have an email address associated with your user." |
There was a problem hiding this comment.
Suggested change
| \ an owner. You must own the team to do that, and you must\ | |
| \ have an email address associated with your user." | |
| \ an owner. You must be an owner of the team yourself to do that." |
There was a problem hiding this comment.
should be resolve by 8b4f360.
fisx
changed the title
Co-Authored-By: Matthias Heinzel <mheinzel@posteo.de>
|
what changed in error labels?
|
This reverts commit 4aa7dd2.
we don't prevent owners to remove themselves
we don't prevent owners from demoting themselves
|
I added more tests, some of which failed because we didn't guard against an owner deleting ( @fisx Please have a look at my changes tomorrow and merge if you're happy with them. (I also had some search-related tests fail, but they seem unrelated, so I hope it's just a local thing and they'll pass in CI) |
mheinzel
approved these changes
Apr 8, 2020
This reverts commit 62d8505.
fisx
commented
Apr 9, 2020
akshaymankar
requested changes
Apr 9, 2020
There was a problem hiding this comment.
I think we shouldn't tackle fanout and billing problems in this PR. Apart from from that minor nitpicks.
PS: We're going to have fun merging this and #1046
This is to be taken care of in these issues later: zinfra/backend-issues#1296 zinfra/backend-issues#1240
akshaymankar
approved these changes
Apr 9, 2020
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://github.com/zinfra/backend-issues/issues/1237