fix: QBD Dashboard bugs #1103
fix: QBD Dashboard bugs #1103
Conversation
ashwin1111
added
staging_deploy
ashwin1111
added
staging_deploy
ashwin1111
added
staging_deploy
|
Warning Rate limit exceeded@ashwin1111 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes in this pull request involve modifications to several TypeScript files and an HTML template within the application. Key updates include adjustments to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 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: 1
🧹 Outside diff range and nitpick comments (3)
src/app/core/services/common/accounting-export.service.ts (1)
75-78: Document the status mapping for QBD DirectThe status mapping from
FAILEDtoERRORfor QBD Direct appears to handle a specific case differently. Consider adding a comment explaining why this mapping is necessary for better maintainability.+ // QBD Direct uses ERROR status instead of FAILED status for consistency with its error handling if (apiParams.status__in?.includes(AccountingExportStatus.FAILED)) { apiParams.status__in = AccountingExportStatus.ERROR; }Additionally, consider using undefined assignment instead of delete operator for better performance:
- delete apiParams.type__in; + apiParams.type__in = undefined;🧰 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)
src/app/core/services/common/dashboard.service.ts (1)
Line range hint
39-46: Add JSDoc to document the method parametersThe method signature has been updated to include an optional
appNameparameter. Consider adding JSDoc to document the parameters and their purpose.+ /** + * Get export errors based on version and app name + * @param version - Optional version string, defaults to latest + * @param appName - Optional app name to handle app-specific error fetching + * @returns Observable containing error response + */ getExportErrors(version?: string | 'v1', appName?: string): Observable<any> {src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts (1)
97-98: Consider making the polling interval configurableThe polling interval is currently hardcoded to 3000ms. Consider making this configurable through a constant or environment variable to allow easier adjustments for different environments.
+ private readonly EXPORT_STATUS_POLLING_INTERVAL = 3000; // ms interval(3000).pipe(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/app/core/models/db/accounting-export.model.ts(1 hunks)src/app/core/services/common/accounting-export.service.ts(1 hunks)src/app/core/services/common/dashboard.service.ts(2 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.html(0 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts(4 hunks)src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.html
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/services/common/accounting-export.service.ts
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (5)
src/app/core/services/common/dashboard.service.ts (1)
66-66: LGTM!
The simplified condition for typeKey assignment improves readability while maintaining the same logic.
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts (3)
55-55: LGTM!
The explicit undefined parameter makes it clear that we're using the app-specific error handling for QBD Direct.
134-135: Good addition of error handling
The addition of catchError operator for getAccountingExportSummary$ improves resilience by preventing crashes when the summary cannot be retrieved.
98-98: Verify the impact of filtering and tracking changes
Two significant changes have been made:
- Removal of accountingExportType filter from getAllTasks
- Changed mapping to use taskLog.id instead of taskLog.expense_group
Please verify that:
- Removing the export type filter doesn't result in processing unnecessary tasks
- Using taskLog.id correctly identifies failed tasks for retry
Also applies to: 121-121
✅ Verification successful
Based on the search results, I can now generate the final response:
Changes to task filtering and tracking are safe
- The removal of accountingExportType filter from getAllTasks is intentional as the filtering is now handled by the exportableAccountingExportIds parameter which contains the specific IDs to process
- The change from expense_group to id for tracking failed tasks is consistent with the codebase - both fields are used to track task failures across different integrations, with id being the more direct reference
The changes maintain proper task filtering and tracking while simplifying the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any potential task types that might be missed
ast-grep --pattern 'getAllTasks($$$, $$$, $accountingExportType, $$$)' | grep -B 2 -A 2 accountingExportType
# Check for references to expense_group in task logs
rg -A 2 'expense_group' --type ts
Length of output: 74080
src/app/core/models/db/accounting-export.model.ts (1)
51-52: LGTM! Verify usage of modified parameters across the codebase.
The type changes enhance flexibility:
- Making
type__inoptional accommodates cases where type filtering isn't needed - Allowing
status__into accept bothstring[]andstringsupports varied filtering scenarios
Let's verify that all usages of AccountingExportGetParam have been updated to handle these changes:
✅ Verification successful
Let me search for the interface usage and parameter patterns to verify the impact of these changes.
The changes to AccountingExportGetParam are safely handled across the codebase
The verification shows that all usages of these parameters are compatible with the type changes:
- The optional
type__inis correctly handled, with code even explicitly removing it in some cases (e.g.,delete apiParams.type__infor QBD_DIRECT) status__inis used consistently with both array and string types, with proper type handling in conditional checks (e.g.,status__in?.includes())
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of AccountingExportGetParam to ensure they handle optional type__in
ast-grep --pattern 'type__in: $$$'
# Search for status__in assignments to ensure they handle both string and string[]
ast-grep --pattern 'status__in: $$$'
Length of output: 75
Script:
#!/bin/bash
# Search for files containing AccountingExportGetParam
rg -l "AccountingExportGetParam"
# Then search for actual usage patterns of these parameters
rg "type__in|status__in" -A 2 -B 2
Length of output: 8976
|
|
||
| handleResolvedMappingStat(): void { | ||
| this.dashboardService.getExportErrors(this.errorsVersion).subscribe((errors) => { | ||
| this.dashboardService.getExportErrors(this.errorsVersion, this.appName).subscribe((errors) => { |
There was a problem hiding this comment.
💡 Codebase verification
Update required: Inconsistent usage of getExportErrors parameters across components
Several components need to be updated to align with the new getExportErrors signature:
intacct-dashboard.component.ts: Only passes version parameterxero-dashboard.component.ts: Only passes version parameterqbo-dashboard.component.ts: Only passes version parameterbusiness-central-dashboard.component.ts: No parameters passedsage300-dashboard.component.ts: No parameters passednetsuite-dashboard.component.ts: Only passes version parameter
Only qbd-direct-dashboard.component.ts and dashboard-error-section.component.ts are using both parameters correctly.
🔗 Analysis chain
LGTM! Verify error handling for different app types.
The addition of this.appName parameter to getExportErrors call aligns with the service's updated signature. This change enables app-specific error handling.
Let's verify that all components using DashboardService.getExportErrors have been updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to getExportErrors to ensure consistent parameter usage
ast-grep --pattern 'this.dashboardService.getExportErrors($$$)'
Length of output: 1592
ashwin1111
added
staging_deploy
ashwin1111
added
staging_deploy
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/app/core/services/common/dashboard.service.ts (2)
39-42: Consider simplifying the version and appName conditionsThe error handling logic looks correct but could be more maintainable.
Consider this refactoring:
- getExportErrors(version?: string | 'v1', appName?: string): Observable<any> { - if (!appName && version === 'v1') { - return this.apiService.get(`/v2/workspaces/${this.workspaceId}/errors/`, {is_resolved: false}); - } else if (appName === AppName.QBD_DIRECT) { + getExportErrors(version?: string | 'v1', appName?: string): Observable<any> { + const params = { is_resolved: false }; + + if (appName === AppName.QBD_DIRECT) { + return this.apiService.get(`/workspaces/${this.workspaceId}/export_logs/errors/`, params); + } else if (version === 'v1') { + return this.apiService.get(`/v2/workspaces/${this.workspaceId}/errors/`, params); + }
66-66: Consider using a constant for the type key mappingThe type key mapping logic could be more maintainable.
Consider extracting the mapping to a constant:
private readonly TYPE_KEY_MAP = { [AppName.INTACCT]: 'task_type', DEFAULT: 'type__in' } as const; // Then in the code: const typeKey = this.TYPE_KEY_MAP[appName as AppName] || this.TYPE_KEY_MAP.DEFAULT;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/app/core/models/db/accounting-export.model.ts(1 hunks)src/app/core/models/enum/enum.model.ts(1 hunks)src/app/core/services/common/accounting-export.service.ts(1 hunks)src/app/core/services/common/dashboard.service.ts(2 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts(5 hunks)src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app/core/models/enum/enum.model.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/core/services/common/accounting-export.service.ts
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (5)
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.html (1)
46-47: LGTM! Verify error handling behavior
The addition of errorsVersion and exportKey attributes aligns with the service layer changes for QBD-specific error handling.
Let's verify the error handling configuration is consistent across the codebase:
✅ Verification successful
Consistent error handling configuration found across integrations
The verification shows that:
- All integrations consistently use
errorsVersion="v1"where error versioning is implemented exportKeyis appropriately customized per integration:- QBD Direct: "export_log"
- Business Central & Sage300: "accounting_export"
- Default (in component): "expense_group"
The implementation in the dashboard-error-section component properly handles these configurations through @Input() decorators and uses them correctly in the error handling logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of dashboard-error-section to ensure consistent version handling
rg -l 'app-dashboard-error-section' | xargs rg 'errorsVersion|exportKey'
Length of output: 1961
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts (2)
55-55: Update required: Inconsistent usage of getExportErrors parameters across components
Several components need to be updated to align with the new getExportErrors signature:
intacct-dashboard.component.ts: Only passes version parameterxero-dashboard.component.ts: Only passes version parameterqbo-dashboard.component.ts: Only passes version parameterbusiness-central-dashboard.component.ts: No parameters passedsage300-dashboard.component.ts: No parameters passednetsuite-dashboard.component.ts: Only passes version parameter
119-121: LGTM! Improved error handling by including fatal errors
The error handling has been enhanced by including both TaskLogState.ERROR and TaskLogState.FATAL states. This ensures that all types of errors are properly captured and handled.
src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts (1)
213-213: LGTM! Proper implementation of app-specific error handling
The addition of this.appName parameter to getExportErrors call aligns with the service's updated signature. This change enables proper app-specific error handling.
src/app/core/models/db/accounting-export.model.ts (1)
51-52: LGTM! Enhanced type flexibility
The type improvements make the interface more flexible and practical:
- Making
type__inoptional allows for cases where the type filter isn't needed - Allowing
status__into accept either an array or single string accommodates different usage patterns
| if (apiParams.status__in?.includes(AccountingExportStatus.FAILED)) { | ||
| apiParams.status__in = AccountingExportStatus.ERROR; | ||
| } | ||
| delete apiParams.type__in; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error status handling and avoid delete operator
The status mapping for QBD_DIRECT looks correct, but there are a few improvements we can make:
Consider this refactoring to improve performance and readability:
- if (apiParams.status__in?.includes(AccountingExportStatus.FAILED)) {
- apiParams.status__in = AccountingExportStatus.ERROR;
- }
- delete apiParams.type__in;
+ // Map FAILED status to ERROR for QBD_DIRECT
+ apiParams.status__in = Array.isArray(apiParams.status__in)
+ ? apiParams.status__in.map(status => status === AccountingExportStatus.FAILED ? AccountingExportStatus.ERROR : status)
+ : apiParams.status__in;
+ apiParams.type__in = undefined;This change:
- Handles both array and string status values
- Avoids the delete operator which can impact performance
- Makes the status mapping more explicit
📝 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.
| if (apiParams.status__in?.includes(AccountingExportStatus.FAILED)) { | |
| apiParams.status__in = AccountingExportStatus.ERROR; | |
| } | |
| delete apiParams.type__in; | |
| // Map FAILED status to ERROR for QBD_DIRECT | |
| apiParams.status__in = Array.isArray(apiParams.status__in) | |
| ? apiParams.status__in.map(status => status === AccountingExportStatus.FAILED ? AccountingExportStatus.ERROR : status) | |
| : apiParams.status__in; | |
| apiParams.type__in = undefined; |
🧰 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)
ashwin1111
added
staging_deploy
|
* fix: QBD Dashboard bugs * fix: QBD Dashboard bugs * fix: QBD Dashboard bugs * fix: QBD Dashboard bugs * fix: QBD Dashboard bugs * fix: QBD Dashboard bugs * fix: QBD Dashboard bugs
2 participants
Description
Please add PR description here, add screenshots if needed
Clickup
https://app.clickup.com/t
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores