Feature: show providers' documents in the attachment manager#2295
Draft
LiamStanziani wants to merge 8 commits intohotfix/01232026from
Draft
Feature: show providers' documents in the attachment manager#2295LiamStanziani wants to merge 8 commits intohotfix/01232026from
LiamStanziani wants to merge 8 commits intohotfix/01232026from
Conversation
Reviewer's GuideAdds support for showing the logged-in provider’s private documents in the attachment manager for consultations and eForms by fetching provider-specific private docs server-side and rendering a new selectable, paginated “Private Documents” section in the JSP. Sequence diagram for loading provider private documents in attachment managersequenceDiagram
actor Provider
participant Browser
participant ConsultationAttachDocs2Action
participant DocumentPreview2Action
participant EDocUtil
participant JSP_attachDocument
Provider->>Browser: Open attachment manager (consultation or eForm)
Browser->>ConsultationAttachDocs2Action: HTTP GET /consultation/attachDocs
ConsultationAttachDocs2Action->>EDocUtil: listDocs(loggedInInfo, demographic, demographicNo, null, PRIVATE, OBSERVATIONDATE)
EDocUtil-->>ConsultationAttachDocs2Action: allDocuments
ConsultationAttachDocs2Action->>EDocUtil: listDocs(loggedInInfo, providers, providerNo, null, PRIVATE, OBSERVATIONDATE)
EDocUtil-->>ConsultationAttachDocs2Action: providerPrivateDocs
ConsultationAttachDocs2Action->>ConsultationAttachDocs2Action: build attachedDocumentIds
ConsultationAttachDocs2Action->>JSP_attachDocument: Forward with allDocuments, providerPrivateDocs, allLabs, allForms, allEForms
alt eForm preview flow
Provider->>Browser: Open attachment manager from eForm
Browser->>DocumentPreview2Action: HTTP GET /document/preview
DocumentPreview2Action->>EDocUtil: listDocs(loggedInInfo, demographic, demographicNo, null, PRIVATE, OBSERVATIONDATE)
EDocUtil-->>DocumentPreview2Action: allDocuments
DocumentPreview2Action->>EDocUtil: listDocs(loggedInInfo, providers, providerNo, null, PRIVATE, OBSERVATIONDATE)
EDocUtil-->>DocumentPreview2Action: providerPrivateDocs
DocumentPreview2Action->>JSP_attachDocument: Forward with allDocuments, providerPrivateDocs, allHRMDocuments, allLabsSortedByVersions, allForms
end
JSP_attachDocument-->>Browser: Render attachment manager with Private Documents section
Provider->>Browser: Select and preview provider private documents
Browser->>DocumentPreview2Action: getPdf(DOC, docId, renderEDocPDF)
DocumentPreview2Action-->>Browser: PDF response
Class diagram for actions populating provider private documentsclassDiagram
class ConsultationAttachDocs2Action {
+String fetchAll()
-HttpServletRequest request
-DocumentAttachmentManager documentAttachmentManager
-FormsManager formsManager
-EFormManager eFormManager
-LoggedInInfo loggedInInfo
-String demographicNo
-String requestId
-List~EDoc~ allDocuments
-List~EDoc~ providerPrivateDocs
-List~EDoc~ attachedDocuments
}
class DocumentPreview2Action {
-HttpServletRequest request
-DocumentAttachmentManager documentAttachmentManager
-FormsManager formsManager
-void populateCommonDocs(LoggedInInfo loggedInInfo, String demographicNo)
-List~EDoc~ allDocuments
-List~EDoc~ providerPrivateDocs
-List~AttachmentLabResultData~ allLabsSortedByVersions
-List~EctFormData.PatientForm~ allForms
}
class EDocUtil {
+List~EDoc~ listDocs(LoggedInInfo loggedInInfo, String type, String ownerId, String requestId, int visibility, EDocSort sort)
<<static>>
+int PRIVATE
+EDocSort EDocSort
}
class JSP_attachDocument {
+allDocuments
+providerPrivateDocs
+allLabs
+allForms
+allEForms
+allHRMDocuments
+allLabsSortedByVersions
+renderPrivateDocumentsSection()
}
ConsultationAttachDocs2Action --> EDocUtil : uses listDocs demographic
ConsultationAttachDocs2Action --> EDocUtil : uses listDocs providers
ConsultationAttachDocs2Action --> JSP_attachDocument : sets providerPrivateDocs
DocumentPreview2Action --> EDocUtil : uses listDocs demographic
DocumentPreview2Action --> EDocUtil : uses listDocs providers
DocumentPreview2Action --> JSP_attachDocument : sets providerPrivateDocs
File-Level Changes
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 | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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. |
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.
NOTE: THIS DESCRIPTION AND FUNCTIONALITY IS IN PROGRESS, THE DESCRIPTION HIGHLIGHTS THE OLD VERSION OF THIS FEATURE, AND WILL BE UPDATED LATER WHEN THE FULL IMPLEMENTATION IS APPROVED
In this PR, I have:
I have tested this by:
Summary by Sourcery
Expose provider-specific private documents in the attachment manager for both consultation requests and general document previews.
New Features:
Enhancements:
Summary by cubic
Shows the logged-in provider’s private documents and all public provider documents in the attachment manager for consultations and eForms. Adds “Private Provider Documents,” “Public Provider Documents,” renames “Documents” to “Patient Documents,” and surfaces soft-deleted attachments.
New Features
Bug Fixes
Written for commit cccb179. Summary will update on new commits.
Summary by CodeRabbit
Release Notes