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

Add user freeze feature #6298

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Add user freeze feature #6298

merged 1 commit into from
Feb 22, 2024

Conversation

touilleMan
Copy link
Member

@touilleMan touilleMan commented Jan 31, 2024

@touilleMan touilleMan requested a review from a team as a code owner January 31, 2024 21:48
@touilleMan touilleMan requested a review from vxgmichel January 31, 2024 21:48
@touilleMan touilleMan added this to the v3.0 milestone Jan 31, 2024
Copy link
Contributor

@vxgmichel vxgmichel left a comment

Choose a reason for hiding this comment

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

A couple of comments, LGTM otherwise 👍

server/parsec/asgi/administration.py Outdated Show resolved Hide resolved
server/parsec/asgi/administration.py Show resolved Hide resolved
server/parsec/events.py Outdated Show resolved Hide resolved
server/parsec/components/memory/user.py Show resolved Hide resolved
server/parsec/events.py Show resolved Hide resolved
server/parsec/components/auth.py Show resolved Hide resolved
@mmmarcos mmmarcos linked an issue Feb 2, 2024 that may be closed by this pull request
3 tasks
Copy link
Contributor

@mmmarcos mmmarcos left a comment

Choose a reason for hiding this comment

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

LGTM overall. Some minor comments.

Comment on lines +380 to +375
outcome = await backend.user.list_users(organization_id)
match outcome:
case list() as users:
pass
case UserListUsersBadOutcome.ORGANIZATION_NOT_FOUND:
raise HTTPException(status_code=404, detail="Organization not found")
case unknown:
assert_never(unknown)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a rationale for the "outcome" wording? (I noticed it was introduced in #6214)

I don't have strong arguments against it, it's just that it doesn't feel "standard" (which is usually better for readability). Usually this things are named result or response. Also suffix BadOutcome for enums which has the downside of using 2 words instead something like just Error.

Copy link
Member Author

Choose a reason for hiding this comment

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

"Outcome" comes from the outcome library, so an outcome wraps the result of a function.

Usually this things are named result or response

I think response is to be used when doing http request.

result would have been possible name, but it lacks the fact both ok and error are contained. Given in Python you expect a function to return the ok part and raise an exception for the errors, a dedicated name like outcome is good for readability.

Also suffix BadOutcome for enums which has the downside of using 2 words instead something like just Error.

This is a feature ! ^^
You have FooError which is an exception and FooBadOutcome which is an enum of possible errors.

server/parsec/asgi/rpc.py Outdated Show resolved Hide resolved
server/parsec/asgi/administration.py Outdated Show resolved Hide resolved
server/parsec/asgi/administration.py Show resolved Hide resolved
server/parsec/asgi/administration.py Outdated Show resolved Hide resolved
server/parsec/events.py Show resolved Hide resolved
@mmmarcos mmmarcos modified the milestones: v3.0, v3.1 Feb 12, 2024
@touilleMan touilleMan force-pushed the server-user-freeze branch 2 times, most recently from a251876 to eab7bfd Compare February 22, 2024 11:06
@touilleMan touilleMan added this pull request to the merge queue Feb 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 22, 2024
@touilleMan touilleMan enabled auto-merge February 22, 2024 16:43
@touilleMan touilleMan added this pull request to the merge queue Feb 22, 2024
Merged via the queue into master with commit 6ebf43b Feb 22, 2024
12 checks passed
@touilleMan touilleMan deleted the server-user-freeze branch February 22, 2024 16:59
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.

[🚀 | Feature request]: Port: User freeze
3 participants