Coffee Chat: Remove lodash isEqual, Add Permissions Checks to createC…

…offeeChat, Make Smarter DB Queries (#652)

* Coffee Chat: Remove lodash isEqual, Add Permissions Checks to createCoffeeChat, Make Smarter DB Queries

* use PermissionError

backend/src/API/coffeeChatAPI.ts

@@ -1,4 +1,3 @@
-import isEqual from 'lodash.isequal';
 import CoffeeChatDao from '../dao/CoffeeChatDao';
 import PermissionsManager from '../utils/permissionsManager';
 import { PermissionError } from '../utils/errors';
@@ -20,16 +19,30 @@ export const getCoffeeChatsByUser = async (user: IdolMember): Promise<CoffeeChat
 /**
  * Creates a new coffee chat for member
  * @param coffeeChat - Newly created CoffeeChat object
+ * @param user - user who is submitting the coffee chat
  * A member can not coffee chat themselves.
  * A member can not coffee chat the same person from previous semesters.
  */
-export const createCoffeeChat = async (coffeeChat: CoffeeChat): Promise<CoffeeChat> => {
-  if (isEqual(coffeeChat.submitter, coffeeChat.otherMember)) {
+export const createCoffeeChat = async (
+  coffeeChat: CoffeeChat,
+  user: IdolMember
+): Promise<CoffeeChat> => {
+  const isLeadOrAdmin = await PermissionsManager.isLeadOrAdmin(user);
+  if (!isLeadOrAdmin && coffeeChat.submitter.email !== user.email) {
+    throw new PermissionError(
+      `User with email ${user.email} does not have permissions to create coffee chat.`
+    );
+  }
+
+  if (coffeeChat.submitter.email === coffeeChat.otherMember.email) {
     throw new Error('Cannot create coffee chat with yourself.');
   }
 
-  const prevChats = await coffeeChatDao.getCoffeeChatsByUser(coffeeChat.submitter);
-  const chatExists = prevChats.some((chat) => isEqual(coffeeChat.otherMember, chat.otherMember));
+  const prevChats = await coffeeChatDao.getCoffeeChatsByUser(
+    coffeeChat.submitter,
+    coffeeChat.otherMember
+  );
+  const chatExists = prevChats.length > 0;
 
   if (chatExists) {
     throw new Error(

backend/src/api.ts

@@ -289,8 +289,8 @@ loginCheckedGet('/coffee-chat', async () => ({
   coffeeChats: await getAllCoffeeChats()
 }));
 
-loginCheckedPost('/coffee-chat', async (req) => ({
-  coffeeChats: await createCoffeeChat(req.body)
+loginCheckedPost('/coffee-chat', async (req, user) => ({
+  coffeeChats: await createCoffeeChat(req.body, user)
 }));
 
 loginCheckedDelete('/coffee-chat', async (_, user) => {

backend/src/dao/BaseDao.ts

@@ -6,7 +6,7 @@ import { firestore } from 'firebase-admin';
  * comparisonOperator -- the comparison operator to use for comparison/filtering (https://cloud.google.com/firestore/docs/query-data/queries)
  * value -- the value to compare the field to
  */
-interface FirestoreFilter {
+export interface FirestoreFilter {
   field: string;
   comparisonOperator: FirebaseFirestore.WhereFilterOp;
   // eslint-disable-next-line @typescript-eslint/no-explicit-any

backend/src/dao/CoffeeChatDao.ts

@@ -2,7 +2,7 @@ import { v4 as uuidv4 } from 'uuid';
 import { memberCollection, coffeeChatsCollection, db } from '../firebase';
 import { DBCoffeeChat } from '../types/DataTypes';
 import { getMemberFromDocumentReference } from '../utils/memberUtil';
-import BaseDao from './BaseDao';
+import BaseDao, { FirestoreFilter } from './BaseDao';
 import { deleteCollection } from '../utils/firebase-utils';
 
 async function materializeCoffeeChat(dbCoffeeChat: DBCoffeeChat): Promise<CoffeeChat> {
@@ -72,16 +72,30 @@ export default class CoffeeChatDao extends BaseDao<CoffeeChat, DBCoffeeChat> {
 
   /**
    * Gets all coffee chat that a user has submitted
-   * @param user - user whose coffee chats should be fetched
+   * @param submitter - submitter whose coffee chats should be fetched
+   * @param otherMember - additional filter for coffee chats with otherMember (optional)
    */
-  async getCoffeeChatsByUser(user: IdolMember): Promise<CoffeeChat[]> {
-    return this.getDocuments([
+  async getCoffeeChatsByUser(
+    submitter: IdolMember,
+    otherMember?: IdolMember
+  ): Promise<CoffeeChat[]> {
+    const filters: FirestoreFilter[] = [
       {
         field: 'submitter',
         comparisonOperator: '==',
-        value: memberCollection.doc(user.email)
+        value: memberCollection.doc(submitter.email)
       }
-    ]);
+    ];
+
+    if (otherMember) {
+      filters.push({
+        field: 'otherMember',
+        comparisonOperator: '==',
+        value: memberCollection.doc(otherMember.email)
+      });
+    }
+
+    return this.getDocuments(filters);
   }
 
   /**

backend/tests/CoffeeChatAPI.test.ts

@@ -1,7 +1,6 @@
-import isEqual from 'lodash.isequal';
 import CoffeeChatDao from '../src/dao/CoffeeChatDao'; // eslint-disable-line @typescript-eslint/no-unused-vars
 import PermissionsManager from '../src/utils/permissionsManager';
-import { fakeIdolMember, fakeCoffeeChat } from './data/createData';
+import { fakeIdolMember, fakeCoffeeChat, fakeIdolLead } from './data/createData';
 import {
   getAllCoffeeChats,
   getCoffeeChatsByUser,
@@ -28,17 +27,26 @@ describe('User is not lead or admin', () => {
   const user = fakeIdolMember();
   const user2 = fakeIdolMember();
   const coffeeChat = { ...fakeCoffeeChat(), submitter: user, otherMember: user2 };
-  createCoffeeChat(coffeeChat);
+  createCoffeeChat(coffeeChat, user);
+
+  test('createCoffeeChat should throw error if submitter does not match person making request', async () => {
+    const chat = { ...coffeeChat, submitter: user2, otherMember: user };
+    await expect(createCoffeeChat(chat, user)).rejects.toThrow(
+      new PermissionError(
+        `User with email ${user.email} does not have permissions to create coffee chat.`
+      )
+    );
+  });
 
   test('createCoffeeChat should throw error if creating a coffee chat with self', async () => {
     const selfChat = { ...coffeeChat, submitter: user, otherMember: user };
-    await expect(createCoffeeChat(selfChat)).rejects.toThrow(
+    await expect(createCoffeeChat(selfChat, user)).rejects.toThrow(
       new Error('Cannot create coffee chat with yourself.')
     );
   });
 
   test('createCoffeeChat should throw error if previous chats exist', async () => {
-    await expect(createCoffeeChat(coffeeChat)).rejects.toThrow(
+    await expect(createCoffeeChat(coffeeChat, user)).rejects.toThrow(
       new Error(
         'Cannot create coffee chat with member. Previous coffee chats from previous semesters exist.'
       )
@@ -74,12 +82,16 @@ describe('User is not lead or admin', () => {
 });
 
 describe('User is lead or admin', () => {
-  const user = { ...fakeIdolMember(), role: 'lead' };
+  const adminUser = fakeIdolLead();
+  const submitter = fakeIdolMember();
+  const otherMember = fakeIdolMember();
 
   beforeAll(() => {
     const mockIsLeadOrAdmin = jest.fn().mockResolvedValue(true);
     const mockGetAllCoffeeChats = jest.fn().mockResolvedValue([fakeCoffeeChat()]);
-    const mockGetCoffeeChatsByUser = jest.fn().mockResolvedValue([fakeCoffeeChat()]);
+    const mockGetCoffeeChatsByUser = jest
+      .fn()
+      .mockImplementation((submitter, otherMember) => (!otherMember ? [fakeCoffeeChat()] : []));
     const mockCreateCoffeeChat = jest.fn().mockResolvedValue(fakeCoffeeChat());
     const mockUpdateCoffeeChat = jest.fn().mockResolvedValue(fakeCoffeeChat());
     const mockDeleteCoffeeChat = jest.fn().mockResolvedValue(undefined);
@@ -96,54 +108,48 @@ describe('User is lead or admin', () => {
     jest.clearAllMocks();
   });
 
+  test('createCoffeeChat should allow an admin user to create coffee chat for other user', async () => {
+    const newChat = { ...fakeCoffeeChat(), submitter, otherMember };
+    const result = await createCoffeeChat(newChat, adminUser);
+    expect(CoffeeChatDao.prototype.createCoffeeChat).toBeCalled();
+    expect(result.submitter).toEqual(newChat.submitter);
+    expect(result.otherMember).toEqual(newChat.otherMember);
+  });
+
   test('getAllCoffeeChats should return all coffee chats', async () => {
     const coffeeChats = await getAllCoffeeChats();
     expect(coffeeChats.length).toBeGreaterThan(0);
     expect(CoffeeChatDao.prototype.getAllCoffeeChats).toBeCalled();
   });
 
   test('getCoffeeChatsByUser should return user coffee chats', async () => {
-    const coffeeChats = await getCoffeeChatsByUser(user);
+    const coffeeChats = await getCoffeeChatsByUser(adminUser);
     expect(coffeeChats.length).toBeGreaterThan(0);
     expect(CoffeeChatDao.prototype.getCoffeeChatsByUser).toBeCalled();
   });
 
   test('createCoffeeChat should successfully create a coffee chat', async () => {
     const coffeeChat = fakeCoffeeChat();
-    const newChat = { ...coffeeChat, submitter: user, otherMember: fakeIdolMember() };
-    const result = await createCoffeeChat(newChat);
+    const newChat = { ...coffeeChat, submitter: adminUser, otherMember: fakeIdolMember() };
+    const result = await createCoffeeChat(newChat, adminUser);
 
     expect(CoffeeChatDao.prototype.createCoffeeChat).toBeCalled();
     expect(result.submitter).toEqual(newChat.submitter);
     expect(result.otherMember).toEqual(newChat.otherMember);
   });
 
   test('updateCoffeeChat should successfully update a coffee chat', async () => {
-    const updatedChat = await updateCoffeeChat(fakeCoffeeChat(), user);
+    const updatedChat = await updateCoffeeChat(fakeCoffeeChat(), adminUser);
     expect(CoffeeChatDao.prototype.updateCoffeeChat).toBeCalled();
     expect(updatedChat).toBeDefined();
   });
 
   test('deleteCoffeeChat should successfully delete a coffee chat', async () => {
     const coffeeChat = fakeCoffeeChat();
-    const newChat = { ...coffeeChat, submitter: user, otherMember: fakeIdolMember() };
-    await createCoffeeChat(newChat);
+    const newChat = { ...coffeeChat, submitter: adminUser, otherMember: fakeIdolMember() };
+    await createCoffeeChat(newChat, adminUser);
 
-    await deleteCoffeeChat(newChat.uuid, user);
+    await deleteCoffeeChat(newChat.uuid, adminUser);
     expect(CoffeeChatDao.prototype.deleteCoffeeChat).toBeCalled();
   });
 });
-
-describe('isEqual function usage', () => {
-  test('isEqual should correctly identify different members', () => {
-    const member1 = fakeIdolMember();
-    const member2 = fakeIdolMember();
-    expect(isEqual(member1, member2)).toBe(false);
-  });
-
-  test('isEqual should correctly identify same members', () => {
-    const member1 = fakeIdolMember();
-    const member2 = { ...member1 };
-    expect(isEqual(member1, member2)).toBe(true);
-  });
-});

backend/tests/data/createData.ts

@@ -29,9 +29,8 @@ const fakeSubteams = (): string[] => {
 };
 
 const fakeRoleObject = () => {
-  const roles: Role[] = ['lead', 'tpm', 'pm', 'developer', 'designer', 'business'];
+  const roles: Role[] = ['tpm', 'pm', 'developer', 'designer', 'business'];
   const role_descriptions: RoleDescription[] = [
-    'Lead',
     'Technical PM',
     'Product Manager',
     'Developer',
@@ -63,6 +62,16 @@ export const fakeIdolMember = (): IdolMember => {
   return member;
 };
 
+/** Create fake non-admin */
+export const fakeIdolLead = (): IdolMember => {
+  const member = {
+    ...fakeIdolMember(),
+    role: 'Lead',
+    roleDescription: 'lead'
+  };
+  return member;
+};
+
 /** Create a fake TeamEventAttendace object. */
 export const fakeTeamEventAttendance = (): TeamEventAttendance => {
   const TEA = {