feat: QBD direct tracker update #1126
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces enhancements to the QuickBooks Desktop (QBD) Direct integration by adding a new enum Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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 using PR comments)
Other keywords and placeholders
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: 2
🧹 Nitpick comments (5)
src/app/core/services/integration/tracking.service.ts (2)
106-107: Consider using discriminated union type for additionalProperties.The method signature uses a union type for
additionalPropertiesthat includesvoid. This could lead to type-safety issues.Consider using a discriminated union type or making the parameter optional:
-additionalProperties: LocationEntityPost | ExportSettingPost | ImportSettingPost | AdvancedSettingsPost | NetsuiteSubsidiaryMappingPost | QbdDirectExportSettingsPost | QbdDirectImportSettingPost | QbdDirectAdvancedSettingsPost | void +additionalProperties?: LocationEntityPost | ExportSettingPost | ImportSettingPost | AdvancedSettingsPost | NetsuiteSubsidiaryMappingPost | QbdDirectExportSettingsPost | QbdDirectImportSettingPost | QbdDirectAdvancedSettingsPost🧰 Tools
🪛 Biome (1.9.4)
[error] 106-106: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
110-111: Consider using discriminated union type for additionalProperties.Similar to the previous comment, the method signature uses a union type with
void.Consider using a discriminated union type or making the parameter optional:
-additionalProperties: Partial<UpdateEventAdditionalProperty> | void +additionalProperties?: Partial<UpdateEventAdditionalProperty>🧰 Tools
🪛 Biome (1.9.4)
[error] 110-110: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.ts (1)
223-238: Consider handling edge cases in tracking logic.While the tracking implementation is generally correct, there are a few considerations:
- The oldState modification might cause unintended side effects.
- No error handling for tracking calls.
Consider this safer implementation:
-const oldWorkspaceResponse = workspaceResponse; -oldWorkspaceResponse.onboarding_state = QbdDirectOnboardingState.DESTINATION_SYNC_COMPLETE; +const oldWorkspaceResponse = { + ...workspaceResponse, + onboarding_state: QbdDirectOnboardingState.DESTINATION_SYNC_COMPLETE +};src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts (1)
186-199: Consider handling tracking errors gracefully.The tracking calls are made within the success callback of the HTTP request, but there's no error handling for the tracking calls themselves.
Consider wrapping tracking calls in try-catch:
-this.trackingService.trackTimeSpent(TrackingApp.QBD_DIRECT, Page.EXPORT_SETTING_QBD_DIRECT, this.sessionStartTime); +try { + this.trackingService.trackTimeSpent(TrackingApp.QBD_DIRECT, Page.EXPORT_SETTING_QBD_DIRECT, this.sessionStartTime); + if (this.workspaceService.getOnboardingState() === QbdDirectOnboardingState.EXPORT_SETTINGS) { + this.trackingService.integrationsOnboardingCompletion(TrackingApp.QBD_DIRECT, QbdDirectOnboardingState.EXPORT_SETTINGS, 3, exportSettingPayload); + } else { + this.trackingService.onUpdateEvent( + TrackingApp.QBD_DIRECT, + QbdDirectUpdateEvent.EXPORT_SETTING_QBD_DIRECT, + { + phase: this.getPhase(), + oldState: this.exportSettings, + newState: response + } + ); + } +} catch (error) { + console.error('Error tracking export settings:', error); +}src/app/core/models/enum/enum.model.ts (1)
137-141: Consider grouping related enum values together.While the additions to the Page enum are correct and maintain consistency, consider grouping all QBD Direct-related pages together for better maintainability. This would make it easier to manage and understand related functionality.
export enum Page { LANDING = 'Landing', BAMBOO_HR_LANDING = 'Bamboo HR Landing', CONNECT_BAMBOO_HR = 'Connect Bamboo HR', CONFIGURE_BAMBOO_HR = 'Bamboo HR Configuration', QBD_LANDING = 'QuickBooks Desktop Landing', INTACCT_LANDING = 'Sage Intacct Landing', + // QuickBooks Desktop Direct pages + CONFIRM_PRE_REQUISITES_QBD_DIRECT = 'Confirm Pre Requisites QuickBooks Desktop Connector', + CONNECT_QBD_DIRECT = 'Connect QuickBooks Desktop Connector', + EXPORT_SETTING_QBD_DIRECT = 'Export Settings QuickBooks Desktop Connector', + IMPORT_SETTINGS_QBD_DIRECT = 'Import Settings QuickBooks Desktop Connector', + ADVANCED_SETTINGS_QBD_DIRECT = 'Advanced Settings QuickBooks Desktop Connector', // ... rest of the enum values }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/app/core/models/enum/enum.model.ts(2 hunks)src/app/core/models/misc/tracking.model.ts(1 hunks)src/app/core/services/integration/tracking.service.ts(3 hunks)src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.ts(4 hunks)src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-pre-requisite/qbd-direct-onboarding-pre-requisite.component.ts(3 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.ts(3 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts(3 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/services/integration/tracking.service.ts
[error] 106-106: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 110-110: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (12)
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-pre-requisite/qbd-direct-onboarding-pre-requisite.component.ts (1)
83-98: Tracking Integration Implemented Correctly
The added tracking code is correctly implemented and aligns with the intended functionality.
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.ts (2)
153-156: Helper Method getPhase() Implemented Correctly
The getPhase() method correctly determines the progress phase based on the onboarding state.
163-177: Tracking Code in save() Method Implemented Correctly
The added tracking code in the save() method correctly logs time spent and tracks onboarding completion or updates.
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.ts (2)
240-243: Helper Method getPhase() Implemented Correctly
The getPhase() method correctly determines the progress phase based on the onboarding state.
248-262: Tracking Code in save() Method Implemented Correctly
The added tracking code in the save() method correctly logs time spent and tracks onboarding completion or updates.
src/app/core/models/misc/tracking.model.ts (1)
23-24: Extended UpdateEventAdditionalProperty Type Definition Correctly
The UpdateEventAdditionalProperty type has been correctly extended to include the new QbdDirect types and Workspace.
src/app/core/services/integration/tracking.service.ts (1)
2-2: LGTM: Import statement updated correctly.
The import statement has been properly updated to include the new QBD Direct related enums (QbdDirectOnboardingState, QbdDirectUpdateEvent).
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.ts (2)
6-6: LGTM: Imports properly organized.
The imports for tracking-related functionality have been correctly added.
Also applies to: 21-21
78-79: LGTM: Session tracking setup.
The sessionStartTime property and TrackingService injection are properly implemented.
Also applies to: 85-86
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts (2)
4-4: LGTM: Imports properly organized.
The imports for tracking-related functionality have been correctly added.
86-87: LGTM: Session tracking setup.
The sessionStartTime property is properly initialized.
src/app/core/models/enum/enum.model.ts (1)
85-91: LGTM! The QbdDirectUpdateEvent enum is well-structured.
The enum follows consistent naming conventions and provides clear, descriptive values for tracking QBD Direct integration events.
| const oldWorkspaceResponse = workspaceResponse; | ||
| oldWorkspaceResponse.onboarding_state = QbdDirectOnboardingState.CONFIRM_PRE_REQUISITES; | ||
| this.trackingService.onUpdateEvent( |
There was a problem hiding this comment.
Modify oldWorkspaceResponse Without Affecting workspaceResponse to Prevent Unintended Side Effects
Assigning oldWorkspaceResponse to workspaceResponse creates a reference to the same object. Modifying oldWorkspaceResponse.onboarding_state will also modify workspaceResponse.onboarding_state, which may not be intended. To prevent unintended side effects, create a copy of workspaceResponse before modifying.
Apply this diff to fix the issue:
- const oldWorkspaceResponse = workspaceResponse;
+ const oldWorkspaceResponse = { ...workspaceResponse };📝 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.
| const oldWorkspaceResponse = workspaceResponse; | |
| oldWorkspaceResponse.onboarding_state = QbdDirectOnboardingState.CONFIRM_PRE_REQUISITES; | |
| this.trackingService.onUpdateEvent( | |
| const oldWorkspaceResponse = { ...workspaceResponse }; | |
| oldWorkspaceResponse.onboarding_state = QbdDirectOnboardingState.CONFIRM_PRE_REQUISITES; | |
| this.trackingService.onUpdateEvent( |
| private getPhase(): ProgressPhase { | ||
| return this.isOnboarding ? ProgressPhase.ONBOARDING : ProgressPhase.POST_ONBOARDING; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding type safety to isOnboarding.
The getPhase method uses isOnboarding which is typed as any.
Add proper typing to improve type safety:
-isOnboarding: any;
+isOnboarding: boolean;Committable suggestion skipped: line range outside the PR's diff.
| @@ -82,6 +82,14 @@ export enum TravelperkUpdateEvent { | |||
| ADVANCED_SETTINGS_TRAVELPERK = 'Advanced Settings TravelPerk', | |||
| } | |||
|
|
|||
| export enum QbdDirectUpdateEvent { | |||
| CONFIRM_PRE_REQUISITES_QBD_DIRECT = 'Confirm Pre Requisites QuickBooks Desktop Connector', | |||
| CONNECT_QBD_DIRECT = 'Connect QuickBooks Desktop Connector', | |||
There was a problem hiding this comment.
let's have all 3 sub steps in 2nd step as trackers
|
Description
feat: QBD direct tracker update
Clickup
https://app.clickup.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation