AMM-1266 and lmpDateformat corrected#59
Conversation
WalkthroughThis pull request updates methods in two Java classes and the application configuration. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as OutBoundWorklistController
participant Service as outboundWorkListServiceImpl
participant Mapper as ObjectMapper
Client->>Controller: getMotherWorklist(userId)
Controller->>Service: getMotherWorkList(userId)
Service-->>Controller: List<FetchMotherOutboundWorklist>
Controller->>Mapper: writeValueAsString(motherWorkList)
Mapper-->>Controller: JSON String
Controller->>Client: ResponseEntity(JSON String)
sequenceDiagram
participant Caller
participant CallClosure as CallClosureImpl
Caller->>CallClosure: closeCall(request)
alt if request.getIsHrp is true
CallClosure->>CallClosure: Evaluate receivedRoleName
alt if receivedRoleName == Constants.ANM
CallClosure->>CallClosure: Set callStatus = "Open"
end
end
CallClosure-->>Caller: Return call closure outcome
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 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
🧹 Nitpick comments (2)
src/main/resources/application.properties (1)
53-54:Details
❓ Verification inconclusive
Review the necessity of Spring configuration changes
These Spring configuration properties can introduce maintenance challenges:
allow-bean-definition-overriding=trueallows later bean definitions to override earlier ones with identical names (disabled by default since Spring 5.1)allow-circular-references=truepermits circular dependencies between beans (typically considered an anti-pattern)Rather than enabling these potentially problematic behaviors, consider refactoring the application to address the underlying design issues that necessitate these properties.
🏁 Script executed:
#!/bin/bash # Check for potential circular dependencies in the codebase echo "Searching for circular dependency patterns in the codebase..." rg -A 2 -B 2 "@Autowired|@Inject" --type java | grep -A 5 -B 5 "private.*Service|private.*Repository|private.*Component|private.*Controller"Length of output: 275
Action Required: Reevaluate Spring Configuration Settings
The configuration in
src/main/resources/application.propertiesat lines 53–54 currently enables:spring.main.allow-bean-definition-overriding=true spring.main.allow-circular-references=trueConcerns:
- Bean Overriding: Allows later bean definitions to override earlier ones, which can obscure configuration issues.
- Circular References: Permits circular dependencies between beans—a practice that may hide underlying design problems and lead to maintenance challenges.
The automated scan for circular dependency patterns did not produce any output. However, this lack of findings does not conclusively rule out issues; the nature of such dependency problems often requires a deeper, manual inspection of bean wiring and application design.
Next Steps:
- Manual Verification: Please review the bean configuration and dependency graph to confirm that these settings are truly necessary.
- Consider Refactoring: Where possible, refactor your bean definitions to eliminate reliance on both overriding and circular references.
src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java (1)
209-214: Added conditional status processing for high-risk patients detected by ANMThe implementation properly sets high risk flag and conditionally updates call status to "Open" when the received role is ANM, ensuring these cases receive appropriate follow-up.
Consider adding a comment explaining the business rule that requires ANM-detected high-risk cases to be reopened, as this would improve code maintainability and make the domain logic more explicit.
if (request.getIsHrp() != null) { callObj.setIsHighRisk(request.getIsHrp()); + // ANM-detected high-risk pregnancies require additional follow-up, so we keep the call status open if(obj.getReceivedRoleName().equalsIgnoreCase(Constants.ANM)){ callObj.setCallStatus(Constants.OPEN); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/ecd/controller/outboundworklist/OutBoundWorklistController.java(3 hunks)src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java(1 hunks)src/main/resources/application.properties(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/iemr/ecd/service/associate/CallClosureImpl.java (1)
src/main/java/com/iemr/ecd/utils/constants/Constants.java (1)
Constants(5-21)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
src/main/java/com/iemr/ecd/controller/outboundworklist/OutBoundWorklistController.java (1)
24-25: New imports added to support JSON serialization with custom date formatThe newly added imports support the modified implementation of
getMotherWorklistmethod.Also applies to: 37-39
| public ResponseEntity<String> getMotherWorklist(@PathVariable int userId) throws JsonProcessingException { | ||
| List<FetchMotherOutboundWorklist> motherWorkList = outboundWorkListServiceImpl.getMotherWorkList(userId); | ||
| ObjectMapper objectMapper = new ObjectMapper(); | ||
| objectMapper.setDateFormat(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss")); | ||
| String writeValueAsString = objectMapper.writeValueAsString(motherWorkList); | ||
|
|
||
| return new ResponseEntity<>(writeValueAsString,HttpStatus.OK); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Standardize JSON response format with custom date formatting
The method now returns a JSON string with a consistent date format pattern, which should improve date handling on the client side. However, this approach differs from the getChildWorklist method that uses Spring's default serialization.
For better consistency and maintainability, consider:
- Using the same approach for both mother and child endpoints
- Configuring date formatting globally in your application
-public ResponseEntity<String> getMotherWorklist(@PathVariable int userId) throws JsonProcessingException {
- List<FetchMotherOutboundWorklist> motherWorkList = outboundWorkListServiceImpl.getMotherWorkList(userId);
- ObjectMapper objectMapper = new ObjectMapper();
- objectMapper.setDateFormat(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"));
- String writeValueAsString = objectMapper.writeValueAsString(motherWorkList);
-
- return new ResponseEntity<>(writeValueAsString,HttpStatus.OK);
+public ResponseEntity<List<FetchMotherOutboundWorklist>> getMotherWorklist(@PathVariable int userId) {
+ return new ResponseEntity<>(outboundWorkListServiceImpl.getMotherWorkList(userId), HttpStatus.OK);
}Then configure the date format globally in your application configuration:
@Configuration
public class JacksonConfig {
@Bean
public Jackson2ObjectMapperBuilderCustomizer customizer() {
return builder -> builder.dateFormat(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"));
}
}🏁 Script executed:
#!/bin/bash
# Check if there's a global ObjectMapper configuration in the codebase
echo "Searching for existing Jackson configuration..."
rg -A 5 -B 5 "ObjectMapper|Jackson2ObjectMapperBuilder|@JsonFormat" --type javaLength of output: 8629
Refactor Serialization Approach for Consistency
We recommend updating the getMotherWorklist method to align with the serialization approach used in getChildWorklist. Rather than manually instantiating an ObjectMapper and setting a date format within the controller, return a typed response (i.e. ResponseEntity<List<FetchMotherOutboundWorklist>>) and centralize the date formatting through a global configuration.
Action Items:
- Update the method’s signature to return the list directly.
- Remove the manual creation of
ObjectMapperand custom date formatting. - Configure global date formatting (e.g., via a
JacksonConfigclass with aJackson2ObjectMapperBuilderCustomizerbean) so both endpoints share the same settings.
Diff Suggestion:
-public ResponseEntity<String> getMotherWorklist(@PathVariable int userId) throws JsonProcessingException {
- List<FetchMotherOutboundWorklist> motherWorkList = outboundWorkListServiceImpl.getMotherWorkList(userId);
- ObjectMapper objectMapper = new ObjectMapper();
- objectMapper.setDateFormat(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"));
- String writeValueAsString = objectMapper.writeValueAsString(motherWorkList);
-
- return new ResponseEntity<>(writeValueAsString,HttpStatus.OK);
-}
+public ResponseEntity<List<FetchMotherOutboundWorklist>> getMotherWorklist(@PathVariable int userId) {
+ return new ResponseEntity<>(outboundWorkListServiceImpl.getMotherWorkList(userId), HttpStatus.OK);
+}Global Configuration Example:
@Configuration
public class JacksonConfig {
@Bean
public Jackson2ObjectMapperBuilderCustomizer customizer() {
return builder -> builder.dateFormat(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"));
}
}There was a problem hiding this comment.
@coderabbitai
if i tried JacksonConfig like above getting timestamp format wrong. so i keep my code only.
There was a problem hiding this comment.
@ravishanigarapu I understand completely. If you've already tested the global configuration approach and found issues with timestamp formatting, then keeping your current implementation makes perfect sense. Sometimes manual configuration is necessary to meet specific formatting requirements.
One small suggestion: consider renaming the variable writeValueAsString to something like jsonResponse since it represents the result rather than the action being performed. But that's just a minor readability improvement.
✏️ Learnings added
Learnt from: ravishanigarapu
PR: PSMRI/ECD-API#59
File: src/main/java/com/iemr/ecd/controller/outboundworklist/OutBoundWorklistController.java:0-0
Timestamp: 2025-04-10T06:29:26.020Z
Learning: In the ECD-API project, manual ObjectMapper configuration with specific date formatting (yyyy-MM-dd HH:mm:ss) is preferred over global Jackson configuration due to timestamp formatting issues experienced when using the global approach.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
2 participants
📋 Description
JIRA ID: AMM-1266
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Bug Fixes
Chores