fix: QBD fixes post testing #1108
Conversation
WalkthroughThe pull request introduces several modifications across multiple files. Key changes include updating the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (1)
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-main.component.ts (1)
51-52: Enhanced user feedback for QBD sync operationGood addition of QBD attributes import and user feedback via toast notification. Consider adding error handling for the subscription.
- this.qbdDirectHelperService.importQBDAttributes(true).subscribe(); + this.qbdDirectHelperService.importQBDAttributes(true).subscribe({ + error: (error) => { + this.toastService.displayToastMessage(ToastSeverity.ERROR, 'Failed to sync QBD dimensions'); + console.error('QBD sync error:', error); + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/app/core/models/db/expense-group.model.ts(1 hunks)src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-onboarding.model.ts(1 hunks)src/app/core/services/common/accounting-export.service.ts(1 hunks)src/app/core/services/common/export-log.service.ts(1 hunks)src/app/integrations/netsuite/netsuite-main/netsuite-main.component.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-main.component.ts(3 hunks)
🔇 Additional comments (8)
src/app/core/models/db/expense-group.model.ts (1)
35-35: LGTM: Type enhancement improves flexibility
The change from string to string | string[] allows for more flexible status filtering while maintaining backward compatibility. This enhancement supports querying multiple statuses, which is particularly useful for QBD's error handling requirements.
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-main.component.ts (2)
6-10: LGTM: Clean import organization
The new imports are properly organized and follow Angular's best practices.
37-40: LGTM: Proper dependency injection
The new services are correctly injected following Angular's dependency injection pattern.
src/app/integrations/netsuite/netsuite-main/netsuite-main.component.ts (1)
36-36: LGTM: Fixed service name typo
Corrected the typo in the service name from 'toastServeice' to 'toastService', maintaining consistency across components.
Also applies to: 43-43
src/app/core/models/qbd-direct/qbd-direct-configuration/qbd-direct-onboarding.model.ts (1)
29-29: Route paths updated from QBO to QBD Direct
The route paths have been correctly updated from /integrations/qbo/onboarding/ to /integrations/qbd_direct/onboarding/ across all onboarding steps.
Let's verify no QBO routes remain in the codebase for QBD:
Also applies to: 37-37, 45-45, 53-53, 61-61
✅ Verification successful
Let me analyze the search results. The output shows that QBO (QuickBooks Online) routes are present in their own dedicated module and components under src/app/integrations/qbo/. These routes are correctly separated from the QBD (QuickBooks Desktop) routes that were updated in the review. The QBO routes found in the search results are legitimate and expected since they belong to a different integration module.
No incorrect QBO routes found in QBD Direct module
The route path changes from /integrations/qbo/onboarding/ to /integrations/qbd_direct/onboarding/ in the QBD Direct module are correct and do not conflict with the existing QBO routes. Each integration module maintains its own separate routing structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining QBO routes that might be related to QBD
rg -i "qbo.*onboarding" --type ts
Length of output: 41942
src/app/core/services/common/export-log.service.ts (1)
89-89: Status mapping expanded to include FATAL errors
The status mapping for QBD Direct now correctly handles both ERROR and FATAL statuses when FAILED status is encountered. This change aligns with the error handling in AccountingExportService.
Let's verify the consistency of status handling across the codebase:
✅ Verification successful
The search results show consistent handling of FAILED and FATAL statuses across the codebase. The pattern of replacing FAILED with [ERROR, FATAL] is consistently implemented in both ExportLogService and AccountingExportService. Additionally, other components like Sage300Dashboard and BusinessCentralDashboard correctly handle both FAILED and FATAL statuses in their filtering logic.
Status handling is consistent across the codebase
The change in ExportLogService aligns with the existing error status handling patterns throughout the codebase, where FAILED and FATAL statuses are handled together in filtering and processing logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of ERROR or FATAL status handling
rg "AccountingExportStatus\.(ERROR|FATAL|FAILED)" --type ts
Length of output: 3403
src/app/core/services/common/accounting-export.service.ts (2)
76-76: Status mapping aligned with ExportLogService
The status mapping for QBD Direct now correctly handles both ERROR and FATAL statuses when FAILED status is encountered, maintaining consistency with ExportLogService.
78-78: Verify removal of type__in parameter
The type__in parameter is being removed for QBD Direct requests. Please confirm if QBD Direct export logs don't require type filtering.
Let's check if type filtering is used elsewhere for QBD Direct:
✅ Verification successful
Based on the code context found, I can now provide a final response:
Type filtering is intentionally removed for QBD Direct export logs
The code explicitly removes type filtering (type__in) for QBD Direct export logs by design. This is handled in the getAccountingExports method where there's a specific condition for QBD Direct that removes the type filter and uses a different API endpoint (/export_logs/) compared to other integrations. This indicates that type filtering is not required for QBD Direct export logs as they are handled differently from other accounting exports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for type filtering in QBD Direct related code
rg "type__in.*QBD" --type ts
Length of output: 30
Script:
#!/bin/bash
# Let's search for any usage of type__in in the codebase
rg "type__in" --type ts -B 2 -A 2
# Also search for QBD related export code
rg "QBD.*export" --type ts -B 2 -A 2
Length of output: 26086
Script:
#!/bin/bash
# Let's search for QBD Direct export logs related code to understand the type filtering
rg "QBD.*Direct.*export.*log" --type ts -B 3 -A 3
# Also search for any QBD Direct specific type filtering
rg "type.*filter.*QBD.*Direct" --type ts -B 3 -A 3
Length of output: 104
Script:
#!/bin/bash
# Let's look at the context around where type__in is being deleted
rg -B 10 -A 10 "delete apiParams.type__in" --type ts
# Also check for any QBD Direct specific export log related code
rg "QBDDirect.*export" --type ts -B 3 -A 3
Length of output: 1996
🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
|
|
||
| refreshDimensions() { | ||
| this.workspaceService.importFyleAttributes(true).subscribe(); | ||
| this.qbdDirectHelperService.importQBDAttributes(true).subscribe(); |
There was a problem hiding this comment.
we have a helper service for this, @ashwin1111 we can use them
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-helper.service.ts (1)
Line range hint
23-27: Add error handling and improve subscription managementWhile the method name correction is good, there are some improvements needed in the implementation:
- Add error handling for API calls
- Show success message only after both operations complete
- Properly manage subscriptions to prevent memory leaks
Here's a suggested implementation using RxJS operators:
- importAttributes(refresh: boolean): void { - this.workspaceService.importFyleAttributes(refresh).subscribe(); - this.importQBDAttributes(refresh).subscribe(); - this.toastService.displayToastMessage(ToastSeverity.SUCCESS, 'Syncing data dimensions from Quickbooks Desktop'); - } + importAttributes(refresh: boolean): void { + forkJoin([ + this.workspaceService.importFyleAttributes(refresh), + this.importQBDAttributes(refresh) + ]).pipe( + take(1) + ).subscribe({ + next: () => { + this.toastService.displayToastMessage(ToastSeverity.SUCCESS, 'Successfully synced data dimensions from Quickbooks Desktop'); + }, + error: () => { + this.toastService.displayToastMessage(ToastSeverity.ERROR, 'Failed to sync data dimensions from Quickbooks Desktop'); + } + }); + }Don't forget to add these imports:
import { forkJoin, take } from 'rxjs';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/app/core/services/qbd-direct/qbd-direct-core/qbd-direct-helper.service.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-main.component.ts(3 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.ts
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-main.component.ts
* fix: QBD fixes post testing * fix
2 participants
Description
Please add PR description here, add screenshots if needed
Clickup
https://app.clickup.com/
Summary by CodeRabbit
New Features
status__inparameter to accept multiple status values.Bug Fixes
Improvements