feat: add HubSpot contact owner toggle #26552
Conversation
|
@Pallava-Joshi is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 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/app-store/hubspot/lib/CrmService.ts">
<violation number="1" location="packages/app-store/hubspot/lib/CrmService.ts:407">
P1: Rule violated: **Avoid Logging Sensitive Information**
Logging the entire `meetingEvent` object exposes potentially sensitive user data. The meeting body contains user form responses, additional notes, location, and description. Consider reverting to logging only the meeting ID:
```typescript
this.log.debug("meeting:creation:ok", { meetingId: meetingEvent.id });
```</violation>
<violation number="2" location="packages/app-store/hubspot/lib/CrmService.ts:549">
P2: The `getPage()` call without parameters fetches all HubSpot owners instead of filtering by email server-side. The previous implementation used the email parameter to filter server-side. This causes unnecessary data transfer and may miss the owner if there are more owners than the default page size (pagination not handled).
Consider passing the email parameter to enable server-side filtering:
```typescript
const ownersResponse = await this.hubspotClient.crm.owners.ownersApi.getPage(email);
```</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
|
@cubic-dev-ai leave a review |
@Pallava-Joshi I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 5 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/app-store/hubspot/lib/CrmService.ts">
<violation number="1" location="packages/app-store/hubspot/lib/CrmService.ts:66">
P2: Organizer information was removed from the HubSpot meeting body. This may be unintentional - the PR description mentions adding a contact owner toggle, but doesn't mention removing organizer info from meeting descriptions. HubSpot meeting records will no longer show who organized the meeting in the body text. If this is intentional, please confirm; otherwise, consider preserving the organizer info.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "hubspot_set_organizer_as_owner": "Set organizer as contact owner", | ||
| "hubspot_overwrite_contact_owner": "Overwrite existing contact owner", |
There was a problem hiding this comment.
since the text is not app specific, can we rename the keys here to not include app name. We can re use it for other apps then
| const organizerEmail = event.organizer.email; | ||
| this.log.debug("hubspot:meeting:fetching_owner"); | ||
|
|
||
| const hubspotOwnerId = await this.getHubspotOwnerIdByEmail(organizerEmail); | ||
|
|
||
| const meetingEvent = await this.hubspotCreateMeeting(event, hubspotOwnerId); |
There was a problem hiding this comment.
Contact owners and meeting owners are two separate things.
Meeeting owners basically mean the organizer / host of the current meeting. This will always be the event.organizer.email. This logic will remain the same
booking organizer -> will always be the meeting owner
Can you revert this please
| private async maybeSetContactOwner( | ||
| contactId: string, | ||
| ownerId: string, | ||
| overwriteContactOwner: boolean | ||
| ): Promise<void> { | ||
| if (overwriteContactOwner) { | ||
| await this.setContactOwner(contactId, ownerId); |
There was a problem hiding this comment.
Not a big fan of such naming maybeSetContactOwner, can we have something more conclusive so it's easy to know what the function does just name?
setOwnerIf....
| const appOptions = this.getAppOptions(); | ||
| if (appOptions?.setOrganizerAsOwner) { | ||
| const organizerEmail = event.organizer.email; | ||
| const ownerId = await this.getHubspotOwnerIdFromEmail(organizerEmail); | ||
| if (ownerId) { | ||
| const firstContact = contacts[0]; | ||
| if (firstContact?.id) { | ||
| await this.maybeSetContactOwner( | ||
| firstContact.id, | ||
| ownerId, | ||
| appOptions?.overwriteContactOwner ?? false | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's make this easy on the eyes
| const appOptions = this.getAppOptions(); | |
| if (appOptions?.setOrganizerAsOwner) { | |
| const organizerEmail = event.organizer.email; | |
| const ownerId = await this.getHubspotOwnerIdFromEmail(organizerEmail); | |
| if (ownerId) { | |
| const firstContact = contacts[0]; | |
| if (firstContact?.id) { | |
| await this.maybeSetContactOwner( | |
| firstContact.id, | |
| ownerId, | |
| appOptions?.overwriteContactOwner ?? false | |
| ); | |
| } | |
| } | |
| } | |
| const firstContact = contacts[0]; | |
| const appOptions = this.getAppOptions(); | |
| // Since you are reverting the change where you removed meeting owner from being set earlier | |
| if (hubspotOwnerId && firstContact?.id && appOptions?.setOrganizerAsOwner) { | |
| await this.maybeSetContactOwner( | |
| firstContact.id, | |
| ownerId, | |
| appOptions?.overwriteContactOwner ?? false | |
| ) | |
| } |
2 participants
What does this PR do?
When a booking is made, automatically sets the HubSpot contact's hubspot_owner_id to match the Cal.com organizer (the sales rep assigned to the meeting). Also overwrites the owner, if the toggle is on, otherwise set to the first sales rep.
Video Demo (if applicable):
Screen.Recording.2026-01-07.at.12.53.43.PM.mov
Mandatory Tasks (DO NOT REMOVE)