Feature/Account Export and Deactivation Requests in APIv2 [PLAT-929] #8527
Conversation
pattisdr
force-pushed
the
feature/APIv2_account_export_deactivate
branch
from
8557311 to
dfc3350
Compare
pattisdr
force-pushed
the
feature/APIv2_account_export_deactivate
branch
from
dfc3350 to
9f4c839
Compare
| @@ -14,6 +14,7 @@ | |||
| 'root-anon-throttle': '1000000/second', | |||
| 'test-user': '2/hour', | |||
| 'test-anon': '1/hour', | |||
| 'send-email': '2/minute', | |||
There was a problem hiding this comment.
Not sure if I should increase throttle for tests - i have a test that is enforcing the throttle kicks in, so that is why I left it as-is.
There was a problem hiding this comment.
Makes sense to me. Potentially a bit high, but easy to change if we need to and better safe than sorry.
| @@ -176,6 +176,7 @@ | |||
| 'root-anon-throttle': '1000/hour', | |||
| 'test-user': '2/hour', | |||
| 'test-anon': '1/hour', | |||
| 'send-email': '2/minute', | |||
There was a problem hiding this comment.
Chose this because of current throttle requiring 30 seconds between requests, so somewhat similar.
| serializer_class = UserAccountExportSerializer | ||
| throttle_classes = (SendEmailThrottle, ) | ||
|
|
||
| def create(self, request, *args, **kwargs): |
There was a problem hiding this comment.
Overwrote the create method on the generic CreateAPIView so I can still check that the proper type is being sent through the serializer, but I don't have to call perform_create, and can return a 204 response.
| permission_classes = ( | ||
| drf_permissions.IsAuthenticatedOrReadOnly, | ||
| base_permissions.TokenHasScope, | ||
| CurrentUser, |
There was a problem hiding this comment.
Existing permission class requiring that logged-in user is the same as the user whose information I am trying to export.
api/users/views.py
Outdated
| CurrentUser, | ||
| ) | ||
|
|
||
| required_read_scopes = [CoreScopes.USERS_READ] |
There was a problem hiding this comment.
Not at all sure myself what the answer is, but I remember in the Node settings PR we worked on together we decided to add a new core scope for settings, namely NODE_SETTINGS_READ. Is that a thing to be done here as well? My gut is that maybe USER_SETTINGS is different enough from just user read that it warrants its own scope, but that very well may be overkill!
| assert user_one.email_last_sent is None | ||
| assert mock_mail.call_count == 0 | ||
|
|
||
| def test_exceed_throttle(self, app, user_one, url, payload): |
There was a problem hiding this comment.
Probably overkill, but you might want to mock out send_mail in these as well just to make sure the call isn't being generated. If not in a decorator at least perhaps in a context manager when you post, so you don't need an unused variable!
| def user_one(self): | ||
| return AuthUserFactory() | ||
|
|
||
| @pytest.fixture() |
There was a problem hiding this comment.
You could define user_one and user_two outside the test class to re-use them in both test classes
…ve user fixtures out so they can be shared between tests.
|
good tips, thanks @erinspace |
|
Looks good to me! |
There was a problem hiding this comment.
Just a question on scopes, but otherwise looks excellent! Thanks, @pattisdr!
| @@ -14,6 +14,7 @@ | |||
| 'root-anon-throttle': '1000000/second', | |||
| 'test-user': '2/hour', | |||
| 'test-anon': '1/hour', | |||
| 'send-email': '2/minute', | |||
There was a problem hiding this comment.
Makes sense to me. Potentially a bit high, but easy to change if we need to and better safe than sorry.
api/users/views.py
Outdated
| ) | ||
|
|
||
| required_read_scopes = [CoreScopes.USER_SETTINGS_READ] | ||
| required_write_scopes = [CoreScopes.NULL] |
There was a problem hiding this comment.
Should these scopes be the other way around since this endpoint is write-only?
required_read_scopes = [CoreScopes.NULL]
required_write_scopes = [CoreScopes.USER_SETTINGS_WRITE]
Purpose
Embosf will need this functionality to implement the account settings page.
Changes
Type is
user-account-export-formPOST to this endpoint should email the help desk (as clicking the button currently does)
Reponse should be
204 No ContentType is
user-account-deactivate-formPOST to this endpoint should email the help desk (as clicking the button currently does)
Reponse should be
204 No ContentQA Notes
None needed, can be tested on front end when finished.
Documentation
None needed. Don't need excessive help desk emails.
Side Effects
Ticket
https://openscience.atlassian.net/browse/PLAT-929