fix: pass params when CRM contact owner fetch fails #24675
Conversation
When fetching CRM contact owner fails in getRoutedUrl, we now catch the error and continue with null CRM values instead of throwing. This ensures that other params (like formResponseId, teamMembersMatchingAttributeLogic) are still passed to the booking URL, preventing unnecessary refetching when the user visits the event URL. Co-Authored-By: [email protected] <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
keithwillcode
added
core
Added cal.crmFetchAttempted flag that is set when CRM contact owner fetch is attempted in routing forms. This flag is checked on the booking page to prevent unnecessary refetching when CRM lookup has already been attempted (whether it succeeded or failed). Changes: - getRoutedUrl: Add cal.crmFetchAttempted=true when fetchCrm is true - getServerSideProps: Check flag before refetching CRM data - queries.ts (App Router): Check flag before refetching CRM data - handleResponse: Catch CRM fetch errors to allow other params to pass Co-Authored-By: [email protected] <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
E2E results are ready! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
|
||
| if (!teamMemberEmail || !crmOwnerRecordType || !crmAppSlug) { | ||
| const shouldSkipCrmRefetch = | ||
| crmFetchAttempted === "true" || (Array.isArray(crmFetchAttempted) && crmFetchAttempted[0] === "true"); |
There was a problem hiding this comment.
Why do we consider crmFetchAttempted as an array ? We should assume it to be a boolean consider "true" to be only truthy value and other values to be false.
There was a problem hiding this comment.
I feel like for this particular case we shouldn't introduce new query params.
Query params being visible in the URL can be easily tampered with, so we introduce them only when absolutely needed.
How about we use existing query params with special value to mark that CRM fetch was attempted and it failed and thus avoid retrying it.
There was a problem hiding this comment.
It looks like a good approach could be to set crmContactOwnerEmail, crmContactOwnerRecordType, crmAppSlug query params to be set always when fetchCrm is true.
We could set them to empty string when the CRM fetch has failed.
When fetchCRM is false, we could just avoid setting these query parameters at all.
There was a problem hiding this comment.
Also, we seem to be in a place where we could remove fetchCrm argument/flag and consider it as true always.
Address Hariom's feedback to avoid introducing new query params: - Remove cal.crmFetchAttempted query parameter entirely - Set CRM params (crmContactOwnerEmail, crmContactOwnerRecordType, crmAppSlug) to empty strings when CRM fetch fails - Update getCRMData logic to check for param presence instead of truthiness - Update getUrlSearchParamsToForward to include CRM params when they are not null/undefined (even if empty string) - Update tests to expect empty strings instead of null values on CRM errors - Fix eslint warning by changing attributeRoutingConfigParams type from any to string This approach uses existing query params with special values (empty strings) to signal that CRM fetch was attempted but failed, avoiding the security concerns of adding a new tamperable query param. Co-Authored-By: [email protected] <[email protected]>
What does this PR do?
This PR fixes an issue where CRM contact owner fetch failures in routing forms would cause the entire form submission to fail, preventing other important params from being passed to the booking URL.
Problem: When
getRoutedUrlcallshandleResponse, if the CRM contact owner fetching fails (viarouterGetCrmContactOwnerEmail), the entire function throws an error. This means params likeformResponseIdandteamMembersMatchingAttributeLogicare lost, causing unnecessary refetching when users visit the event URL.Solution: Wrap the CRM contact owner fetching in a try-catch block. On error, log it and set CRM values to null, allowing the form submission to continue and pass other routing params successfully.
Link to Devin run: https://app.devin.ai/sessions/9eef198322914697a9acb3bba1fa68dc
Requested by: [email protected] (@joeauyeung)
Mandatory Tasks
How should this be tested?
Environment variables needed:
NEXT_PUBLIC_WEBAPP_URLsetTest scenario:
formResponseId,teamMembersMatchingAttributeLogic, and other params in the URL, even though CRM routing failedBefore this change: Form submission would fail entirely, showing an error page
After this change: Form submission succeeds with null CRM values, passes other routing params
Review Checklist
Critical items to review:
crmContactOwnerEmail,crmContactOwnerRecordType, andcrmAppSlugcorrectly prevents refetching and doesn't cause issues downstream in the booking flowhandleResponse, so they don't exercise this error path. Should we add a specific test for CRM fetch failures?Checklist
Summary by cubic
Form submissions no longer fail when CRM contact owner lookup errors. We catch the error, log it, and continue with null CRM values so other routing params (formResponseId, teamMembersMatchingAttributeLogic) are preserved and passed to the booking URL.