17227-implement-audit-event-recording-for-patient-entity-create-operations#18263
17227-implement-audit-event-recording-for-patient-entity-create-operations#18263denuwanhendalage wants to merge 6 commits intodevelopmentfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds auditing to PatientController: injects Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Controller as PatientController
participant Facade as AuditEventFacade
participant Service as AuditService
participant DB as Database
User->>Controller: saveSelected(patient)
Controller->>Controller: validate & persist Patient
Controller->>Facade: findLastByObjectId(objectId)
Facade->>DB: query latest AuditEvent
DB-->>Facade: return previous AuditEvent
Facade-->>Controller: previous audit (before)
Controller->>Controller: build current snapshot via createPatientJson
Controller->>Service: createAuditEventPatientSaved(with before/after)
Service->>DB: persist audit event
DB-->>Service: ack
Service-->>Controller: audit recorded
Controller-->>User: save complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/main/java/com/divudi/bean/common/PatientController.java`:
- Line 3174: The "Nothing to Delete" notification in PatientController is using
JsfUtil.addSuccessMessage incorrectly; change it to a non-success type (e.g.,
JsfUtil.addErrorMessage or JsfUtil.addWarningMessage) so the message reflects
that no patient was selected for deletion. Locate the call to
JsfUtil.addSuccessMessage("Nothing to Delete") in the PatientController and
replace it with the appropriate error/warning helper to convey failure instead
of success.
- Line 110: Remove the invalid static import of objectId from LogFactory which
causes compilation to fail: delete the line importing "static
org.apache.commons.logging.LogFactory.objectId" in PatientController (or replace
it with a valid import if a different symbol was intended), then recompile to
ensure no usages of objectId remain; if code references objectId, replace those
usages with the correct variable or method call.
- Line 3192: Replace the incorrect success notification with an error
notification: locate the call to JsfUtil.addSuccessMessage("Nothing to Delete")
and change it to JsfUtil.addErrorMessage("Nothing to Delete") (or the
appropriate i18n key) so the UI shows an error message when there is nothing to
delete; update any surrounding logic in the same method if it assumes a success
outcome.
🧹 Nitpick comments (2)
src/main/java/com/divudi/bean/common/PatientController.java (2)
293-293: Unused field declaration.The
objectIdfield is declared but never used. ThecreateAuditEventPatientSavedmethod at line 3487 declares a local variable with the same name instead.🧹 Proposed fix: Remove the unused field
- private Long objectId;
3406-3502: Audit implementation is functional but has improvement opportunities.The audit event creation logic correctly:
- Retrieves previous state from the last audit event (consistent with BhtEditController pattern per learnings)
- Captures comprehensive patient and person data
- Wraps in try-catch to prevent audit failures from breaking patient save
However, consider these improvements:
Logging: Replace
System.err.printlnwith a proper logging framework (e.g.,java.util.logging.Loggeralready imported in this file).Method length: At ~95 lines, this method could be split into smaller helpers (e.g.,
buildPatientStateMap,getPreviousAuditState).♻️ Proposed improvement for logging
} catch (Exception e) { // Don't let audit failures break the patient save operation - System.err.println("Error creating audit event: " + e.getMessage()); - e.printStackTrace(); + Logger.getLogger(PatientController.class.getName()).log(Level.WARNING, "Error creating audit event", e); }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/com/divudi/bean/common/PatientController.java`:
- Around line 3400-3402: The audit logic currently computes eventTrigger from
before == null which mislabels updates for existing patients lacking prior audit
entries; in PatientController, capture a boolean isNew = (p.getId() == null) (or
equivalent new-entity check) before saving and pass or use that flag when
calling createAuditEventPatientSaved(p) (and any related audit methods) so the
trigger is based on isNew rather than a post-save lookup; remove/replace the
before==null lookup and use isNew to choose "Create patient" vs "Edit patient"
in the methods referenced (createAuditEventPatientSaved and the surrounding
audit logic).
There was a problem hiding this comment.
Actionable comments posted: 0
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/divudi/bean/common/PatientController.java (1)
3324-3328:⚠️ Potential issue | 🔴 CriticalFix NPE:
isNewcomputed before null check.
Line 3324 dereferencespbefore the null guard, which will throw ifsaveSelected(null)is called (regression from prior behavior).🐛 Suggested fix
public boolean saveSelected(Patient p) { - boolean isNew = (p.getId() == null); if (p == null) { JsfUtil.addErrorMessage("No Current. Error. NOT SAVED"); return false; } + boolean isNew = (p.getId() == null);
buddhika75
left a comment
buddhika75
left a comment
There was a problem hiding this comment.
Review Summary
The audit logging feature is a good addition, but there's a critical bug that must be fixed before merging.
🔴 Critical: NullPointerException Bug (Line 3320-3325)
The current code will throw a NullPointerException if saveSelected(null) is called:
public boolean saveSelected(Patient p) {
boolean isNew = (p.getId() == null); // ← NPE here if p is null!
if (p == null) {
JsfUtil.addErrorMessage("No Current. Error. NOT SAVED");
return false;
}Fix: Move the isNew assignment AFTER the null check:
public boolean saveSelected(Patient p) {
if (p == null) {
JsfUtil.addErrorMessage("No Current. Error. NOT SAVED");
return false;
}
boolean isNew = (p.getId() == null); // ← Safe now⚠️ Minor Issues
-
Unused field (Line 293):
private Long objectId;is declared but never used. A local variable with the same name is used in the method instead. Remove this unused field. -
Inconsistent indentation: The
createAuditEventPatientSavedmethod has inconsistent indentation. The try block and its contents should be properly indented. -
Unnecessary whitespace changes (Lines 3173, 3191): Extra spaces were added before
JsfUtil.addErrorMessage. These unrelated changes add noise to the diff. -
Missing space before brace (Line 3313):
public AuditEventFacade getAuditEventFacade(){should have a space before{.
Please fix the critical NPE bug and address the minor issues, then I can approve the PR.
…the details are edited. Signed-off-by: denuwanhendalage <shashankadenuwanb@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/java/com/divudi/bean/common/PatientController.java`:
- Around line 292-294: The fields isNewPatient and before on PatientController
are session-scoped and can persist stale state; in saveSelected(Patient p) (and
any entry point that saves patients) reset isNewPatient and before at the start
by computing isNewPatient = (p == null || p.getId() == null) and setting before
= null (or capturing the real previous state only when needed) so each save uses
fresh audit context; also remove the unused objectId field (it's never read and
is shadowed by a local variable) and update any navigation methods
(navigateToEmrEditPatient, navigateToOpdPatientEditFromId) to no longer rely on
pre-setting those fields.
- Around line 1084-1087: The EMR edit flow is missing audit snapshots and
secondary OPD paths don't capture the `before` state; update either the EMR save
flow or routing so audits are consistent: ensure `createPatientJson(current)` is
invoked before edits and `createAuditEventPatientSaved()` is called when a
patient is saved from the EMR flow (methods: navigateToEmrEditPatient(),
saveAndNavigateToProfile()), or alternatively route EMR edits into the existing
OPD save flow by calling `saveSelected()` from the EMR path so `before` is set
and `createAuditEventPatientSaved()` runs; also add a `before =
createPatientJson(current)` in `navigateToOpdPatientEditFromId()` (or make it
navigate to the actual edit form) to ensure secondary OPD edits capture the
snapshot.
🧹 Nitpick comments (3)
src/main/java/com/divudi/bean/common/PatientController.java (3)
1090-1137: Minor issues increatePatientJson.
- Misleading variable name (line 1097):
patientMapactually holds person details — consider renaming topersonMap.- Fabricated timestamp (lines 1122-1125): When
createdAtis null, the snapshot recordsnew Date()as the creation time. This silently invents data and could mislead auditors. Usenullinstead, or omit the key.- Return type: Method returns
Objectbut always producesMap<String, Object>. A specific return type improves readability and avoids unchecked casts downstream.Suggested adjustments
- public Object createPatientJson(Patient patient){ + public Map<String, Object> createPatientJson(Patient patient){ Map<String , Object> json = new HashMap<>(); ... if (patient.getPerson() != null){ - Map<String , Object> patientMap = new HashMap<>(); + Map<String , Object> personMap = new HashMap<>(); ... } ... - } else { - java.text.SimpleDateFormat sdf = new java.text.SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss"); - json.put("createdAt", sdf.format(new Date())); - } + } else { + json.put("createdAt", null); + }
3459-3486: Use proper logging instead ofSystem.err/printStackTrace.The catch block uses
System.err.printlnande.printStackTrace()(lines 3483-3484). The rest of the codebase usesjava.util.logging.Logger. Also,if (isNewPatient == true)is verbose.Suggested cleanup
public void createAuditEventPatientSaved(Patient patient){ try { - if (isNewPatient == true){ + if (isNewPatient){ before = null; } ... } catch (Exception e) { - System.err.println("Error creating audit event: " + e.getMessage()); - e.printStackTrace(); + Logger.getLogger(PatientController.class.getName()).log(Level.SEVERE, "Error creating audit event", e); } }
70-71: Unused import:TypeToken.
com.google.gson.reflect.TypeToken(line 71) is imported but never used in this file. OnlyGsonis imported but also appears unused — thecreatePatientJsonmethod builds aMapmanually without using Gson at all.
| private Long objectId; | ||
| private boolean isNewPatient; | ||
| private Object before; |
There was a problem hiding this comment.
Stale audit state in @SessionScoped bean risks mislabeled audit events.
isNewPatient and before persist across requests. If a user enters via a navigation path that doesn't explicitly set them (e.g., navigateToEmrEditPatient, navigateToOpdPatientEditFromId, or any call to saveSelected() outside the OPD flow), the values will carry over from the previous operation, producing incorrect audit records.
Additionally, the objectId field is never read — it's shadowed by a local variable at line 3471.
Consider:
- Resetting
isNewPatientandbeforeat the start ofsaveSelected(Patient p)based onp.getId() == null, rather than relying on navigation methods to set them. - Removing the unused
objectIdfield.
Proposed fix sketch
public boolean saveSelected(Patient p) {
+ // Determine new vs. update from entity state, not navigation flags
+ boolean isNew = (p != null && p.getId() == null);
if (p == null) {
JsfUtil.addErrorMessage("No Current. Error. NOT SAVED");
return false;
}
...
- createAuditEventPatientSaved(p);
+ createAuditEventPatientSaved(p, isNew);
return true;
}
-public void createAuditEventPatientSaved(Patient patient){
+public void createAuditEventPatientSaved(Patient patient, boolean isNew){
try {
- if (isNewPatient == true){
- before = null;
- }
Object after = createPatientJson(patient);
- String eventTrigger = isNewPatient ? "Create patient" : "Update patient";
- Long objectId = patient.getId();
+ String eventTrigger = isNew ? "Create patient" : "Update patient";
+ Object beforeSnapshot = isNew ? null : this.before;
auditService.logAudit(
- before, after,
+ beforeSnapshot, after,
sessionController.getLoggedUser(),
"Patient", eventTrigger,
- objectId
+ patient.getId()
);
} catch (Exception e) {
System.err.println("Error creating audit event: " + e.getMessage());
- e.printStackTrace();
}
}And remove the unused field:
- private Long objectId;🤖 Prompt for AI Agents
In `@src/main/java/com/divudi/bean/common/PatientController.java` around lines 292
- 294, The fields isNewPatient and before on PatientController are
session-scoped and can persist stale state; in saveSelected(Patient p) (and any
entry point that saves patients) reset isNewPatient and before at the start by
computing isNewPatient = (p == null || p.getId() == null) and setting before =
null (or capturing the real previous state only when needed) so each save uses
fresh audit context; also remove the unused objectId field (it's never read and
is shadowed by a local variable) and update any navigation methods
(navigateToEmrEditPatient, navigateToOpdPatientEditFromId) to no longer rely on
pre-setting those fields.
|
|
||
| isNewPatient = false; | ||
| before = createPatientJson(current); | ||
| return "/opd/patient_edit?faces-redirect=true"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, get the total line count to understand file size
wc -l src/main/java/com/divudi/bean/common/PatientController.java
# Search for all navigation methods and related audit setup
rg -n "navigateTo|isNewPatient\s*=|before\s*=\s*createPatientJson" src/main/java/com/divudi/bean/common/PatientController.javaRepository: hmislk/hmis
Length of output: 4909
🏁 Script executed:
#!/bin/bash
# Check navigateToOpdPatientEditFromId implementation
sed -n '1139,1160p' src/main/java/com/divudi/bean/common/PatientController.java
# Check navigateToEmrEditPatient implementation
sed -n '3157,3180p' src/main/java/com/divudi/bean/common/PatientController.java
# Find saveSelected method
rg -n "saveSelected|createAuditEventPatientSaved" src/main/java/com/divudi/bean/common/PatientController.java | head -20Repository: hmislk/hmis
Length of output: 2206
🏁 Script executed:
#!/bin/bash
# Check saveSelected method implementation
sed -n '3352,3380p' src/main/java/com/divudi/bean/common/PatientController.java
# Check createAuditEventPatientSaved method implementation
sed -n '3459,3550p' src/main/java/com/divudi/bean/common/PatientController.javaRepository: hmislk/hmis
Length of output: 4353
🏁 Script executed:
#!/bin/bash
# Check what calls navigateToOpdPatientEditFromId and navigateToEmrEditPatient
rg -n "navigateToOpdPatientEditFromId|navigateToEmrEditPatient" src/main/webapp --type=xhtml
# Look for save buttons on /opd/patient and /emr/patient pages
find src/main/webapp -name "patient.xhtml" -o -name "patient_*.xhtml" | head -10Repository: hmislk/hmis
Length of output: 580
🏁 Script executed:
#!/bin/bash
# Search in all files for references to save methods in navigation paths
rg -l "navigateToOpdPatientEditFromId|navigateToEmrEditPatient" src/main/webapp
# Check if /opd/patient.xhtml exists
ls -la src/main/webapp/opd/patient.xhtml 2>&1
# Look for save-related calls in patient edit pages
rg "saveSelected|patient\.save" src/main/webapp/emr/patient.xhtml | head -20Repository: hmislk/hmis
Length of output: 780
🏁 Script executed:
#!/bin/bash
# Check /opd/patient.xhtml for save functionality
rg -n "saveSelected|save\(" src/main/webapp/opd/patient.xhtml | head -20
# Check /emr/patient.xhtml for save functionality
rg -n "saveSelected|save\(" src/main/webapp/emr/patient.xhtml | head -20
# Also check the size and look for patient form management
wc -l src/main/webapp/opd/patient.xhtml src/main/webapp/emr/patient.xhtmlRepository: hmislk/hmis
Length of output: 146
🏁 Script executed:
#!/bin/bash
# Read /opd/patient.xhtml to understand the edit workflow
cat src/main/webapp/opd/patient.xhtml
echo "---"
# Read /emr/patient.xhtml
cat src/main/webapp/emr/patient.xhtmlRepository: hmislk/hmis
Length of output: 50367
🏁 Script executed:
#!/bin/bash
# Find saveAndNavigateToProfile method
rg -n "saveAndNavigateToProfile" src/main/java/com/divudi/bean/common/PatientController.java -A 10
# Check what navigateToEmrEditPatient actually does and if it sets isNewPatient
sed -n '3157,3170p' src/main/java/com/divudi/bean/common/PatientController.javaRepository: hmislk/hmis
Length of output: 936
🏁 Script executed:
#!/bin/bash
# Get full saveAndNavigateToProfile implementation
sed -n '3583,3620p' src/main/java/com/divudi/bean/common/PatientController.java
# Find where navigateToEmrEditPatient is called
rg -n "navigateToEmrEditPatient" src/main/webapp --type=xmlRepository: hmislk/hmis
Length of output: 2369
🏁 Script executed:
#!/bin/bash
# Check the full saveSelected implementation to see when createAuditEventPatientSaved is called
sed -n '3379,3470p' src/main/java/com/divudi/bean/common/PatientController.java
# Check what saveAndNavigateToProfile returns next
sed -n '3620,3635p' src/main/java/com/divudi/bean/common/PatientController.javaRepository: hmislk/hmis
Length of output: 4289
🏁 Script executed:
#!/bin/bash
# Find all patient edit pages
find src/main/webapp -name "*patient*edit*" -o -name "*patient*" | grep -i edit
# Check if there's a separate edit page
rg -l "patient_edit" src/main/webapp
# Verify the complete workflow: what page does navigateToOpdPatientEdit actually redirect to?
rg -B5 -A5 "navigateToOpdPatientEdit" src/main/webapp --type=xml | head -50Repository: hmislk/hmis
Length of output: 7978
🏁 Script executed:
#!/bin/bash
# Check /opd/patient_edit.xhtml to see if it has save functionality
wc -l src/main/webapp/opd/patient_edit.xhtml
# Search for saveSelected calls in that file
rg -n "saveSelected|save" src/main/webapp/opd/patient_edit.xhtml | head -20Repository: hmislk/hmis
Length of output: 333
🏁 Script executed:
#!/bin/bash
# Find saveAndNavigateToOpdPatientProfile method
rg -n "saveAndNavigateToOpdPatientProfile" src/main/java/com/divudi/bean/common/PatientController.java -A 15Repository: hmislk/hmis
Length of output: 657
Audit trail incomplete for EMR patient edits and missing before snapshot in secondary OPD paths.
Only navigateToOpdPatientEdit() captures the before state at line 1086. However, the EMR edit workflow (navigateToEmrEditPatient() → /emr/patient.xhtml → saveAndNavigateToProfile()) does not call createAuditEventPatientSaved() at all, so patient updates via EMR do not trigger audit events. Additionally, navigateToOpdPatientEditFromId() neither captures before nor leads to an actual edit form (it returns /opd/patient, a profile view).
For complete audit coverage, either:
- Implement audit event logging in
saveAndNavigateToProfile()(EMR edit path), or - Route EMR edits through the same
saveSelected()method used in OPD, ensuringbeforeis consistently captured before navigation to any edit form.
🤖 Prompt for AI Agents
In `@src/main/java/com/divudi/bean/common/PatientController.java` around lines
1084 - 1087, The EMR edit flow is missing audit snapshots and secondary OPD
paths don't capture the `before` state; update either the EMR save flow or
routing so audits are consistent: ensure `createPatientJson(current)` is invoked
before edits and `createAuditEventPatientSaved()` is called when a patient is
saved from the EMR flow (methods: navigateToEmrEditPatient(),
saveAndNavigateToProfile()), or alternatively route EMR edits into the existing
OPD save flow by calling `saveSelected()` from the EMR path so `before` is set
and `createAuditEventPatientSaved()` runs; also add a `before =
createPatientJson(current)` in `navigateToOpdPatientEditFromId()` (or make it
navigate to the actual edit form) to ensure secondary OPD edits capture the
snapshot.
Signed-off-by: denuwanhendalage <shashankadenuwanb@gmail.com>
2 participants
I create createAuditEventPatientSaved function to create audit event when create new patient and update patient detials.
Summary by CodeRabbit