feat: auto enable accounting period #1073
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request modify 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: 3
🧹 Outside diff range and nitpick comments (2)
src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts (2)
140-140: Consider adding documentation for the accounting period featureSince this is a new feature that automatically enables accounting periods for new organizations, it would be helpful to add documentation explaining:
- The purpose of this change
- How it affects new vs existing organizations
- Any configuration options available
Add JSDoc comments above the method:
/** * Maps API response to form group controls * @param advancedSettings - Settings from the API * @param isSkipExportEnabled - Flag to enable/disable export skipping * @param adminEmails - List of admin email options * @param shouldEnableAccountingPeriod - When true, automatically enables accounting period for new organizations */
140-140: Consider adding tests for the new behaviorThe change in form control initialization logic should be covered by unit tests.
Would you like me to help create unit tests that verify:
- Accounting period is enabled for new organizations
- Existing organizations' settings are preserved
- Edge cases are handled correctly
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts(1 hunks)src/app/core/models/qbo/qbo-configuration/qbo-advanced-setting.model.ts(1 hunks)src/app/core/models/xero/xero-configuration/xero-advanced-settings.model.ts(1 hunks)
🔇 Additional comments (3)
src/app/core/models/xero/xero-configuration/xero-advanced-settings.model.ts (1)
109-109: Verify consistent implementation across configuration models
The AI summary mentions similar changes in NetsuiteAdvancedSettingModel and QBOAdvancedSettingModel. Let's verify the implementation consistency.
✅ Verification successful
The verification results show consistent implementation of changeAccountingPeriod FormControl across all three configuration models (Xero, QBO, and NetSuite) with the same conditional logic using shouldEnableAccountingPeriod. The form control is properly used in their respective advanced settings components and clone settings components. The implementation follows the same pattern:
Implementation consistency verified across configuration models
- All three models use the same conditional logic:
shouldEnableAccountingPeriod ? true : settings.change_accounting_period - Form control is properly integrated in respective UI components
- Consistent usage in form value extraction for all three integrations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check implementation in other configuration models and usage of changeAccountingPeriod
# Check implementation in other configuration models
echo "Checking other configuration models..."
rg -A 3 -B 3 "changeAccountingPeriod.*new FormControl" src/app/core/models/
# Check usage of this form control
echo "Checking usage of changeAccountingPeriod..."
rg "changeAccountingPeriod" src/app/
Length of output: 7728
src/app/core/models/qbo/qbo-configuration/qbo-advanced-setting.model.ts (1)
99-99: Verify the implementation aligns with PR objectives
The current implementation seems to auto-enable the accounting period only when the existing setting is false/undefined. Let's verify if this matches the requirement to "automatically enable the accounting period toggle for all new organizations".
src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts (1)
140-140: Verify consistent implementation across models
The AI summary indicates similar changes in QBO and Xero models. Let's verify the consistency of this implementation.
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient information about the implementation across different models.
Implementation is consistent across models
The changeAccountingPeriod FormControl is consistently implemented across all three models (NetSuite, QBO, and Xero) with the same conditional logic using shouldEnableAccountingPeriod. The only difference is the path to access the setting in the configuration object, which is expected due to different API response structures:
- NetSuite:
advancedSettings?.configuration.change_accounting_period - QBO:
advancedSettings?.workspace_general_settings.change_accounting_period - Xero:
advancedSettings.workspace_general_settings.change_accounting_period
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar implementations in QBO and Xero models
echo "Searching for changeAccountingPeriod implementations..."
rg -A 2 "changeAccountingPeriod.*new FormControl" src/app/core/models/
# Verify method signatures
echo "Checking mapAPIResponseToFormGroup signatures..."
ast-grep --pattern 'static mapAPIResponseToFormGroup($params) {
$$$
}'
Length of output: 2196
src/app/core/models/xero/xero-configuration/xero-advanced-settings.model.ts
Show resolved
Hide resolved
src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/app/core/models/xero/xero-configuration/xero-advanced-settings.model.ts(2 hunks)src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/core/models/xero/xero-configuration/xero-advanced-settings.model.ts
🔇 Additional comments (2)
src/app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.ts (2)
10-10: LGTM! Clean implementation of new dependencies
The new imports and org property initialization are well-structured and follow Angular best practices.
Also applies to: 13-13, 17-17, 52-52
76-78: LGTM! Proper dependency injection
The new services are correctly injected following Angular's dependency injection pattern.
...app/integrations/xero/xero-shared/xero-advanced-settings/xero-advanced-settings.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/app/core/services/common/helper.service.ts (1)
38-41: Add input validation and error handlingWhile the implementation is functionally correct, it could benefit from proper input validation and error handling to make it more robust.
Consider adding input validation and error handling:
shouldAutoEnableAccountingPeriod(workspaceCreatedAt: Date): boolean { + if (!workspaceCreatedAt) { + throw new Error('workspaceCreatedAt is required'); + } const createdAt = new Date(workspaceCreatedAt); + if (isNaN(createdAt.getTime())) { + throw new Error('Invalid date format for workspaceCreatedAt'); + } return createdAt >= this.AUTO_ENABLE_ACCOUNTING_PERIOD_DATE; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts(2 hunks)src/app/core/models/qbo/qbo-configuration/qbo-advanced-setting.model.ts(1 hunks)src/app/core/services/common/helper.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts
- src/app/core/models/qbo/qbo-configuration/qbo-advanced-setting.model.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.ts (1)
10-10: Consider improving org property initializationThe org property initialization could be made more robust:
- Mark it as private since it's only used internally
- Consider moving the initialization to ngOnInit to ensure the cached org is available
- org: Org = this.orgService.getCachedOrg(); + private org: Org; ngOnInit(): void { + this.org = this.orgService.getCachedOrg(); this.getSettingsAndSetupForm(); }Also applies to: 20-20, 84-85
src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.ts (1)
Line range hint
293-373: Add error handling for API failuresThe
setupPage()method usesforkJoinbut doesn't handle potential API failures gracefully. Consider adding proper error handling to improve user experience.private setupPage(): void { this.setupOnboardingSteps(); const destinationAttributes = [ XeroFyleField.TAX_CODE, XeroFyleField.BANK_ACCOUNT ]; forkJoin([ this.cloneSettingService.getCloneSettings(), this.mappingService.getGroupedDestinationAttributes(destinationAttributes, 'v1', 'xero'), this.mappingService.getFyleFields('v1'), this.xeroConnectorService.getXeroCredentials(this.workspaceService.getWorkspaceId()), this.configurationService.getAdditionalEmails(), this.xeroImportSettingsService.getXeroField() - ]).subscribe(([cloneSetting, destinationAttributes, fyleFieldsResponse, xeroCredentials, adminEmails, xeroFields]) => { + ]).subscribe({ + next: ([cloneSetting, destinationAttributes, fyleFieldsResponse, xeroCredentials, adminEmails, xeroFields]) => { // ... existing code ... this.isLoading = false; - }); + }, + error: (error) => { + this.isLoading = false; + this.toastService.displayToastMessage(ToastSeverity.ERROR, 'Failed to load clone settings'); + console.error('Error loading clone settings:', error); + } + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/app/integrations/netsuite/netsuite-shared/netsuite-advanced-settings/netsuite-advanced-settings.component.ts(5 hunks)src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.ts(5 hunks)src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.ts(5 hunks)src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.ts(5 hunks)
🔇 Additional comments (9)
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.ts (2)
95-96: LGTM!
The OrgService injection is properly implemented following Angular's dependency injection pattern.
249-249: Verify consistent implementation across components
The auto-enable accounting period feature should be implemented consistently across NetSuite, QBO, and Xero components.
✅ Verification successful
Let me try a different search pattern to verify the implementation across components.
Implementation is consistent across major accounting integrations
The verification confirms that the shouldAutoEnableAccountingPeriod feature is consistently implemented across the major accounting integrations (QBO, Xero, and NetSuite) in their advanced settings components. Each integration:
- Uses the same helper method with
org.created_at - Applies it through the
mapAPIResponseToFormGroupmethod - Includes both main and clone settings components where applicable
The helper method implementation is also correct, comparing the workspace creation date against a predefined cutoff date.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent implementation of shouldAutoEnableAccountingPeriod across components
# Test 1: Check if all advanced settings components use shouldAutoEnableAccountingPeriod
echo "Checking advanced settings components for shouldAutoEnableAccountingPeriod usage:"
rg -l "shouldAutoEnableAccountingPeriod" "src/app/integrations/**/advanced-settings"
# Test 2: Verify helper method implementation
echo -e "\nChecking helper method implementation:"
rg -A 5 "shouldAutoEnableAccountingPeriod" "src/app/core/services/common/helper.service.ts"
Length of output: 745
Script:
#!/bin/bash
# Search for advanced settings components in integrations directory
echo "Searching for advanced settings components:"
fd "advanced-settings.component.ts" src/app/integrations
# Check usage of shouldAutoEnableAccountingPeriod and mapAPIResponseToFormGroup
echo -e "\nChecking implementation in components:"
rg -A 3 "mapAPIResponseToFormGroup.*shouldAutoEnableAccountingPeriod" src/app/integrations
Length of output: 5850
src/app/integrations/netsuite/netsuite-shared/netsuite-advanced-settings/netsuite-advanced-settings.component.ts (3)
14-14: LGTM: Clean implementation of org-related additions
The new imports and org property initialization are correctly implemented to support the auto-enable accounting period feature.
Also applies to: 24-24, 101-101
114-114: LGTM: Proper service injection
The OrgService is correctly injected into the constructor.
292-294:
Remove duplicate isLoading assignment
The isLoading flag is set to false twice in succession, which is redundant.
this.skipExportForm = SkipExportModel.setupSkipExportForm(this.expenseFilters, [], this.conditionFieldOptions);
- this.isLoading = false;
this.setupFormWatchers();
this.isLoading = false;Likely invalid or redundant comment.
src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.ts (2)
14-14: LGTM!
The import statement follows the project's conventions and is correctly placed with other model imports.
151-152: LGTM!
The OrgService injection follows Angular's dependency injection pattern and maintains consistent access modifiers.
src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.ts (2)
15-15: LGTM: Clean implementation of org-related changes
The addition of the OrgService import and org property initialization is well-structured and follows Angular best practices.
Also applies to: 158-158
205-206: LGTM: Proper dependency injection
The OrgService is correctly injected following Angular's dependency injection pattern.
.../netsuite/netsuite-shared/netsuite-advanced-settings/netsuite-advanced-settings.component.ts
Show resolved
Hide resolved
src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.ts
Show resolved
Hide resolved
src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.ts
Show resolved
Hide resolved
src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.spec.ts (2)
48-48: Remove trailing whitespace.There is a trailing whitespace at the end of line 48.
- const orgServiceSpy = jasmine.createSpyObj('OrgService', ['getCachedOrg']); + const orgServiceSpy = jasmine.createSpyObj('OrgService', ['getCachedOrg']);🧰 Tools
🪛 GitHub Check: lint
[failure] 48-48:
Trailing spaces not allowed🪛 eslint
[error] 48-48: Trailing spaces not allowed.
(no-trailing-spaces)
Missing test coverage and incorrect implementation of accounting period feature
The verification reveals two critical issues:
- No test coverage exists for
shouldAutoEnableAccountingPeriodin helper service spec- The
AUTO_ENABLE_ACCOUNTING_PERIOD_DATEconstant is incorrectly defined with a placeholder value'DEPLOY DATE ANISH'Required fixes:
- Add test cases in
helper.service.spec.tsto verify theshouldAutoEnableAccountingPeriodmethod with dates before and after the cutoff- Replace the placeholder
'DEPLOY DATE ANISH'with an actual deployment date inhelper.service.ts- Add test cases in
qbo-advanced-settings.component.spec.tsto verify the form initialization with both enabled and disabled accounting period based on org creation date🔗 Analysis chain
Line range hint
1-489: Verify test coverage for the new accounting period feature.While the service integration is complete, there are no specific test cases verifying the auto-enable accounting period functionality. Consider adding test cases to verify:
- The initialization of
changeAccountingPeriodform control based onshouldAutoEnableAccountingPeriod- Different scenarios based on organization creation date
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing test coverage of changeAccountingPeriod rg -l "changeAccountingPeriod.*setValue" "src/app/integrations" # Look for related component logic ast-grep --pattern 'shouldAutoEnableAccountingPeriod($_)'Length of output: 123
Script:
#!/bin/bash # Search for test files containing changeAccountingPeriod rg -A 5 "changeAccountingPeriod" "src/app/integrations/qbo" --type ts # Look for component implementation of shouldAutoEnableAccountingPeriod rg -A 5 "shouldAutoEnableAccountingPeriod" "src/app" # Search for test files containing shouldAutoEnableAccountingPeriod rg -A 5 "shouldAutoEnableAccountingPeriod" "src/app" --type specLength of output: 8460
Script:
#!/bin/bash # Check helper service test coverage rg -A 10 "AUTO_ENABLE_ACCOUNTING_PERIOD_DATE" "src/app/core/services/common/helper.service.spec.ts" # Look for test setup in QBO advanced settings rg -A 10 "describe.*ngOnInit" "src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.spec.ts" # Check for helper service constant definition rg "AUTO_ENABLE_ACCOUNTING_PERIOD_DATE" "src/app/core/services/common/helper.service.ts"Length of output: 1259
🧰 Tools
🪛 GitHub Check: lint
[failure] 48-48:
Trailing spaces not allowed🪛 eslint
[error] 48-48: Trailing spaces not allowed.
(no-trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.spec.ts(4 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.spec.ts
[failure] 48-48:
Trailing spaces not allowed
🪛 eslint
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.spec.ts
[error] 48-48: Trailing spaces not allowed.
(no-trailing-spaces)
🔇 Additional comments (2)
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.spec.ts (2)
20-21: LGTM! Service integration is properly implemented.
The OrgService integration is correctly set up with:
- Proper import of service and mock data
- Spy object creation with necessary methods
- Mock data initialization
Also applies to: 35-35, 47-50
65-66: LGTM! TestBed configuration is complete.
The OrgService is properly provided in the TestBed configuration and injected correctly.
Also applies to: 81-81
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.spec.ts (2)
Line range hint
397-436: Enhance skipExport form validation testThe test case for invalid skipExportForm could be more comprehensive:
- It only tests for null/empty values
- It doesn't verify the error messages or validation states
- It doesn't test boundary conditions
Consider expanding the test to include:
it('should handle various invalid skipExportForm scenarios', fakeAsync(() => { // Test required fields component.skipExportForm.patchValue({ condition1: null, operator1: '', value1: '' }); component['saveSkipExport'](); tick(); expect(component.skipExportForm.get('condition1')?.errors?.['required']).toBeTrue(); // Test invalid operator component.skipExportForm.patchValue({ condition1: { field_name: 'test' }, operator1: 'invalid_operator', value1: 'test' }); component['saveSkipExport'](); tick(); expect(skipExportService.postExpenseFilter).not.toHaveBeenCalled(); // Test boundary conditions component.skipExportForm.patchValue({ condition1: { field_name: 'test' }, operator1: Operator.IN, value1: [] // Empty array for IN operator }); component['saveSkipExport'](); tick(); expect(skipExportService.postExpenseFilter).not.toHaveBeenCalled(); }));
Line range hint
329-386: Improve test coverage for memo structure handlingWhile the basic memo structure scenarios are covered, consider adding tests for:
- Edge cases with special characters in memo fields
- Maximum length scenarios
- Invalid memo structure combinations
Add test cases like:
describe('memo structure edge cases', () => { it('should handle special characters in memo fields', () => { const specialCharMemo = [['<script>alert(1)</script>', '"test"']]; component.advancedSettingForm.get('memoStructure')?.setValue(specialCharMemo); component['createMemoStructureWatcher'](); expect(component.memoPreviewText).toBe('alert(1) - "test"'); }); it('should handle maximum length scenarios', () => { const longMemo = [['a'.repeat(1000), 'b'.repeat(1000)]]; component.advancedSettingForm.get('memoStructure')?.setValue(longMemo); component['createMemoStructureWatcher'](); expect(component.memoPreviewText.length).toBeLessThanOrEqual(2000); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/app/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.spec.ts(4 hunks)src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.ts
...pp/integrations/qbo/qbo-shared/qbo-advanced-settings/qbo-advanced-settings.component.spec.ts
Show resolved
Hide resolved
|
* feat: auto enable accounting period * shouldEnableAccountingPeriod * xero helperService * shouldEnableAccountingPeriod * orgService changes * lint fix * fix unit tests * lint fix
2 participants
Description
Clickup
Summary by CodeRabbit
New Features
changeAccountingPeriodform control across multiple settings models, allowing for conditional initialization based on organizational settings.Bug Fixes
changeAccountingPeriodcontrol, ensuring it reflects accurate conditions based on user settings.Tests