#160555688 Super admin can update user roles #54

CEOehis · 2018-09-18T09:15:09Z

What does this PR do?

implement the ability for a super admin to assign and revoke admin roles

Description of Task to be completed?

Only a super admin has access to this endpoint in the application. The super admin can make users into "admins" and can revoke such access.

include relevant tests
add methods to handle user role update
update swagger doc

How should this be manually tested?

copy the contents of .env.sample to .env and provide the required values.
Run npm run start:dev
log in with a super-admin account
send a POST request to /api/admin/:userId/roles using the super-admin token in the Authorization header to make a user an admin.
send a DELETE request to /api/admin/:userId/roles using the super-admin token in the Authorization header to revoke the admin access

What are the relevant pivotal tracker stories?

#160555688

houndci-bot · 2018-09-18T09:15:28Z

server/migrations/20180916093338-user-add-super-admin-role.js

+      DROP TYPE "enum_Users_role";
+      ALTER TYPE "enum_Users_role_new" RENAME TO "enum_Users_role";
+      `
+    ),


Unexpected newline before ')' function-paren-newline

houndci-bot · 2018-09-18T09:15:28Z

server/migrations/20180916093338-user-add-super-admin-role.js

+    ),
+
+  down: queryInterface => queryInterface.sequelize
+    .query(


Unexpected newline after '(' function-paren-newline

houndci-bot · 2018-09-18T09:15:28Z

server/migrations/20180916093338-user-add-super-admin-role.js

+      `
+        ALTER TYPE "enum_Users_role" ADD VALUE IF NOT EXISTS 'superAdmin';
+      `
+    ),


Unexpected newline before ')' function-paren-newline

houndci-bot · 2018-09-18T09:15:28Z

server/migrations/20180916093338-user-add-super-admin-role.js

+*/
+module.exports = {
+  up: queryInterface => queryInterface.sequelize
+    .query(


Unexpected newline after '(' function-paren-newline

coveralls · 2018-09-18T09:25:25Z

Pull Request Test Coverage Report for Build 495

64 of 64 (100.0%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 93.412%

Totals
Change from base Build 487:	0.3%
Covered Lines:	2284
Relevant Lines:	2404

💛 - Coveralls

emmaadesile · 2018-09-18T11:14:43Z

server/routes/api/admin.js


 // get authenticateUser method
-const { authenticateUser, authorizeAdmin } = auth;
+const { authenticateUser, authorizeAdmin, authorizeSuperAdmin } = auth;
 const adminRoutes = require('express').Router();


Good work declaring the adminRoutes on one line but you can make it more es6.

import { Router } from 'express' const adminRoutes = Router();

Thanks. I'll fix that ASAP.

iamuchejude

Implement fix on line 46 of adminHandleRoles.js in controller folder.
I also think some of your comments are not really necessary like you have on line 2 of server/migrations/20180916093338-user-add-super-admin-role.js.
Here is an article on the best practices for commenting your code to guide you: https://improvingsoftware.com/2011/06/27/5-best-practices-for-commenting-your-code/

iamuchejude · 2018-09-18T16:56:23Z

server/controllers/adminHandleRoles.js

+  }
+
+  /**
+   * assign a user an admin role


Misleading comment.
From what I see, the method is revoking admin role, not assigning admin role.

Thanks for the resource. That was an oversight. I'll fix that ASAP.

iamuchejude

It's advised you describe you add a description of a function parameter in jsdoc for better readability of your code.
Example is:

@params {obj} res response object

iamuchejude

It's advised you describe you add a description of a function parameter in jsdoc for better readability of your code. You did not do that in lines 99-101 in middleware/auth.js
An example is:

@params {obj} res response object

aknwosu · 2018-09-19T07:19:26Z

server/middleware/auth.js

+    const { userRole } = req;
+
+    if (!isSuperAdmin(userRole)) {
+      const error = new Error('you are not a Super Admin');


only a Super Admin can take this action

A super admin will be able to update user roles on the platform. Thus a new role and authorization is necessary. - add new role to users model - add method to authorize super admin - include relevant tests

Only a super admin has access to this endpoint in the application. The super admin can make users into "admins" and can revoke such access. - include relevant tests - add methods to handle user role update - update swagger doc [delivers 160555688]

Enable admins to update users' profile as well as assign roles from within the same method. Also added checks to limit assigning admin role to just the super admin. Refactored failing tests: used bulkcreate to create mock users. - update relevant tests

houndci-bot reviewed Sep 18, 2018

View reviewed changes

CEOehis force-pushed the ft-super-admin-role-160555688 branch from 35ba878 to 8c30823 Compare September 18, 2018 09:18

CEOehis force-pushed the ft-super-admin-role-160555688 branch from 8c30823 to 7ca39cf Compare September 18, 2018 09:26

CEOehis requested review from iamuchejude, emmaadesile, lauragift21 and aknwosu and removed request for lauragift21 September 18, 2018 09:35

CEOehis force-pushed the ft-super-admin-role-160555688 branch from 7ca39cf to a8d587e Compare September 18, 2018 11:26

emmaadesile suggested changes Sep 18, 2018

View reviewed changes

iamuchejude suggested changes Sep 18, 2018

View reviewed changes

CEOehis force-pushed the ft-super-admin-role-160555688 branch 2 times, most recently from e78d7f4 to 2659dea Compare September 18, 2018 17:15

iamuchejude reviewed Sep 18, 2018

View reviewed changes

iamuchejude approved these changes Sep 18, 2018

View reviewed changes

aknwosu reviewed Sep 19, 2018

View reviewed changes

CEOehis added 2 commits September 20, 2018 08:22

feat(admin-role): create super admin role

ada5f50

A super admin will be able to update user roles on the platform. Thus a new role and authorization is necessary. - add new role to users model - add method to authorize super admin - include relevant tests

CEOehis force-pushed the ft-super-admin-role-160555688 branch from 2659dea to 70d275e Compare September 20, 2018 10:28

CEOehis force-pushed the ft-super-admin-role-160555688 branch from 70d275e to e09e004 Compare September 20, 2018 10:56

aknwosu merged commit 675007f into develop Sep 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#160555688 Super admin can update user roles #54

#160555688 Super admin can update user roles #54

CEOehis commented Sep 18, 2018 •

edited

Loading

houndci-bot Sep 18, 2018

houndci-bot Sep 18, 2018

houndci-bot Sep 18, 2018

houndci-bot Sep 18, 2018

coveralls commented Sep 18, 2018 •

edited

Loading

emmaadesile Sep 18, 2018

CEOehis Sep 18, 2018

iamuchejude left a comment •

edited

Loading

iamuchejude Sep 18, 2018

CEOehis Sep 18, 2018

iamuchejude left a comment •

edited by aknwosu

Loading

iamuchejude left a comment •

edited by aknwosu

Loading

aknwosu Sep 19, 2018

#160555688 Super admin can update user roles #54

#160555688 Super admin can update user roles #54

Conversation

CEOehis commented Sep 18, 2018 • edited Loading

What does this PR do?

Description of Task to be completed?

How should this be manually tested?

What are the relevant pivotal tracker stories?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 18, 2018 • edited Loading

Pull Request Test Coverage Report for Build 495

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamuchejude left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamuchejude left a comment • edited by aknwosu Loading

Choose a reason for hiding this comment

iamuchejude left a comment • edited by aknwosu Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CEOehis commented Sep 18, 2018 •

edited

Loading

coveralls commented Sep 18, 2018 •

edited

Loading

iamuchejude left a comment •

edited

Loading

iamuchejude left a comment •

edited by aknwosu

Loading

iamuchejude left a comment •

edited by aknwosu

Loading