Conversation
keithwillcode
added
core
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
| expect(querySpy).toHaveBeenNthCalledWith( | ||
| 1, | ||
| "SELECT Id, Email, OwnerId, AccountId FROM Contact WHERE Email = 'test@example.com' AND AccountId != null" | ||
| ); | ||
| expect(querySpy).toHaveBeenNthCalledWith(2, "SELECT Id, OwnerId FROM Account WHERE Id = 'acc001'"); | ||
| expect(querySpy).toHaveBeenNthCalledWith( | ||
| 3, | ||
| expect.stringContaining("SELECT Id, Email, Name FROM User WHERE Id = 'owner001'") | ||
| ); |
There was a problem hiding this comment.
I am also checking the exact query in every test
| const accountId = await this.getAccountIdBasedOnEmailDomainOfContacts(attendeeEmail); | ||
| if (accountId) { | ||
| soql = `SELECT Id, OwnerId FROM Account WHERE Id = '${accountId}'`; | ||
| const log = logger.getSubLogger({ prefix: [`[getContacts]:${emails}`] }); |
There was a problem hiding this comment.
For better logging and debugging.
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (12/12/24)1 reviewer was added to this PR based on Keith Williams's automation. |
|
@Udit-takkar tests are failing |
| if (accountId) { | ||
| soql = `SELECT Id, OwnerId FROM Account WHERE Id = '${accountId}'`; | ||
| const log = logger.getSubLogger({ prefix: [`[getContacts]:${emails}`] }); | ||
| try { |
There was a problem hiding this comment.
As a followup, we should add try-catch at a higher level so that no CRM app can crash the booking page and not just salesforce.
There was a problem hiding this comment.
AFAIK CrmManager prevents a crash to block bookings doesn't it?
There was a problem hiding this comment.
This fn is called while loading the booking page. There it isn't handled
|
|
||
| // If this is for a round robin skip then we need to return the account record | ||
| if (forRoundRobinSkip) { | ||
| const results = await conn.query(soql); |
There was a problem hiding this comment.
Followup:
It is best to have type assertion here that considers records as possibly nullish and records. Items of records should be asserted as nullish.
In this way if we accidentally miss an optional chaining operator, TS will complain.
Right now records is unknown[], so TS won't complain if we miss a ? after records.
| log.info("Account contact search results", { resultCount: results.records.length }); | ||
|
|
||
| if (results.records?.length) { | ||
| const contact = results.records[0] as { AccountId?: string }; | ||
| if (contact?.AccountId) { | ||
| soql = `SELECT Id, OwnerId FROM Account WHERE Id = '${contact.AccountId}'`; | ||
| } | ||
| } else { | ||
| // If we can't find the exact contact, then we need to search for an account where the contacts share the same email domain | ||
| const accountId = await this.getAccountIdBasedOnEmailDomainOfContacts(attendeeEmail); | ||
| if (accountId) { | ||
| soql = `SELECT Id, OwnerId FROM Account WHERE Id = '${accountId}'`; | ||
| } | ||
| } | ||
| } | ||
| // If creating events on contacts or leads | ||
| } else { | ||
| // Handle Contact/Lead record types | ||
| soql = `SELECT Id, Email, OwnerId FROM ${recordToSearch} WHERE Email IN ('${emailArray.join( | ||
| "','" | ||
| )}')`; | ||
| } |
There was a problem hiding this comment.
This entire function is quite complex now. As a followup we should break it down into smaller functions but after adding good amount of unit tests for this,
There was a problem hiding this comment.
Yeah i have mentioned this in the PR description. we would have to split all the bug functions into smaller ones.
hariombalhara
left a comment
hariombalhara
left a comment
There was a problem hiding this comment.
There are failing unit tests and logs need to be improved.
dosubot
bot
added
the
crm-apps
E2E results are ready! |
5 participants
What does this PR do?
For better debugging and prevent Type error
https://app.campsite.com/cal/posts/vwtobznl53bv#comment-yze7du8uorc9
Follow up Steps
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?