Ocean eReferral attachment cross-patient validation#2385
Open
D3V41 wants to merge 2 commits intohotfix/01232026from
Open
Ocean eReferral attachment cross-patient validation#2385D3V41 wants to merge 2 commits intohotfix/01232026from
D3V41 wants to merge 2 commits intohotfix/01232026from
Conversation
…bmission Adds server-side validation in ERefer2Action to confirm all selected attachments belong to the current patient before processing an Ocean eReferral. Covers all four document types (documents, labs, eforms, HRM). If validation fails, an error is shown and submission is aborted entirely. Also adds missing SecurityInfoManager privilege check to attachOceanEReferralConsult.
Reviewer's GuideImplements server-side cross-patient validation for Ocean eReferral attachments across all supported attachment types, wires a synchronous client-side validation call into the Ocean consult workflows, and adds a missing privilege check to the eReferral attachment endpoint. Sequence diagram for Ocean eReferral attachment cross-patient validationsequenceDiagram
actor User
participant Browser
participant ERefer2Action
participant SecurityInfoManager
participant DocumentAttachmentManager
participant DocumentDao
participant PatientLabRoutingDao
participant EFormDataDao
participant HRMUtil
participant HRMDocumentToDemographicDao
User->>Browser: Click send eReferral / attach Ocean attachments
Browser->>ERefer2Action: POST method=validateDocuments
ERefer2Action->>SecurityInfoManager: hasPrivilege(_con, READ)
alt Not authorized
SecurityInfoManager-->>ERefer2Action: false
ERefer2Action-->>Browser: {valid:false}
Browser->>User: Show error alert
Browser--xUser: Abort eReferral
else Authorized
SecurityInfoManager-->>ERefer2Action: true
ERefer2Action->>DocumentAttachmentManager: validateDocumentsBelongToPatient(loggedInInfo, demographicNo, documents)
DocumentAttachmentManager->>DocumentDao: findDocumentNosForDemographic(demographicNo, docIds)
DocumentDao-->>DocumentAttachmentManager: matchingDocIds
DocumentAttachmentManager->>PatientLabRoutingDao: findLabNosForDemographic(demographicNo, labIds)
PatientLabRoutingDao-->>DocumentAttachmentManager: matchingLabIds
DocumentAttachmentManager->>EFormDataDao: findFdidsForDemographic(demographicNo, eformIds)
EFormDataDao-->>DocumentAttachmentManager: matchingFdidIds
DocumentAttachmentManager->>HRMUtil: findHRMDocumentIdsForPatient(demographicNo, hrmIds)
HRMUtil->>HRMDocumentToDemographicDao: findHrmIdsForDemographic(demographicNo, hrmIds)
HRMDocumentToDemographicDao-->>HRMUtil: matchingHrmIds
HRMUtil-->>DocumentAttachmentManager: matchingHrmIds
alt All IDs match
DocumentAttachmentManager-->>ERefer2Action: true
ERefer2Action-->>Browser: {valid:true}
Browser->>ERefer2Action: POST method=attachOceanEReferralConsult or editOceanEReferralConsult
ERefer2Action->>SecurityInfoManager: hasPrivilege(_con, READ)
alt Authorized
SecurityInfoManager-->>ERefer2Action: true
ERefer2Action-->>Browser: Attachments processed
else Not authorized
SecurityInfoManager-->>ERefer2Action: false
ERefer2Action--xBrowser: Throw SecurityException
end
else Any mismatch or parse error
DocumentAttachmentManager-->>ERefer2Action: false
ERefer2Action-->>Browser: {valid:false}
Browser->>User: Show error alert
Browser--xUser: Abort eReferral
end
end
Updated class diagram for Ocean eReferral attachment validation componentsclassDiagram
class ERefer2Action {
-DocumentAttachmentManager documentAttachmentManager
-SecurityInfoManager securityInfoManager
+String execute()
+void validateDocuments()
+void attachOceanEReferralConsult()
-void writeResponse(boolean valid)
}
class SecurityInfoManager {
+boolean hasPrivilege(LoggedInInfo loggedInInfo, String objectName, int privilege, Object extra)
}
class DocumentAttachmentManager {
<<interface>>
+String convertPDFToBase64(Path renderedDocument)
+void flattenPDFFormFields(Path pdfPath)
+boolean validateDocumentsBelongToPatient(LoggedInInfo loggedInInfo, Integer demographicNo, String[] documents)
}
class DocumentAttachmentManagerImpl {
-ConsultDocsDao consultDocsDao
-EFormDocsDao eFormDocsDao
-DocumentDao documentDao
-PatientLabRoutingDao patientLabRoutingDao
-EFormDataDao eFormDataDao
-ConsultationManager consultationManager
+boolean validateDocumentsBelongToPatient(LoggedInInfo loggedInInfo, Integer demographicNo, String[] documents)
}
class DocumentDao {
+List~Integer~ findDocumentNosForDemographic(Integer demographicNo, List~Integer~ docNos)
}
class EFormDataDao {
+List~Integer~ findFdidsForDemographic(Integer demographicNo, List~Integer~ fdids)
}
class PatientLabRoutingDao {
+List~Integer~ findLabNosForDemographic(Integer demographicNo, List~Integer~ labNos)
}
class HRMUtil {
+List~Integer~ findHRMDocumentIdsForPatient(Integer demographicNo, List~Integer~ hrmIds)
}
class HRMDocumentToDemographicDao {
+List~Integer~ findHrmIdsForDemographic(Integer demographicNo, List~Integer~ hrmIds)
}
ERefer2Action --> DocumentAttachmentManager
ERefer2Action --> SecurityInfoManager
DocumentAttachmentManagerImpl ..|> DocumentAttachmentManager
DocumentAttachmentManagerImpl --> DocumentDao
DocumentAttachmentManagerImpl --> PatientLabRoutingDao
DocumentAttachmentManagerImpl --> EFormDataDao
HRMUtil --> HRMDocumentToDemographicDao
DocumentAttachmentManagerImpl --> HRMUtil
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR adds document validation functionality across multiple layers to ensure attachments belong to a specific patient demographic. New DAO query methods are introduced for Documents, EFormData, PatientLabRouting, and HRM records, bundled into a manager validation method that parses document type codes, queries the appropriate DAO, and returns ownership results. A new API endpoint exposes this validation with security checks, and frontend code integrates the validation before document submission. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant Action as ERefer2Action
participant Manager as DocumentAttachmentManager
participant DocDAO as DocumentDao
participant LabDAO as PatientLabRoutingDao
participant FormDAO as EFormDataDao
participant HRMDAO as HRMDocumentToDemographicDao
participant DB as Database
Client->>Action: POST validateDocuments(demographicNo, documents[])
Action->>Action: Check _con read privilege
alt Privilege denied
Action-->>Client: {valid: false}
else Privilege granted
Action->>Manager: validateDocumentsBelongToPatient(demographicNo, documents)
Manager->>Manager: Parse document types (D/L/E/H) and IDs
alt Type D (Documents)
Manager->>DocDAO: findDocumentNosForDemographic(demographicNo, docIds)
DocDAO->>DB: Query CtlDocument filtered by demographic
DB-->>DocDAO: Matching document IDs
DocDAO-->>Manager: Results
end
alt Type L (Lab Routing)
Manager->>LabDAO: findLabNosForDemographic(demographicNo, labIds)
LabDAO->>DB: Query PatientLabRouting filtered by demographic
DB-->>LabDAO: Matching lab IDs
LabDAO-->>Manager: Results
end
alt Type E (EForm)
Manager->>FormDAO: findFdidsForDemographic(demographicNo, fdids)
FormDAO->>DB: Query EFormData filtered by demographic
DB-->>FormDAO: Matching form IDs
FormDAO-->>Manager: Results
end
alt Type H (HRM)
Manager->>HRMDAO: findHrmIdsForDemographic(demographicNo, hrmIds)
HRMDAO->>DB: Query HRMDocumentToDemographic filtered by demographic
DB-->>HRMDAO: Matching HRM IDs
HRMDAO-->>Manager: Results
end
Manager->>Manager: Validate requested vs. found count match
alt All validations pass
Manager-->>Action: true
Action-->>Client: {valid: true}
else Any validation fails
Manager-->>Action: false
Action-->>Client: {valid: false}
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 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 docstrings
🧪 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security and data integrity of the Ocean eReferral system by implementing a robust cross-patient validation mechanism for attached documents. It prevents users from inadvertently or maliciously attaching documents belonging to other patients to an eReferral, thereby safeguarding sensitive patient information before submission. This ensures that only relevant and authorized patient data is included in referrals. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Review Summary by QodoAdd cross-patient attachment validation for Ocean eReferral submission
WalkthroughsDescription• Adds server-side validation to prevent cross-patient attachment submission in Ocean eReferral • Implements validateDocuments endpoint checking all four document types (documents, labs, eforms, HRM) • Adds missing security privilege check to attachOceanEReferralConsult method • Integrates client-side validation in JavaScript before attachment submission Diagramflowchart LR
A["User submits Ocean eReferral"] --> B["Client validates attachments"]
B --> C["validateDocuments endpoint"]
C --> D["Check document ownership"]
D --> E{All docs belong to patient?}
E -->|Yes| F["Allow submission"]
E -->|No| G["Block submission with error"]
H["Security privilege check"] --> F
File Changes1. src/main/java/ca/openosp/openo/commn/dao/DocumentDao.java
|
Code Review by Qodo
1.
|
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- In
DocumentAttachmentManager.validateDocumentsBelongToPatient, theLoggedInInfoparameter is never used; consider either leveraging it for audit/logging or removing it from the method signature (and interface) to keep the API minimal and focused. - The new
validateAttachmentsfunction usesasync: falseand throws an exception on validation failure, which can freeze the UI and may not be caught; consider switching to an asynchronous pattern (callbacks/promises) and returning gracefully instead of throwing to keep the UX responsive and predictable. - In
ERefer2Action.attachOceanEReferralConsult, throwing a bareSecurityExceptionon missing_conprivilege may translate poorly at the HTTP layer; consider returning an explicit 403-style response or structured error to the client, similar to the JSON approach used invalidateDocuments, so that the client can handle authorization failures cleanly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DocumentAttachmentManager.validateDocumentsBelongToPatient`, the `LoggedInInfo` parameter is never used; consider either leveraging it for audit/logging or removing it from the method signature (and interface) to keep the API minimal and focused.
- The new `validateAttachments` function uses `async: false` and throws an exception on validation failure, which can freeze the UI and may not be caught; consider switching to an asynchronous pattern (callbacks/promises) and returning gracefully instead of throwing to keep the UX responsive and predictable.
- In `ERefer2Action.attachOceanEReferralConsult`, throwing a bare `SecurityException` on missing `_con` privilege may translate poorly at the HTTP layer; consider returning an explicit 403-style response or structured error to the client, similar to the JSON approach used in `validateDocuments`, so that the client can handle authorization failures cleanly.
## Individual Comments
### Comment 1
<location path="src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java" line_range="418-427" />
<code_context>
+ public boolean validateDocumentsBelongToPatient(LoggedInInfo loggedInInfo, Integer demographicNo, String[] documents) {
</code_context>
<issue_to_address>
**suggestion:** The LoggedInInfo parameter is currently unused in validateDocumentsBelongToPatient.
Consider either removing this parameter from the method signature or actually using it (for logging/auditing, e.g., including the provider in any audit trail). Leaving unused parameters in a public API can confuse callers and suggest missing behavior.
Suggested implementation:
```java
@Override
public boolean validateDocumentsBelongToPatient(LoggedInInfo loggedInInfo, Integer demographicNo, String[] documents) {
if (loggedInInfo != null && logger.isDebugEnabled()) {
logger.debug(
"validateDocumentsBelongToPatient called by user {} for demographic {} and documents {}",
loggedInInfo.getLoggedInProviderNo(),
demographicNo,
java.util.Arrays.toString(documents)
);
}
List<Integer> docIds = new ArrayList<>();
```
1. Ensure this class has a `Logger` instance, for example:
`private static final Logger logger = LoggerFactory.getLogger(DocumentAttachmentManagerImpl.class);`
and the corresponding imports:
`import org.slf4j.Logger;`
`import org.slf4j.LoggerFactory;`
2. If `LoggedInInfo` exposes a different accessor than `getLoggedInProviderNo()` (e.g., `getLoggedInUser()`, `getProviderNo()`, etc.), adjust the logging call to use the appropriate method.
3. If you prefer to audit only failures instead of all invocations, you can move this logging block to the branch where the method returns `false`, passing along the `loggedInInfo` details there.
</issue_to_address>
### Comment 2
<location path="src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java" line_range="440-442" />
<code_context>
+ }
+ }
+
+ if (!docIds.isEmpty()) {
+ List<Integer> found = documentDao.findDocumentNosForDemographic(demographicNo, docIds);
+ if (found.size() != docIds.size()) return false;
+ }
+ if (!labIds.isEmpty()) {
</code_context>
<issue_to_address>
**issue:** Size comparison may misbehave if the input list contains duplicate IDs.
If `documents` can contain the same typed ID more than once (e.g., `D42` twice), `docIds` may have duplicates while the query returns each `documentNo` only once. Then `found.size()` < `docIds.size()` and validation fails even though all documents belong to the patient. If duplicates are valid input, normalize to a `Set<Integer>` for the query and compare membership rather than relying on list sizes.
</issue_to_address>
### Comment 3
<location path="src/main/webapp/js/custom/ocean/conreq.js" line_range="39-45" />
<code_context>
+function validateAttachments(demographicNo, docs) {
+ let valid = false;
+ jQuery.ajax({
+ type: 'POST',
+ async: false,
+ url: document.getElementById("contextPath").value + '/oscarEncounter/eRefer.do',
+ data: "demographicNo=" + demographicNo + "&method=validateDocuments&" + jQuery.param({documents: docs}, true),
+ dataType: 'json',
+ success: function (response) { valid = response.valid; }
+ });
+ if (!valid) {
</code_context>
<issue_to_address>
**suggestion (performance):** Using synchronous AJAX (async: false) can freeze the UI and is generally discouraged.
This call blocks the browser until it finishes, leading to UI freezes and poor UX, especially with high latency or slow backends. Prefer an asynchronous flow (e.g., return a Promise from `validateAttachments` and have `eRefer`/`attachOceanAttachments` await it before proceeding with the main POST).
Suggested implementation:
```javascript
function validateAttachments(demographicNo, docs) {
return jQuery.ajax({
type: 'POST',
url: document.getElementById("contextPath").value + '/oscarEncounter/eRefer.do',
data: "demographicNo=" + demographicNo + "&method=validateDocuments&" + jQuery.param({ documents: docs }, true),
dataType: 'json'
}).then(function (response) {
var valid = response && response.valid === true;
if (!valid) {
alert("Something went wrong. Please close this window and restart the process from the beginning.");
}
return valid;
}).catch(function () {
alert("Something went wrong. Please close this window and restart the process from the beginning.");
return false;
});
}
```
To fully implement the asynchronous flow and avoid UI blocking, you should also:
1. Update all callers of `validateAttachments` (e.g., `eRefer`, `attachOceanAttachments`) to handle the returned Promise instead of a boolean. For example:
- If the call is currently `if (!validateAttachments(demographicNo, docs)) { return; }`, change it to:
```js
validateAttachments(demographicNo, docs).then(function (valid) {
if (!valid) {
return;
}
// continue with main POST / eRefer logic here
});
```
- Or, if the surrounding function is `async`, use:
```js
const valid = await validateAttachments(demographicNo, docs);
if (!valid) {
return;
}
// continue with main POST / eRefer logic here
```
2. If any of these functions (`eRefer`, `attachOceanAttachments`, etc.) are used as event handlers and rely on `return false;` to prevent default behaviour, convert them to use `event.preventDefault()` and `event.stopPropagation()` explicitly, then perform the asynchronous flow described above.
</issue_to_address>
### Comment 4
<location path="src/main/webapp/js/custom/ocean/conreq.js" line_range="60-61" />
<code_context>
let documents = getDocuments(event);
- let data = demographicNo + "&" + documents + "&method=attachOceanEReferralConsult";
+ let docs = documents.replace('documents=', '').split('|').filter(Boolean);
+ if (!validateAttachments(document.getElementById("demographicNo").value, docs)) {
+ throw new Error("Document validation failed");
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Throwing an Error on validation failure may be heavy-handed and hard to handle in this UI flow.
Because the alert is already shown in `validateAttachments`, throwing a raw `Error` here can lead to uncaught exceptions in the console and makes the flow harder to manage for callers. Consider returning early from `eRefer` / `attachOceanAttachments` when `validateAttachments` fails (e.g., `if (!validateAttachments(...)) return;`) instead of throwing, to keep control flow simpler and avoid unintended side effects.
Suggested implementation:
```javascript
function eRefer(event) {
let demographicNo = document.getElementById("demographicNo").serialize();
let documents = getDocuments(event);
let docs = documents.replace('documents=', '').split('|').filter(Boolean);
if (!validateAttachments(document.getElementById("demographicNo").value, docs)) {
// Validation failed; alert already shown in validateAttachments, so just stop here.
return;
}
let data = demographicNo + "&" + documents + "&method=attachOceanEReferralConsult";
```
```javascript
function validateAttachments(demographicNo, docs) {
let valid = false;
jQuery.ajax({
type: 'POST',
async: false,
url: document.getElementById("contextPath").value + '/oscarEncounter/eRefer.do',
data: "demographicNo=" + demographicNo + "&method=validateDocuments&" + jQuery.param({documents: docs}, true),
dataType: 'json',
success: function (response) { valid = response.valid; }
});
return valid;
```
1. Ensure that `data` (restored in `eRefer`) is still used later in the function as before the PR (e.g., passed to an AJAX call or form submission). If that code was removed or changed in the PR, it should be updated to use the restored `data` variable.
2. If there are other call sites of `validateAttachments` in this file, they can now rely on its boolean return value instead of ignoring it; adjust them if needed.
</issue_to_address>
### Comment 5
<location path="src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java" line_range="72-73" />
<code_context>
+ writeResponse(true);
+ return;
+ }
+ boolean valid = documentAttachmentManager.validateDocumentsBelongToPatient(
+ loggedInInfo, Integer.parseInt(demographicNo), documents);
+ writeResponse(valid);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Parsing demographicNo without error handling could lead to a 500 on bad input.
If `demographicNo` is not a valid integer, `Integer.parseInt` will throw `NumberFormatException` and return a 500. For a validation endpoint, it would be better to catch this and treat it as invalid input (e.g. `valid=false` or a 4xx response) instead of letting the exception propagate.
</issue_to_address>
### Comment 6
<location path="src/main/webapp/js/custom/ocean/conreq.js" line_range="56" />
<code_context>
// This code will be executed when a user sends a new e-refer to OceanMD from the consult request form.
// It involves saving attachments in the 'EreferAttachment' table by sending the selected attachments (documents)
// and demographic information to the EreferAction.java class.
function eRefer(event) {
let demographicNo = document.getElementById("demographicNo").serialize();
let documents = getDocuments(event);
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new validation logic to avoid using exceptions for normal control flow, de-duplicate document parsing, and standardize how `demographicNo` is obtained to simplify the code.
You can reduce the added complexity without changing behaviour by:
1. Avoiding exceptions as control flow.
2. Removing duplicated document parsing.
3. Normalizing how `demographicNo` is derived.
### 1. Remove `throw` for normal control flow
You already have a boolean return from `validateAttachments`. The `throw` is unnecessary and adds control-flow complexity. Callers can just early-return.
```js
function eRefer(event) {
const demographicNoSerialized = document.getElementById("demographicNo").serialize();
const demographicNo = document.getElementById("demographicNo").value;
const documents = getDocuments(event);
const docs = extractDocArray(documents);
if (!validateAttachments(demographicNo, docs)) {
return; // abort quietly after alert in validateAttachments
}
jQuery.ajax({
type: 'POST',
url: document.getElementById("contextPath").value + '/oscarEncounter/eRefer.do',
data: demographicNoSerialized + "&" + documents + "&method=attachOceanEReferralConsult",
success: function (response) { console.log(response); }
});
}
function attachOceanAttachments() {
const demographicNoSerialized = document.getElementById("demographicNo").serialize();
const demographicNo = document.getElementById("demographicNo").value;
const requestId = document.getElementById("requestId").serialize();
const documents = getDocuments();
const docs = extractDocArray(documents);
if (!validateAttachments(demographicNo, docs)) {
return;
}
jQuery.ajax({
type: 'POST',
url: document.getElementById("contextPath").value + '/oscarEncounter/eRefer.do',
data: demographicNoSerialized + "&" + requestId + "&" + documents + "&method=editOceanEReferralConsult",
success: function (response) { console.log(response); }
});
}
```
### 2. Extract document parsing to a helper
This removes the duplicated string manipulation and keeps all existing behaviour:
```js
function extractDocArray(documentsParam) {
return documentsParam.replace('documents=', '').split('|').filter(Boolean);
}
```
Use it in both places (as shown above) instead of duplicating `replace/split/filter`.
### 3. Normalize `demographicNo` usage
Right now you pass `document.getElementById("demographicNo").value` into `validateAttachments` and also use `.serialize()` elsewhere. You can read both forms once at the top to reduce cognitive load:
```js
function eRefer(event) {
const demographicEl = document.getElementById("demographicNo");
const demographicNo = demographicEl.value;
const demographicNoSerialized = jQuery(demographicEl).serialize();
// ... same as in the first snippet ...
}
```
(Using `jQuery(element).serialize()` avoids calling `.serialize()` on a raw DOM element, which isn’t a standard DOM API.)
These small refactors keep the synchronous validation and all functional behaviour intact while simplifying the control flow and reducing duplication.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java
Show resolved
Hide resolved
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to validate that documents selected for e-referrals belong to the specified patient demographic. This involves adding new DAO methods to retrieve document IDs by demographic for various types (general documents, lab results, e-forms, and HRM documents). A new DocumentAttachmentManager method orchestrates this validation, which is exposed via a new validateDocuments endpoint in ERefer2Action.java. Client-side JavaScript in conreq.js now calls this endpoint to perform validation before allowing document attachments. Key feedback from the review includes improving security checks in ERefer2Action by making them patient-specific and using the correct privilege level for write operations, refactoring client-side synchronous AJAX calls to be asynchronous to prevent UI blocking, ensuring consistent retrieval of demographicNo in JavaScript, adding clarifying comments for @SuppressWarnings("unchecked") annotations in DAO implementations, and providing more informative user-facing error messages for validation failures.
src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 12 files
Confidence score: 3/5
- There is meaningful regression risk from concrete runtime/validation issues:
Integer.parseInt(demographicNo)insrc/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.javacan throw on non-numeric input, and unsupported attachment prefixes insrc/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.javaare currently ignored instead of rejected. - Attachment validation logic in
src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.javaandsrc/main/java/ca/openosp/openo/commn/dao/PatientLabRoutingDaoImpl.javamay produce false negatives (duplicate IDs / non-distinctlabNo), which can incorrectly fail valid attachment flows. async: falseinsrc/main/webapp/js/custom/ocean/conreq.jscan freeze the UI during network calls, and throwing a new Error there may surface uncaught browser exceptions; these are user-facing but likely fixable without large redesign.- Pay close attention to
src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java,src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java,src/main/java/ca/openosp/openo/commn/dao/PatientLabRoutingDaoImpl.java, andsrc/main/webapp/js/custom/ocean/conreq.js- input parsing, attachment validation correctness, and frontend request behavior need careful verification.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/webapp/js/custom/ocean/conreq.js">
<violation number="1" location="src/main/webapp/js/custom/ocean/conreq.js:41">
P2: Avoid synchronous AJAX (`async: false`) here. It blocks the UI thread during network calls and can make the consultation form appear frozen.</violation>
<violation number="2" location="src/main/webapp/js/custom/ocean/conreq.js:61">
P2: `throw new Error("Document validation failed")` will produce an uncaught exception in the browser console since no caller wraps this in a try/catch. The alert is already shown inside `validateAttachments`, so a simple `return;` is sufficient to halt execution cleanly without polluting the error log.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java">
<violation number="1" location="src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java:429">
P1: Reject unsupported attachment type prefixes instead of silently ignoring them.</violation>
<violation number="2" location="src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java:442">
P2: Size comparison will produce a false negative if the input contains duplicate IDs (e.g. `D42` appears twice). `docIds` will contain the duplicate, but the DB query returns each matching ID only once. `found.size() != docIds.size()` then fails even though every document belongs to the patient. Convert `docIds` (and the other ID lists) to a `Set<Integer>` before querying, or deduplicate before comparison.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java">
<violation number="1" location="src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java:72">
P1: `Integer.parseInt(demographicNo)` will throw an unhandled `NumberFormatException` if the request parameter is non-numeric (e.g. `demographicNo=abc`). Wrap in a try-catch or validate the format before parsing, and return `{"valid":false}` on failure.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/commn/dao/PatientLabRoutingDaoImpl.java">
<violation number="1" location="src/main/java/ca/openosp/openo/commn/dao/PatientLabRoutingDaoImpl.java:376">
P2: Use `DISTINCT` when selecting `labNo`; duplicate routing rows can otherwise cause false negatives in attachment validation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java
Outdated
Show resolved
Hide resolved
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/ca/openosp/openo/commn/dao/DocumentDaoImpl.java (1)
268-283:⚠️ Potential issue | 🟡 MinorFix stale JavaDoc and handle empty list case.
Two issues:
The existing JavaDoc (lines 268-270) references
@param demoNowhich doesn't match the new method signature. This appears to be leftover documentation from a different method.Same empty list concern—
IN :docNoswith an empty list can cause JPA provider-specific behavior.🛡️ Suggested fix
- /** - * Finds all documents for the specified demographic id - * - * `@param` demoNo - */ + /** + * Returns the subset of provided document numbers that are linked to the specified patient demographic. + * Excludes documents with deleted status ('D'). + * + * `@param` demographicNo Integer the patient's demographic number + * `@param` docNos List<Integer> the list of document numbers to validate + * `@return` List<Integer> document numbers from the input list that belong to the patient + */ `@SuppressWarnings`("unchecked") `@Override` public List<Integer> findDocumentNosForDemographic(Integer demographicNo, List<Integer> docNos) { + if (docNos == null || docNos.isEmpty()) { + return new ArrayList<>(); + } Query query = entityManager.createQuery( "SELECT c.id.documentNo FROM CtlDocument c " + "WHERE c.id.module = 'demographic' AND c.id.moduleId = :demographicNo " + "AND c.status != 'D' AND c.id.documentNo IN :docNos"); query.setParameter("demographicNo", demographicNo); query.setParameter("docNos", docNos); return query.getResultList(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/dao/DocumentDaoImpl.java` around lines 268 - 283, Update the JavaDoc for findDocumentNosForDemographic to match the actual parameters (replace or remove the incorrect `@param` demoNo and document the demographicNo and docNos parameters), and guard against an empty docNos before building/executing the query in findDocumentNosForDemographic: if docNos is null or empty return Collections.emptyList() immediately, otherwise proceed to create the Query (the one using entityManager.createQuery with "IN :docNos") and set parameters as before; keep the method signature and return type unchanged.src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java (1)
114-122:⚠️ Potential issue | 🟠 MajorMissing security privilege check in
editOceanEReferralConsult().This method modifies consult request attachments but lacks a
SecurityInfoManager.hasPrivilege()check, which is required per coding guidelines. Since this PR adds security checks toattachOceanEReferralConsult()andvalidateDocuments(), consider adding the same check here for consistency and security.🛡️ Proposed fix
public void editOceanEReferralConsult() { LoggedInInfo loggedInInfo = LoggedInInfo.getLoggedInInfoFromSession(request); + String demographicNo = StringUtils.isNullOrEmpty(request.getParameter("demographicNo")) ? "" : request.getParameter("demographicNo"); + Integer demoNo = demographicNo.isEmpty() ? null : Integer.parseInt(demographicNo); + if (!securityInfoManager.hasPrivilege(loggedInInfo, "_con", SecurityInfoManager.WRITE, demoNo)) { + throw new SecurityException("missing required sec object (_con)"); + } String providerNo = loggedInInfo.getLoggedInProviderNo(); - String demographicNo = StringUtils.isNullOrEmpty(request.getParameter("demographicNo")) ? "" : request.getParameter("demographicNo"); String requestId = StringUtils.isNullOrEmpty(request.getParameter("requestId")) ? "" : request.getParameter("requestId");As per coding guidelines: "ALL actions MUST include SecurityInfoManager.hasPrivilege() checks before performing operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java` around lines 114 - 122, The editOceanEReferralConsult method lacks the required SecurityInfoManager.hasPrivilege() check; add a privilege check (same as in attachOceanEReferralConsult() and validateDocuments()) near the start of editOceanEReferralConsult (after obtaining LoggedInInfo) and return/stop processing if the check fails. Use the same privilege constant/logic used by attachOceanEReferralConsult()/validateDocuments() and ensure the method returns immediately on unauthorized access to prevent further edits to consult request attachments.
🧹 Nitpick comments (9)
src/main/java/ca/openosp/openo/commn/dao/PatientLabRoutingDao.java (1)
79-80: Add JavaDoc documentation for the new DAO method.The method lacks required JavaDoc documentation. As per coding guidelines for
src/main/java/**/*.java, all public methods must have comprehensive JavaDoc with@paramand@returntags. Additionally, forsrc/main/java/**/commn/dao/**/*.java, healthcare domain context should be documented.📝 Suggested JavaDoc
+ /** + * Returns the subset of provided lab numbers that are associated with the specified patient demographic. + * Used for validating that lab result attachments belong to the correct patient. + * + * `@param` demographicNo Integer the patient's demographic number + * `@param` labNos List<Integer> the list of lab numbers to validate + * `@return` List<Integer> lab numbers from the input list that belong to the patient + */ public List<Integer> findLabNosForDemographic(Integer demographicNo, List<Integer> labNos);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/dao/PatientLabRoutingDao.java` around lines 79 - 80, Add comprehensive JavaDoc to the public DAO method findLabNosForDemographic in PatientLabRoutingDao: document the healthcare context/purpose of the method (what "demographicNo" and "labNos" represent in clinical terms), include `@param` tags for demographicNo and labNos describing expected values and null/empty handling, and add an `@return` tag describing the returned List<Integer> (what lab numbers represent and when the list may be empty). Ensure the JavaDoc follows project style for src/main/java/**/*.java and includes domain-specific clarity for src/main/java/**/commn/dao/**/*.java.src/main/java/ca/openosp/openo/commn/dao/DocumentDao.java (1)
118-119: Add JavaDoc documentation for the new DAO method.The method lacks required JavaDoc documentation. As per coding guidelines, all public methods must have comprehensive JavaDoc with
@paramand@returntags documenting exact types.📝 Suggested JavaDoc
+ /** + * Returns the subset of provided document numbers that are linked to the specified patient demographic. + * Excludes documents with deleted status ('D'). Used for cross-patient attachment validation. + * + * `@param` demographicNo Integer the patient's demographic number + * `@param` docNos List<Integer> the list of document numbers to validate + * `@return` List<Integer> document numbers from the input list that belong to the patient + */ public List<Integer> findDocumentNosForDemographic(Integer demographicNo, List<Integer> docNos);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/dao/DocumentDao.java` around lines 118 - 119, The public DAO method findDocumentNosForDemographic(Integer demographicNo, List<Integer> docNos) is missing required JavaDoc; add a JavaDoc block above the method that describes its purpose and includes `@param` tags for demographicNo (Integer) and docNos (List<Integer>) and an `@return` tag describing the returned List<Integer> of document numbers (or empty list) and any nullability/exception behavior to meet coding guidelines.src/main/java/ca/openosp/openo/commn/dao/EFormDataDao.java (1)
106-107: Add JavaDoc documentation for the new DAO method.The method lacks required JavaDoc documentation per coding guidelines.
📝 Suggested JavaDoc
+ /** + * Returns the subset of provided eForm data IDs (fdids) that are associated with the specified patient demographic. + * Used for validating that eForm attachments belong to the correct patient. + * + * `@param` demographicNo Integer the patient's demographic number + * `@param` fdids List<Integer> the list of eForm data IDs to validate + * `@return` List<Integer> fdids from the input list that belong to the patient + */ public List<Integer> findFdidsForDemographic(Integer demographicNo, List<Integer> fdids);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/dao/EFormDataDao.java` around lines 106 - 107, Add JavaDoc for the DAO method findFdidsForDemographic(Integer demographicNo, List<Integer> fdids): document the purpose (filter/return fdid values that match the given demographic), describe parameters (`@param` demographicNo the demographic identifier, `@param` fdids list of candidate fdid integers), describe the return value (`@return` list of matching fdid integers, empty list if none), and note any null/validation behavior or exceptions (e.g., how null demographicNo or null/empty fdids is handled). Place the JavaDoc immediately above the method declaration in EFormDataDao.src/main/webapp/js/custom/ocean/conreq.js (3)
59-62: Consider returning early instead of throwing an error.Throwing an
Errorwill stop execution but may produce unhandled exception warnings in the console. A simplereturnwould achieve the same effect more cleanly since the AJAX call is below this check.♻️ Cleaner flow control
let docs = documents.replace('documents=', '').split('|').filter(Boolean); if (!validateAttachments(document.getElementById("demographicNo").value, docs)) { - throw new Error("Document validation failed"); + return; // Validation failed - user was already alerted }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/js/custom/ocean/conreq.js` around lines 59 - 62, Replace the thrown exception with an early return to avoid unhandled console errors: in conreq.js where you build docs and call validateAttachments (using documents.replace(...) and validateAttachments(document.getElementById("demographicNo").value, docs)), change the block that currently does throw new Error("Document validation failed") to simply return (or return false) so the function exits cleanly and prevents the AJAX call from running when validation fails.
101-104: Same suggestion: return early instead of throwing.Apply the same refactor here for consistency.
♻️ Cleaner flow control
let docs = documents.replace('documents=', '').split('|').filter(Boolean); if (!validateAttachments(document.getElementById("demographicNo").value, docs)) { - throw new Error("Document validation failed"); + return; // Validation failed - user was already alerted }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/js/custom/ocean/conreq.js` around lines 101 - 104, Replace the throw in this validation block with an early return to match the other refactor: when validateAttachments(document.getElementById("demographicNo").value, docs) is false, do an early return (e.g. return false or return) instead of throwing new Error("Document validation failed"); update the caller if needed to handle the returned failure. Target the code around the validateAttachments call and the docs parsing (variables: documents, docs, validateAttachments, demographicNo).
37-51: Synchronous AJAX (async: false) is deprecated.While the synchronous behavior is intentional to block submission until validation completes,
async: falseis deprecated and blocks the browser's main thread. Consider refactoring to use async/await with Promises for better UX.♻️ Async/await alternative
async function validateAttachments(demographicNo, docs) { try { const response = await jQuery.ajax({ type: 'POST', url: document.getElementById("contextPath").value + '/oscarEncounter/eRefer.do', data: "demographicNo=" + demographicNo + "&method=validateDocuments&" + jQuery.param({documents: docs}, true), dataType: 'json' }); if (!response.valid) { alert("Something went wrong. Please close this window and restart the process from the beginning."); } return response.valid; } catch (e) { alert("Something went wrong. Please close this window and restart the process from the beginning."); return false; } }Callers would need to be updated to
await validateAttachments(...).As per coding guidelines: "Always use async/await for promises in TypeScript and JavaScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/js/custom/ocean/conreq.js` around lines 37 - 51, The validateAttachments function currently uses deprecated synchronous AJAX (async: false); change validateAttachments to an async function that awaits the jQuery.ajax Promise (remove async: false), catch errors, show the same alert on failure, and return the boolean response.valid (or false on error); then update every caller of validateAttachments to await the call or handle the returned Promise (make caller functions async where needed) so the code no longer blocks the main thread.src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java (1)
80-89: Pass the actualdemographicNotohasPrivilege()for patient-level authorization.Similar to
validateDocuments(), the security check passesnullinstead of the patient's demographic number. Consider parsingdemographicNofirst and using it in the privilege check.🛡️ Proposed fix
public void attachOceanEReferralConsult() { LoggedInInfo loggedInInfo = LoggedInInfo.getLoggedInInfoFromSession(request); - if (!securityInfoManager.hasPrivilege(loggedInInfo, "_con", SecurityInfoManager.READ, null)) { - throw new SecurityException("missing required sec object (_con)"); - } String demographicNo = StringUtils.isNullOrEmpty(request.getParameter("demographicNo")) ? "" : request.getParameter("demographicNo"); + Integer demoNo = demographicNo.isEmpty() ? null : Integer.parseInt(demographicNo); + if (!securityInfoManager.hasPrivilege(loggedInInfo, "_con", SecurityInfoManager.READ, demoNo)) { + throw new SecurityException("missing required sec object (_con)"); + } String documents = StringUtils.isNullOrEmpty(request.getParameter("documents")) ? "" : request.getParameter("documents");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java` around lines 80 - 89, The privilege check in attachOceanEReferralConsult() uses null for the patient identifier, so it doesn't perform patient-level authorization; move the extraction/parsing of the demographicNo request parameter before the security check (like validateDocuments() does) and pass that demographicNo into securityInfoManager.hasPrivilege(loggedInInfo, "_con", SecurityInfoManager.READ, demographicNo) so the check enforces patient-level access.src/main/java/ca/openosp/openo/hospitalReportManager/dao/HRMDocumentToDemographicDao.java (1)
152-160: Add JavaDoc documentation for this public method.Per coding guidelines, all public methods must have comprehensive JavaDoc documentation including
@paramand@returntags.📝 Proposed JavaDoc
+ /** + * Finds HRM document IDs that belong to a specific patient demographic. + * + * `@param` demographicNo Integer the patient demographic number + * `@param` hrmIds List<Integer> the list of HRM document IDs to validate + * `@return` List<Integer> list of HRM document IDs that belong to the specified patient + * `@since` 2026-03-25 + */ `@SuppressWarnings`("unchecked") public List<Integer> findHrmIdsForDemographic(Integer demographicNo, List<Integer> hrmIds) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/hospitalReportManager/dao/HRMDocumentToDemographicDao.java` around lines 152 - 160, Add comprehensive JavaDoc to the public method findHrmIdsForDemographic in HRMDocumentToDemographicDao: document the method purpose, describe parameters `demographicNo` and `hrmIds` with `@param` tags, specify the return value with `@return` (List of HRM document IDs matching the demographic and input IDs), and note any relevant behavior (e.g., null/empty handling or that it queries the database). Place the JavaDoc immediately above the method signature and keep it consistent with project style.src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java (1)
417-458: Add JavaDoc and consider logging unknown document types.The method is missing required JavaDoc documentation. Additionally, unknown document type codes (not D/L/E/H) are silently ignored, which could mask malformed input.
📝 Proposed improvements
+ /** + * Validates that the specified documents belong to the given patient demographic. + * Used to prevent cross-patient attachment vulnerabilities in Ocean eReferrals. + * + * `@param` loggedInInfo LoggedInInfo the logged-in user context + * `@param` demographicNo Integer the patient demographic number + * `@param` documents String[] array of document identifiers in format [type][id] (e.g., "D123", "L456") + * `@return` boolean true if all documents belong to the patient, false otherwise + * `@since` 2026-03-25 + */ `@Override` public boolean validateDocumentsBelongToPatient(LoggedInInfo loggedInInfo, Integer demographicNo, String[] documents) {Consider logging a warning for unknown type codes to aid debugging:
switch (type) { case 'D': docIds.add(id); break; case 'L': labIds.add(id); break; case 'E': eformIds.add(id); break; case 'H': hrmIds.add(id); break; + default: + MiscUtils.getLogger().warn("Unknown document type code: " + type); + break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java` around lines 417 - 458, Add JavaDoc for validateDocumentsBelongToPatient describing parameters, return value and behavior; while parsing the documents array, detect and log any unknown type codes (anything other than 'D','L','E','H') instead of silently ignoring them, and treat malformed entries consistently (keep the NumberFormatException behavior). Use the class-level logger (create private static final Logger logger if none exists) and emit a warning that includes the offending doc string and demographicNo so malformed or unexpected type codes are visible in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/ca/openosp/openo/commn/dao/EFormDataDaoImpl.java`:
- Around line 601-611: The method findFdidsForDemographic in EFormDataDaoImpl
can fail when fdids is empty because some JPA providers throw on "IN :fdids";
add a defensive early return: if fdids is null or fdids.isEmpty() return
Collections.emptyList() before creating the query (or alternatively skip the IN
clause when empty). This change prevents provider-specific runtime exceptions
and keeps the existing query/parameters unchanged for non-empty lists.
In `@src/main/java/ca/openosp/openo/commn/dao/PatientLabRoutingDaoImpl.java`:
- Around line 372-381: The findLabNosForDemographic method in
PatientLabRoutingDaoImpl should defensively handle a null or empty labNos to
avoid JPA provider issues with "IN :labNos"; add an early return when labNos is
null or labNos.isEmpty() (return Collections.emptyList() or new ArrayList<>())
before building the Query with entityManager, so only non-empty lists are passed
into the "r.labNo IN :labNos" parameter.
In
`@src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java`:
- Around line 59-75: The security check currently calls
securityInfoManager.hasPrivilege(..., null) and then later parses demographicNo
with Integer.parseInt(demographicNo) which can throw NumberFormatException;
update validateDocuments to pass the demographic scope to hasPrivilege (use the
demographic identifier rather than null — e.g., pass demographicNo or
Integer.valueOf(demographicNo) consistent with hasPrivilege's expected type) and
guard/parse demographicNo safely before calling hasPrivilege: validate that
demographicNo is a valid integer (or wrap Integer.parseInt(demographicNo) in a
try/catch), return an appropriate response (writeResponse(false)) on parse
error, and only then call
documentAttachmentManager.validateDocumentsBelongToPatient with the parsed id;
reference: validateDocuments, securityInfoManager.hasPrivilege(...),
Integer.parseInt(demographicNo),
documentAttachmentManager.validateDocumentsBelongToPatient.
---
Outside diff comments:
In `@src/main/java/ca/openosp/openo/commn/dao/DocumentDaoImpl.java`:
- Around line 268-283: Update the JavaDoc for findDocumentNosForDemographic to
match the actual parameters (replace or remove the incorrect `@param` demoNo and
document the demographicNo and docNos parameters), and guard against an empty
docNos before building/executing the query in findDocumentNosForDemographic: if
docNos is null or empty return Collections.emptyList() immediately, otherwise
proceed to create the Query (the one using entityManager.createQuery with "IN
:docNos") and set parameters as before; keep the method signature and return
type unchanged.
In
`@src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java`:
- Around line 114-122: The editOceanEReferralConsult method lacks the required
SecurityInfoManager.hasPrivilege() check; add a privilege check (same as in
attachOceanEReferralConsult() and validateDocuments()) near the start of
editOceanEReferralConsult (after obtaining LoggedInInfo) and return/stop
processing if the check fails. Use the same privilege constant/logic used by
attachOceanEReferralConsult()/validateDocuments() and ensure the method returns
immediately on unauthorized access to prevent further edits to consult request
attachments.
---
Nitpick comments:
In `@src/main/java/ca/openosp/openo/commn/dao/DocumentDao.java`:
- Around line 118-119: The public DAO method
findDocumentNosForDemographic(Integer demographicNo, List<Integer> docNos) is
missing required JavaDoc; add a JavaDoc block above the method that describes
its purpose and includes `@param` tags for demographicNo (Integer) and docNos
(List<Integer>) and an `@return` tag describing the returned List<Integer> of
document numbers (or empty list) and any nullability/exception behavior to meet
coding guidelines.
In `@src/main/java/ca/openosp/openo/commn/dao/EFormDataDao.java`:
- Around line 106-107: Add JavaDoc for the DAO method
findFdidsForDemographic(Integer demographicNo, List<Integer> fdids): document
the purpose (filter/return fdid values that match the given demographic),
describe parameters (`@param` demographicNo the demographic identifier, `@param`
fdids list of candidate fdid integers), describe the return value (`@return` list
of matching fdid integers, empty list if none), and note any null/validation
behavior or exceptions (e.g., how null demographicNo or null/empty fdids is
handled). Place the JavaDoc immediately above the method declaration in
EFormDataDao.
In `@src/main/java/ca/openosp/openo/commn/dao/PatientLabRoutingDao.java`:
- Around line 79-80: Add comprehensive JavaDoc to the public DAO method
findLabNosForDemographic in PatientLabRoutingDao: document the healthcare
context/purpose of the method (what "demographicNo" and "labNos" represent in
clinical terms), include `@param` tags for demographicNo and labNos describing
expected values and null/empty handling, and add an `@return` tag describing the
returned List<Integer> (what lab numbers represent and when the list may be
empty). Ensure the JavaDoc follows project style for src/main/java/**/*.java and
includes domain-specific clarity for src/main/java/**/commn/dao/**/*.java.
In
`@src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java`:
- Around line 417-458: Add JavaDoc for validateDocumentsBelongToPatient
describing parameters, return value and behavior; while parsing the documents
array, detect and log any unknown type codes (anything other than
'D','L','E','H') instead of silently ignoring them, and treat malformed entries
consistently (keep the NumberFormatException behavior). Use the class-level
logger (create private static final Logger logger if none exists) and emit a
warning that includes the offending doc string and demographicNo so malformed or
unexpected type codes are visible in logs.
In
`@src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java`:
- Around line 80-89: The privilege check in attachOceanEReferralConsult() uses
null for the patient identifier, so it doesn't perform patient-level
authorization; move the extraction/parsing of the demographicNo request
parameter before the security check (like validateDocuments() does) and pass
that demographicNo into securityInfoManager.hasPrivilege(loggedInInfo, "_con",
SecurityInfoManager.READ, demographicNo) so the check enforces patient-level
access.
In
`@src/main/java/ca/openosp/openo/hospitalReportManager/dao/HRMDocumentToDemographicDao.java`:
- Around line 152-160: Add comprehensive JavaDoc to the public method
findHrmIdsForDemographic in HRMDocumentToDemographicDao: document the method
purpose, describe parameters `demographicNo` and `hrmIds` with `@param` tags,
specify the return value with `@return` (List of HRM document IDs matching the
demographic and input IDs), and note any relevant behavior (e.g., null/empty
handling or that it queries the database). Place the JavaDoc immediately above
the method signature and keep it consistent with project style.
In `@src/main/webapp/js/custom/ocean/conreq.js`:
- Around line 59-62: Replace the thrown exception with an early return to avoid
unhandled console errors: in conreq.js where you build docs and call
validateAttachments (using documents.replace(...) and
validateAttachments(document.getElementById("demographicNo").value, docs)),
change the block that currently does throw new Error("Document validation
failed") to simply return (or return false) so the function exits cleanly and
prevents the AJAX call from running when validation fails.
- Around line 101-104: Replace the throw in this validation block with an early
return to match the other refactor: when
validateAttachments(document.getElementById("demographicNo").value, docs) is
false, do an early return (e.g. return false or return) instead of throwing new
Error("Document validation failed"); update the caller if needed to handle the
returned failure. Target the code around the validateAttachments call and the
docs parsing (variables: documents, docs, validateAttachments, demographicNo).
- Around line 37-51: The validateAttachments function currently uses deprecated
synchronous AJAX (async: false); change validateAttachments to an async function
that awaits the jQuery.ajax Promise (remove async: false), catch errors, show
the same alert on failure, and return the boolean response.valid (or false on
error); then update every caller of validateAttachments to await the call or
handle the returned Promise (make caller functions async where needed) so the
code no longer blocks the main thread.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd403c93-64f7-4128-8754-87000f06ab81
📒 Files selected for processing (12)
src/main/java/ca/openosp/openo/commn/dao/DocumentDao.javasrc/main/java/ca/openosp/openo/commn/dao/DocumentDaoImpl.javasrc/main/java/ca/openosp/openo/commn/dao/EFormDataDao.javasrc/main/java/ca/openosp/openo/commn/dao/EFormDataDaoImpl.javasrc/main/java/ca/openosp/openo/commn/dao/PatientLabRoutingDao.javasrc/main/java/ca/openosp/openo/commn/dao/PatientLabRoutingDaoImpl.javasrc/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManager.javasrc/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.javasrc/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.javasrc/main/java/ca/openosp/openo/hospitalReportManager/HRMUtil.javasrc/main/java/ca/openosp/openo/hospitalReportManager/dao/HRMDocumentToDemographicDao.javasrc/main/webapp/js/custom/ocean/conreq.js
src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java
Show resolved
Hide resolved
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
src/main/java/ca/openosp/openo/hospitalReportManager/dao/HRMDocumentToDemographicDao.java
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/encounter/oceanEReferal/pageUtil/ERefer2Action.java
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/documentManager/DocumentAttachmentManagerImpl.java
Show resolved
Hide resolved
- Fix demographicNo/requestId serialization in conreq.js (use jQuery .serialize() for POST data, .val() for raw value) - Add empty-list guard to findLabNosForDemographic, findFdidsForDemographic, findDocumentNosForDemographic, findHrmIdsForDemographic to prevent JPA IN-clause exceptions - Add JavaDoc to HRMDocumentToDemographicDao.findHrmIdsForDemographic
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a
validateDocumentsendpoint inERefer2Actionthat confirms all selected attachments belong to the specified patient before the referral is submittedCovers all four attachment types: documents (
CtlDocument), labs (PatientLabRouting), eforms (EFormData), and HRM documents (HRMDocumentToDemographic)If validation fails, an error message is shown and execution is halted entirely - no tab is opened, no data is sent
Adds missing
SecurityInfoManager.hasPrivilegecheck toattachOceanEReferralConsultSummary by Sourcery
Add server- and client-side validation to ensure Ocean eReferral attachments belong to the selected patient and enforce consult access privileges when sending referrals.
New Features:
Bug Fixes:
Enhancements:
Summary by cubic
Adds a server-validated preflight check to ensure all Ocean eReferral attachments belong to the selected patient before submit/edit. Also hardens DAOs and fixes form serialization to avoid crashes and wrong attachments.
New Features
validateDocumentsaction inERefer2ActionandvalidateDocumentsBelongToPatientinDocumentAttachmentManagerto verify documents, labs, eForms, and HRM docs against the patient.conreq.jsnow calls the validation endpoint before submit/edit; invalid selections show an error and stop the flow.HRMUtilhelper to scope IDs to the patient.Bug Fixes
SecurityInfoManager.hasPrivilegefor_conREAD in consult attachment and validation.conreq.jsfield handling: use jQuery.serialize()for POST data and.val()for raw values.Written for commit 792b66a. Summary will update on new commits.
Summary by CodeRabbit
Release Notes