-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: intacct advanced settings save functionality #1026
test: intacct advanced settings save functionality #1026
Conversation
WalkthroughThe pull request enhances the test suite for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.spec.ts (4)
169-169
: Avoid usingas any
; provide a proper type forskipExportChild
At line 169,
skipExportChild
is assigned an object cast asany
. This bypasses TypeScript's type checking and can lead to unexpected errors. Define an appropriate type or use the actual component reference to maintain strong typing.Apply this change to define a proper type:
- component.skipExportChild = { saveSkipExportFields: jasmine.createSpy('saveSkipExportFields') } as any; + component.skipExportChild = { + saveSkipExportFields: jasmine.createSpy('saveSkipExportFields') + } as jasmine.SpyObj<SkipExportComponent>;Ensure that
SkipExportComponent
is imported and used correctly in the test setup.
196-199
: Avoid hard-coded IDs in tests for better maintainabilityIn lines 197-199, the test uses hard-coded IDs (
1
and2
) when verifying calls todeleteExpenseFilter
. Relying on hard-coded values can make tests brittle and harder to maintain. Consider defining constants or obtaining these IDs dynamically to enhance readability and maintainability.Apply this change:
- expect(advancedSettingsService.deleteExpenseFilter).toHaveBeenCalledWith(1); - expect(advancedSettingsService.deleteExpenseFilter).toHaveBeenCalledWith(2); + const expenseFilterId1 = 1; // Replace with a meaningful constant or obtained ID + const expenseFilterId2 = 2; // Replace with a meaningful constant or obtained ID + expect(advancedSettingsService.deleteExpenseFilter).toHaveBeenCalledWith(expenseFilterId1); + expect(advancedSettingsService.deleteExpenseFilter).toHaveBeenCalledWith(expenseFilterId2);
218-220
: Provide more informative error handling in testsIn the error handling test at lines 218-220, the error message is simply
'Error'
. Providing a more descriptive error message or using an actual error object can improve test clarity and ensure that the component handles various error scenarios appropriately.Consider updating the test as follows:
- const getError = () => new Error('Error'); + const getError = () => new Error('Network error');This makes the error scenario more specific and can help in verifying that the component displays appropriate messages to the user.
78-107
: Add assertions to verify all critical form fields and component propertiesIn the initialization tests between lines 78-107, while several properties are being asserted, some component properties or form controls may not be verified. To ensure comprehensive test coverage, consider adding assertions for all critical properties and form controls.
Review and add assertions as needed:
expect(component.advancedSettingsForm.get('someOtherControl')?.value).toEqual(expectedValue); expect(component.anotherComponentProperty).toBe(expectedState);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.spec.ts (1 hunks)
- src/app/integrations/intacct/intacct.fixture.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/app/integrations/intacct/intacct.fixture.ts (3)
954-981
: Advanced Settings Initialization Looks GoodThe
advancedSettings
constant is correctly defined with a proper structure and appropriate values.
983-985
: Expense Filter Initialization Looks GoodThe
expenseFilter
constant is properly initialized with acount
property set to0
.
987-994
: Grouped Attributes Are Correctly DefinedThe
groupedAttributes
constant is properly defined with the expected keys and correspondingIntacctDestinationAttribute
arrays.
...intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.spec.ts
Outdated
Show resolved
Hide resolved
...intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.spec.ts
Show resolved
Hide resolved
...intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.spec.ts
Show resolved
Hide resolved
...intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.spec.ts (2)
164-183
: Missing Assertions on Service CallsIn the save functionality test during onboarding, there are multiple service calls, but not all are asserted. To ensure the test is robust, include assertions for all expected service interactions.
Consider adding assertions for the following service calls:
workspaceService.getIntacctOnboardingState
component.skipExportChild.saveSkipExportFields
Update the test to include these assertions.
40-40
: Ensure All Spied Methods Are UsedIn the spy object
mappingServiceSpy
, methods likerefreshSageIntacctDimensions
andrefreshFyleDimensions
are included but not used in the tests. Remove unused methods from the spy object to keep the test setup clean.Apply this diff to remove unused methods:
- const mappingServiceSpy = jasmine.createSpyObj('SiMappingsService', ['getGroupedDestinationAttributes', 'getConfiguration', 'refreshSageIntacctDimensions', 'refreshFyleDimensions']); + const mappingServiceSpy = jasmine.createSpyObj('SiMappingsService', ['getGroupedDestinationAttributes', 'getConfiguration']);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.spec.ts (1 hunks)
- src/app/integrations/intacct/intacct.fixture.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (7)
src/app/integrations/intacct/intacct.fixture.ts (3)
14-14
: LGTM: New imports are appropriate.The added imports for
GroupedDestinationAttribute
,IntacctDestinationAttribute
,QBDEmailOptions
, andAdvancedSettingsGet
are relevant to the new constants introduced in this file.Also applies to: 16-17
949-985
: LGTM: New constants are well-structured and appropriate for testing.The newly added constants
adminEmails
,advancedSettings
, andexpenseFilter
are well-structured and contain appropriate mock data for testing purposes. The use of type assertions ensures type safety when using these constants in tests.
987-1002
: LGTM: New constants enhance test data coverage.The newly added constants
groupedAttributes
andconfigurationForAddvancedSettings
are well-structured and provide comprehensive mock data for testing various scenarios. The use of enums and type assertions ensures type safety.Overall, the changes in this file significantly enhance the test data coverage for Intacct integration, particularly for advanced settings and grouped attributes. This will allow for more thorough and accurate testing of the related functionality.
src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.spec.ts (4)
198-199
: Redundant Calls todeleteExpenseFilter
The test checks for two calls to
advancedSettingsService.deleteExpenseFilter
with IDs1
and2
. Ensure that these IDs are correct and that both calls are necessary. If the IDs are dynamic or fetched from a list, consider using a loop or parameterized test.Verify that IDs
1
and2
correspond to existing filters.
175-175
: Missingtick()
After Async OperationIn the test, there is a call to
component.save();
followed bytick();
. Ensure that all asynchronous operations have correspondingtick()
calls to simulate the passage of time correctly.
126-127
:⚠️ Potential issuePotential Flaky Test Due to Dynamic Date
The test uses
new Date(Date.now()).toLocaleDateString()
to generate a date inmemoPreviewText
, which can lead to flaky tests if the date changes or in different time zones. It's better to use a fixed date to ensure consistent test results.Apply this diff to use a fixed date in the test:
- expect(component.memoPreviewText).toBe('Client Meeting - Meals and Entertainment - ' + new Date(Date.now()).toLocaleDateString()); + const fixedDate = '01/01/2021'; + expect(component.memoPreviewText).toBe('Client Meeting - Meals and Entertainment - ' + fixedDate);And update the component or mock the date accordingly.
Likely invalid or redundant comment.
83-83
: Possible Issue with Email Concatenation OrderThe concatenation of
adminEmails
andadvancedSettings.workspace_schedules.additional_email_options
may result in the emails being in an unintended order. Verify if this is the desired sequence.Run the following script to check the order of concatenated emails:
Clickup
https://app.clickup.com/t/86cwh86cy
Summary by CodeRabbit