add tests + make role decription optional + refacto

Signed-off-by: Pierre Noël <petersg83@gmail.com>

packages/strapi-admin/controllers/role.js

@@ -5,7 +5,7 @@ const { validateRoleUpdateInput } = require('../validation/role');
 module.exports = {
   async findOne(ctx) {
     const { id } = ctx.params;
-    const role = await strapi.admin.services.role.findOne({ id });
+    const role = await strapi.admin.services.role.findOne({ id }, []);
 
     if (!role) {
       return ctx.notFound('role.notFound');
@@ -16,7 +16,7 @@ module.exports = {
     };
   },
   async findAll(ctx) {
-    const roles = await strapi.admin.services.role.findAll();
+    const roles = await strapi.admin.services.role.findAll([]);
     ctx.body = {
       data: roles,
     };
@@ -36,8 +36,10 @@ module.exports = {
       return ctx.notFound('role.notFound');
     }
 
+    const sanitizedRole = strapi.admin.services.role.sanitizeRole(role);
+
     ctx.body = {
-      data: role,
+      data: sanitizedRole,
     };
   },
 };

packages/strapi-admin/ee/config/routes.json

@@ -7,14 +7,16 @@
       "config": {
         "policies": []
       }
-    }, {
+    },
+    {
       "method": "DELETE",
       "path": "/roles/:id",
       "handler": "role.deleteOne",
       "config": {
         "policies": []
       }
-    }, {
+    },
+    {
       "method": "POST",
       "path": "/roles/batch-delete",
       "handler": "role.deleteMany",

packages/strapi-admin/ee/controllers/role.js

@@ -44,10 +44,7 @@ module.exports = {
 
     const roles = await strapi.admin.services.role.deleteByIds([id]);
 
-    let sanitizedRole = null;
+    const sanitizedRole = roles.map(strapi.admin.services.role.sanitizeRole)[0] || null;
-    if (roles[0]) {
-      sanitizedRole = strapi.admin.services.role.sanitizeRole(roles[0]);
-    }
 
     ctx.body = {
       data: sanitizedRole,
@@ -61,7 +58,7 @@ module.exports = {
       return ctx.badRequest('ValidationError', err);
     }
 
-    let roles = await strapi.admin.services.role.deleteByIds(body.ids);
+    const roles = await strapi.admin.services.role.deleteByIds(body.ids);
     const sanitizedRoles = roles.map(strapi.admin.services.role.sanitizeRole);
 
     ctx.body = {

packages/strapi-admin/ee/validation/role.js

@@ -12,7 +12,7 @@ const roleCreateUpdateSchema = yup
       .string()
       .min(1)
       .required(),
-    description: yup.string().required(),
+    description: yup.string().nullable(),
   })
   .noUnknown();
 

packages/strapi-admin/models/Role.settings.json

@@ -17,8 +17,7 @@
     },
     "description": {
       "type": "string",
-      "configurable": false,
+      "configurable": false
-      "required": true
     },
     "users": {
       "configurable": false,

packages/strapi-admin/services/__tests__/role.test.js

@@ -36,7 +36,7 @@ describe('Role', () => {
 
       const foundRole = await roleService.findOne({ id: role.id });
 
-      expect(dbFindOne).toHaveBeenCalledWith({ id: role.id });
+      expect(dbFindOne).toHaveBeenCalledWith({ id: role.id }, undefined);
       expect(foundRole).toStrictEqual(role);
     });
   });
@@ -57,7 +57,7 @@ describe('Role', () => {
 
       const foundRoles = await roleService.find();
 
-      expect(dbFind).toHaveBeenCalledWith({});
+      expect(dbFind).toHaveBeenCalledWith({}, undefined);
       expect(foundRoles).toStrictEqual(roles);
     });
   });
@@ -78,7 +78,7 @@ describe('Role', () => {
 
       const foundRoles = await roleService.findAll();
 
-      expect(dbFind).toHaveBeenCalledWith({ _limit: -1 });
+      expect(dbFind).toHaveBeenCalledWith({ _limit: -1 }, undefined);
       expect(foundRoles).toStrictEqual(roles);
     });
   });
@@ -133,9 +133,11 @@ describe('Role', () => {
       };
       const dbDelete = jest.fn(() => Promise.resolve(role));
       const dbCount = jest.fn(() => Promise.resolve(0));
+      const dbDeleteByRolesIds = jest.fn(() => Promise.resolve());
 
       global.strapi = {
         query: () => ({ delete: dbDelete, count: dbCount }),
+        admin: { services: { permission: { deleteByRolesIds: dbDeleteByRolesIds } } },
       };
 
       const deletedRoles = await roleService.deleteByIds([role.id]);
@@ -162,9 +164,11 @@ describe('Role', () => {
       const rolesIds = roles.map(r => r.id);
       const dbDelete = jest.fn(() => Promise.resolve(roles));
       const dbCount = jest.fn(() => Promise.resolve(0));
+      const dbDeleteByRolesIds = jest.fn(() => Promise.resolve());
 
       global.strapi = {
         query: () => ({ delete: dbDelete, count: dbCount }),
+        admin: { services: { permission: { deleteByRolesIds: dbDeleteByRolesIds } } },
       };
 
       const deletedRoles = await roleService.deleteByIds(rolesIds);

packages/strapi-admin/services/permission.js

@@ -0,0 +1,7 @@
+const deleteByRolesIds = rolesIds => {
+  return strapi.query('permission', 'admin').delete({ role_in: rolesIds });
+};
+
+module.exports = {
+  deleteByRolesIds,
+};

packages/strapi-admin/services/role.js

@@ -1,7 +1,7 @@
 const _ = require('lodash');
 
 const sanitizeRole = role => {
-  return _.omit(role, ['users']);
+  return _.omit(role, ['users', 'permissions']);
 };
 
 /**
@@ -25,8 +25,8 @@ const create = async attributes => {
  * @param params query params to find the role
  * @returns {Promise<role>}
  */
-const findOne = (params = {}) => {
+const findOne = (params = {}, populate) => {
-  return strapi.query('role', 'admin').findOne(params);
+  return strapi.query('role', 'admin').findOne(params, populate);
 };
 
 /**
@@ -34,16 +34,16 @@ const findOne = (params = {}) => {
  * @param params query params to find the roles
  * @returns {Promise<array>}
  */
-const find = (params = {}) => {
+const find = (params = {}, populate) => {
-  return strapi.query('role', 'admin').find(params);
+  return strapi.query('role', 'admin').find(params, populate);
 };
 
 /**
  * Find all roles in database
  * @returns {Promise<array>}
  */
-const findAll = () => {
+const findAll = populate => {
-  return strapi.query('role', 'admin').find({ _limit: -1 });
+  return strapi.query('role', 'admin').find({ _limit: -1 }, populate);
 };
 
 /**
@@ -93,8 +93,7 @@ const deleteByIds = async (ids = []) => {
     }
   }
 
-  // TODO: Waiting for permissions
+  await strapi.admin.services.permission.deleteByRolesIds(ids);
-  // await strapi.admin.services.permission.delete({ roleId_in: rolesToDeleteIds });
 
   let deletedRoles = await strapi.query('role', 'admin').delete({ id_in: ids });
 

packages/strapi-admin/test/admin-role-crud.test.e2e.js

@@ -8,7 +8,9 @@ const edition = process.env.STRAPI_DISABLE_EE === 'true' ? 'CE' : 'EE';
 let rq;
 
 const data = {
-  roles: [],
+  rolesWithUsers: [],
+  rolesWithoutUsers: [],
+  users: [],
 };
 
 describe('Role CRUD End to End', () => {
@@ -42,10 +44,10 @@ describe('Role CRUD End to End', () => {
           created_at: expect.anything(),
           updated_at: expect.anything(),
         });
-        data.roles.push(res.body.data);
+        data.rolesWithoutUsers.push(res.body.data);
       });
       test('Cannot create a role already existing', async () => {
-        const role = _.pick(data.roles[0], ['name', 'description']);
+        const role = _.pick(data.rolesWithoutUsers[0], ['name', 'description']);
         const res = await rq({
           url: '/admin/roles',
           method: 'POST',
@@ -57,17 +59,37 @@ describe('Role CRUD End to End', () => {
           name: [`The name must be unique and a role with name \`${role.name}\` already exists.`],
         });
       });
+      test('Can create a user with a role', async () => {
+        const user = {
+          email: 'new-user@strapi.io',
+          firstname: 'New',
+          lastname: 'User',
+          roles: [data.rolesWithoutUsers[5].id],
+        };
+
+        const res = await rq({
+          url: '/admin/users',
+          method: 'POST',
+          body: user,
+        });
+
+        expect(res.statusCode).toBe(201);
+
+        data.users.push(res.body.data);
+        data.rolesWithUsers.push(data.rolesWithoutUsers[5]);
+        data.rolesWithoutUsers.splice(5, 1);
+      });
     });
 
     describe('Find a role', () => {
       test('Can find a role successfully', async () => {
         const res = await rq({
-          url: `/admin/roles/${data.roles[0].id}`,
+          url: `/admin/roles/${data.rolesWithoutUsers[0].id}`,
           method: 'GET',
         });
 
         expect(res.statusCode).toBe(200);
-        expect(res.body.data).toMatchObject(data.roles[0]);
+        expect(res.body.data).toMatchObject(data.rolesWithoutUsers[0]);
       });
     });
 
@@ -79,7 +101,7 @@ describe('Role CRUD End to End', () => {
         });
 
         expect(res.statusCode).toBe(200);
-        expect(res.body.data).toMatchObject(data.roles);
+        expect(res.body.data).toMatchObject(data.rolesWithoutUsers.concat(data.rolesWithUsers));
       });
     });
 
@@ -90,18 +112,18 @@ describe('Role CRUD End to End', () => {
           description: 'new description - Can update a role successfully',
         };
         const res = await rq({
-          url: `/admin/roles/${data.roles[0].id}`,
+          url: `/admin/roles/${data.rolesWithoutUsers[0].id}`,
           method: 'PUT',
           body: updates,
         });
 
         expect(res.statusCode).toBe(200);
         expect(res.body.data).toMatchObject({
-          ...data.roles[0],
+          ...data.rolesWithoutUsers[0],
           ...updates,
           updated_at: expect.anything(),
         });
-        data.roles[0] = res.body.data;
+        data.rolesWithoutUsers[0] = res.body.data;
       });
       test('Can update description of a role successfully', async () => {
         const updates = {
@@ -109,26 +131,26 @@ describe('Role CRUD End to End', () => {
           description: 'new description - Can update description of a role successfully',
         };
         const res = await rq({
-          url: `/admin/roles/${data.roles[0].id}`,
+          url: `/admin/roles/${data.rolesWithoutUsers[0].id}`,
           method: 'PUT',
           body: updates,
         });
 
         expect(res.statusCode).toBe(200);
         expect(res.body.data).toMatchObject({
-          ...data.roles[0],
+          ...data.rolesWithoutUsers[0],
           ...updates,
           updated_at: expect.anything(),
         });
-        data.roles[0] = res.body.data;
+        data.rolesWithoutUsers[0] = res.body.data;
       });
       test('Cannot update the name of a role if already exists', async () => {
         const updates = {
-          name: data.roles[0].name,
+          name: data.rolesWithoutUsers[0].name,
           description: 'new description - Cannot update the name of a role if already exists',
         };
         const res = await rq({
-          url: `/admin/roles/${data.roles[1].id}`,
+          url: `/admin/roles/${data.rolesWithoutUsers[1].id}`,
           method: 'PUT',
           body: updates,
         });
@@ -136,7 +158,7 @@ describe('Role CRUD End to End', () => {
         expect(res.statusCode).toBe(400);
         expect(res.body.data).toMatchObject({
           name: [
-            `The name must be unique and a role with name \`${data.roles[0].name}\` already exists.`,
+            `The name must be unique and a role with name \`${data.rolesWithoutUsers[0].name}\` already exists.`,
           ],
         });
       });
@@ -161,25 +183,48 @@ describe('Role CRUD End to End', () => {
     });
     describe('Delete roles', () => {
       describe('batch-delete', () => {
+        test("Don't delete the roles if some still have assigned users", async () => {
+          const roles = [data.rolesWithUsers[0], data.rolesWithUsers[0]];
+          const rolesIds = roles.map(r => r.id);
+          let res = await rq({
+            url: '/admin/roles/batch-delete',
+            method: 'POST',
+            body: { ids: rolesIds },
+          });
+
+          expect(res.statusCode).toBe(400);
+          expect(res.body.data).toMatchObject({
+            ids: ['Some roles are still assigned to some users.'],
+          });
+
+          for (let role of roles) {
+            res = await rq({
+              url: `/admin/roles/${role.id}`,
+              method: 'GET',
+            });
+            expect(res.statusCode).toBe(200);
+            expect(res.body.data).toMatchObject(role);
+          }
+        });
         test('Can delete a role', async () => {
           let res = await rq({
             url: '/admin/roles/batch-delete',
             method: 'POST',
-            body: { ids: [data.roles[0].id] },
+            body: { ids: [data.rolesWithoutUsers[0].id] },
           });
           expect(res.statusCode).toBe(200);
-          expect(res.body.data).toMatchObject([data.roles[0]]);
+          expect(res.body.data).toMatchObject([data.rolesWithoutUsers[0]]);
 
           res = await rq({
-            url: `/admin/roles/${data.roles[0].id}`,
+            url: `/admin/roles/${data.rolesWithoutUsers[0].id}`,
             method: 'GET',
           });
           expect(res.statusCode).toBe(404);
 
-          data.roles.shift();
+          data.rolesWithoutUsers.shift();
         });
         test('Can delete two roles', async () => {
-          const roles = data.roles.slice(0, 2);
+          const roles = data.rolesWithoutUsers.slice(0, 2);
           const rolesIds = roles.map(r => r.id);
 
           let res = await rq({
@@ -196,7 +241,7 @@ describe('Role CRUD End to End', () => {
               method: 'GET',
             });
             expect(res.statusCode).toBe(404);
-            data.roles.shift();
+            data.rolesWithoutUsers.shift();
           }
         });
         test("No error if deleting a role that doesn't exist", async () => {
@@ -209,26 +254,23 @@ describe('Role CRUD End to End', () => {
           expect(res.statusCode).toBe(200);
           expect(res.body.data).toEqual([]);
         });
-        test.skip("Don't delete any role if some still have assigned users", async () => {
-          // TODO
-        });
       });
       describe('simple delete', () => {
         test('Can delete a role', async () => {
           let res = await rq({
-            url: `/admin/roles/${data.roles[0].id}`,
+            url: `/admin/roles/${data.rolesWithoutUsers[0].id}`,
             method: 'DELETE',
           });
           expect(res.statusCode).toBe(200);
-          expect(res.body.data).toMatchObject(data.roles[0]);
+          expect(res.body.data).toMatchObject(data.rolesWithoutUsers[0]);
 
           res = await rq({
-            url: `/admin/roles/${data.roles[0].id}`,
+            url: `/admin/roles/${data.rolesWithoutUsers[0].id}`,
             method: 'GET',
           });
           expect(res.statusCode).toBe(404);
 
-          data.roles.shift();
+          data.rolesWithoutUsers.shift();
         });
         test("No error if deleting a role that doesn't exist", async () => {
           const res = await rq({
@@ -239,8 +281,23 @@ describe('Role CRUD End to End', () => {
           expect(res.statusCode).toBe(200);
           expect(res.body.data).toEqual(null);
         });
-        test.skip("Don't delete a role if it still has assigned users", async () => {
+        test("Don't delete a role if it still has assigned users", async () => {
-          // TODO
+          let res = await rq({
+            url: `/admin/roles/${data.rolesWithUsers[0].id}`,
+            method: 'DELETE',
+          });
+
+          expect(res.statusCode).toBe(400);
+          expect(res.body.data).toMatchObject({
+            ids: ['Some roles are still assigned to some users.'],
+          });
+
+          res = await rq({
+            url: `/admin/roles/${data.rolesWithUsers[0].id}`,
+            method: 'GET',
+          });
+          expect(res.statusCode).toBe(200);
+          expect(res.body.data).toMatchObject(data.rolesWithUsers[0]);
         });
       });
     });

packages/strapi-connector-bookshelf/lib/buildQuery.js

@@ -1,6 +1,5 @@
 const _ = require('lodash');
 const { singular } = require('pluralize');
-const { findModelByAssoc } = require('strapi-utils');
 
 /**
  * Build filters on a bookshelf query
@@ -164,7 +163,7 @@ const buildJoinsAndFilter = (qb, model, whereClauses) => {
         continue;
       }
 
-      const assocModel = findModelByAssoc(assoc);
+      const assocModel = strapi.db.getModelByAssoc(assoc);
 
       // if the last part of the path is an association
       // add the primary key of the model to the parts

packages/strapi-connector-mongoose/lib/buildQuery.js

@@ -295,7 +295,7 @@ const buildQueryAggregate = (model, { paths } = {}) => {
  */
 const buildLookup = ({ model, key, paths }) => {
   const assoc = model.associations.find(a => a.alias === key);
-  const assocModel = findModelByAssoc({ assoc });
+  const assocModel = strapi.db.getModelByAssoc(assoc);
 
   if (!assocModel) return [];
 
@@ -542,7 +542,7 @@ const getAssociationFromFieldKey = (model, fieldKey) => {
   for (let key of parts) {
     assoc = tmpModel.associations.find(ast => ast.alias === key);
     if (assoc) {
-      tmpModel = findModelByAssoc({ assoc });
+      tmpModel = strapi.db.getModelByAssoc(assoc);
     }
   }
 
@@ -565,7 +565,7 @@ const findModelByPath = ({ rootModel, path }) => {
   for (let part of parts) {
     const assoc = tmpModel.associations.find(ast => ast.alias === part);
     if (assoc) {
-      tmpModel = findModelByAssoc({ assoc });
+      tmpModel = strapi.db.getModelByAssoc(assoc);
     }
   }
 
@@ -587,7 +587,7 @@ const findModelPath = ({ rootModel, path }) => {
     const assoc = tmpModel.associations.find(ast => ast.alias === part);
 
     if (assoc) {
-      tmpModel = findModelByAssoc({ assoc });
+      tmpModel = strapi.db.getModelByAssoc(assoc);
       tmpPath.push(part);
     }
   }
@@ -595,9 +595,4 @@ const findModelPath = ({ rootModel, path }) => {
   return tmpPath.length > 0 ? tmpPath.join('.') : null;
 };
 
-const findModelByAssoc = ({ assoc }) => {
-  const { models } = strapi.plugins[assoc.plugin] || strapi;
-  return models[assoc.model || assoc.collection];
-};
-
 module.exports = buildQuery;

packages/strapi-database/index.d.ts

@@ -10,6 +10,7 @@ export class DatabaseManager {
   initialize(): Promise<DatabaseManager>;
   query(model: string, plugin: string): Repository;
   getModel(model: string, plugin: string): Model;
+  getModelByAssoc(assoc: object): Model;
 }
 
 class Model {}

packages/strapi-database/lib/database-manager.js

@@ -108,6 +108,10 @@ class DatabaseManager {
     return _.get(strapi, ['models', key]) || _.get(strapi, ['components', key]);
   }
 
+  getModelByAssoc(assoc) {
+    return this.getModel(assoc.collection || assoc.model, assoc.plugin);
+  }
+
   getModelByCollectionName(collectionName) {
     return Array.from(this.models.values()).find(model => {
       return model.collectionName === collectionName;

packages/strapi-utils/lib/buildQuery.js

@@ -142,7 +142,4 @@ const buildQuery = ({ model, filters = {}, ...rest }) => {
   return strapi.db.connectors.get(model.orm).buildQuery({ model, filters, ...rest });
 };
 
-module.exports = {
+module.exports = buildQuery;
-  buildQuery,
-  findModelByAssoc,
-};

packages/strapi-utils/lib/index.js

@@ -5,7 +5,7 @@
  */
 
 const convertRestQueryParams = require('./convertRestQueryParams');
-const { buildQuery, findModelByAssoc } = require('./buildQuery');
+const buildQuery = require('./buildQuery');
 const parseMultipartData = require('./parse-multipart');
 const sanitizeEntity = require('./sanitize-entity');
 const parseType = require('./parse-type');
@@ -34,7 +34,6 @@ module.exports = {
   templateConfiguration,
   convertRestQueryParams,
   buildQuery,
-  findModelByAssoc,
   parseMultipartData,
   sanitizeEntity,
   parseType,