-
Notifications
You must be signed in to change notification settings - Fork 12k
feat: Add attribute sync logic from Salesforce #26517
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
Changes from all commits
8aac966
6e50f46
dd776bf
49e3f33
2c897bf
4968b21
deeec8e
25ca942
7b178e9
09ac928
c0a71d2
22dbcfb
0264a9c
80144ab
8d0d61f
fd39eae
e83a8e4
b0eba94
a4796d3
ccbf84e
3ce313d
98af081
3303c2f
9cc2e5a
3fd9430
a6a4dc8
96802ed
bbce738
013c51e
327ea66
19c8423
652c87c
daad1d5
5d176bd
da07950
48fe98c
fece321
920bd80
6857276
8fc4eaf
6d98c8b
26889b6
3b9f334
eea3710
c43f6ad
9113610
53c50ef
24255e6
ef3b4a5
767e5dc
60f4171
cf0cbe0
1c0b033
04f60d7
08cd9a0
fec867f
947d91e
28ed00d
9b0d437
898542f
ad65b71
b288081
1a3abbc
fce660a
e37b5d8
a83c2e9
544877d
5c43536
8c38861
2eceb83
dda286d
ba5d4a4
ad1e092
4f1ce3f
c2252a6
e475921
2fc58dc
efa9260
e28dc1d
7e549d3
ebf508c
3c0a79d
6ff5680
d60966d
2d336b2
4578077
20fd629
b96c26c
6d39072
751add9
3cde78d
a8e4ede
fe847ae
dad327a
a41af8e
c34b00e
baca20b
4a516ad
b259a4f
1b88faf
1034c4f
22da98c
51fcf5d
ac481a6
084faed
8c8d282
d6480e4
12d3997
7806b2c
d126a57
95ce977
be1c679
961bae6
bdf1029
0454827
f474450
3b5d82e
09b12dc
76341a1
c6d77aa
39f2d98
d0719ab
7e258c9
d0bfb29
0c13825
5fcdde5
45966c8
d0b145f
22a12e8
efa51ce
4d3cde9
fa7724a
9d35ac7
964d207
9de1027
4179ce4
4a9e060
d1368fa
b218bf0
3efe7a5
d4f3198
168148c
2d46869
f9fe52c
e5b5c6d
f244af4
8a40874
2febd5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the endpoint that Salesforce will send the user updated payload to |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,25 +1,143 @@ | ||
| import type { NextApiRequest, NextApiResponse } from "next"; | ||
|
|
||
| import { CredentialRepository } from "@calcom/features/credentials/repositories/CredentialRepository"; | ||
| import { getAttributeSyncRuleService } from "@calcom/features/ee/integration-attribute-sync/di/AttributeSyncRuleService.container"; | ||
| import { getIntegrationAttributeSyncService } from "@calcom/features/ee/integration-attribute-sync/di/IntegrationAttributeSyncService.container"; | ||
| import { UserRepository } from "@calcom/features/users/repositories/UserRepository"; | ||
| import logger from "@calcom/lib/logger"; | ||
| import { prisma } from "@calcom/prisma"; | ||
| import type { NextApiRequest, NextApiResponse } from "next"; | ||
| import { getAttributeSyncFieldMappingService } from "@calcom/features/ee/integration-attribute-sync/di/AttributeSyncFieldMappingService.container"; | ||
|
|
||
| const log = logger.getSubLogger({ prefix: ["[salesforce/user-sync]"] }); | ||
|
|
||
| export default async function handler(req: NextApiRequest, res: NextApiResponse) { | ||
| export default async function handler( | ||
| req: NextApiRequest, | ||
| res: NextApiResponse | ||
| ) { | ||
| if (req.method !== "POST") { | ||
| return res.status(405).json({ error: "Method not allowed" }); | ||
| } | ||
|
|
||
| const { instanceUrl, orgId, salesforceUserId, email, changedFields, timestamp } = req.body; | ||
| const { | ||
| instanceUrl, | ||
| orgId: sfdcOrgId, | ||
| salesforceUserId, | ||
| email, | ||
| changedFields, | ||
joeauyeung marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| timestamp, | ||
| } = req.body; | ||
|
|
||
| log.info("Received user sync request", { | ||
| instanceUrl, | ||
| orgId, | ||
| sfdcOrgId, | ||
| salesforceUserId, | ||
| email, | ||
| changedFields, | ||
| timestamp, | ||
| }); | ||
|
|
||
| // TODO: Validate instanceUrl + orgId against stored credentials | ||
| // TODO: Sync changedFields to Cal.com user | ||
| const credentialRepository = new CredentialRepository(prisma); | ||
| const credential = await credentialRepository.findByAppIdAndKeyValue({ | ||
| appId: "salesforce", | ||
| keyPath: ["instance_url"], | ||
| value: instanceUrl, | ||
| keyFields: ["id"], | ||
| }); | ||
|
Comment on lines
+39
to
+44
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First step of validating that a payload belongs to an org in Cal is to grab the instance URL from the payload and match it with a Salesforce credential |
||
|
|
||
| if (!credential) { | ||
| log.error(`No credential found for ${instanceUrl}`); | ||
| return res.status(400).json({ error: "Invalid instance URL" }); | ||
| } | ||
|
|
||
| if (!credential?.teamId) { | ||
| log.error(`Missing teamId for credential ${credential.id}`); | ||
| return res.status(400).json({ error: "Invalid credential ID" }); | ||
| } | ||
|
Comment on lines
38
to
54
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Auth check isn't strong enough, do we plan to fix it within this PR?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry about that I just published my review comments. For the auth check we first get the credential via the Salesforce instance URL. Then we check that the incoming Salesforce org ID matches what we have stored in the credential. The probability of guessing the right combination is very very low.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is still not the correct way to authorize in the long run because if I have obtained those somehow, I have no-expiry access to make changes to attributes of an organization. This is mostly secure by obfuscation as I understand it. |
||
|
|
||
| const salesforceCredentialId = (credential.key as { id?: string } | null)?.id; | ||
|
|
||
| if (!salesforceCredentialId) { | ||
| log.error(`Missing SFDC id for credential ${credential.id}`); | ||
| return res.status(400).json({ error: "Invalid credential ID" }); | ||
| } | ||
|
|
||
| let storedSfdcOrgId: string | undefined; | ||
| try { | ||
| storedSfdcOrgId = new URL(salesforceCredentialId).pathname.split("/")[2]; | ||
| } catch { | ||
| log.error(`Invalid SFDC credential URL format for credential ${credential.id}`); | ||
| return res.status(400).json({ error: "Invalid credential format" }); | ||
| } | ||
|
|
||
| if (storedSfdcOrgId !== sfdcOrgId) { | ||
| log.error(`Mismatched orgId ${sfdcOrgId} for credential ${credential.id}`); | ||
| return res.status(400).json({ error: "Invalid org ID" }); | ||
| } | ||
|
|
||
| const userRepository = new UserRepository(prisma); | ||
| const user = await userRepository.findByEmailAndTeamId({ | ||
| email, | ||
| teamId: credential.teamId, | ||
| }); | ||
|
|
||
| if (!user) { | ||
| log.error( | ||
| `User not found for email ${email} and teamId ${credential.teamId}` | ||
joeauyeung marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ); | ||
| return res.status(400).json({ error: "Invalid user" }); | ||
| } | ||
|
|
||
| const organizationId = credential.teamId; | ||
|
|
||
| const integrationAttributeSyncService = getIntegrationAttributeSyncService(); | ||
|
|
||
| const integrationAttributeSyncs = | ||
| await integrationAttributeSyncService.getAllByCredentialId(credential.id); | ||
|
|
||
| const attributeSyncRuleService = getAttributeSyncRuleService(); | ||
| const attributeSyncFieldMappingService = | ||
| getAttributeSyncFieldMappingService(); | ||
|
|
||
| const results = await Promise.allSettled( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any errors here are not being logged |
||
| integrationAttributeSyncs.map(async (sync) => { | ||
| // Only check rule if one exists - skip sync only if rule returns false | ||
| if (sync.attributeSyncRule) { | ||
| const shouldSyncApplyToUser = | ||
| await attributeSyncRuleService.shouldSyncApplyToUser({ | ||
| user: { | ||
| id: user.id, | ||
| organizationId, | ||
| }, | ||
| attributeSyncRule: sync.attributeSyncRule.rule, | ||
|
Comment on lines
+105
to
+110
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I shared the concern in the previous PR, but it is important enough to be mentioned here again. We would probably need batch processing methods here to work for multiple users, soon.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In each request we would only receive a single user to update. Where we are batching though is creating new attribute options and assigning those attributes to the user.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I meant that in the call out we should send a batch of users as there is a limit in Queuable in the number of callouts we can make That would mean that we would then need to handle a batch of users. |
||
| }); | ||
|
|
||
| if (!shouldSyncApplyToUser) return; | ||
| } | ||
|
Comment on lines
+103
to
+114
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Integration attribute syncs can be saved with no rules so they apply to everyone in the org |
||
|
|
||
| // Salesforce multi-select picklists use `;` as separator, convert to `,` for the service | ||
| const integrationFields = Object.fromEntries( | ||
| Object.entries(changedFields as Record<string, unknown>) | ||
| .filter(([, value]) => value != null) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are filtering out null values here. This could potentially ignore salesforce field changes that were explicitly setting the values to null. So, those changes won;'t be synced to Cal.com |
||
| .map(([key, value]) => [key, String(value).replaceAll(";", ",")]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This replacement of ; to , should probably be delayed till we know the attribute type in which this value is going to be used. This could potentially cause unexpected literal ; being replaced
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Working with a customer on this and can confirm they don't have |
||
| ); | ||
|
Comment on lines
+117
to
+121
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Salesforce specifically separates values with |
||
|
|
||
| await attributeSyncFieldMappingService.syncIntegrationFieldsToAttributes({ | ||
| userId: user.id, | ||
| organizationId, | ||
| syncFieldMappings: sync.syncFieldMappings, | ||
| integrationFields, | ||
| }); | ||
| }) | ||
| ); | ||
|
|
||
| const errors = results.filter( | ||
| (result): result is PromiseRejectedResult => result.status === "rejected" | ||
| ); | ||
|
|
||
| if (errors.length > 0) { | ||
| log.error("Errors syncing user attributes", { | ||
| errors: errors.map((e) => e.reason), | ||
| }); | ||
| } | ||
|
|
||
| return res.status(200).json({ success: true }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| { | ||
| "defaultdevhubusername": "DevHub" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| @isTest | ||
| private class CalComCalloutQueueableTest { | ||
|
|
||
| @isTest | ||
| static void testQueueableExecutionSuccess() { | ||
| CalComHttpMock.resetState(); | ||
|
|
||
| List<CalComCalloutQueueable.UserChangePayload> payloads = createTestPayloads(1); | ||
|
|
||
| Test.setMock(HttpCalloutMock.class, new CalComHttpMock(200, '{"success": true}')); | ||
|
|
||
| Test.startTest(); | ||
| CalComCalloutQueueable queueable = new CalComCalloutQueueable(payloads); | ||
| System.enqueueJob(queueable); | ||
| Test.stopTest(); | ||
|
|
||
| System.assertEquals(1, CalComHttpMock.getCallCount(), 'Expected exactly one HTTP callout'); | ||
| List<HttpRequest> requests = CalComHttpMock.getCapturedRequests(); | ||
| System.assertEquals(1, requests.size(), 'Expected one captured request'); | ||
| System.assertEquals('POST', requests[0].getMethod(), 'Request method should be POST'); | ||
| } | ||
|
|
||
| @isTest | ||
| static void testQueueableExecutionWithMultiplePayloads() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: This test creates 3 payloads but doesn't verify batching behavior. The underlying (Based on your team's feedback about batching payloads and Salesforce callout limits.) Prompt for AI agents |
||
| CalComHttpMock.resetState(); | ||
|
|
||
| List<CalComCalloutQueueable.UserChangePayload> payloads = createTestPayloads(3); | ||
|
|
||
| Test.setMock(HttpCalloutMock.class, new CalComHttpMock(200, '{"success": true}')); | ||
|
|
||
| Test.startTest(); | ||
| CalComCalloutQueueable queueable = new CalComCalloutQueueable(payloads); | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } | ||
|
|
||
| @isTest | ||
| static void testQueueableExecutionFailureResponse() { | ||
| CalComHttpMock.resetState(); | ||
|
|
||
| List<CalComCalloutQueueable.UserChangePayload> payloads = createTestPayloads(1); | ||
|
|
||
| Test.setMock(HttpCalloutMock.class, new CalComHttpMock(500, '{"error": "Internal Server Error"}')); | ||
|
|
||
| Test.startTest(); | ||
| CalComCalloutQueueable queueable = new CalComCalloutQueueable(payloads); | ||
| System.enqueueJob(queueable); | ||
| Test.stopTest(); | ||
|
|
||
| System.assertEquals(1, CalComHttpMock.getCallCount(), 'Expected HTTP callout even for failure response'); | ||
| } | ||
|
|
||
| @isTest | ||
| static void testQueueableExecutionEmptyPayloads() { | ||
| CalComHttpMock.resetState(); | ||
|
|
||
| List<CalComCalloutQueueable.UserChangePayload> payloads = new List<CalComCalloutQueueable.UserChangePayload>(); | ||
|
|
||
| Test.setMock(HttpCalloutMock.class, new CalComHttpMock()); | ||
|
|
||
| Test.startTest(); | ||
| CalComCalloutQueueable queueable = new CalComCalloutQueueable(payloads); | ||
| System.enqueueJob(queueable); | ||
| Test.stopTest(); | ||
|
|
||
| System.assertEquals(0, CalComHttpMock.getCallCount(), 'Expected no HTTP callouts with empty payloads'); | ||
| } | ||
|
|
||
| @isTest | ||
| static void testUserChangePayloadStructure() { | ||
| CalComCalloutQueueable.UserChangePayload payload = new CalComCalloutQueueable.UserChangePayload(); | ||
| payload.salesforceUserId = UserInfo.getUserId(); | ||
| payload.email = 'test@example.com'; | ||
| payload.instanceUrl = 'https://test.salesforce.com'; | ||
| payload.orgId = UserInfo.getOrganizationId(); | ||
| payload.changedFields = new Map<String, Object>{'FirstName' => 'Test'}; | ||
| payload.timestamp = Datetime.now().formatGMT('yyyy-MM-dd\'T\'HH:mm:ss.SSS\'Z\''); | ||
|
|
||
| System.assertNotEquals(null, payload.salesforceUserId, 'salesforceUserId should be set'); | ||
| System.assertEquals('test@example.com', payload.email, 'email should match'); | ||
| System.assertNotEquals(null, payload.instanceUrl, 'instanceUrl should be set'); | ||
| System.assertNotEquals(null, payload.orgId, 'orgId should be set'); | ||
| System.assertEquals(1, payload.changedFields.size(), 'changedFields should have one entry'); | ||
| System.assertNotEquals(null, payload.timestamp, 'timestamp should be set'); | ||
| } | ||
|
|
||
| private static List<CalComCalloutQueueable.UserChangePayload> createTestPayloads(Integer count) { | ||
| List<CalComCalloutQueueable.UserChangePayload> payloads = new List<CalComCalloutQueueable.UserChangePayload>(); | ||
|
|
||
| for (Integer i = 0; i < count; i++) { | ||
| CalComCalloutQueueable.UserChangePayload payload = new CalComCalloutQueueable.UserChangePayload(); | ||
| payload.salesforceUserId = UserInfo.getUserId(); | ||
| payload.email = 'test' + i + '@example.com'; | ||
| payload.instanceUrl = URL.getOrgDomainUrl().toExternalForm(); | ||
| payload.orgId = UserInfo.getOrganizationId(); | ||
| payload.changedFields = new Map<String, Object>{ | ||
| 'FirstName' => 'TestFirst' + i, | ||
| 'LastName' => 'TestLast' + i | ||
| }; | ||
| payload.timestamp = Datetime.now().formatGMT('yyyy-MM-dd\'T\'HH:mm:ss.SSS\'Z\''); | ||
| payloads.add(payload); | ||
| } | ||
|
|
||
| return payloads; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <ApexClass xmlns="http://soap.sforce.com/2006/04/metadata"> | ||
| <apiVersion>64.0</apiVersion> | ||
| <status>Active</status> | ||
| </ApexClass> |
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.
Temporary files generated when creating a scratch org via the SF CLI tool