fix: Successful export dialog view #1160
Conversation
|
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 48 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 pull request modifies 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: 0
🔭 Outside diff range comments (1)
src/app/shared/components/dashboard/dashboard-export-summary-section/dashboard-export-summary-section.component.ts (1)
Line range hint
78-83: Consider maintaining consistency with v1 implementation.The v1 implementation in
getExpenseGroupsuses date filtering, while v2 removes it. This inconsistency could lead to different behaviors between versions.Consider one of these approaches:
- Apply the same date filtering logic as v1:
- this.accountingExportService.getAccountingExports(this.accountingExportType, [status], null, limit, offset, null, lastExportedAt, null, this.appName) + const dateFilter = lastExportedAt ? { + startDate: new Date(lastExportedAt), + endDate: new Date() + } : null; + this.accountingExportService.getAccountingExports(this.accountingExportType, [status], dateFilter, limit, offset, null, lastExportedAt, null, this.appName)
- Update v1 to match v2 if date filtering is handled server-side.
🧹 Nitpick comments (1)
src/app/shared/components/dashboard/dashboard-export-summary-section/dashboard-export-summary-section.component.ts (1)
Line range hint
78-83: Verify error handling for the API call.The subscription to the API response lacks error handling, which could lead to unhandled exceptions if the API call fails.
Consider adding error handling:
this.accountingExportService.getAccountingExports(this.accountingExportType, [status], null, limit, offset, null, lastExportedAt, null, this.appName).subscribe( - accountingExportResponse => { + { + next: (accountingExportResponse) => { const accountingExports: AccountingExportList[] = accountingExportResponse.results.map((accountingExport: AccountingExport) => AccountingExportModel.parseAPIResponseToExportLog(accountingExport, this.org_id) ); this.setFormattedAccountingExport(accountingExports); + }, + error: (error) => { + // Handle API errors appropriately + console.error('Failed to fetch accounting exports:', error); + } } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/app/shared/components/dashboard/dashboard-export-summary-section/dashboard-export-summary-section.component.ts(1 hunks)
🔇 Additional comments (1)
src/app/shared/components/dashboard/dashboard-export-summary-section/dashboard-export-summary-section.component.ts (1)
78-78: Verify the intentional removal of date filtering.The removal of date filtering logic differs from the v1 implementation in
getExpenseGroups. This change could affect how exports are filtered and displayed, especially for completed exports wherelastExportedAtis provided.Please confirm:
- Is this an intentional change to remove date-based filtering?
- Does the backend API handle the date filtering when
nullis passed?Let's verify the API behavior with this script:
✅ Verification successful
Removal of date filtering is intentional for the dashboard summary view
The change is verified to be intentional and correctly implemented:
- Dashboard summary section uses a simplified query without date filtering
- Detailed export logs across all integrations maintain date filtering capability
- The API service accepts null as a valid parameter for date filter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the API endpoint implementation to verify null date filter handling rg -t ts "getAccountingExports.*\(" --glob "!*.spec.ts" -A 10Length of output: 76260
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts (2)
351-352: Consider standardizing date options handling across expense types.The reimbursable date options are now handled without brand checks, while CCC options still maintain brand-specific logic. This pattern is similar to the Xero implementation.
Consider standardizing the handling of both reimbursable and CCC date options to maintain consistency across the codebase.
Line range hint
113-116: Consider complete standardization of date options handling.The changes across components show a pattern of removing brand-specific logic for reimbursable expense date options while maintaining it for CCC options. This creates an inconsistency in the codebase.
Recommendations:
- Standardize the handling of both reimbursable and CCC date options
- Document any business requirements that necessitate different handling
- Consider extracting common date options logic into a shared service
Also applies to: 122-123, 127-128, 162-173, 351-352
src/app/integrations/intacct/intacct-shared/intacct-export-settings/intacct-export-settings.component.ts (1)
Line range hint
527-534: Consider removing the remaining brand-specific check for consistency.The code still contains a brand-specific check for 'fyle' in the CCC export group handler. For consistency with the reimbursable expense handler above, consider removing this check to ensure uniform behavior across all brands.
this.exportSettingsForm.controls.cccExportGroup?.valueChanges.subscribe((cccExportGroup) => { - if (brandingConfig.brandId==='fyle') { this.cccExpenseGroupingDateOptions = IntacctExportSettingModel.getExpenseGroupingDateOptions(); this.cccExpenseGroupingDateOptions = ExportSettingModel.constructGroupingDateOptions(cccExportGroup, this.cccExpenseGroupingDateOptions); if (this.exportSettingsForm?.value.cccExportType === IntacctCorporateCreditCardExpensesObject.CHARGE_CARD_TRANSACTION) { this.setCCExpenseDateOptions(this.exportSettingsForm?.value.cccExportType); } - } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/core/models/common/export-settings.model.ts(1 hunks)src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts(1 hunks)src/app/integrations/intacct/intacct-shared/intacct-export-settings/intacct-export-settings.component.ts(1 hunks)src/app/integrations/qbo/qbo-shared/qbo-export-settings/qbo-export-settings.component.ts(1 hunks)src/app/integrations/sage300/sage300-shared/sage300-export-settings/sage300-export-settings.component.ts(1 hunks)src/app/integrations/xero/xero-shared/xero-export-settings/xero-export-settings.component.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app/core/models/common/export-settings.model.ts
🔇 Additional comments (4)
src/app/integrations/xero/xero-shared/xero-export-settings/xero-export-settings.component.ts (1)
113-116: Verify the intentional difference in date options handling.The reimbursable date options are now updated unconditionally while CCC date options still have a brand check. Please confirm if this difference in handling between reimbursable and CCC date options is intentional.
Consider standardizing the behavior for both reimbursable and CCC date options to maintain consistency.
src/app/integrations/sage300/sage300-shared/sage300-export-settings/sage300-export-settings.component.ts (1)
122-123: Well-structured date options handling!The implementation follows a clean pattern of resetting options to their default state before constructing new ones. This ensures proper state management and prevents potential option mixing issues.
Also applies to: 127-128
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts (1)
162-173: Robust date options handling with validation!The implementation includes proper validation and cleanup:
- Resets and constructs date options correctly
- Validates CCC export date values against visible options
- Automatically resets invalid date values
This prevents invalid states and improves user experience.
src/app/integrations/intacct/intacct-shared/intacct-export-settings/intacct-export-settings.component.ts (1)
523-524: LGTM! The removal of brand-specific check simplifies the code.The unconditional update of reimbursable expense grouping date options makes the code more maintainable.
|
Description
Please add PR description here, add screenshots if needed
Clickup
https://app.clickup.com/
Summary by CodeRabbit