-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement Deactivate user endpoint on Connect #356
base: main
Are you sure you want to change the base?
Conversation
commcare_connect/users/views.py
Outdated
@csrf_exempt | ||
@api_view(["POST"]) | ||
@authentication_classes([OAuth2Authentication]) | ||
def user_deactivate(request): |
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.
is mobile supposed to call this or does connectid do it?
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 called by mobile.
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.
I think it would be safer and better if this were called by connectid. That way we don't need to rely on mobile, and as soon as a user account is deactivated we can be sure no new data will flow in
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.
Yeah, that makes sense. We need to think about the Authentication between Connect and ConnectID for making requests to this API.
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.
Sravan made a good point against this on the spec. Let's chat about this offline and make a decision
commcare_connect/users/urls.py
Outdated
@@ -21,4 +22,5 @@ | |||
path("accept_invite/<slug:invite_id>/", view=accept_invite, name="accept_invite"), | |||
path("demo_users/", view=demo_user_tokens, name="demo_users"), | |||
path("sms_status_callback/", SMSStatusCallbackView.as_view(), name="sms_status_callback"), | |||
path("user_suspend/", user_deactivate, name="user_suspend"), |
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.
nit
user_suspend
-> suspend_user
. user_deactivate
-> deactivate_user
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.
Fixed. 48a2069
@csrf_exempt | ||
@api_view(["POST"]) | ||
@authentication_classes([OAuth2Authentication]) | ||
def deactivate_user(request): |
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.
It would be a good practice to add a small note here as to where this is used.
Ticket