refactor(events): streamline bulk event ID validation and enhance error handling in bulkToggleEventMark and bulkUpdateAssignee methods

Dobrunia · Dobrunia · commit dd84f03da8a0 · 2026-04-22T20:39:26.000+03:00
diff --git a/src/models/eventsFactory.js b/src/models/eventsFactory.js
@@ -919,37 +919,16 @@ class EventsFactory extends Factory {
   }
 
   /**
-   * Bulk mark for resolved / ignored / starred (not the same as per-event toggleEventMark).
-   * - If every found event already has the mark: remove it from all (bulk "undo").
-   * - Otherwise: set the mark on every found event that does not have it yet (never remove
-   *   from a subset when the selection is mixed).
-   * Only 'resolved', 'ignored' and 'starred' are allowed for bulk.
+   * Bulk toggle mark for original events.
    *
    * @param {string[]} eventIds - original event ids
    * @param {string} mark - 'resolved' | 'ignored' | 'starred'
    * @returns {Promise<{ updatedCount: number, updatedEventIds: string[], failedEventIds: string[] }>}
    */
   async bulkToggleEventMark(eventIds, mark) {
     const unique = [ ...new Set((eventIds || []).map(id => String(id))) ];
-
     const failedEventIds = [];
-    const validObjectIds = [];
-
-    for (const id of unique) {
-      if (!ObjectId.isValid(id)) {
-        failedEventIds.push(id);
-      } else {
-        validObjectIds.push(new ObjectId(id));
-      }
-    }
-
-    if (validObjectIds.length === 0) {
-      return {
-        updatedCount: 0,
-        updatedEventIds: [],
-        failedEventIds,
-      };
-    }
+    const validObjectIds = unique.map(id => new ObjectId(id));
 
     const collection = this.getCollection(this.TYPES.EVENTS);
     const found = await collection.find({ _id: { $in: validObjectIds } }).toArray();
@@ -1017,23 +996,7 @@ class EventsFactory extends Factory {
   async bulkUpdateAssignee(eventIds, assignee) {
     const unique = [ ...new Set((eventIds || []).map(id => String(id))) ];
     const failedEventIds = [];
-    const validObjectIds = [];
-
-    for (const id of unique) {
-      if (!ObjectId.isValid(id)) {
-        failedEventIds.push(id);
-      } else {
-        validObjectIds.push(new ObjectId(id));
-      }
-    }
-
-    if (validObjectIds.length === 0) {
-      return {
-        updatedCount: 0,
-        updatedEventIds: [],
-        failedEventIds,
-      };
-    }
+    const validObjectIds = unique.map(id => new ObjectId(id));
 
     const collection = this.getCollection(this.TYPES.EVENTS);
     const found = await collection.find({ _id: { $in: validObjectIds } }).toArray();
diff --git a/src/resolvers/event.js b/src/resolvers/event.js
@@ -1,39 +1,12 @@
 const getEventsFactory = require('./helpers/eventsFactory').default;
-const sendPersonalNotification = require('../utils/personalNotifications').default;
+const {
+  fireAndForgetAssigneeNotifications,
+  parseBulkEventIds,
+  mergeFailedEventIds,
+} = require('./helpers/bulkEvents');
 const { aiService } = require('../services/ai');
 const { UserInputError } = require('apollo-server-express');
 
-/**
- * Enqueue assignee notifications in background (do not block resolver response)
- *
- * @param {object} args - notification args
- * @param {object} args.assigneeData - assigned user data
- * @param {string[]} args.eventIds - original event ids
- * @param {string} args.projectId - project id
- * @param {string} args.assigneeId - assignee id
- * @param {string} args.whoAssignedId - user id who performed assignment
- * @returns {void}
- */
-function fireAndForgetAssigneeNotifications({
-  assigneeData,
-  eventIds,
-  projectId,
-  assigneeId,
-  whoAssignedId,
-}) {
-  void Promise.allSettled(eventIds.map(eventId => sendPersonalNotification(assigneeData, {
-    type: 'assignee',
-    payload: {
-      assigneeId,
-      projectId,
-      whoAssignedId,
-      eventId,
-    },
-  }))).catch((error) => {
-    console.error('Failed to enqueue assignee notifications', error);
-  });
-}
-
 /**
  * See all types and fields here {@see ../typeDefs/event.graphql}
  */
@@ -201,13 +174,23 @@ module.exports = {
         throw new UserInputError('bulkToggleEventMarks supports only resolved, ignored and starred marks');
       }
 
-      if (!eventIds || !eventIds.length) {
-        throw new UserInputError('eventIds must contain at least one id');
+      const { validEventIds, invalidEventIds } = parseBulkEventIds(eventIds);
+
+      if (validEventIds.length === 0) {
+        return {
+          updatedCount: 0,
+          updatedEventIds: [],
+          failedEventIds: invalidEventIds,
+        };
       }
 
       const factory = getEventsFactory(context, projectId);
+      const result = await factory.bulkToggleEventMark(validEventIds, mark);
 
-      return factory.bulkToggleEventMark(eventIds, mark);
+      return {
+        ...result,
+        failedEventIds: mergeFailedEventIds(result, invalidEventIds),
+      };
     },
 
     /**
@@ -296,12 +279,18 @@ module.exports = {
      */
     async bulkUpdateAssignee(_obj, { input }, { factories, user, ...context }) {
       const { projectId, eventIds, assignee } = input;
-      const factory = getEventsFactory(context, projectId);
+      const { validEventIds, invalidEventIds } = parseBulkEventIds(eventIds);
 
-      if (!eventIds || !eventIds.length) {
-        throw new UserInputError('eventIds must contain at least one id');
+      if (validEventIds.length === 0) {
+        return {
+          updatedCount: 0,
+          updatedEventIds: [],
+          failedEventIds: invalidEventIds,
+        };
       }
 
+      const factory = getEventsFactory(context, projectId);
+
       if (assignee) {
         const userExists = await factories.usersFactory.findById(assignee);
 
@@ -318,14 +307,18 @@ module.exports = {
         }
       }
 
-      const result = await factory.bulkUpdateAssignee(eventIds, assignee);
+      const result = await factory.bulkUpdateAssignee(validEventIds, assignee);
+      const resultWithInvalid = {
+        ...result,
+        failedEventIds: mergeFailedEventIds(result, invalidEventIds),
+      };
 
-      if (assignee && result.updatedEventIds.length > 0) {
+      if (assignee && resultWithInvalid.updatedEventIds.length > 0) {
         void factories.usersFactory.dataLoaders.userById.load(assignee)
           .then((assigneeData) => {
             fireAndForgetAssigneeNotifications({
               assigneeData,
-              eventIds: result.updatedEventIds,
+              eventIds: resultWithInvalid.updatedEventIds,
               projectId,
               assigneeId: assignee,
               whoAssignedId: user.id,
@@ -336,7 +329,7 @@ module.exports = {
           });
       }
 
-      return result;
+      return resultWithInvalid;
     },
   },
 };
diff --git a/src/resolvers/helpers/bulkEvents.js b/src/resolvers/helpers/bulkEvents.js
@@ -0,0 +1,80 @@
+const sendPersonalNotification = require('../../utils/personalNotifications').default;
+const { UserInputError } = require('apollo-server-express');
+const { ObjectId } = require('mongodb');
+
+/**
+ * Enqueue assignee notifications in background (do not block resolver response)
+ *
+ * @param {object} args - notification args
+ * @param {object} args.assigneeData - assigned user data
+ * @param {string[]} args.eventIds - original event ids
+ * @param {string} args.projectId - project id
+ * @param {string} args.assigneeId - assignee id
+ * @param {string} args.whoAssignedId - user id who performed assignment
+ * @returns {void}
+ */
+function fireAndForgetAssigneeNotifications({
+  assigneeData,
+  eventIds,
+  projectId,
+  assigneeId,
+  whoAssignedId,
+}) {
+  void Promise.allSettled(eventIds.map(eventId => sendPersonalNotification(assigneeData, {
+    type: 'assignee',
+    payload: {
+      assigneeId,
+      projectId,
+      whoAssignedId,
+      eventId,
+    },
+  }))).catch((error) => {
+    console.error('Failed to enqueue assignee notifications', error);
+  });
+}
+
+/**
+ * Validate and normalize bulk event ids from resolver input.
+ *
+ * @param {string[]} eventIds - raw event ids from GraphQL input
+ * @returns {{ validEventIds: string[], invalidEventIds: string[] }}
+ */
+function parseBulkEventIds(eventIds) {
+  if (!eventIds || !eventIds.length) {
+    throw new UserInputError('eventIds must contain at least one id');
+  }
+
+  const uniqueEventIds = [ ...new Set(eventIds.map(id => String(id))) ];
+  const invalidEventIds = [];
+  const validEventIds = [];
+
+  uniqueEventIds.forEach((id) => {
+    if (ObjectId.isValid(id)) {
+      validEventIds.push(id);
+    } else {
+      invalidEventIds.push(id);
+    }
+  });
+
+  return {
+    validEventIds,
+    invalidEventIds,
+  };
+}
+
+/**
+ * Merge failed ids returned by factory with invalid ids from resolver validation.
+ *
+ * @param {{ failedEventIds?: string[] }} result - factory response
+ * @param {string[]} invalidEventIds - invalid ids detected on resolver level
+ * @returns {string[]}
+ */
+function mergeFailedEventIds(result, invalidEventIds) {
+  return [ ...new Set([ ...(result.failedEventIds || []), ...invalidEventIds ]) ];
+}
+
+module.exports = {
+  fireAndForgetAssigneeNotifications,
+  parseBulkEventIds,
+  mergeFailedEventIds,
+};
diff --git a/test/models/eventsFactory-bulk-toggle.test.ts b/test/models/eventsFactory-bulk-toggle.test.ts
@@ -107,17 +107,6 @@ describe('EventsFactory.bulkToggleEventMark', () => {
     expect(ops).toHaveLength(1);
   });
 
-  it('should return failedEventIds for invalid ObjectIds and skip bulkWrite', async () => {
-    const factory = new EventsFactory(projectId);
-
-    const result = await factory.bulkToggleEventMark([ 'not-a-valid-id' ], 'resolved');
-
-    expect(result.updatedCount).toBe(0);
-    expect(result.updatedEventIds).toEqual([]);
-    expect(result.failedEventIds).toContain('not-a-valid-id');
-    expect(collectionMock.bulkWrite).not.toHaveBeenCalled();
-  });
-
   it('should list valid but missing document ids in failedEventIds', async () => {
     const factory = new EventsFactory(projectId);
     const missing = new ObjectId();
diff --git a/test/models/eventsFactory-bulk-update-assignee.test.ts b/test/models/eventsFactory-bulk-update-assignee.test.ts
@@ -43,16 +43,6 @@ describe('EventsFactory.bulkUpdateAssignee', () => {
     collectionMock.updateMany.mockResolvedValue({ modifiedCount: 0 });
   });
 
-  it('should return failed ids for invalid ObjectIds and skip updateMany', async () => {
-    const factory = new EventsFactory(projectId);
-    const result = await factory.bulkUpdateAssignee([ 'bad-id' ], 'user-1');
-
-    expect(result.updatedCount).toBe(0);
-    expect(result.updatedEventIds).toEqual([]);
-    expect(result.failedEventIds).toEqual([ 'bad-id' ]);
-    expect(collectionMock.updateMany).not.toHaveBeenCalled();
-  });
-
   it('should update only events with changed assignee', async () => {
     const factory = new EventsFactory(projectId);
     const a = new ObjectId();
diff --git a/test/resolvers/event-bulk-toggle-marks.test.ts b/test/resolvers/event-bulk-toggle-marks.test.ts
@@ -108,4 +108,51 @@ describe('Mutation.bulkToggleEventMarks', () => {
     expect(result).toEqual(payload);
   });
 
+  it('should validate ids on resolver level and merge invalid ids into failedEventIds', async () => {
+    bulkToggleEventMark.mockResolvedValue({
+      updatedCount: 1,
+      updatedEventIds: [ '507f1f77bcf86cd799439011' ],
+      failedEventIds: [ '507f1f77bcf86cd799439099' ],
+    });
+
+    const result = await eventResolvers.Mutation.bulkToggleEventMarks(
+      {},
+      {
+        projectId: 'p1',
+        eventIds: [ '507f1f77bcf86cd799439011', 'invalid-id' ],
+        mark: 'ignored',
+      },
+      ctx
+    );
+
+    expect(bulkToggleEventMark).toHaveBeenCalledWith(
+      [ '507f1f77bcf86cd799439011' ],
+      'ignored'
+    );
+    expect(result).toEqual({
+      updatedCount: 1,
+      updatedEventIds: [ '507f1f77bcf86cd799439011' ],
+      failedEventIds: [ '507f1f77bcf86cd799439099', 'invalid-id' ],
+    });
+  });
+
+  it('should return early when all ids are invalid', async () => {
+    const result = await eventResolvers.Mutation.bulkToggleEventMarks(
+      {},
+      {
+        projectId: 'p1',
+        eventIds: [ 'bad-1', 'bad-2' ],
+        mark: 'ignored',
+      },
+      ctx
+    );
+
+    expect(bulkToggleEventMark).not.toHaveBeenCalled();
+    expect(result).toEqual({
+      updatedCount: 0,
+      updatedEventIds: [],
+      failedEventIds: [ 'bad-1', 'bad-2' ],
+    });
+  });
+
 });
diff --git a/test/resolvers/event-bulk-update-assignee.test.ts b/test/resolvers/event-bulk-update-assignee.test.ts
