updated auditdetails logic #974
Conversation
WalkthroughThe Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- municipal-services/ws-services/src/main/java/org/egov/waterconnection/service/EnrichmentService.java (1 hunks)
🧰 Additional context used
| AuditDetails currentAuditDetails = waterConnectionRequest.getWaterConnection().getAuditDetails(); | ||
| AuditDetails auditDetails = waterServicesUtil | ||
| .getAuditDetails(waterConnectionRequest.getRequestInfo().getUserInfo().getUuid(), false); | ||
|
|
||
| auditDetails.setCreatedBy(currentAuditDetails.getCreatedBy()); | ||
| auditDetails.setCreatedTime(currentAuditDetails.getCreatedTime()); | ||
|
|
There was a problem hiding this comment.
Potential NullPointerException when accessing currentAuditDetails
Accessing currentAuditDetails.getCreatedBy() and currentAuditDetails.getCreatedTime() without checking if currentAuditDetails is null may lead to a NullPointerException. Consider adding a null check before accessing these methods.
Apply this diff to add a null check:
+ if (currentAuditDetails != null) {
auditDetails.setCreatedBy(currentAuditDetails.getCreatedBy());
auditDetails.setCreatedTime(currentAuditDetails.getCreatedTime());
+ }📝 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.
| AuditDetails currentAuditDetails = waterConnectionRequest.getWaterConnection().getAuditDetails(); | |
| AuditDetails auditDetails = waterServicesUtil | |
| .getAuditDetails(waterConnectionRequest.getRequestInfo().getUserInfo().getUuid(), false); | |
| auditDetails.setCreatedBy(currentAuditDetails.getCreatedBy()); | |
| auditDetails.setCreatedTime(currentAuditDetails.getCreatedTime()); | |
| AuditDetails currentAuditDetails = waterConnectionRequest.getWaterConnection().getAuditDetails(); | |
| AuditDetails auditDetails = waterServicesUtil | |
| .getAuditDetails(waterConnectionRequest.getRequestInfo().getUserInfo().getUuid(), false); | |
| if (currentAuditDetails != null) { | |
| auditDetails.setCreatedBy(currentAuditDetails.getCreatedBy()); | |
| auditDetails.setCreatedTime(currentAuditDetails.getCreatedTime()); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
municipal-services/ws-services/src/main/java/org/egov/waterconnection/service/EnrichmentService.java (1)
201-209: LGTM! Consider handling the null case explicitly.The changes to preserve the original creation details during updates are good. This helps maintain an accurate audit trail. However, consider explicitly handling the case where
currentAuditDetailsis null, perhaps by logging a warning or setting default values.You might want to add an
elseclause to handle the null case:if (currentAuditDetails != null) { auditDetails.setCreatedBy(currentAuditDetails.getCreatedBy()); auditDetails.setCreatedTime(currentAuditDetails.getCreatedTime()); +} else { + log.warn("currentAuditDetails is null, unable to set original creation details"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- municipal-services/ws-services/src/main/java/org/egov/waterconnection/service/EnrichmentService.java (1 hunks)
- municipal-services/ws-services/src/main/java/org/egov/waterconnection/service/WaterServiceImpl.java (4 hunks)
🧰 Additional context used
| public void enrichUpdateWaterConnection(AuditDetails currentAuditDetails,WaterConnectionRequest waterConnectionRequest) { | ||
| AuditDetails auditDetails = waterServicesUtil | ||
| .getAuditDetails(waterConnectionRequest.getRequestInfo().getUserInfo().getUuid(), false); | ||
|
|
||
| if (currentAuditDetails != null) { | ||
| auditDetails.setCreatedBy(currentAuditDetails.getCreatedBy()); | ||
| auditDetails.setCreatedTime(currentAuditDetails.getCreatedTime()); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring for improved maintainability.
The enrichUpdateWaterConnection method is quite long and handles multiple responsibilities. Consider breaking it down into smaller, more focused methods to improve readability and maintainability.
For example, you could extract the document, plumber info, and road cutting info enrichment into separate methods:
private void enrichDocuments(WaterConnection connection, AuditDetails auditDetails) {
if (!CollectionUtils.isEmpty(connection.getDocuments())) {
connection.getDocuments().forEach(document -> {
// ... (existing document enrichment logic)
});
}
}
private void enrichPlumberInfo(WaterConnection connection, AuditDetails auditDetails) {
if (!CollectionUtils.isEmpty(connection.getPlumberInfo())) {
connection.getPlumberInfo().forEach(plumberInfo -> {
// ... (existing plumber info enrichment logic)
});
}
}
private void enrichRoadCuttingInfo(WaterConnection connection, AuditDetails auditDetails) {
if (!CollectionUtils.isEmpty(connection.getRoadCuttingInfo())) {
connection.getRoadCuttingInfo().forEach(roadCuttingInfo -> {
// ... (existing road cutting info enrichment logic)
});
}
}Then, call these methods from enrichUpdateWaterConnection:
public void enrichUpdateWaterConnection(AuditDetails currentAuditDetails, WaterConnectionRequest waterConnectionRequest) {
// ... (existing audit details logic)
WaterConnection connection = waterConnectionRequest.getWaterConnection();
enrichDocuments(connection, auditDetails);
enrichPlumberInfo(connection, auditDetails);
enrichRoadCuttingInfo(connection, auditDetails);
enrichingAdditionalDetails(waterConnectionRequest);
}This refactoring would make the method easier to understand and maintain.
| @@ -252,7 +253,7 @@ public List<WaterConnection> updateWaterConnection(WaterConnectionRequest waterC | |||
| String previousApplicationStatus = workflowService.getApplicationStatus(waterConnectionRequest.getRequestInfo(), | |||
| waterConnectionRequest.getWaterConnection().getApplicationNo(), | |||
| waterConnectionRequest.getWaterConnection().getTenantId(), config.getBusinessServiceValue()); | |||
| enrichmentService.enrichUpdateWaterConnection(waterConnectionRequest); | |||
| enrichmentService.enrichUpdateWaterConnection(auditDetails, waterConnectionRequest); | |||
There was a problem hiding this comment.
Ensure auditDetails is properly initialized before use
At line 256, auditDetails is passed to enrichmentService.enrichUpdateWaterConnection(auditDetails, waterConnectionRequest);. Ensure that auditDetails has been properly initialized and is not null to prevent potential NullPointerException.
| @@ -342,7 +344,7 @@ private List<WaterConnection> updateWaterConnectionForModifyFlow(WaterConnection | |||
| String previousApplicationStatus = workflowService.getApplicationStatus(waterConnectionRequest.getRequestInfo(), | |||
| waterConnectionRequest.getWaterConnection().getApplicationNo(), | |||
| waterConnectionRequest.getWaterConnection().getTenantId(), config.getModifyWSBusinessServiceName()); | |||
| enrichmentService.enrichUpdateWaterConnection(waterConnectionRequest); | |||
| enrichmentService.enrichUpdateWaterConnection(auditDetails, waterConnectionRequest); | |||
There was a problem hiding this comment.
Ensure auditDetails is properly initialized before use
At line 347, auditDetails is passed to enrichmentService.enrichUpdateWaterConnection(auditDetails, waterConnectionRequest);. Verify that auditDetails is not null to prevent potential NullPointerException during method execution.
| @@ -236,6 +236,7 @@ public List<WaterConnection> updateWaterConnection(WaterConnectionRequest waterC | |||
| mDMSValidator.validateMISFields(waterConnectionRequest); | |||
| waterConnectionValidator.validateWaterConnection(waterConnectionRequest, WCConstants.UPDATE_APPLICATION); | |||
| List<WaterConnection> waterConnection = getWaterConnectionForOldConnectionNo(waterConnectionRequest); | |||
| AuditDetails auditDetails=waterConnection.get(0).getAuditDetails(); | |||
There was a problem hiding this comment.
Potential NullPointerException or IndexOutOfBoundsException due to unchecked access of waterConnection
At line 239, waterConnection.get(0).getAuditDetails() is called without verifying whether waterConnection is non-null and contains elements. If waterConnection is null or empty, this will lead to a NullPointerException or IndexOutOfBoundsException.
Apply this diff to fix the issue:
+if (waterConnection != null && !waterConnection.isEmpty()) {
AuditDetails auditDetails = waterConnection.get(0).getAuditDetails();
+} else {
+ // Handle the scenario where waterConnection is null or empty
+ throw new CustomException("WATER_CONNECTION_NOT_FOUND", "No water connections found for the given criteria");
+}Committable suggestion was skipped due to low confidence.
| @@ -326,6 +327,7 @@ private List<WaterConnection> getWaterConnectionForOldConnectionNo(WaterConnecti | |||
| private List<WaterConnection> updateWaterConnectionForModifyFlow(WaterConnectionRequest waterConnectionRequest) { | |||
| waterConnectionValidator.validateWaterConnection(waterConnectionRequest, WCConstants.MODIFY_CONNECTION); | |||
| List<WaterConnection> waterConnection = getWaterConnectionForOldConnectionNo(waterConnectionRequest); | |||
| AuditDetails auditDetails=waterConnection.get(0).getAuditDetails(); | |||
There was a problem hiding this comment.
Potential NullPointerException or IndexOutOfBoundsException due to unchecked access of waterConnection
At line 330, waterConnection.get(0).getAuditDetails() is accessed without checking if waterConnection is non-null and not empty. This can lead to exceptions if waterConnection is null or has no elements.
Apply this diff to fix the issue:
+if (waterConnection != null && !waterConnection.isEmpty()) {
AuditDetails auditDetails = waterConnection.get(0).getAuditDetails();
+} else {
+ // Handle the scenario where waterConnection is null or empty
+ throw new CustomException("WATER_CONNECTION_NOT_FOUND", "No water connections found for the given criteria");
+}Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes
Chores