[Aarti-panchal01]

📋 Description

Proof of concept fix for the HTTP status code and patient data logging issues identified in PSMRI/AMRIT#153.

Problem 1: Always HTTP 200
Every controller method returned String instead of ResponseEntity, meaning the HTTP status was always 200 — even on failures. The error details were buried in the JSON body, forcing clients to parse the body to detect failures instead of using standard HTTP semantics. The fix already existed — OutputResponse.toStringWithHttpStatus() was implemented but never used. This PR uses it.

Problem 2: Patient data in logs
Every controller logged the full raw requestObj at INFO level. These request bodies contain patient vitals, medical history, examination details and prescriptions. INFO logs are always-on in production — meaning sensitive patient data was being stored in plain text logs. Replaced with a generic log message containing no patient data.

Fixes: PSMRI/AMRIT#153

---

✅ Type of Change

• 🐞 Bug fix (non-breaking change which resolves an issue)
• 🛠 Refactor (change that is neither a fix nor a new feature)

---

ℹ️ Additional Information

Scope: One method in GeneralOPDController as proof of concept.

Changes made:

• Return type: String → ResponseEntity<String>
• response.toString() → response.toStringWithHttpStatus()
• Raw requestObj logging removed, replaced with generic message

Next steps if approved:

• Add @ControllerAdvice global exception handler
• Extend to remaining 20+ controllers
• Fix BAD_REQUEST mapped to 404 instead of 400 in OutputResponse.java

Summary by CodeRabbit

• Bug Fixes
  • Fixed nurse OPD data endpoint to properly return appropriate HTTP status codes for requests, ensuring accurate API response status handling.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1de5e894-4234-403c-8ecb-e98bcd77886f

---

📝 Walkthrough

Walkthrough

The saveBenGenOPDNurseData endpoint in GeneralOPDController is updated to return ResponseEntity<String> instead of plain String. The return statement now uses response.toStringWithHttpStatus() to include HTTP status information. Request logging is simplified to a generic message instead of echoing the request payload.

Changes

Nurse OPD Save Endpoint

Layer / File(s)	Summary
Nurse save endpoint return type and HTTP response handling src/main/java/com/iemr/hwc/controller/generalOPD/GeneralOPDController.java	The ResponseEntity import is added, the saveBenGenOPDNurseData method signature is updated to return ResponseEntity<String>, request logging is simplified to "Request received", and the return statement is updated to use response.toStringWithHttpStatus() for HTTP-aware serialization.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related issues

• [HWC-API] API always returns HTTP 200 even on failure + patient data getting logged at INFO level AMRIT#153: Directly addresses the same nurse save endpoint update—changing the return type to ResponseEntity, using OutputResponse.toStringWithHttpStatus(), and reducing sensitive request-body logging.

---

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly reflects the two main changes in the PR: returning correct HTTP status codes (via ResponseEntity and toStringWithHttpStatus) and removing patient data logging (replacing raw requestObj logging with generic messages).
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Comment @coderabbitai help to get the list of available commands.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/main/java/com/iemr/hwc/controller/generalOPD/GeneralOPDController.java (1)

84-109: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Move JSON parsing inside the try-catch block.

