- Notifications
You must be signed in to change notification settings - Fork928
feat: add PUT /api/v2/users/:user-id/suspend endpoint#1154
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedApr 25, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #1154 +/- ##==========================================- Coverage 66.34% 66.23% -0.11%========================================== Files 263 263 Lines 16615 16662 +47 Branches 156 156 ==========================================+ Hits 11023 11036 +13- Misses 4452 4477 +25- Partials 1140 1149 +9
Continue to review full report at Codecov.
|
Also related to#598 |
johnstcn left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
The change looks good to me (excepting the failed unit tests), but I'm confused as to why we're now creating auser_status column instead of asuspended column as was originally discussedhere.
What kind of user statuses do we plan to have besides "active" and "suspended"?
If these are the only we currently have planned, would it be simpler to have a boolean columnsuspended with a default value offalse?
| @@ -28,12 +28,20 @@ type UsersRequest struct { | |||
| Offset int `json:"offset"` | |||
| } | |||
| type UserStatus string | |||
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.
Instead ofUserStatus I'd be more in favor of@johnstcn's suggestion to usesuspended as a boolean. Because we only have two statuses for now, it feels weird to make it generic.
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.
There is this conversation herehttps://codercom.slack.com/archives/C014JH42DBJ/p1650895472652489?thread_ts=1650895472.652489&cid=C014JH42DBJ but I'm good using both.
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.
There is this conversation herehttps://codercom.slack.com/archives/C014JH42DBJ/p1650895472652489?thread_ts=1650895472.652489&cid=C014JH42DBJ but I'm good using both.
Ah, I missed parts of that thread. I'm fine either way honestly.
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.
@kylecarbs@johnstcn I think we will have more statuses likedormant
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.
Could a user bedormant andsuspended?
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.
@kylecarbs I believe the user would just besuspended.
-- Just my take --
Onlyactive users count towards the licensed seats. Adormant user is "inactive" based on activity. Adormant user can reactive themselves by just being active again. (Maybe they have to do some more steps, idk).
Asuspended user cannot even log in. Their account must be reactivated by an admin. The user has 0 input on this.
Uh oh!
There was an error while loading.Please reload this page.
Related to#662