Fix: missing staged meds when using save and print functionality with multiple different patient Rx related pages (Medications, Allergies) open - LEGACY BUG#75
Open
LiamStanziani wants to merge 16 commits intoMagentaHealth:release/2026-01-19from
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
Reviewer's GuideScopes Rx prescription session state per patient via a new RxSessionFilter, per-patient RxSessionBean session keys, and a client-side rxSessionInterceptor so multiple Medications/Allergies tabs can coexist without overwriting each other, while tightening parameter parsing, error reporting, and logging. Sequence diagram for multi-tab Rx request with RxSessionFilter and rxSessionInterceptorsequenceDiagram
actor User
participant Browser
participant RxJS as rxSessionInterceptor_js
participant Filter as RxSessionFilter
participant Struts as Struts2_and_RxAction
participant Session as HttpSession
participant Bean as RxSessionBean_per_patient
User->>Browser: Open_Medications_tab_for_patient_123
Browser->>RxJS: Load_page_and_scripts
RxJS->>Browser: Set_currentDemographicNo_123
User->>Browser: Click_Save_and_Print
Browser->>RxJS: Trigger_AJAX_or_form_submit
RxJS->>Browser: Attach_demographicNo=123_to_request
Browser->>Filter: HTTP_request_with_demographicNo=123
Filter->>Session: getAttribute(RxSessionBean_123)
alt per_patient_bean_exists
Session-->>Filter: Bean_for_123
Filter->>Session: setAttribute(RxSessionBean, Bean_for_123)
else no_per_patient_bean
Filter->>Session: removeAttribute(RxSessionBean)
end
Filter->>Session: sync_Patient_attribute_for_123
Filter->>Struts: chain_doFilter
Struts->>Session: getAttribute(RxSessionBean)
Struts-->>Bean: use_correct_patient_context
Struts-->>Browser: Response_with_correct_staged_and_printed_meds
Updated class diagram for RxSessionBean and RxSessionFilter multi-patient supportclassDiagram
class RxSessionBean {
<<Serializable>>
-Logger logger
-String SESSION_KEY_PREFIX
-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(String RHS)
}
class RxSessionFilter {
<<Filter>>
-Logger logger
-String LEGACY_KEY
-String PATIENT_KEY
+void init(FilterConfig filterConfig)
+void doFilter(ServletRequest req, ServletResponse res, FilterChain chain)
+void destroy()
-static int parseDemographicNo(String param)
}
class RxPatientData {
}
class RxPatientData_Patient {
+int getDemographicNo()
+Object getDemographic()
}
class LoggedInInfo {
+static LoggedInInfo getLoggedInInfoFromSession(HttpServletRequest request)
}
class RxWriteScript2Action {
-HttpServletRequest request
-HttpServletResponse response
-RxSessionBean bean
+String updateDrug()
+int getDemographicNo()
+void setDemographicNo(String RHS)
}
class RxShowAllergy2Action {
-HttpServletRequest request
+String execute()
}
class RxChoosePatient2Action {
-HttpServletRequest request
+String execute()
}
class CaseManagementView2Action {
-HttpServletRequest request
+String view()
}
RxSessionFilter --> RxSessionBean : selects_and_swaps_per_patient_beans
RxSessionFilter --> RxPatientData_Patient : loads_Patient_from_demographicNo
RxSessionFilter --> LoggedInInfo : reads_logged_in_user
RxPatientData_Patient ..> RxPatientData : nested_type
RxWriteScript2Action --> RxSessionBean : uses_bean_from_session
RxShowAllergy2Action --> RxSessionBean : uses_bean_from_session
RxChoosePatient2Action --> RxSessionBean : creates_and_saves_per_patient_bean
CaseManagementView2Action --> RxSessionBean : creates_and_saves_per_patient_bean
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…icNo and there was no per-patient session bean for the connected demographicNo, meaning a bean was never created in the first place
![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 left some high level feedback:
- In RxSessionFilter.parseDemographicNo, an invalid (non-numeric) demographicNo parameter causes a fallback to the legacy bean instead of being treated as an explicit mismatch; consider treating invalid values the same as an explicit demographic change (e.g., clearing the legacy bean) to avoid potential cross-patient leakage when a bad param is sent.
- The form auto-wiring in rxSessionInterceptor.js only adds demographicNo when the form action contains '/oscarRx/' or 'Rx'; verify that all prescription-related forms (including newer endpoints like clearPending.do or others without 'Rx' in the path) match these heuristics, or broaden the matching criteria to avoid missing demographicNo on some flows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In RxSessionFilter.parseDemographicNo, an invalid (non-numeric) demographicNo parameter causes a fallback to the legacy bean instead of being treated as an explicit mismatch; consider treating invalid values the same as an explicit demographic change (e.g., clearing the legacy bean) to avoid potential cross-patient leakage when a bad param is sent.
- The form auto-wiring in rxSessionInterceptor.js only adds demographicNo when the form action contains '/oscarRx/' or 'Rx'; verify that all prescription-related forms (including newer endpoints like clearPending.do or others without 'Rx' in the path) match these heuristics, or broaden the matching criteria to avoid missing demographicNo on some flows.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
Ticket link for manual testing: openo-beta#2237 |
Collaborator
Author
|
Deval has reviewed this PR, see the comment from the other PR (openo-beta#2261) 
|
LiamStanziani
changed the title
Collaborator
Author
|
I'm a bit worried that this might still persist when looking at recent messages about this issue, I think I will do some extra testing soon to see if I can trigger the issue in other circumstances, which are more plausible for users to run into (testing with custom instructions in different ways, seeing if that can trigger the issue as explained) |
Collaborator
Author
|
For now, I think I will set this PR (and its openo-beta equivilant) to draft, I need to update some parts (remove added prototype usage) and will run some investigations (see previous comment) that might cause more updates later on, I think it's best this sits for a bit before more review |
LiamStanziani
changed the title
- Move RxSessionFilter after auth filters in web.xml to prevent
unauthenticated session mutation
- Add Patient session attribute swapping to RxSessionFilter, removing
redundant manual lookups in ListDrugs.jsp
- Support both demographicNo and demographic_no params in filter
- Fix fetch interceptor to preserve Request object properties
- Revert generatePreviewPDF.jsp bean read to use filter-swapped legacy key
- Remove deferred stash clearing fix (resetStashAndClose) from
ViewScript.jsp and ViewScript2.jsp
- Revert RxSessionFilter mappings to before Struts2 in web.xml.
Struts2 short-circuits chain.doFilter() for .do action URLs,
so placing the filter after Struts2 prevented bean swapping
for actions like RxWriteScript2Action. Auth for .do URLs is
handled by SecurityInfoManager inside actions, not LoginFilter.
- Simplify CaseManagementView2Action and RxChoosePatient2Action
bean setup — remove redundant getFromSession lookups since
the filter handles per-request swapping.
- Restore RxShowAllergy2Action's original provider/demographic
match check to preserve existing bean state (stash, view).
- All three actions now use saveToSession instead of setAttribute
for per-patient key storage.
…pt to new save to session method usual setup
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
rxSessionInterceptor.jsfetch/jQuery/Prototype interception currently appendsdemographicNoto all URLs, including non-Rx and potentially third‑party endpoints; consider restricting this behavior to same-origin Rx URLs (e.g., paths under/oscarRx/) to avoid leaking patient identifiers to unrelated requests. - Now that
RxSessionBean.getFromSession/saveToSessionexist, some places (e.g.,RxShowAllergy2Action, JSPs likegeneratePreviewPDF.jspandStaticScript2.jsp) still manually work with the"RxSessionBean"session attribute; consider refactoring them to consistently use the helper methods to reduce duplication and future session-handling bugs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `rxSessionInterceptor.js` fetch/jQuery/Prototype interception currently appends `demographicNo` to all URLs, including non-Rx and potentially third‑party endpoints; consider restricting this behavior to same-origin Rx URLs (e.g., paths under `/oscarRx/`) to avoid leaking patient identifiers to unrelated requests.
- Now that `RxSessionBean.getFromSession`/`saveToSession` exist, some places (e.g., `RxShowAllergy2Action`, JSPs like `generatePreviewPDF.jsp` and `StaticScript2.jsp`) still manually work with the `"RxSessionBean"` session attribute; consider refactoring them to consistently use the helper methods to reduce duplication and future session-handling bugs.
## Individual Comments
### Comment 1
<location path="src/main/webapp/oscarRx/js/rxSessionInterceptor.js" line_range="81-89" />
<code_context>
+ }
+
+ // Intercept fetch API
+ if (typeof window.fetch === 'function') {
+ var originalFetch = window.fetch;
+ window.fetch = function(url, options) {
+ var isRequest = url instanceof Request;
+ var urlStr = isRequest ? url.url : String(url);
+ if (urlStr.indexOf('demographicNo=') === -1) {
+ var separator = urlStr.indexOf('?') === -1 ? '?' : '&';
+ var newUrl = urlStr + separator + 'demographicNo=' + encodeURIComponent(demoNo);
+ if (isRequest) {
+ url = new Request(newUrl, url);
+ } else {
</code_context>
<issue_to_address>
**issue (bug_risk):** Rebuilding Request objects in the fetch interceptor is incorrect and may throw at runtime.
When `url` is a `Request`, calling `new Request(newUrl, url)` is invalid: the second argument must be a `RequestInit`, not a `Request`. This can cause runtime errors or drop properties (method, headers, body, etc.), and behavior may vary across browsers. Either:
- Only modify the URL when `url` is a string and leave `Request` objects unchanged, or
- Properly clone the `Request` by constructing `new Request(newUrl, { method: url.method, headers: url.headers, body: url.body, credentials: url.credentials, cache: url.cache, redirect: url.redirect, referrer: url.referrer })` (or equivalent) so all important fields are preserved.
</issue_to_address>
### Comment 2
<location path="src/main/webapp/oscarRx/js/rxSessionInterceptor.js" line_range="12" />
<code_context>
+ *
+ * @since 2026-01-30
+ */
+(function() {
+ 'use strict';
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the interceptor into smaller per-framework functions, scoping fetch interception to Rx URLs, and possibly removing redundant Ajax.Updater wrapping to make the module easier to understand and maintain.
You can keep all behavior but reduce complexity and coupling a bit with some small restructurings.
### 1. Split per‑framework interception into small functions
Right now everything sits in a single IIFE body. You can move each framework’s interception into its own function and just invoke them, which makes the file easier to scan and reason about without changing behavior:
```js
(function () {
'use strict';
if (typeof currentDemographicNo === 'undefined' || !currentDemographicNo) return;
var demoNo = String(currentDemographicNo);
function hasDemographicNo(params) { /* existing implementation */ }
function addDemographicNo(params) { /* existing implementation */ }
function interceptPrototypeAjax() {
if (typeof Ajax === 'undefined') return;
if (Ajax.Request && Ajax.Request.prototype.initialize) {
var origRequestInit = Ajax.Request.prototype.initialize;
Ajax.Request.prototype.initialize = function (url, options) {
options = options || {};
if (!hasDemographicNo(options.parameters)) {
options.parameters = addDemographicNo(options.parameters);
}
if (options.postBody && !hasDemographicNo(options.postBody)) {
options.postBody = addDemographicNo(options.postBody);
}
origRequestInit.call(this, url, options);
};
}
if (Ajax.Updater && Ajax.Updater.prototype.initialize) {
var origUpdaterInit = Ajax.Updater.prototype.initialize;
Ajax.Updater.prototype.initialize = function (container, url, options) {
options = options || {};
if (!hasDemographicNo(options.parameters)) {
options.parameters = addDemographicNo(options.parameters);
}
origUpdaterInit.call(this, container, url, options);
};
}
}
function interceptFetch() {
if (typeof window.fetch !== 'function') return;
var originalFetch = window.fetch;
window.fetch = function (url, options) {
var isRequest = url instanceof Request;
var urlStr = isRequest ? url.url : String(url);
if (urlStr.indexOf('demographicNo=') === -1) {
var separator = urlStr.indexOf('?') === -1 ? '?' : '&';
var newUrl = urlStr + separator + 'demographicNo=' + encodeURIComponent(demoNo);
url = isRequest ? new Request(newUrl, url) : newUrl;
}
return originalFetch.call(this, url, options || {});
};
}
function interceptJQueryAjax() {
if (typeof jQuery === 'undefined') return;
jQuery.ajaxPrefilter(function (options) {
if (!hasDemographicNo(options.data)) {
options.data = addDemographicNo(options.data);
}
});
}
function injectDemographicIntoForms() {
function processAllForms() {
var forms = document.querySelectorAll('form');
for (var i = 0; i < forms.length; i++) {
var form = forms[i];
if (form.querySelector('input[name="demographicNo"]')) continue;
var action = form.getAttribute('action') || '';
if (action.indexOf('/oscarRx/') === -1 && action.indexOf('Rx') === -1 && action !== '') continue;
var input = document.createElement('input');
input.type = 'hidden';
input.name = 'demographicNo';
input.value = demoNo;
form.appendChild(input);
}
}
if (document.readyState === 'loading') {
document.addEventListener('DOMContentLoaded', processAllForms);
} else {
processAllForms();
}
}
// Single entry point
interceptPrototypeAjax();
interceptFetch();
interceptJQueryAjax();
injectDemographicIntoForms();
})();
```
This keeps all logic and helpers exactly as you have them, but limits the “everything in one place” effect that makes the module feel dense.
### 2. Restrict global `fetch` interception to Rx URLs (optional tightening)
If most of the *intended* usage is Rx‑related, you can keep functionality while reducing cross‑cutting impact and complexity by only touching Rx URLs. That also makes the behavior easier to reason about:
```js
function interceptFetch() {
if (typeof window.fetch !== 'function') return;
var originalFetch = window.fetch;
window.fetch = function (url, options) {
var isRequest = url instanceof Request;
var urlStr = isRequest ? url.url : String(url);
// Only touch Rx‑related endpoints
if (urlStr.indexOf('/oscarRx/') === -1 && urlStr.indexOf('Rx') === -1) {
return originalFetch.call(this, url, options || {});
}
if (urlStr.indexOf('demographicNo=') === -1) {
var separator = urlStr.indexOf('?') === -1 ? '?' : '&';
var newUrl = urlStr + separator + 'demographicNo=' + encodeURIComponent(demoNo);
url = isRequest ? new Request(newUrl, url) : newUrl;
}
return originalFetch.call(this, url, options || {});
};
}
```
This doesn’t remove any Rx behavior; it just avoids rewriting unrelated `fetch` calls, which simplifies the mental model of what the interceptor is doing.
### 3. Re‑evaluate necessity of `Ajax.Updater` interception
Since `Ajax.Updater` delegates to `Ajax.Request`, you might find the `Ajax.Updater.prototype.initialize` wrapper redundant in your Prototype version. If, after verifying in your environment, `Ajax.Request` interception already covers all `Updater` cases, you can safely delete just this block:
```js
if (typeof Ajax !== 'undefined' && Ajax.Updater && Ajax.Updater.prototype.initialize) {
var origUpdaterInit = Ajax.Updater.prototype.initialize;
Ajax.Updater.prototype.initialize = function (container, url, options) {
options = options || {};
if (!hasDemographicNo(options.parameters)) {
options.parameters = addDemographicNo(options.parameters);
}
origUpdaterInit.call(this, container, url, options);
};
}
```
That would remove some duplication and one interception path without changing behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
1 participant
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:
Summary by Sourcery
Isolate prescription session data per patient to support multiple Medications tabs safely and improve robustness of Rx operations.
Bug Fixes:
Enhancements:
Documentation:
Summary by Sourcery
Isolate prescription session data per patient to support multiple concurrent Medications/Allergies tabs and make Rx operations more robust and secure.
Bug Fixes:
Enhancements: