Fix PR: Refactor user e2e tests, fix domain logic, add user::deleteOne route

packages/strapi-admin/config/routes.json

@@ -150,6 +150,11 @@
       "path": "/users/:id",
       "handler": "user.update"
     },
+    {
+      "method": "DELETE",
+      "path": "/users/:id",
+      "handler": "user.delete"
+    },
     {
       "method": "GET",
       "path": "/roles/:id/permissions",

packages/strapi-admin/controllers/__tests__/user.test.js

@@ -202,10 +202,10 @@ describe('User Controller', () => {
     test('User not found', async () => {
       const fakeId = 42;
       const exists = jest.fn(() => false);
-      const badRequest = jest.fn();
+      const notFound = jest.fn();
       const body = { username: 'Foo' };
 
-      const ctx = createContext({ params: { id: fakeId }, body }, { badRequest });
+      const ctx = createContext({ params: { id: fakeId }, body }, { notFound });
 
       global.strapi = {
         admin: {
@@ -219,7 +219,7 @@ describe('User Controller', () => {
 
       expect(exists).toHaveBeenCalledWith({ id: fakeId });
       expect(exists).toHaveReturnedWith(false);
-      expect(badRequest).toHaveBeenCalledWith('User does not exist');
+      expect(notFound).toHaveBeenCalledWith('User does not exist');
     });
 
     test('Validation error', async () => {

packages/strapi-admin/controllers/user.js

@@ -50,7 +50,7 @@ module.exports = {
     const user = await strapi.admin.services.user.findOne({ id });
 
     if (!user) {
-      return ctx.badRequest('User does not exist');
+      return ctx.notFound('User does not exist');
     }
 
     ctx.body = {
@@ -68,16 +68,32 @@ module.exports = {
       return ctx.badRequest('ValidationError', err);
     }
 
-    const userExists = await strapi.admin.services.user.exists({ id });
+    const updatedUser = await strapi.admin.services.user.update({ id }, input);
 
-    if (!userExists) {
+    if (!updatedUser) {
       return ctx.badRequest('User does not exist');
     }
 
-    const updatedUser = await strapi.admin.services.user.update({ id }, input);
-
     ctx.body = {
       data: strapi.admin.services.user.sanitizeUser(updatedUser),
     };
   },
+
+  async delete(ctx) {
+    const { id } = ctx.params;
+
+    if (!id) {
+      return ctx.badRequest('Invalid ID');
+    }
+
+    const deletedUser = await strapi.admin.services.user.deleteOne({ id });
+
+    if (!deletedUser) {
+      return ctx.notFound('User not found');
+    }
+
+    return ctx.deleted({
+      data: strapi.admin.services.user.sanitizeUser(deletedUser),
+    });
+  },
 };

packages/strapi-admin/domain/user.js

@@ -8,6 +8,7 @@ function createUser(attributes) {
   return {
     roles: [],
     isActive: false,
+    username: null,
     ...attributes,
   };
 }

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

@@ -1,7 +1,6 @@
 'use strict';
 
 const { yup, formatYupErrors } = require('strapi-utils');
-const { strapiId } = require('../../validation/common-validators');
 
 const handleReject = error => Promise.reject(formatYupErrors(error));
 
@@ -21,7 +20,7 @@ const roleDeleteSchema = yup
   .shape({
     ids: yup
       .array()
-      .of(strapiId)
+      .of(yup.strapiID())
       .min(1)
       .required(),
   })

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

@@ -29,7 +29,8 @@
       "type": "email",
       "minLength": 6,
       "configurable": false,
-      "required": true
+      "required": true,
+      "unique": true
     },
     "password": {
       "type": "password",

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

@@ -284,7 +284,7 @@ describe('User', () => {
       };
     });
 
-    test('Find and returns a user by its ID', async () => {
+    test('Finds and returns a user by its ID', async () => {
       const input = { id: 1 };
       const res = await userService.findOne(input);
 
@@ -292,7 +292,7 @@ describe('User', () => {
       expect(res).toStrictEqual(user);
     });
 
-    test('Fail to find a user with provided params', async () => {
+    test('Fails to find a user with provided params', async () => {
       const input = { id: 27 };
       const res = await userService.findOne(input);
 

packages/strapi-admin/services/user.js

@@ -132,6 +132,14 @@ const searchPage = async query => {
   return strapi.query('user', 'admin').searchPage(query);
 };
 
+/** Delete a user
+ * @param query
+ * @returns {Promise<user>}
+ */
+const deleteOne = async query => {
+  return strapi.query('user', 'admin').delete(query);
+};
+
 module.exports = {
   create,
   update,
@@ -142,4 +150,5 @@ module.exports = {
   findOne,
   findPage,
   searchPage,
+  deleteOne,
 };

packages/strapi-admin/test/admin-auth.test.e2e.js

@@ -4,25 +4,50 @@ const { createAuthRequest } = require('../../../test/helpers/request');
 
 let rq;
 
-const createUser = data => {
-  return rq({
-    url: '/admin/users',
+const createAuthRole = async () => {
+  const res = await rq({
+    url: '/admin/roles',
     method: 'POST',
     body: {
-      roles: [1],
-      ...data,
+      name: 'auth_test_role',
+      description: 'Only used for auth crud test (e2e)',
     },
   });
+
+  return res && res.body && res.body.data;
 };
 
 describe('Admin Auth End to End', () => {
+  const testData = {
+    role: null,
+  };
+
+  const createUser = data => {
+    return rq({
+      url: '/admin/users',
+      method: 'POST',
+      body: {
+        roles: [testData.role.id],
+        ...data,
+      },
+    });
+  };
+
   beforeAll(async () => {
     const token = await registerAndLogin();
     rq = createAuthRequest(token);
+    testData.role = await createAuthRole();
+  }, 60000);
+
+  afterAll(async () => {
+    await rq({
+      url: `/admin/roles/${testData.role.id}`,
+      method: 'DELETE',
+    });
   }, 60000);
 
   describe('Login', () => {
-    test('Can connect successfuklly', async () => {
+    test('Can connect successfully', async () => {
       const res = await rq({
         url: '/admin/login',
         method: 'POST',

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

@@ -1,253 +0,0 @@
-// Helpers.
-const { registerAndLogin } = require('../../../test/helpers/auth');
-const { createAuthRequest } = require('../../../test/helpers/request');
-
-let rq;
-
-describe('Admin User CRUD End to End', () => {
-  beforeAll(async () => {
-    const token = await registerAndLogin();
-    rq = createAuthRequest(token);
-  }, 60000);
-
-  describe('Create a new user', () => {
-    // FIXME: Waiting for strapi-admin::roles API
-    test.skip('Can create a user successfully', async () => {
-      const user = {
-        email: 'new-user@strapi.io',
-        firstname: 'New',
-        lastname: 'User',
-        roles: ['41224d776a326fb40f000001'],
-      };
-
-      const res = await rq({
-        url: '/admin/users',
-        method: 'POST',
-        body: user,
-      });
-
-      expect(res.statusCode).toBe(201);
-      expect(res.body).toMatchObject({
-        firstname: user.firstname,
-        lastname: user.lastname,
-        email: user.email,
-        registrationToken: expect.any(String),
-        isActive: false,
-        roles: [],
-      });
-    });
-
-    test('Fails on missing field (email)', async () => {
-      const user = {
-        firstname: 'New',
-        lastname: 'User',
-        roles: [1, 2],
-      };
-
-      const res = await rq({
-        url: '/admin/users',
-        method: 'POST',
-        body: user,
-      });
-
-      expect(res.statusCode).toBe(400);
-      expect(res.body).toMatchObject({
-        statusCode: 400,
-        error: 'Bad Request',
-        message: 'ValidationError',
-        data: {
-          email: ['email is a required field'],
-        },
-      });
-    });
-
-    test('Fails on invalid field type (firstname)', async () => {
-      const user = {
-        email: 'new-user@strapi.io',
-        firstname: 1,
-        lastname: 'User',
-        roles: [1, 2],
-      };
-
-      const res = await rq({
-        url: '/admin/users',
-        method: 'POST',
-        body: user,
-      });
-
-      expect(res.statusCode).toBe(400);
-      expect(res.body).toMatchObject({
-        statusCode: 400,
-        error: 'Bad Request',
-        message: 'ValidationError',
-        data: {
-          firstname: ['firstname must be a `string` type, but the final value was: `1`.'],
-        },
-      });
-    });
-  });
-
-  describe('Update a user', () => {
-    test('Fails on invalid payload', async () => {
-      const body = { firstname: 42 };
-
-      const res = await rq({
-        url: '/admin/users/1',
-        method: 'PUT',
-        body,
-      });
-
-      expect(res.statusCode).toBe(400);
-      expect(res.body).toStrictEqual({
-        data: {
-          firstname: ['firstname must be a `string` type, but the final value was: `42`.'],
-        },
-        error: 'Bad Request',
-        message: 'ValidationError',
-        statusCode: 400,
-      });
-    });
-
-    test('Fails on user not found', async () => {
-      const body = { firstname: 'foo' };
-
-      const res = await rq({
-        url: '/admin/users/999999',
-        method: 'PUT',
-        body,
-      });
-
-      expect(res.statusCode).toBe(400);
-      expect(res.body).toStrictEqual({
-        error: 'Bad Request',
-        message: 'User does not exist',
-        statusCode: 400,
-      });
-    });
-
-    test('Can update a user successfully', async () => {
-      const body = { firstname: 'foo' };
-
-      const res = await rq({
-        url: '/admin/users/1',
-        method: 'PUT',
-        body,
-      });
-
-      expect(res.statusCode).toBe(200);
-      expect(res.body).toStrictEqual({
-        data: {
-          email: 'admin@strapi.io',
-          firstname: 'foo',
-          lastname: 'admin',
-          username: null,
-          id: 1,
-          isActive: true,
-          registrationToken: null,
-          roles: [],
-        },
-      });
-    });
-  });
-
-  describe('Fetch a user', () => {
-    test('User does not exist', async () => {
-      const res = await rq({
-        url: '/admin/users/999',
-        method: 'GET',
-      });
-
-      expect(res.statusCode).toBe(400);
-      expect(res.body).toStrictEqual({
-        error: 'Bad Request',
-        message: 'User does not exist',
-        statusCode: 400,
-      });
-    });
-
-    test('Find one user successfully', async () => {
-      const res = await rq({
-        url: '/admin/users/1',
-        method: 'GET',
-      });
-
-      expect(res.statusCode).toBe(200);
-      expect(res.body).toStrictEqual({
-        data: {
-          email: 'admin@strapi.io',
-          firstname: 'foo',
-          lastname: 'admin',
-          username: null,
-          id: 1,
-          isActive: true,
-          registrationToken: null,
-          roles: [],
-        },
-      });
-    });
-  });
-
-  describe('Fetch a list of user', () => {
-    test('Using findPage', async () => {
-      const res = await rq({
-        url: '/admin/users',
-        method: 'GET',
-      });
-
-      expect(res.statusCode).toBe(200);
-      expect(res.body).toStrictEqual({
-        data: {
-          pagination: {
-            page: 1,
-            pageSize: 100,
-            total: 1,
-            pageCount: 1,
-          },
-          results: [
-            {
-              email: 'admin@strapi.io',
-              firstname: 'foo',
-              lastname: 'admin',
-              username: null,
-              registrationToken: null,
-              roles: [],
-              id: 1,
-              isActive: true,
-            },
-          ],
-        },
-      });
-    });
-
-    test('Using searchPage', async () => {
-      const res = await rq({
-        url: '/admin/users?_q=foo',
-        method: 'GET',
-      });
-
-      expect(res.statusCode).toBe(200);
-      expect(res.body).toStrictEqual({
-        data: {
-          pagination: {
-            page: 1,
-            pageSize: 100,
-            total: 1,
-            pageCount: 1,
-          },
-          results: [
-            {
-              email: 'admin@strapi.io',
-              firstname: 'foo',
-              lastname: 'admin',
-              username: null,
-              registrationToken: null,
-              roles: [],
-              id: 1,
-              isActive: true,
-            },
-          ],
-        },
-      });
-    });
-  });
-});

packages/strapi-admin/test/admin-user.test.e2e.js

@@ -0,0 +1,276 @@
+'use strict';
+
+const { login, registerAndLogin } = require('../../../test/helpers/auth');
+const { createAuthRequest } = require('../../../test/helpers/request');
+
+const getAuthToken = async () => {
+  let token = await login();
+
+  if (!token) {
+    token = await registerAndLogin();
+  }
+
+  return token;
+};
+
+const createUserRole = async () => {
+  const res = await rq({
+    url: '/admin/roles',
+    method: 'POST',
+    body: {
+      name: 'user_test_role',
+      description: 'Only used for user crud test (e2e)',
+    },
+  });
+
+  return res && res.body && res.body.data;
+};
+
+const deleteUserRole = async id => {
+  await rq({
+    url: `/admin/roles/${id}`,
+    method: 'DELETE',
+  });
+};
+
+let rq;
+
+/**
+ * == Test Suite Overview ==
+ *
+ * N°   Description
+ * -------------------------------------------
+ * 1.   Create a user (fail/body)
+ * 2.   Create a user (success)
+ * 3.   Update a user (success)
+ * 4.   Update a user (fail/body)
+ * 5.   Get a user (success)
+ * 6.   Get a list of users (success/full)
+ * 7.   Delete a user (success)
+ * 8.   Delete a user (fail/notFound)
+ * 9.   Update a user (fail/notFound)
+ * 10.  Get a user (fail/notFound)
+ * 11.  Get a list of users (success/empty)
+ */
+describe('Admin User CRUD (e2e)', () => {
+  // Local test data used across the test suite
+  let testData = {
+    user: undefined,
+    role: undefined,
+  };
+
+  // Initialization Actions
+  beforeAll(async () => {
+    const token = await getAuthToken();
+    rq = createAuthRequest(token);
+    testData.role = await createUserRole();
+  });
+
+  // Cleanup actions
+  afterAll(async () => {
+    await deleteUserRole(testData.role.id);
+  });
+
+  test('1. Creates a user (wrong body)', async () => {
+    const body = {
+      firstname: 'user_tests-firstname',
+      lastname: 'user_tests-lastname',
+      roles: [testData.role.id],
+    };
+
+    const res = await rq({
+      url: '/admin/users',
+      method: 'POST',
+      body,
+    });
+
+    expect(res.statusCode).toBe(400);
+    expect(res.body).toMatchObject({
+      statusCode: 400,
+      error: 'Bad Request',
+      message: 'ValidationError',
+      data: {
+        email: ['email is a required field'],
+      },
+    });
+  });
+
+  test('2. Creates a user (successfully)', async () => {
+    const body = {
+      email: 'user-tests@strapi-e2e.com',
+      firstname: 'user_tests-firstname',
+      lastname: 'user_tests-lastname',
+      roles: [testData.role.id],
+    };
+
+    const res = await rq({
+      url: '/admin/users',
+      method: 'POST',
+      body,
+    });
+
+    expect(res.statusCode).toBe(201);
+    expect(res.body.data).not.toBeNull();
+
+    // Using the created user as an example for the rest of the tests
+    testData.user = res.body.data;
+  });
+
+  test('3. Updates a user (wrong body)', async () => {
+    const body = {
+      email: 42,
+    };
+
+    const res = await rq({
+      url: `/admin/users/${testData.user.id}`,
+      method: 'PUT',
+      body,
+    });
+
+    expect(res.statusCode).toBe(400);
+    expect(res.body).toMatchObject({
+      statusCode: 400,
+      error: 'Bad Request',
+      message: 'ValidationError',
+      data: {
+        email: ['email must be a `string` type, but the final value was: `42`.'],
+      },
+    });
+  });
+
+  test('4. Updates a user (successfully)', async () => {
+    const body = {
+      firstname: 'foobar',
+    };
+
+    const res = await rq({
+      url: `/admin/users/${testData.user.id}`,
+      method: 'PUT',
+      body,
+    });
+
+    expect(res.statusCode).toBe(200);
+    expect(res.body).not.toBeNull();
+    expect({ updatedAt: null, ...res.body.data }).toMatchObject({
+      ...testData.user,
+      ...body,
+      updatedAt: expect.stringOrNull(),
+    });
+
+    // Update the local copy of the user
+    testData.user = res.body.data;
+  });
+
+  test('5. Finds a user (successfully)', async () => {
+    const res = await rq({
+      url: `/admin/users/${testData.user.id}`,
+      method: 'GET',
+    });
+
+    expect(res.statusCode).toBe(200);
+    expect(res.body.data).toMatchObject(testData.user);
+  });
+
+  describe('6. Finds a list of users (contains user)', () => {
+    const expectedResults = () => ({
+      data: {
+        pagination: {
+          page: 1,
+          pageSize: expect.any(Number),
+          pageCount: expect.any(Number),
+          total: expect.any(Number),
+        },
+        results: expect.any(Array),
+      },
+    });
+
+    test('6.1. Using findPage', async () => {
+      const res = await rq({
+        url: `/admin/users?email=${testData.user.email}`,
+        method: 'GET',
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(res.body).toMatchObject(expectedResults());
+      expect(res.body.data.results).toContainEqual(testData.user);
+    });
+
+    test('6.2. Using searchPage', async () => {
+      const res = await rq({
+        url: `/admin/users?_q=${testData.user.email}`,
+        method: 'GET',
+      });
+
+      expect(res.statusCode).toBe(200);
+      expect(res.body).toMatchObject(expectedResults());
+      expect(res.body.data.results).toContainEqual(testData.user);
+    });
+  });
+
+  test('7. Deletes a user (successfully)', async () => {
+    const res = await rq({
+      url: `/admin/users/${testData.user.id}`,
+      method: 'DELETE',
+    });
+
+    expect(res.statusCode).toBe(200);
+    expect(res.body.data).toMatchObject(testData.user);
+  });
+
+  test('8. Deletes a user (not found)', async () => {
+    const res = await rq({
+      url: `/admin/users/${testData.user.id}`,
+      method: 'DELETE',
+    });
+
+    expect(res.statusCode).toBe(404);
+  });
+
+  test('9. Updates a user (not found)', async () => {
+    const body = {
+      lastname: 'doe',
+    };
+
+    const res = await rq({
+      url: `/admin/users/${testData.user.id}`,
+      method: 'PUT',
+      body,
+    });
+
+    expect(res.statusCode).toBe(404);
+    expect(res.body).toMatchObject({
+      error: 'Not Found',
+      message: 'entry.notFound',
+      statusCode: 404,
+    });
+  });
+
+  test('10. Finds a user (not found)', async () => {
+    const res = await rq({
+      url: `/admin/users/${testData.user.id}`,
+      method: 'GET',
+    });
+
+    expect(res.statusCode).toBe(404);
+    expect(res.body.data).toBeUndefined();
+  });
+
+  test('12. Finds a list of users (missing user)', async () => {
+    const res = await rq({
+      url: `/admin/users?email=${testData.user.email}`,
+      method: 'GET',
+    });
+
+    expect(res.statusCode).toBe(200);
+    expect(res.body.data).toMatchObject({
+      pagination: {
+        page: 1,
+        pageSize: expect.any(Number),
+        pageCount: expect.any(Number),
+        total: expect.any(Number),
+      },
+      results: expect.any(Array),
+    });
+    expect(res.body.data.results).not.toContainEqual(testData.user);
+  });
+});

packages/strapi-admin/validation/role.js

@@ -7,7 +7,7 @@ const handleReject = error => Promise.reject(formatYupErrors(error));
 const roleUpdateSchema = yup
   .object()
   .shape({
-    description: yup.string(),
+    description: yup.string().nullable(),
   })
   .noUnknown();
 

packages/strapi-admin/validation/user.js

@@ -22,11 +22,11 @@ const validateUserCreationInput = data => {
 const profileUpdateSchema = yup
   .object()
   .shape({
-    email: validators.email,
-    firstname: validators.firstname,
-    lastname: validators.lastname,
+    email: validators.email.notNull(),
+    firstname: validators.firstname.notNull(),
+    lastname: validators.lastname.notNull(),
     username: validators.username.nullable(),
-    password: validators.password,
+    password: validators.password.notNull(),
   })
   .noUnknown();
 
@@ -39,13 +39,13 @@ const validateProfileUpdateInput = data => {
 const userUpdateSchema = yup
   .object()
   .shape({
-    email: validators.email,
-    firstname: validators.firstname,
-    lastname: validators.lastname,
+    email: validators.email.notNull(),
+    firstname: validators.firstname.notNull(),
+    lastname: validators.lastname.notNull(),
     username: validators.username.nullable(),
-    password: validators.password,
-    isActive: yup.bool(),
-    roles: validators.roles.min(1),
+    password: validators.password.notNull(),
+    isActive: yup.bool().notNull(),
+    roles: validators.roles.min(1).notNull(),
   })
   .noUnknown();
 

packages/strapi/lib/middlewares/boom/index.js

@@ -129,7 +129,19 @@ module.exports = strapi => {
         this.body = data;
       };
 
-      this.delegator.method('send').method('created');
+      strapi.app.response.deleted = function(data) {
+        if (_.isNil(data)) {
+          this.status = 204;
+        } else {
+          this.status = 200;
+          this.body = data;
+        }
+      };
+
+      this.delegator
+        .method('send')
+        .method('created')
+        .method('deleted');
     },
   };
 };

test/helpers/auth.js

@@ -40,7 +40,13 @@ module.exports = {
     await register();
 
     // login
-    const { token } = await login();
-    return token;
+    const user = await login();
+
+    return user && user.token;
+  },
+  async login() {
+    const user = await login();
+
+    return user && user.token;
   },
 };