Add a user action log when user change notification configuration (#7339) #7393
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across various controllers and services within the Additionally, the test classes for Assessment against linked issues
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (11)
src/test/cypress/cypress/integration/UserActionLogs.spec.js (5)
60-63: LGTM: Pagination check updated to reflect new action countThe changes in the pagination check correctly account for the addition of the 'NOTIFICATION_CONFIG' action. The increase in the number of table lines and the adjustment of action orders are consistent with the new feature implementation.
Consider adding a comment explaining why the number of lines has increased to 3, to improve test readability and maintainability.
Line range hint
83-113: LGTM: Export check updated to include new 'NOTIFICATION_CONFIG' actionThe changes in the export check accurately reflect the addition of the 'NOTIFICATION_CONFIG' action. The increase in the expected number of results and the inclusion of the new action in the exported rows are consistent with the implementation of the new feature.
To improve test robustness, consider adding an assertion to verify the presence of the 'NOTIFICATION_CONFIG' action regardless of its specific index in the exported data. This would make the test less brittle to future changes in the order of actions.
122-128: LGTM: Card details accessibility check updatedThe changes in the card details accessibility check correctly account for the addition of the 'NOTIFICATION_CONFIG' action. The increase in the expected number of results and the adjustment of line numbers clicked are consistent with the new feature implementation.
To improve test maintainability, consider using constants or calculated values for the line numbers instead of hard-coded values. This would make the test more resilient to future changes in the number or order of actions.
159-159: LGTM: Added notification configuration change to traced actionsThe addition of the 'changeNotificationConfiguration()' function call is consistent with the PR objective of implementing a user action log for notification configuration changes. This ensures that the new action is included in the sequence of actions to be traced.
Consider adding a comment above this line explaining the purpose of adding this action, to improve code readability and maintainability.
183-189: LGTM: Added function to change notification configurationThe new 'changeNotificationConfiguration' function correctly implements the steps to change notification settings, which is essential for testing the new 'NOTIFICATION_CONFIG' action. The implementation includes appropriate navigation and interaction with UI elements.
To improve test robustness, consider adding assertions after each step to verify that the expected changes have been applied successfully. For example, you could check that the checkbox is indeed unchecked after line 185.
src/docs/asciidoc/reference_doc/users_management.adoc (1)
202-202: Approved: New user action log entry added.The addition of
NOTIFICATION_CONFIGto the list of user actions that can be logged is correct and aligns with the PR objectives.Consider adding a brief explanation of what
NOTIFICATION_CONFIGrepresents, similar to how other action types are described in the codebase. This would provide more context for users of the documentation. For example:- NOTIFICATION_CONFIG + NOTIFICATION_CONFIG: Logs changes made to user notification configurationsservices/users/src/test/java/org/opfab/users/services/CurrentUserWithPerimetersServiceShould.java (1)
57-57: Update to UserSettingsService constructor looks correct, but could be improved.The change correctly updates the
UserSettingsServiceconstructor call to match its new signature. However, I have a few suggestions to improve clarity and robustness:
Consider adding a comment explaining the purpose of the new parameters. This will help other developers understand why they're set to
nullandfalsein the test context.Using
nullfor a dependency parameter might hide potential issues if the dependency is required in some scenarios. Consider using a mock or stub object instead, which could provide more realistic behavior and catch potential issues earlier.Here's a suggested improvement:
// New parameters: userActionLogService (not needed for these tests) and userActionLogActivated flag UserActionLogService mockUserActionLogService = Mockito.mock(UserActionLogService.class); boolean userActionLogActivated = false; userSettingsService = new UserSettingsService(userSettingsRepositoryStub, usersServiceStub, null, mockUserActionLogService, userActionLogActivated);This approach provides more context and uses a mock object instead of
null, which can be safer and more flexible for future test scenarios.services/users/src/test/java/org/opfab/users/services/UserSettingsServiceShould.java (2)
167-176: LGTM: Improved test clarity with consistent user naming.The change from "user3" to "userWithNoSettings" improves the test's clarity by using a more descriptive and consistent user name. This aligns well with the setup in the
clear()method.Consider updating the test method name to reflect this change, e.g., "GIVEN_User_With_No_Settings_WHEN_Patch_Settings_THEN_Settings_Are_Created".
178-189: LGTM: Good addition of edge case test.This new test method effectively covers the scenario of attempting to patch settings for a non-existing user. It correctly verifies both the error type and the error message.
Consider adding an assertion to verify that the settings were not actually created for the non-existing user. This could be done by checking that
userSettingsRepositoryStub.findById("notExistingUser")returns an empty result.services/users/src/main/java/org/opfab/users/services/UserSettingsService.java (1)
106-112: UseStringBuilderinstead ofStringBufferfor better performanceThe method
getProcessesStatesNotNotifiedTextusesStringBuffer, which is synchronized and may introduce unnecessary overhead in a single-threaded context. Consider usingStringBuilderfor improved performance.Apply this diff to make the change:
-private String getProcessesStatesNotNotifiedText(Map<String, List<String>> processesStatesNotNotified) { - StringBuffer sb = new StringBuffer(); +private String getProcessesStatesNotNotifiedText(Map<String, List<String>> processesStatesNotNotified) { + StringBuilder sb = new StringBuilder(); processesStatesNotNotified.forEach((process, states) -> { sb.append(process).append(": [").append(String.join(",", states)).append("]\n"); }); return sb.toString(); }services/users/src/main/java/org/opfab/users/controllers/CurrentUserWithPerimetersController.java (1)
45-45: Consistency in Configuration Property InjectionWhile injecting configuration properties via constructor parameters with
@Valueis valid, it's more common to inject them into fields for consistency and clarity. Consider refactoring as follows:@Value("${operatorfabric.userActionLogActivated:true}") private boolean userActionLogActivated;This keeps all injected properties in one place and improves code readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- services/users/src/main/java/org/opfab/users/controllers/CurrentUserWithPerimetersController.java (3 hunks)
- services/users/src/main/java/org/opfab/users/controllers/NotificationConfigurationController.java (2 hunks)
- services/users/src/main/java/org/opfab/users/controllers/UserWithPerimetersController.java (3 hunks)
- services/users/src/main/java/org/opfab/users/controllers/UsersController.java (3 hunks)
- services/users/src/main/java/org/opfab/users/services/UserSettingsService.java (4 hunks)
- services/users/src/test/java/org/opfab/users/services/CurrentUserWithPerimetersServiceShould.java (1 hunks)
- services/users/src/test/java/org/opfab/users/services/UserSettingsServiceShould.java (3 hunks)
- src/docs/asciidoc/reference_doc/users_management.adoc (1 hunks)
- src/test/cypress/cypress/integration/UserActionLogs.spec.js (6 hunks)
- tools/user-action-tracing/src/main/java/org/opfab/useractiontracing/model/UserActionEnum.java (1 hunks)
- ui/main/src/app/business/model/user-action-log.model.ts (1 hunks)
- ui/main/src/app/business/view/useractionlogs/userActionLogsPageDescription.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (19)
tools/user-action-tracing/src/main/java/org/opfab/useractiontracing/model/UserActionEnum.java (1)
13-13: LGTM! The new enum constant aligns with the PR objectives.The addition of
NOTIFICATION_CONFIGto theUserActionEnumis consistent with the PR objectives to implement a user action log for notification configuration changes. The naming convention follows the established pattern, and the placement at the end of the enum list maintains backward compatibility.ui/main/src/app/business/view/useractionlogs/userActionLogsPageDescription.ts (1)
23-24: LGTM! Verify related parts of the codebase.The addition of 'NOTIFICATION_CONFIG' to the
actionListaligns well with the PR objective of implementing a user action log for notification configuration changes. The syntax is correct, and the placement at the end of the list is appropriate.To ensure consistency across the codebase, please verify:
- Any components or services that use this action list are updated accordingly.
- If action names are localized, ensure 'NOTIFICATION_CONFIG' is added to the relevant translation files.
Run the following script to check for potential areas that might need updates:
Review the output to identify any areas that might need additional updates.
✅ Verification successful
Verified the addition of 'NOTIFICATION_CONFIG'. No further updates are required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of actionList or NOTIFICATION_CONFIG # Test 1: Search for files that might be using the actionList echo "Files potentially using actionList:" rg --type typescript "actionList" ./ui/main/src # Test 2: Search for any existing uses of NOTIFICATION_CONFIG echo "\nExisting uses of NOTIFICATION_CONFIG:" rg --type typescript "NOTIFICATION_CONFIG" ./ui/main/src # Test 3: Check translation files for action names echo "\nTranslation files that might need updating:" fd -e json -e properties "translation" ./ui/main/srcLength of output: 498
Script:
#!/bin/bash # Description: Search for usages of actionList or NOTIFICATION_CONFIG with corrected file type specification # Test 1: Search for files that might be using the actionList echo "Files potentially using actionList:" rg -g '*.ts' "actionList" ./ui/main/src # Test 2: Search for any existing uses of NOTIFICATION_CONFIG echo "\nExisting uses of NOTIFICATION_CONFIG:" rg -g '*.ts' "NOTIFICATION_CONFIG" ./ui/main/src # Test 3: Check translation files for action names echo "\nTranslation files that might need updating:" fd -e json -e properties "translation" ./ui/main/srcLength of output: 859
src/test/cypress/cypress/integration/UserActionLogs.spec.js (2)
45-57: LGTM: Test case updated to include new 'NOTIFICATION_CONFIG' actionThe changes in this test case accurately reflect the addition of the new 'NOTIFICATION_CONFIG' action, which aligns with the PR objective of implementing a user action log for notification configuration changes. The increase in the expected number of results and the adjustment of action indices are consistent with this new feature.
To ensure full coverage, please confirm that the 'NOTIFICATION_CONFIG' action is being properly logged in the backend implementation.
Line range hint
1-248: Overall changes align well with PR objectivesThe modifications to this test file comprehensively cover the new 'NOTIFICATION_CONFIG' action, aligning perfectly with the PR objective of implementing a user action log for notification configuration changes. The changes include:
- Updates to expected result counts in various test cases.
- Addition of checks for the new action in log content, pagination, and export tests.
- Adjustment of existing checks to accommodate the new action.
- Implementation of a new helper function to trigger notification configuration changes.
These changes ensure thorough testing of the new feature across different aspects of the user action logs functionality.
To ensure complete coverage, please verify that all possible notification configuration changes are being logged, not just the one implemented in the 'changeNotificationConfiguration' function.
services/users/src/test/java/org/opfab/users/services/UserSettingsServiceShould.java (3)
135-135: LGTM: Improved test method name.The test method name has been updated to more accurately reflect the expected behavior when fetching settings for a non-existing user. This change improves the clarity of the test's purpose.
Line range hint
1-589: Overall improvements to UserSettingsServiceShould test class.The changes made to this test class enhance its quality and coverage:
- The UserSettingsService constructor now includes additional parameters, which may need verification in the main code.
- Test method names and user identifiers have been updated for better clarity and consistency.
- A new test case for patching settings of a non-existing user has been added, improving edge case coverage.
These modifications contribute to a more robust and maintainable test suite.
72-72: Verify the purpose of new constructor parameters.The UserSettingsService constructor now includes two additional parameters:
nullandfalse. Could you please clarify the purpose of these new parameters? It's important to ensure that they are correctly utilized in the service implementation.services/users/src/main/java/org/opfab/users/controllers/NotificationConfigurationController.java (1)
37-39: Verify dependency injection configuration for the updated constructorThe constructor for
NotificationConfigurationControllernow includes additional parameters:UserActionLogRepository userActionLogRepositoryand@Value("${operatorfabric.userActionLogActivated:true}") boolean userActionLogActivated. Ensure that the dependency injection framework (e.g., Spring) is properly configured to inject these new dependencies, and that all beans are correctly wired to reflect these changes.services/users/src/main/java/org/opfab/users/controllers/UsersController.java (3)
17-18: Imports added are appropriateThe imports for
UserActionLogRepositoryandUserActionLogServiceare necessary for the new functionality and are correctly added.
31-31: Importing@Valueannotation is appropriateThe import
org.springframework.beans.factory.annotation.Valueis required for injecting theuserActionLogActivatedproperty from the application configuration.
61-61: Addition ofuserActionLogServicefieldThe new field
private final UserActionLogService userActionLogService;correctly introduces the service needed for user action logging.services/users/src/main/java/org/opfab/users/services/UserSettingsService.java (4)
18-19: Approved: Added necessary imports for user action loggingThe new imports for
UserActionEnumandUserActionLogServiceare appropriate for integrating user action logging functionality.
44-46: Approved: Introduced fields for user action loggingThe addition of
userActionLogServiceanduserActionLogActivatedfields to theUserSettingsServiceclass aligns with the requirement to log user actions based on activation.
75-79: Approved: Enhanced user existence check inpatchUserSettingsThe added check for user existence improves error handling by returning a not-found operation result if the user does not exist.
49-49: Verify constructor updates across the codebaseThe constructor now includes
UserActionLogServiceandboolean userActionLogActivatedas parameters. Ensure that all instantiations ofUserSettingsServiceare updated to reflect this change to prevent potential instantiation errors.You can verify constructor usages with the following script:
✅ Verification successful
All constructor usages of
UserSettingsServiceare correctly updated with the new parameters. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instantiations of UserSettingsService and check for updated constructor parameters. # Test: Search for the constructor usage. Expect: All instances should include the new parameters. rg --type java 'new UserSettingsService\(' -A 3Length of output: 3818
services/users/src/main/java/org/opfab/users/controllers/CurrentUserWithPerimetersController.java (4)
12-13: Addition of UserActionLog ImportsThe imports for
UserActionLogRepositoryandUserActionLogServiceare correctly added to enable user action logging functionality.
26-26: Importing@Valuefor Configuration PropertyThe import of
org.springframework.beans.factory.annotation.Valueis necessary for injecting theuserActionLogActivatedconfiguration property.
42-45: Constructor Parameter ExpansionThe constructor now includes
UserActionLogRepositoryand the injecteduserActionLogActivatedflag. This setup appropriately incorporates user action logging dependencies.
50-52: Initialization ofUserSettingsServicewith User Action LoggingThe
UserSettingsServiceis now instantiated withuserActionLogServiceanduserActionLogActivated. This correctly integrates user action logging into user settings management as per the PR objectives.
| CLOSE_SUBSCRIPTION = 'CLOSE_SUBSCRIPTION', | ||
| NOTIFICATION_CONFIG = 'NOTIFICATION_CONFIG' |
There was a problem hiding this comment.
💡 Codebase verification
NOTIFICATION_CONFIG not utilized in UI code.
While NOTIFICATION_CONFIG has been correctly added to ActionTypeEnum and is being used in the backend services, its usage is absent in the UI codebase. To ensure full integration and consistent functionality, please update the UI components and services to handle the NOTIFICATION_CONFIG action type.
Areas to address:
- UI Components: Ensure that any components responsible for triggering or displaying user actions include handling for
NOTIFICATION_CONFIG. - Services/State Management: Update services or state management logic to process the new action type appropriately.
- Testing: Add or update unit and integration tests to cover scenarios involving
NOTIFICATION_CONFIG.
🔗 Analysis chain
LGTM! Verify usage across the codebase.
The addition of NOTIFICATION_CONFIG to ActionTypeEnum is correct and aligns with the PR objectives. It follows the existing naming convention and structure.
To ensure complete implementation, please verify:
- Any switch statements or if-else chains based on
ActionTypeEnumare updated to handle this new value. - Documentation and UI components listing action types are updated to include
NOTIFICATION_CONFIG. - The backend is prepared to handle this new action type.
Run the following script to check for potential areas that might need updating:
Please review the results and update the relevant parts of the codebase as necessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find potential areas that might need updating due to the new enum value
# Test 1: Search for switch statements or if-else chains using ActionTypeEnum
echo "Searching for switch statements or if-else chains using ActionTypeEnum:"
rg --type typescript "switch.*ActionTypeEnum|if.*ActionTypeEnum.*else" ./ui
# Test 2: Search for places where all ActionTypeEnum values might be listed
echo "Searching for places where all ActionTypeEnum values might be listed:"
rg --type typescript "ActionTypeEnum\.\w+" ./ui
# Test 3: Search for backend code that might need to handle the new action type
echo "Searching for backend code that might need to handle the new action type:"
rg --type java "ActionTypeEnum|NOTIFICATION_CONFIG" ./services
Length of output: 1752
Script:
#!/bin/bash
# Description: Find potential areas that might need updating due to the new enum value
# Test 1: Search for switch statements or if-else chains using ActionTypeEnum without specifying file type
echo "Searching for switch statements or if-else chains using ActionTypeEnum in all files:"
rg "switch.*ActionTypeEnum|if.*ActionTypeEnum.*else" ./ui
# Test 2: Search for places where all ActionTypeEnum values might be listed by targeting .ts and .tsx files
echo "Searching for places where all ActionTypeEnum values might be listed in TypeScript files:"
rg "ActionTypeEnum\.\w+" -g "*.ts" -g "*.tsx" ./ui
# Test 3: Confirm backend handling remains consistent
echo "Verifying backend handling of ActionTypeEnum.NOTIFICATION_CONFIG remains consistent:"
rg "ActionTypeEnum\.NOTIFICATION_CONFIG" ./services
Length of output: 1087
| NotificationService notificationService = new NotificationService(userRepository, eventBus); | ||
| UsersService usersService = new UsersService(userRepository, groupRepository, entityRepository, | ||
| perimeterRepository, notificationService); | ||
| UserActionLogService userActionLogService = new UserActionLogService(userActionLogRepository); | ||
| UserSettingsService userSettingsService = new UserSettingsService(userSettingsRepository, usersService, | ||
| notificationService, userActionLogService, userActionLogActivated); | ||
| this.currentUserWithPerimetersService = new CurrentUserWithPerimetersService(usersService, | ||
| userSettingsService, | ||
| entityRepository); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider injecting services rather than instantiating them manually
Manually instantiating service objects within the controller can lead to tighter coupling and makes unit testing more difficult. It's recommended to use Spring's dependency injection to inject these services via the constructor or field injection. This approach promotes better maintainability, testability, and adheres to Spring's best practices.
| this.notificationService = new NotificationService(userRepository, eventBus); | ||
| this.userSettingsService = new UserSettingsService(userSettingsRepository, null, notificationService); | ||
| UserActionLogService userActionLogService = new UserActionLogService(userActionLogRepository); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider injecting UserActionLogService instead of manual instantiation
Manually creating an instance of UserActionLogService within the controller can make testing and maintenance more challenging. It is advisable to leverage dependency injection to inject UserActionLogService, promoting better testability and adherence to the inversion of control principle.
Apply this diff to inject UserActionLogService via the constructor:
public class NotificationConfigurationController {
private final UserSettingsService userSettingsService;
private final NotificationService notificationService;
+ private final UserActionLogService userActionLogService;
public NotificationConfigurationController(UserRepository userRepository,
UserSettingsRepository userSettingsRepository,
+ UserActionLogService userActionLogService,
EventBus eventBus,
@Value("${operatorfabric.userActionLogActivated:true}") boolean userActionLogActivated) {
this.notificationService = new NotificationService(userRepository, eventBus);
- UserActionLogService userActionLogService = new UserActionLogService(userActionLogRepository);
this.userActionLogService = userActionLogService;
this.userSettingsService = new UserSettingsService(userSettingsRepository, null, notificationService,
userActionLogService, userActionLogActivated);
}
// Rest of the class...
}This refactoring enhances the code by utilizing the dependency injection framework to manage UserActionLogService, leading to cleaner code and easier testing.
Committable suggestion was skipped due to low confidence.
| UserActionLogRepository userActionLogRepository, EventBus eventBus, JwtProperties jwtProperties, | ||
| GroupsProperties groupsProperties, @Value("${operatorfabric.userActionLogActivated:true}") boolean userActionLogActivated) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constructor parameter list is becoming lengthy
The constructor now has a large number of parameters, which can make the code harder to read and maintain. Consider refactoring to reduce the number of constructor parameters, possibly by grouping related parameters into configuration objects or using setter injection where appropriate.
| this.userActionLogService = new UserActionLogService(userActionLogRepository); | ||
| this.userSettingsService = new UserSettingsService(userSettingsRepository, usersService, notificationService, | ||
| this.userActionLogService, userActionLogActivated); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to use dependency injection instead of manual instantiation
The services UserActionLogService and UserSettingsService are being manually instantiated using new, which bypasses Spring's dependency injection mechanisms. This approach can lead to tight coupling and makes testing more difficult. It's recommended to inject these services via constructor injection so that they are managed by the Spring container.
To refactor, modify the constructor to accept UserActionLogService and adjust the initialization of UserSettingsService accordingly.
Proposed Changes:
-
Modify the constructor to inject
UserActionLogServiceand removeUserActionLogRepositoryanduserActionLogActivatedparameters:public UsersController(UserRepository userRepository, UserSettingsRepository userSettingsRepository, GroupRepository groupRepository, EntityRepository entityRepository, PerimeterRepository perimeterRepository, - UserActionLogRepository userActionLogRepository, EventBus eventBus, JwtProperties jwtProperties, - GroupsProperties groupsProperties, @Value("${operatorfabric.userActionLogActivated:true}") boolean userActionLogActivated) { + UserActionLogService userActionLogService, EventBus eventBus, JwtProperties jwtProperties, + GroupsProperties groupsProperties) { -
Assign the injected
UserActionLogServiceto the class field:- this.userActionLogService = new UserActionLogService(userActionLogRepository); + this.userActionLogService = userActionLogService;
-
Modify the initialization of
UserSettingsServiceto remove manual instantiation and injectuserActionLogActivatedif necessary via configuration:this.usersService = new UsersService(userRepository, groupRepository, entityRepository, perimeterRepository, notificationService); - this.userSettingsService = new UserSettingsService(userSettingsRepository, usersService, notificationService, - this.userActionLogService, userActionLogActivated); + this.userSettingsService = new UserSettingsService(userSettingsRepository, usersService, notificationService, + userActionLogService); -
Ensure
UserActionLogServiceandUserSettingsServiceare defined as Spring beans and configured for injection. IfuserActionLogActivatedis needed, consider injecting it into theUserSettingsServicebean directly.
By allowing Spring to manage the instantiation and injection of these services, you improve modularity and testability of your code.
Committable suggestion was skipped due to low confidence.
| if (userActionLogActivated && userSettingsPatch.getProcessesStatesNotNotified() != null) { | ||
| userActionLogService.insertUserActionLog(login, UserActionEnum.NOTIFICATION_CONFIG, user.getEntities(), null, | ||
| getProcessesStatesNotNotifiedText(newSettings.getProcessesStatesNotNotified())); | ||
| } |
There was a problem hiding this comment.
Prevent potential NullPointerException in user action logging
newSettings.getProcessesStatesNotNotified() may return null, which could lead to a NullPointerException when passed to getProcessesStatesNotNotifiedText(). It's advisable to add a null check or provide a default value to ensure robustness.
Apply this diff to handle potential null values:
if (userActionLogActivated && userSettingsPatch.getProcessesStatesNotNotified() != null) {
- userActionLogService.insertUserActionLog(login, UserActionEnum.NOTIFICATION_CONFIG, user.getEntities(), null,
- getProcessesStatesNotNotifiedText(newSettings.getProcessesStatesNotNotified()));
+ Map<String, List<String>> processesStates = Optional.ofNullable(newSettings.getProcessesStatesNotNotified())
+ .orElse(Collections.emptyMap());
+ userActionLogService.insertUserActionLog(login, UserActionEnum.NOTIFICATION_CONFIG, user.getEntities(), null,
+ getProcessesStatesNotNotifiedText(processesStates));
}Alternatively, modify getProcessesStatesNotNotifiedText to handle null inputs gracefully.
📝 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 (userActionLogActivated && userSettingsPatch.getProcessesStatesNotNotified() != null) { | |
| userActionLogService.insertUserActionLog(login, UserActionEnum.NOTIFICATION_CONFIG, user.getEntities(), null, | |
| getProcessesStatesNotNotifiedText(newSettings.getProcessesStatesNotNotified())); | |
| } | |
| if (userActionLogActivated && userSettingsPatch.getProcessesStatesNotNotified() != null) { | |
| Map<String, List<String>> processesStates = Optional.ofNullable(newSettings.getProcessesStatesNotNotified()) | |
| .orElse(Collections.emptyMap()); | |
| userActionLogService.insertUserActionLog(login, UserActionEnum.NOTIFICATION_CONFIG, user.getEntities(), null, | |
| getProcessesStatesNotNotifiedText(processesStates)); | |
| } |
quinarygio
force-pushed
the
FE-7339-add-user-action-log-notification-configuration-change
branch
from
1b6f9db
to
8f463fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
services/users/src/main/java/org/opfab/users/controllers/UserWithPerimetersController.java (2)
42-45: Consider refactoring for better maintainability.The updated constructor signature correctly includes the new
UserActionLogRepositoryanduserActionLogActivatedflag. The use of@Valuefor injecting the configuration is a good practice. However, the growing number of parameters suggests that this class might be taking on too many responsibilities.Consider refactoring this class to use the builder pattern or splitting it into smaller, more focused classes to improve maintainability and adhere to the Single Responsibility Principle.
Line range hint
1-67: Summary: Functionality added, but refactoring needed.The changes successfully introduce user action logging functionality. However, the class would greatly benefit from the following refactoring:
- Implement dependency injection for all services.
- Consider using the builder pattern or splitting the class into smaller, more focused classes to reduce complexity.
- Ensure all dependencies are managed by Spring, removing manual instantiations.
These changes will improve testability, maintainability, and adherence to SOLID principles. Consider creating a separate task to address these architectural concerns in a future iteration.
services/users/src/main/java/org/opfab/users/controllers/CurrentUserWithPerimetersController.java (1)
Line range hint
1-70: Summary of review for CurrentUserWithPerimetersController.javaThe changes successfully implement the user action logging functionality as per the PR objectives. However, there are opportunities for improvement in the overall design and adherence to Spring best practices:
- The constructor has become more complex with the addition of new parameters.
- Services are still being manually instantiated within the constructor.
Main recommendations:
- Implement constructor injection for all dependencies, including services.
- Declare all services as Spring components (e.g.,
@Service).- Move the
@Valueannotation foruserActionLogActivatedto a class field.These changes will significantly improve the maintainability, testability, and overall design of the controller. Please consider implementing these suggestions to align the code more closely with Spring best practices and to prepare for potential future enhancements.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- services/users/src/main/java/org/opfab/users/controllers/CurrentUserWithPerimetersController.java (3 hunks)
- services/users/src/main/java/org/opfab/users/controllers/NotificationConfigurationController.java (2 hunks)
- services/users/src/main/java/org/opfab/users/controllers/UserWithPerimetersController.java (3 hunks)
- services/users/src/main/java/org/opfab/users/controllers/UsersController.java (3 hunks)
- services/users/src/main/java/org/opfab/users/services/UserSettingsService.java (4 hunks)
- services/users/src/test/java/org/opfab/users/services/CurrentUserWithPerimetersServiceShould.java (1 hunks)
- services/users/src/test/java/org/opfab/users/services/UserSettingsServiceShould.java (3 hunks)
- src/docs/asciidoc/reference_doc/users_management.adoc (1 hunks)
- src/test/cypress/cypress/integration/UserActionLogs.spec.js (6 hunks)
- tools/user-action-tracing/src/main/java/org/opfab/useractiontracing/model/UserActionEnum.java (2 hunks)
- ui/main/src/app/business/model/user-action-log.model.ts (1 hunks)
- ui/main/src/app/business/view/useractionlogs/userActionLogsPageDescription.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- services/users/src/main/java/org/opfab/users/controllers/UsersController.java
- services/users/src/test/java/org/opfab/users/services/CurrentUserWithPerimetersServiceShould.java
- services/users/src/test/java/org/opfab/users/services/UserSettingsServiceShould.java
- src/docs/asciidoc/reference_doc/users_management.adoc
- tools/user-action-tracing/src/main/java/org/opfab/useractiontracing/model/UserActionEnum.java
- ui/main/src/app/business/model/user-action-log.model.ts
- ui/main/src/app/business/view/useractionlogs/userActionLogsPageDescription.ts
🧰 Additional context used
🔇 Additional comments (22)
services/users/src/main/java/org/opfab/users/controllers/UserWithPerimetersController.java (3)
12-13: LGTM: New imports for user action logging.The added imports for
UserActionLogRepositoryandUserActionLogServiceare necessary for the new user action logging functionality and are correctly placed.
25-25: LGTM: Import for @value annotation.The added import for the
@Valueannotation from Spring Framework is correct and necessary for injecting configuration values.
47-55:⚠️ Potential issueRefactor to use dependency injection.
While the new
UserActionLogServiceis correctly integrated, the manual instantiation of services within the constructor persists. This approach violates the Dependency Inversion Principle, making the code harder to test and maintain.To address this:
- Use constructor injection for all services.
- Remove manual instantiations from the constructor.
- Let Spring manage the lifecycle of these services.
Example refactor:
@Autowired public UserWithPerimetersController( NotificationService notificationService, UsersService usersService, UserSettingsService userSettingsService, CurrentUserWithPerimetersService currentUserWithPerimetersService) { this.currentUserWithPerimetersService = currentUserWithPerimetersService; }This change will significantly improve testability and maintainability of the code.
services/users/src/main/java/org/opfab/users/controllers/CurrentUserWithPerimetersController.java (2)
12-13: LGTM: New imports for user action logging.The added imports for
UserActionLogRepositoryandUserActionLogServiceare consistent with the PR objectives and support the implementation of user action logging.
26-26: LGTM: Import for @value annotation.The addition of the @value annotation import supports the injection of the
userActionLogActivatedconfiguration, promoting better configuration management.services/users/src/main/java/org/opfab/users/controllers/NotificationConfigurationController.java (5)
12-13: LGTM: New imports are appropriate for the changes.The added imports for
UserActionLogRepository,UserActionLogService, and@Valueannotation are consistent with the new functionality being introduced in the controller.Also applies to: 19-19
37-39: LGTM: Constructor signature updated appropriately.The constructor has been updated to include
UserActionLogRepositoryand a configurableuserActionLogActivatedflag. The use of@Valuewith a default value is a good practice for making the feature configurable.
Line range hint
1-68: Summary: User action logging implementation looks good, with room for improvement.The changes successfully introduce user action logging for notification configuration changes, aligning with the PR objectives. However, there are opportunities for improvement:
- Consider injecting
UserActionLogServiceinstead of creating it in the constructor.- Verify and potentially refactor the
UserSettingsServiceconstructor to avoid passingnullparameters.These improvements would enhance testability, maintainability, and robustness of the code.
41-41: 🛠️ Refactor suggestionConsider injecting
UserActionLogServiceinstead of manual instantiation.Creating
UserActionLogServicewithin the constructor can make testing more challenging and violates the Dependency Inversion Principle. Consider injectingUserActionLogServiceas a constructor parameter instead.Apply this diff to inject
UserActionLogService:public NotificationConfigurationController(UserRepository userRepository, UserSettingsRepository userSettingsRepository, UserActionLogRepository userActionLogRepository, - EventBus eventBus, @Value("${operatorfabric.userActionLogActivated:true}") boolean userActionLogActivated) { + EventBus eventBus, UserActionLogService userActionLogService, + @Value("${operatorfabric.userActionLogActivated:true}") boolean userActionLogActivated) { this.notificationService = new NotificationService(userRepository, eventBus); - UserActionLogService userActionLogService = new UserActionLogService(userActionLogRepository); this.userSettingsService = new UserSettingsService(userSettingsRepository, null, notificationService, userActionLogService, userActionLogActivated); }Likely invalid or redundant comment.
42-43:⚠️ Potential issueVerify
UserSettingsServiceconstructor and avoid passingnull.The
UserSettingsServiceis instantiated with new parameters to support user action logging, which is good. However, passingnullas the second parameter is a potential issue.
- Verify that
UserSettingsServicecan handle thenullparameter safely.- Consider refactoring
UserSettingsServiceto avoid the need for anullparameter.Run this script to inspect the
UserSettingsServiceconstructor:src/test/cypress/cypress/integration/UserActionLogs.spec.js (9)
45-45: Expected result count updated to 13The expected number of results is correctly incremented to 13 to account for the new 'NOTIFICATION_CONFIG' action in the user action logs.
49-56: Actions and indices correctly updated in the user action logs testThe test assertions have been appropriately updated to include 'NOTIFICATION_CONFIG' at index 2, with subsequent actions shifted accordingly. This ensures that the test reflects the correct order of actions after the new log entry is added.
60-63: Pagination checks adjusted for additional log entryThe pagination test now expects 3 lines instead of 2, which correctly accounts for the additional 'NOTIFICATION_CONFIG' log entry. The action checks have been updated to validate the correct actions and users on each page.
83-83: Expected export result count updated to 15The expected number of exported results is incremented to 15 to include the new 'NOTIFICATION_CONFIG' action. This change ensures that the export functionality is tested with all current log entries.
97-97: Update expected row count in export data testThe assertion now expects 15 rows in the exported Excel sheet, accurately reflecting the total number of user action logs, including the new 'NOTIFICATION_CONFIG' entry.
103-109: Exported action log entries correctly updatedThe test checks for 'NOTIFICATION_CONFIG' at the correct index in the exported data, with subsequent indices adjusted accordingly. This ensures the exported log entries match the expected actions and users.
122-122: Adjusted expected results in card details accessibility testThe expected number of results is updated to 17, accounting for the new action log entry. This change ensures that the test accurately reflects the total number of user actions after the addition.
125-128: Line numbers updated for accessing card detailsThe line numbers used to access card details are adjusted to match the new ordering after adding the 'NOTIFICATION_CONFIG' action. This ensures the test continues to validate the accessibility of card details correctly.
159-159: InvokechangeNotificationConfiguration()to generate new action logAdding the call to
changeNotificationConfiguration()indoSomeActionToBeTracedInUserActionLogs()ensures that the 'NOTIFICATION_CONFIG' action is performed and logged. This addition is necessary to test the logging of notification configuration changes.services/users/src/main/java/org/opfab/users/services/UserSettingsService.java (3)
44-47: Variable Declarations: Ensure Consistency with Coding StandardsThe addition of
userActionLogServiceanduserActionLogActivatedis appropriate. Please ensure that the ordering and spacing of these variable declarations align with the project's coding conventions for readability and maintainability.
75-79: User Existence Check: Good Practice ImplementedAdding a check for the user's existence before patching settings enhances error handling and prevents operations on non-existent users. This is a commendable improvement for data integrity.
99-102:⚠️ Potential issuePotential NullPointerException Remains Unaddressed
The previous review comment regarding a potential
NullPointerExceptioningetProcessesStatesNotNotifiedText(newSettings.getProcessesStatesNotNotified())is still applicable. The methodgetProcessesStatesNotNotifiedTextmay receive anullargument ifnewSettings.getProcessesStatesNotNotified()returnsnull.
| PerimeterRepository perimeterRepository, EntityRepository entityRepository, | ||
| UserSettingsRepository userSettingsRepository, UserActionLogRepository userActionLogRepository, | ||
| EventBus eventBus, | ||
| @Value("${operatorfabric.userActionLogActivated:true}") boolean userActionLogActivated) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing dependency injection for better maintainability.
While the new parameters UserActionLogRepository and userActionLogActivated are necessary for the logging functionality, the constructor is becoming increasingly complex. To improve maintainability and adhere to Spring best practices, consider:
- Using constructor injection for all dependencies.
- Utilizing
@Autowiredon the constructor (optional in newer Spring versions). - Moving the
@Valueannotation to a class field for theuserActionLogActivatedflag.
Example refactoring:
@Autowired
public CurrentUserWithPerimetersController(
UserRepository userRepository,
GroupRepository groupRepository,
PerimeterRepository perimeterRepository,
EntityRepository entityRepository,
UserSettingsRepository userSettingsRepository,
UserActionLogRepository userActionLogRepository,
EventBus eventBus,
NotificationService notificationService,
UsersService usersService,
UserActionLogService userActionLogService,
UserSettingsService userSettingsService,
CurrentUserWithPerimetersService currentUserWithPerimetersService) {
this.currentUserWithPerimetersService = currentUserWithPerimetersService;
// Other necessary initializations
}
@Value("${operatorfabric.userActionLogActivated:true}")
private boolean userActionLogActivated;This approach would simplify the constructor and improve testability.
| NotificationService notificationService = new NotificationService(userRepository, eventBus); | ||
| UsersService usersService = new UsersService(userRepository, groupRepository, entityRepository, | ||
| perimeterRepository, notificationService); | ||
| UserActionLogService userActionLogService = new UserActionLogService(userActionLogRepository); | ||
| UserSettingsService userSettingsService = new UserSettingsService(userSettingsRepository, usersService, | ||
| notificationService, userActionLogService, userActionLogActivated); | ||
| this.currentUserWithPerimetersService = new CurrentUserWithPerimetersService(usersService, | ||
| userSettingsService, | ||
| entityRepository); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strongly recommend using Spring's dependency injection for service instantiation.
The current approach of manually instantiating services within the constructor has several drawbacks:
- It makes the code harder to test due to tight coupling.
- It goes against Spring's dependency injection principle.
- It reduces the clarity of dependencies.
To address these issues and improve the overall design, consider the following refactoring:
- Declare all service classes as Spring components (e.g.,
@Service). - Use constructor injection to inject these services into the controller.
Example refactoring:
@Service
public class NotificationService { /* ... */ }
@Service
public class UsersService { /* ... */ }
@Service
public class UserActionLogService { /* ... */ }
@Service
public class UserSettingsService { /* ... */ }
@Service
public class CurrentUserWithPerimetersService { /* ... */ }
@RestController
@RequestMapping({ "/CurrentUserWithPerimeters", "/internal/CurrentUserWithPerimeters" })
public class CurrentUserWithPerimetersController implements UserExtractor {
private final CurrentUserWithPerimetersService currentUserWithPerimetersService;
@Autowired
public CurrentUserWithPerimetersController(CurrentUserWithPerimetersService currentUserWithPerimetersService) {
this.currentUserWithPerimetersService = currentUserWithPerimetersService;
}
// ... rest of the controller code
}This approach will:
- Simplify the controller code
- Improve testability by allowing easy mocking of dependencies
- Adhere to Spring best practices
- Enhance overall maintainability of the codebase
| private String getProcessesStatesNotNotifiedText(Map<String, List<String>> processesStatesNotNotified) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| processesStatesNotNotified.forEach((process, states) -> | ||
| sb.append(process).append(": [").append(String.join(",", states)).append("]\n"); | ||
| ); | ||
| return sb.toString(); | ||
| } |
There was a problem hiding this comment.
Handle Null Input in getProcessesStatesNotNotifiedText Method to Prevent Exceptions
The method getProcessesStatesNotNotifiedText does not account for null inputs, leading to a potential NullPointerException when processesStatesNotNotified is null. To enhance robustness, include a null check or provide a default value.
Apply this diff to handle potential null values:
private String getProcessesStatesNotNotifiedText(Map<String, List<String>> processesStatesNotNotified) {
+ if (processesStatesNotNotified == null || processesStatesNotNotified.isEmpty()) {
+ return "";
+ }
StringBuilder sb = new StringBuilder();
processesStatesNotNotified.forEach((process, states) ->
sb.append(process).append(": [").append(String.join(",", states)).append("]\n");
);
return sb.toString();
}📝 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.
| private String getProcessesStatesNotNotifiedText(Map<String, List<String>> processesStatesNotNotified) { | |
| StringBuilder sb = new StringBuilder(); | |
| processesStatesNotNotified.forEach((process, states) -> | |
| sb.append(process).append(": [").append(String.join(",", states)).append("]\n"); | |
| ); | |
| return sb.toString(); | |
| } | |
| private String getProcessesStatesNotNotifiedText(Map<String, List<String>> processesStatesNotNotified) { | |
| if (processesStatesNotNotified == null || processesStatesNotNotified.isEmpty()) { | |
| return ""; | |
| } | |
| StringBuilder sb = new StringBuilder(); | |
| processesStatesNotNotified.forEach((process, states) -> | |
| sb.append(process).append(": [").append(String.join(",", states)).append("]\n"); | |
| ); | |
| return sb.toString(); | |
| } |
| NotificationService notificationService, UserActionLogService userActionLogService, boolean userActionLogActivated) { | ||
| this.userSettingsRepository = userSettingsRepository; | ||
| this.userService = usersService; | ||
| this.notificationService = notificationService; | ||
| this.userActionLogService = userActionLogService; | ||
| this.userActionLogActivated = userActionLogActivated; |
There was a problem hiding this comment.
💡 Codebase verification
Dependency Injection Inconsistencies Detected
Several instantiations of UserSettingsService are missing the new constructor parameters UserActionLogService and boolean userActionLogActivated. This could lead to potential NullPointerException issues.
Affected Locations:
services/users/src/main/java/org/opfab/users/controllers/UserWithPerimetersController.javaservices/users/src/main/java/org/opfab/users/controllers/NotificationConfigurationController.javaservices/users/src/main/java/org/opfab/users/controllers/UsersController.javaservices/users/src/test/java/org/opfab/users/services/UserSettingsServiceShould.javaservices/users/src/main/java/org/opfab/users/controllers/CurrentUserWithPerimetersController.java
Action Required:
Update all affected constructor calls to include the new parameters to ensure proper dependency injection and prevent runtime exceptions.
🔗 Analysis chain
Constructor Update: Verify Dependency Injection Consistency
The constructor now includes UserActionLogService and boolean userActionLogActivated, which is suitable for enabling user action logging based on feature activation. Ensure that all instantiations of UserSettingsService are updated to pass these new parameters to prevent any NullPointerException due to uninitialized dependencies.
To confirm that all constructor calls have been updated, run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all instantiations of UserSettingsService include the new constructor parameters.
# Search for 'new UserSettingsService(' and display the lines for review.
rg --type java 'new UserSettingsService\('
Length of output: 1288
) Signed-off-by: Giovanni Ferrari <[email protected]>
quinarygio
force-pushed
the
FE-7339-add-user-action-log-notification-configuration-change
branch
from
8f463fd
to
3e398bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
services/users/src/main/java/org/opfab/users/services/UserSettingsService.java (1)
44-47: Consider declaring fieldsfinalfor immutabilityIf
userActionLogServiceanduserActionLogActivatedare not modified after initialization, declaring them asfinalcan enhance code readability and thread safety.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- services/users/src/main/java/org/opfab/users/controllers/CurrentUserWithPerimetersController.java (3 hunks)
- services/users/src/main/java/org/opfab/users/controllers/NotificationConfigurationController.java (2 hunks)
- services/users/src/main/java/org/opfab/users/controllers/UserWithPerimetersController.java (3 hunks)
- services/users/src/main/java/org/opfab/users/controllers/UsersController.java (3 hunks)
- services/users/src/main/java/org/opfab/users/services/UserSettingsService.java (4 hunks)
- services/users/src/test/java/org/opfab/users/services/CurrentUserWithPerimetersServiceShould.java (1 hunks)
- services/users/src/test/java/org/opfab/users/services/UserSettingsServiceShould.java (3 hunks)
- src/docs/asciidoc/reference_doc/users_management.adoc (1 hunks)
- src/test/cypress/cypress/integration/UserActionLogs.spec.js (6 hunks)
- tools/user-action-tracing/src/main/java/org/opfab/useractiontracing/model/UserActionEnum.java (2 hunks)
- ui/main/src/app/business/model/user-action-log.model.ts (1 hunks)
- ui/main/src/app/business/view/useractionlogs/userActionLogsPageDescription.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- services/users/src/main/java/org/opfab/users/controllers/CurrentUserWithPerimetersController.java
- services/users/src/main/java/org/opfab/users/controllers/UserWithPerimetersController.java
- services/users/src/test/java/org/opfab/users/services/CurrentUserWithPerimetersServiceShould.java
- services/users/src/test/java/org/opfab/users/services/UserSettingsServiceShould.java
- src/docs/asciidoc/reference_doc/users_management.adoc
- tools/user-action-tracing/src/main/java/org/opfab/useractiontracing/model/UserActionEnum.java
- ui/main/src/app/business/model/user-action-log.model.ts
- ui/main/src/app/business/view/useractionlogs/userActionLogsPageDescription.ts
🧰 Additional context used
🔇 Additional comments (18)
services/users/src/main/java/org/opfab/users/controllers/NotificationConfigurationController.java (3)
12-13: LGTM: New imports for user action logging.The added imports for
UserActionLogRepository,UserActionLogService, and@Valueare appropriate for the new user action logging functionality being introduced.Also applies to: 19-19
37-39: LGTM: Updated constructor signature for user action logging.The constructor signature has been appropriately updated to include
UserActionLogRepositoryanduserActionLogActivatedflag. The use of@Valueannotation with a default value is a good practice for configuration injection.
41-41: 🛠️ Refactor suggestionConsider using dependency injection for
UserActionLogService.While the
UserActionLogServiceis correctly instantiated with the injected repository, it's generally better to use dependency injection for services as well. This improves testability and follows the Inversion of Control principle.Consider modifying the constructor to accept
UserActionLogServicedirectly:public NotificationConfigurationController(UserRepository userRepository, UserSettingsRepository userSettingsRepository, UserActionLogRepository userActionLogRepository, + UserActionLogService userActionLogService, EventBus eventBus, @Value("${operatorfabric.userActionLogActivated:true}") boolean userActionLogActivated) { this.notificationService = new NotificationService(userRepository, eventBus); - UserActionLogService userActionLogService = new UserActionLogService(userActionLogRepository); this.userSettingsService = new UserSettingsService(userSettingsRepository, null, notificationService, userActionLogService, userActionLogActivated); }This change would require updating the dependency injection configuration accordingly.
Likely invalid or redundant comment.
src/test/cypress/cypress/integration/UserActionLogs.spec.js (6)
45-57: LGTM: Test updated to include new 'NOTIFICATION_CONFIG' actionThe test case has been correctly updated to reflect the addition of the 'NOTIFICATION_CONFIG' action. The expected number of results and the order of actions in the log are now accurate, aligning with the PR objectives of implementing a user action log for notification configuration changes.
60-63: LGTM: Pagination test updated to reflect new action orderThe pagination test has been correctly adjusted to account for the addition of the 'NOTIFICATION_CONFIG' action. The number of lines and the order of actions in the paginated view are now accurate, maintaining consistency with the changes made in the previous test case.
Line range hint
83-113: LGTM: Export test updated to include new 'NOTIFICATION_CONFIG' actionThe export test case has been correctly updated to reflect the addition of the 'NOTIFICATION_CONFIG' action. The expected number of results and the order of actions in the exported file are now accurate, aligning with the PR objectives and maintaining consistency with the previous test case updates.
122-128: LGTM: Card details accessibility test updatedThe card details accessibility test has been correctly adjusted to account for the addition of the 'NOTIFICATION_CONFIG' action. The expected number of results and the line numbers for checking card details are now accurate, maintaining consistency with the changes made in the previous test cases.
159-159: LGTM: Added notification configuration change to test setupThe addition of the
changeNotificationConfiguration()function call in the test setup aligns perfectly with the PR objectives. This ensures that the new 'NOTIFICATION_CONFIG' action is triggered and logged, allowing for comprehensive testing of the new feature.
190-190: LGTM: Improved code formattingThe addition of an empty line after the
changeNotificationConfiguration()function improves code readability by clearly separating it from the subsequent functions.services/users/src/main/java/org/opfab/users/controllers/UsersController.java (5)
17-18: Added necessary imports for User Action LoggingThe imports for
UserActionLogRepositoryandUserActionLogServiceare appropriate and necessary for the new user action logging functionality.
31-31: Import of@ValueAnnotationThe import of
org.springframework.beans.factory.annotation.Valueis required for property injection ofuserActionLogActivatedand is correctly added.
61-61: Proper Declaration ofuserActionLogServiceFieldThe
userActionLogServiceis correctly declared as aprivate finalfield, adhering to best practices for immutability and encapsulation.
66-67: Constructor Parameter List Remains LengthyAs previously noted, the constructor has a large number of parameters, which can impact readability and maintainability. Consider refactoring to reduce the parameter count, possibly by using configuration objects or dependency injection frameworks.
73-75: Continued Use of Manual Service InstantiationThe earlier suggestion to use dependency injection for
UserActionLogServiceinstead of manual instantiation withnewis still applicable. This change would enhance modularity and testability.services/users/src/main/java/org/opfab/users/services/UserSettingsService.java (4)
18-19: Imports added appropriatelyThe added imports
UserActionEnumandUserActionLogServiceare necessary for the new logging functionality.
49-54: Constructor updated with new dependenciesThe constructor now includes
UserActionLogServiceanduserActionLogActivated, which is appropriate for enabling user action logging based on feature activation.
75-79: User existence verification addedAdding a check for user existence before proceeding prevents potential
NullPointerExceptionand improves error handling.
106-112: Method implementation correctThe
getProcessesStatesNotNotifiedTextmethod correctly formats the processes and states into a string representation.
| this.userSettingsService = new UserSettingsService(userSettingsRepository, null, notificationService, | ||
| userActionLogService, userActionLogActivated); |
There was a problem hiding this comment.
💡 Codebase verification
UserSettingsService constructor does not handle null for the second argument.
The UserSettingsService is instantiated with a null value as its second argument, and the constructor lacks checks or handling for this null value. This can lead to potential NullPointerException during runtime.
- File:
services/users/src/main/java/org/opfab/users/services/UserSettingsService.java- Constructor: Does not handle
nullfor the second parameter.
- Constructor: Does not handle
🔗 Analysis chain
Verify null handling in UserSettingsService constructor.
The UserSettingsService is instantiated with a null value as its second argument. This might lead to NullPointerException if not handled properly within the service.
Please ensure that UserSettingsService can handle this null argument safely. Consider running the following script to inspect the UserSettingsService constructor:
If null is not handled safely, consider refactoring UserSettingsService to use an optional parameter or provide an overloaded constructor.
Further verify null handling in UserSettingsService constructor.
The previous script did not find any null handling in the UserSettingsService constructor. To ensure that null is appropriately managed, please run the following script to examine the constructor implementation more thoroughly:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Inspect the constructor of UserSettingsService for null handling.
# Search for null checks or handling within the constructor.
rg --type java 'UserSettingsService\(' -A 10 services/users/src/main/java/org/opfab/users/services/UserSettingsService.java | rg -i 'null|requireNonNull|Optional\.ofNullable'
Length of output: 176
Script:
#!/bin/bash
# Description: Inspect the UserSettingsService constructor for null handling using a broader search.
# Extract the constructor of UserSettingsService
rg --type java 'public\s+UserSettingsService\s*\(' services/users/src/main/java/org/opfab/users/services/UserSettingsService.java -A 20 |
# Search for any null checks or handling within the constructor
rg -i 'null|requireNonNull|Optional\.ofNullable|if\s*\('
Length of output: 409
| function changeNotificationConfiguration() { | ||
| opfab.navigateToNotificationConfiguration(); | ||
| cy.get('.opfab-notificationconfiguration-processlist').eq(0).find('p').eq(1).find('input').uncheck({force: true}); | ||
| cy.get('#opfab-notificationconfiguration-btn-confirm').click(); // Save settings | ||
| cy.get('#opfab-btn-ok').click(); // and confirm | ||
| cy.get('#opfab-btn-ok').should('not.exist'); // wait for dialog to go away | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM with suggestion: Implement changeNotificationConfiguration() function
The new changeNotificationConfiguration() function correctly implements the process of changing notification settings, which is essential for testing the new 'NOTIFICATION_CONFIG' action log. This aligns well with the PR objectives.
However, consider improving the selector robustness:
The current selector .opfab-notificationconfiguration-processlist').eq(0).find('p').eq(1).find('input') might be fragile if the DOM structure changes. Consider using more stable selectors like IDs, classes, or data attributes to make the test more reliable.
Example improvement:
cy.get('[data-cy="notification-config-checkbox"]').uncheck({force: true});This assumes you can add a data-cy attribute to the relevant input in the application code.
|
Fix #7339