Feature: show providers' public and private documents in the attachment manager#2295
Open
LiamStanziani wants to merge 27 commits into
Open
Feature: show providers' public and private documents in the attachment manager#2295LiamStanziani wants to merge 27 commits into
LiamStanziani wants to merge 27 commits into
Conversation
Reviewer's GuideImplements provider public/private eDoc sections in the attachment manager for consults and eForms, centralizes attached-doc classification, enforces _edoc read security, normalizes document metadata, and adds DAO helpers plus tests around attached-document querying and ctl classification. Sequence diagram for closing attachment dialog with private eDoc warning and delegate syncsequenceDiagram
actor Provider
participant Dialog as attachDocument.jsp
participant Parent as ParentForm_or_Toolbar
Provider->>Dialog: Click OK to close attachment dialog
Dialog->>Dialog: confirmPrivateDocsIfAny('#attachDocumentsForm')
alt one or more new private eDocs selected
Dialog->>Provider: confirm(...) warning about sharing private eDocs
alt Provider cancels
Provider-->>Dialog: Cancel
Dialog-->>Provider: Abort close (return false)
else Provider confirms
Provider-->>Dialog: OK
Dialog->>Dialog: Build deduped selection with attachable_check
Dialog->>Parent: For each checked box create hidden delegate via buildDelegateInput
Dialog->>Parent: Populate entry_name+value rows for attachments
Dialog->>Parent: Remove rows for unchecked _pre_check items
Dialog->>Dialog: Downgrade *_pre_check to *_check and clear data-pre-attached
Dialog-->>Provider: Close dialog
end
else no new private eDocs selected
Dialog->>Dialog: Skip warning
Dialog->>Parent: Sync delegates from checked attachable_check inputs
Dialog->>Parent: Remove delegates for unchecked *_pre_check inputs
Dialog-->>Provider: Close dialog
end
Sequence diagram for fetching consult or eForm documents with provider sectionssequenceDiagram
actor Provider
participant UI as Consult_or_eForm_UI
participant Action as DocumentPreview2Action
participant Util as EDocUtil
participant DAO as CtlDocumentDao
Provider->>UI: Open attachment dialog
UI->>Action: fetchConsultDocuments or fetchEFormDocuments(demographicNo, requestId or fdid)
Action->>Action: LoggedInInfo.getLoggedInInfoFromSession(request)
Action->>Action: securityInfoManager.hasPrivilege(_edoc, r)
alt missing _edoc read privilege
Action-->>UI: SecurityException
UI-->>Provider: Show access denied
else has privilege
Action->>Action: positiveIntParamOrDefault on demographicNo
Action->>Action: positiveIntParamOrNull on requestId or fdid
Action->>Action: populateCommonDocs(loggedInInfo, demographicNo)
Action->>Util: listDocs(demographic, demographicNo, PRIVATE, OBSERVATIONDATE)
Util->>DAO: findDocuments via DocumentDao (implicit)
DAO-->>Util: Document rows
Util-->>Action: allDocuments as EDoc list
Action->>Util: getProviderPrivateDocs(loggedInInfo)
Util->>Util: listDocs(providers, providerNo, PRIVATE, OBSERVATIONDATE)
Util-->>Action: providerPrivateDocs
Action->>Util: getProviderPublicDocs(loggedInInfo)
Util->>Util: listDocs(providers, providerNo, PUBLIC, OBSERVATIONDATE)
Util-->>Action: providerPublicDocs
Action->>Action: populateAttachedContextForConsult or populateAttachedContextForEForm
Action->>Util: listDocs attached for consult or eForm (EDocUtil.ATTACHED)
Util->>DAO: findByDocumentNos for ctl_document
DAO-->>Util: CtlDocument rows
Util->>Util: enrichAttachedWithCtl attach module and moduleId
Util-->>Action: attachedDocs with classification data
Action->>Action: mergeAttachedContext(loggedInInfo, attachedDocs)
Action-->>UI: Forward to attachDocument.jsp with request attributes
UI-->>Provider: Render Patient Documents, Public eDocs, Private eDocs sections
end
Entity relationship diagram for documents and ctl_document classificationerDiagram
DOCUMENT {
int document_no PK
string description
date observationdate
int public1
char status
string contenttype
datetime contentdatetime
}
CTL_DOCUMENT {
int document_no PK
string module PK
int module_id PK
}
PROVIDER {
int provider_no PK
}
DEMOGRAPHIC {
int demographic_no PK
}
DOCUMENT ||--o{ CTL_DOCUMENT : has_binding
PROVIDER ||--o{ CTL_DOCUMENT : provider_binding
DEMOGRAPHIC ||--o{ CTL_DOCUMENT : demographic_binding
%% Classification rules implemented in code:
%% - CTL_DOCUMENT.module in {provider, providers} => provider document
%% - DOCUMENT.public1 = 1 => public provider document
%% - DOCUMENT.public1 = 0 and provider binding => private provider document
%% - No provider binding or module not provider => patient document
Class diagram for updated document attachment and classification logicclassDiagram
class DocumentPreview2Action {
-HttpServletRequest request
-DocumentAttachmentManager documentAttachmentManager
-FormsManager formsManager
-SecurityInfoManager securityInfoManager
-ObjectMapper objectMapper
+String fetchConsultDocuments()
+String fetchEFormDocuments()
-void populateCommonDocs(LoggedInInfo loggedInInfo, String demographicNo)
-void populateAttachedContextForConsult(LoggedInInfo loggedInInfo, String demographicNo, String requestId)
-void populateAttachedContextForEForm(LoggedInInfo loggedInInfo, String demographicNo, String fdid)
-void mergeAttachedContext(LoggedInInfo loggedInInfo, List~EDoc~ attachedDocs)
+static void mergeSingleAttachedDoc(EDoc attachedDoc, String currentProviderNo, List~EDoc~ allDocuments, List~EDoc~ providerPrivateDocs, List~EDoc~ providerPublicDocs, Set~String~ foreignPrivateDocIds)
+static boolean isOwnedByCurrentProvider(EDoc doc, String currentProviderNo)
+static boolean isProviderModule(String module)
-static String positiveIntParamOrNull(String value)
-static String positiveIntParamOrDefault(String value, String defaultValue)
}
class EDocUtil {
-static CtlDocumentDao ctlDocumentDao
+static ArrayList~EDoc~ listDocs(LoggedInInfo loggedInInfo, String module, String moduleId, String docType, String publicDoc, EDocSort sort)
-static ArrayList~EDoc~ listDocs(LoggedInInfo loggedInInfo, boolean attached, String module, String moduleId, String docType, String publicDoc, EDocSort sort, String includeOption)
-static void enrichAttachedWithCtl(List~EDoc~ attachedDocs)
-static boolean preferProvider(CtlDocument candidate, CtlDocument current)
+static List~EDoc~ getProviderPrivateDocs(LoggedInInfo loggedInInfo)
+static List~EDoc~ getProviderPublicDocs(LoggedInInfo loggedInInfo)
+static EDoc getEDocFromDocId(String docId)
}
class CtlDocumentDao {
+List~CtlDocument~ findByDocumentNoAndModule(Integer ctlDocNo, String module)
+List~CtlDocument~ findByDocumentNos(List~Integer~ documentNos)
}
class CtlDocumentDaoImpl {
-EntityManager entityManager
+List~CtlDocument~ findByDocumentNoAndModule(Integer ctlDocNo, String module)
+List~CtlDocument~ findByDocumentNos(List~Integer~ documentNos)
}
class CtlDocument {
+CtlDocumentPK id
+CtlDocumentPK getId()
}
class CtlDocumentPK {
+Integer documentNo
+String module
+Integer moduleId
+Integer getDocumentNo()
+String getModule()
+Integer getModuleId()
}
class EDoc {
+String docId
+String description
+char status
+String observationDate
+String docPublic
+String module
+String moduleId
+String getDocId()
+char getStatus()
+String getModule()
+String getModuleId()
+void setStatus(char status)
+void setObservationDate(String observationDate)
+void setDocPublic(String docPublic)
+void setModule(String module)
+void setModuleId(String moduleId)
}
class LoggedInInfo {
+String getLoggedInProviderNo()
}
class SecurityInfoManager {
+boolean hasPrivilege(LoggedInInfo loggedInInfo, String objectName, String privilege, Object extra)
}
class DocumentAttachmentManager {
+List~AttachmentLabResultData~ getAllLabsSortedByVersions(LoggedInInfo loggedInInfo, String demographicNo)
+List~EFormData~ getAllEFormsExpectFdid(LoggedInInfo loggedInInfo, int demographicNo, int fdid)
}
class FormsManager {
+List~EctFormData.PatientForm~ getEncounterFormsbyDemographicNumber(LoggedInInfo loggedInInfo, int demographicNo, boolean showOnlySaved, boolean includeDeleted)
}
DocumentPreview2Action --> EDocUtil : uses
DocumentPreview2Action --> SecurityInfoManager : checks _edoc privilege
DocumentPreview2Action --> DocumentAttachmentManager : loads labs and eForms
DocumentPreview2Action --> FormsManager : loads encounter forms
DocumentPreview2Action --> EDoc : classifies and merges
DocumentPreview2Action --> LoggedInInfo : gets provider number
EDocUtil --> CtlDocumentDao : calls
EDocUtil --> CtlDocument : enriches attached EDocs
EDocUtil --> EDoc : builds and enriches
CtlDocumentDaoImpl ..|> CtlDocumentDao
CtlDocumentDaoImpl --> CtlDocument : returns entities
CtlDocument --> CtlDocumentPK : embedded id
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughTwo action classes are updated to retrieve provider-scoped private documents and expose them as request attributes. A JSP view is modified to display these provider-private documents in a new UI section alongside existing document listings. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 @LiamStanziani, 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 enhances the attachment manager functionality by introducing a dedicated section for private documents owned by the logged-in provider. This allows providers to easily view and manage their personal documents directly within the attachment interface, improving accessibility and workflow efficiency. Highlights
Changelog
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Collaborator
Author
|
@coderabbitai review |
Collaborator
Author
|
@SourceryAI 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 1 issue, and left some high level feedback:
- The logic to fetch provider private documents in both DocumentPreview2Action.populateCommonDocs and ConsultationAttachDocs2Action.fetchAll is duplicated; consider extracting a shared helper to reduce repetition and keep behavior consistent.
- The 'Show X More Private Documents' button always computes
${providerPrivateDocs.size() - 20}even when size ≤ 20 and only hides via CSS; you may want to render the button and its title text conditionally so the DOM doesn’t contain negative or misleading counts. - The select-all checkbox calls
toggleSelectAll(this, 'providerDocument_');but the individual provider checkboxes use IDs starting withproviderDocNoand classproviderDocument_check; verify and align the selector/prefix so the select-all behavior works for this new section.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic to fetch provider private documents in both DocumentPreview2Action.populateCommonDocs and ConsultationAttachDocs2Action.fetchAll is duplicated; consider extracting a shared helper to reduce repetition and keep behavior consistent.
- The 'Show X More Private Documents' button always computes `${providerPrivateDocs.size() - 20}` even when size ≤ 20 and only hides via CSS; you may want to render the button and its title text conditionally so the DOM doesn’t contain negative or misleading counts.
- The select-all checkbox calls `toggleSelectAll(this, 'providerDocument_');` but the individual provider checkboxes use IDs starting with `providerDocNo` and class `providerDocument_check`; verify and align the selector/prefix so the select-all behavior works for this new section.
## Individual Comments
### Comment 1
<location> `src/main/webapp/documentManager/attachDocument.jsp:381-382` </location>
<code_context>
+ onclick="toggleSelectAll(this, 'providerDocument_');" value="providerDocument_check"
+ title="Select/un-select all private documents."/>
+ <label for="selectAllProviderDocuments">Select all</label>
+ <button class="show-all-button ${providerPrivateDocs.size() > 20 ? '' : 'hide'}"
+ type="button" title="Show ${providerPrivateDocs.size() - 20} More Private Documents"
+ onclick="showAll(this, 'providerDoc')">Show ${providerPrivateDocs.size() - 20} More
+ Private Documents
</code_context>
<issue_to_address>
**issue (bug_risk):** When there are fewer than 20 private documents, the hidden button will still contain a negative count in its text/title.
The button is visually hidden when `providerPrivateDocs.size() <= 20`, but the title and label still compute `size() - 20`, so with 5 docs the DOM contains “Show -15 More Private Documents”. This is misleading for assistive tech and could become visible if styles change. Consider only rendering the button when `size() > 20`, or clamping/omitting the count when `size() <= 20`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 adds the functionality to display a provider's private documents in the attachment manager. The changes in the Java action classes correctly fetch the private documents and pass them to the JSP. The JSP is updated to render these documents in a new section. My review has identified a couple of areas with significant code duplication, both in the Java action classes and in the JSP file. I've left comments with suggestions on how to refactor this duplicated code to improve maintainability and consistency. Addressing these points would make the code cleaner and easier to manage in the future.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/pageUtil/ConsultationAttachDocs2Action.java (1)
80-171:⚠️ Potential issue | 🔴 CriticalMissing
SecurityInfoManager.hasPrivilege()check in action methods.The coding guidelines require all Struts2 actions to include
SecurityInfoManager.hasPrivilege()checks before performing operations, with the security check as the first operation before any business logic. ThefetchAll()method lacks this check. This method accesses sensitive patient health information (labs, documents, forms, eforms, HRM data) without privilege verification, creating a security gap.Add a privilege check at the top of
fetchAll():Proposed fix
public String fetchAll() { + if (!securityInfoManager.hasPrivilege(LoggedInInfo.getLoggedInInfoFromSession(request), "_con", "r", null)) { + throw new SecurityException("missing required sec object (_con)"); + } LoggedInInfo loggedInInfo = LoggedInInfo.getLoggedInInfoFromSession(request);Also inject
SecurityInfoManageras a field:FaxManager faxManager = SpringUtils.getBean(FaxManager.class); +SecurityInfoManager securityInfoManager = SpringUtils.getBean(SecurityInfoManager.class);🤖 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/oscarConsultationRequest/pageUtil/ConsultationAttachDocs2Action.java` around lines 80 - 171, The fetchAll() action is missing the required privilege check; inject a SecurityInfoManager field (e.g., private SecurityInfoManager securityInfoManager; via SpringUtils.getBean or as a class member) and at the very start of fetchAll() call securityInfoManager.hasPrivilege(loggedInInfo, <appropriate-privilege-constant>) (use the same LoggedInInfo obtained by LoggedInInfo.getLoggedInInfoFromSession(request)); if the check fails return the appropriate security result (e.g., "security" or redirect/HTTP 403) immediately before any business logic (before populating labs/docs/forms/HRM) to prevent access to patient data. Ensure the call and early return are placed in ConsultationAttachDocs2Action.fetchAll().
🤖 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/webapp/documentManager/attachDocument.jsp`:
- Line 391: The title attribute is inserting document.description directly into
HTML (title="${ document.description }") creating an XSS risk; change the
attribute to use an encoder such as OWASP Encoder's
Encode.forHtmlAttribute(document.getDescription()) or JSTL
fn:escapeXml(document.description) so the value is HTML-attribute-encoded;
update both occurrences that set title from document.description (the new code
at line ~391 and the pre-existing one around line ~355) to call the encoder
instead of embedding the raw property, keeping the existing <c:out> usage for
label content unchanged.
- Line 389: The private-document checkbox input currently includes the class
"document_check" so toggleSelectAll(this, 'document_') also toggles private
docs; update the input element in attachDocument.jsp (the checkbox with
class="providerDocument_check document_check") to remove the "document_check"
prefix (e.g., use "private_document_check" or just "providerDocument_check") so
it no longer matches the 'document_' selector used by toggleSelectAll; if you
introduce a new class like "private_document_check", ensure any JS that should
target private docs is updated accordingly.
---
Outside diff comments:
In
`@src/main/java/ca/openosp/openo/encounter/oscarConsultationRequest/pageUtil/ConsultationAttachDocs2Action.java`:
- Around line 80-171: The fetchAll() action is missing the required privilege
check; inject a SecurityInfoManager field (e.g., private SecurityInfoManager
securityInfoManager; via SpringUtils.getBean or as a class member) and at the
very start of fetchAll() call securityInfoManager.hasPrivilege(loggedInInfo,
<appropriate-privilege-constant>) (use the same LoggedInInfo obtained by
LoggedInInfo.getLoggedInInfoFromSession(request)); if the check fails return the
appropriate security result (e.g., "security" or redirect/HTTP 403) immediately
before any business logic (before populating labs/docs/forms/HRM) to prevent
access to patient data. Ensure the call and early return are placed in
ConsultationAttachDocs2Action.fetchAll().
… Patient Documents and Provider Documents, might be reverted depending on discussion about this change
…on names in the attachment manager
7 tasks
…Docs(), add OWASP encoding for doc title attributes, fix provider doc attachment save/restore in consultation requests and eForms when switching away from doc class to seperate class
…seperate UI sections, and added a JS warning when attaching a private provider document
…ctions, fix entry ID mismatch for provider doc detach in consultations, add deleted doc styling, and update confirm dialog wording
…n merge method, wrap allDocuments list consistently
LiamStanziani
changed the title
…nvariant, removing fallback resolution as its not needed anymore
LiamStanziani
changed the title
Collaborator
|
@LiamStanziani just want to confirm: this PR is still being worked on, correct? |
Collaborator
Author
|
@sebastian-j-ibanez This PR is waiting for testing by Keith, not currently working it at the moment but I might need to update it later on if something comes up. Since this is in draft I will probably need to update it somewhat when setting it ready to review for AI comments, but I plan to do that after Keith has approved the feature behaviour. |
Collaborator
|
Okay, sounds good! Just wanted to make sure this should be kept open. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/main/webapp/documentManager/attachDocument.jsp" line_range="393" />
<code_context>
- <li class="doc ${loop.index > 19 ? 'hide' : ''}">
- <input class="document_check" type="checkbox" name="docNo"
+ <c:set var="isDeleted" value="${fn:contains(document.status, 'D')}"/>
+ <c:set var="isAttached" value="${not empty attachedDocumentIds and attachedDocumentIds.contains(document.docId)}"/>
+ <li class="doc ${loop.index > 19 ? 'hide' : ''} ${isDeleted ? 'deleted-doc' : ''}">
+ <input class="document_${isAttached ? 'pre_check' : 'check'} attachable_check" type="checkbox" name="docNo"
</code_context>
<issue_to_address>
**issue (bug_risk):** Calling `attachedDocumentIds.contains(...)` in EL is likely invalid and will fail at runtime.
Standard JSP EL doesn’t support calling instance methods like `attachedDocumentIds.contains(...)`, so this is likely to fail with a property/method resolution error at render time. If `attachedDocumentIds` is a `Collection`, use the `in` operator instead (e.g. `${document.docId in attachedDocumentIds}`) or a JSTL function, and update the similar `foreignPrivateDocIds.contains(...)` usage as well.
</issue_to_address>
### Comment 2
<location path="src/test-modern/java/ca/openosp/openo/documentManager/dao/DocumentDaoAttachedFindIntegrationTest.java" line_range="38-47" />
<code_context>
+ @Autowired
+ private CtlDocumentDao ctlDocumentDao;
+
+ @Test
+ @DisplayName("returns an empty list when the input is null")
+ void shouldReturnEmpty_whenInputIsNull() {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a negative test to confirm non-DOC attachment types are excluded
Since the query is intended to return only `docType = DOCTYPE_DOC` rows, consider persisting an attachment for the same consult/fdid/response with a different `docType` (e.g., letter) and asserting it is excluded from the results. This will help catch regressions if the query or `docType` constants change.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ment doc consolidation, updated another provider text to be more specific on state of attachment
Collaborator
Author
|
Testing this has gone very well, should be fully ready for review soon |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
attachDocument.jspand the related JS, there is still a lot of explicit class-based checkbox selection (e.g..document*_check,.providerPrivateDocument*_check, etc.); now that you've introduced the sharedattachable_checkmarker, consider refactoring the selectors and beforeClose handlers to rely on this one class to reduce duplication and avoid divergence when new attachment types/sections are added. - In
mergeAttachedContext, when any ofallDocuments,providerPrivateDocs, orproviderPublicDocsis missing you skip the merge entirely; if this method is ever reused in a context that doesn't load provider lists, it might be safer to still populateattachedDocumentIds/foreignPrivateDocIds(and perhaps only guard the per-section injections) so that attached-state handling continues to work. - The OWASP encoder taglib is referenced via
uri="https://www.owasp.org/index.php/OWASP_Java_Encoder_Project"; double-check that this matches the actual encoder configuration in the project (and OWASP’s recommended URI) to avoid runtime taglib resolution issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `attachDocument.jsp` and the related JS, there is still a lot of explicit class-based checkbox selection (e.g. `.document*_check`, `.providerPrivateDocument*_check`, etc.); now that you've introduced the shared `attachable_check` marker, consider refactoring the selectors and beforeClose handlers to rely on this one class to reduce duplication and avoid divergence when new attachment types/sections are added.
- In `mergeAttachedContext`, when any of `allDocuments`, `providerPrivateDocs`, or `providerPublicDocs` is missing you skip the merge entirely; if this method is ever reused in a context that doesn't load provider lists, it might be safer to still populate `attachedDocumentIds`/`foreignPrivateDocIds` (and perhaps only guard the per-section injections) so that attached-state handling continues to work.
- The OWASP encoder taglib is referenced via `uri="https://www.owasp.org/index.php/OWASP_Java_Encoder_Project"`; double-check that this matches the actual encoder configuration in the project (and OWASP’s recommended URI) to avoid runtime taglib resolution issues.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 14 files
Confidence score: 3/5
- There is moderate merge risk because both reported issues are medium severity (6/10) with high confidence, and each can affect user-visible behavior.
- In
src/main/webapp/documentManager/attachDocument.jsp, inconsistent use ofe:forHtmlAttributefor title attributes (document vs eForm/HRM/Form titles) could leave uneven output encoding and potential attribute-safety gaps. - In
src/main/java/ca/openosp/openo/documentManager/EDocUtil.java, equal-priority multi-bound document selection is non-deterministic, so canonicalCtlDocumentchoice may vary by query row order and cause inconsistent outcomes. - Pay close attention to
src/main/webapp/documentManager/attachDocument.jspandsrc/main/java/ca/openosp/openo/documentManager/EDocUtil.java- align attribute encoding and make canonical Ctl selection deterministic.
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/documentManager/attachDocument.jsp">
<violation number="1" location="src/main/webapp/documentManager/attachDocument.jsp:383">
P2: Inconsistent HTML attribute encoding: the OWASP encoder (`e:forHtmlAttribute`) was applied to document title attributes but not to eForm, HRM, or Form titles on these same modified lines. Encode these consistently to prevent attribute injection.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/documentManager/EDocUtil.java">
<violation number="1" location="src/main/java/ca/openosp/openo/documentManager/EDocUtil.java:538">
P2: Ctl selection for multi-bound documents is non-deterministic because equal-priority bindings fall back to query row order.
(Based on your team's feedback about deterministic canonical CtlDocument selection across attachment paths.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
… avoid potential issue if any document type is null and method returns early, add extra OWASP encoding to titles in attach document JSP
D3V41
reviewed
Apr 23, 2026
…ppropriate helper classes based on functionality
…eCommonDocs to avoid redundant attribute calls
…th provider public and private documents for Ocean, update and add new tests to account for recent changes
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
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/java/ca/openosp/openo/commn/dao/DocumentDaoImpl.java">
<violation number="1" location="src/main/java/ca/openosp/openo/commn/dao/DocumentDaoImpl.java:249">
P1: Validation now treats all provider-module docs as valid attachments, which can allow unauthorized private provider documents to pass patient-attachment checks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
D3V41
approved these changes
Apr 24, 2026
Collaborator
|
QA testing for this looks good. The issues that I previously flagged have been resolved, and Deval reviewed the Ocean-related fix. Waiting for @LiamStanziani to open the MHI PR for this, and we'll get this one merged in after it's tested at MHI. |
…achment-manager Signed-off-by: Liam Stanziani <102267755+LiamStanziani@users.noreply.github.com>
Collaborator
Author
|
@lacarmen sounds good, PR has been opened for this feature towards release here: MagentaHealth#129 |
…on headers via hasClass instead of a reversed indexOf that broke once checkboxes became multi-class
Collaborator
Author
|
@D3V41 I have added a fix for: https://github.com/openo-beta/Open-O/issues/2470 In this branch, it has also been sent towards MagentaHealth release but it is fixed here for consistency: Here is the commit if you would like to review: 2e566db Below is information about the issue, from the corresponding MagentaHealth PRSummaryWhen attaching items to a consultation request via Manage Attachments, labs, forms, eForms, and HRM documents were all displayed under the generic Documents header before the consultation was saved, instead of under their own section headings. They only appeared correctly categorized after saving and reopening. This fixes the pre-save categorization so each item lands under its correct header immediately. ProblemThe JavaScript that decides which section table each attached item goes into used a reversed if ("lab_check".indexOf(element.attr("class")) !== -1) { target = "#attachedLabsTable"; }This asks whether the element's entire class string is a substring of the literal That latent bug was exposed by PR #2295 (provider public/private documents in the attachment manager), which added a second CSS class ( This affected the release branch (which has PR #2295) but not SolutionReplaced the four reversed if (element.hasClass("lab_check")) { target = "#attachedLabsTable"; }
if (element.hasClass("form_check")) { target = "#attachedFormsTable"; }
if (element.hasClass("eForm_check")) { target = "#attachedEFormsTable"; }
if (element.hasClass("hrm_check")) { target = "#attachedHRMDocumentsTable"; }
Documents and provider public/private docs continue to fall through to the default Documents table (unchanged behaviour — they are all documents). Scope: one file, |
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
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.
Summary
Adds provider-owned documents to the attachment manager so providers can attach their own private or public eDocs to consultations and eForms, alongside the existing patient-specific documents.
Closes #2282.
User-facing changes
(attached by another provider)label, so reviewing providers can see that a doc they're looking at belongs to a different provider's library.(deleted)label so they don't silently vanish from saved consults/eForms after a library cleanup.New consultation attachment page:
Cross-provider consultation attachment page (deleted documents, attached documents by other providers):
Warning popup on attachment page when attaching new private provider documents
Note: since both consultation and eForm's use the same base JSP for the attachment manager, I have only shown screenshots based on the consultation flow since attaching both shouldn't show any differences.
Implementation approach
DAO layer — unchanged shape
The 3 attached-doc DAO methods (
findDocsAndConsultDocsByConsultId,findDocsAndEFormDocsByFdid,findDocsAndConsultResponseDocsByConsultId) are unchanged versus develop. Their pre-PR 2-column contract is preserved so the 16 downstream callers (PDF printing, fax, REST APIs, etc.) see no behavioral change.EDocUtil — ctl enrichment for attached docs
New
enrichAttachedWithCtlhelper runs after the DAO returns attached-doc tuples. It does one batch query via a newCtlDocumentDao.findByDocumentNos(List<Integer>)to fetch ctl rows for all attached docs and picks the preferred ctl per doc (provider bindings win over demographic so library docs classify correctly). Orphan docs with no ctl row pass through unclassified and fall through to the patient section.DocumentPreview2Action — attached-state classification
New
mergeAttachedContext/mergeSingleAttachedDocwalk the attached-doc list and classify each into the correct section (patient / provider public / provider private), re-injecting soft-deleted items and flagging cross-provider private docs viaforeignPrivateDocIds.JSP and JS
attachDocument.jspwith select-all + show-more + preview + deleted/foreign markers.<script>block:confirmPrivateDocsIfAny,buildDelegateInput,syncPreCheckedToDelegates— consumed by both the consult JSP and the eForm toolbar.data-delegate-type="doc"attribute on doc delegate inputs so the toolbar JS resolves checkboxes by name+value across the three sections (a plain#docNo<id>selector would only match the patient section).Privacy and PHI considerations
The attachment uses a reference-based model: attaching a provider's private doc to a consult creates a row in
consultdocs(oreform_docs) pointing at the originaldocument_no, not a copy.Key property: there is no reverse lookup in the application code that takes a
document_noand returns every patient/consult/eForm it's attached to. Each forward lookup (DisplayDemographicConsultationRequests, eForm display, etc.) is scoped to a single request/fdid, so one provider document attached to multiple patients' records never becomes cross-patient visibility through app code.Testing
Automated
DocumentDaoAttachedFindIntegrationTest— 14 cases across 3 nested classes covering each attached-doc query: happy path, multi-doc, orphan docs (no ctl row), soft-deleted, scope-by-id, non-DOC docType exclusion.CtlDocumentDaoFindByDocumentNosIntegrationTest— 6 cases for the new batch method: null input, empty input, single doc, multiple docs, doc with multiple ctl rows, scope-by-doc-ids.DocumentPreview2ActionMergeUnitTest— 12 cases across 5 nested classes covering the full matrix ofmergeSingleAttachedDocclassification: patient / public provider / own private / foreign private × active / deleted, plus edge cases (null current provider, legacy "provider" vs "providers" module spelling).Manual smoke tests performed
Covered all scenarios from the spec's "Testing Performed" section plus several derived ones:
Playwright verification
Confirmed the following at the DOM level in a running dev instance:
providerPrivateDoc,foreign-doc,deleted-doc).syncPreCheckedToDelegatescorrectly unchecks/reverts class/stripsdata-pre-attachedwhen delegate is missing, and leaves state alone when delegate is present.foreign-docclass and "(attached by another provider)" label.Design decisions worth noting
document_no, verified viaGROUP BY document_no HAVING COUNT(*) > 1→ 0 rows).