feat: Add attribute sync logic from Salesforce#26517
Merged
joeauyeung merged 148 commits intomainfrom Jan 15, 2026
Merged
Conversation
- getById - Init updateIncludeRulesAndMappings
… tests Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
graphite-app
bot
added
the
enterprise
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 34 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/prisma/migrations/20251231032625_add_integration_attribute_sync/migration.sql">
<violation number="1" location="packages/prisma/migrations/20251231032625_add_integration_attribute_sync/migration.sql:48">
P1: Schema-migration mismatch: The Prisma schema defines `@@unique([integrationAttributeSyncId, attributeId])` but this migration removes the corresponding unique index. This will cause the database to allow duplicate mappings while Prisma expects uniqueness, potentially leading to data integrity issues. Either keep this index creation or update the schema to remove the `@@unique` directive.</violation>
</file>
<file name="packages/features/attributes/repositories/PrismaAttributeRepository.ts">
<violation number="1" location="packages/features/attributes/repositories/PrismaAttributeRepository.ts:114">
P2: Using `include: { options: true }` fetches all fields from the options table, which can expose unnecessary data and increase performance overhead. Use a nested `select` within the `include` to specify only the fields needed, following the pattern used in `findManyByNamesAndOrgIdIncludeOptions` in this same file.</violation>
</file>
<file name="packages/features/credentials/repositories/CredentialRepository.ts">
<violation number="1" location="packages/features/credentials/repositories/CredentialRepository.ts:344">
P1: This instance method uses the global `prisma` import instead of `this.primaClient`. Other instance methods in this class use `this.primaClient` for consistency and proper dependency injection support. This should be changed to `this.primaClient.credential.findFirst(...)`.</violation>
<violation number="2" location="packages/features/credentials/repositories/CredentialRepository.ts:355">
P2: Using `include` fetches all fields from `attributeSyncRule` and `syncFieldMappings` relations. Per project guidelines, prefer `select` to only retrieve needed fields, reducing data exposure and improving performance. Consider specifying only the required fields with nested `select` instead of `include: true`.</violation>
</file>
<file name="packages/features/ee/integration-attribute-sync/services/AttributeSyncFieldMappingService.ts">
<violation number="1" location="packages/features/ee/integration-attribute-sync/services/AttributeSyncFieldMappingService.ts:242">
P2: Fetching all organization options is inefficient and creates O(n*m) complexity with the nested `.find()` loop. Consider either:
1. Querying only for the specific attributeIds that were created: filter by `attributeId: { in: optionsToCreate.map(o => o.attributeId) }`
2. Convert `allOptions` to a Map for O(1) lookups: `new Map(allOptions.map(o => [\`${o.attributeId}:${o.value.toLowerCase()}\`, o]))`
This violates the guideline to only select needed data and avoid O(n²)-like complexity in backend code.</violation>
</file>
<file name="packages/app-store/salesforce/api/user-sync.ts">
<violation number="1" location="packages/app-store/salesforce/api/user-sync.ts:25">
P2: Logging `changedFields` at info level may expose sensitive user data from Salesforce. Consider logging only the field names (`Object.keys(changedFields)`) instead of the full values, or omit it entirely.</violation>
<violation number="2" location="packages/app-store/salesforce/api/user-sync.ts:63">
P1: The `new URL(salesforceCredentialId)` call will throw an uncaught TypeError if the credential's `id` field is not a valid URL, causing a 500 error. Wrap this in a try-catch block or validate the URL format first.</violation>
<violation number="3" location="packages/app-store/salesforce/api/user-sync.ts:80">
P1: Rule violated: **Avoid Logging Sensitive Information**
`log.error` emits the end user’s email directly into logs. Replace the email with a masked identifier or omit it entirely to comply with the Avoid Logging Sensitive Information rule.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "AttributeSyncFieldMapping_integrationAttributeSyncId_attrib_key" ON "public"."AttributeSyncFieldMapping"("integrationAttributeSyncId", "attributeId"); | ||
|
|
||
| -- AddForeignKey |
Contributor
There was a problem hiding this comment.
P1: Schema-migration mismatch: The Prisma schema defines @@unique([integrationAttributeSyncId, attributeId]) but this migration removes the corresponding unique index. This will cause the database to allow duplicate mappings while Prisma expects uniqueness, potentially leading to data integrity issues. Either keep this index creation or update the schema to remove the @@unique directive.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/prisma/migrations/20251231032625_add_integration_attribute_sync/migration.sql, line 48:
<comment>Schema-migration mismatch: The Prisma schema defines `@@unique([integrationAttributeSyncId, attributeId])` but this migration removes the corresponding unique index. This will cause the database to allow duplicate mappings while Prisma expects uniqueness, potentially leading to data integrity issues. Either keep this index creation or update the schema to remove the `@@unique` directive.</comment>
<file context>
@@ -45,9 +45,6 @@ CREATE UNIQUE INDEX "AttributeSyncRule_integrationAttributeSyncId_key" ON "publi
--- CreateIndex
-CREATE UNIQUE INDEX "AttributeSyncFieldMapping_integrationAttributeSyncId_attrib_key" ON "public"."AttributeSyncFieldMapping"("integrationAttributeSyncId", "attributeId");
-
-- AddForeignKey
ALTER TABLE "public"."IntegrationAttributeSync" ADD CONSTRAINT "IntegrationAttributeSync_organizationId_fkey" FOREIGN KEY ("organizationId") REFERENCES "public"."Team"("id") ON DELETE CASCADE ON UPDATE CASCADE;
</file context>
packages/features/credentials/repositories/CredentialRepository.ts
Outdated
Show resolved
Hide resolved
packages/features/credentials/repositories/CredentialRepository.ts
Outdated
Show resolved
Hide resolved
| createManyInput: optionsToCreate, | ||
| }); | ||
|
|
||
| const allOptions = await this.deps.attributeOptionRepository.findMany({ |
Contributor
There was a problem hiding this comment.
P2: Fetching all organization options is inefficient and creates O(n*m) complexity with the nested .find() loop. Consider either:
- Querying only for the specific attributeIds that were created: filter by
attributeId: { in: optionsToCreate.map(o => o.attributeId) } - Convert
allOptionsto a Map for O(1) lookups:new Map(allOptions.map(o => [\${o.attributeId}:${o.value.toLowerCase()}`, o]))`
This violates the guideline to only select needed data and avoid O(n²)-like complexity in backend code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/ee/integration-attribute-sync/services/AttributeSyncFieldMappingService.ts, line 242:
<comment>Fetching all organization options is inefficient and creates O(n*m) complexity with the nested `.find()` loop. Consider either:
1. Querying only for the specific attributeIds that were created: filter by `attributeId: { in: optionsToCreate.map(o => o.attributeId) }`
2. Convert `allOptions` to a Map for O(1) lookups: `new Map(allOptions.map(o => [\`${o.attributeId}:${o.value.toLowerCase()}\`, o]))`
This violates the guideline to only select needed data and avoid O(n²)-like complexity in backend code.</comment>
<file context>
@@ -0,0 +1,270 @@
+ createManyInput: optionsToCreate,
+ });
+
+ const allOptions = await this.deps.attributeOptionRepository.findMany({
+ orgId,
+ });
</file context>
Contributor
There was a problem hiding this comment.
I've addressed this by implementing option 2 from the suggestion - converting allOptions to a Map for O(1) lookups:
const optionLookup = new Map(
allOptions.map((o) => [`${o.attributeId}:${o.value.toLowerCase()}`, o])
);
for (const newOption of optionsToCreate) {
const key = `${newOption.attributeId}:${newOption.value.toLowerCase()}`;
const createdOption = optionLookup.get(key);
// ...
}This eliminates the O(n*m) complexity by using O(1) Map lookups instead of O(m) .find() calls in the loop.
Add error handling when getting orgId from stored salesforce id Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
- PrismaAttributeRepository: use nested select instead of include: true for options - CredentialRepository: use this.primaClient instead of global prisma, use select instead of include for relations - AttributeSyncFieldMappingService: optimize O(n*m) complexity with Map lookup for O(1) access Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>
Contributor
Devin AI is addressing Cubic AI's review feedbackA Devin session has been created to address the issues identified by Cubic AI. |
Contributor
There was a problem hiding this comment.
4 issues found across 10 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="packages/app-store/salesforce/sfdc-package/force-app/main/default/classes/UserUpdateHandlerTest.cls">
<violation number="1" location="packages/app-store/salesforce/sfdc-package/force-app/main/default/classes/UserUpdateHandlerTest.cls:26">
P1: All test assertions use `System.assert(true, ...)` which always passes unconditionally. These tests only verify no exceptions occur but don't actually validate:
- HTTP callouts were made to Cal.com
- Correct payload was sent with changed fields
- Handler correctly processes user changes
Consider using `System.assertEquals` with actual expected values, or verify the mock was called with expected data using a custom mock that tracks invocations.</violation>
<violation number="2" location="packages/app-store/salesforce/sfdc-package/force-app/main/default/classes/UserUpdateHandlerTest.cls:53">
P2: Early return with `System.assert(true, ...)` silently skips the test without proper indication. If the org doesn't have 2+ active users, this test passes without testing anything. Consider using `System.assert(false, 'Test skipped: insufficient test data')` or creating test users with `@TestSetup` to ensure consistent test execution.</violation>
</file>
<file name="packages/app-store/salesforce/sfdc-package/force-app/main/default/classes/CalComCalloutQueueableTest.cls">
<violation number="1" location="packages/app-store/salesforce/sfdc-package/force-app/main/default/classes/CalComCalloutQueueableTest.cls:15">
P2: This `System.assert(true, ...)` assertion always passes and doesn't verify any actual behavior. Consider using `Test.getEventBus().deliver()` with platform events, or verify observable outcomes like logs, records created, or assert against mock invocation counts. At minimum, the test should verify that the mock's `respond` method was called.</violation>
<violation number="2" location="packages/app-store/salesforce/sfdc-package/force-app/main/default/classes/CalComCalloutQueueableTest.cls:19">
P1: This test creates 3 payloads but doesn't verify batching behavior. The underlying `CalComCalloutQueueable.execute()` makes a separate HTTP call per payload, which will hit Salesforce's ~100 callout-per-transaction limit with larger batches. Consider batching multiple payloads into a single request to `/api/integrations/salesforce/user-sync` and testing that behavior.
(Based on your team's feedback about batching payloads and Salesforce callout limits.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...s/app-store/salesforce/sfdc-package/force-app/main/default/classes/UserUpdateHandlerTest.cls
Show resolved
Hide resolved
| } | ||
|
|
||
| @isTest | ||
| static void testQueueableExecutionWithMultiplePayloads() { |
Contributor
There was a problem hiding this comment.
P1: This test creates 3 payloads but doesn't verify batching behavior. The underlying CalComCalloutQueueable.execute() makes a separate HTTP call per payload, which will hit Salesforce's ~100 callout-per-transaction limit with larger batches. Consider batching multiple payloads into a single request to /api/integrations/salesforce/user-sync and testing that behavior.
(Based on your team's feedback about batching payloads and Salesforce callout limits.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app-store/salesforce/sfdc-package/force-app/main/default/classes/CalComCalloutQueueableTest.cls, line 19:
<comment>This test creates 3 payloads but doesn't verify batching behavior. The underlying `CalComCalloutQueueable.execute()` makes a separate HTTP call per payload, which will hit Salesforce's ~100 callout-per-transaction limit with larger batches. Consider batching multiple payloads into a single request to `/api/integrations/salesforce/user-sync` and testing that behavior.
(Based on your team's feedback about batching payloads and Salesforce callout limits.) </comment>
<file context>
@@ -0,0 +1,97 @@
+ }
+
+ @isTest
+ static void testQueueableExecutionWithMultiplePayloads() {
+ List<CalComCalloutQueueable.UserChangePayload> payloads = createTestPayloads(3);
+
</file context>
| List<User> testUsers = [SELECT Id, FirstName, LastName, Email FROM User WHERE IsActive = true LIMIT 2]; | ||
|
|
||
| if (testUsers.size() < 2) { | ||
| System.assert(true, 'Not enough users to test multiple user scenario'); |
Contributor
There was a problem hiding this comment.
P2: Early return with System.assert(true, ...) silently skips the test without proper indication. If the org doesn't have 2+ active users, this test passes without testing anything. Consider using System.assert(false, 'Test skipped: insufficient test data') or creating test users with @TestSetup to ensure consistent test execution.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app-store/salesforce/sfdc-package/force-app/main/default/classes/UserUpdateHandlerTest.cls, line 53:
<comment>Early return with `System.assert(true, ...)` silently skips the test without proper indication. If the org doesn't have 2+ active users, this test passes without testing anything. Consider using `System.assert(false, 'Test skipped: insufficient test data')` or creating test users with `@TestSetup` to ensure consistent test execution.</comment>
<file context>
@@ -0,0 +1,122 @@
+ List<User> testUsers = [SELECT Id, FirstName, LastName, Email FROM User WHERE IsActive = true LIMIT 2];
+
+ if (testUsers.size() < 2) {
+ System.assert(true, 'Not enough users to test multiple user scenario');
+ return;
+ }
</file context>
| System.enqueueJob(queueable); | ||
| Test.stopTest(); | ||
|
|
||
| System.assert(true, 'Queueable executed without exceptions'); |
Contributor
There was a problem hiding this comment.
P2: This System.assert(true, ...) assertion always passes and doesn't verify any actual behavior. Consider using Test.getEventBus().deliver() with platform events, or verify observable outcomes like logs, records created, or assert against mock invocation counts. At minimum, the test should verify that the mock's respond method was called.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app-store/salesforce/sfdc-package/force-app/main/default/classes/CalComCalloutQueueableTest.cls, line 15:
<comment>This `System.assert(true, ...)` assertion always passes and doesn't verify any actual behavior. Consider using `Test.getEventBus().deliver()` with platform events, or verify observable outcomes like logs, records created, or assert against mock invocation counts. At minimum, the test should verify that the mock's `respond` method was called.</comment>
<file context>
@@ -0,0 +1,97 @@
+ System.enqueueJob(queueable);
+ Test.stopTest();
+
+ System.assert(true, 'Queueable executed without exceptions');
+ }
+
</file context>
✅ Addressed in 2febd5d
Contributor
Devin AI is addressing Cubic AI's review feedbackA Devin session has been created to address the issues identified by Cubic AI. |
- Enhanced CalComHttpMock to track HTTP callout invocations and capture requests - Updated UserUpdateHandlerTest to verify HTTP callouts are made with correct data - Updated CalComCalloutQueueableTest to verify HTTP callouts are made correctly - Replaced System.assert(true, ...) with meaningful assertions that verify: - Correct number of HTTP callouts - Request body contains expected fields - Request method is POST Addresses Cubic AI review feedback (confidence 9/10 issues only) Co-Authored-By: unknown <>
Contributor
There was a problem hiding this comment.
1 issue found across 3 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="packages/app-store/salesforce/sfdc-package/force-app/main/default/classes/CalComCalloutQueueableTest.cls">
<violation number="1" location="packages/app-store/salesforce/sfdc-package/force-app/main/default/classes/CalComCalloutQueueableTest.cls:36">
P2: This test now enforces one HTTP callout per payload, which contradicts the requirement to batch payloads and risks hitting Salesforce’s 100-callout limit. Update the queueable logic (and this assertion) so multiple payloads are sent in a single request whenever possible.
(Based on your team's feedback about batching Salesforce payloads to stay within callout limits.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| System.enqueueJob(queueable); | ||
| Test.stopTest(); | ||
|
|
||
| System.assertEquals(3, CalComHttpMock.getCallCount(), 'Expected three HTTP callouts for three payloads'); |
Contributor
There was a problem hiding this comment.
P2: This test now enforces one HTTP callout per payload, which contradicts the requirement to batch payloads and risks hitting Salesforce’s 100-callout limit. Update the queueable logic (and this assertion) so multiple payloads are sent in a single request whenever possible.
(Based on your team's feedback about batching Salesforce payloads to stay within callout limits.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app-store/salesforce/sfdc-package/force-app/main/default/classes/CalComCalloutQueueableTest.cls, line 36:
<comment>This test now enforces one HTTP callout per payload, which contradicts the requirement to batch payloads and risks hitting Salesforce’s 100-callout limit. Update the queueable logic (and this assertion) so multiple payloads are sent in a single request whenever possible.
(Based on your team's feedback about batching Salesforce payloads to stay within callout limits.) </comment>
<file context>
@@ -26,11 +33,13 @@ private class CalComCalloutQueueableTest {
Test.stopTest();
- System.assert(true, 'Queueable executed with multiple payloads without exceptions');
+ System.assertEquals(3, CalComHttpMock.getCallCount(), 'Expected three HTTP callouts for three payloads');
}
</file context>
Contributor
Devin AI is addressing Cubic AI's review feedbackNew feedback has been sent to the existing Devin session. |
hariombalhara
approved these changes
Jan 15, 2026
volnei
approved these changes
Jan 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
https://www.loom.com/share/1ef59edb84eb4b3c8cfa19d722c8e985
Implements end-to-end Salesforce user attribute sync functionality. When Salesforce triggers a user sync webhook, this PR:
Key additions:
POST /api/salesforce/user-syncendpoint for receiving Salesforce webhooksAttributeSyncRuleService- evaluates attribute-based conditions to determine sync applicabilityAttributeSyncFieldMappingService- maps integration fields to attributes, handles option creationAttributeService- fetches user's organization attributes for rule evaluationUpdates since last revision
Improved Salesforce Apex test assertions to verify actual behavior instead of using unconditional
System.assert(true, ...):CalComHttpMockto track HTTP callout invocations and capture requestsUserUpdateHandlerTestandCalComCalloutQueueableTestto verify:Human Review Checklist
Promise.allSettledusage in user-sync.ts - currently returns success even if some syncs failMandatory Tasks (DO NOT REMOVE)
How should this be tested?
For Apex tests: Deploy to a Salesforce scratch org and run
sf apex run test --test-level RunLocalTestsChecklist
Summary by cubic
Implements end-to-end Salesforce user attribute sync. Validates instance/org, resolves the team user, evaluates sync rules, maps changed fields, and updates attributes.
New Features
Refactors
Written for commit 8a40874. Summary will update on new commits.