-
Notifications
You must be signed in to change notification settings - Fork 11.6k
feat(api-v2): add event-type level selected calendars for conflict checking #26730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(api-v2): add event-type level selected calendars for conflict checking #26730
Conversation
…ecking - Add SelectedCalendar_2024_06_14 input DTO for selected calendars - Add SelectedCalendarOutput_2024_06_14 output DTO for selected calendars - Add useEventLevelSelectedCalendars and selectedCalendars fields to create/update event-type inputs - Add useEventLevelSelectedCalendars and selectedCalendars fields to event-type output (non-public only) - Add repository methods for event-type selected calendars operations - Add service method to handle selected calendars CRUD operations - Exclude selectedCalendars from public event-type endpoints output Co-Authored-By: [email protected] <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
E2E results are ready! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
12 issues found across 16 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/input-event-types.service.ts">
<violation number="1" location="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/input-event-types.service.ts:137">
P1: Missing validation for `selectedCalendars` input. The field is accepted but never validated to ensure the calendars exist and are accessible by the user. Add a validation method similar to `validateInputDestinationCalendar` that checks:
1. Each calendar's integration is valid
2. The externalId exists and belongs to the user
3. The user has the necessary permissions</violation>
<violation number="2" location="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/input-event-types.service.ts:137">
P1: Incomplete implementation: selectedCalendars fields are accepted but silently ignored. The fields are destructured in the transformation service but never used, and the controller doesn't handle them separately. Users passing these fields will experience silent data loss.</violation>
</file>
<file name="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts">
<violation number="1" location="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts:393">
P0: Missing authorization check allows any user to modify another user's event type selected calendars. Add ownership verification before performing updates.</violation>
<violation number="2" location="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts:406">
P1: Multiple related database operations (update flag, delete calendars, create new calendars) are not wrapped in a transaction. If any operation fails, data will be in an inconsistent state. Wrap all operations in a database transaction to ensure atomicity.</violation>
<violation number="3" location="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts:417">
P2: O(n*m) complexity: loop performs linear search on userSelectedCalendars for each calendar. Create a Map for O(1) lookups instead.</violation>
<violation number="4" location="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts:422">
P2: N+1 query problem: sequential awaits in loop. Consider bulk insert operation or Promise.all for better performance.</violation>
<violation number="5" location="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts:434">
P1: Method assigns ownerId from parameter without verifying the user actually owns the event type. This could expose data to unauthorized users if the caller doesn't perform authorization checks.</violation>
</file>
<file name="apps/api/v2/src/modules/selected-calendars/selected-calendars.repository.ts">
<violation number="1" location="apps/api/v2/src/modules/selected-calendars/selected-calendars.repository.ts:133">
P2: Use `select` to specify only the needed fields instead of returning all columns from selectedCalendar table. This prevents unnecessary data exposure (especially credentialId) and improves performance by reducing data transfer volume.</violation>
<violation number="2" location="apps/api/v2/src/modules/selected-calendars/selected-calendars.repository.ts:141">
P2: Add explicit `select` clause to return only required fields after creation instead of returning all columns.
(Based on your team's feedback about selecting only required fields to avoid fetching unnecessary columns.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts">
<violation number="1" location="apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts:193">
P1: Replace `include` with `select` to explicitly specify only required fields from related tables. Using `include: true` fetches all fields from related tables, causing performance overhead and potentially exposing sensitive data.</violation>
</file>
<file name="apps/api/v2/src/ee/event-types/event-types_2024_06_14/controllers/event-types.controller.ts">
<violation number="1" location="apps/api/v2/src/ee/event-types/event-types_2024_06_14/controllers/event-types.controller.ts:103">
P1: Operations are not atomic - if `updateEventTypeSelectedCalendars` fails after event type creation, the event type will exist without the requested selected calendars, leaving the system in an inconsistent state. Consider wrapping all three operations in a database transaction or handling partial failure by cleaning up the created event type.</violation>
<violation number="2" location="apps/api/v2/src/ee/event-types/event-types_2024_06_14/controllers/event-types.controller.ts:117">
P2: Silent failure handling with `??` operator may mask errors - if `getEventTypeWithSelectedCalendars` returns null due to a failure, the response omits selected calendars without indicating an error occurred. Consider explicit error handling or logging when the fallback is used.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts
Show resolved
Hide resolved
| disableRescheduling, | ||
| disableCancelling, | ||
| calVideoSettings, | ||
| useEventLevelSelectedCalendars: _useEventLevelSelectedCalendars, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Missing validation for selectedCalendars input. The field is accepted but never validated to ensure the calendars exist and are accessible by the user. Add a validation method similar to validateInputDestinationCalendar that checks:
- Each calendar's integration is valid
- The externalId exists and belongs to the user
- The user has the necessary permissions
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/input-event-types.service.ts, line 137:
<comment>Missing validation for `selectedCalendars` input. The field is accepted but never validated to ensure the calendars exist and are accessible by the user. Add a validation method similar to `validateInputDestinationCalendar` that checks:
1. Each calendar's integration is valid
2. The externalId exists and belongs to the user
3. The user has the necessary permissions</comment>
<file context>
@@ -134,6 +134,8 @@ export class InputEventTypesService_2024_06_14 {
disableRescheduling,
disableCancelling,
calVideoSettings,
+ useEventLevelSelectedCalendars: _useEventLevelSelectedCalendars,
+ selectedCalendars: _selectedCalendars,
...rest
</file context>
| const hasSelectedCalendars = selectedCalendars && selectedCalendars.length > 0; | ||
| const shouldUseEventLevelCalendars = useEventLevelSelectedCalendars ?? hasSelectedCalendars ?? false; | ||
|
|
||
| await this.eventTypesRepository.updateUseEventLevelSelectedCalendars( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Multiple related database operations (update flag, delete calendars, create new calendars) are not wrapped in a transaction. If any operation fails, data will be in an inconsistent state. Wrap all operations in a database transaction to ensure atomicity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts, line 406:
<comment>Multiple related database operations (update flag, delete calendars, create new calendars) are not wrapped in a transaction. If any operation fails, data will be in an inconsistent state. Wrap all operations in a database transaction to ensure atomicity.</comment>
<file context>
@@ -385,4 +389,58 @@ export class EventTypesService_2024_06_14 {
+ const hasSelectedCalendars = selectedCalendars && selectedCalendars.length > 0;
+ const shouldUseEventLevelCalendars = useEventLevelSelectedCalendars ?? hasSelectedCalendars ?? false;
+
+ await this.eventTypesRepository.updateUseEventLevelSelectedCalendars(
+ eventTypeId,
+ shouldUseEventLevelCalendars
</file context>
✅ Addressed in 152dff0
apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/input-event-types.service.ts
Outdated
Show resolved
Hide resolved
apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts
Show resolved
Hide resolved
| getByEventTypeId(eventTypeId: number) { | ||
| return this.dbRead.prisma.selectedCalendar.findMany({ | ||
| where: { | ||
| eventTypeId, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Use select to specify only the needed fields instead of returning all columns from selectedCalendar table. This prevents unnecessary data exposure (especially credentialId) and improves performance by reducing data transfer volume.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/v2/src/modules/selected-calendars/selected-calendars.repository.ts, line 133:
<comment>Use `select` to specify only the needed fields instead of returning all columns from selectedCalendar table. This prevents unnecessary data exposure (especially credentialId) and improves performance by reducing data transfer volume.</comment>
<file context>
@@ -129,4 +129,38 @@ export class SelectedCalendarsRepository {
});
}
+
+ getByEventTypeId(eventTypeId: number) {
+ return this.dbRead.prisma.selectedCalendar.findMany({
+ where: {
</file context>
Fix confidence (alpha): 8/10
| getByEventTypeId(eventTypeId: number) { | |
| return this.dbRead.prisma.selectedCalendar.findMany({ | |
| where: { | |
| eventTypeId, | |
| }, | |
| }); | |
| } | |
| getByEventTypeId(eventTypeId: number) { | |
| return this.dbRead.prisma.selectedCalendar.findMany({ | |
| where: { | |
| eventTypeId, | |
| }, | |
| select: { | |
| id: true, | |
| userId: true, | |
| integration: true, | |
| externalId: true, | |
| credentialId: true, | |
| eventTypeId: true, | |
| }, | |
| }); | |
| } |
✅ Addressed in 152dff0
apps/api/v2/src/modules/selected-calendars/selected-calendars.repository.ts
Show resolved
Hide resolved
| if (hasSelectedCalendars) { | ||
| const userSelectedCalendars = await this.selectedCalendarsRepository.getUserSelectedCalendars(userId); | ||
|
|
||
| for (const calendar of selectedCalendars) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: O(n*m) complexity: loop performs linear search on userSelectedCalendars for each calendar. Create a Map for O(1) lookups instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts, line 417:
<comment>O(n*m) complexity: loop performs linear search on userSelectedCalendars for each calendar. Create a Map for O(1) lookups instead.</comment>
<file context>
@@ -385,4 +389,58 @@ export class EventTypesService_2024_06_14 {
+ if (hasSelectedCalendars) {
+ const userSelectedCalendars = await this.selectedCalendarsRepository.getUserSelectedCalendars(userId);
+
+ for (const calendar of selectedCalendars) {
+ const matchingUserCalendar = userSelectedCalendars.find(
+ (uc) => uc.integration === calendar.integration && uc.externalId === calendar.externalId
</file context>
✅ Addressed in 152dff0
| (uc) => uc.integration === calendar.integration && uc.externalId === calendar.externalId | ||
| ); | ||
|
|
||
| await this.selectedCalendarsRepository.createForEventType( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: N+1 query problem: sequential awaits in loop. Consider bulk insert operation or Promise.all for better performance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts, line 422:
<comment>N+1 query problem: sequential awaits in loop. Consider bulk insert operation or Promise.all for better performance.</comment>
<file context>
@@ -385,4 +389,58 @@ export class EventTypesService_2024_06_14 {
+ (uc) => uc.integration === calendar.integration && uc.externalId === calendar.externalId
+ );
+
+ await this.selectedCalendarsRepository.createForEventType(
+ eventTypeId,
+ userId,
</file context>
✅ Addressed in 152dff0
apps/api/v2/src/ee/event-types/event-types_2024_06_14/controllers/event-types.controller.ts
Outdated
Show resolved
Hide resolved
Devin AI is addressing Cubic AI's review feedbackNew feedback has been sent to the existing Devin session. |
- Add authorization check in updateEventTypeSelectedCalendars - Add ownership verification in getEventTypeWithSelectedCalendars - Wrap database operations in transaction for atomicity - Optimize O(n*m) complexity with Map for O(1) lookups - Use bulk insert (createMany) instead of N+1 queries - Add select clause in repositories to return only needed fields Co-Authored-By: [email protected] <[email protected]>
…s://git-manager.devin.ai/proxy/github.com/calcom/cal.com into devin/event-type-selected-calendars-1768203692
- Add selectedCalendars to getEventTypeByIdWithHosts repository method - Update EventTypeRelations type to use partial SelectedCalendar type - Fix type compatibility for select clause optimization Co-Authored-By: [email protected] <[email protected]>
Devin AI is resolving merge conflictsThis PR has merge conflicts with the Devin will:
If you prefer to resolve conflicts manually, you can close the Devin session and handle it yourself. |
supalarry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review:
| @Type(() => SelectedCalendar_2024_06_14) | ||
| @DocsPropertyOptional({ | ||
| description: | ||
| "An array of calendars to be used for conflict checking for this event type. Only used when useEventLevelSelectedCalendars is true. Refer to the /api/v2/calendars endpoint to retrieve the integration type and external IDs of your connected calendars.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is used only when useEventLevelSelectedCalendars is true, then can't we get rid of useEventLevelSelectedCalendars and set it as true automatically if selectedCalendars are passed to the api to simplify API?
And then to switch back to checking calendars on user level developer submits selectedCalendars as empty array and then we set useEventLevelSelectedCalendars as false ourselves.
| getResponseEventTypeWithoutHiddenFields(eventType: EventTypeOutput_2024_06_14): EventTypeOutput_2024_06_14 { | ||
| if (!Array.isArray(eventType?.bookingFields) || eventType.bookingFields.length === 0) return eventType; | ||
| const { | ||
| selectedCalendars: _selectedCalendars, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For getResponseEventTypeWithoutHiddenFields - why are we excluding selected calendars from the response? The function filters out hidden booking fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public endpoint imo we should never return this field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
selectedCalendars are not bookingFields so I am not sure to understand fully
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see Devin did something strange with this function indeed
supalarry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review 2:
|
|
||
| const eventType = await this.eventTypesService.createUserEventType(user, transformedBody); | ||
|
|
||
| await this.eventTypesService.updateEventTypeSelectedCalendars( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This controller is getting too crowded. We should move updateEventTypeSelectedCalendars at the end of createUserEventType.
| disableRescheduling, | ||
| disableCancelling, | ||
| calVideoSettings, | ||
| useEventLevelSelectedCalendars: _useEventLevelSelectedCalendars, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We call await this.eventTypesService.updateEventTypeSelectedCalendars( in controller and then remove these properties here. If we move await this.eventTypesService.updateEventTypeSelectedCalendars( then we can keep them instead of discarding them.
- Simplify API by inferring useEventLevelSelectedCalendars from selectedCalendars - Move updateEventTypeSelectedCalendars logic from controller to service - Rename getResponseEventTypeWithoutHiddenFields to getResponseEventTypeForPublicEndpoint - Keep selectedCalendars and useEventLevelSelectedCalendars in transformed type Co-Authored-By: [email protected] <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts">
<violation number="1" location="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts:74">
P2: `destinationCalendar` from the create payload is no longer forwarded to `updateEventType`, so created event types silently ignore the requested destination calendar.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/event-types.service.ts
Show resolved
Hide resolved
Devin AI is addressing Cubic AI's review feedbackA Devin session has been created to address the issues identified by Cubic AI. |
…UserEventType Co-Authored-By: unknown <>
Since useEventLevelSelectedCalendars is now inferred from selectedCalendars, there's no need to have it in the API v2 input and output types. - Removed useEventLevelSelectedCalendars from CreateEventTypeInput_2024_06_14 - Removed useEventLevelSelectedCalendars from UpdateEventTypeInput_2024_06_14 - Removed useEventLevelSelectedCalendars from EventTypeOutput_2024_06_14 - Updated service methods to only use selectedCalendars parameter - Updated input/output transformation to not include useEventLevelSelectedCalendars - Updated API docs to explain that selectedCalendars controls the feature Co-Authored-By: [email protected] <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts">
<violation number="1" location="apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:475">
P2: `getResponseEventTypeForPublicEndpoint` no longer removes `useEventLevelSelectedCalendars`, so public event-type responses will leak that internal flag again. Remove the flag alongside `selectedCalendars` when shaping the public payload.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts
Outdated
Show resolved
Hide resolved
Devin AI is addressing Cubic AI's review feedbackA Devin session has been created to address the issues identified by Cubic AI. |
…nt response Address Cubic AI review feedback (confidence 9/10): The getResponseEventTypeForPublicEndpoint function was only removing selectedCalendars but not useEventLevelSelectedCalendars, which would leak the internal flag in public event-type responses. Co-Authored-By: unknown <>
…es from output service - Removed useEventLevelSelectedCalendars from Input type - Removed useEventLevelSelectedCalendars from getResponseEventType return object Co-Authored-By: [email protected] <[email protected]>
…s://git-manager.devin.ai/proxy/github.com/calcom/cal.com into devin/event-type-selected-calendars-1768203692
| } | ||
|
|
||
| getResponseEventTypesWithoutHiddenFields( | ||
| getResponseEventTypesForPublicEndpoint( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevinAI I don't understand why getResponseEventTypesWithoutHiddenFields was changed, this seems to not be related with the selected calendars change wanted in this pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right - the rename wasn't necessary for the selected calendars feature. I've reverted it back to getResponseEventTypesWithoutHiddenFields / getResponseEventTypeWithoutHiddenFields in commit 05aa421.
The function still removes selectedCalendars from public endpoint responses (which is the new behavior for this PR), but keeping the original name makes the diff cleaner and avoids unnecessary changes.
…s references Co-Authored-By: [email protected] <[email protected]>
…om public responses Co-Authored-By: [email protected] <[email protected]>
| await tx.eventType.update({ | ||
| where: { id: eventTypeId }, | ||
| data: { useEventLevelSelectedCalendars: shouldUseEventLevelCalendars }, | ||
| }); | ||
|
|
||
| if (selectedCalendars !== undefined) { | ||
| await tx.selectedCalendar.deleteMany({ | ||
| where: { eventTypeId }, | ||
| }); | ||
|
|
||
| if (calendarsToCreate.length > 0) { | ||
| await tx.selectedCalendar.createMany({ | ||
| data: calendarsToCreate, | ||
| }); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevinAI move the different prisma calls, so eventType.update tx.selectedCalendar.deleteMany and tx.selectedCalendar.createMany into the associated repositories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I've moved the Prisma calls from the service to the repositories:
- Added
updateSelectedCalendarsWithTransactionmethod toEventTypesRepository_2024_06_14- handles the transactional update ofuseEventLevelSelectedCalendarsflag and selected calendars - Added
createManyForEventTypemethod toSelectedCalendarsRepository- handles bulk creation of selected calendars - Updated
updateEventTypeSelectedCalendarsin the service to use the new repository method instead of direct Prisma calls - Removed
PrismaWriteServiceimport anddbWritedependency from the service
This follows the "no Prisma outside of repositories" pattern.
…s://git-manager.devin.ai/proxy/github.com/calcom/cal.com into devin/event-type-selected-calendars-1768203692
…epository methods with tx parameter Co-Authored-By: [email protected] <[email protected]>
| if (calendarsToCreate.length > 0) { | ||
| await this.eventTypesRepository.createManySelectedCalendarsWithTx(tx, calendarsToCreate); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated per your Slack feedback - I've corrected the refactoring:
- Transaction stays in the service -
this.dbWrite.prisma.$transaction()remains inupdateEventTypeSelectedCalendars - Individual Prisma calls moved to repository methods with
txparameter:updateUseEventLevelSelectedCalendarsWithTx(tx, eventTypeId, useEventLevelSelectedCalendars)deleteSelectedCalendarsByEventTypeIdWithTx(tx, eventTypeId)createManySelectedCalendarsWithTx(tx, calendars)
This way the service coordinates the transaction, but the actual database operations are delegated to the repository layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevinAI move createManySelectedCalendarsWithTx and deleteSelectedCalendarsByEventTypeIdWithTx to a repository called SelectedCalendarsRepository, create it if it does not exist already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I've moved the selected calendar transaction methods to SelectedCalendarsRepository:
- Added
deleteByEventTypeIdWithTx(tx, eventTypeId)toSelectedCalendarsRepository - Added
createManyForEventTypeWithTx(tx, calendars)toSelectedCalendarsRepository - Removed those methods from
EventTypesRepository_2024_06_14 - Updated the service to use
this.selectedCalendarsRepository.deleteByEventTypeIdWithTx()andthis.selectedCalendarsRepository.createManyForEventTypeWithTx()
The updateUseEventLevelSelectedCalendarsWithTx method remains in EventTypesRepository_2024_06_14 since it operates on the eventType table, not selectedCalendar.
…arsRepository Co-Authored-By: [email protected] <[email protected]>
What does this PR do?
Adds support for event-type level selected calendars for conflict checking in the API v2 event-types endpoints. This allows users to specify which calendars should be used for conflict checking on a per-event-type basis instead of using the user's default selected calendars.
Changes:
SelectedCalendar_2024_06_14input DTO andSelectedCalendarOutput_2024_06_14output DTOselectedCalendarsfield to create/update event-type inputs and outputuseEventLevelSelectedCalendarsflag is automatically inferred fromselectedCalendars(non-empty array = true, empty array = false)getByEventTypeId,createForEventType,deleteByEventTypeId)selectedCalendarsfrom public event-type endpoints output (per user request)Updates (review feedback fixes):
updateEventTypeSelectedCalendars- verifies user owns event type before modificationsgetEventTypeWithSelectedCalendars- throws ForbiddenException if user doesn't own event type$transactionfor atomicity (update flag, delete calendars, create new calendars)createManyselectclauses in repository methods to return only needed fieldsUpdates (addressing @supalarry's review feedback):
useEventLevelSelectedCalendarsfrom API input/output types entirely - it's now purely internal. The API only exposesselectedCalendars, and the system infers whether to use event-level calendars based on whether the array is non-empty.updateEventTypeSelectedCalendarslogic from controller into service methods (createUserEventTypeandupdateEventType)Updates (addressing Cubic AI review feedback):
destinationCalendarnot being forwarded toupdateEventTypeincreateUserEventType- the field was being destructured out but not passed through, causing created event types to silently ignore the requested destination calendarUpdates (function separation per @ThyMinimalDev's feedback):
getResponseEventTypesForPublicEndpointfunction that removesselectedCalendarsfrom public responsesgetResponseEventTypeWithoutHiddenFieldsnow only filters hidden booking fields (its original purpose)getResponseEventTypeForPublicEndpointremovesselectedCalendarsAND calls the hidden fields filteruseEventLevelSelectedCalendarsreferences (since it's no longer in the API)Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
POST /v2/event-typeswithselectedCalendarsarray containing calendar objects withintegrationandexternalIdfieldsPATCH /v2/event-types/:idwith the same fieldsGET /v2/event-types/:idand verify response includesselectedCalendarsGET /v2/event-types) do NOT includeselectedCalendarsselectedCalendars: []destinationCalendarworks correctly when creating event typesHuman Review Checklist
selectedCalendarsis excluded from public endpoint responsesuseEventLevelSelectedCalendarsflag based onselectedCalendarsarraygetResponseEventTypesForPublicEndpointis called for public endpoints andgetResponseEventTypeWithoutHiddenFieldsis preserved for its original purposedestinationCalendaris properly forwarded when creating event typesChecklist