requestObj is parsed at lines 91–92 before the try block (which starts at line 94), so malformed JSON or non-object payloads throw JsonSyntaxException or IllegalStateException before reaching the error handler. These exceptions propagate uncaught and return a default 500 error instead of the structured error response. Wrap the parse and getAsJsonObject() calls inside the try-catch so exceptions are caught and handled via response.setError(), then properly serialized by toStringWithHttpStatus().

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/java/com/iemr/hwc/controller/generalOPD/GeneralOPDController.java`
around lines 84 - 109, The JSON parsing (JsonParser/JsonElement and jsnOBJ =
jsnElmnt.getAsJsonObject()) must be moved inside the existing try block in
saveBenGenOPDNurseData so JsonSyntaxException/IllegalStateException are caught
and turned into a structured error via response.setError(...) before returning
response.toStringWithHttpStatus(); ensure jsnOBJ is declared outside the try so
it can still be passed to generalOPDServiceImpl.deleteVisitDetails(jsnOBJ) in
the catch, and do not let parsing occur prior to the try that would bypass the
error handler.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/main/java/com/iemr/hwc/controller/generalOPD/GeneralOPDController.java`:
- Around line 84-109: The JSON parsing (JsonParser/JsonElement and jsnOBJ =
jsnElmnt.getAsJsonObject()) must be moved inside the existing try block in
saveBenGenOPDNurseData so JsonSyntaxException/IllegalStateException are caught
and turned into a structured error via response.setError(...) before returning
response.toStringWithHttpStatus(); ensure jsnOBJ is declared outside the try so
it can still be passed to generalOPDServiceImpl.deleteVisitDetails(jsnOBJ) in
the catch, and do not let parsing occur prior to the try that would bypass the
error handler.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 723aa9f4-e80d-4aa5-8a6e-b48def05add8

📥 Commits

Reviewing files that changed from the base of the PR and between 3861dd1 and e3250b8.

📒 Files selected for processing (1)

• src/main/java/com/iemr/hwc/controller/generalOPD/GeneralOPDController.java

[Aarti-panchal01]

Hi @drtechie and @sharma-sugurthi
Implemented the proof of concept for #153 in GeneralOPDController.saveBenGenOPDNurseData():

1. Return type: String → ResponseEntity
2. Used existing toStringWithHttpStatus() (was already there, just unused!)
3. Removed raw requestObj logging — patient vitals/history were being
   logged at INFO level in production

This is intentionally scoped to one method as a proof of concept.
Happy to extend to other controllers if the approach is approved.

[sharma-sugurthi]

hiii...
a few things I noticed:

1. the JSON parsing (lines 91-92, JsonParser and getAsJsonObject()) is still outside the try-catch.if someone sends malformed JSON, it throws JsonSyntaxException before reaching the error handler, so instead of a structured error response, the client gets a raw 500 stacktrace.that needs to be inside the try block.

2. SonarCloud shows 0% coverage on new code. since this changes the return type and response behavior, at least one test verifying the new ResponseEntity return would be good.

3. this only covers one method in one controller. the issue describes the pattern across 20+ controllers,would be good to mention the scope and next steps more clearly.

[sharma-sugurthi]

This issue is a core design desicion,recommend to wait until approval of approaches from atleast now,if not this may waste lots of time on unwanted things and makes things messy..
Until then,I recommend you to study the AMRIT repo and project scopes thoroughly....

[Aarti-panchal01]

Hi @sharma-sugurthi, thanks for looking at this!

The JSON parsing point is valid -> moving it inside the try block
makes sense, I'll push that fix.

On scope, that was intentional -> one method to validate the approach
before touching 20+ controllers. Didn't want to refactor everything
before getting a nod on the direction.

On tests -> fair, I'll add one verifying the ResponseEntity return.

And agreed on waiting for architectural sign-off before going further.

@drtechie -- whenever you get a chance, happy to hear your thoughts
on the approach before I extend it further.

[vanitha1822]

Need to have change from HWC-UI

[Aarti-panchal01]

@vanitha1822 the UI side is in PSMRI/HWC-UI#290. Once this returns real HTTP status codes, a failed nurse save goes to the error callback, and #290 shows the server's errorMessage there (falling back to the existing common.unableSave text) instead of [object Object].

@sharma-sugurthi both of your points are now pushed (rebased on main, 0cb91e3):

• JSON parsing is inside the try, so malformed or non-object JSON returns a structured 400 instead of an uncaught exception.
• Added GeneralOPDControllerTest: success → 200, service failure → 500 with the visit rolled back, malformed JSON → 400, non-object JSON → 400. 4/4 pass locally.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud