Feat/microsoft dynamics connection #625
Conversation
|
Qovery can create a Preview Environment for this PR.
This comment has been generated from Qovery AI 🤖.
|
|
|
|
|
Caution Review failedThe pull request is closed. WalkthroughWalkthroughThe recent updates enhance the integration capabilities of the Panora API, particularly in relation to Ecommerce and Microsoft Dynamics Sales. Key modifications include renaming resources from "files" to "products," enhancing state data handling for greater flexibility, and introducing support for Microsoft Dynamics Sales. These changes aim to improve clarity, functionality, and security for users working with product data and CRM services. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (6)
- docs/ecommerce/quickstart.mdx (4 hunks)
- packages/api/src/@core/connections/connections.controller.ts (2 hunks)
- packages/api/src/@core/connections/crm/crm.connection.module.ts (2 hunks)
- packages/api/src/@core/connections/crm/services/microsoftdynamicssales/microsoftdynamicssales.service.ts (1 hunks)
- packages/shared/src/authUrl.ts (2 hunks)
- packages/shared/src/connectors/metadata.ts (1 hunks)
Additional context used
LanguageTool
docs/ecommerce/quickstart.mdx
[misspelling] ~7-~7: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ..." icon: "star" --- ## List products in a Ecommerce provider using Panora <Check...(EN_A_VS_AN)
Biome
packages/shared/src/authUrl.ts
[error] 60-60: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 181-181: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 182-182: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
packages/api/src/@core/connections/crm/services/microsoftdynamicssales/microsoftdynamicssales.service.ts
[error] 25-25: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 93-93: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
[error] 134-135: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 189-189: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
[error] 240-240: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
Additional comments not posted (14)
docs/ecommerce/quickstart.mdx (2)
35-41: LGTM!The changes align with the new functionality of listing products.
Line range hint
53-68:
LGTM!The changes align with the new functionality of listing products.
packages/api/src/@core/connections/crm/crm.connection.module.ts (2)
23-23: LGTM!The import statement for
MicrosoftDynamicsSalesConnectionServiceis necessary for the new functionality.
48-48: LGTM!The addition of
MicrosoftDynamicsSalesConnectionServiceto theprovidersarray is necessary for the new functionality.packages/api/src/@core/connections/connections.controller.ts (2)
38-38: LGTM!The enhancement to
StateDataTypeadds flexibility to the state data structure.
85-92: LGTM!The destructuring assignment of
stateDatanow includes theresourceproperty, aligning with the new functionality.packages/shared/src/authUrl.ts (2)
58-61: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
constructAuthUrlpass theadditionalParamsobject when the provider ismicrosoftdynamicssales.Tools
Biome
[error] 60-60: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
172-184: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
handleOAuth2Urlpass theadditionalParamsobject when the provider ismicrosoftdynamicssales.Verification successful
Verification successful: The
additionalParamsobject is correctly populated and passed tohandleOAuth2Urlwhen the provider ismicrosoftdynamicssales.
- The
stateobject is updated to includeadditionalParams.end_user_domainasresourcewhen the provider ismicrosoftdynamicssales.- This
stateobject is then passed tohandleOAuth2Url.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `handleOAuth2Url` pass the `additionalParams` object when the provider is `microsoftdynamicssales`. # Test: Search for the function usage. Expect: Occurrences of the new signature. rg --type typescript -A 5 $'handleOAuth2Url'Length of output: 78
Script:
#!/bin/bash # Description: Verify all function calls to `handleOAuth2Url` pass the `additionalParams` object when the provider is `microsoftdynamicssales`. # Find all TypeScript files and search for `handleOAuth2Url` within those files. fd --extension ts --exec rg 'handleOAuth2Url'Length of output: 136
Script:
#!/bin/bash # Description: Verify all function calls to `handleOAuth2Url` pass the `additionalParams` object when the provider is `microsoftdynamicssales`. # Find all TypeScript files and search for `handleOAuth2Url` within those files, including more context lines for better understanding. fd --extension ts --exec rg 'handleOAuth2Url' -A 10 -B 5Length of output: 751
Script:
#!/bin/bash # Description: Verify that `additionalParams` is correctly populated and passed to `handleOAuth2Url` when the provider is `microsoftdynamicssales`. # Find all TypeScript files and search for `additionalParams` within those files, including more context lines for better understanding. fd --extension ts --exec rg 'additionalParams' -A 10 -B 10Length of output: 6113
Tools
Biome
[error] 181-181: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 182-182: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
packages/shared/src/connectors/metadata.ts (6)
213-213: Update logo path.The
logoPathhas been updated to a new URL. Ensure that the new logo is correctly displayed and the URL is accessible.Verification successful
The new logo URL is accessible and returns an HTTP/2 200 status code. The update to the
logoPathis correctly implemented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the accessibility of the new logo URL. # Test: Check if the new logo URL is accessible. curl -I 'https://play-lh.googleusercontent.com/MC_Aoa7rlMjGtcgAdiLJGeIm3-kpVw7APQmQUrUZtXuoZokiqVOJqR-bTu7idJBD8g' | grep 'HTTP/2 200'Length of output: 463
206-206: Ensure consistent naming conventions.The connector key has been changed from
'microsoft_dynamics_sales'to'microsoftdynamicssales'. Ensure that this change is reflected consistently throughout the codebase to avoid any discrepancies.Verification successful
Consistent naming conventions verified.
The connector key has been updated to
microsoftdynamicssalesand is used consistently throughout the codebase.
- No occurrences of the old key
microsoft_dynamics_saleswere found.- The new key
microsoftdynamicssalesis present in all relevant files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of the new connector key `microsoftdynamicssales`. # Test: Search for the old and new connector keys. Expect: No occurrences of the old key, only the new key should be present. rg --type js --type ts 'microsoft_dynamics_sales' rg --type js --type ts 'microsoftdynamicssales'Length of output: 1659
220-220: Modernize authentication strategy.The
authStrategyhas shifted fromAuthStrategy.api_keytoAuthStrategy.oauth2, which is a more secure and modern approach. Ensure that all relevant parts of the codebase are updated to handle OAuth2 authentication.
210-210: Verify dynamic URL generation.The
authBaseUrlnow uses a template string incorporating theorgNameparameter. Ensure thatorgNameis correctly provided and sanitized to prevent any security issues.
216-218: Validate new options.The
optionsproperty now includesend_user_domain: true. Ensure that this option is correctly handled in the codebase and does not introduce any security vulnerabilities.
211-211: Update API version.The
apiUrlhas been set to'/api/data/v9.2'. Ensure that all API calls are compatible with this version and update any documentation or references to the API version accordingly.
| @@ -4,7 +4,7 @@ description: "Read data from multiple Ecommerce platforms using a single API" | |||
| icon: "star" | |||
| --- | |||
|
|
|||
| ## List files in a Ecommerce provider using Panora | |||
| ## List products in a Ecommerce provider using Panora | |||
There was a problem hiding this comment.
Fix grammatical issue.
Use "an" instead of "a" before "Ecommerce provider".
- ## List products in a Ecommerce provider using Panora
+ ## List products in an Ecommerce provider using PanoraCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## List products in a Ecommerce provider using Panora | |
| ## List products in an Ecommerce provider using Panora |
Tools
LanguageTool
[misspelling] ~7-~7: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ..." icon: "star" --- ## List products in a Ecommerce provider using Panora <Check...(EN_A_VS_AN)
| 'microsoft_dynamics_sales': { | ||
| scopes: '', | ||
| 'microsoftdynamicssales': { | ||
| scopes: 'offline_access', |
There was a problem hiding this comment.
Add a more descriptive scope.
The scope 'offline_access' has been added. While this is correct, consider adding more specific scopes that align with the permissions required by the Microsoft Dynamics Sales API for better security and functionality.
- scopes: 'offline_access',
+ scopes: 'offline_access Dynamics.CRM.read Dynamics.CRM.write',Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| scopes: 'offline_access', | |
| scopes: 'offline_access Dynamics.CRM.read Dynamics.CRM.write', |
packages/shared/src/authUrl.ts
Outdated
| let b = `https://${resource}/.default`; | ||
| b += (' offline_access'); | ||
| console.log("scopes is "+ b) | ||
| console.log("BASE URL is "+ BASE_URL) |
There was a problem hiding this comment.
Use template literals for string concatenation.
Template literals are preferred over string concatenation for better readability.
- let b = `https://${resource}/.default`;
- b += (' offline_access');
- console.log("scopes is "+ b)
- console.log("BASE URL is "+ BASE_URL)
+ let b = `https://${resource}/.default offline_access`;
+ console.log(`scopes is ${b}`);
+ console.log(`BASE URL is ${BASE_URL}`);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let b = `https://${resource}/.default`; | |
| b += (' offline_access'); | |
| console.log("scopes is "+ b) | |
| console.log("BASE URL is "+ BASE_URL) | |
| let b = `https://${resource}/.default offline_access`; | |
| console.log(`scopes is ${b}`); | |
| console.log(`BASE URL is ${BASE_URL}`); |
Tools
Biome
[error] 181-181: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 182-182: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
packages/shared/src/authUrl.ts
Outdated
| const state = encodeURIComponent(JSON.stringify({ projectId, linkedUserId, providerName, vertical, returnUrl })); | ||
| let state = encodeURIComponent(JSON.stringify({ projectId, linkedUserId, providerName, vertical, returnUrl })); | ||
| if (providerName == 'microsoftdynamicssales') { | ||
| state = encodeURIComponent(JSON.stringify({ projectId, linkedUserId, providerName, vertical, returnUrl, resource: additionalParams!.end_user_domain })); |
There was a problem hiding this comment.
Avoid non-null assertion.
Using non-null assertion (!) can lead to runtime errors. Replace it with optional chaining (?.) to ensure safety.
- state = encodeURIComponent(JSON.stringify({ projectId, linkedUserId, providerName, vertical, returnUrl, resource: additionalParams!.end_user_domain }));
+ state = encodeURIComponent(JSON.stringify({ projectId, linkedUserId, providerName, vertical, returnUrl, resource: additionalParams?.end_user_domain }));Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| state = encodeURIComponent(JSON.stringify({ projectId, linkedUserId, providerName, vertical, returnUrl, resource: additionalParams!.end_user_domain })); | |
| state = encodeURIComponent(JSON.stringify({ projectId, linkedUserId, providerName, vertical, returnUrl, resource: additionalParams?.end_user_domain })); |
Tools
Biome
[error] 60-60: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
| }); | ||
| this.logger.log('OAuth credentials updated : microsoftdynamicssales '); | ||
| } catch (error) { | ||
| throw error; |
There was a problem hiding this comment.
Remove redundant catch clause.
The catch clause that only rethrows the original error is redundant and can be removed.
- } catch (error) {
- throw error;
- }
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw error; | |
| } |
Tools
Biome
[error] 240-240: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
| config.linkedUserId, | ||
| ); | ||
| } catch (error) { | ||
| throw error; |
There was a problem hiding this comment.
Remove redundant catch clause.
The catch clause that only rethrows the original error is redundant and can be removed.
- } catch (error) {
- throw error;
- }
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw error; | |
| } |
Tools
Biome
[error] 93-93: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
| } | ||
| return db_res; | ||
| } catch (error) { | ||
| throw error; |
There was a problem hiding this comment.
Remove redundant catch clause.
The catch clause that only rethrows the original error is redundant and can be removed.
- } catch (error) {
- throw error;
- }
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw error; | |
| } |
Tools
Biome
[error] 189-189: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
| 'OAuth credentials : microsoftdynamicssales crm ' + | ||
| JSON.stringify(data), |
There was a problem hiding this comment.
Use template literals for string concatenation.
Template literals are preferred over string concatenation for better readability.
- 'OAuth credentials : microsoftdynamicssales crm ' +
- JSON.stringify(data),
+ `OAuth credentials : microsoftdynamicssales crm ${JSON.stringify(data)}`,Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'OAuth credentials : microsoftdynamicssales crm ' + | |
| JSON.stringify(data), | |
| `OAuth credentials : microsoftdynamicssales crm ${JSON.stringify(data)}`, |
Tools
Biome
[error] 134-135: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
| import axios from 'axios'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
| import { ServiceRegistry } from '../registry.service'; | ||
| import { URLSearchParams } from 'url'; |
There was a problem hiding this comment.
Use node: protocol for Node.js built-in modules.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
- import { URLSearchParams } from 'url';
+ import { URLSearchParams } from 'node:url';Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { URLSearchParams } from 'url'; | |
| import { URLSearchParams } from 'node:url'; |
Tools
Biome
[error] 25-25: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes