test: qbo base mapping #999
Conversation
WalkthroughThe pull request introduces comprehensive enhancements to the test suite for 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: 4
🧹 Outside diff range and nitpick comments (2)
src/app/integrations/qbo/qbo.fixture.ts (1)
2446-2459: Approve new constant, but fix indentation.The new
mockMappingSettingconstant is well-structured and correctly typed. It provides a good mock object for testing purposes. However, the indentation is inconsistent, mixing 2 and 4 space indents.Please adjust the indentation to be consistent. Here's the suggested fix:
export const mockMappingSetting: MappingSetting[] = [ - { - source_field: 'VENDOR', - destination_field: AccountingField.ACCOUNT, - id: 0, - created_at: new Date(), - updated_at: new Date(), - workspace: 0, - import_to_fyle: false, - is_custom: false, - source_placeholder: null - } + { + source_field: 'VENDOR', + destination_field: AccountingField.ACCOUNT, + id: 0, + created_at: new Date(), + updated_at: new Date(), + workspace: 0, + import_to_fyle: false, + is_custom: false, + source_placeholder: null + } ];src/app/integrations/qbo/qbo-main/qbo-mapping/qbo-base-mapping/qbo-base-mapping.component.spec.ts (1)
58-65: Enhance loading state assertions in 'should trigger auto map employees successfully' testConsider verifying that
component.isLoadingis correctly set totruewhen the operation begins and then tofalseafter it completes. This ensures the loading indicator functions as expected.You might add assertions like:
expect(component.isLoading).toBeTrue(); // Trigger the operation component.triggerAutoMapEmployees(); // After the operation completes expect(component.isLoading).toBeFalse();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/app/integrations/qbo/qbo-main/qbo-mapping/qbo-base-mapping/qbo-base-mapping.component.spec.ts (1 hunks)
- src/app/integrations/qbo/qbo.fixture.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/app/integrations/qbo/qbo.fixture.ts (1)
2-2: LGTM: Import statement is correct and consistent.The new import statement for
AccountingFieldis properly formatted and consistent with other imports in the file. It's correctly importing from a relative path.
| it('should return employee_field_mapping when sourceField is EMPLOYEE', () => { | ||
| component.sourceField = FyleField.EMPLOYEE; | ||
| const workspaceGeneralSetting = { employee_field_mapping: 'VENDOR' } as QBOWorkspaceGeneralSetting; | ||
| const mappingSettings: MappingSetting[] = []; | ||
|
|
||
| const result = (component as any).getDestinationField(workspaceGeneralSetting, mappingSettings); | ||
| expect(result).toBe('VENDOR'); | ||
| }); |
There was a problem hiding this comment.
Remove duplicate test cases for 'should return employee_field_mapping when sourceField is EMPLOYEE'
The test case in lines 137-144 is a duplicate of the one in lines 89-96. Please remove the duplicate to avoid redundancy.
Apply this diff to remove the duplicate test case:
- it('should return employee_field_mapping when sourceField is EMPLOYEE', () => {
- component.sourceField = FyleField.EMPLOYEE;
- const workspaceGeneralSetting = { employee_field_mapping: 'VENDOR' } as QBOWorkspaceGeneralSetting;
- const mappingSettings: MappingSetting[] = [];
-
- const result = (component as any).getDestinationField(workspaceGeneralSetting, mappingSettings);
- expect(result).toBe('VENDOR');
- });Also applies to: 137-144
| it('should return ACCOUNT when sourceField is CATEGORY', () => { | ||
| component.sourceField = FyleField.CATEGORY; | ||
| const workspaceGeneralSetting = {} as QBOWorkspaceGeneralSetting; | ||
| const mappingSettings: MappingSetting[] = []; | ||
|
|
||
| const result = (component as any).getDestinationField(workspaceGeneralSetting, mappingSettings); | ||
| expect(result).toBe(AccountingField.ACCOUNT); | ||
| }); |
There was a problem hiding this comment.
Remove duplicate test cases for 'should return ACCOUNT when sourceField is CATEGORY'
The test case in lines 146-153 is duplicated from lines 98-105. Consider removing the duplicate to keep the test suite concise.
Apply this diff to remove the duplicate test case:
- it('should return ACCOUNT when sourceField is CATEGORY', () => {
- component.sourceField = FyleField.CATEGORY;
- const workspaceGeneralSetting = {} as QBOWorkspaceGeneralSetting;
- const mappingSettings: MappingSetting[] = [];
-
- const result = (component as any).getDestinationField(workspaceGeneralSetting, mappingSettings);
- expect(result).toBe(AccountingField.ACCOUNT);
- });Also applies to: 146-153
| it('should return empty string if no matching mapping setting is found', () => { | ||
| component.sourceField = FyleField.VENDOR; | ||
| const workspaceGeneralSetting = {} as QBOWorkspaceGeneralSetting; | ||
| const mappingSettings: MappingSetting[] = []; | ||
|
|
||
| const result = (component as any).getDestinationField(workspaceGeneralSetting, mappingSettings); | ||
| expect(result).toBe(''); | ||
| }); |
There was a problem hiding this comment.
Remove duplicate test cases for 'should return empty string if no matching mapping setting is found'
The test case in lines 163-170 is a duplicate of the one in lines 128-135. Removing duplicates helps maintain clarity in your tests.
Apply this diff to remove the duplicate test case:
- it('should return empty string if no matching mapping setting is found', () => {
- component.sourceField = FyleField.VENDOR;
- const workspaceGeneralSetting = {} as QBOWorkspaceGeneralSetting;
- const mappingSettings: MappingSetting[] = [];
-
- const result = (component as any).getDestinationField(workspaceGeneralSetting, mappingSettings);
- expect(result).toBe('');
- });Also applies to: 163-170
| it('should handle route parameter changes', () => { | ||
| mockWorkspaceService.getWorkspaceGeneralSettings.and.returnValue(of(mockGeneralSettings)); | ||
| mockMappingService.getMappingSettings.and.returnValue(of({ count: 1, next: null, previous: null, results: mockImportSettings.mapping_settings })); | ||
| mockImportSettingsService.getImportSettings.and.returnValue(of(mockImportSettings)); | ||
| mockMappingService.getPaginatedDestinationAttributes.and.returnValue(of(mockCreditCardAccounts)); | ||
|
|
||
| mockActivatedRoute.params = of({ source_field: 'Vendor' }); | ||
| fixture.detectChanges(); | ||
|
|
||
| expect(component.sourceField).toBe(FyleField.EMPLOYEE); | ||
| expect(component.isLoading).toBeFalse(); | ||
| }); |
There was a problem hiding this comment.
Correct the expectation in 'should handle route parameter changes' test
In this test, you're setting mockActivatedRoute.params = of({ source_field: 'Vendor' }); (line 82), but the expectation on line 85 is expect(component.sourceField).toBe(FyleField.EMPLOYEE);. Shouldn't component.sourceField reflect the updated route parameter and be FyleField.VENDOR instead?
Apply this diff to correct the expectation:
- expect(component.sourceField).toBe(FyleField.EMPLOYEE);
+ expect(component.sourceField).toBe(FyleField.VENDOR);📝 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.
| it('should handle route parameter changes', () => { | |
| mockWorkspaceService.getWorkspaceGeneralSettings.and.returnValue(of(mockGeneralSettings)); | |
| mockMappingService.getMappingSettings.and.returnValue(of({ count: 1, next: null, previous: null, results: mockImportSettings.mapping_settings })); | |
| mockImportSettingsService.getImportSettings.and.returnValue(of(mockImportSettings)); | |
| mockMappingService.getPaginatedDestinationAttributes.and.returnValue(of(mockCreditCardAccounts)); | |
| mockActivatedRoute.params = of({ source_field: 'Vendor' }); | |
| fixture.detectChanges(); | |
| expect(component.sourceField).toBe(FyleField.EMPLOYEE); | |
| expect(component.isLoading).toBeFalse(); | |
| }); | |
| it('should handle route parameter changes', () => { | |
| mockWorkspaceService.getWorkspaceGeneralSettings.and.returnValue(of(mockGeneralSettings)); | |
| mockMappingService.getMappingSettings.and.returnValue(of({ count: 1, next: null, previous: null, results: mockImportSettings.mapping_settings })); | |
| mockImportSettingsService.getImportSettings.and.returnValue(of(mockImportSettings)); | |
| mockMappingService.getPaginatedDestinationAttributes.and.returnValue(of(mockCreditCardAccounts)); | |
| mockActivatedRoute.params = of({ source_field: 'Vendor' }); | |
| fixture.detectChanges(); | |
| expect(component.sourceField).toBe(FyleField.VENDOR); | |
| expect(component.isLoading).toBeFalse(); | |
| }); |
Description
Clickup
https://app.clickup.com/t/86cwcuq1z
Summary by CodeRabbit
New Features
Bug Fixes
triggerAutoMapEmployeesmethod tests.Documentation