test: unit test the intacct export log pages#989
test: unit test the intacct export log pages#989JustARatherRidiculouslyLongUsername merged 12 commits intomasterfrom
Conversation
Currently the page size saved in localstorage does not update at all in intacct export logs. After this change, we store the new page size to localstorage on page size being updated
Currently the page size saved in localstorage does not update at all in intacct skipped export logs. After this change, we store the new page size to localstorage on page size being updated
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
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: 8
🧹 Outside diff range and nitpick comments (7)
src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-export-log.component.spec.ts (2)
33-36: Good addition: Test for initial navigation.The new test case effectively verifies the initial navigation behavior of the component. It checks both the active module setting and the correct URL navigation.
Consider adding a descriptive comment above the test case to explain what specific behavior it's testing. For example:
// Verifies that the component navigates to the completed export log page on initialization it('should navigate to completed export log', () => { // ... existing test code ... });This will improve the readability and maintainability of the test suite.
Line range hint
1-36: Consider expanding test coverage.The current test suite covers component creation and initial navigation, which is a good start. To improve the overall test coverage and robustness of the component, consider adding the following test cases:
- Test different navigation scenarios based on different active modules.
- Verify the behavior of the component when export data changes.
- Test any public methods or properties of the component that are not currently covered.
- Add edge case tests, such as handling empty module lists or unexpected data.
These additional tests will help ensure the component behaves correctly under various conditions and increase confidence in its functionality.
src/app/core/services/common/export-log.service.ts (1)
Line range hint
26-55: LGTM! Consider a minor improvement for consistency.The changes to make
selectedDateFilterandqueryoptional parameters improve the method's flexibility without breaking existing functionality. The internal logic handles these optional parameters correctly.For consistency, consider using the
workspaceIdclass property instead of callingthis.workspaceService.getWorkspaceId()again. Apply this small change:-const workspaceId = this.workspaceService.getWorkspaceId(); +const workspaceId = this.workspaceId;This change aligns with how
workspaceIdis used in the rest of the method and thegetExpenseGroupsmethod.src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-skip-export-log/intacct-skip-export-log.component.ts (2)
Line range hint
141-160: LGTM: Improved date range handling with a minor suggestionThe changes to
setupFormmethod significantly improve date range handling and calendar visibility management. The logic for updating theselectedDateFilterand triggering the expense fetch is now more robust.However, the use of
setTimeoutto toggle calendar visibility might cause a slight UI flicker. Consider using Angular'sChangeDetectorRefto trigger a manual change detection cycle instead, which could provide a smoother user experience.Here's a suggested refactor to avoid using
setTimeout:import { ChangeDetectorRef } from '@angular/core'; constructor( // ... other dependencies private cdr: ChangeDetectorRef ) { } // In setupForm method this.hideCalendar = true; this.selectedDateFilter = { startDate: dateRange[0], endDate: dateRange[1] }; this.isDateSelected = true; this.cdr.detectChanges(); this.hideCalendar = false; this.getSkippedExpenses(paginator.limit, paginator.offset);
Line range hint
163-171: LGTM: Improved initialization logicThe new
getSkippedExpensesAndSetupPagemethod effectively encapsulates the component's initialization logic, improving code organization and readability. It correctly sets up the form, initializes pagination, and fetches initial data.To complete this refactoring, ensure that
ngOnInitis updated to use this new method:ngOnInit(): void { this.getSkippedExpensesAndSetupPage(); }src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.spec.ts (2)
58-66: Consider adding an assertion to verify 'getExpenseGroups' is called during initialization.While the test verifies the component properties after initialization, adding an expectation to ensure
exportLogService.getExpenseGroupsis called will confirm that data loading is triggered as expected.You can add:
expect(exportLogService.getExpenseGroups).toHaveBeenCalled();
128-135: Ensure consistent use of 'undefined' and 'null' in method parameters.In the call to
getExpenseGroups, consider usingundefinedconsistently for optional parameters unlessnullhas a specific meaning. This can help avoid potential confusion and bugs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- src/app/core/services/common/export-log.service.ts (2 hunks)
- src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.spec.ts (1 hunks)
- src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.ts (2 hunks)
- src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-export-log.component.spec.ts (2 hunks)
- src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-skip-export-log/intacct-skip-export-log.component.spec.ts (1 hunks)
- src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-skip-export-log/intacct-skip-export-log.component.ts (3 hunks)
- src/app/integrations/intacct/intacct.fixture.ts (2 hunks)
🧰 Additional context used
🪛 Biome
src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-skip-export-log/intacct-skip-export-log.component.spec.ts
[error] 46-52: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (32)
src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-export-log.component.spec.ts (3)
6-6: LGTM: Router import added correctly.The import statement for
provideRouterandRouterfrom '@angular/router' is correctly added. This is necessary for the new router-related functionality in the tests.
8-8: Great: Test suite activated.Changing from
xdescribetodescribeactivates the test suite for IntacctExportLogComponent. This is an important change as it ensures that these tests will now be executed as part of the test run.
11-11: Well done: Router setup for testing.The changes made to set up the router for testing are correct and follow good practices:
- Declaring the
routervariable for use in tests.- Adding
provideRouter([])to the providers array in the TestBed configuration.- Injecting the router and setting up a spy on
navigateByUrl.These changes enable proper testing of navigation-related functionality in the component.
Also applies to: 17-17, 21-23
src/app/core/services/common/export-log.service.ts (2)
Line range hint
57-89: LGTM! The changes improve method flexibility.The modification to make
selectedDateFilteran optional parameter enhances the method's flexibility without altering its core functionality. The internal logic correctly handles this optional parameter, and the overall structure of the method remains sound.
Line range hint
1-89: Overall, the changes enhance method flexibility and align with PR objectives.The modifications to
getSkippedExpensesandgetExpenseGroupsmethods improve their flexibility by making certain parameters optional. This change aligns well with the PR objectives of implementing unit tests for the Intacct export log pages, as it allows for more versatile method calls during testing.The internal logic of both methods remains intact, ensuring that existing functionality is preserved while accommodating the new optional parameters. These changes should facilitate easier and more comprehensive unit testing of the Intacct export log pages.
src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-skip-export-log/intacct-skip-export-log.component.ts (4)
43-43: LGTM: Improved property flexibilityMaking
selectedDateFilteroptional enhances the component's flexibility, allowing for better handling of cases where the date filter might not be set initially or cleared during runtime.
63-63: LGTM: Enhanced property flexibilityMaking
searchQueryoptional improves the component's ability to handle scenarios where a search query might not be present, aligning with best practices for optional properties in TypeScript.
Line range hint
1-185: Overall assessment: Well-structured improvements with minor suggestionsThe changes in this file significantly improve the
IntacctSkipExportLogComponent. Key improvements include:
- Enhanced flexibility with optional properties.
- Improved date range handling and calendar visibility management.
- Better organization of initialization logic.
The code changes align well with the PR objectives of implementing unit tests for the Intacct export log pages. While the actual test implementations are not visible in this file, these changes lay a solid foundation for testable component behavior.
Consider implementing the suggested minor improvements for an even more robust and maintainable component.
95-95: Verify impact of delayed limit assignmentMoving the
limitassignment to the end of the method ensures it's updated after the API call. This change looks good, but please verify that any code depending on the updatedlimitvalue isn't affected by this delayed assignment.src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.ts (2)
45-45: LGTM: Improved property signature forselectedDateFilterThe change to make
selectedDateFilteroptional (?) is a good improvement. It better reflects the property's usage in the component, allowing for cases where it might be undefined. This change enhances type safety without introducing breaking changes.
Line range hint
1-191: Verify alignment with PR objectivesThe PR objectives mention implementing unit tests for the Intacct export log pages. However, this file doesn't contain any test-related changes. It only includes minor improvements to the component itself.
Could you please clarify if there are separate test files that implement the unit tests mentioned in the PR objectives? If so, please include them in the review. If not, consider updating the PR description to accurately reflect the changes made.
src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.spec.ts (8)
1-47: Setup and dependency injection are correctly implemented.The test setup and dependency injection for the services are properly configured using spies.
53-55: Component creation test is valid.The basic creation test ensures that the component instance is created successfully.
68-75: Page size change handling is correctly tested.The test correctly verifies that changing the page size updates the limit, resets the current page, stores the new page size, and fetches the expense groups.
77-83: Page change handling is correctly tested.The test verifies that changing the page updates the offset and current page, and triggers data fetching.
85-94: Simple search functionality is correctly tested.The test ensures that searching updates the search query, resets the offset and current page, and fetches the expense groups after the debounce period.
96-101: Opening expense in Fyle is correctly tested.The test ensures that
window.openis called with the correct expense ID and target.
103-115: Date filter change handling is correctly tested.The test verifies that setting a date range updates the
selectedDateFilter, setsisDateSelectedto true, fetches the expense groups, and displays the calendar as expected.
117-124: Clearing date filter is correctly tested.The test ensures that setting the date filter to null clears the
selectedDateFilter, setsisDateSelectedto false, and fetches the expense groups.src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-skip-export-log/intacct-skip-export-log.component.spec.ts (10)
1-12: LGTM on imports and initial setup.The imports and initial test setup correctly include all necessary modules, services, and mocks required for the unit tests.
58-66: Well-written initialization test.The test for component initialization effectively verifies that the component's properties are set correctly based on the mocked service responses, ensuring the component loads data as expected.
68-76: Effective test for page size changes.The test correctly checks that updating the page size adjusts the component's
limitandcurrentPageproperties, stores the new page size, and triggers a data reload.
77-85: Proper handling of page navigation in tests.The test ensures that changing pages updates the
offsetandcurrentPage, and that the component fetches the new data accordingly.
86-96: Comprehensive test for simple search functionality.The use of
fakeAsyncandtickaccurately simulates asynchronous behavior, ensuring the search functionality updates the component state and triggers the expected service call.
97-109: Accurate testing of date filter changes.The test verifies that setting a date range updates the
selectedDateFilterandisDateSelectedproperties, and that the component correctly reloads the filtered data.
110-118: Effective test for clearing date filters.The test confirms that clearing the date filter resets the relevant properties and fetches the data without any date constraints.
119-128: Verification of service method calls with correct parameters.The test ensures that
getSkippedExpensesis called with the correct arguments, validating the integration with the export log service.
129-134: Proper tracking of date filter usage.The test confirms that the
trackDateFiltermethod of the tracking service is called with the appropriate parameters, ensuring analytics are correctly recorded.
135-143: Correct handling of asynchronous UI updates.Using
fakeAsyncandtickaccurately tests thathideCalendaris updated after a timeout, ensuring the UI behaves as expected when interacting with the date picker.src/app/integrations/intacct/intacct.fixture.ts (3)
8-10: New imports are appropriate and necessaryThe added imports for
ExpenseGroup,ExpenseGroupDescription,ExpenseGroupResponse,Paginator, andSkipExportLogResponseare correctly included to support the newly added mock data structures.
469-472:mockPaginatoris correctly definedThe
mockPaginatorobject is properly set withlimitandoffsetvalues, which will be useful for testing pagination functionality.
374-374: Confirm the use of negative amounts inamountfieldThe
amountfield in the expenses has negative values (-460.0,-550.0). Verify whether negative amounts are appropriate in this context, as they might represent refunds or credits.To ensure consistency, run the following script to check how
amountvalues are used across expense groups:Also applies to: 402-402
...in/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.ts
Show resolved
Hide resolved
...tacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.spec.ts
Show resolved
Hide resolved
...ct-main/intacct-export-log/intacct-skip-export-log/intacct-skip-export-log.component.spec.ts
Show resolved
Hide resolved
|
|
||
| fixture = TestBed.createComponent(IntacctCompletedExportLogComponent); | ||
| component = fixture.componentInstance; | ||
| fixture.detectChanges(); |
There was a problem hiding this comment.
why is this line removed?
There was a problem hiding this comment.
Just because all tests dont require a full re-render, for example should handle page size changes. The tests that require a component render calls fixture.detectChanges() within the test case. For example: should handle page changes
| dateOptions: DateFilter[] = AccountingExportModel.getDateOptionsV2(); | ||
|
|
||
| selectedDateFilter: SelectedDateFilter | null; | ||
| selectedDateFilter?: SelectedDateFilter | null; |
There was a problem hiding this comment.
why are we add '?' here
There was a problem hiding this comment.
Since the default value saved of selectedDate is undefined, this test case was not passing the ts type check. In it, we are testing the case where we call the exportLogService.getExpenseGroups with this.selectedDateFilter set to undefined (this line):
this.exportLogService.getExpenseGroups(TaskLogState.COMPLETE, limit, offset, this.selectedDateFilter, null, this.searchQuery)
| dateOptions: DateFilter[] = AccountingExportModel.getDateOptionsV2(); | ||
|
|
||
| selectedDateFilter: SelectedDateFilter | null; | ||
| selectedDateFilter?: SelectedDateFilter | null; |
There was a problem hiding this comment.
same reason
| readonly brandingConfig = brandingConfig; | ||
|
|
||
| searchQuery: string | null; | ||
| searchQuery?: string | null; |
There was a problem hiding this comment.
same reason
| this.currentPage = 1; | ||
| this.selectedDateFilter = this.selectedDateFilter ? this.selectedDateFilter : null; | ||
| this.getSkippedExpenses(limit, this.offset); | ||
| this.limit = limit; |
There was a problem hiding this comment.
why we change the position of the limit, any reason
There was a problem hiding this comment.
Yes, since calling this.getSkippedExpenses with limit and this.limit set to the same value prevents it from updating the localstorage with the new value because of this check. Essentially that leaves the localstorage functionality unused in this page. Calling this.getSkippedExpenses before updating this.limit updates the localStorage value as intended.
I've also raised a separate PR (#990) for this
3 participants
Clickup
https://app.clickup.com/t/86cwh86cx
https://app.clickup.com/t/86cwh86cu
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
IntacctCompletedExportLogComponentandIntacctSkipExportLogComponent, including various functionalities like initialization, pagination, and search handling.Chores