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

Admin users can no longer delete/demote themselves #625

Merged
merged 1 commit into from
Sep 2, 2016
Merged

Conversation

linzjax
Copy link
Contributor

@linzjax linzjax commented Aug 31, 2016

Proposed changes in this pull request

When should this PR be merged

Whenever.

Risks

  • None

Follow up actions

  • Some changes to the API, so the documentation of errors needs to be updated, but not sure if it's worth it until the great API clean-up.

if user == request.user:
return Response(
{
_('WARNING'):
Copy link
Contributor

@ian-ross ian-ross Sep 2, 2016

Choose a reason for hiding this comment

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

This is the sole and single line I would quibble with in this PR. To me, "warning" implies that the action was taken, but you might not have wanted to do it. This though is an "error", since the action wasn't taken (because it's forbidden).

Also, you've invented a response format here that we don't use anywhere else (the {"WARNING": "blah"} thing). Instead, it might be better just to raise a Django PermissionDenied exception and let Django deal with generating the response in its usual format.

…ization

administrator can no longer change their own role in the organization.
@ian-ross ian-ross merged commit 1b39a37 into master Sep 2, 2016
@ian-ross ian-ross deleted the bug/#548 branch September 2, 2016 19:50
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.

4 participants