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

test: Revoking admin status #181

Closed
wants to merge 1 commit into from

Conversation

sharifa2708
Copy link

Description

Added tests for revoking admin status.
Fixes #142

Type of Change:

  • Code
  • Quality Assurance

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

NA

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged
    Code/Quality Assurance Only
  • My changes generate no new warnings

@sys-bot
Copy link

sys-bot commented Jan 24, 2019

Please send PRs only to approved issues.

@sys-bot sys-bot closed this Jan 24, 2019
@m-murad m-murad reopened this Jan 24, 2019
Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

Thank you for sending the PR @sharifa2708 🎉

From what I'm seeing there is some logic missing from the tests because the tests should fail when you don't send an auth header. Feel free to ask any questions regarding my change requests.

db.session.add(self.other_user)
db.session.commit()

def test_revoke_admin_admin(self):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_revoke_admin_admin(self):
def test_revoke_admin_role(self):

db.session.commit()

def test_revoke_admin_admin(self):
if (self.verified_user.is_admin and self.other_user.is_admin):
Copy link
Member

Choose a reason for hiding this comment

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

remove this if statement. you want to test the api, with this values regardless of their values

# self.assertEqual(expected_response, json.loads(actual_response.data))

def test_revoke_self_admin(self):
if(self.verified_user.is_admin and self.verified_user==self.other_user):
Copy link
Member

Choose a reason for hiding this comment

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

remove this if



def test_revoke_user(self):
if(self.verified_user.is_admin and self.other_user.is_admin == 0):
Copy link
Member

Choose a reason for hiding this comment

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

remove this if statement

def test_revoke_admin_admin(self):
if (self.verified_user.is_admin and self.other_user.is_admin):
expected_response = {'message': 'User admin status was revoked.'}
actual_response = self.client.get('/admin/remove', follow_redirects=True)
Copy link
Member

Choose a reason for hiding this comment

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

to test api, you have to create header arguments and then use it on the api call. Example: auth_header, otherwise the calls will fail, because the test user isn't authenticated. Do this for the other tests

@isabelcosta isabelcosta added the Category: Quality Assurance Changes to code or files that improve testing or fixes bugs. label Jan 30, 2019
Copy link
Contributor

@ramitsawhney27 ramitsawhney27 left a comment

Choose a reason for hiding this comment

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

Also, please look into your failing build and fix it.

super(TestListAdminsApi, self).setUp()

self.verified_user = UserModel(
name=user1['name'],
Copy link
Contributor

Choose a reason for hiding this comment

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

space between variables and operator, follow everywhere, complying to PEP8

self.assertEqual(expected_response, json.loads(actual_response.data))


# def test_revoke_non_user(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

No commented code in PRs please.

@isabelcosta
Copy link
Member

@sharifa2708 are you still working on this? If you're not, no problem :) We can make the issue available for other contributors.

@isabelcosta
Copy link
Member

Will close this issue and make it available again for lack of updates.
Thank you for your work here @sharifa2708 , I'm sure this will be useful for furure contributors :)

cc @ramitsawhney27

@isabelcosta isabelcosta closed this May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Quality Assurance Changes to code or files that improve testing or fixes bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests for Revoking admin role API
5 participants