HRM validation - XML validation, report match warnings, sending-facility registry, and auto-categorization#2456
Open
D3V41 wants to merge 56 commits into
Open
HRM validation - XML validation, report match warnings, sending-facility registry, and auto-categorization#2456D3V41 wants to merge 56 commits into
D3V41 wants to merge 56 commits into
Conversation
…md_cds_dt.xsd, report_manager_cds.xsd, report_manager.xsd, report_manager_dt.xsd). Updated HRMReportParser and HRMXMLHandler to load the canonical ontariomd_hrm.xsd from classpath using URL-based SchemaFactory (works in both exploded and packaged WAR deployments).
…me on report view
- Log WARN when a report ingests from an unknown SF ID - Show "Unregistered" badge on the report view - Banner on SF admin page with quick-add for unknown facilities
….forJavaScript on CSRF token
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Reviewer's GuideImplements end-to-end HRM validation and robustness improvements: stronger XML/schema and content validation with pre-JAXB checks and placeholder-date normalization; richer per-file upload results and non-fatal match-quality warnings; a new HRM Sending Facility registry surfaced across inbox, reports, mappings, PDFs, and alerts for unregistered facilities; import-time auto-categorization plus an inbox filter for uncategorized reports; sFTP configuration hardening with validation, error notifications, and audit logging; UI fixes and N+1 query reductions. Sequence diagram for HRM upload, XML validation, and warning surfacingsequenceDiagram
actor User
participant HRMUploadLab2Action
participant HRMReportParser
participant HRMXmlValidator
participant HRMSendingFacilityDao
participant SFTPConnector
participant UploadJsp as hospitalReportManager.jsp
User->>HRMUploadLab2Action: withUploadedFiles(uploadedFiles)
User->>HRMUploadLab2Action: execute()
HRMUploadLab2Action->>HRMUploadLab2Action: processFiles(loggedInInfo)
HRMUploadLab2Action->>HRMReportParser: parseReport(loggedInInfo, filePath, parseErrors)
HRMReportParser->>HRMXmlValidator: validateNoRequiredElementsEmpty(tmpXMLholder)
HRMReportParser->>HRMXmlValidator: normalizeInvalidDates(tmpXMLholder, parseWarnings)
HRMReportParser->>HRMReportParser: validateReportContent(parsed)
HRMReportParser->>HRMReportParser: validateDateOfBirth(parsed)
HRMReportParser-->>HRMUploadLab2Action: HRMReport (with uploadWarnings)
alt report parsed
HRMUploadLab2Action->>HRMReportParser: addReportToInbox(loggedInInfo, report, warnings)
HRMReportParser->>HRMReportParser: addUnknownSendingFacilityWarning(report, warnings)
HRMReportParser->>HRMReportParser: addUnknownSubClassWarning(report, warnings)
HRMReportParser->>HRMReportParser: resolveCategoryId(report)
HRMReportParser->>HRMSendingFacilityDao: findBySendingFacilityId(sendingFacilityId)
alt facility unregistered
HRMReportParser->>SFTPConnector: notifyHrmAdmin(loggedInInfo, subject, message)
end
HRMReportParser-->>HRMUploadLab2Action: document persisted
HRMUploadLab2Action-->>HRMUploadLab2Action: new UploadResult(COMPLETED, warnings)
else parse or import error
HRMUploadLab2Action-->>HRMUploadLab2Action: new UploadResult(INVALID/FAILED, errorMessage)
end
HRMUploadLab2Action->>UploadJsp: request.setAttribute("uploadResults", resultsMap)
UploadJsp-->>User: per-file status, expandable errorMessage and warnings
Entity-relationship diagram for HRM Sending Facility registryerDiagram
HRMSendingFacility {
int id PK
varchar sendingFacilityId UK
varchar facilityName
}
HRMDocument {
int id PK
varchar sourceFacility
varchar sourceFacilityReportNo
int hrmCategoryId
}
HRMSendingFacility ||--o{ HRMDocument : maps
HRMSendingFacility ||--o{ HRMDocument : sendingFacilityId_to_sourceFacility
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an HRM sending-facility registry (schema, entity, DAO, admin UI), enhances HRM XML validation/normalization with parse-time warnings, refactors upload processing to produce UploadResult objects with warnings, threads registry lookup into inbox/listing and PDFs, and updates SFTP/config notification helpers and UI wiring. ChangesHRM Sending Facility Registry and Report Enhancement
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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 registry for Hospital Report Manager (HRM) Sending Facilities to map facility IDs to human-readable names, alongside enhanced XML validation, date normalization, and unmatched category/provider warnings. While these changes significantly improve the robustness and usability of HRM report processing, several critical issues must be addressed. Specifically, the XML parsing in HRMXmlValidator is vulnerable to XML External Entity (XXE) injection, a potential NullPointerException exists in HRMReportParser.addUnknownSubClassWarning due to a missing null check, and there is a redundant loop in addStrictMatchMismatchWarnings that returns unconditionally on the first iteration.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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 4 issues, and left some high level feedback:
- In HRMXmlValidator.normalizeInvalidDates, DT_NS is set to the literal "cds_dt" but getElementsByTagNameNS expects a namespace URI, not a prefix, so the date leaf elements will never be found; consider using the actual namespace URI from the XSD or switching to getElementsByTagName for this use case.
- BinaryFileExtension.HTML.detect unconditionally calls content.charAt(0) before checking for emptiness, so an empty payload will throw an IndexOutOfBoundsException; guard for bytes.length == 0 or content.isEmpty() before inspecting the first character.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In HRMXmlValidator.normalizeInvalidDates, DT_NS is set to the literal "cds_dt" but getElementsByTagNameNS expects a namespace URI, not a prefix, so the date leaf elements will never be found; consider using the actual namespace URI from the XSD or switching to getElementsByTagName for this use case.
- BinaryFileExtension.HTML.detect unconditionally calls content.charAt(0) before checking for emptiness, so an empty payload will throw an IndexOutOfBoundsException; guard for bytes.length == 0 or content.isEmpty() before inspecting the first character.
## Individual Comments
### Comment 1
<location path="src/main/java/ca/openosp/openo/commn/model/enumerator/BinaryFileExtension.java" line_range="39-40" />
<code_context>
+ }
+ }
+ },
+ TIFF(".tiff") {
+ @Override protected boolean detect(byte[] b) { return isImageOfFormat(b, "tif"); }
+ },
+ RTF(".rtf") {
</code_context>
<issue_to_address>
**issue (bug_risk):** TIFF detection may never succeed because the ImageIO format name is likely "tiff" rather than "tif".
In isImageOfFormat() you compare against ImageReaderSpi.getFormatNames(), which for TIFF is typically "tiff", not "tif". Using only "tif" means valid TIFFs may never be detected. Please either support both "tif" and "tiff" or align this constant with the actual format names returned by ImageIO.
</issue_to_address>
### Comment 2
<location path="src/main/java/ca/openosp/openo/hospitalReportManager/HRMUploadLab2Action.java" line_range="118-120" />
<code_context>
+ @Override protected boolean detect(byte[] b) {
+ try (PDDocument ignored = PDDocument.load(b)) {
+ return true;
+ } catch (IOException e) {
+ return false;
+ }
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Surfacing raw exception messages in the upload result can leak internal details to end users.
For IO/SecurityException cases, `errorMessage` is now taken directly from `e.getMessage()`, which may reveal paths, config details, or stack info. Instead, map these to generic user-facing messages and log the full exception server-side, limiting raw exception text to admin-only views if required.
Suggested implementation:
```java
} catch (Exception e) {
MiscUtils.getLogger().error("Couldn't handle uploaded HRM report", e);
// Use a generic, user-safe message instead of surfacing internal exception details
String userSafeMessage = "Failed to process the uploaded report. Please verify the file and try again, or contact support if the problem persists.";
resultsMap.put(fileName, new UploadResult(FileStatus.FAILED, userSafeMessage));
}
```
There may be other places in this action (or related upload actions) where `UploadResult` is constructed with `e.getMessage()` for IO or security-related failures. For consistency and to avoid leaking internal details, those should be updated in the same way: log the exception in full, but pass a generic, user-facing message into `UploadResult` while keeping raw exception details only in server logs or admin-only views.
</issue_to_address>
### Comment 3
<location path="src/main/java/ca/openosp/openo/hospitalReportManager/HRMReportParser.java" line_range="190" />
<code_context>
return null;
}
+ private static void validateReportContent(OmdCds root) throws SAXException {
+ if (root == null || root.getPatientRecord() == null) return;
+ for (ReportsReceived report : root.getPatientRecord().getReportsReceived()) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the validation logic into a dedicated validator class and introducing an HRM processing context object so that `HRMReportParser` focuses on the main flow without `(report, warnings)` overloads.
You can keep all the new behaviour but cut complexity by pushing concerns out of `HRMReportParser` and removing the `(report, warnings)` overload pattern.
### 1. Extract validation into a dedicated validator
All the `validate*` helpers are pure, domain-level checks that don’t need to live on `HRMReportParser`. Moving them into a dedicated component keeps `parseReport` linear and easier to read.
Example:
```java
// new class
public final class HRMReportValidator {
private HRMReportValidator() {}
public static void validate(OmdCds root) throws SAXException {
validateReportContent(root);
validateDateOfBirth(root);
}
private static void validateReportContent(OmdCds root) throws SAXException {
if (root == null || root.getPatientRecord() == null) return;
for (ReportsReceived report : root.getPatientRecord().getReportsReceived()) {
if (ReportFormat.BINARY.equals(report.getFormat())) {
validateBinaryReport(report);
} else if (ReportFormat.TEXT.equals(report.getFormat())) {
validateTextReport(report);
}
}
}
// move validateBinaryReport, validateTextReport, validateDateOfBirth, validateNoEmptyElements here
}
```
Then `parseReport` becomes:
```java
Document normalizedDoc = HRMXmlValidator.normalizeInvalidDates(tmpXMLholder, parseWarnings);
JAXBContext jc = JAXBContext.newInstance("omd.hrm");
Unmarshaller u = jc.createUnmarshaller();
u.setSchema(schema);
OmdCds parsed = (OmdCds) u.unmarshal(normalizedDoc);
HRMReportValidator.validate(parsed);
root = parsed;
```
This keeps all existing behaviour (same exceptions, same validations), but removes parsing‑unrelated logic from `HRMReportParser`.
### 2. Replace `List<String> warnings` threading with a context object
Passing a nullable `List<String>` through many methods and adding overloads like `addReportToInbox(report)` / `addReportToInbox(report, warnings)` increases branching and call‑site ambiguity. You can keep the warnings feature but encapsulate it in a small context object, and have a single method signature for each operation.
Introduce a context:
```java
public final class HRMProcessingContext {
private final HRMReport report;
private HRMDocument document;
private final List<String> warnings = new ArrayList<>();
public HRMProcessingContext(HRMReport report) {
this.report = report;
}
public HRMReport getReport() { return report; }
public HRMDocument getDocument() { return document; }
public void setDocument(HRMDocument document) { this.document = document; }
public List<String> getWarnings() { return warnings; }
}
```
Refactor `addReportToInbox` to a single entry point:
```java
public static void addReportToInbox(LoggedInInfo loggedInInfo, HRMReport report) {
if (report == null) {
logger.info("addReportToInbox cannot continue, report parameter is null");
return;
}
HRMProcessingContext ctx = new HRMProcessingContext(report);
ctx.getWarnings().addAll(report.getUploadWarnings());
addUnknownSendingFacilityWarning(ctx);
addUnknownSubClassWarning(ctx);
HRMDocument document = buildDocumentFromReport(loggedInInfo, ctx);
ctx.setDocument(document);
// use ctx everywhere instead of passing warnings
String demProviderNo = routeReportToDemographic(ctx);
Boolean routed = routeReportToProvider(ctx);
// ...
}
```
Example of converting one of the helpers:
```java
private static String routeReportToDemographic(HRMProcessingContext ctx) {
HRMReport report = ctx.getReport();
HRMDocument mergedDocument = ctx.getDocument();
// existing logic...
if (demProviderNo == null) {
addStrictMatchMismatchWarnings(report, matchingDemographicListByHin, ctx.getWarnings());
}
return demProviderNo;
}
```
And the provider routing:
```java
public static boolean routeReportToProvider(HRMProcessingContext ctx) {
HRMReport report = ctx.getReport();
HRMDocument document = ctx.getDocument();
List<String> warnings = ctx.getWarnings();
// existing logic: use `warnings` instead of nullable parameter
return !sendToProviderList.isEmpty();
}
```
With this pattern:
- No `(report)` vs `(report, warnings)` overloads.
- No `warnings == null` checks: a context always has a warnings list.
- The high‑level flow (`addReportToInbox`) reads as “build context → route to demographic → route to provider → add subclass”, while all UC65–UC73 warning details stay in dedicated helpers that take the context.
These two changes (validator extraction + context) keep all functional changes, but trim the responsibilities and branching inside `HRMReportParser` so the core workflows are easier to follow and extend.
</issue_to_address>
### Comment 4
<location path="src/main/java/ca/openosp/openo/commn/model/enumerator/BinaryFileExtension.java" line_range="28" />
<code_context>
+ * <li>RTF — magic bytes ({\rtf), no lightweight parser exists in the project</li>
+ * </ul>
+ */
+public enum BinaryFileExtension {
+
+ PDF(".pdf") {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the format-detection logic from the enum into a dedicated helper (or functional field) so the enum only delegates and holds metadata.
You can reduce complexity by decoupling detection logic from the enum while keeping behavior identical.
### 1. Move detection logic into a helper class
Create a helper/utility that encapsulates all detection logic. The enum then just delegates:
```java
public final class BinaryFileDetectors {
private static final Pattern HTML_TAG_PATTERN = Pattern.compile(
"<(html|head|body|div|span|p|table|ul|li|br|h[1-6])\\b[^>]*>",
Pattern.CASE_INSENSITIVE | Pattern.DOTALL
);
private BinaryFileDetectors() {}
public static boolean isPdf(byte[] b) {
try (PDDocument ignored = PDDocument.load(b)) {
return true;
} catch (IOException e) {
return false;
}
}
public static boolean isImageOfFormat(byte[] bytes, String expectedFormat) {
try (ImageInputStream iis = ImageIO.createImageInputStream(new ByteArrayInputStream(bytes))) {
if (iis == null) return false;
Iterator<ImageReader> readers = ImageIO.getImageReaders(iis);
while (readers.hasNext()) {
ImageReaderSpi spi = readers.next().getOriginatingProvider();
if (spi == null) continue;
for (String name : spi.getFormatNames()) {
if (name.equalsIgnoreCase(expectedFormat)) return true;
}
}
return false;
} catch (IOException e) {
return false;
}
}
public static boolean isRtf(byte[] b) {
return startsWithMagic(b, '{','\\','r','t','f');
}
public static boolean isHtml(byte[] b) {
String content = new String(b, StandardCharsets.UTF_8);
if (!content.isEmpty() && content.charAt(0) == '\uFEFF') content = content.substring(1);
content = content.trim();
if (content.isEmpty()) return false;
if (content.regionMatches(true, 0, "<!DOCTYPE html", 0, 14)
|| content.regionMatches(true, 0, "<html", 0, 5)) {
return true;
}
return HTML_TAG_PATTERN.matcher(content).find();
}
private static boolean startsWithMagic(byte[] bytes, int... magic) {
if (bytes.length < magic.length) return false;
for (int i = 0; i < magic.length; i++) {
if ((bytes[i] & 0xFF) != magic[i]) return false;
}
return true;
}
}
```
### 2. Make the enum a data holder that delegates
Use the helper in the enum, so the enum is just mapping + delegation:
```java
public enum BinaryFileExtension {
PDF(".pdf") {
@Override protected boolean detect(byte[] b) {
return BinaryFileDetectors.isPdf(b);
}
},
TIFF(".tiff") {
@Override protected boolean detect(byte[] b) {
return BinaryFileDetectors.isImageOfFormat(b, "tif");
}
},
RTF(".rtf") {
@Override protected boolean detect(byte[] b) {
return BinaryFileDetectors.isRtf(b);
}
},
JPEG(".jpeg") {
@Override protected boolean detect(byte[] b) {
return BinaryFileDetectors.isImageOfFormat(b, "jpeg");
}
},
GIF(".gif") {
@Override protected boolean detect(byte[] b) {
return BinaryFileDetectors.isImageOfFormat(b, "gif");
}
},
PNG(".png") {
@Override protected boolean detect(byte[] b) {
return BinaryFileDetectors.isImageOfFormat(b, "png");
}
},
HTML(".html") {
@Override protected boolean detect(byte[] b) {
return BinaryFileDetectors.isHtml(b);
}
};
private final String extension;
BinaryFileExtension(String extension) {
this.extension = extension;
}
public final boolean matchesContent(byte[] bytes) {
if (bytes == null || bytes.length == 0) return false;
return detect(bytes);
}
protected abstract boolean detect(byte[] bytes);
// matches, fromExtension, allValues remain unchanged
}
```
### 3. Optional: use a functional field instead of abstract methods
If you want to eliminate per-constant bodies entirely:
```java
public enum BinaryFileExtension {
PDF(".pdf", BinaryFileDetectors::isPdf),
TIFF(".tiff", b -> BinaryFileDetectors.isImageOfFormat(b, "tif")),
RTF(".rtf", BinaryFileDetectors::isRtf),
JPEG(".jpeg", b -> BinaryFileDetectors.isImageOfFormat(b, "jpeg")),
GIF(".gif", b -> BinaryFileDetectors.isImageOfFormat(b, "gif")),
PNG(".png", b -> BinaryFileDetectors.isImageOfFormat(b, "png")),
HTML(".html", BinaryFileDetectors::isHtml);
private final String extension;
private final java.util.function.Predicate<byte[]> detector;
BinaryFileExtension(String extension, java.util.function.Predicate<byte[]> detector) {
this.extension = extension;
this.detector = detector;
}
public final boolean matchesContent(byte[] bytes) {
if (bytes == null || bytes.length == 0) return false;
return detector.test(bytes);
}
}
```
This keeps the enum as a simple registry + delegator, while all heavy detection logic lives in a dedicated, easily testable utility.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found across 44 files
Confidence score: 2/5
- Merge risk is high because there are concrete security issues with strong confidence:
src/main/java/ca/openosp/openo/hospitalReportManager/HRMXmlValidator.javaperforms both DOM and SAX XML parsing without XXE hardening, which can expose the upload/import path to entity expansion attacks. src/main/webapp/hospitalReportManager/hrmSendingFacilities.jspintroduces state-changing POST forms without CSRF token integration, andsrc/main/java/ca/openosp/openo/hospitalReportManager/HRMUploadLab2Action.javareturns raw exception text to users—together this increases exploitability and information disclosure risk.- There is also functional/regression risk in
src/main/java/ca/openosp/openo/documentManager/actions/DocumentPreview2Action.java, where duplicate filtering may hide older legitimate HRM attachments, causing inconsistent document selection behavior. - Pay close attention to
src/main/java/ca/openosp/openo/hospitalReportManager/HRMXmlValidator.java,src/main/webapp/hospitalReportManager/hrmSendingFacilities.jsp, andsrc/main/java/ca/openosp/openo/hospitalReportManager/HRMUploadLab2Action.java- security hardening (XXE/CSRF) and safer error handling should be addressed before merge.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/main/java/ca/openosp/openo/hospitalReportManager/HRMUtil.java (2)
282-309: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd comprehensive JavaDoc documentation for public method.
The
listMappingspublic method lacks JavaDoc documentation. As per coding guidelines, all public methods must have comprehensive JavaDoc including method description,@returntag with exact return type, and healthcare domain context for medical data.📝 Suggested JavaDoc template
+ /** + * Returns a list of all HRM subclass mappings with sending facility display names. + * + * Retrieves all configured HRM subclass mappings that define how incoming report + * classifications from external healthcare facilities are categorized within the system. + * Each mapping includes the sending facility display name (resolved from the registry), + * class name, subclass name, mnemonic, description, and associated HRM category. + * + * `@return` ArrayList<HashMap<String, ? extends Object>> list of HRM subclass mapping configurations + */ public static ArrayList<HashMap<String, ? extends Object>> listMappings() {As per coding guidelines for
src/main/java/**/*.java: "All public classes and methods MUST have comprehensive JavaDoc documentation" and forsrc/main/java/**/commn/{model,dao}/**/*.java: "Document healthcare domain context in JavaDoc for medical data classes".🤖 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/ca/openosp/openo/hospitalReportManager/HRMUtil.java` around lines 282 - 309, The public method listMappings in HRMUtil lacks JavaDoc; add comprehensive JavaDoc above the listMappings method that describes its purpose in the healthcare/medical-data context (what mappings it returns and how they relate to HRMSubClass/HRMSendingFacility), document that it returns an ArrayList<HashMap<String, ? extends Object>> (explain the map keys such as "id","facility_display","sub_class","class","category","mnemonic","description","mappingId"), include an `@return` tag with the exact return type, mention any relevant side-effects or null/empty return behavior and thread-safety assumptions, and include author/version or other project-required tags per coding guidelines.
69-215: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd comprehensive JavaDoc documentation for public method.
The
listHRMDocumentspublic method lacks JavaDoc documentation. As per coding guidelines, all public methods must have comprehensive JavaDoc including method description,@paramtags with specific data types,@returntags with exact return types, and@throwstags for exceptions.📝 Suggested JavaDoc template
+ /** + * Returns a list of HRM documents associated with a specific patient demographic. + * + * Retrieves all HRM documents linked to the specified demographic number and generates + * summary information for each document including metadata such as report type, status, + * sending facility display name, and report number. When filterDuplicates is true, + * multiple versions of the same report are consolidated to show only the most recent. + * + * `@param` loggedInInfo LoggedInInfo the logged-in user information for security validation + * `@param` sortBy String the field name to sort by (e.g., "time_received", "report_date", "report_name", "category") + * `@param` sortAsc boolean true to sort ascending, false to sort descending + * `@param` demographicNo String the patient's demographic number + * `@param` filterDuplicates boolean true to filter out duplicate report versions, false to show all versions + * `@return` ArrayList<HashMap<String, ? extends Object>> list of HRM document summaries with metadata + */ public static ArrayList<HashMap<String, ? extends Object>> listHRMDocuments(LoggedInInfo loggedInInfo, String sortBy, boolean sortAsc, String demographicNo, boolean filterDuplicates) {As per coding guidelines for
src/main/java/**/*.java: "All public classes and methods MUST have comprehensive JavaDoc documentation" and "Document@paramtags with specific data types" and "Document@returntags with exact return types in JavaDoc".🤖 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/ca/openosp/openo/hospitalReportManager/HRMUtil.java` around lines 69 - 215, The public method listHRMDocuments is missing JavaDoc; add comprehensive JavaDoc above the method listHRMDocuments(LoggedInInfo loggedInInfo, String sortBy, boolean sortAsc, String demographicNo, boolean filterDuplicates) describing what the method does, and include `@param` entries for each parameter with their concrete types and semantics, an `@return` that specifies the exact return type (ArrayList<HashMap<String, ? extends Object>>) and what the list contains, and `@throws` tags documenting any runtime/DAO/security exceptions that callers might encounter (e.g., SecurityException or any DataAccess/DAO exceptions thrown by hrmDocumentToDemographicDao/hrmCategoryDao/etc.); keep the description concise and follow the project JavaDoc style.src/main/java/ca/openosp/openo/lab/ca/all/parsers/HRMXMLHandler.java (1)
71-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAttach the compiled HRM XSD to the JAXB unmarshaller (enable validation).
Schema schema = factory.newSchema(schemaUrl);is created (lines 71-76) but never applied to theUnmarshaller, sou.unmarshal(...)runs without XSD validation.Suggested fix
JAXBContext jc = JAXBContext.newInstance("ca.openosp.openo.hospitalReportManager.xsd"); Unmarshaller u = jc.createUnmarshaller(); + u.setSchema(schema); root = (OmdCds) u.unmarshal(byeArrayInputStream);🤖 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/ca/openosp/openo/lab/ca/all/parsers/HRMXMLHandler.java` around lines 71 - 80, The Unmarshaller in HRMXMLHandler is not using the compiled Schema (variable schema) so XML is unmarshalled without XSD validation; fix by applying the Schema to the Unmarshaller instance (call setSchema on u) before calling u.unmarshal(byeArrayInputStream) and optionally attach a ValidationEventHandler to u to fail/collect validation errors (ensure the schema variable created from schemaUrl is passed to u.setSchema(schema) so OmdCds unmarshalling is validated).src/main/webapp/hospitalReportManager/disable_msg_action.jsp (1)
1-18: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd comprehensive JSP comment block after copyright.
A JSP comment block documenting the page's purpose, features, and
@sincetag is required after the copyright header. As per coding guidelines: "Include comprehensive JSP comment blocks after copyright headers including purpose, features, parameters, and@since."📝 Suggested JSP comment structure
--%> +<%-- + Purpose: Dismisses HRM outage notifications for the current provider + + Features: + - Adds the logged-in provider to the HRM outage notification suppression list + - Redirects back to main HRM page with confirmation message + + `@since` 2026-05-22 +--%> <%@page import="ca.openosp.openo.utility.LoggedInInfo" %>As per coding guidelines for
src/main/webapp/**/*.jsp.🤖 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/webapp/hospitalReportManager/disable_msg_action.jsp` around lines 1 - 18, Add a comprehensive JSP comment block immediately after the existing copyright header in disable_msg_action.jsp describing the page purpose (e.g., marks a logged-in user as "do not send" and redirects to hospitalReportManager.jsp), list key behaviors/features (uses LoggedInInfo.getLoggedInInfoFromSession and calls SFTPConnector.addMeToDoNotSendList, then redirects), document expected parameters/inputs (HTTP request/session use), and include an `@since` tag with the current version/date; place this JSP comment block before any import directives or scriptlet code.src/main/webapp/hospitalReportManager/hospitalReportManager.jsp (1)
1-41: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd comprehensive JSP comment block after copyright.
A JSP comment block documenting the page's purpose, features, parameters, and
@sincetag is required after the copyright header. As per coding guidelines: "Include comprehensive JSP comment blocks after copyright headers including purpose, features, parameters, and@since."📝 Suggested JSP comment structure
--%> +<%-- + Purpose: Hospital Report Manager main page for uploading, fetching, and managing HRM reports + + Features: + - Manual HRM report file upload with validation and warning display + - SFTP auto-fetch trigger for downloading reports from external HRM system + - Provider confidentiality statement management + - HRM outage notification dismissal + + Parameters: + - fetch (optional): Set to "true" to trigger SFTP auto-fetch + - outageDismissed (optional): Set to "true" to show dismissal confirmation message + + `@since` 2026-05-22 +--%> <!DOCTYPE html>As per coding guidelines for
src/main/webapp/**/*.jsp.🤖 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/webapp/hospitalReportManager/hospitalReportManager.jsp` around lines 1 - 41, Add a comprehensive JSP comment block immediately after the existing copyright header in hospitalReportManager.jsp describing the page purpose (e.g., "Hospital report management UI and SFTP integration"), key features/behaviors (what the page renders, any actions like upload/download, security checks via security:oscarSec), expected parameters or session attributes used (e.g., session attributes "userrole", "user", LoggedInInfo via LoggedInInfo.getLoggedInInfoFromSession), important classes referenced (SFTPConnector, HRMProviderConfidentialityStatementDao), and include an `@since` tag with the current version/date; place it before any scriptlet/imports so it clearly documents the file.src/main/java/ca/openosp/openo/hospitalReportManager/HRMUploadLab2Action.java (1)
38-202: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd comprehensive JavaDoc documentation.
The class and its public methods lack JavaDoc documentation. As per coding guidelines, all public classes and methods must have comprehensive JavaDoc including class purpose, parameter types, return types, and thrown exceptions.
As per coding guidelines for
src/main/java/**/*.java.🤖 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/ca/openosp/openo/hospitalReportManager/HRMUploadLab2Action.java` around lines 38 - 202, Add JavaDoc to the public class HRMUploadLab2Action and its public methods: document the class purpose and high-level behavior; add JavaDoc for withUploadedFiles(List<UploadedFile> uploadedFiles) describing the uploadedFiles parameter and its expected state; add JavaDoc for execute() describing the return value, side-effects (sets request attribute "uploadResults"), and runtime exceptions it may throw (e.g., SecurityException on missing privileges). Ensure tags cover parameters (`@param`), return (`@return`), and exceptions (`@throws`) where applicable and mention important interactions with securityInfoManager, processFiles, and UploadedFilesAware contract.
🧹 Nitpick comments (4)
src/main/java/ca/openosp/openo/hospitalReportManager/SFTPConnector.java (1)
622-626: ⚡ Quick winCache sending-facility lookups within the batch.
This adds a DAO round-trip inside the per-file import loop. Large auto-polls will repeatedly query the same sending-facility IDs and reintroduce the N+1 pattern this PR is otherwise trying to remove. Cache known/missing IDs for the current fetch or prefetch the registered IDs before iterating.
🤖 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/ca/openosp/openo/hospitalReportManager/SFTPConnector.java` around lines 622 - 626, The per-file loop calls hrmSendingFacilityDao.findBySendingFacilityId(sfId) for each report, causing N+1 DB calls; introduce a batch-local cache (e.g., Map<String,Boolean> sendingFacilityExists or two Sets for present/missing) keyed by report.getSendingFacilityId() and consult it before calling hrmSendingFacilityDao.findBySendingFacilityId; if your DAO supports bulk lookup, prefetch all sending IDs for the current fetch into a Set via a method like hrmSendingFacilityDao.findBySendingFacilityIds(Set) and use that Set to decide when to call hrmLogEntry.setError, otherwise populate the local cache on first lookup and reuse it for subsequent reports in SFTPConnector’s processing loop.src/main/java/ca/openosp/openo/hospitalReportManager/HRMSendingFacility2Action.java (1)
31-37: ⚡ Quick winDocument thrown exceptions in public method JavaDoc.
These public methods throw
SecurityException, but their JavaDoc omits@throwsentries.📝 Proposed doc update
/** * Routes the request to {`@link` `#save`()}, {`@link` `#delete`()}, or {`@link` `#list`()} based on the * {`@code` method} request parameter. Save and delete are rejected unless the request is a POST. * * `@return` String the Struts result name + * `@throws` SecurityException when a state-changing method is requested via non-POST */ @@ /** * Lists all registered sending facilities plus the unregistered facilities seen on HRM * documents. When a valid numeric {`@code` id} parameter is supplied, loads that facility into * the {`@code` editing} request attribute so the form is pre-filled for editing. * * `@return` String the Struts result name + * `@throws` SecurityException when the caller lacks _admin read privilege */ @@ /** * Creates a new sending facility or updates an existing one from the posted * {`@code` sendingFacilityId} and {`@code` facilityName}. Validates that both fields are present * and that the sending-facility ID is not already used by a different entry; on any validation * error an {`@code` errorMessage} is set and the list view is re-rendered. * * `@return` String the Struts result name + * `@throws` SecurityException when the caller lacks _admin write privilege */ @@ /** * Deletes the sending facility identified by the posted {`@code` id}. A missing or invalid id is * a no-op; failures are logged and surfaced via an {`@code` errorMessage} on the list view. * * `@return` String the Struts result name + * `@throws` SecurityException when the caller lacks _admin write privilege */As per coding guidelines, "Use exception handling that includes
@throwsJavaDoc tags documenting all exceptions thrown by methods."Also applies to: 54-60, 80-87, 138-143
🤖 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/ca/openosp/openo/hospitalReportManager/HRMSendingFacility2Action.java` around lines 31 - 37, The public method execute() is missing an `@throws` JavaDoc for SecurityException; update the JavaDoc for execute() to include an `@throws` SecurityException entry describing when the exception is thrown (e.g., when authorization fails), and do the same for the other public action methods in this class that throw SecurityException (notably save(), delete(), list() and any other public methods flagged in the review) so each JavaDoc includes a clear `@throws` SecurityException description matching the method's failure conditions.src/main/java/ca/openosp/openo/hospitalReportManager/dao/HRMDocumentDao.java (1)
226-251: ⚡ Quick winDocument the new
categoryUnmatchedcontract on the public DAO API.Adding a fifth positional boolean materially changes these methods, but callers still have no JavaDoc explaining that this flag filters on
x.hrmCategoryId IS NULL. Please document the new parameter on both methods so the DAO contract stays usable.As per coding guidelines,
src/main/java/**/*.java: "All public classes and methods MUST have comprehensive JavaDoc documentation" and "Document@paramtags with specific data types ...".🤖 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/ca/openosp/openo/hospitalReportManager/dao/HRMDocumentDao.java` around lines 226 - 251, The public DAO methods query(...) and queryForCount(...) were extended with a new boolean parameter categoryUnmatched but lack JavaDoc; add comprehensive JavaDoc to both methods describing the new parameter and the method contract, including an `@param` tag for categoryUnmatched that explicitly states it filters records where x.hrmCategoryId IS NULL, document the other parameters (providerNo, providerUnmatched, noSignOff, demographicUnmatched, start, length, orderColumn, orderDirection) with concrete types/semantics, and include a clear `@return` description for the list/count; ensure the JavaDoc follows the project's style guidelines for public APIs and appears immediately above the method signatures in HRMDocumentDao.java.src/main/webapp/hospitalReportManager/hospitalReportManager.jsp (1)
278-290: ⚖️ Poor tradeoffConsider adding CSRF token to upload form.
The upload form lacks a CSRF hidden input. While this PR's scope is validation and parsing improvements, consider adding CSRF protection in a follow-up security-focused PR. As per coding guidelines: "Include CSRF token in all HTML forms:
<input type='hidden' name='${_csrf.parameterName}' value='${_csrf.token}'/>".As per coding guidelines for
**/*.jsp.🤖 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/webapp/hospitalReportManager/hospitalReportManager.jsp` around lines 278 - 290, Add a CSRF hidden input to the upload form so the server can validate requests: inside the <form ... action="/hospitalReportManager/UploadLab.do" ...> (the form that contains the file input id="fileInput" and submit button id="file-upload-btn"), insert a hidden input using the app's CSRF parameter and token (use ${_csrf.parameterName} and ${_csrf.token}) so the UploadLab.do handler can verify the token on submit; ensure this is included alongside the existing file input and before the submit button so validateForm()/getFileList(event) behavior is unchanged.
🤖 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.
Inline comments:
In `@src/main/java/ca/openosp/openo/hospitalReportManager/HRMReportParser.java`:
- Around line 171-173: The code is forwarding raw parser exception text
(cause.getMessage()) to SFTPConnector.notifyHrmError which may contain PHI;
change the call to send a generic admin-facing message (e.g., "Report parsing
failed — contact admin") to SFTPConnector.notifyHrmError instead of the raw msg,
and log the full exception details only to the internal logger (e.g., use
HRMReportParser's logger.error with the caught exception or cause) so detailed
error text remains in server logs and never goes to the HRM notification
channel.
In
`@src/main/java/ca/openosp/openo/hospitalReportManager/HRMSendingFacility2Action.java`:
- Around line 74-75: The code logs raw request-derived values (editId and idStr)
in HRMSendingFacility2Action, which can allow log injection; update all logging
sites that reference editId and idStr (including the warn at
MiscUtils.getLogger().warn(...) and the other occurrences around the
delete/update flow) to sanitize those values with Encode.forJava() before
concatenating into log messages (e.g., pass Encode.forJava(editId) /
Encode.forJava(idStr) to the logger) so logs contain encoded, safe
representations of user-supplied IDs.
In `@src/main/java/ca/openosp/openo/hospitalReportManager/HRMXmlValidator.java`:
- Around line 194-197: Sanitize the untrusted XML text before embedding it in
warnings and logs: in HRMXmlValidator where you build the warning and log (the
warnings.add(...) call and the logger.warn(...) call that use the variable
value), replace raw value with an encoded version using OWASP Encoder
Encode.forJava(value) (and use the same encoded value when building the warning
message string) so CR/LF or other injection cannot be introduced into logs or
warning text.
- Around line 84-87: In both validateNoRequiredElementsEmpty(...) (where
SAXParserFactory spf/newSAXParser().parse(xmlFile, new DefaultHandler()) is
used) and normalizeInvalidDates(...) (where DocumentBuilderFactory
dbf/newDocumentBuilder().parse(xmlFile) is used) enable JAXP XXE protections:
set FEATURE_SECURE_PROCESSING, disallow DOCTYPE
(http://apache.org/xml/features/disallow-doctype-decl = true), disable
external-general-entities and external-parameter-entities, and set
"http://javax.xml.XMLConstants/feature/secure-processing" (or
XMLConstants.FEATURE_SECURE_PROCESSING) appropriately; additionally register an
EntityResolver that returns an empty InputSource for any external entity
resolution before calling parse to fully block external entities. In
normalizeInvalidDates(...), stop logging/storing raw XML-derived strings
directly—sanitize or redact the invalid date text before calling
logger.warn(...) and warnings.add(...) to prevent log injection and PHI leakage
(e.g., replace newlines/special chars and truncate or mask content).
In `@src/main/java/ca/openosp/openo/hospitalReportManager/SFTPConnector.java`:
- Around line 712-720: The parsePort helper currently parses the string but
doesn't reject out-of-range TCP ports; update parsePort(String port) to after
trimming and Integer.parseInt(portTrimmed) validate that the resulting int is
between 1 and 65535 (inclusive) and if not throw an IllegalStateException with a
clear message including the original port value; keep the existing null/empty
check and NumberFormatException handling but add this range check so invalid
ports (0, negatives, >65535) fail fast before reaching the SFTP layer.
- Around line 712-742: Add comprehensive JavaDoc to the four new public methods
parsePort, requirePrivateKeyPath, notifyHrmError, and notifyHrmAdmin: describe
what each method does, document all parameters with `@param`, any return values
with `@return` (e.g., parsePort returns int, requirePrivateKeyPath returns
String), document the exceptions thrown with `@throws` (e.g.,
IllegalStateException for missing/invalid config), and add an accurate `@since`
tag for each method using the git history date for when they were introduced;
ensure the JavaDoc is placed immediately above each method declaration and
follows the project's style and formatting conventions.
- Around line 723-730: The requirePrivateKeyPath method currently only checks
for null privateKeyDirectory and can return a relative/invalid path; update
requirePrivateKeyPath to reject blank/empty privateKeyDirectory (trim and
isEmpty) and to resolve and validate the concatenated path using
PathValidationUtils from ca.openosp.openo.utility.PathValidationUtils before
returning it; if validation fails, throw an IllegalStateException with a clear
message (same style as existing exceptions) so the final path passed to JSch is
guaranteed validated and absolute.
In `@src/main/java/ca/openosp/openo/hospitalReportManager/UploadResult.java`:
- Around line 6-49: The UploadResult class and its public API (class
UploadResult, nested enum FileStatus, both constructors
UploadResult(FileStatus,String) and
UploadResult(FileStatus,String,List<String>), and public methods getStatus,
getErrorMessage, getWarnings, isHasWarnings, getCssClass, getStatusText) lack
JavaDoc; add comprehensive JavaDoc to the class explaining its responsibility,
document each enum constant in FileStatus, annotate each constructor parameter
with `@param` and describe behavior (including null-handling of warnings),
document each method with `@return` and describe return semantics (e.g.,
getCssClass mapping for statuses and warning state, getStatusText messages), and
include `@throws` if any are added or relevant; keep descriptions concise and
follow project JavaDoc style.
In `@src/main/webapp/hospitalReportManager/hrmSendingFacilities.jsp`:
- Around line 107-110: The POST forms in hrmSendingFacilities.jsp that set
method="save" and method="delete" lack CSRF tokens; update both form blocks (the
one with <input name="method" value="save"/> and the other with value="delete")
to include a hidden CSRF input using the Spring vars: add a hidden field with
name="${_csrf.parameterName}" and value="${_csrf.token}" inside each form so the
server can validate the CSRF token on submit; ensure the token input is rendered
within the same <form> elements that contain the existing hidden "method" and
"id" inputs.
In `@src/main/webapp/hospitalReportManager/inbox.js`:
- Around line 102-103: The new column definition for "report_number" in the
columns array of inbox.js is currently sortable by default; update the column
object for "report_number" to explicitly disable ordering (e.g., set orderable:
false) so DataTables won't send sort requests for it, or alternatively add a
backend mapping from report_number -> sourceFacilityReportNo in HRM2Action and
include sourceFacilityReportNo in the DAO order-fragment allowlist; target the
"report_number" column definition in inbox.js (or the HRM2Action/DAO allowlist
if choosing the backend route) when making the change.
---
Outside diff comments:
In
`@src/main/java/ca/openosp/openo/hospitalReportManager/HRMUploadLab2Action.java`:
- Around line 38-202: Add JavaDoc to the public class HRMUploadLab2Action and
its public methods: document the class purpose and high-level behavior; add
JavaDoc for withUploadedFiles(List<UploadedFile> uploadedFiles) describing the
uploadedFiles parameter and its expected state; add JavaDoc for execute()
describing the return value, side-effects (sets request attribute
"uploadResults"), and runtime exceptions it may throw (e.g., SecurityException
on missing privileges). Ensure tags cover parameters (`@param`), return (`@return`),
and exceptions (`@throws`) where applicable and mention important interactions
with securityInfoManager, processFiles, and UploadedFilesAware contract.
In `@src/main/java/ca/openosp/openo/hospitalReportManager/HRMUtil.java`:
- Around line 282-309: The public method listMappings in HRMUtil lacks JavaDoc;
add comprehensive JavaDoc above the listMappings method that describes its
purpose in the healthcare/medical-data context (what mappings it returns and how
they relate to HRMSubClass/HRMSendingFacility), document that it returns an
ArrayList<HashMap<String, ? extends Object>> (explain the map keys such as
"id","facility_display","sub_class","class","category","mnemonic","description","mappingId"),
include an `@return` tag with the exact return type, mention any relevant
side-effects or null/empty return behavior and thread-safety assumptions, and
include author/version or other project-required tags per coding guidelines.
- Around line 69-215: The public method listHRMDocuments is missing JavaDoc; add
comprehensive JavaDoc above the method listHRMDocuments(LoggedInInfo
loggedInInfo, String sortBy, boolean sortAsc, String demographicNo, boolean
filterDuplicates) describing what the method does, and include `@param` entries
for each parameter with their concrete types and semantics, an `@return` that
specifies the exact return type (ArrayList<HashMap<String, ? extends Object>>)
and what the list contains, and `@throws` tags documenting any
runtime/DAO/security exceptions that callers might encounter (e.g.,
SecurityException or any DataAccess/DAO exceptions thrown by
hrmDocumentToDemographicDao/hrmCategoryDao/etc.); keep the description concise
and follow the project JavaDoc style.
In `@src/main/java/ca/openosp/openo/lab/ca/all/parsers/HRMXMLHandler.java`:
- Around line 71-80: The Unmarshaller in HRMXMLHandler is not using the compiled
Schema (variable schema) so XML is unmarshalled without XSD validation; fix by
applying the Schema to the Unmarshaller instance (call setSchema on u) before
calling u.unmarshal(byeArrayInputStream) and optionally attach a
ValidationEventHandler to u to fail/collect validation errors (ensure the schema
variable created from schemaUrl is passed to u.setSchema(schema) so OmdCds
unmarshalling is validated).
In `@src/main/webapp/hospitalReportManager/disable_msg_action.jsp`:
- Around line 1-18: Add a comprehensive JSP comment block immediately after the
existing copyright header in disable_msg_action.jsp describing the page purpose
(e.g., marks a logged-in user as "do not send" and redirects to
hospitalReportManager.jsp), list key behaviors/features (uses
LoggedInInfo.getLoggedInInfoFromSession and calls
SFTPConnector.addMeToDoNotSendList, then redirects), document expected
parameters/inputs (HTTP request/session use), and include an `@since` tag with the
current version/date; place this JSP comment block before any import directives
or scriptlet code.
In `@src/main/webapp/hospitalReportManager/hospitalReportManager.jsp`:
- Around line 1-41: Add a comprehensive JSP comment block immediately after the
existing copyright header in hospitalReportManager.jsp describing the page
purpose (e.g., "Hospital report management UI and SFTP integration"), key
features/behaviors (what the page renders, any actions like upload/download,
security checks via security:oscarSec), expected parameters or session
attributes used (e.g., session attributes "userrole", "user", LoggedInInfo via
LoggedInInfo.getLoggedInInfoFromSession), important classes referenced
(SFTPConnector, HRMProviderConfidentialityStatementDao), and include an `@since`
tag with the current version/date; place it before any scriptlet/imports so it
clearly documents the file.
---
Nitpick comments:
In
`@src/main/java/ca/openosp/openo/hospitalReportManager/dao/HRMDocumentDao.java`:
- Around line 226-251: The public DAO methods query(...) and queryForCount(...)
were extended with a new boolean parameter categoryUnmatched but lack JavaDoc;
add comprehensive JavaDoc to both methods describing the new parameter and the
method contract, including an `@param` tag for categoryUnmatched that explicitly
states it filters records where x.hrmCategoryId IS NULL, document the other
parameters (providerNo, providerUnmatched, noSignOff, demographicUnmatched,
start, length, orderColumn, orderDirection) with concrete types/semantics, and
include a clear `@return` description for the list/count; ensure the JavaDoc
follows the project's style guidelines for public APIs and appears immediately
above the method signatures in HRMDocumentDao.java.
In
`@src/main/java/ca/openosp/openo/hospitalReportManager/HRMSendingFacility2Action.java`:
- Around line 31-37: The public method execute() is missing an `@throws` JavaDoc
for SecurityException; update the JavaDoc for execute() to include an `@throws`
SecurityException entry describing when the exception is thrown (e.g., when
authorization fails), and do the same for the other public action methods in
this class that throw SecurityException (notably save(), delete(), list() and
any other public methods flagged in the review) so each JavaDoc includes a clear
`@throws` SecurityException description matching the method's failure conditions.
In `@src/main/java/ca/openosp/openo/hospitalReportManager/SFTPConnector.java`:
- Around line 622-626: The per-file loop calls
hrmSendingFacilityDao.findBySendingFacilityId(sfId) for each report, causing N+1
DB calls; introduce a batch-local cache (e.g., Map<String,Boolean>
sendingFacilityExists or two Sets for present/missing) keyed by
report.getSendingFacilityId() and consult it before calling
hrmSendingFacilityDao.findBySendingFacilityId; if your DAO supports bulk lookup,
prefetch all sending IDs for the current fetch into a Set via a method like
hrmSendingFacilityDao.findBySendingFacilityIds(Set) and use that Set to decide
when to call hrmLogEntry.setError, otherwise populate the local cache on first
lookup and reuse it for subsequent reports in SFTPConnector’s processing loop.
In `@src/main/webapp/hospitalReportManager/hospitalReportManager.jsp`:
- Around line 278-290: Add a CSRF hidden input to the upload form so the server
can validate requests: inside the <form ...
action="/hospitalReportManager/UploadLab.do" ...> (the form that contains the
file input id="fileInput" and submit button id="file-upload-btn"), insert a
hidden input using the app's CSRF parameter and token (use
${_csrf.parameterName} and ${_csrf.token}) so the UploadLab.do handler can
verify the token on submit; ensure this is included alongside the existing file
input and before the submit button so validateForm()/getFileList(event) behavior
is unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc4421f8-4270-4404-8553-cf901180815f
📒 Files selected for processing (44)
database/mysql/oscarinit.sqldatabase/mysql/updates/update-2026-05-22-hrm-sending-facility.sqlsrc/main/java/ca/openosp/openo/commn/model/enumerator/BinaryFileExtension.javasrc/main/java/ca/openosp/openo/documentManager/actions/DocumentPreview2Action.javasrc/main/java/ca/openosp/openo/hospitalReportManager/HRMPDFCreator.javasrc/main/java/ca/openosp/openo/hospitalReportManager/HRMPreferences.javasrc/main/java/ca/openosp/openo/hospitalReportManager/HRMPreferences2Action.javasrc/main/java/ca/openosp/openo/hospitalReportManager/HRMReport.javasrc/main/java/ca/openosp/openo/hospitalReportManager/HRMReportParser.javasrc/main/java/ca/openosp/openo/hospitalReportManager/HRMSendingFacility2Action.javasrc/main/java/ca/openosp/openo/hospitalReportManager/HRMUploadKey2Action.javasrc/main/java/ca/openosp/openo/hospitalReportManager/HRMUploadLab2Action.javasrc/main/java/ca/openosp/openo/hospitalReportManager/HRMUtil.javasrc/main/java/ca/openosp/openo/hospitalReportManager/HRMXmlValidator.javasrc/main/java/ca/openosp/openo/hospitalReportManager/SFTPConnector.javasrc/main/java/ca/openosp/openo/hospitalReportManager/SchedulerJob.javasrc/main/java/ca/openosp/openo/hospitalReportManager/UploadResult.javasrc/main/java/ca/openosp/openo/hospitalReportManager/dao/HRMDocumentDao.javasrc/main/java/ca/openosp/openo/hospitalReportManager/dao/HRMSendingFacilityDao.javasrc/main/java/ca/openosp/openo/hospitalReportManager/model/HRMSendingFacility.javasrc/main/java/ca/openosp/openo/hospitalReportManager/v2018/HRM2Action.javasrc/main/java/ca/openosp/openo/hospitalReportManager/v2018/HRMDownloadJob.javasrc/main/java/ca/openosp/openo/lab/ca/all/parsers/HRMXMLHandler.javasrc/main/resources/oscar_mcmaster.propertiessrc/main/resources/xsd/hrm/1.1.2/ontariomd_hrm_dt.xsdsrc/main/webapp/WEB-INF/classes/struts.xmlsrc/main/webapp/admin/admin.jspsrc/main/webapp/administration/leftNav.jspfsrc/main/webapp/hospitalReportManager/OMD/ontariomd_cds_dt.xsdsrc/main/webapp/hospitalReportManager/OMD/report_manager.xsdsrc/main/webapp/hospitalReportManager/OMD/report_manager_cds.xsdsrc/main/webapp/hospitalReportManager/OMD/report_manager_dt.xsdsrc/main/webapp/hospitalReportManager/configure.jspsrc/main/webapp/hospitalReportManager/disable_msg_action.jspsrc/main/webapp/hospitalReportManager/displayHRMDocList.jspsrc/main/webapp/hospitalReportManager/displayHRMReport.jspsrc/main/webapp/hospitalReportManager/hospitalReportManager.jspsrc/main/webapp/hospitalReportManager/hrmCategories.jspsrc/main/webapp/hospitalReportManager/hrmKeyUploader.jspsrc/main/webapp/hospitalReportManager/hrmPreferences.jspsrc/main/webapp/hospitalReportManager/hrmSendingFacilities.jspsrc/main/webapp/hospitalReportManager/hrmShowMapping.jspsrc/main/webapp/hospitalReportManager/inbox.jssrc/main/webapp/hospitalReportManager/inbox.jsp
💤 Files with no reviewable changes (10)
- src/main/webapp/hospitalReportManager/hrmKeyUploader.jsp
- src/main/webapp/hospitalReportManager/hrmPreferences.jsp
- src/main/java/ca/openosp/openo/hospitalReportManager/HRMPreferences2Action.java
- src/main/webapp/hospitalReportManager/OMD/report_manager.xsd
- src/main/java/ca/openosp/openo/hospitalReportManager/HRMUploadKey2Action.java
- src/main/java/ca/openosp/openo/hospitalReportManager/HRMPreferences.java
- src/main/webapp/hospitalReportManager/OMD/report_manager_dt.xsd
- src/main/webapp/hospitalReportManager/OMD/report_manager_cds.xsd
- src/main/webapp/hospitalReportManager/OMD/ontariomd_cds_dt.xsd
- src/main/java/ca/openosp/openo/hospitalReportManager/SchedulerJob.java
… loop simplification
…in HRM upload error handling
…Context to reduce HRMReportParser complexity
… was never applied to the unmarshaller
…displayHRMReport.jsp
…in HRMReportParser
…— count queries are not paginated by offset
…receive token injection
…ent external mutation
…t whitespace-padded SF IDs being misclassified as unregistered
…dationUtils in SFTPConnector.requirePrivateKeyPath
There was a problem hiding this comment.
2 issues found across 13 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ew from getWarnings(), addWarning/addWarnings for mutation
LiamStanziani
approved these changes
Jun 8, 2026
| <li><a href="hrmCategories.jsp">Categories</a></li> | ||
| <li><a href="inbox.jsp">HRM Inbox</a></li> | ||
| <li><a href="hospitalReportManager/hrmShowMapping.jsp">Class Mappings</a></li> | ||
| <li><a href="hospitalReportManager/hrmCategories.jsp">Categories</a></li> |
Collaborator
There was a problem hiding this comment.
Nitpick. but could we prepend these paths with <%=request.getContextPath()%>/ ?
| <li><a href="log.jsp">Log</a></li> | ||
| <li><a href="prefs.jsp">Prefs</a></li> | ||
| <li><a href="hospitalReportManager/log.jsp">Log</a></li> | ||
| <li><a href="hospitalReportManager/prefs.jsp">Prefs</a></li> |
Collaborator
There was a problem hiding this comment.
Nitpick. but could we prepend these paths with <%=request.getContextPath()%>/ ?
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.
HRM validation work split across two tracks:
Required Migration
database/mysql/updates/update-2026-05-22-hrm-sending-facility.sql
Changes
XML Validation
ontariomd_hrm.xsdfrom classpath (works in exploded and packaged WAR)HRMXmlValidatorFormatandFileExtensionAndVersionfields after JAXB parse; validate report content matches declared file type; validateDateOfBirthis within acceptable rangeReport Match Quality Warnings
DeliverToUserIDprefix disagrees with provider role or looks like a CPSO/billing ID swapSendingFacilityorSubClassis not configuredImport Robustness & UI
Encode.forJavaScriptmisapplied to CSRF tokenconfigure.jspin admin left navSending Facility Registry (BP6)
HRMSendingFacilityentity, DAO,HRMSendingFacility2Action, andhrmSendingFacilities.jspadmin UI to map facility ID → readable nameUnregistered Facility Alerting (PS08)
Auto-Categorization at Import (HRM02.08 / HRM02.09)
HRMReportParser.resolveCategoryIdpersistshrmCategoryIdon the document at import (was derived at display time only)hrmCategoryId IS NULL) to surface reports that matched no configured categorysFTP / Config Hardening
hrmPreferences.jspcluster (hrmKeyUploader.jsp,HRMPreferences2Action,HRMUploadKey2Action, deadSchedulerJob, Struts mappings);configure.jspis the sole config pageFixes & Performance
hrmCategories.jspadmin links; correct context-path on inbox DataTables linksSummary by Sourcery
Strengthen HRM XML validation and parsing, surface detailed upload warnings, introduce a sending-facility registry with UI and unregistered-facility alerting, and persist HRM category mappings at import to enable new inbox filtering and display improvements.
New Features:
Bug Fixes:
Enhancements:
Chores:
Summary by cubic
Adds end-to-end HRM validation and import hardening, a Sending Facility registry with admin tooling, and auto‑categorization at import. Improves upload UX, quality warnings, and sFTP safety, and removes legacy HRM config pages.
New Features
BinaryFileExtension; DOB range checks; per-file results with safe error details; extractedHRMXmlValidator,HRMReportValidator, andHRMProcessingContext(now encapsulates warnings with an unmodifiable view).DeliverToUserIDrole/id issues; duplicates; CPSO-name mismatches; missingSendingFacility/SubClass.HRMSendingFacilitytable/DAO/action/UI; resolve facility names across inbox/report/PDF/mappings; flag/audit unregistered facilities and quick-add; show Report Number; trim whitespace on IDs; prefetch to avoid N+1 queries.hrmCategoryIdat import; new “Unmatched Category” inbox filter.PathValidationUtils, notify on failures, and audit config changes.Migration
database/mysql/updates/update-2026-05-22-hrm-sending-facility.sqlto createHRMSendingFacility.hrmCategoryId; existing reports remain unchanged.Written for commit 052cf0d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Removed