Conversation
| /** | ||
| * Max original event ids per bulkToggleEventMark request | ||
| */ | ||
| static get BULK_TOGGLE_EVENT_MARK_MAX() { | ||
| return 100; | ||
| } |
| if (mark !== 'resolved' && mark !== 'ignored' && mark !== 'starred') { | ||
| throw new Error(`bulkToggleEventMark: mark must be resolved, ignored or starred, got ${mark}`); | ||
| } |
There was a problem hiding this comment.
validation should be on resolver level (or via graphql schema)
…or handling in bulkToggleEventMark and bulkUpdateAssignee methods
…ventMarks and clean up related tests
…e events as visited for a user
…hod for improved code reuse and clarity
There was a problem hiding this comment.
Pull request overview
Adds backend support and test coverage for multi-select bulk actions on events, including bulk visit, bulk mark toggle, and bulk assignee updates.
Changes:
- Introduces new GraphQL mutations/types for bulk visiting events, bulk toggling marks, and bulk updating assignees.
- Adds resolver/helper logic to validate/dedupe bulk IDs, merge invalid IDs into
failedEventIds, and enqueue assignee notifications asynchronously. - Implements
EventsFactorybulk operations and adds Jest tests for resolvers/helpers/factory methods.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/resolvers/event-bulk-visit.test.ts | Adds resolver tests for bulkVisitEvents validation + failed ID merging |
| test/resolvers/event-bulk-update-assignee.test.ts | Adds resolver tests for EventsMutations.bulkUpdateAssignee and notification enqueueing |
| test/resolvers/event-bulk-toggle-marks.test.ts | Adds resolver tests for bulkToggleEventMarks semantics + validation |
| test/resolvers/bulk-events-helper.test.ts | Adds helper tests for fire-and-forget notification enqueueing |
| test/models/eventsFactory-bulk-visit.test.ts | Adds model tests for EventsFactory.bulkVisitEvent |
| test/models/eventsFactory-bulk-update-assignee.test.ts | Adds model tests for EventsFactory.bulkUpdateAssignee |
| test/models/eventsFactory-bulk-toggle.test.ts | Adds model tests for EventsFactory.bulkToggleEventMark |
| src/typeDefs/event.ts | Adds new GraphQL inputs/types/mutations for bulk operations |
| src/resolvers/helpers/bulkEvents.js | Adds bulk ID parsing/merging helpers + async notification enqueue helper |
| src/resolvers/event.js | Wires new bulk mutations and switches assignee notification sending to fire-and-forget |
| src/models/eventsFactory.js | Implements bulk visit, bulk mark toggle, bulk assignee update, and shared resolver for bulk IDs |
| package.json | Bumps package version |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| """ | ||
| Toggle the same mark on many original events at once (resolved, ignored or starred). | ||
| Same toggle semantics as toggleEventMark per event. |
There was a problem hiding this comment.
The mutation comment says “Same toggle semantics as toggleEventMark per event”, but this bulk operation has different semantics (it only clears when all selected already have the mark). Please adjust the docstring so clients don’t assume per-event toggling behavior.
| Same toggle semantics as toggleEventMark per event. | |
| Uses bulk semantics: if every selected event already has the mark, clear it for all; | |
| otherwise set it on each selected event that does not have it yet. |
| type BulkToggleEventMarksResult { | ||
| """ | ||
| Number of events updated in the database | ||
| """ | ||
| updatedCount: Int! | ||
|
|
||
| """ | ||
| Original event ids actually toggled in this operation | ||
| """ | ||
| updatedEventIds: [ID!]! | ||
|
|
||
| """ | ||
| Event ids that were not updated (invalid id or not found) | ||
| """ | ||
| failedEventIds: [ID!]! | ||
| } | ||
|
|
||
| """ | ||
| Result of bulk marking events as viewed | ||
| """ | ||
| type BulkVisitEventsResult { | ||
| """ |
There was a problem hiding this comment.
Type naming is inconsistent: the file predominantly uses *Response (e.g., UpdateAssigneeResponse, BulkUpdateAssigneeResponse), but these new types use *Result. Consider renaming them to BulkToggleEventMarksResponse / BulkVisitEventsResponse (or switching all bulk types to *Result) to keep the public GraphQL schema consistent.
| Promise.allSettled(eventIds.map(eventId => sendPersonalNotification(assigneeData, { | ||
| type: 'assignee', | ||
| payload: { | ||
| assigneeId, | ||
| projectId, | ||
| whoAssignedId, | ||
| eventId, | ||
| }, | ||
| }))) |
There was a problem hiding this comment.
Promise.allSettled(eventIds.map(...)) will fire all notification enqueues at once. With large bulk selections this can create an unbounded burst of RabbitMQ enqueue calls and memory pressure. Consider batching/chunking or applying a concurrency limit (e.g., a small pool) while still keeping the resolver non-blocking.
| .then((results) => { | ||
| const failedResults = results.filter(result => result.status === 'rejected'); | ||
|
|
||
| if (failedResults.length > 0) { | ||
| console.error('Failed to enqueue assignee notifications', failedResults); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| console.error('Failed to enqueue assignee notifications', error); | ||
| }); |
There was a problem hiding this comment.
failedResults may contain raw rejection reasons from enqueue (potentially including endpoints or other sensitive details). Logging the full array could leak data into logs; consider logging only counts + sanitized error messages, or at least logging reason.message without payload details.
| if (assignee) { | ||
| const userExists = await factories.usersFactory.findById(assignee); | ||
|
|
||
| if (!userExists) { | ||
| throw new UserInputError('assignee not found'); | ||
| } |
There was a problem hiding this comment.
assignee is only checked for truthiness. If the client passes an invalid ID (or an empty string), usersFactory.findById() will throw from new ObjectId(id) inside the dataloader instead of returning a controlled UserInputError. Validate assignee explicitly (e.g., ObjectId.isValid) and treat empty string as invalid since the API contract says “pass null to clear”.
| } | ||
|
|
||
| """ | ||
| Result of bulk toggling event marks (resolve / ignore) |
There was a problem hiding this comment.
In the schema/docs, this says bulk toggle marks is for “resolve / ignore”, but the mutation and factory also support starred. Update the description to include starred to avoid misleading API consumers.
| Result of bulk toggling event marks (resolve / ignore) | |
| Result of bulk toggling event marks (resolve / ignore / starred) |
| * @param {string|ObjectId} userId - id of the user who is visiting events | ||
| * @returns {Promise<{ updatedCount: number, updatedEventIds: string[], failedEventIds: string[] }>} | ||
| */ | ||
| async bulkVisitEvent(eventIds, userId) { |
There was a problem hiding this comment.
| async bulkVisitEvent(eventIds, userId) { | |
| async bulkVisitEvents(eventIds, userId) { |
Summary
Introduces multi-select on the project daily events list with a bulk action bar