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

Revoke a user's admin role #54

Merged
merged 1 commit into from Jun 25, 2018

Conversation

isabelcosta
Copy link
Member

@isabelcosta isabelcosta commented Jun 21, 2018

Description

  • Added DAO function and API at POST /admin/remove, to revoke an admin role from a user
  • Added tests for DAO function
  • Updated Swagger API
  • Fix some points of discussion pending for project weekly meeting
  • Make sure these logic is respected... as a user:
    • I can assign myself as an admin = F
    • I can revoke my admin role = F
    • I have to be an admin to assign and revoke admin roles of others = T
    • I cannot delete my account if I’m the only Admin = T

Fixes #17

Type of Change:

  • Code

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update (software upgrade on readme file)
  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Added 3 tests for the AdminDAO function that revokes admin roles for the following cases:

  • Revoking a user which is not an admin - should fail
  • Revoking a user that does not exist - should fail
  • Revoking a user that is an admin - should succeed

Tested on Swagger UI, the same tests above to test the API response.

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • Update Postman API at /docs folder
  • Update Swagger documentation and the exported file at /docs folder

Code/Quality Assurance Only

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Contributor

@m-murad m-murad left a comment

Choose a reason for hiding this comment

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

Update the responses that you have written before this PR. Do it in the same PR

new_admin_user = UserModel.find_by_id(new_admin_user_id)

if new_admin_user:

if new_admin_user.is_admin:
return {"message": "User is already an Admin"}, 201
return {"message": "User is already an Admin."}, 200
Copy link
Contributor

Choose a reason for hiding this comment

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

The response should be a bad requestand code 400


else:
return {
"message": "You don't have admin status. You can't assign other user as admin."
Copy link
Contributor

Choose a reason for hiding this comment

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

403 Forbidden

Copy link
Member Author

Choose a reason for hiding this comment

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

This is fixed @m-murad (it does not show here because you commented on the line above)


else:
return {
"message": "You don't have admin status. You can't assign another admin"
}, 401
"message": "You don't have admin status. You can't revoke other admin user."
Copy link
Contributor

Choose a reason for hiding this comment

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

403 Forbidden

Copy link
Member Author

Choose a reason for hiding this comment

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

Also fixed @m-murad


return {"message": "User does not exist"}, 401
return {"message": "User does not exist."}, 400
Copy link
Contributor

Choose a reason for hiding this comment

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

404 - not found

return {"message": "User is now an Admin"}, 201
return {"message": "User is now an Admin."}, 200

return {"message": "User does not exist."}, 400
Copy link
Contributor

Choose a reason for hiding this comment

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

404


new_admin_user_id = data['user_id']

if assigner_user_id is new_admin_user_id:
return {"message": "You cannot assign yourself as an Admin."}, 400
Copy link
Contributor

Choose a reason for hiding this comment

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

403 Forbidden

admin_user_id = data['user_id']

if revoker_user_id is admin_user_id:
return {"message": "You cannot revoke your admin status."}, 400
Copy link
Contributor

Choose a reason for hiding this comment

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

403 Forbidden

if user:
user.delete_from_db()
return {"message": "User was deleted successfully"}, 201

return {"message": "User does not exist"}, 201
return {"message": "User does not exist"}, 400
Copy link
Contributor

Choose a reason for hiding this comment

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

404

@isabelcosta
Copy link
Member Author

wow thank you @m-murad , those error code reviews are very helpful

@isabelcosta
Copy link
Member Author

@m-murad @Dilu9218 fixed the required changes by @m-murad

@m-murad m-murad merged commit 0d51cd3 into anitab-org:gsoc18-code Jun 25, 2018
@isabelcosta isabelcosta deleted the remove-admin-role branch June 26, 2018 11:45
@isabelcosta isabelcosta added the Program: GSOC Related to work completed during the Google Summer of Code Program. label Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Program: GSOC Related to work completed during the Google Summer of Code Program.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants