Fix: prevent empty Rx save from archiving ReRx'd med when not staged#145
Open
LiamStanziani wants to merge 3 commits into
Conversation
…JSDoc about why drugName_ is specifically used for clarity
Reviewer's GuideGuards the Rx ReRx flow so that Save/Save And Print only operate when there are actually staged medications, and tightens server-side handling so only valid, truly re-prescribed source medications are archived and empty scripts are never created. Sequence diagram for guarded ReRx Save and archival behaviorsequenceDiagram
actor User
participant Browser as Browser_JS
participant RxAction as RxWriteScript2Action
participant Bean as RxBean
participant DrugDao
User->>Browser: click SaveAndPrint
Browser->>Browser: countStagedMedications()
alt no staged medications
Browser-->>User: alert("No medications have been added.")
Browser-->>RxAction: (no request sent)
else staged medications present
Browser->>RxAction: updateSaveAllDrugs()
RxAction->>Bean: getStashSize()
alt stashSize == 0
RxAction-->>Browser: savedScriptId = null
else stashSize > 0
RxAction->>RxAction: saveDrug(request)
loop each stash item
RxAction->>Bean: getStashItem(i)
RxAction->>Bean: addRandomIdDrugIdPair()
RxAction->>RxAction: collect drugReferenceId in represcribedOriginalIds
end
RxAction->>RxAction: iterate reRxDrugIdList
loop each reRxDrugId
RxAction->>RxAction: Integer.parseInt(reRxDrugId)
alt id in represcribedOriginalIds
RxAction->>DrugDao: find(originalDrugId)
alt drug found
RxAction->>DrugDao: setArchived(true), setArchivedReason(REPRESCRIBED)
else drug null
RxAction-->>RxAction: continue
end
else not represcribed or malformed
RxAction-->>RxAction: skip archival
end
end
RxAction-->>Browser: savedScriptId
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 client/server coordination around detecting staged meds is brittle since both sides hard-code the
drugName_marker; consider centralizing this marker (e.g., via a shared constant, data attribute, or server-provided config) so future changes don't silently desync the gating logic. - When ignoring or skipping non-numeric
reRxDrugIdvalues, thelogger.warncalls could include the offending value (with parameterized logging) to make it easier to diagnose polluted sessions or crafted requests in production.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The client/server coordination around detecting staged meds is brittle since both sides hard-code the `drugName_` marker; consider centralizing this marker (e.g., via a shared constant, data attribute, or server-provided config) so future changes don't silently desync the gating logic.
- When ignoring or skipping non-numeric `reRxDrugId` values, the `logger.warn` calls could include the offending value (with parameterized logging) to make it easier to diagnose polluted sessions or crafted requests in production.
## Individual Comments
### Comment 1
<location path="src/main/java/ca/openosp/openo/prescript/pageUtil/RxWriteScript2Action.java" line_range="1440-1449" />
<code_context>
while (i.hasNext()) {
String item = i.next();
+ int originalDrugId;
+ try {
+ originalDrugId = Integer.parseInt(item);
+ } catch (NumberFormatException e) {
+ // Defensive: skip any malformed entry from a polluted session
+ // rather than failing the whole save (#2453).
+ logger.warn("Skipping non-numeric reRxDrugId");
+ continue;
+ }
</code_context>
<issue_to_address>
**suggestion:** Improve diagnostic value of the non-numeric reRxDrugId warning.
This warning doesn’t include the offending `item` value, which will make production debugging harder. Please log the actual `item` (possibly truncated or sanitized) so operators can see which session value caused the parse failure.
```suggestion
String item = i.next();
+ int originalDrugId;
+ try {
+ originalDrugId = Integer.parseInt(item);
+ } catch (NumberFormatException e) {
+ // Defensive: skip any malformed entry from a polluted session
+ // rather than failing the whole save (#2453).
+ String loggedItem = item;
+ if (loggedItem != null && loggedItem.length() > 128) {
+ loggedItem = loggedItem.substring(0, 125) + "...";
+ }
+ logger.warn("Skipping non-numeric reRxDrugId value: '{}'", loggedItem);
+ continue;
+ }
```
</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.
Summary
In the Medications (Rx) module, checking the ReRx box next to an existing
medication and then clicking Save or Save And Print — without clicking
Stage Medication — caused the medication to drop off the active list, and
quietly persisted an empty prescription. This fix makes an unstaged save do
nothing harmful: Save And Print warns "No medications have been added" and
Save is a no-op, with no medication archived and no empty prescription
written. A server-side guard additionally ensures a ReRx'd medication is only
archived when a replacement was actually staged and saved.
Original PR openo-beta#2461
Problem
Checking the ReRx checkbox immediately parks the drug's id in the session's
reRxDrugIdList(via an AJAX call) and shows a confirmation box prompting theuser to click Stage Medication. Staging is what actually builds the new
prescription. If the user skipped staging and clicked Save:
RxWriteScript2Action.saveDrug()ran its archivalloop over every id in
reRxDrugIdList, archiving the original medication(reason
REPRESCRIBED) even though no replacement had been created. Archiveddrugs don't render in the active list, so the medication appeared deleted.
saveScript()always ran first, so an emptyprescriptionrow was persistedon every such save, and Save And Print then opened a print preview that
errored on the empty script.
Neither outcome matched the expected behavior: an unstaged save should warn
(Save And Print) or do nothing (Save), and must never remove the medication.
Solution
Two layers, both required:
Client gate —
src/main/webapp/oscarRx/SearchDrug3.jspcountStagedMedications(), which counts the staged drugs actuallypresent in the prescription form (each staged drug renders a
drugName_<rand>input; a ReRx box that was only checked does not).updateSaveAllDrugsPrintCheckContinue()andupdateSaveAllDrugsCheckContinue()now short-circuit when nothing isstaged: Save And Print alerts "No medications have been added." and Save
returns without submitting. This prevents the empty prescription and the
erroring print preview, and never reaches the archival code in the empty
case.
warning (or the Save no-op) the box stays checked and the user can simply
click Stage Medication to continue. This is safe because the checkbox
is rendered from the server-side
reRxDrugIdListon every load(
ListDrugs.jsp), so the on-screen state always matches the session, andthe server guard below ensures a parked id is never archived unless it was
actually staged and saved.
Server guard —
RxWriteScript2Action.saveDrug()actually re-prescribed (each staged re-prescription carries the source
drug's id in
drugReferenceId, set byRxPrescriptionData.newPrescription(.., rePrescribe)).reRxDrugIdListonly if it is inthat set, with a null guard on the lookup. This covers the mixed case
(stage one drug while another ReRx box is left checked but unstaged) and
any stale parked id, so a medication is only ever archived when a
replacement genuinely exists.
Verification
Reproduced and verified against the running app on a test patient:
no empty script — medication remains.
warning, ReRx box stays checked, no archival, no empty script, no broken
preview — medication remains.
new drug created and the original archived as
REPRESCRIBED(normal flowunchanged).
unstaged: only the staged medication is archived; the unstaged one is left
untouched (server guard).
Summary by Sourcery
Prevent unstaged ReRx saves from archiving medications or creating empty prescriptions by adding client and server guards, and harden ReRx handling against malformed IDs.
Bug Fixes:
Enhancements: