Fix: missing meds when saving - LEGACY BUG#2261
Draft
LiamStanziani wants to merge 12 commits intohotfix/01232026from
Draft
Fix: missing meds when saving - LEGACY BUG#2261LiamStanziani wants to merge 12 commits intohotfix/01232026from
LiamStanziani wants to merge 12 commits intohotfix/01232026from
Conversation
…eeded for fixes to this issue when reproducing the workflow
…ate as needed for fixes to this issue when reproducing the workflow" This reverts commit 8d3b9b7.
…providers, will need to update to iron out bugs and less memory usage
…ng the demographicNo
…working fully, but works for most actions inside of the Medications module
… account for the changes
…icNo and there was no per-patient session bean for the connected demographicNo, meaning a bean was never created in the first place
Reviewer's GuideImplements per-patient Rx session management and multi-tab safety for prescriptions by keying RxSessionBean off demographicNo, introducing a servlet filter and client-side interceptor to propagate demographicNo across requests, and tightening error handling around the Rx stash and prescription updates. Sequence diagram for multi-tab Rx request routing via RxSessionFilter and rxSessionInterceptor.jssequenceDiagram
actor Provider
participant TabA as BrowserTab_A(demographicNo_101)
participant TabB as BrowserTab_B(demographicNo_202)
participant Interceptor as rxSessionInterceptor_js
participant Filter as RxSessionFilter
participant Session as HttpSession
participant Bean101 as RxSessionBean_101
participant Bean202 as RxSessionBean_202
participant Action as Rx_action_or_JSP
Provider->>TabA: Open Medications for patient 101
TabA->>Interceptor: Load page scripts
Interceptor-->>TabA: Attach demographicNo=101 to AJAX/forms
Provider->>TabB: Open Medications for patient 202
TabB->>Interceptor: Load page scripts
Interceptor-->>TabB: Attach demographicNo=202 to AJAX/forms
Note over TabA: Save prescription for patient 101
TabA->>Filter: POST /oscarRx/... demographicNo=101
Filter->>Session: lookup RxSessionBean_101 via RxSessionBean.getSessionKey
alt bean exists
Session-->>Filter: Bean101
else create new
Filter->>Session: create Bean101 and store under per-patient key
end
Filter->>Session: set legacy RxSessionBean = Bean101
Filter->>Action: continue filterChain
Action->>Session: getAttribute(RxSessionBean)
Action-->>Action: work in context of patient 101
Note over TabB: Save prescription for patient 202
TabB->>Filter: POST /oscarRx/... demographicNo=202
Filter->>Session: lookup RxSessionBean_202
Session-->>Filter: Bean202
Filter->>Session: set legacy RxSessionBean = Bean202
Filter->>Action: continue filterChain
Action->>Session: getAttribute(RxSessionBean)
Action-->>Action: work in context of patient 202
Class diagram for updated RxSessionBean, RxSessionFilter, and RxWriteScript2ActionclassDiagram
class RxSessionBean {
<<Serializable>>
- Logger logger
- static String SESSION_KEY_PREFIX
- static String LEGACY_SESSION_KEY
- String providerNo
- int demographicNo
+ static String getSessionKey(int demographicNo)
+ static RxSessionBean getFromSession(HttpSession session, int demographicNo)
+ static RxSessionBean getFromSession(HttpServletRequest request, int demographicNo)
+ static void saveToSession(HttpSession session, RxSessionBean bean)
+ static void saveToSession(HttpServletRequest request, RxSessionBean bean)
+ String getProviderNo()
+ void setProviderNo(String providerNo)
+ int getDemographicNo()
+ void setDemographicNo(int demographicNo)
}
class RxSessionFilter {
<<Filter>>
- Logger logger
- static String LEGACY_KEY
+ void init(FilterConfig filterConfig)
+ void doFilter(ServletRequest req, ServletResponse res, FilterChain chain)
+ void destroy()
- static int parseDemographicNo(String param)
}
class RxWriteScript2Action {
+ String updateDrug()
+ int getDemographicNo()
+ void setDemographicNo(String RHS)
}
class rxSessionInterceptor_js {
<<JavaScript_module>>
- String demoNo
+ void hasDemographicNo(params)
+ void addDemographicNo(params)
+ void processAllForms()
}
RxSessionFilter --> RxSessionBean : uses
RxWriteScript2Action --> RxSessionBean : uses
rxSessionInterceptor_js --> RxSessionFilter : supplies_demographicNo_param
rxSessionInterceptor_js ..> RxSessionBean : indirect_support
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis pull request refactors the prescription system's session management to support multi-patient/multi-tab scenarios by implementing per-demographic-number session keys instead of a single shared session attribute. A new servlet filter (RxSessionFilter) and utility methods in RxSessionBean manage dual-key storage for backward compatibility. Client-side changes propagate the demographicNo parameter through AJAX calls and forms via a new JavaScript interceptor. Several Java actions are updated to use the new session utilities, and multiple JSP pages are modified to pass and use demographicNo context. Additional defensive checks handle null prescriptions and validate parameters. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 significantly enhances the prescription management module by introducing a comprehensive per-patient session handling mechanism. This architectural improvement addresses data integrity concerns and improves the user experience by allowing the simultaneous management of prescriptions for multiple patients across different browser tabs. The solution integrates both server-side session filtering and client-side JavaScript interception to ensure accurate and consistent data handling. 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
|
![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 significant and well-designed enhancement for handling prescription sessions on a per-patient basis, which is crucial for supporting multi-tab workflows. The use of a servlet filter, session bean helpers with backward compatibility, and a client-side AJAX interceptor is a solid approach. My review has identified a recurring high-severity issue: several new code paths parse request parameters into integers without proper error handling for NumberFormatException. This could lead to unhandled exceptions and server errors. I have provided specific comments and code suggestions to address this by adding robust try-catch blocks.
src/main/java/ca/openosp/openo/casemgmt/web/CaseManagementView2Action.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/prescript/pageUtil/RxChoosePatient2Action.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/prescript/pageUtil/RxShowAllergy2Action.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/prescript/pageUtil/RxWriteScript2Action.java
Outdated
Show resolved
Hide resolved
Collaborator
Author
|
@copilot review |
|
@LiamStanziani I've opened a new pull request, #2262, to work on those changes. Once the pull request is ready, I'll request review from you. |
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In `@src/main/java/ca/openosp/openo/casemgmt/web/CaseManagementView2Action.java`:
- Around line 458-467: The code currently calls Integer.parseInt(demoNo) without
handling NumberFormatException; wrap the parse in a safe guard (e.g., check
demoNo for null/empty and digits or use a try/catch) inside the method in
CaseManagementView2Action before calling
RxSessionBean.getFromSession/saveToSession; on parse failure, either use a
sensible default demographicNo (e.g., 0) or skip creating/saving the
RxSessionBean and log a warning with the invalid demoNo so the request does not
throw a 500. Ensure you still call RxSessionBean.setProviderNo and save only
when a valid demographicNo was obtained.
In `@src/main/java/ca/openosp/openo/prescript/data/RxPrescriptionData.java`:
- Around line 1650-1654: The code calls getSpecial() which may log raw PHI;
instead read the underlying field directly in RxPrescriptionData (avoid invoking
getSpecial()) or add a non-logging accessor (e.g., getSpecialRaw()) and use that
to compute length for the warning; ensure the log message only includes the
length (no content) and if you must log any sanitized value use Encode.forJava()
to prevent log injection.
In
`@src/main/java/ca/openosp/openo/prescript/pageUtil/RxChoosePatient2Action.java`:
- Around line 85-96: The code calls Integer.parseInt(this.getDemographicNo())
without validating the input, which will throw if getDemographicNo() is null or
not numeric; update the method in RxChoosePatient2Action to first check that
this.getDemographicNo() is non-null and matches a numeric pattern (or try
parsing inside a try/catch), and if invalid return the appropriate error view
("error.html" or "Logout"); then only call Integer.parseInt and proceed to use
RxSessionBean.getFromSession(request, demographicNoInt) and
RxSessionBean.saveToSession(request, bean) when the demographicNo is valid.
In `@src/main/java/ca/openosp/openo/prescript/pageUtil/RxSessionBean.java`:
- Around line 47-112: The public session helper methods (getSessionKey,
getFromSession(HttpSession,int), getFromSession(HttpServletRequest,int),
saveToSession(HttpSession,RxSessionBean),
saveToSession(HttpServletRequest,RxSessionBean)) need complete JavaDoc: add
explicit `@param` entries specifying types (e.g., "@param demographicNo int",
"@param session HttpSession", "@param request HttpServletRequest", "@param bean
RxSessionBean"), add precise `@return` tags (e.g., "@return RxSessionBean or null"
or "@return String" for getSessionKey), add `@throws` tags for any
unchecked/checked exceptions the methods may surface (or explicitly state none),
and add `@since` tags populated from git history for each public method; update
the JavaDoc blocks above each referenced method accordingly.
In `@src/main/java/ca/openosp/openo/prescript/pageUtil/RxSessionFilter.java`:
- Around line 36-95: Add comprehensive JavaDoc to the public class
RxSessionFilter and its public methods init(FilterConfig),
doFilter(ServletRequest, ServletResponse, FilterChain), and destroy(): document
each `@param` with exact types (e.g., `@param` filterConfig
javax.servlet.FilterConfig), include `@throws` tags for
ServletException/IOException where applicable, add `@since` tags using the
commit/date when the class was introduced, and include a brief description of
behavior and return type (where applicable — void) for each method; ensure the
class-level JavaDoc also lists the purpose and the LEGACY_KEY behavior
referenced in doFilter.
- Around line 81-83: The logger.warn in RxSessionFilter currently logs
demographicNo (PHI) and the raw request URI; remove demographicNo from the log
message and sanitize the URI before logging. Update the logging call in
RxSessionFilter (the logger.warn that references demographicNo and
request.getRequestURI()) to omit demographicNo entirely and pass the request URI
through a safe sanitizer such as Encode.forJava() (or your project's approved
sanitizer) before including it in the log message to prevent log injection and
PHI exposure. Ensure only non-PHI, sanitized values are logged and adjust the
log text accordingly.
In `@src/main/java/ca/openosp/openo/prescript/pageUtil/RxShowAllergy2Action.java`:
- Around line 175-190: The code in RxShowAllergy2Action currently uses
Integer.parseInt(demo_no) which can throw NumberFormatException; update the
handler to validate demo_no (e.g., check null/empty and digits or use try/catch
around Integer.parseInt) and on invalid input set the response status to
SC_BAD_REQUEST (or return "failure") instead of allowing a 500; preserve the
existing RxSessionBean creation and RxSessionBean.saveToSession logic when the
parse succeeds, and ensure any NumberFormatException branch skips bean creation
and exits early with the bad-request response.
In `@src/main/java/ca/openosp/openo/prescript/pageUtil/RxWriteScript2Action.java`:
- Around line 813-821: The logger.error call in RxWriteScript2Action logs the
user-supplied randomId directly; sanitize it using Encode.forJava(randomId) and
switch to parameterized logging (e.g., logger.error("Prescription not found in
stash for randomId: {}", Encode.forJava(randomId))) so the input is encoded and
not concatenated into the message; update the logger invocation around the
null-check for rx and add the necessary import for Encode.
- Around line 1563-1570: Add comprehensive JavaDoc to the public setter
setDemographicNo in class RxWriteScript2Action describing its purpose, include a
typed `@param` tag (e.g., `@param` RHS String - numeric string to parse into an
int), a `@since` tag with the appropriate git-history date, and document behavior
on invalid input (note that NumberFormatException is caught and the default
remains 0) — include an `@throws` tag only if the method is changed to propagate
exceptions; otherwise explicitly state in the JavaDoc that invalid numeric input
is swallowed and no exception is thrown.
In `@src/main/webapp/oscarRx/js/rxSessionInterceptor.js`:
- Around line 21-38: The demographic injection functions must handle FormData
and URLSearchParams explicitly: in addDemographicNo (and correspondingly
hasDemographicNo if needed) detect instances of FormData and URLSearchParams
(e.g., params instanceof FormData / params instanceof URLSearchParams) and use
their APIs instead of treating them as plain objects—call
formData.append('demographicNo', demoNo) for FormData and
urlSearchParams.set('demographicNo', demoNo) for URLSearchParams; preserve
existing behavior for strings (append as query param) and plain objects (assign
params.demographicNo = demoNo), and ensure hasDemographicNo checks FormData.has
and URLSearchParams.has or inspects string/query properly so injection isn't
skipped.
In `@src/main/webapp/oscarRx/SearchDrug3.jsp`:
- Around line 238-243: The demographicNo is being output unencoded in
JavaScript, HTML attributes, and URI contexts; add the import
org.owasp.encoder.Encode at the top and replace direct uses of
rxSessionBean.getDemographicNo() with context-appropriate encoding: wrap it with
Encode.forJavaScript(...) when assigning the JavaScript variable
currentDemographicNo, use Encode.forHtmlAttribute(...) where demographicNo is
inserted into hidden input value attributes, and use Encode.forUriComponent(...)
where demographicNo is concatenated into query parameter strings (search for
currentDemographicNo, the hidden input value usages, and the methods/builders
that append demographicNo to query URIs to update each).
In `@src/main/webapp/oscarRx/ViewScript.jsp`:
- Around line 322-325: The RxClearPendingForm POST is missing the CSRF token;
update the form named "RxClearPendingForm" (action "/oscarRx/clearPending.do")
to include a hidden input using the Spring CSRF parameters: add an input with
name="${_csrf.parameterName}" and value="${_csrf.token}" alongside the existing
hidden fields (e.g., near the demographicNo input) so the form submits the CSRF
token.
- Around line 171-176: Import the OWASP encoder by adding <%@ page
import="org.owasp.encoder.Encode" %> and then apply context-appropriate encoding
to demographicNo: replace the unencoded embed that sets the JavaScript variable
currentDemographicNo (variable currentDemographicNo in ViewScript.jsp) with
Encode.forJavaScript(bean.getDemographicNo()), encode the URL parameter usage
(the code that builds the src or query param around rxSessionInterceptor.js or
other URL at line ~317) with Encode.forUriComponent(bean.getDemographicNo()),
and encode any HTML attribute usage (the attribute at ~324) with
Encode.forHtmlAttribute(bean.getDemographicNo()); finally, add the missing CSRF
hidden input to the POST form (the form around line ~322) using <input
type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/> so the form
includes the CSRF token.
In `@src/main/webapp/oscarRx/ViewScript2.jsp`:
- Around line 675-678: The RxClearPendingForm POST form (form name
"RxClearPendingForm", action "/oscarRx/clearPending.do") is missing a CSRF
token; add a hidden input using the application's CSRF attributes (e.g. <input
type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/> or the
project's equivalent) inside the form (near the other hidden inputs like
"action" and "demographicNo") so the server can validate the token on
submission.
- Around line 238-243: currentDemographicNo is being output unencoded in
multiple contexts causing XSS/CSRF issues; replace all direct uses of
bean.getDemographicNo() with context-appropriate OWASP encoders: wrap the value
in Encode.forJavaScript() when assigning the JavaScript variable
currentDemographicNo (the script block that sets var currentDemographicNo), use
Encode.forUriComponent() when appending demographicNo into URLs (the places
building query params), and use Encode.forHtmlAttribute() when inserting
demographicNo into HTML attributes; update the other occurrences of
demographicNo (the remaining usages in this JSP) similarly to their contexts,
and add a hidden CSRF token input to the POST form (include the server-side CSRF
token variable provided by the app framework) so the form submits the CSRF
token.
🧹 Nitpick comments (4)
src/main/webapp/oscarRx/js/rxSessionInterceptor.js (1)
41-68: Use camelCase for local aliases.
RenameOriginalRequest/OriginalUpdaterto camelCase to match JS naming conventions.As per coding guidelines: **/*.{java,js,ts,tsx}: Use camelCase for variable and method names (e.g., `ticklerManager`, `isPatientActive()`, `getLastUpdateDate()`).♻️ Suggested refactor
- var OriginalRequest = Ajax.Request; + var originalRequest = Ajax.Request; Ajax.Request = function(url, options) { @@ - return new OriginalRequest(url, options); + return new originalRequest(url, options); }; - Ajax.Request.prototype = OriginalRequest.prototype; + Ajax.Request.prototype = originalRequest.prototype; @@ - var OriginalUpdater = Ajax.Updater; + var originalUpdater = Ajax.Updater; Ajax.Updater = function(container, url, options) { @@ - return new OriginalUpdater(container, url, options); + return new originalUpdater(container, url, options); }; - Ajax.Updater.prototype = OriginalUpdater.prototype; + Ajax.Updater.prototype = originalUpdater.prototype;src/main/webapp/oscarRx/ListDrugs.jsp (1)
69-95: Move demographicNo parsing out of JSP scriptlets.
Consider resolving the demographic context in a controller and passing it via request attributes/JSTL to keep the view logic minimal.As per coding guidelines: JSP views should use JSTL tags and avoid Java scriptlets - use request.getAttribute() for passing data.
src/main/webapp/messenger/generatePreviewPDF.jsp (2)
410-423: RenameRxbeanto camelCaserxBean.
Aligns with Java naming conventions.As per coding guidelines: **/*.{java,js,ts,tsx}: Use camelCase for variable and method names (e.g., `ticklerManager`, `isPatientActive()`, `getLastUpdateDate()`).♻️ Suggested refactor
- RxSessionBean Rxbean = RxSessionBean.getFromSession(request.getSession(), demoNoInt); + RxSessionBean rxBean = RxSessionBean.getFromSession(request.getSession(), demoNoInt); - if (Rxbean == null) { - Rxbean = new RxSessionBean(); + if (rxBean == null) { + rxBean = new RxSessionBean(); } @@ - Rxbean.setProviderNo((String) request.getSession().getAttribute("user")); - Rxbean.setDemographicNo(demoNoInt); + rxBean.setProviderNo((String) request.getSession().getAttribute("user")); + rxBean.setDemographicNo(demoNoInt); @@ - RxSessionBean.saveToSession(request.getSession(), Rxbean); + RxSessionBean.saveToSession(request.getSession(), rxBean);
410-423: Move Rx session setup out of JSP scriptlets.
Prefer resolving RxSessionBean in a controller and passing it via request attributes/JSTL.As per coding guidelines: JSP views should use JSTL tags and avoid Java scriptlets - use request.getAttribute() for passing data.
src/main/java/ca/openosp/openo/casemgmt/web/CaseManagementView2Action.java
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/prescript/data/RxPrescriptionData.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/openosp/openo/prescript/pageUtil/RxChoosePatient2Action.java
Outdated
Show resolved
Hide resolved
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||
![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 2 issues, and left some high level feedback:
- In RxWriteScript2Action.updateDrug, when returning the JSON error responses you should explicitly set the response Content-Type (e.g., application/json with UTF‑8) and use a consistent charset when writing bytes instead of relying on defaults.
- The heuristics in rxSessionInterceptor.js for adding demographicNo (string checks for 'Rx' in form action and only checking for 'demographicNo=' in raw strings) are quite broad and could affect unrelated forms or miss encoded params; consider tightening the action matching and using proper querystring parsing instead of substring checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In RxWriteScript2Action.updateDrug, when returning the JSON error responses you should explicitly set the response Content-Type (e.g., application/json with UTF‑8) and use a consistent charset when writing bytes instead of relying on defaults.
- The heuristics in rxSessionInterceptor.js for adding demographicNo (string checks for 'Rx' in form action and only checking for 'demographicNo=' in raw strings) are quite broad and could affect unrelated forms or miss encoded params; consider tightening the action matching and using proper querystring parsing instead of substring checks.
## Individual Comments
### Comment 1
<location> `src/main/webapp/oscarRx/ViewScript.jsp:384-387` </location>
<code_context>
<td><span><input type=button
value="<fmt:setBundle basename="oscarResources"/><fmt:message key="ViewScript.msgBackToOscar"/>"
class="ControlPushButton" style="width: 200px"
- onClick="javascript:clearPending('close');"/></span></td>
+ onClick="resetStash();javascript:clearPending('close');"/></span></td>
</tr>
</code_context>
<issue_to_address>
**issue (bug_risk):** Calling resetStash() here will throw a JS error because it is not defined on this page.
On ViewScript2.jsp `resetStash()` is defined locally, but on ViewScript.jsp it isn’t present and there’s no script import that provides it. As written, clicking “Back to Oscar” will throw `ReferenceError: resetStash is not defined`. To keep the stash-clearing behavior, either define `resetStash` on this page, move it into a shared JS file, or reuse an existing function that already clears the stash.
</issue_to_address>
### Comment 2
<location> `src/main/java/ca/openosp/openo/prescript/pageUtil/RxWriteScript2Action.java:811-819` </location>
<code_context>
String randomId = request.getParameter("randomId");
- RxPrescriptionData.Prescription rx = bean.getStashItem2(Integer.parseInt(randomId));
- // get prescript from randomId
+ int randomIdInt;
+ try {
+ randomIdInt = Integer.parseInt(randomId);
+ } catch (NumberFormatException e) {
+ logger.error("Invalid randomId parameter: {}", Encode.forJava(randomId));
+ response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
+ ObjectNode errorResponse = objectMapper.createObjectNode();
+ errorResponse.put("error", "Invalid prescription identifier.");
+ response.getOutputStream().write(errorResponse.toString().getBytes());
+ return null;
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Error JSON responses should set content type and charset to avoid client-side parsing issues.
In the invalid `randomId` (and similar `rx == null`) branches you write a JSON error body to the `ServletOutputStream` without setting a `Content-Type`. This can prevent some clients or intermediaries from treating it as JSON and may introduce charset issues. Please set `response.setContentType("application/json;charset=UTF-8");` before writing the body and encode with `getBytes(StandardCharsets.UTF_8)` (or use a writer).
Suggested implementation:
```java
logger.error("Invalid randomId parameter: {}", Encode.forJava(randomId));
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
ObjectNode errorResponse = objectMapper.createObjectNode();
errorResponse.put("error", "Invalid prescription identifier.");
response.setContentType("application/json;charset=UTF-8");
response.getOutputStream().write(errorResponse.toString().getBytes(StandardCharsets.UTF_8));
return null;
```
```java
logger.error("Prescription not found in stash for randomId: {}. " +
"Session may have been reset or prescription was not properly staged.",
Encode.forJava(randomId));
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
ObjectNode errorResponse = objectMapper.createObjectNode();
errorResponse.put("error", "Prescription not found. Please refresh and try again.");
response.setContentType("application/json;charset=UTF-8");
response.getOutputStream().write(errorResponse.toString().getBytes(StandardCharsets.UTF_8));
return null;
```
1. Ensure `java.nio.charset.StandardCharsets` is imported at the top of `RxWriteScript2Action.java`, e.g.:
`import java.nio.charset.StandardCharsets;`
2. If there are other JSON error branches in this class that write directly to the response without setting a content type or charset, apply the same `setContentType("application/json;charset=UTF-8")` and `getBytes(StandardCharsets.UTF_8)` pattern there for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/main/java/ca/openosp/openo/prescript/pageUtil/RxWriteScript2Action.java
Outdated
Show resolved
Hide resolved
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 15 files
Confidence score: 3/5
- Multi-tab sessions can submit the wrong patient ID because
src/main/webapp/oscarRx/SearchDrug3.jspstill uses the legacy sessionpatientobject instead of the per-tab RxSessionBean demographicNo. src/main/java/ca/openosp/openo/prescript/pageUtil/RxSessionFilter.javacan leave a stale legacy bean when demographicNo is provided but no per-patient bean exists, risking cross-patient data bleed.- These are concrete, user-facing behaviors around patient context and navigation, so there’s some regression risk despite otherwise localized changes.
- Pay close attention to
src/main/webapp/oscarRx/SearchDrug3.jsp,src/main/java/ca/openosp/openo/prescript/pageUtil/RxSessionFilter.java,src/main/webapp/oscarRx/ViewScript.jsp- patient context isolation and client-side navigation errors.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/main/webapp/oscarRx/SearchDrug3.jsp">
<violation number="1" location="src/main/webapp/oscarRx/SearchDrug3.jsp:796">
P1: This hidden input still uses the session `patient` object instead of the per-tab RxSessionBean demographicNo, so multi-tab sessions can submit the wrong patient ID. Use `demoNo` (from RxSessionBean) to keep requests tied to the correct tab.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/prescript/pageUtil/RxSessionFilter.java">
<violation number="1" location="src/main/java/ca/openosp/openo/prescript/pageUtil/RxSessionFilter.java:78">
P1: When a demographicNo is provided but no per-patient bean exists, the legacy session bean is left untouched, so downstream code can still read a stale bean from a different patient. Clear the legacy key (or set it to null) in this case to avoid cross-patient leakage.</violation>
</file>
<file name="src/main/webapp/oscarRx/ViewScript.jsp">
<violation number="1" location="src/main/webapp/oscarRx/ViewScript.jsp:392">
P2: resetStash() is not defined for this page, so clicking “Back to Oscar” will throw a ReferenceError and can block clearPending from executing. Either define/resetStash for this page or guard the call.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Collaborator
|
@LiamStanziani Reviewed it. Looks good. |
Collaborator
Author
|
Thanks for the review! |
LiamStanziani
changed the title
yingbull
pushed a commit
to carlos-emr/carlos
that referenced
this pull request
Feb 10, 2026
Implements isolated session management for the prescription (Rx) module to prevent medication data cross-contamination when providers have multiple patient tabs open simultaneously. Core changes: - Add static helpers to RxSessionBean (getFromSession/saveToSession) for per-patient session keying using RxSessionBean_<demographicNo> - Create RxSessionFilter servlet filter to swap correct per-patient bean into legacy "RxSessionBean" session key per request - Create rxSessionInterceptor.js to inject demographicNo into AJAX calls and form submissions for Prototype.js, jQuery, and native forms - Update all Rx Action classes to use new per-patient session API - Add demographicNo propagation to ViewScript/ViewScript2 iframes and forms - Improve error handling in RxWriteScript2Action for missing prescriptions - Update ListDrugs.jsp to resolve patient from request parameter first Adapted from openo-beta/open-o PR #2261 by LiamStanziani. Original commits: 8d3b9b7..be7f6576 (squashed, path/namespace translated) Upstream PR: openo-beta/Open-O#2261 Changes from upstream: - File paths translated: ca/openosp/openo/ -> io/github/carlos_emr/carlos/ - Package names translated: ca.openosp.openo. -> io.github.carlos_emr.carlos. - RxPrescriptionData.java changes already present in CARLOS (skipped) - struts.xml whitespace-only change skipped - Adapted to existing CARLOS codebase divergences https://claude.ai/code/session_01Jg6sucQ6C6PgndyCrCHqs1
yingbull
pushed a commit
to carlos-emr/carlos
that referenced
this pull request
Feb 25, 2026
…contamination
Port of openo-beta/open-o PR #2261 ("Fix: missing meds when saving - LEGACY BUG")
originally authored by LiamStanziani.
When multiple prescription tabs are open simultaneously for different patients,
a single shared "RxSessionBean" session attribute causes medications to disappear
or mix between patients. This fix introduces per-patient session keys
(RxSessionBean_<demographicNo>) with a servlet filter that swaps the correct bean
into the legacy key per request, maintaining backward compatibility.
Changes:
- RxSessionBean: Add static getFromSession/saveToSession methods with per-patient keys
- RxSessionFilter: New servlet filter for per-request bean swapping (web.xml mapped)
- rxSessionInterceptor.js: AJAX interceptor injecting demographicNo into all Rx requests
- CaseManagementView2Action, RxChoosePatient2Action, RxShowAllergy2Action: Use new
session methods instead of direct session attribute access
- RxWriteScript2Action: Add error handling for randomId parsing, String demographicNo setter
- ListDrugs.jsp: Multi-tab patient lookup from request parameter
- SearchDrug3.jsp, ViewScript.jsp, ViewScript2.jsp: Include interceptor, add resetStash,
pass demographicNo through forms and iframes
- generatePreviewPDF.jsp: Use getFromSession/saveToSession
Original PR: openo-beta/Open-O#2261
Original Author: LiamStanziani
https://claude.ai/code/session_01D1MB9CvCDC4XaJGCXgxtB3
Collaborator
Author
|
See latest comment on this PR for an explanation as to why this is being set back to draft: MagentaHealth#75 |
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.
In this PR, I have fixed:
I have tested this by:
User description
Summary by Sourcery
Implement per-patient prescription session handling and multi-tab safety across the Rx workflow to keep medications tied to the correct patient and avoid losing staged prescriptions.
Bug Fixes:
Enhancements:
Summary by cubic
Fixes medications disappearing or mixing between patients by adding per‑patient Rx session handling and multi‑tab safety across the prescription workflow. All Rx requests now carry demographicNo so the correct patient context is always used.
New Features
Bug Fixes
Written for commit 13084e2. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
PR Type
Bug fix, Enhancement
Description
Implement per-patient RxSessionBean storage with demographicNo-based session keys to prevent medication loss across multiple patient tabs
Add RxSessionFilter servlet filter to swap correct patient bean per request, enabling multi-tab prescription safety
Introduce rxSessionInterceptor.js to inject demographicNo into AJAX calls, forms, and iframes for consistent patient context
Improve error handling in prescription workflows with clearer validation and error messages for missing/invalid data
Diagram Walkthrough
File Walkthrough
12 files
Add per-patient session key helpers and storage methodsNew servlet filter for multi-patient tab session isolationUpdate to use per-patient RxSessionBean retrievalRefactor bean initialization with per-patient session supportUpdate RxSessionBean initialization with per-patient keysSimplify special instructions logging with clearer formatNew AJAX interceptor to propagate demographicNo across requestsEnhance patient data retrieval with demographicNo parameter supportAdd rxSessionInterceptor and improve lightwindow close handlingInclude rxSessionInterceptor and add demographicNo to iframe/formAdd rxSessionInterceptor and implement stash clearing on closeUpdate RxSessionBean initialization with per-patient session support1 files
Add validation and error handling for missing prescriptions1 files
Register RxSessionFilter for Rx-related URL patterns1 files
Minor formatting adjustment to struts configuration