-
Notifications
You must be signed in to change notification settings - Fork 437
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
feat(integration-templates): add quickbooks actions & syncs #2741
Conversation
I've been utilizing the QuickBooks sandbox environment for testing, as it offers a comprehensive set of test data and API simulation capabilities. I'll update the integration accordingly once the review is finalized. |
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.
Massive work 💪🏻
Nothing critical in my comments, just a few open questions and possible improvements
* | ||
* @param {NangoAction} nango - The NangoAction instance for handling the get Connection task. | ||
* @returns {Promise<string>} - The realmId for the QuickBooks instance. | ||
* @throws {Error} - Throws an error if the realmId cannot be retrieved. |
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.
just out of curiosity, your new PR seems to have a lot jsdoc, is this a new requirement?
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.
It’s not a new requirement; I just thought it would be beneficial to include JSDoc comments for better documentation. What do you think?
const quickBooksAccount = toQuickBooksAccount(input); | ||
|
||
const config: ProxyConfiguration = { | ||
baseUrlOverride: 'https://sandbox-quickbooks.api.intuit.com', |
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.
Is this something correct to hardcode?
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.
I've been utilizing the QuickBooks sandbox environment for testing, as it offers a comprehensive set of test data and API simulation capabilities. That is why I had to override the base url. But I will change this after review, before merging the PR 👍.
} | ||
|
||
// Validate required fields | ||
if (!input.customer_ref || !input.customer_ref.value) { |
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.
Just a matter of thought we can generate zod models, maybe it's shorter and more reliable than manual validation
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.
I was thinking the same thing. Now that we also generate tests, it would be good practice to generate the Zod models and override them where possible. It would also be nice to apply this approach to the other models. What do you think?
Describe your changes
Issue ticket number and link
EXT-143