fix: QBD direct bug fixes #1085
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on updating parameter naming conventions and enhancing enums for better state representation. The Changes
Possibly related PRs
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 (2)
src/app/core/services/common/export-log.service.ts (1)
Line range hint
27-54: Consider consistent error handling across methodsThe
getSkippedExpensesmethod might benefit from similar FAILED to ERROR status conversion for QBD direct cases to maintain consistency withgetExpenseGroups.src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts (1)
Line range hint
1-180: Consider architectural improvements for better maintainability.A few suggestions to enhance the code:
- Consider extracting the polling logic into a separate service to improve reusability and testability.
- The response array indices in
setupPageare magic numbers. Consider destructuring the response array or using named constants for better maintainability.- The error handling could be more robust by implementing retry logic for failed API calls.
Example of response destructuring:
forkJoin({ exportErrors: this.getExportErrors$, accountingExport: this.getAccountingExportSummary$.pipe(catchError(() => of(null))), tasks: this.dashboardService.getAllTasks(...), exportIds: this.dashboardService.getExportableAccountingExportIds('v2'), exportSettings: this.QbdDirectExportSettingsService.getQbdExportSettings(), importSettings: this.importSettingService.getImportSettings() }).subscribe(({exportErrors, accountingExport, tasks, exportIds, exportSettings, importSettings}) => { // Use named properties instead of array indices });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/app/core/models/db/task-log.model.ts(1 hunks)src/app/core/models/enum/enum.model.ts(1 hunks)src/app/core/services/common/dashboard.service.ts(1 hunks)src/app/core/services/common/export-log.service.ts(2 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts(2 hunks)
🔇 Additional comments (8)
src/app/core/models/db/task-log.model.ts (2)
26-26: Verify consistent parameter naming across the codebase
The parameter name change from export_log_id__in to export_log__id appears to be part of the QBD bug fixes. Let's ensure all consumers are updated accordingly.
✅ Verification successful
Parameter naming change is correctly implemented
The verification shows that:
- No instances of the old parameter name
export_log_id__inexist in the codebase - The new parameter
export_log__idis used consistently in:- The type definition in
task-log.model.ts - The dashboard service where it's specifically used for QBD Direct app context
- The type definition in
The change appears to be part of a deliberate API parameter naming strategy where different parameter names are used based on the app context (Intacct vs QBD Direct).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usages of the old parameter name
# and verify the new parameter name is used consistently
echo "Checking for old parameter name usage..."
rg "export_log_id__in" -l
echo "Verifying new parameter usage..."
rg "export_log__id" -A 2 -B 2
Length of output: 967
26-26: Confirm API compatibility with backend
Since this parameter is likely used in API requests, we should verify that the backend API expects this new parameter name.
Consider adding a comment to document this parameter's purpose and expected format, especially if it represents a breaking change in the API contract.
✅ Verification successful
API compatibility verified - parameter name is correct
The search results confirm that export_log__id is the correct parameter name used consistently across the codebase, particularly in the QBD Direct integration. This is evidenced by:
- The parameter is used in API requests in
dashboard.service.ts:export_log__idis explicitly used as the parameter name for QBD Direct application - The endpoint
/workspaces/${workspaceId}/export_logs/is consistently used across multiple services - The export log functionality is properly integrated into the API layer through
export-log.service.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for API endpoint definitions or documentation
# that might reference this parameter
echo "Searching for API endpoint definitions..."
fd -e ts -e py | xargs rg -l "export_log.*id.*="
echo "Checking for API documentation..."
fd "api|swagger|openapi" --type f | xargs rg -l "export_log"
Length of output: 281
Script:
#!/bin/bash
# Let's try a broader search to understand the context and usage of export_log
# First, let's find all files containing "export_log"
echo "Files containing export_log:"
rg -l "export_log"
# Then, let's look for API-related files and endpoints
echo -e "\nAPI-related files:"
fd -e service.ts -e controller.ts -e api.ts
# Let's also check for any HTTP request implementations
echo -e "\nHTTP requests containing export_log:"
rg -B2 -A2 "export_log" -g "*.service.ts" -g "*.api.ts"
Length of output: 13474
src/app/core/services/common/export-log.service.ts (2)
6-6: LGTM!
The addition of AccountingExportStatus import is necessary for the new error handling logic.
88-91:
Verify the status replacement logic
The current implementation replaces all statuses with ERROR when FAILED is present, which might inadvertently remove other valid statuses. Consider preserving other statuses while only replacing FAILED with ERROR.
Consider this alternative approach:
- if (params.status__in?.includes(AccountingExportStatus.FAILED)) {
- params.status__in = AccountingExportStatus.ERROR;
- }
+ if (Array.isArray(params.status__in)) {
+ params.status__in = params.status__in.map(status =>
+ status === AccountingExportStatus.FAILED ? AccountingExportStatus.ERROR : status
+ );
+ }Let's verify the impact:
✅ Verification successful
Based on the search results, I can now provide a final response:
The status replacement logic is correct and intentional
The codebase shows that FAILED and ERROR statuses are handled differently across different integrations:
- For QBD Direct specifically (where this change is made), FAILED is converted to ERROR
- Other integrations like Sage300 and Business Central handle FAILED alongside FATAL status
- The conversion is specific to the QBD Direct export logs endpoint only
The current implementation is correct because:
- It's intentionally scoped to QBD Direct integration only
- The status conversion is a complete replacement rather than a mapping, which aligns with how QBD Direct specifically handles these statuses
- Other integrations preserve their existing FAILED status handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other usages of AccountingExportStatus.FAILED
# to ensure we're handling all cases consistently
# Search for FAILED status usage
rg "AccountingExportStatus.FAILED" -A 5 -B 5
# Search for status__in assignments
ast-grep --pattern 'status__in = $_'
Length of output: 17127
src/app/core/services/common/dashboard.service.ts (1)
61-61: LGTM! Consider adding documentation for parameter naming.
The parameter name change from 'export_log_id__in' to 'export_log__id' for QBD_DIRECT aligns with the PR objectives and should fix the QBD direct integration issues.
Consider adding a comment explaining the different parameter naming conventions for each app type to improve maintainability:
+ // Parameter naming conventions:
+ // - QBD_DIRECT: export_log__id
+ // - INTACCT: expense_group_ids
+ // - Others: expense_group_id__in
const expenseKey = appName === AppName.INTACCT ? 'expense_group_ids' : appName === AppName.QBD_DIRECT ? 'export_log__id' : 'expense_group_id__in';Let's verify the API contract for the QBD direct integration:
✅ Verification successful
Parameter naming change is consistent across the codebase
The verification confirms that:
- The new parameter name
export_log__idis used consistently in both the dashboard service and the task-log model - No occurrences of the old parameter name
export_log_id__inwere found
The change is properly implemented and maintains consistency across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the API parameter naming in the codebase
# Expected: The new parameter name should be consistently used across the codebase
# Check for any remaining occurrences of the old parameter name
rg "export_log_id__in" --type ts
# Check for consistent usage of the new parameter name
rg "export_log__id" --type ts
Length of output: 335
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts (2)
146-146: Verify the response array structure change.
The employee field mapping index changed from responses[3] to responses[4], which suggests a change in the API response structure. This change needs to be verified against the forkJoin order in the setupPage method.
✅ Verification successful
Let me gather more information about the forkJoin structure to verify the response array indices.
The index change to responses[4] is correct and consistent with the forkJoin structure
The setupPage() method shows that getQbdExportSettings() is the fifth item in the forkJoin array (index 4), which correctly corresponds to accessing responses[4].employee_field_mapping. The order is:
getExportErrors$[0]getAccountingExportSummary$[1]getAllTasks()[2]getExportableAccountingExportIds()[3]getQbdExportSettings()[4]getImportSettings()[5]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for other instances of this response array access to ensure consistency
rg 'responses\[\d\]\.employee_field_mapping'
# Check the forkJoin structure in other QBD components
ast-grep --pattern 'forkJoin([
$$$
$_ExportSettingsService.getQbdExportSettings(),
$$$
])'
Length of output: 1798
Script:
#!/bin/bash
# Check the forkJoin setup in the QBD direct dashboard component
rg -A 20 'setupPage\(\)' src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts
# Also check the import statements to understand the service methods being used
rg -A 5 'import.*ExportSettingsService' src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts
Length of output: 1706
118-118: Verify the QBD-specific accounting summary parser implementation.
The change to use parseAPIResponseToAccountingSummaryForQbdDirect suggests QBD-specific parsing logic. Ensure that this new parser handles all QBD-specific edge cases correctly.
✅ Verification successful
Looking at the code, I can now see that there are two distinct parser methods in the AccountingExportSummaryModel class:
parseAPIResponseToAccountingSummary- The original parserparseAPIResponseToAccountingSummaryForQbdDirect- The QBD-specific parser
The key difference between them is in how they map the response fields:
- Original parser maps:
total_expense_groups_count,successful_expense_groups_count,failed_expense_groups_count - QBD parser maps:
total_export_log_count,successful_export_log_count,failed_export_log_count
This is a valid and necessary change as QBD Direct uses a different API response structure with "export_log" counts instead of "expense_groups" counts. The rest of the fields remain the same.
The QBD-specific parser implementation is correct and properly handles the different response structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of the new parser method to verify its handling
ast-grep --pattern 'parseAPIResponseToAccountingSummaryForQbdDirect($_) {
$$$
}'
# Search for any existing tests of this parser
rg -g '*.spec.ts' 'parseAPIResponseToAccountingSummaryForQbdDirect'
Length of output: 152
Script:
#!/bin/bash
# Let's try a broader search for the model and its methods
rg -A 5 "class AccountingExportSummaryModel"
# Also search for any files containing AccountingExportSummaryModel
fd AccountingExportSummaryModel
# Search for any static methods in this model
ast-grep --pattern 'static parseAPIResponseToAccountingSummaryForQbdDirect'
# Let's also check for test files that might contain this model name
rg -g '*.spec.ts' "AccountingExportSummaryModel"
Length of output: 1910
Script:
#!/bin/bash
# Let's check the content of the accounting-export-summary model file
cat src/app/core/models/db/accounting-export-summary.model.ts
# Let's also check for any recent changes to this file
git log -p --max-count=1 src/app/core/models/db/accounting-export-summary.model.ts
# Search for any QBD-specific implementations or references
rg -g '*.ts' -B 2 -A 2 'qbd.*direct'
Length of output: 102642
src/app/core/models/enum/enum.model.ts (1)
625-626: LGTM! The changes improve error handling capabilities.
The addition of the ERROR state to AccountingExportStatus enum enhances error handling granularity, while the trailing comma maintains consistent code style.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.html (2)
Line range hint
77-77: Add security attributes to the iframe.The YouTube iframe should include security-related attributes to prevent potential vulnerabilities.
Apply this diff:
-<iframe width="427" height="259" src="https://www.youtube.com/embed/2oYdc8KcQnk" frameborder="0"></iframe> +<iframe + width="427" + height="259" + src="https://www.youtube.com/embed/2oYdc8KcQnk" + frameborder="0" + allow="accelerometer; encrypted-media; gyroscope; picture-in-picture" + sandbox="allow-scripts allow-same-origin allow-presentation" + loading="lazy" + title="QuickBooks Web Connector Setup Guide"> +</iframe>
Line range hint
41-47: Enhance accessibility for password visibility toggle.The password visibility toggle buttons lack proper ARIA labels for screen readers.
Apply this diff:
-<app-svg-icon *ngIf="!isPasswordShown" [svgSource]="'eye-slash-medium'" [width]="'18px'" [height]="'18px'" [isTextColorAllowed]="true" [styleClasses]="'tw-text-icon-muted tw-cursor-pointer'" (iconClick)="showPassword(true)"></app-svg-icon> -<app-svg-icon *ngIf="isPasswordShown" [svgSource]="'eye-medium'" [width]="'18px'" [height]="'18px'" [isTextColorAllowed]="true" [styleClasses]="'tw-text-icon-muted tw-cursor-pointer'" (iconClick)="showPassword(false)"></app-svg-icon> +<app-svg-icon + *ngIf="!isPasswordShown" + [svgSource]="'eye-slash-medium'" + [width]="'18px'" + [height]="'18px'" + [isTextColorAllowed]="true" + [styleClasses]="'tw-text-icon-muted tw-cursor-pointer'" + [ariaLabel]="'Show password'" + role="button" + (iconClick)="showPassword(true)"> +</app-svg-icon> +<app-svg-icon + *ngIf="isPasswordShown" + [svgSource]="'eye-medium'" + [width]="'18px'" + [height]="'18px'" + [isTextColorAllowed]="true" + [styleClasses]="'tw-text-icon-muted tw-cursor-pointer'" + [ariaLabel]="'Hide password'" + role="button" + (iconClick)="showPassword(false)"> +</app-svg-icon>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
src/assets/icons/check-circle-outline-extra-large.svgis excluded by!**/*.svgsrc/assets/icons/co/grv-checkmark-encircled-extra-large.svgis excluded by!**/*.svgsrc/assets/icons/eye-medium.svgis excluded by!**/*.svgsrc/assets/sprites/sprite.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.html(1 hunks)src/app/shared/components/core/svg-icon/svg-icon.component.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.html
🔇 Additional comments (3)
src/app/shared/components/core/svg-icon/svg-icon.component.ts (2)
23-23: LGTM! Icon mapping follows established pattern
The new icon mapping follows the existing naming convention and size progression pattern (small -> medium -> extra-large).
23-23: Verify the existence of the icon asset
Please ensure that the grv-checkmark-encircled-extra-large icon asset exists in your icon system.
✅ Verification successful
The icon asset exists and is properly implemented
The icon grv-checkmark-encircled-extra-large exists in both:
- As an individual SVG file at
src/assets/icons/co/grv-checkmark-encircled-extra-large.svg - As a symbol in the sprite sheet at
src/assets/sprites/sprite.svg
The icon implementation is correct and available for use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the existence of the icon asset in common asset locations
# Search for the icon in asset files
echo "Searching for icon asset references..."
rg -l "grv-checkmark-encircled-extra-large" \
--type html \
--type css \
--type scss \
--type svg \
--type ts
Length of output: 215
Script:
#!/bin/bash
# Description: Check for the existence of the icon asset in common asset locations with corrected file types
# Search for the icon in asset files
echo "Searching for icon asset references..."
rg -l "grv-checkmark-encircled-extra-large"
# Also search for SVG files that might contain this icon
echo -e "\nSearching for SVG files..."
fd -e svg
Length of output: 7432
Script:
#!/bin/bash
# Let's check the content of sprite.svg and the specific icon to verify its existence
echo "Checking sprite.svg for the icon..."
rg "grv-checkmark-encircled-extra-large" src/assets/sprites/sprite.svg -C 2
echo -e "\nChecking the individual SVG file..."
cat "src/assets/icons/co/grv-checkmark-encircled-extra-large.svg"
Length of output: 131374
Script:
#!/bin/bash
# Let's check the specific icon file content
echo "Checking the icon file content..."
cat src/assets/icons/co/grv-checkmark-encircled-extra-large.svg
echo -e "\nChecking sprite.svg references..."
rg -A 2 -B 2 "grv-checkmark-encircled-extra-large" src/assets/sprites/sprite.svg
Length of output: 132314
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.html (1)
88-88: LGTM! Verify the SVG icon registration.
The increased icon size (40px) and updated source improve the success state's visibility.
Let's verify that the new SVG icon is properly registered:
✅ Verification successful
SVG icon is properly registered ✓
The check-circle-outline-extra-large icon is correctly registered in the SVG icon registry at src/app/shared/components/core/svg-icon/svg-icon.component.ts and maps to grv-checkmark-encircled-extra-large.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new SVG icon is registered in the codebase
# Search for SVG icon registration
rg -A 2 "check-circle-outline-extra-large" --type ts
Length of output: 385
DhaaraniCIT
added
deploy
|
* QA fixes * qa fixes * qa fixes * fix: qbd direct export settings onboarding changes and content changes (#1082) * qbd direct export settings onboarding changes and content changes * fix: qbd direct import settings onboarding changes and content changes (#1083) * qbd direct import settings onboarding changes and content changes * qbd direct advanced settings onboarding changes and content changes (#1084) * QA fixes * fix: QBD direct bug fixes (#1085) * QBD direct bug fixes * QBD direct bug fixes * QBD direct bug fixes
Description
fix: QBD direct bug fixes
Clickup
https://app.clickup.com/t/86cwzceku
Summary by CodeRabbit
Release Notes
New Features
ERRORstate to the Accounting Export status for better error categorization.Improvements
Bug Fixes
These changes improve the overall functionality and reliability of the export processes within the application.