fix: add a default bank account field for CCC expenses #1114
Conversation
|
Warning Rate limit exceeded@JustARatherRidiculouslyLongUsername has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 52 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 introduce new properties related to default bank account details for credit card expenses in the Changes
Possibly related PRs
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 and nitpick comments (2)
src/app/core/models/business-central/business-central-configuration/business-central-export-setting.model.ts (1)
18-19: LGTM with a minor suggestion for consistency.The new CCC bank account properties are properly integrated into the model, form handling, and payload creation.
Consider using camelCase for the form control name to match Angular conventions:
-cccDefaultBankName: new FormControl(exportSettings?.default_CCC_bank_account_name ? findObjectByDestinationId(accounts, exportSettings?.default_CCC_bank_account_id) : null), +defaultCccBankName: new FormControl(exportSettings?.default_CCC_bank_account_name ? findObjectByDestinationId(accounts, exportSettings?.default_CCC_bank_account_id) : null),Also applies to: 189-189, 210-211
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts (1)
Line range hint
190-207: Consider optimizing array operations for better performance.The current implementation performs multiple array operations that could be optimized:
-const newOptions = [...bankAccounts.results, ...accounts.results]; -newOptions.forEach((newOption) => { - if (!this.bankAccountOptions.find((existingOption) => existingOption.destination_id === newOption.destination_id)) { - this.bankAccountOptions.push(newOption); - } -}); -this.bankAccountOptions.sort((a, b) => (a.value || '').localeCompare(b.value || '')); +// Create a map of existing options for O(1) lookup +const existingOptionsMap = new Map( + this.bankAccountOptions.map(option => [option.destination_id, option]) +); + +// Add new unique options +const newOptions = [...bankAccounts.results, ...accounts.results] + .filter(option => !existingOptionsMap.has(option.destination_id)); + +// Update array and sort in one go +this.bankAccountOptions = [...this.bankAccountOptions, ...newOptions] + .sort((a, b) => (a.value || '').localeCompare(b.value || ''));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/app/core/models/business-central/business-central-configuration/business-central-export-setting.model.ts(3 hunks)src/app/core/models/enum/enum.model.ts(1 hunks)src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html(2 hunks)src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts(4 hunks)src/app/integrations/business-central/business-central-shared/business-central.fixture.ts(1 hunks)
🔇 Additional comments (5)
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html (2)
94-95: LGTM! Consistent naming for bank account options.
The refactoring to use generic bank account options and enum improves code consistency.
176-190: LGTM! Default bank account field added for CCC expenses.
The implementation follows the same pattern as reimbursable expenses and is correctly conditioned on journal entry export type.
Let's verify the component handles both bank account selections correctly:
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information to verify the component's handling of both bank account selections. The code shows:
-
Both bank account form controls are properly initialized in the form group:
defaultBankNamefor reimbursable expensescccDefaultBankNamefor CCC expenses
-
The
bankAccountOptionsare correctly populated and sorted for both fields to use -
The mapping between form values and API response is properly handled in both directions:
- Form initialization maps API response to form controls
- Form submission maps form values back to API format
-
The validation rules are properly set up for both fields when export type is JOURNAL_ENTRY
Both bank account selections are properly implemented and handled consistently.
The component correctly manages both bank account selections with proper initialization, validation, and data mapping. The implementation follows the established patterns and maintains consistency between reimbursable and CCC expense handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the component's handling of both bank account selections
# Test: Check if the component properly initializes and updates both bank account form controls
# Search for form initialization and bank account handling
rg -A 5 "bankAccountOptions|defaultBankName|cccDefaultBankName" --type ts
Length of output: 11660
src/app/core/models/enum/enum.model.ts (1)
706-706: LGTM! Simplified bank account enum handling.
The consolidation to a single BANK_ACCOUNT enum value improves code consistency and reusability across different expense types.
src/app/integrations/business-central/business-central-shared/business-central.fixture.ts (1)
15-16: LGTM!
The new test data properties for CCC bank account are properly added to the fixture.
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts (1)
32-32: LGTM!
The variable renaming from bankOptions to bankAccountOptions improves clarity, and the initialization properly combines and sorts the bank account options.
Also applies to: 308-310
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts (2)
Line range hint
190-207: Consider optimizing array operations for better performanceWhile the logic is correct, the current implementation performs multiple array operations that could be optimized:
- Creating a new array with spread operator
- Iterating through new options to check for duplicates
- Sorting the entire array after each search
Consider this more efficient approach:
- const newOptions = [...bankAccounts.results, ...accounts.results]; - newOptions.forEach((newOption) => { - if (!this.bankAccountOptions.find((existingOption) => existingOption.destination_id === newOption.destination_id)) { - this.bankAccountOptions.push(newOption); - } - }); - this.bankAccountOptions.sort((a, b) => (a.value || '').localeCompare(b.value || '')); + const existingIds = new Set(this.bankAccountOptions.map(option => option.destination_id)); + const uniqueNewOptions = [...bankAccounts.results, ...accounts.results] + .filter(option => !existingIds.has(option.destination_id)); + if (uniqueNewOptions.length > 0) { + this.bankAccountOptions = [...this.bankAccountOptions, ...uniqueNewOptions] + .sort((a, b) => (a.value || '').localeCompare(b.value || '')); + }
250-271: Enhance method documentation for clarityWhile the implementation is solid, the comment could be more specific about the method's purpose and behavior.
Consider updating the comment to be more descriptive:
- // Since pagination API response is a subset of all options, we're making use of the export settings response to fill in options + /** + * Ensures that default bank accounts and vendor options from export settings + * are present in their respective option lists, even if they weren't included + * in the paginated API response. This prevents missing default selections in + * the UI dropdowns. + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts(5 hunks)
🔇 Additional comments (1)
src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts (1)
32-32: LGTM: Clear and well-typed property declaration
The property name is descriptive and follows the component's naming conventions.
| this.vendorOptions = vendors.results; | ||
| this.bankAccountOptions = [...reimbursableBankAccounts.results, ...reimbursableAccounts.results]; | ||
| this.bankAccountOptions.sort((a, b) => (a.value || '').localeCompare(b.value || '')); | ||
| this.addMissingOptions(); | ||
| this.exportSettingForm = BusinessCentralExportSettingModel.mapAPIResponseToFormGroup(this.exportSettings, this.bankAccountOptions, vendors.results); | ||
|
|
There was a problem hiding this comment.
Add error handling for bank account initialization
The current implementation lacks error handling for the forkJoin operation of bank account initialization. If either the reimbursableBankAccounts or reimbursableAccounts requests fail, it could lead to an incomplete or broken state.
Add error handling:
- this.bankAccountOptions = [...reimbursableBankAccounts.results, ...reimbursableAccounts.results];
- this.bankAccountOptions.sort((a, b) => (a.value || '').localeCompare(b.value || ''));
+ try {
+ if (!reimbursableBankAccounts?.results || !reimbursableAccounts?.results) {
+ throw new Error('Failed to fetch bank account options');
+ }
+ this.bankAccountOptions = [...reimbursableBankAccounts.results, ...reimbursableAccounts.results];
+ this.bankAccountOptions.sort((a, b) => (a.value || '').localeCompare(b.value || ''));
+ } catch (error) {
+ this.toastService.displayToastMessage(ToastSeverity.ERROR, 'Failed to initialize bank account options');
+ this.bankAccountOptions = [];
+ }Committable suggestion skipped: line range outside the PR's diff.
JustARatherRidiculouslyLongUsername
added
the
staging_deploy
JustARatherRidiculouslyLongUsername
temporarily deployed
to
Staging
GitHub Actions
Inactive
...els/business-central/business-central-configuration/business-central-export-setting.model.ts
Outdated
Show resolved
Hide resolved
JustARatherRidiculouslyLongUsername
added
staging_deploy
JustARatherRidiculouslyLongUsername
temporarily deployed
to
Staging
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
src/app/shared/components/configuration/configuration-select-field/configuration-select-field.component.html (1)
100-101: Consider simplifying complex conditions and improving maintainabilityThe auto-selection message conditions are complex and could benefit from some improvements:
- Move the message text to constants or i18n files for better maintainability and internationalization support
- Extract the complex conditions to component methods for better readability
Example refactor:
- <span *ngIf="(appName === AppName.QBD_DIRECT && form.controls.creditCardExportType.value === 'CREDIT_CARD_PURCHASE' && formControllerName === 'creditCardExportGroup') || (appName !== AppName.QBD_DIRECT)">Auto-selected based on your export module</span> - <span *ngIf="(formControllerName === 'reimbursableExportGroup' && form.controls.reimbursableExportType.value !== 'BILL' && appName === AppName.QBD_DIRECT) || (appName === AppName.QBD_DIRECT && form.controls.creditCardExportType.value !== 'CREDIT_CARD_PURCHASE' && formControllerName === 'creditCardExportGroup')"> Auto-selected when your default credit account is set to an Accounts Payable account</span> + <span *ngIf="shouldShowExportModuleMessage()">{{messages.autoSelectedExportModule}}</span> + <span *ngIf="shouldShowAccountsPayableMessage()">{{messages.autoSelectedAccountsPayable}}</span>In the component:
// Add to component class messages = { autoSelectedExportModule: 'Auto-selected based on your export module', autoSelectedAccountsPayable: 'Auto-selected when your default credit account is set to an Accounts Payable account' }; shouldShowExportModuleMessage(): boolean { return ( (this.appName === AppName.QBD_DIRECT && this.form.controls.creditCardExportType.value === 'CREDIT_CARD_PURCHASE' && this.formControllerName === 'creditCardExportGroup') || (this.appName !== AppName.QBD_DIRECT) ); } shouldShowAccountsPayableMessage(): boolean { return ( (this.formControllerName === 'reimbursableExportGroup' && this.form.controls.reimbursableExportType.value !== 'BILL' && this.appName === AppName.QBD_DIRECT) || (this.appName === AppName.QBD_DIRECT && this.form.controls.creditCardExportType.value !== 'CREDIT_CARD_PURCHASE' && this.formControllerName === 'creditCardExportGroup') ); }src/app/core/models/common/advanced-settings.model.ts (1)
79-94: Improve static method clarity by using class name instead of 'this'.Replace
this.getDefaultMemoOptions()withAdvancedSettingsModel.getDefaultMemoOptions()to make the static context clearer.static getMemoOptions(exportSettings: IntacctConfiguration | ExportSettingGet | NetSuiteExportSettingGet | QBOExportSettingGet, appName: string): string[] { - const defaultOptions = this.getDefaultMemoOptions(); + const defaultOptions = AdvancedSettingsModel.getDefaultMemoOptions(); let cccExportType: string | undefined; // ...rest of the method }🧰 Tools
🪛 Biome (1.9.4)
[error] 80-80: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
src/app/branding/fyle-branding-config.ts (1)
Line range hint
94-94: Unrelated TODO comment found.There's an unrelated TODO comment about updating KB articles for Sage 300. Consider creating a separate issue to track this.
Would you like me to create a GitHub issue to track the Sage 300 KB articles update?
src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts (1)
96-96: Consider optimizing the initialization of defaultMemoFields.The property is being set twice:
- Initially with
getDefaultMemoOptions()- Later with
getMemoOptions(configuration, AppName.INTACCT)This could be optimized by removing the initial declaration and only setting it once when the configuration is available.
-defaultMemoFields: string[] = AdvancedSettingsModel.getDefaultMemoOptions(); +defaultMemoFields: string[];Also applies to: 316-316
src/app/branding/fyle-contents-config.ts (1)
253-253: LGTM with a minor suggestion for consistency.The added customization sub-label text is well-formed and consistent with the overall configuration structure. However, for better consistency with other similar labels in the file, consider adding a period at the end of the sentence.
- customizeSectionSubLabel: 'In this section, you can customize the data that you\'d like to export from ' + brandingConfig.brandName + ' to Xero. You can choose what data points need to be exported and what shouldn\'t be.', + customizeSectionSubLabel: 'In this section, you can customize the data that you\'d like to export from ' + brandingConfig.brandName + ' to Xero. You can choose what data points need to be exported and what shouldn\'t be.',deploy_dump.sh (2)
6-6: Consider making repository URL configurableThe base URL is hardcoded which makes the script less reusable across different repositories.
Consider making it configurable:
-base_url="https://github.com/fylein/fyle-integrations-app/commit" +# Default can be overridden by environment variable +base_url="${REPO_URL:-https://github.com/fylein/fyle-integrations-app/commit}"
2-2: Enhance usage message with example and descriptionThe current usage message could be more informative.
Consider enhancing the usage message:
- echo "Usage: sh $0 '2024-12-09'" + echo "Usage: $0 <since-date>" + echo "Generates a CSV file of commits since the specified date" + echo "Example: $0 '2024-12-09'"src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts (1)
Line range hint
1-190: PR implementation appears incompleteWhile the changes maintain consistency in chart of accounts handling across components, they don't implement the default bank account field for CCC expenses as indicated in the PR title. Consider:
- Adding default bank account field to relevant models
- Implementing UI controls for setting default bank account
- Adding logic to apply default bank account for CCC expenses
Would you like assistance in implementing the missing functionality for default bank account handling?
🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.gitignore(1 hunks)deploy_dump.sh(1 hunks)src/app/branding/c1-branding-config.ts(0 hunks)src/app/branding/c1-contents-config.ts(1 hunks)src/app/branding/fyle-branding-config.ts(2 hunks)src/app/branding/fyle-contents-config.ts(1 hunks)src/app/core/models/branding/content-configuration.model.ts(1 hunks)src/app/core/models/branding/kb-article.model.ts(0 hunks)src/app/core/models/business-central/business-central-configuration/business-central-export-setting.model.ts(3 hunks)src/app/core/models/common/advanced-settings.model.ts(2 hunks)src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts(5 hunks)src/app/integrations/business-central/business-central-shared/business-central.fixture.ts(1 hunks)src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts(2 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts(2 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-mapping/qbd-direct-base-mapping/qbd-direct-base-mapping.component.ts(2 hunks)src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.html(3 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.ts(4 hunks)src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.html(1 hunks)src/app/shared/components/configuration/configuration-select-field/configuration-select-field.component.html(1 hunks)
💤 Files with no reviewable changes (2)
- src/app/branding/c1-branding-config.ts
- src/app/core/models/branding/kb-article.model.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/integrations/business-central/business-central-shared/business-central.fixture.ts
- src/app/core/models/business-central/business-central-configuration/business-central-export-setting.model.ts
👮 Files not reviewed due to content moderation or server errors (4)
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.html
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-advanced-settings/qbd-direct-advanced-settings.component.ts
- src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.ts
- src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.ts
🧰 Additional context used
🪛 Shellcheck (0.10.0)
deploy_dump.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🪛 Biome (1.9.4)
src/app/core/models/common/advanced-settings.model.ts
[error] 80-80: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (10)
src/app/core/models/common/advanced-settings.model.ts (1)
79-79: LGTM! Good type safety improvement.
Adding IntacctConfiguration as a possible type for exportSettings improves type safety and makes the method more flexible.
src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.html (1)
106-106: LGTM! Good use of branding content.
The changes properly utilize the brandingContent object for labels and sub-labels, improving maintainability and consistency.
Also applies to: 114-115
src/app/branding/fyle-branding-config.ts (1)
70-70: LGTM! Consistent URL updates for QBD beta integration.
The URLs are properly updated to point to the beta integration documentation, maintaining consistency across all QBD_DIRECT references.
Also applies to: 129-132
src/app/core/models/branding/content-configuration.model.ts (1)
251-253: LGTM! Type definitions enhanced for better type safety.
The additions properly type the new configuration properties for both Xero and Intacct integrations, maintaining consistency across different integration types.
Also applies to: 257-260
src/app/branding/c1-contents-config.ts (1)
257-260: LGTM! Clear and consistent UI messaging.
The added content strings provide clear guidance for users configuring Xero's advanced settings, maintaining consistency with other parts of the application.
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-mapping/qbd-direct-base-mapping/qbd-direct-base-mapping.component.ts (3)
11-11: LGTM: Import statement is correctly added
The import of QbdDirectImportSettingModel is necessary for accessing the static getChartOfAccountTypesList method.
Line range hint 73-82: Default bank account implementation appears to be missing
While the getCCCAccountOptions method handles CCC account types, it doesn't implement the default bank account field mentioned in the PR title. Consider adding logic to handle default bank account selection for CCC expenses.
110-110: Verify the impact of conditional chart of accounts assignment
The change introduces a conditional assignment of chartOfAccounts based on import_account_as_category flag. This could affect the available options in the mapping UI.
✅ Verification successful
Based on the gathered evidence, I can now generate the final response:
The conditional chart of accounts assignment is safely implemented
The change is consistent with the existing codebase pattern, as evidenced by:
- The same conditional logic is already implemented in
qbd-direct-dashboard.component.ts - The
chartOfAccountsis properly consumed by thedestinationOptionsWatchermethod in the mapping component - The change maintains backward compatibility by falling back to
getChartOfAccountTypesList()whenimport_account_as_categoryis false
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing usages of chart_of_accounts that might be affected
# by this conditional logic change
# Search for chart_of_accounts references
rg "chart_of_accounts" --type ts
# Search for any QBD mapping related tests that might need updates
fd -e spec.ts -e test.ts . | xargs rg "chart_of_accounts|getChartOfAccountTypesList"
Length of output: 1303
Script:
#!/bin/bash
# Let's check for any UI components or templates that might be using chartOfAccounts
rg "chartOfAccounts" -t html
# Let's also check the implementation of getChartOfAccountTypesList
ast-grep --pattern 'getChartOfAccountTypesList() {
$$$
}'
# Check for any mapping-related components that might be affected
rg "chartOfAccounts" -t ts -A 5
Length of output: 10943
Script:
#!/bin/bash
# Let's check the implementation of getChartOfAccountTypesList method to understand the default behavior
ast-grep --pattern 'class QbdDirectImportSettingModel {
$$$
}'
# Let's also check where this chartOfAccounts is being used in the mapping component
rg -B 5 -A 5 "destinationOptionsWatcher.*chartOfAccounts" --type ts
Length of output: 1918
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.ts (2)
12-12: LGTM: Consistent import across components
The import of QbdDirectImportSettingModel maintains consistency with the base-mapping component.
155-155: Verify the consistency of chart of accounts handling
The conditional assignment of chartOfAccounts matches the implementation in the base-mapping component, which is good for consistency. However, this change appears unrelated to the PR's objective of adding a default bank account field for CCC expenses.
✅ Verification successful
The search results show that:
- The
import_account_as_categoryimplementation is consistent between the dashboard and base-mapping components - The default bank account functionality exists only in the Business Central integration, not in QBD Direct
Let me verify if this PR is indeed adding default bank account functionality to QBD Direct by checking recent changes.
Let me try a different approach to check the PR changes.
Based on the search results, I can now provide a final response:
The chart of accounts implementation is consistent and relevant to the PR
The chart of accounts conditional assignment is not only consistent between components but is also relevant to the PR's objective. The search results show extensive CCC (Corporate Credit Card) related changes in the QBD Direct integration, including new form controls for default CCC accounts payable settings. The chart of accounts implementation is necessary as it provides the available accounts list for these new CCC-related account mappings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent implementation across components
# Check for consistent usage of import_account_as_category
rg "import_account_as_category" --type ts
# Look for any existing default bank account implementations
rg "defaultBankAccount|default_bank_account" --type ts --type html
Length of output: 3448
Script:
#!/bin/bash
# Check recent changes in QBD Direct related to default bank account
rg -B2 -A2 "defaultBankAccount|default_bank_account" src/app/integrations/qbd-direct --type ts --type html
# Check the PR changes in QBD Direct integration
fd . src/app/integrations/qbd-direct -t f -e ts -e html --changed
Length of output: 429
Script:
#!/bin/bash
# Look for any QBD Direct files that contain default bank account related code
rg -B2 -A2 "defaultBankAccount|default_bank_account" src/app/integrations/qbd-direct --type ts --type html
# Check for CCC expenses related changes in QBD Direct
rg "CCC|ccc|corporate card|corporate credit card" src/app/integrations/qbd-direct --type ts --type html
Length of output: 10622
| base_url="https://github.com/fylein/fyle-integrations-app/commit" | ||
| branch_name=$(git rev-parse --abbrev-ref HEAD) | ||
|
|
||
| git log --since="$1" --pretty=format:"$base_url/%H,%an,%ad,%s,$branch_name,integrations-app" > commits.csv |
There was a problem hiding this comment.
💡 Codebase verification
Add commits.csv to .gitignore to prevent accidental commits
The verification reveals that:
commits.csvis not listed in.gitignore- Only
deploy_dump.shreferences this file, so there are no conflicts with other scripts
To prevent accidental commits of this generated file, it should be added to .gitignore.
🔗 Analysis chain
Verify commits.csv handling
Let's ensure the generated file is properly handled in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if commits.csv is properly gitignored
if ! grep -q "commits.csv" .gitignore; then
echo "Warning: commits.csv is not in .gitignore"
fi
# Check if any other scripts reference commits.csv
rg -l "commits.csv" --type sh
Length of output: 77
| @@ -0,0 +1,11 @@ | |||
| if [ -z "$1" ]; then | |||
There was a problem hiding this comment.
Add shebang line for shell script portability
The script is missing a shebang line which affects its portability across different environments.
Add this line at the beginning of the file:
+#!/bin/bash
if [ -z "$1" ]; then📝 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 [ -z "$1" ]; then | |
| #!/bin/bash | |
| if [ -z "$1" ]; then |
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
|
|
||
| git log --since="$1" --pretty=format:"$base_url/%H,%an,%ad,%s,$branch_name,integrations-app" > commits.csv | ||
|
|
||
| open commits.csv |
There was a problem hiding this comment.
Make file opening cross-platform compatible
The open command is macOS specific and won't work on Linux or Windows.
Replace with a cross-platform solution:
-open commits.csv
+# Cross-platform file opening
+case "$(uname -s)" in
+ Darwin*) open commits.csv ;;
+ Linux*) xdg-open commits.csv ;;
+ CYGWIN*|MINGW*) start commits.csv ;;
+ *) echo "Please open commits.csv manually" ;;
+esac📝 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.
| open commits.csv | |
| # Cross-platform file opening | |
| case "$(uname -s)" in | |
| Darwin*) open commits.csv ;; | |
| Linux*) xdg-open commits.csv ;; | |
| CYGWIN*|MINGW*) start commits.csv ;; | |
| *) echo "Please open commits.csv manually" ;; | |
| esac |
| branch_name=$(git rev-parse --abbrev-ref HEAD) | ||
|
|
||
| git log --since="$1" --pretty=format:"$base_url/%H,%an,%ad,%s,$branch_name,integrations-app" > commits.csv |
There was a problem hiding this comment.
Add error handling for git commands
The script assumes git commands will succeed but they might fail if not in a git repository or if there are connectivity issues.
Add error handling:
-branch_name=$(git rev-parse --abbrev-ref HEAD)
+branch_name=$(git rev-parse --abbrev-ref HEAD) || {
+ echo "Error: Failed to get branch name. Are you in a git repository?" >&2
+ exit 1
+}
-git log --since="$1" --pretty=format:"$base_url/%H,%an,%ad,%s,$branch_name,integrations-app" > commits.csv
+# Add CSV header
+echo "Commit URL,Author,Date,Message,Branch,Repository" > commits.csv
+
+git log --since="$1" --pretty=format:"$base_url/%H,%an,%ad,%s,$branch_name,integrations-app" >> commits.csv || {
+ echo "Error: Failed to generate git log" >&2
+ exit 1
+}📝 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.
| branch_name=$(git rev-parse --abbrev-ref HEAD) | |
| git log --since="$1" --pretty=format:"$base_url/%H,%an,%ad,%s,$branch_name,integrations-app" > commits.csv | |
| branch_name=$(git rev-parse --abbrev-ref HEAD) || { | |
| echo "Error: Failed to get branch name. Are you in a git repository?" >&2 | |
| exit 1 | |
| } | |
| # Add CSV header | |
| echo "Commit URL,Author,Date,Message,Branch,Repository" > commits.csv | |
| git log --since="$1" --pretty=format:"$base_url/%H,%an,%ad,%s,$branch_name,integrations-app" >> commits.csv || { | |
| echo "Error: Failed to generate git log" >&2 | |
| exit 1 | |
| } |
JustARatherRidiculouslyLongUsername
merged commit 56db591
into
je-group-by-expense-only
* fix: restrict JE modules to group by expense only * fix: add a default bank account field for CCC expenses (#1114) * fix: remove validation temporarily (#1111) * fix: add a default bank account field for CCC expenses * fix: add missing options to bank accounts on page init * fix: dynamic content for xero customize settings (#1112) * fix: update sublabel key to avoid build fail (#1116) * fix: Prod fixes of QBD direct (#1118) * fix bugs (#1119) * refactor: capitalization * fix: only ccc exports not being saved (#1121) --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]>
|
* fix: initialize chart of accounts multiselect when there is no api response (#1110) * fix: remove the posted at date option for ccc expenses grouped by report (#1105) * fix: update login error flow and fix redirect url (#1117) * fix: restrict JE modules to group by expense only (#1113) * fix: restrict JE modules to group by expense only * fix: add a default bank account field for CCC expenses (#1114) * fix: remove validation temporarily (#1111) * fix: add a default bank account field for CCC expenses * fix: add missing options to bank accounts on page init * fix: dynamic content for xero customize settings (#1112) * fix: update sublabel key to avoid build fail (#1116) * fix: Prod fixes of QBD direct (#1118) * fix bugs (#1119) * refactor: capitalization * fix: only ccc exports not being saved (#1121) --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]> --------- Co-authored-by: Ashwin Thanaraj <[email protected]> Co-authored-by: Nilesh Pant <[email protected]> Co-authored-by: Dhaarani <[email protected]> Co-authored-by: Anish Kr Singh <[email protected]>
5 participants
Clickup
https://app.clickup.com/t/86cxaatt3
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores