fix: #448 #481
fix: #448 #481
Conversation
|
|
@developerdhruv is attempting to deploy a commit to the Panora Team on Vercel. A member of the Team first needs to authorize it. |
|
|
|
💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe/Alipay. |
WalkthroughThe recent updates focus on integrating the Affinity CRM service into the existing system. Key changes include adding dependencies for NestJS and Axios, introducing new services for handling OAuth and contact synchronization with Affinity, and refining the structure and imports across various modules to support these functionalities. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant CRMSystem
participant AffinityConnectionService
participant AffinityService
participant HttpService
User->>CRMSystem: Initiates OAuth Callback
CRMSystem->>AffinityConnectionService: handleCallback()
AffinityConnectionService->>HttpService: POST OAuth token request
HttpService-->>AffinityConnectionService: Response with tokens
AffinityConnectionService-->>CRMSystem: Return Connection
User->>CRMSystem: Add Contact
CRMSystem->>AffinityService: addContact()
AffinityService->>HttpService: POST contact data
HttpService-->>AffinityService: Response with contact ID
AffinityService-->>CRMSystem: Return Contact ID
Poem
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 Configration File (
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/axios@1.6.8, npm/sharp@0.33.3 |
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is an install script?Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts. Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (3)
packages/api/src/crm/contact/services/contact.service.ts (3)
Line range hint
179-180: Refactor to use optional chaining for better readability and safety.- existingContact && existingContact.crm_email_addresses[index] + existingContact?.crm_email_addresses[index]Also applies to: 203-203, 227-227
Line range hint
444-446: ReplaceforEachwithfor...offor better performance and readability.- values.forEach((value) => { + for (const value of values) {Also applies to: 530-532
Line range hint
603-605: This code is unreachable. Please review the logic to ensure that all code paths are necessary and reachable.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (10)
- package.json (1 hunks)
- packages/api/src/@core/connections/crm/crm.connection.module.ts (3 hunks)
- packages/api/src/@core/connections/crm/services/affinity/affinityCrm.service.ts (1 hunks)
- packages/api/src/crm/contact/contact.module.ts (2 hunks)
- packages/api/src/crm/contact/services/affinity/index.ts (1 hunks)
- packages/api/src/crm/contact/services/affinity/mappers.ts (1 hunks)
- packages/api/src/crm/contact/services/affinity/types.ts (1 hunks)
- packages/api/src/crm/contact/services/attio/index.ts (1 hunks)
- packages/api/src/crm/contact/services/contact.service.ts (1 hunks)
- packages/api/src/crm/contact/types/mappingsTypes.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- packages/api/src/crm/contact/services/affinity/types.ts
Additional context used
Biome
packages/api/src/crm/contact/services/contact.service.ts
[error] 179-180: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 203-203: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 227-227: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 444-446: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 530-532: Prefer for...of instead of forEach. (lint/complexity/noForEach)
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 603-605: This code is unreachable (lint/correctness/noUnreachable)
Additional comments not posted (11)
package.json (2)
32-32: Dependency@nestjs/axiosadded correctly.
33-33: Dependencyaxiosadded correctly.packages/api/src/crm/contact/services/affinity/mappers.ts (1)
10-47: The implementation ofAffinityMapperlooks correct and adheres to theIContactMapperinterface requirements.packages/api/src/crm/contact/types/mappingsTypes.ts (1)
Line range hint
6-39: Integration ofAffinityMapperintocontactUnificationMappingis done correctly, ensuring proper method binding and context management.packages/api/src/crm/contact/contact.module.ts (1)
Line range hint
17-45:AffinityServicecorrectly added to the providers ofContactModule. This aligns with NestJS best practices for module configuration.packages/api/src/@core/connections/crm/crm.connection.module.ts (2)
Line range hint
2-20:HttpModulecorrectly added to the imports ofCrmConnectionModule. This is essential for enabling HTTP communication capabilities within the module.
Line range hint
3-36:AffinityConnectionServicecorrectly added to the providers ofCrmConnectionModule. This setup is crucial for managing CRM connections.packages/api/src/crm/contact/services/affinity/index.ts (1)
13-118: The implementation ofAffinityServiceis robust, featuring comprehensive error handling, logging, and proper use of HTTP services for contact management. This aligns well with the objectives of enhancing CRM functionalities.packages/api/src/@core/connections/crm/services/affinity/affinityCrm.service.ts (2)
26-38: Constructor properly initializes the service with necessary dependencies and registers the service. Good use of dependency injection for maintainability and testability.
40-72: Ensure proper error handling inhandleCallbackmethod.Verification successful
The search results indicate that there are numerous instances of
tryblocks across the codebase, suggesting that error handling is a common practice. However, to ensure proper error handling in thehandleCallbackmethod, we need to verify if the error handling in this specific method is consistent with the patterns used in other parts of the codebase.Given the extensive list of files containing
tryblocks, it would be more efficient to focus on a few representative examples to compare their error handling practices with thehandleCallbackmethod.
The extracted error handling patterns from the representative files show that they generally follow a consistent approach of logging the error and sometimes using a custom error handling function. Here are the key observations:
- Logging the Error: Most methods log the error using a logger instance.
- Custom Error Handling: Some methods use a custom error handling function like
handleServiceError.The
handleCallbackmethod inaffinityCrm.service.tsalready logs the error usingthis.logger.errorand throws a new error with a specific message. This approach is consistent with the patterns observed in other parts of the codebase.Observations:
- Logging: The
handleCallbackmethod logs the error, which is a common practice in the codebase.- Error Throwing: The method throws a new error with a specific message, which is also a reasonable approach.
Conclusion:
The error handling in the
handleCallbackmethod is consistent with the practices observed in other parts of the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper error handling in `handleCallback` method. # Test: Search for error handling patterns in similar methods across the codebase. rg --type python "try {"Length of output: 100
Script:
#!/bin/bash # Description: Verify proper error handling in `handleCallback` method. # Test: Search for error handling patterns in similar methods across the codebase. rg --type ts "try {"Length of output: 96
Script:
#!/bin/bash # Description: Verify proper error handling in `handleCallback` method. # Test: Search for error handling patterns in similar methods across the codebase. rg --type ts "try \{"Length of output: 36320
Script:
#!/bin/bash # Description: Extract error handling patterns from a few representative files for comparison. # Extracting error handling from user.controller.ts rg --context 5 "try \{" packages/api/src/ticketing/user/user.controller.ts # Extracting error handling from sync.service.ts rg --context 5 "try \{" packages/api/src/ticketing/user/sync/sync.service.ts # Extracting error handling from user.service.ts rg --context 5 "try \{" packages/api/src/ticketing/user/services/user.service.tsLength of output: 3158
packages/api/src/crm/contact/services/attio/index.ts (1)
9-21: Constructor sets up the service context and registers the service correctly. Good use of dependency injection.
| async handleTokenRefresh(opts: RefreshParams): Promise<any> { | ||
| const { connectionId, refreshToken } = opts; | ||
| const clientId = this.env.get('AFFINITY_CLIENT_ID'); | ||
| const clientSecret = this.env.get('AFFINITY_CLIENT_SECRET'); | ||
|
|
||
| try { | ||
| const tokenResponse = await lastValueFrom(this.httpService.post('https://api.affinity.co/oauth/token', { | ||
| grant_type: 'refresh_token', | ||
| refresh_token: refreshToken, | ||
| client_id: clientId, | ||
| client_secret: clientSecret, | ||
| })); | ||
|
|
||
| const { access_token, refresh_token, expires_in } = tokenResponse.data; | ||
|
|
||
| await this.prisma.connection.update({ | ||
| where: { id: connectionId }, | ||
| data: { | ||
| accessToken: access_token, | ||
| refreshToken: refresh_token, | ||
| expiresAt: new Date(Date.now() + expires_in * 1000), | ||
| }, | ||
| }); | ||
|
|
||
| return { access_token, refresh_token, expires_in }; | ||
| } catch (error) { | ||
| this.logger.error('Error refreshing token for Affinity', error); | ||
| throw new Error('Failed to refresh token for Affinity'); | ||
| } |
There was a problem hiding this comment.
The handleTokenRefresh method follows a similar pattern to handleCallback and properly uses async-await with try-catch for error handling. Consider adding more specific error messages to enhance debugging and user feedback.
- throw new Error('Failed to refresh token for Affinity');
+ throw new Error(`Failed to refresh token for Affinity: ${error.message}`);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.
| async handleTokenRefresh(opts: RefreshParams): Promise<any> { | |
| const { connectionId, refreshToken } = opts; | |
| const clientId = this.env.get('AFFINITY_CLIENT_ID'); | |
| const clientSecret = this.env.get('AFFINITY_CLIENT_SECRET'); | |
| try { | |
| const tokenResponse = await lastValueFrom(this.httpService.post('https://api.affinity.co/oauth/token', { | |
| grant_type: 'refresh_token', | |
| refresh_token: refreshToken, | |
| client_id: clientId, | |
| client_secret: clientSecret, | |
| })); | |
| const { access_token, refresh_token, expires_in } = tokenResponse.data; | |
| await this.prisma.connection.update({ | |
| where: { id: connectionId }, | |
| data: { | |
| accessToken: access_token, | |
| refreshToken: refresh_token, | |
| expiresAt: new Date(Date.now() + expires_in * 1000), | |
| }, | |
| }); | |
| return { access_token, refresh_token, expires_in }; | |
| } catch (error) { | |
| this.logger.error('Error refreshing token for Affinity', error); | |
| throw new Error('Failed to refresh token for Affinity'); | |
| } | |
| async handleTokenRefresh(opts: RefreshParams): Promise<any> { | |
| const { connectionId, refreshToken } = opts; | |
| const clientId = this.env.get('AFFINITY_CLIENT_ID'); | |
| const clientSecret = this.env.get('AFFINITY_CLIENT_SECRET'); | |
| try { | |
| const tokenResponse = await lastValueFrom(this.httpService.post('https://api.affinity.co/oauth/token', { | |
| grant_type: 'refresh_token', | |
| refresh_token: refreshToken, | |
| client_id: clientId, | |
| client_secret: clientSecret, | |
| })); | |
| const { access_token, refresh_token, expires_in } = tokenResponse.data; | |
| await this.prisma.connection.update({ | |
| where: { id: connectionId }, | |
| data: { | |
| accessToken: access_token, | |
| refreshToken: refresh_token, | |
| expiresAt: new Date(Date.now() + expires_in * 1000), | |
| }, | |
| }); | |
| return { access_token, refresh_token, expires_in }; | |
| } catch (error) { | |
| this.logger.error('Error refreshing token for Affinity', error); | |
| throw new Error(`Failed to refresh token for Affinity: ${error.message}`); | |
| } |
| "gitmoji-cli": "^9.0.0", | ||
| "nestjs": "^0.0.1", |
There was a problem hiding this comment.
Check the version of nestjs. The version ^0.0.1 seems incorrect, possibly a typo or a placeholder.
| async addContact( | ||
| contactData: AffinityContactInput, | ||
| linkedUserId: string, | ||
| ): Promise<ApiResponse<AffinityContactOutput>> { | ||
| // Implementation for adding a contact to Affinity CRM | ||
| // This should interact with Affinity CRM's API to add a contact | ||
| return; | ||
| } |
There was a problem hiding this comment.
The addContact method lacks implementation details. Ensure that the method interacts with the Affinity CRM's API as intended.
Please implement the interaction with Affinity CRM's API or provide a stub if this is a work in progress.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Added support for Affinity CRM, including OAuth callbacks and token refresh.
- Introduced
AffinityServicefor adding and syncing contacts. - Added
AffinityMapperfor converting data between unified contact and Affinity contact formats. - Updated dependencies: added
@nestjs/axios,axios, andnestjs. - Enhanced contact management with new methods for handling Affinity contacts.
| @@ -15,7 +17,7 @@ import { AttioConnectionService } from './services/attio/attio.service'; | |||
| import { ConnectionsStrategiesService } from '@@core/connections-strategies/connections-strategies.service'; | |||
|
|
|||
There was a problem hiding this comment.
Consider removing the trailing comma after HttpModule for consistency.
| @@ -0,0 +1,119 @@ | |||
| import { Injectable } from '@nestjs/common'; | |||
There was a problem hiding this comment.
Consider adding a newline at the end of the file for POSIX compliance.
| try { | ||
| const accessToken = await this.getAccessToken(linkedUserId); | ||
| const response = await lastValueFrom( | ||
| this.httpService.post('https://api.affinity.co/contacts', contactData, { |
There was a problem hiding this comment.
Use handleServiceError to manage errors consistently across services.
| ): Promise<ApiResponse<AffinityContactOutput[]>> { | ||
| try { | ||
| const accessToken = await this.getAccessToken(linkedUserId); | ||
| const response = await lastValueFrom( |
There was a problem hiding this comment.
Use handleServiceError to manage errors consistently across services.
| @@ -0,0 +1,48 @@ | |||
| import { Address } from '@crm/@lib/@types'; | |||
There was a problem hiding this comment.
Consider adding a newline at the end of the file.
| lastName: source.lastName, | ||
| email: source.email, | ||
| phone: source.phone, | ||
| company: source.company, | ||
| }; | ||
| } | ||
|
|
||
| unify( | ||
| source: AffinityContactOutput | AffinityContactOutput[], | ||
| customFieldMappings?: { | ||
| slug: string; | ||
| remote_id: string; | ||
| }[], | ||
| ): UnifiedContactOutput | UnifiedContactOutput[] { | ||
| if (Array.isArray(source)) { |
There was a problem hiding this comment.
Handle potential null or undefined values in source properties.
|
|
||
| export type AffinityContactInput = Partial<AffinityContact>; | ||
| export type AffinityContactOutput = AffinityContact; | ||
|
|
There was a problem hiding this comment.
Add a newline at the end of the file.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/api/src/@core/connections/crm/services/affinity/affinityCrm.service.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/api/src/@core/connections/crm/services/affinity/affinityCrm.service.ts
|
@rflihxyz |
Hey @developerdhruv ! Can you upload a loom video of you testing the integration end-to-end ? |
fix issue: #448
/claim #448
Summary by CodeRabbit
New Features
AffinityConnectionServiceto manage OAuth callbacks and token refresh for the Affinity CRM service.AffinityServiceto handle adding and syncing contacts with Affinity.AffinityMapperto convert data between unified contact and Affinity contact formats.Dependencies
@nestjs/axios,axios, andnestjsto the project dependencies.