feat: Create Integration Attribute Sync records#26007
Conversation
- getById - Init updateIncludeRulesAndMappings
| "attribute_sync_credential_required": "Credential is required", | ||
| "attribute_sync_credential_validation": "Select a credential for the attribute sync", | ||
| "attribute_sync_select_credential": "Select a credential...", | ||
| "attribute_sync_credential_description": "Choose which integration credential to use for syncing attributes", |
There was a problem hiding this comment.
Why do we need to choose credential here? Can we not use the latest one or automatically decide from the list of credentials.
The problem with manual selection would be that if I somehow that credential is removed, syncing would stop or we might not be able to delete the credential because there are dependencies on it.
There was a problem hiding this comment.
We don't want this to be dependent on just Salesforce. In the future we also want to expand this to other apps as well.
If a credential is removed then there are a whole host of other issues that aren't just unique to this. Also if the credential is removed, having it set as the target of the attribute sync won't be a blocker.
| const currentUserOrgId = ctx.user.organizationId; | ||
| const integrationAttributeSyncService = getIntegrationAttributeSyncService(); | ||
|
|
||
| const integrationAttributeSync = await integrationAttributeSyncService.getById(input.id); | ||
|
|
||
| if (!integrationAttributeSync) throw new TRPCError({ code: "NOT_FOUND" }); | ||
|
|
||
| if (integrationAttributeSync.organizationId !== currentUserOrgId) { | ||
| throw new TRPCError({ code: "UNAUTHORIZED" }); | ||
| } | ||
| await integrationAttributeSyncService.deleteById(input.id); | ||
| }; |
There was a problem hiding this comment.
I think we should bake permissions within the Service itself.
It allows easy integration with API V2 or any other controller, where the controller just provides the data and does no application logic.
There was a problem hiding this comment.
I would argue against this because it feels like that violates the principle of single responsibility. It should be up to the main caller to check the permissions while the IntegrationAttributeSyncService should only focus on logic surrounding the attribute syncs.
packages/features/ee/integration-attribute-sync/services/IntegrationAttributeSyncService.ts
Show resolved
Hide resolved
packages/features/ee/integration-attribute-sync/services/IntegrationAttributeSyncService.ts
Show resolved
Hide resolved
| credentialId: input.credentialId, | ||
| enabled: input.enabled, | ||
| rule: parsedRule, | ||
| syncFieldMappings: input.syncFieldMappings, |
There was a problem hiding this comment.
We should also make sure probably that integrationFieldName is stored lowercase. AFAIU, salesfore custom field names are case-insensitive only
There was a problem hiding this comment.
Salesforce is case-insensitive but we might expand this to other integrations where that might not be the case. Ex. Hubspot is case-sensitive.
We can normalize when we receive a payload from Salesforce in #26517
hariombalhara
left a comment
There was a problem hiding this comment.
Left some suggestions !!
* feat: implement FeatureOptInService WIP * clean up * feat: consolidate feature repositories and add updateFeatureForUser - Implement updateFeatureForUser in FeaturesRepository (similar to updateFeatureForTeam) - Move getUserFeatureState and getTeamFeatureState from PrismaFeatureOptInRepository to FeaturesRepository - Update FeatureOptInService to use only FeaturesRepository - Add setUserFeatureState and setTeamFeatureState methods to FeatureOptInService - Update _router.ts to remove PrismaFeatureOptInRepository usage - Remove PrismaFeatureOptInRepository.ts and FeatureOptInRepositoryInterface.ts - Update features.repository.interface.ts and features.repository.mock.ts - Add integration tests for updateFeatureForUser, getUserFeatureState, getTeamFeatureState - Update service.integration-test.ts to use FeaturesRepository Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * refactor: rename updateFeatureForUser to setUserFeatureState Rename to match the convention used for setTeamFeatureState Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * refactor: return FeatureState type from getUserFeatureState and getTeamFeatureState * fix integration tests * clean up logics * update services and router * refactor: change getUserFeatureState and getTeamFeatureState to accept featureIds array - Renamed getUserFeatureState to getUserFeatureStates - Renamed getTeamFeatureState to getTeamFeatureStates - Changed parameter from featureId: string to featureIds: string[] - Changed return type from FeatureState to Record<string, FeatureState> - Updated FeatureOptInService to use the new batch methods - Added tests for querying multiple features in a single call - Optimized listFeaturesForTeam to fetch all feature states in one query Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * feat: add getFeatureStateForTeams for batch querying multiple teams - Added getFeatureStateForTeams method to query a single feature across multiple teams in one call - Updated FeatureOptInService.resolveFeatureStateAcrossTeams to use the new batch method - Replaces N+1 queries with a single database query for team states - Added comprehensive integration tests for the new method Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * refactor: combine org and team state queries into single call - Include orgId in the teamIds array passed to getFeatureStateForTeams - Extract org state and team states from the combined result - Reduces database queries from 3 to 2 in resolveFeatureStateAcrossTeams Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * refactor: use team.isOrganization and clarify computeEffectiveState comment Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * refactor: use MembershipRepository.findAllByUserId with isOrganization Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * feat: add featureId validation using isOptInFeature type guard Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * less queries * add fallback value * fix type error * move files * add autoOptInFeatures column * use autoOptInFeatures flag within FeatureOptInService * add setUserAutoOptIn and setTeamAutoOptIn * fix computeEffectiveState logic * rewrite computeEffectiveState * clean up integration tests * clean up in afterEach * fix type error * refactor: use FeaturesRepository methods instead of direct Prisma calls Replace all manual userFeatures and teamFeatures Prisma operations with the new setUserFeatureState and setTeamFeatureState repository methods. Changes include: - Admin handlers (assignFeatureToTeam, unassignFeatureFromTeam) - Test fixtures and integration tests - Playwright fixtures - Development scripts This ensures consistent feature flag management through the repository pattern and supports the new tri-state semantics (enabled/disabled/inherit). Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * clean up * fix the logic * extract some logic into applyAutoOptIn() * remove wrong code * refactor: convert setUserFeatureState and setTeamFeatureState to object params with discriminated union - Convert multiple positional parameters to single object parameter - Use discriminated union types: assignedBy required for enabled/disabled, omitted for inherit - Update all callers across repository, service, handlers, fixtures, and tests * fix type error * use Promise.all * fix --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
There was a problem hiding this comment.
6 issues found across 41 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="packages/features/ee/integration-attribute-sync/components/RuleBuilder.tsx">
<violation number="1" location="packages/features/ee/integration-attribute-sync/components/RuleBuilder.tsx:459">
P2: Using `index` as `key` in a list that supports removal can cause React reconciliation bugs. When a condition is removed from the middle, subsequent conditions shift indices and may retain incorrect internal state. Consider generating a unique ID when creating conditions (e.g., using `crypto.randomUUID()` or a counter).</violation>
</file>
<file name="packages/prisma/schema.prisma">
<violation number="1" location="packages/prisma/schema.prisma:3128">
P2: Missing index on `credentialId` in `IntegrationAttributeSync`. The schema consistently indexes `credentialId` on other models with credential relations (DestinationCalendar, SelectedCalendar, CalendarCache). Add `@@index([credentialId])` for efficient cascade deletes and credential-based queries.</violation>
</file>
<file name="packages/trpc/server/routers/viewer/attribute-sync/updateAttributeSync.handler.ts">
<violation number="1" location="packages/trpc/server/routers/viewer/attribute-sync/updateAttributeSync.handler.ts:23">
P2: Add an early organization check before making service calls. The `createAttributeSync.handler.ts` validates the user has an organization before any service call, but this handler calls `getById` first. Following the early return pattern would be more efficient and provide a better error message for users without an organization.</violation>
</file>
<file name="packages/features/ee/integration-attribute-sync/components/NewIntegrationAttributeSyncCard.tsx">
<violation number="1" location="packages/features/ee/integration-attribute-sync/components/NewIntegrationAttributeSyncCard.tsx:30">
P2: Translation key `credential_required` does not exist. Use `attribute_sync_credential_required` to match the parent component and the existing locale definition.</violation>
</file>
<file name="packages/trpc/server/routers/viewer/attribute-sync/createAttributeSync.handler.ts">
<violation number="1" location="packages/trpc/server/routers/viewer/attribute-sync/createAttributeSync.handler.ts:40">
P2: String matching on error messages is fragile. The service already defines custom error classes (`DuplicateAttributeWithinSyncError`, `DuplicateAttributeAcrossSyncsError`). Consider adding a `CredentialNotFoundError` class for consistency and reliability. If the error message text changes in the service, this catch block will silently fail.</violation>
</file>
<file name="packages/features/ee/integration-attribute-sync/services/IntegrationAttributeSyncService.test.ts">
<violation number="1" location="packages/features/ee/integration-attribute-sync/services/IntegrationAttributeSyncService.test.ts:397">
P2: `expect.fail()` is not a standard Vitest API and will throw a TypeError at runtime. If the service doesn't throw as expected, this causes confusing test failures. Consider throwing an Error directly or using the `rejects.toSatisfy()` pattern to check error properties.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/features/ee/integration-attribute-sync/components/RuleBuilder.tsx
Outdated
Show resolved
Hide resolved
packages/trpc/server/routers/viewer/attribute-sync/updateAttributeSync.handler.ts
Outdated
Show resolved
Hide resolved
packages/features/ee/integration-attribute-sync/components/NewIntegrationAttributeSyncCard.tsx
Outdated
Show resolved
Hide resolved
packages/trpc/server/routers/viewer/attribute-sync/createAttributeSync.handler.ts
Outdated
Show resolved
Hide resolved
...ages/features/ee/integration-attribute-sync/services/IntegrationAttributeSyncService.test.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 @@index([credentialId]) to IntegrationAttributeSync model for efficient cascade deletes and credential-based queries (confidence: 9/10) - Fix translation key from 'credential_required' to 'attribute_sync_credential_required' to match existing locale definition (confidence: 10/10) Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
- RuleBuilder.tsx: Use unique IDs for React keys instead of array index to prevent reconciliation bugs when removing conditions - updateAttributeSync.handler.ts: Add early organization check before service calls for consistency - createAttributeSync.handler.ts: Add CredentialNotFoundError class for type-safe error handling instead of string matching - IntegrationAttributeSyncService.test.ts: Replace expect.fail() with proper Vitest rejects.toSatisfy() pattern Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
Addressed comments and reviewed by Volnei
What does this PR do?
Adds an organization-level Attribute Sync feature that allows admins to create, edit, and delete integration attribute syncs with rules and field mappings. This enables syncing user attributes from third-party integrations (currently Salesforce) to Cal.com attributes.
https://www.loom.com/share/ddb1af433f5d4ce3a75cfbbacc680de1
Key changes:
/settings/organizations/attributes/syncwith PBAC gatingIntegrationAttributeSync,AttributeSyncRule,AttributeSyncFieldMappingfindByTeamIdAndSlugsmethod toCredentialRepositoryFormCardcomponent to support custom actionsUpdates since last revision
Addressed cubic review feedback:
crypto.randomUUID()) for React keys instead of array index to prevent reconciliation bugs when removing conditions from the middle of the listCredentialNotFoundErrorclass for type-safe error handling instead of fragile string matchingexpect.fail()with proper Vitestrejects.toSatisfy()patternPrevious updates:
attributeSyncRulesarray to singularattributeSyncRuleto match the database schema's one-to-one relationshipMandatory Tasks (DO NOT REMOVE)
How should this be tested?
/settings/organizations/attributes/syncPrerequisites:
Human Review Checklist
organization.attributes.create) is working correctlyattributeSyncRuletype works correctly when creating/editing syncsChecklist
Link to Devin run: https://app.devin.ai/sessions/888243402bd14cbd98b1cfe1c3a5424b
Requested by: joe@cal.com (joe@cal.com) (@joeauyeung)