feat: implement patient demographic merge and unmerge#2416
Open
D3V41 wants to merge 135 commits into
Open
Conversation
- Interface with all method signatures for the copy engine - copyDemographic() uses INSERT INTO...SELECT (single SQL statement, no row-by-row) - DatabaseMetaData replaces information_schema (JDBC standard, H2-compatible) - ConcurrentHashMap caches table existence and column lists across 40+ table copies - validateIdentifier() whitelist prevents SQL injection on dynamic table/column names
…eMgmtIssue, CaseMgmtIssueNotes, CaseMgmtIssueNotesPK JPA entity classes
BillingONItem, Consent, ConsultationRequest, ConsultationRequestExt, DemographicCustArchive, DemographicExtArchive, DigitalSignature, EFormValue, EReferAttachment, EmailAttachment, EmailLog, Measurement, MeasurementsDeleted, OLISQueryLog, OLISResults, PatientLabRouting, Prescription, Prevention, PreventionExt, FormBCAR2020, HRMDocumentToDemographic to support Hibernate detach-persist copy pattern
…ConsultationArchiveGroup
…ns are not orphaned
…ltationArchiveGroup
…ttachment_data copy
- Add DemographicMergeManager interface and DemographicMergeManagerImpl - Add MERGED to PatientStatus enum in Demographic model - Mark primary and secondaries as MERGED after merge (not IN) - Restore to AC on unmerge; deactivate merged record C to IN - Block MERGED and IN patients from re-merging (validation guard)
- Add demographicMergeRecord.jsp (step 1: search & select patients) - Add demographicMergePrimarySelect.jsp (step 2: pick primary record) - Both JSPs use pure JSTL/EL - no scriptlets, Bootstrap 5, OWASP e: encoding - Security check via security:oscarSec tag with c:redirect (no scriptlets) - Add searchDemographicsForMerge() and searchMergedDemographicsForUnmerge() to DemographicManager/DemographicManagerImpl - Refactor DemographicMergeAction to handle search/selectPrimary display routes and use only managers (no direct DAO access)
- Register admin/DemographicMerge action in struts.xml with search, selectPrimary, success, successUnMerge, and failure results - Fix action URLs in both JSPs to use admin/DemographicMerge.do - Add Merge Patient Records link to admin.jsp popup menu - Add Merge Patient Records link to leftNav.jspf - Add admin.admin.mergeDemographic key to all 5 language resource files en, fr, es, pl, pt_BR)
…pyFormBooleanValues(). No generated key is needed here so batching is straightforward. Reduces DB round-trips from O(n) to 1 per form.
In copyCasemgmtNoteGroup(), the previous nested loop fired one JPQL UPDATE per (note × appointment) pair. Rewrite to loop once per appointment and pass all copied note PKs via IN :noteIds, reducing UPDATE statements from notes×appointments down to appointments.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace legacy DemographicMerged head/tail model with an event-driven DemographicMerge system: add DemographicMerge entity/table, merge manager/service and copy-operation DAO; update DAOs, DTOs, JSPs, REST, and tests; remove merged-wrapper DAOs, templates, interceptor, and head-resolution code. ChangesDemographic merge event migration
Sequence Diagram(s) sequenceDiagram
participant UI as UI (DemographicMergeAction)
participant Manager as DemographicMergeManager
participant MergeEventDAO as DemographicMergeDao
participant OperationDAO as DemographicMergeOperationDao
participant DB as Database
UI->>Manager: merge(primary, [secondaries])
Manager->>Manager: validate privileges & inputs
Manager->>DB: load primary & secondary demographics
Manager->>DB: clone primary → new merged demographic
Manager->>DB: persist merged demographic
Manager->>MergeEventDAO: persist MERGE event (DemographicMerge)
loop per secondary
Manager->>OperationDAO: copyAllDataForSource(source -> merged)
OperationDAO->>DB: copy identity, forms, clinical records
OperationDAO->>DB: copy appointments/allergies → return PK remaps
OperationDAO->>DB: copy notes/issues with remapping
end
Manager->>DB: mark primary & secondaries IN (inactive)
Manager->>DB: write audit LogAction entries
Manager->>UI: return merged demographic ID
UI->>Manager: unmerge(mergedId)
Manager->>MergeEventDAO: findLatestMergeEventByMergedDemographicNo(mergedId)
Manager->>DB: restore primary & secondaries to AC
Manager->>DB: mark merged record IN
Manager->>MergeEventDAO: persist UNMERGE event
Manager->>DB: write audit LogAction entries
Manager->>UI: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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 implements a new patient demographic merge and unmerge system, replacing legacy logic with a dedicated manager and a Hibernate-based copy engine. The changes include a modernized Bootstrap 5 UI, a new REST service, and updated database schemas for an event-based merge model. Feedback identifies a critical SQL injection vulnerability in the search criteria and potential unique constraint violations when cloning chart numbers. Further recommendations address the destructive nature of the SQL update script, the use of standard logging over console output, and the need for more specific exception handling and safer request object management in the web layer.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
database/mysql/oscarinit_2025.sql (1)
793-795:⚠️ Potential issue | 🟡 MinorStale comment references wrong table.
The comment states "Alter table structure for table
demographic_merged" but the following statement (line 797) altersfax_config. This appears to be leftover from previous edits.📝 Proposed fix
-- --- Alter table structure for table `demographic_merged`, modified column sequence +-- Alter table structure for table `fax_config`, modified column defaults and sequence --🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/mysql/oscarinit_2025.sql` around lines 793 - 795, The SQL comment above the ALTER statement is stale: it mentions "Alter table structure for table `demographic_merged`" but the subsequent ALTER statement targets `fax_config`; update the comment to accurately describe the change (e.g., "Alter table structure for table `fax_config`" or a brief description of the actual modification) so the comment matches the ALTER statement for `fax_config` (or if the intent was to modify `demographic_merged`, replace the ALTER statement with the correct one); ensure the comment and the ALTER `fax_config` statement are consistent.
🟠 Major comments (21)
src/main/java/ca/openosp/openo/commn/model/Measurement.java-167-169 (1)
167-169:⚠️ Potential issue | 🟠 MajorRemove the
setId()method—it violates immutability and lacks required documentation.The
setId(Integer id)method at lines 167–169 exposes a public setter on a@GeneratedValue(strategy = GenerationType.IDENTITY)primary key, breaking the immutability contract declared in the class JavaDoc (lines 50–51). The@PreUpdatecallback prevents updates, but this setter still allows external ID mutation, risking identifier tampering and data integrity issues. Additionally, as a public method, it must have JavaDoc per coding guidelines, and currently has none.No active callsites of
Measurement.setId()were found in the codebase (the only reference in MeasurementConverter.java is commented out), so safe removal is recommended.Suggested change
- public void setId(Integer id) { - this.id = id; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/Measurement.java` around lines 167 - 169, The public setter setId(Integer id) on the Measurement entity breaks the class's immutability guarantee and allows external mutation of the `@GeneratedValue`(strategy = GenerationType.IDENTITY) primary key; remove the public method setId from the Measurement class to prevent ID tampering, update any commented or potential callsites (e.g., MeasurementConverter) to avoid relying on it, and ensure no other code expects a public ID setter; do not replace it with another public API—keep the id field immutable and managed by JPA/@PreUpdate as intended.src/main/java/ca/openosp/openo/commn/model/BillingONCHeader1.java-228-230 (1)
228-230:⚠️ Potential issue | 🟠 MajorAdd JavaDoc and constrain
setIdvisibility on generated-identity entity.The public setter for
idon line 228 lacks required JavaDoc documentation and creates unnecessary risk for identity mutation on an entity using@GeneratedValue(strategy = GenerationType.IDENTITY). Since no production code relies on public visibility (verified via codebase search), change to protected and document the method per repository guidelines.Suggested change
+ /** + * Sets the persistent identifier. + * + * `@param` id Integer the database primary key value + */ - public void setId(Integer id) { + protected void setId(Integer id) { this.id = id; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/BillingONCHeader1.java` around lines 228 - 230, The setter for the generated identity field should be non-public and documented: change the visibility of BillingONCHeader1.setId(Integer id) from public to protected and add the repository-standard JavaDoc explaining this is for JPA use only and must not be used to mutate the entity id (mention the id field and `@GeneratedValue` strategy). Ensure the JavaDoc follows project wording conventions and keep the method body unchanged.src/main/java/ca/openosp/openo/commn/model/PatientLabRouting.java-126-128 (1)
126-128: 🛠️ Refactor suggestion | 🟠 MajorPlease add JavaDoc for
setId.Line 126 adds a new public mutator and should include JavaDoc (
@param id Integer ...+@since).As per coding guidelines, "All public classes and methods MUST have comprehensive JavaDoc documentation", "Document
@paramtags with specific data types", and "Use@sincetags in JavaDoc with accurate dates from git history."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/PatientLabRouting.java` around lines 126 - 128, Add JavaDoc to the public setter method setId in class PatientLabRouting: document the parameter with an `@param` tag that specifies the type and purpose (e.g., "@param id Integer the primary key / identifier for this PatientLabRouting"), and add an `@since` tag with the accurate date/version pulled from git history; ensure the JavaDoc is placed immediately above the public void setId(Integer id) method and follows the project's JavaDoc style guidelines.src/main/java/ca/openosp/openo/commn/model/EReferAttachment.java-49-51 (1)
49-51: 🛠️ Refactor suggestion | 🟠 MajorDocument the new public
setIdmethod.Please add method JavaDoc for Line 49 with a typed
@paramand@since.As per coding guidelines, "All public classes and methods MUST have comprehensive JavaDoc documentation", "Document
@paramtags with specific data types", and "Use@sincetags in JavaDoc with accurate dates from git history."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/EReferAttachment.java` around lines 49 - 51, Add a JavaDoc comment to the public setter method setId in class EReferAttachment: document the parameter with a typed `@param` tag (e.g., `@param` id Integer the identifier to set) and include an accurate `@since` tag using the correct commit/date from git history; ensure the JavaDoc sits immediately above the public void setId(Integer id) method and follows existing project formatting conventions.src/main/java/ca/openosp/openo/commn/model/ConsultationRequest.java-116-118 (1)
116-118: 🛠️ Refactor suggestion | 🟠 MajorAdd JavaDoc for the new
setIdAPI.Line 116 introduces a public method; please document it with typed
@paramand@since.As per coding guidelines, "All public classes and methods MUST have comprehensive JavaDoc documentation", "Document
@paramtags with specific data types", and "Use@sincetags in JavaDoc with accurate dates from git history."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/ConsultationRequest.java` around lines 116 - 118, Add a JavaDoc comment for the public setter setId in ConsultationRequest: document the method with a brief description, include a typed `@param` tag specifying the parameter type (Integer id) and its meaning, and add an `@since` tag with the correct date/version pulled from git history; update the JavaDoc above the method declaration for setId(Integer id) accordingly.src/main/java/ca/openosp/openo/commn/model/Consent.java-90-92 (1)
90-92: 🛠️ Refactor suggestion | 🟠 MajorPlease document
setId(Integer id).The new public mutator at Line 90 should have JavaDoc including
@param id Integer ...and@since.As per coding guidelines, "All public classes and methods MUST have comprehensive JavaDoc documentation", "Document
@paramtags with specific data types", and "Use@sincetags in JavaDoc with accurate dates from git history."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/Consent.java` around lines 90 - 92, Add JavaDoc for the public mutator setId(Integer id) in class Consent: write a one-line description of what the setter does, document the parameter with an `@param` tag specifying type and meaning (e.g., "@param id Integer the unique identifier for this consent"), and add an accurate `@since` tag using the commit/date from git history when this method was introduced; ensure the JavaDoc follows the project's style and appears immediately above the setId(Integer id) method declaration.src/main/java/ca/openosp/openo/commn/model/Prevention.java-220-222 (1)
220-222: 🛠️ Refactor suggestion | 🟠 MajorAdd JavaDoc for
setId(Integer id).The method added at Line 220 is public and should include JavaDoc with typed
@paramand@since.As per coding guidelines, "All public classes and methods MUST have comprehensive JavaDoc documentation", "Document
@paramtags with specific data types", and "Use@sincetags in JavaDoc with accurate dates from git history."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/Prevention.java` around lines 220 - 222, Add comprehensive JavaDoc to the public setter setId(Integer id) in class Prevention: document the method purpose, include a typed `@param` tag describing the Integer id parameter, and add an accurate `@since` tag using the commit/date from git history when this method was introduced; ensure the JavaDoc follows project conventions and sits immediately above the setId(Integer id) method declaration.src/main/java/ca/openosp/openo/commn/model/Prescription.java-98-100 (1)
98-100: 🛠️ Refactor suggestion | 🟠 MajorDocument the new
setId(Integer id)method.Line 98 adds a public method without JavaDoc. Please include method JavaDoc with typed
@paramand@since.As per coding guidelines, "All public classes and methods MUST have comprehensive JavaDoc documentation", "Document
@paramtags with specific data types", and "Use@sincetags in JavaDoc with accurate dates from git history."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/Prescription.java` around lines 98 - 100, Add comprehensive JavaDoc to the public method setId(Integer id) in class Prescription: include a one-line description of the setter (what id represents), a typed `@param` tag that specifies the parameter type (Integer id) and any nullability/constraints, and an `@since` tag with the correct date taken from the git history for this change; place the JavaDoc immediately above the setId(Integer id) method signature.src/main/java/ca/openosp/openo/commn/model/AppointmentArchive.java-284-286 (1)
284-286: 🛠️ Refactor suggestion | 🟠 MajorAdd JavaDoc to
setId(Integer id).The new public method at Line 284 should be documented with typed
@paramand@since.As per coding guidelines, "All public classes and methods MUST have comprehensive JavaDoc documentation", "Document
@paramtags with specific data types", and "Use@sincetags in JavaDoc with accurate dates from git history."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/AppointmentArchive.java` around lines 284 - 286, The public setter method setId(Integer id) in class AppointmentArchive is missing JavaDoc: add a JavaDoc comment immediately above the method that briefly describes the purpose of the setter, include a typed `@param` tag that specifies the parameter type (e.g., `@param` id Integer the identifier to set), and add an `@since` tag with the accurate commit/date from git history; ensure the comment follows project JavaDoc conventions and is placed directly above the setId(Integer id) method.src/main/java/ca/openosp/openo/commn/model/EFormValue.java-64-66 (1)
64-66: 🛠️ Refactor suggestion | 🟠 MajorAdd JavaDoc to the new public
setIdmutator.Line 64 introduces a new public method without JavaDoc. Please add method-level JavaDoc with healthcare-domain context, a typed
@param, and an@sincedate from git history.Proposed update
+ /** + * Sets the persisted identifier for this eForm value record during controlled + * demographic merge/copy operations. + * + * `@param` id Integer the primary key value for this eForm value row + * `@since` <populate from git history> + */ public void setId(Integer id) { this.id = id; }As per coding guidelines, "All public classes and methods MUST have comprehensive JavaDoc documentation", "Document
@paramtags with specific data types", and "Use@sincetags in JavaDoc with accurate dates from git history."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/EFormValue.java` around lines 64 - 66, Add comprehensive JavaDoc to the public mutator method setId in class EFormValue: document the method purpose in the healthcare domain (e.g., setting the unique identifier for a patient or form value record), include a typed `@param` tag (e.g., `@param` id Integer the database identifier for this EFormValue), and add an `@since` tag using the accurate commit date from git history; update the JavaDoc immediately above the public void setId(Integer id) method in EFormValue.src/main/java/ca/openosp/openo/commn/model/CaseMgmtNote.java-40-43 (1)
40-43:⚠️ Potential issue | 🟠 MajorAdd encryption handling for PHI-bearing persisted fields.
note,history, andpasswordare mapped as plain persisted strings. For OpenO healthcare data, these fields need EncryptionUtils-backed protection at rest before persistence.As per coding guidelines,
Encrypt sensitive PHI data before storage using EncryptionUtilsandHealthcare models must support encryption fields for sensitive PHI using ca.openosp.openo.utilities.encryption.EncryptionUtils.Also applies to: 68-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/CaseMgmtNote.java` around lines 40 - 43, The CaseMgmtNote model stores PHI fields (note, history, password) as plain strings; update the class to encrypt these fields using ca.openosp.openo.utilities.encryption.EncryptionUtils before they are persisted and to decrypt them when loaded. Implement lifecycle hooks (e.g., `@PrePersist/`@PreUpdate to call EncryptionUtils.encrypt(...) on the note, history and password fields, and `@PostLoad` to call EncryptionUtils.decrypt(...)) or add a JPA AttributeConverter that delegates to EncryptionUtils and apply it to those fields; ensure null/empty checks and idempotency so already-encrypted values are not double-encrypted and exceptions are handled/logged appropriately.src/main/webapp/demographic/demographiceditdemographic.jsp-1353-1353 (1)
1353-1353:⚠️ Potential issue | 🟠 MajorRestore a real canonical-record check here.
Line 1353 turns the merged-record edit gate into a no-op. That means secondary merged charts now get the same Edit affordance as the primary chart, which can reintroduce divergent data after a merge. Please replace this with an explicit “current record is editable/canonical” check from the new merge model instead of
true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/demographic/demographiceditdemographic.jsp` at line 1353, The unconditional gate "if (true)" removes the canonical-record check and lets secondary merged charts show edit UI; restore the proper check by replacing that "if (true)" with the new merge-model canonical/editable predicate (e.g., call the merge model's method like isCanonicalRecord(currentRecord) or currentRecord.isEditablePerMergeModel()) so only the canonical/primary record shows edit affordances; locate the check that currently reads "if (true)" in demographiceditdemographic.jsp and swap it for the merge-model predicate used elsewhere (the new merge model API method that determines canonical/editable status).database/mysql/updates/update-2026-04-10-demographic-merged-extend.sql-7-7 (1)
7-7:⚠️ Potential issue | 🟠 MajorData loss risk:
DROP TABLEdestroys existing merge history.This migration unconditionally drops
demographic_merged, losing any existing merge records without migration. For production deployments, consider:
- Backing up or migrating existing data before dropping
- Using
RENAME TABLEto preserve old data temporarily- Adding a data migration step if historical merge records need to be preserved
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/mysql/updates/update-2026-04-10-demographic-merged-extend.sql` at line 7, The migration currently drops the demographic_merged table unconditionally which destroys existing merge history; instead, preserve or migrate data by (a) creating the new target schema (new_demographic_merged) and copying existing rows from demographic_merged into it, or (b) renaming the existing table (RENAME TABLE demographic_merged TO demographic_merged_backup) before creating the new table so you retain a backup, or (c) exporting/backing up demographic_merged first and then running an explicit data transformation step to move historical merge records into the new structure; update the migration SQL to perform one of these safe paths and include an explicit backup/rename/copy step referencing demographic_merged and the new table names.src/main/java/ca/openosp/openo/commn/model/Relationships.java-146-152 (1)
146-152:⚠️ Potential issue | 🟠 MajorPotential NullPointerException in
setDeleted(Boolean).The field
deletedis declared as primitiveboolean(line 70), but the setter now accepts boxedBoolean. Ifnullis passed tosetDeleted(null), auto-unboxing will throw aNullPointerException.Either change the field to
Booleanto support nullability, or keep the setter asboolean(primitive) to maintain type consistency with the field.🐛 Option 1: Change field to Boolean
- private boolean deleted; + private Boolean deleted;🐛 Option 2: Keep primitive setter
- public void setDeleted(Boolean deleted) { + public void setDeleted(boolean deleted) { this.deleted = deleted; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/Relationships.java` around lines 146 - 152, The setter setDeleted(Boolean) accepts a boxed Boolean but the class field deleted is a primitive boolean, which risks a NullPointerException on auto-unboxing; fix by making the types consistent: either change the field deleted from primitive boolean to Boolean (to allow nulls) and keep getDeleted/setDeleted signatures as Boolean, or change the setter signature to setDeleted(boolean) (and adjust callers to never pass null). Update the declaration of deleted and the getter/setter pair (getDeleted/setDeleted) accordingly to ensure type consistency across the class.src/main/webapp/WEB-INF/classes/struts.xml-1228-1230 (1)
1228-1230:⚠️ Potential issue | 🟠 MajorPreserve unmerge mode on the failure redirect.
successUnMergereturns to the unmerge tab, but the sharedfailureredirect dropsmode=unmerge. Any unmerge validation/error path will bounce the user back to the merge screen and hide the context of the failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/WEB-INF/classes/struts.xml` around lines 1228 - 1230, The failure redirect in struts.xml drops the unmerge context; update the <result name="failure"> for the DemographicMerge action to preserve the current mode by adding the mode parameter (e.g. change the URL to "/admin/DemographicMerge.do?outcome=failure&mode=%{mode}"), so when mode == "unmerge" the user returns to the unmerge tab; keep the result name "failure" unchanged.src/main/java/ca/openosp/openo/commn/dao/DemographicDao.java-296-336 (1)
296-336:⚠️ Potential issue | 🟠 MajorRestore domain-scoped filtering on these merged-record searches.
These are the first demographic search APIs in this interface that drop
providerNo/outOfDomain. That makes it impossible for the DAO contract to enforce the same domain visibility rules as the neighboring search methods, so unmerge search can return cross-domain patient records.As per coding guidelines, "PHI (Patient Health Information) must NEVER be logged or exposed".Suggested API shape
- public List<Demographic> findActiveMergedDemographicByName(String searchStr, int limit, int offset); + public List<Demographic> findActiveMergedDemographicByName(String searchStr, int limit, int offset, + String providerNo, boolean outOfDomain); - public List<Demographic> findActiveMergedDemographicByDOB(String dobStr, int limit, int offset); + public List<Demographic> findActiveMergedDemographicByDOB(String dobStr, int limit, int offset, + String providerNo, boolean outOfDomain); - public List<Demographic> findActiveMergedDemographicByPhone(String phoneStr, int limit, int offset); + public List<Demographic> findActiveMergedDemographicByPhone(String phoneStr, int limit, int offset, + String providerNo, boolean outOfDomain); - public List<Demographic> findActiveMergedDemographicByHIN(String hinStr, int limit, int offset); + public List<Demographic> findActiveMergedDemographicByHIN(String hinStr, int limit, int offset, + String providerNo, boolean outOfDomain); - public List<Demographic> findActiveMergedDemographicByAddress(String addressStr, int limit, int offset); + public List<Demographic> findActiveMergedDemographicByAddress(String addressStr, int limit, int offset, + String providerNo, boolean outOfDomain);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/dao/DemographicDao.java` around lines 296 - 336, The five merged-record search methods in DemographicDao (findActiveMergedDemographicByName, findActiveMergedDemographicByDOB, findActiveMergedDemographicByPhone, findActiveMergedDemographicByHIN, findActiveMergedDemographicByAddress) dropped domain-scoping params; restore domain-scoped filtering by adding providerNo (or equivalent domain identifier) and outOfDomain boolean params to each method signature and update their implementations to apply providerNo/outOfDomain filtering in the query execution path so searches cannot return cross-domain records; also ensure no PHI (names, DOB, phone, HIN, addresses) is logged anywhere in these methods or their implementations.database/mysql/oscarinit.sql-8250-8259 (1)
8250-8259:⚠️ Potential issue | 🟠 MajorRestore the required audit columns on
demographic_merged.This table drops
lastUpdateUserandlastUpdateDate, so it no longer meets the repo’s audit-trail requirement.provider_noandevent_datedescribe the merge event, but they do not capture who last modified the row itself or when.🛠️ Proposed fix
CREATE TABLE IF NOT EXISTS demographic_merged ( id INT(10) NOT NULL PRIMARY KEY AUTO_INCREMENT, primary_demographic_no INT(10) NULL DEFAULT NULL, secondary_demographic_no VARCHAR(500) NULL DEFAULT NULL, merged_demographic_no INT(10) NULL DEFAULT NULL, event_type VARCHAR(20) NULL DEFAULT NULL, provider_no VARCHAR(6) NULL DEFAULT NULL, event_date DATETIME NULL DEFAULT NULL, + lastUpdateUser VARCHAR(6) NULL DEFAULT NULL, + lastUpdateDate DATETIME NOT NULL, INDEX idx_dm_merged_demo_no (merged_demographic_no), INDEX idx_dm_event_type (event_type) );As per coding guidelines,
database/mysql/**/*.sql: Always includelastUpdateUserandlastUpdateDatefields in database tables for audit trail compliance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/mysql/oscarinit.sql` around lines 8250 - 8259, The demographic_merged table is missing the required audit columns; add lastUpdateUser and lastUpdateDate columns to the CREATE TABLE for demographic_merged (referencing the table name demographic_merged) so every row records who and when it was last modified: add a lastUpdateUser column (e.g., VARCHAR(64) NULL DEFAULT NULL) and a lastUpdateDate column (e.g., DATETIME NULL DEFAULT NULL) to the table definition and include them alongside the other column definitions and indexes to restore audit-trail compliance.database/mysql/oscarinit_2025.sql-775-791 (1)
775-791:⚠️ Potential issue | 🟠 MajorMissing required audit trail columns
lastUpdateUserandlastUpdateDate.The new
demographic_mergedtable does not includelastUpdateUserandlastUpdateDatecolumns, which are required for audit trail compliance per repository guidelines.🛠️ Proposed fix to add audit columns
CREATE TABLE `demographic_merged` ( `id` INT(10) NOT NULL PRIMARY KEY AUTO_INCREMENT, `primary_demographic_no` INT(10) NULL DEFAULT NULL, `secondary_demographic_no` VARCHAR(500) NULL DEFAULT NULL, `merged_demographic_no` INT(10) NULL DEFAULT NULL, `event_type` VARCHAR(20) NULL DEFAULT NULL, `provider_no` VARCHAR(6) NULL DEFAULT NULL, `event_date` DATETIME NULL DEFAULT NULL, + `lastUpdateUser` VARCHAR(6) NULL DEFAULT NULL, + `lastUpdateDate` DATETIME NULL DEFAULT NULL, INDEX `idx_dm_merged_demo_no` (`merged_demographic_no`), INDEX `idx_dm_event_type` (`event_type`) );As per coding guidelines: "All database tables MUST include lastUpdateUser and lastUpdateDate columns for audit trails"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/mysql/oscarinit_2025.sql` around lines 775 - 791, The `demographic_merged` table is missing the required audit trail columns; update the CREATE TABLE for `demographic_merged` to add `lastUpdateUser` and `lastUpdateDate` columns (e.g., `lastUpdateUser` as a non-null varchar for the username and `lastUpdateDate` as a non-null DATETIME with a default/current-timestamp and on-update behavior) so the table complies with the repository audit guideline; ensure the column names match exactly (`lastUpdateUser`, `lastUpdateDate`) and add them to the CREATE TABLE statement alongside the other columns and indexes.src/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.java-3132-3141 (1)
3132-3141:⚠️ Potential issue | 🟠 MajorThe new merged-record search is narrower than the existing merge search.
findActiveMergedDemographicByPhone()only checksde.phone, while the existing demographic phone search also coversPhone2anddemo_cell.findActiveMergedDemographicByAddress()also changed address matching from contains to prefix-only. Unmerge mode will miss records that merge mode can still find.Also applies to: 3160-3169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.java` around lines 3132 - 3141, The merged-record phone/address searches are narrower than existing searches: update findActiveMergedDemographicByPhone to match de.phone OR de.phone2 OR de.demo_cell using LIKE '%{phone}%' (use q.setParameter with "%" + phoneStr.trim() + "%") so it mirrors the original multi-column contains search, and update findActiveMergedDemographicByAddress to use a contains/substring match (LIKE '%{address}%') instead of prefix-only; keep the ACTIVE_MERGED_SUBQUERY and existing ORDER/BIND logic intact when adjusting the SQL and parameters.src/main/webapp/admin/demographicMergeRecord.jsp-493-527 (1)
493-527:⚠️ Potential issue | 🟠 MajorRemove previously appended hidden inputs before confirming again.
Canceling and reopening the merge confirmation modal appends another
primaryDemographicNoand another set ofsecondaryDemographicNofields. The final POST can include stale IDs from an earlier selection.Suggested fix
if (mergeConfirmBtn) { mergeConfirmBtn.addEventListener('click', function () { const form = document.getElementById('mergeForm'); const primaryRadio = document.querySelector('input.primary-radio:checked'); const checked = Array.from(document.querySelectorAll('input.demo-check:checked')); + + form.querySelectorAll('input[name="primaryDemographicNo"], input[name="secondaryDemographicNo"]').forEach(function (input) { + input.remove(); + }); const pi = document.createElement('input'); pi.type = 'hidden'; pi.name = 'primaryDemographicNo'; pi.value = primaryRadio.value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/admin/demographicMergeRecord.jsp` around lines 493 - 527, Before appending new hidden inputs in the mergeConfirmBtn click handler, remove any previously appended inputs named "primaryDemographicNo" and "secondaryDemographicNo" from the form (element with id "mergeForm") to avoid stale values; locate the click handler for mergeConfirmBtn and, at its start, use form.querySelectorAll('input[name="primaryDemographicNo"], input[name="secondaryDemographicNo"]') and remove each node, then proceed to create the new hidden inputs (the existing logic that creates the input for primaryRadio.value and the loop adding secondaryDemographicNo inputs should remain unchanged).src/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.java-107-110 (1)
107-110:⚠️ Potential issue | 🟠 Major
getMergedDemographics()now returns the merged record, not the merged children.The method contract still says this returns merged child IDs. After an
A + B -> Cmerge, this query returnsC, so any caller expectingBwill get the wrong demographic IDs back.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.java` around lines 107 - 110, The query in DemographicDaoImpl.getMergedDemographics() is returning the merged record ID (merged_demographic_no) instead of the merged child IDs; change the SQL to return the child IDs and match the method contract (for example, select primary_demographic_no when filtered by merged_demographic_no and event_type='MERGE') and update any related log/javadoc to reflect the corrected behavior so callers receive the merged children as intended.
🧹 Nitpick comments (7)
src/main/java/ca/openosp/openo/commn/model/CaseMgmtCpp.java (1)
74-115: Consider multi-line formatting for getter/setter methods.The single-line getter/setter format works but reduces readability. This is a minor style preference and not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/CaseMgmtCpp.java` around lines 74 - 115, The CaseMgmtCpp class currently uses single-line getter/setter methods (e.g., getId(), setId(Integer), getDemographicNo(), setDemographicNo(String), etc.), which hurts readability; update each getter and setter to standard multi-line formatting so each method has its signature line, a separate body line (return or assignment), and a closing brace on its own line; apply the change to all methods in CaseMgmtCpp (getId/setId through getPastMedications/setPastMedications) and run the project's Java formatter/IDE code style to ensure consistent spacing and indentation.src/main/java/ca/openosp/openo/commn/model/CaseMgmtNoteExt.java (1)
17-24: Consider extendingAbstractModel<Integer>for DAO consistency.Other entity classes in this package (e.g.,
Relationships) extendAbstractModel<Integer>. This base class typically provides common functionality expected byAbstractDao. Without it, generic DAO operations may not work as expected.♻️ Proposed fix
`@Entity` `@Table`(name = "casemgmt_note_ext") -public class CaseMgmtNoteExt { +public class CaseMgmtNoteExt extends AbstractModel<Integer> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/CaseMgmtNoteExt.java` around lines 17 - 24, The CaseMgmtNoteExt entity should extend the shared base class to align with DAO expectations: change the class declaration of CaseMgmtNoteExt to extend AbstractModel<Integer> (matching other entities like Relationships) so generic DAO operations in AbstractDao will work; update the class signature and imports to reference AbstractModel<Integer> and ensure any required abstract methods or constructors from AbstractModel are implemented or their defaults are honored.src/main/java/ca/openosp/openo/commn/model/CaseMgmtNoteLink.java (1)
16-23: Consider extendingAbstractModel<Integer>for DAO consistency.Same as
CaseMgmtNoteExt, this entity should extendAbstractModel<Integer>for consistency with other entities and proper DAO functionality.♻️ Proposed fix
`@Entity` `@Table`(name = "casemgmt_note_link") -public class CaseMgmtNoteLink { +public class CaseMgmtNoteLink extends AbstractModel<Integer> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/model/CaseMgmtNoteLink.java` around lines 16 - 23, Update the entity class CaseMgmtNoteLink to extend AbstractModel<Integer> for DAO consistency (i.e., change its declaration to extend AbstractModel<Integer>), remove or stop duplicating the id field declared locally (since AbstractModel already defines the primary key), and keep the JPA annotations on the inherited id mapping or adjust to `@Id/`@GeneratedValue in the base model; also add the necessary import for AbstractModel and ensure any code referencing getId()/setId() uses the inherited methods.src/test/java/ca/openosp/openo/webserv/DemographicServiceTest.java (1)
82-92: LGTM on merge workflow test update.The test correctly aligns with the new event-based merge model. The comments clearly document the A+B→C merge workflow.
Consider adding assertions after
unmergeDemographicto verify that A and B are restored to active status and C is deactivated, ensuring full round-trip coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ca/openosp/openo/webserv/DemographicServiceTest.java` around lines 82 - 92, After calling mergeService.unmergeDemographic(mergedDemoId) add assertions to verify the round-trip: fetch A and B (parent.getDemographicNo(), child.getDemographicNo()) and assert they are active, fetch C (mergedDemoId) and assert it is deactivated; you can reuse mergeService.getMergeEvent(mergedDemoId) or the service methods used elsewhere in this test to load demographics/statuses and assert expected active/inactive flags so the test fully verifies that unmerge restores A and B and deactivates C.database/mysql/updates/update-2026-04-10-demographic-merged-extend.sql (1)
12-12: Clarify:secondary_demographic_noasVARCHAR(500).Using
VARCHAR(500)for a demographic number column is unusual. If this stores comma-separated IDs for multi-record merges, consider documenting this in the schema comment or using a separate junction table for better normalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/mysql/updates/update-2026-04-10-demographic-merged-extend.sql` at line 12, The column secondary_demographic_no defined as VARCHAR(500) is unusual for storing demographic IDs; either document its intent or normalize it. Update the migration (update-2026-04-10-demographic-merged-extend.sql) to one of two fixes: (A) if you must keep a delimited list, add a column COMMENT on secondary_demographic_no explaining it stores comma-separated demographic IDs and constrain length appropriately, or (B) prefer normalization—create a new junction table (e.g., demographic_merge_links) that stores one row per merged pair (primary_demographic_id, secondary_demographic_id, merge_reason, created_at) and remove or deprecate secondary_demographic_no from the schema; apply corresponding changes to any code that reads/writes secondary_demographic_no (search for usages of secondary_demographic_no).src/test-modern/java/ca/openosp/openo/demographic/merge/DemographicMergeOperationDaoIntegrationTest.java (1)
55-59: Add the CRUD tags that the modern test suite expects.The class has the layer tags, but the individual tests are missing the
create/read/update/deletetags used for filtering the modern integration suite.Based on learnings, "Add CRUD operation tags (
@Tag("create"),@Tag("read"),@Tag("update"),@Tag("delete")) to test methods for operation filtering."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test-modern/java/ca/openosp/openo/demographic/merge/DemographicMergeOperationDaoIntegrationTest.java` around lines 55 - 59, The test class DemographicMergeOperationDaoIntegrationTest has layer tags but individual test methods need the CRUD operation tags the modern suite expects; for each test method in DemographicMergeOperationDaoIntegrationTest (e.g., methods that create records should get `@Tag`("create"), those that fetch/assert records `@Tag`("read"), methods that modify data `@Tag`("update"), and those that remove/clean up `@Tag`("delete")), add the appropriate `@Tag` annotations to the method declarations so the modern integration test runner can filter by create/read/update/delete operations.src/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeAction.java (1)
57-57: Rename this Struts2 action to the*2Actionpattern.This is a new Struts2 action, but the class name does not follow the repo's
*2Action.javaconvention.Based on learnings, "Use
*2Action.javanaming convention for all new Struts2 actions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeAction.java` at line 57, The class DemographicMergeAction does not follow the repo convention for Struts2 actions; rename the class to DemographicMerge2Action and update all references/imports/usages accordingly (e.g., servlet config/struts XML entries, any code that instantiates or references DemographicMergeAction, and package exports). Ensure the Java filename matches the new class name and update any unit tests, dependency injection bindings, and serialization identifiers (if present) for the class DemographicMerge2Action so compilation and runtime wiring continue to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9614e91c-43b7-4127-8400-3bcdc1cb9532
📒 Files selected for processing (107)
database/mysql/oscarinit.sqldatabase/mysql/oscarinit_2025.sqldatabase/mysql/updates/update-2026-04-10-demographic-merged-extend.sqlsrc/main/java/ca/openosp/openo/billing/CA/ON/model/BillingONCHeader2.javasrc/main/java/ca/openosp/openo/commn/DemographicSearchResultTransformer.javasrc/main/java/ca/openosp/openo/commn/dao/AllergyMergedDemographicDao.javasrc/main/java/ca/openosp/openo/commn/dao/AllergyMergedDemographicDaoImpl.javasrc/main/java/ca/openosp/openo/commn/dao/ConsultationRequestMergedDemographicDao.javasrc/main/java/ca/openosp/openo/commn/dao/ConsultationRequestMergedDemographicDaoImpl.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicDao.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicMergeDao.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicMergeDaoImpl.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicMergeOperationDao.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicMergeOperationDaoImpl.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicMergedDao.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicMergedDaoImpl.javasrc/main/java/ca/openosp/openo/commn/dao/DocumentMergeDemographicDAO.javasrc/main/java/ca/openosp/openo/commn/dao/DocumentMergeDemographicDAOImpl.javasrc/main/java/ca/openosp/openo/commn/dao/DocumentResultsMergedDemographicDao.javasrc/main/java/ca/openosp/openo/commn/dao/DocumentResultsMergedDemographicDaoImpl.javasrc/main/java/ca/openosp/openo/commn/dao/DrugMergedDemographicDao.javasrc/main/java/ca/openosp/openo/commn/dao/DrugMergedDemographicDaoImpl.javasrc/main/java/ca/openosp/openo/commn/dao/PreventionMergedDemographicDao.javasrc/main/java/ca/openosp/openo/commn/dao/PreventionMergedDemographicDaoImpl.javasrc/main/java/ca/openosp/openo/commn/merge/AbstractTemplate.javasrc/main/java/ca/openosp/openo/commn/merge/MergedDemographicInterceptor.javasrc/main/java/ca/openosp/openo/commn/merge/MergedDemographicTemplate.javasrc/main/java/ca/openosp/openo/commn/model/AppointmentArchive.javasrc/main/java/ca/openosp/openo/commn/model/BillingONCHeader1.javasrc/main/java/ca/openosp/openo/commn/model/BillingONItem.javasrc/main/java/ca/openosp/openo/commn/model/CaseMgmtCpp.javasrc/main/java/ca/openosp/openo/commn/model/CaseMgmtIssue.javasrc/main/java/ca/openosp/openo/commn/model/CaseMgmtIssueNotes.javasrc/main/java/ca/openosp/openo/commn/model/CaseMgmtIssueNotesPK.javasrc/main/java/ca/openosp/openo/commn/model/CaseMgmtNote.javasrc/main/java/ca/openosp/openo/commn/model/CaseMgmtNoteExt.javasrc/main/java/ca/openosp/openo/commn/model/CaseMgmtNoteLink.javasrc/main/java/ca/openosp/openo/commn/model/Consent.javasrc/main/java/ca/openosp/openo/commn/model/ConsultationRequest.javasrc/main/java/ca/openosp/openo/commn/model/ConsultationRequestExt.javasrc/main/java/ca/openosp/openo/commn/model/Demographic.javasrc/main/java/ca/openosp/openo/commn/model/DemographicContact.javasrc/main/java/ca/openosp/openo/commn/model/DemographicCustArchive.javasrc/main/java/ca/openosp/openo/commn/model/DemographicExtArchive.javasrc/main/java/ca/openosp/openo/commn/model/DemographicMerge.javasrc/main/java/ca/openosp/openo/commn/model/DemographicMerged.javasrc/main/java/ca/openosp/openo/commn/model/DigitalSignature.javasrc/main/java/ca/openosp/openo/commn/model/EFormValue.javasrc/main/java/ca/openosp/openo/commn/model/EReferAttachment.javasrc/main/java/ca/openosp/openo/commn/model/EmailAttachment.javasrc/main/java/ca/openosp/openo/commn/model/EmailLog.javasrc/main/java/ca/openosp/openo/commn/model/Measurement.javasrc/main/java/ca/openosp/openo/commn/model/MeasurementsDeleted.javasrc/main/java/ca/openosp/openo/commn/model/OLISQueryLog.javasrc/main/java/ca/openosp/openo/commn/model/OLISResults.javasrc/main/java/ca/openosp/openo/commn/model/PatientLabRouting.javasrc/main/java/ca/openosp/openo/commn/model/Prescription.javasrc/main/java/ca/openosp/openo/commn/model/Prevention.javasrc/main/java/ca/openosp/openo/commn/model/PreventionExt.javasrc/main/java/ca/openosp/openo/commn/model/Relationships.javasrc/main/java/ca/openosp/openo/demographic/data/DemographicMerged.javasrc/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManager.javasrc/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.javasrc/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeAction.javasrc/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeRecord2Action.javasrc/main/java/ca/openosp/openo/form/model/FormBCAR2020.javasrc/main/java/ca/openosp/openo/hospitalReportManager/model/HRMDocumentToDemographic.javasrc/main/java/ca/openosp/openo/lab/ca/all/upload/MessageUploader.javasrc/main/java/ca/openosp/openo/managers/DemographicManager.javasrc/main/java/ca/openosp/openo/managers/DemographicManagerImpl.javasrc/main/java/ca/openosp/openo/webserv/rest/DemographicMergeService.javasrc/main/java/ca/openosp/openo/webserv/rest/conversion/DemographicMergedConverter.javasrc/main/java/ca/openosp/openo/webserv/rest/to/model/DemographicMergedTo1.javasrc/main/java/ca/openosp/openo/webserv/transfer_objects/DemographicTransfer.javasrc/main/java/ca/openosp/openo/webserv/transfer_objects/DemographicTransfer2.javasrc/main/resources/applicationContext.xmlsrc/main/resources/ca/openosp/openo/commn/model/Demographic.hbm.xmlsrc/main/resources/oscarResources_en.propertiessrc/main/resources/oscarResources_es.propertiessrc/main/resources/oscarResources_fr.propertiessrc/main/resources/oscarResources_pl.propertiessrc/main/resources/oscarResources_pt_BR.propertiessrc/main/resources/spring_jpa.xmlsrc/main/resources/spring_managers.xmlsrc/main/webapp/WEB-INF/classes/struts.xmlsrc/main/webapp/admin/admin.jspsrc/main/webapp/admin/demographicMergeRecord.jspsrc/main/webapp/admin/demographicmergerecord.jspsrc/main/webapp/administration/leftNav.jspfsrc/main/webapp/appointment/appointmentaddarecord.jspsrc/main/webapp/appointment/appointmentaddrecordprint.jspsrc/main/webapp/demographic/demographiceditdemographic.jspsrc/main/webapp/demographic/demographicsearch2apptresults.jspsrc/main/webapp/demographic/demographicsearchresults.jspsrc/main/webapp/oscarMDS/PatientSearch.jspsrc/test-modern/java/ca/openosp/openo/demographic/merge/DemographicMergeOperationDaoIntegrationTest.javasrc/test-modern/java/ca/openosp/openo/managers/DemographicManagerUnitTest.javasrc/test-modern/java/ca/openosp/openo/managers/DemographicUnitTestBase.javasrc/test-modern/java/ca/openosp/openo/util/Doc2PDFIntegrationTest.javasrc/test-modern/resources/META-INF/persistence.xmlsrc/test-modern/resources/test-context-full.xmlsrc/test-modern/resources/test-spring_jpa.xmlsrc/test-modern/resources/test-spring_managers.xmlsrc/test/java/ca/openosp/openo/commn/dao/DemographicMergedDaoTest.javasrc/test/java/ca/openosp/openo/webserv/DemographicServiceTest.javasrc/test/resources/applicationContextTest.xml
💤 Files with no reviewable changes (31)
- src/main/resources/spring_jpa.xml
- src/test-modern/resources/test-spring_jpa.xml
- src/main/resources/spring_managers.xml
- src/main/java/ca/openosp/openo/commn/dao/ConsultationRequestMergedDemographicDao.java
- src/main/java/ca/openosp/openo/webserv/transfer_objects/DemographicTransfer2.java
- src/main/java/ca/openosp/openo/commn/merge/AbstractTemplate.java
- src/main/java/ca/openosp/openo/webserv/transfer_objects/DemographicTransfer.java
- src/main/java/ca/openosp/openo/commn/dao/ConsultationRequestMergedDemographicDaoImpl.java
- src/main/java/ca/openosp/openo/commn/dao/DemographicMergedDao.java
- src/test-modern/resources/test-spring_managers.xml
- src/main/java/ca/openosp/openo/commn/dao/AllergyMergedDemographicDao.java
- src/main/java/ca/openosp/openo/commn/model/DemographicMerged.java
- src/main/java/ca/openosp/openo/commn/dao/DocumentResultsMergedDemographicDao.java
- src/main/webapp/admin/demographicmergerecord.jsp
- src/main/java/ca/openosp/openo/commn/dao/DocumentResultsMergedDemographicDaoImpl.java
- src/main/java/ca/openosp/openo/lab/ca/all/upload/MessageUploader.java
- src/main/java/ca/openosp/openo/commn/DemographicSearchResultTransformer.java
- src/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeRecord2Action.java
- src/main/java/ca/openosp/openo/commn/dao/DocumentMergeDemographicDAO.java
- src/main/java/ca/openosp/openo/demographic/data/DemographicMerged.java
- src/main/java/ca/openosp/openo/commn/dao/DocumentMergeDemographicDAOImpl.java
- src/main/java/ca/openosp/openo/commn/dao/PreventionMergedDemographicDaoImpl.java
- src/main/java/ca/openosp/openo/commn/dao/PreventionMergedDemographicDao.java
- src/main/java/ca/openosp/openo/commn/dao/AllergyMergedDemographicDaoImpl.java
- src/test/java/ca/openosp/openo/commn/dao/DemographicMergedDaoTest.java
- src/main/java/ca/openosp/openo/commn/merge/MergedDemographicInterceptor.java
- src/main/java/ca/openosp/openo/commn/merge/MergedDemographicTemplate.java
- src/main/java/ca/openosp/openo/commn/dao/DemographicMergedDaoImpl.java
- src/main/resources/ca/openosp/openo/commn/model/Demographic.hbm.xml
- src/main/java/ca/openosp/openo/commn/dao/DrugMergedDemographicDao.java
- src/main/java/ca/openosp/openo/commn/dao/DrugMergedDemographicDaoImpl.java
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
28 issues found across 107 files
Confidence score: 2/5
- High risk due to data loss and integrity issues:
database/mysql/updates/update-2026-04-10-demographic-merged-extend.sqldrops and recreatesdemographic_merged, which would delete existing merge history during upgrades. - Several merge-flow problems can leave records in inconsistent states or reactivate inactive patients, e.g., non-atomic
merge()insrc/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManager.javaand lax status validation insrc/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.java. - User-facing regressions are likely:
src/main/webapp/appointment/appointmentaddarecord.jspcan attach appointments to inactive pre-merge demographics, andsrc/main/webapp/demographic/demographiceditdemographic.jspexposes edit mode for merged secondary records. - Pay close attention to
database/mysql/updates/update-2026-04-10-demographic-merged-extend.sql,src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManager.java,src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.java,src/main/webapp/appointment/appointmentaddarecord.jsp,src/main/webapp/demographic/demographiceditdemographic.jsp- data preservation, merge atomicity, and correct target records.
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed. cubic prioritises the most important files to review.
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/demographic/merge/DemographicMergeManager.java">
<violation number="1" location="src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManager.java:81">
P1: `merge()` is not an atomic public operation. Requiring callers to separately invoke `applyMergeStatuses()` can leave both the merged chart and the source charts active if the second call is skipped or fails.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/commn/model/Relationships.java">
<violation number="1" location="src/main/java/ca/openosp/openo/commn/model/Relationships.java:150">
P2: The new `Boolean` setter can throw a `NullPointerException` when `null` is passed, because the field is still a primitive `boolean`.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.java">
<violation number="1" location="src/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.java:108">
P1: Exclude demographics that were later unmerged; this query still returns stale MERGE audit rows.</violation>
<violation number="2" location="src/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.java:1908">
P1: Parameterize `assignedToProviderNo` instead of concatenating it into the SQL restriction.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/commn/model/BillingONItem.java">
<violation number="1" location="src/main/java/ca/openosp/openo/commn/model/BillingONItem.java:88">
P1: Do not expose a public setter for this auto-generated entity ID; mutating it can corrupt entity identity and bypass the database-generated key.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/commn/model/Measurement.java">
<violation number="1" location="src/main/java/ca/openosp/openo/commn/model/Measurement.java:167">
P1: Do not expose a public setter for this generated primary key. It lets callers mutate the entity's identity, which conflicts with the class's immutability contract and breaks `equals`/`hashCode` semantics in `AbstractModel`.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/webserv/rest/to/model/DemographicMergedTo1.java">
<violation number="1" location="src/main/java/ca/openosp/openo/webserv/rest/to/model/DemographicMergedTo1.java:44">
P2: Renaming fields on `DemographicMergedTo1` breaks the existing v1 REST contract. If this payload needs new semantics, introduce a new transfer-object version or preserve the old serialized property names.</violation>
</file>
<file name="src/main/webapp/demographic/demographiceditdemographic.jsp">
<violation number="1" location="src/main/webapp/demographic/demographiceditdemographic.jsp:1339">
P2: This now renders the current demographic as the merge head, so merged child records lose the link back to their primary record.</violation>
<violation number="2" location="src/main/webapp/demographic/demographiceditdemographic.jsp:1353">
P1: The hardcoded `if (true)` exposes Edit mode for merged secondary records that were previously blocked.</violation>
</file>
<file name="src/main/webapp/WEB-INF/classes/struts.xml">
<violation number="1" location="src/main/webapp/WEB-INF/classes/struts.xml:1230">
P2: Preserve `mode=unmerge` on the shared failure redirect; otherwise unmerge errors bounce users back into merge mode.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/commn/model/MeasurementsDeleted.java">
<violation number="1" location="src/main/java/ca/openosp/openo/commn/model/MeasurementsDeleted.java:91">
P2: Keep the generated primary key read-only. A public `setId` allows callers to mutate entity identity even though this row already has `originalId` for tracking the source measurement.</violation>
</file>
<file name="src/main/webapp/appointment/appointmentaddrecordprint.jsp">
<violation number="1" location="src/main/webapp/appointment/appointmentaddrecordprint.jsp:103">
P1: Persisting the raw `demographic_no` here can link new appointments to an inactive pre-merge patient record instead of the active merged demographic.</violation>
</file>
<file name="src/main/webapp/demographic/demographicsearch2apptresults.jsp">
<violation number="1" location="src/main/webapp/demographic/demographicsearch2apptresults.jsp:69">
P1: Keep excluding non-head merged demographics from appointment search results; otherwise merged child records become selectable for new appointments again.</violation>
</file>
<file name="database/mysql/updates/update-2026-04-10-demographic-merged-extend.sql">
<violation number="1" location="database/mysql/updates/update-2026-04-10-demographic-merged-extend.sql:7">
P0: Dropping and recreating `demographic_merged` here deletes existing merge history during upgrades; this migration needs a data-preserving transform instead of a hard reset.</violation>
<violation number="2" location="database/mysql/updates/update-2026-04-10-demographic-merged-extend.sql:18">
P2: Add an index that leads with `primary_demographic_no`; the current schema leaves `getMergedDemographics(...)` without a selective index for its `WHERE primary_demographic_no = ? AND event_type = 'MERGE'` lookup.</violation>
</file>
<file name="src/main/webapp/administration/leftNav.jspf">
<violation number="1" location="src/main/webapp/administration/leftNav.jspf:898">
P1: This link now targets an action that checks `_demographic` but not `_admin`, which makes an admin-only merge workflow directly reachable by non-admin users who know the URL.</violation>
</file>
<file name="src/main/webapp/appointment/appointmentaddarecord.jsp">
<violation number="1" location="src/main/webapp/appointment/appointmentaddarecord.jsp:140">
P1: Persisting the raw `demographic_no` can attach new appointments to an inactive pre-merge record instead of the active merged demographic.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/managers/DemographicManagerImpl.java">
<violation number="1" location="src/main/java/ca/openosp/openo/managers/DemographicManagerImpl.java:1390">
P2: The new unmerge phone search only matches `phone`, so merged patients with matches in `phone2` or the cell-phone extension become impossible to find by phone.</violation>
</file>
<file name="src/main/webapp/admin/demographicMergeRecord.jsp">
<violation number="1" location="src/main/webapp/admin/demographicMergeRecord.jsp:372">
P2: `resultCount` is the current page size, so this `Next` check renders dead-end pagination on full last pages and misreports the number of matches.</violation>
<violation number="2" location="src/main/webapp/admin/demographicMergeRecord.jsp:487">
P2: Validate that a merged record is selected before showing the unmerge confirmation; otherwise an empty submission falls straight into the generic failure path.</violation>
<violation number="3" location="src/main/webapp/admin/demographicMergeRecord.jsp:497">
P2: Guard the merge confirm button against repeat clicks; otherwise duplicate hidden inputs can make the server reject an otherwise valid merge.</violation>
</file>
<file name="src/main/webapp/demographic/demographicsearchresults.jsp">
<violation number="1" location="src/main/webapp/demographic/demographicsearchresults.jsp:439">
P1: Resolve merged search results to the primary demographic before opening the edit page.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/commn/dao/DemographicMergeOperationDaoImpl.java">
<violation number="1" location="src/main/java/ca/openosp/openo/commn/dao/DemographicMergeOperationDaoImpl.java:1">
P1: `DemographicExtArchive.archiveId` needs remapping to the new `DemographicArchive` IDs; copying the source `archiveId` breaks the archive linkage.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeAction.java">
<violation number="1" location="src/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeAction.java:127">
P2: Clamp `limit2`; this endpoint currently lets a request fan out into arbitrarily large merge/unmerge searches.</violation>
</file>
<file name="database/mysql/oscarinit.sql">
<violation number="1" location="database/mysql/oscarinit.sql:8253">
P1: `secondary_demographic_no` is too short for an unbounded comma-separated ID list, so large merges can produce incomplete audit data and break unmerge.</violation>
</file>
<file name="src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.java">
<violation number="1" location="src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.java:99">
P2: Avoid writing merge/unmerge identifiers to stdout; this bypasses the configured logger and exposes patient/provider IDs in server logs.</violation>
<violation number="2" location="src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.java:489">
P1: This validation only rejects `IN`, so other non-active patients can still be merged and later get reactivated as `AC`.</violation>
</file>
<file name="database/mysql/oscarinit_2025.sql">
<violation number="1" location="database/mysql/oscarinit_2025.sql:791">
P2: Explicitly set the storage engine to InnoDB when creating this table.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeAction.java (1)
57-57: Rename this new Struts2 action to follow the*2Actionconvention.This is a new Struts2 action, but it still uses the legacy
*Actionnaming. Renaming it now keeps the migration pattern consistent before more mappings and references build around it. As per coding guidelines, "Use *2Action.java naming convention for all new Struts2 actions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeAction.java` at line 57, Rename the new Struts2 action class DemographicMergeAction to follow the *2Action convention (e.g., DemographicMerge2Action): update the public class declaration from DemographicMergeAction to DemographicMerge2Action, rename the containing file accordingly, and adjust all references/usages (imports, instantiations, struts config/action mappings, tests) to the new class name so compilation and wiring remain correct; ensure any constructors, serialVersionUID or annotations remain intact after the rename.
🤖 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/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.java`:
- Around line 487-493: The loadAndValidateNotMerged method currently only
rejects demographics with STATUS_INACTIVE but allows previously merged targets
(e.g., records with merged state like "AC"/"C"); update loadAndValidateNotMerged
to also query the merge-event repository/table (use the service/repo used
elsewhere for merge events) to check whether demographicNo is already a target
of a completed merge event and throw an IllegalStateException with a clear
message if so; keep the existing check for STATUS_INACTIVE and reuse
loadAndValidateExists(demographicNo, label) at the start, then perform the
merge-event existence check (by demographicNo) before returning the Demographic.
- Around line 329-357: requestPkMap produced by copyConsultationsGroup(...) is
never added to linkedEntityPkMaps, so casemgmt_note_link entries for
consultation requests won't be remapped; before calling
copyCasemgmtNoteGroup(...) add requestPkMap to linkedEntityPkMaps (e.g., if
(!requestPkMap.isEmpty())
linkedEntityPkMaps.put(CaseManagementNoteLink.CONSULTATION_REQUEST,
requestPkMap)) so the DAO remap logic sees the consultation-request PK map when
processing notes.
In
`@src/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeAction.java`:
- Around line 198-200: The catch blocks in DemographicMergeAction.doMerge (and
the similar catch at lines ~231-233) currently call
logger.error("DemographicMergeAction.doMerge: merge failed", e) which may log
PHI from exception details; replace these logger.error calls with a
generic/redacted message (e.g. "DemographicMergeAction.doMerge: merge failed -
details recorded in audit trail") and DO NOT pass the exception object to the
logger. Keep the full exception/validation details only in the audited merge
records or the existing LogAction trail so debugging info is preserved without
writing patient identifiers to routine logs.
- Around line 121-126: In DemographicMergeAction (around the offset/limit
parsing), don't accept negative or huge values as-is: parse
request.getParameter("limit1") and "limit2" into temporaries, on
NumberFormatException leave offset/limit as their existing defaults, then clamp
offset = Math.max(0, parsedOffset) and clamp limit to a sane range (e.g. if
parsedLimit <= 0 use existing default limit, else limit = Math.min(parsedLimit,
MAX_LIMIT)); use the existing offset and limit variables as the fallbacks and
introduce a MAX_LIMIT constant if needed so invalid or out-of-range inputs never
propagate into the demographic search layer.
In `@src/main/webapp/demographic/demographiceditdemographic.jsp`:
- Around line 1339-1344: The JSP outputs raw demographic_no and mergedId values
into both the href query string and the link text; replace those raw scriptlet
expressions so the query-param values are encoded with Encode.forUriComponent()
when inserted into the URL and the visible link text is encoded with
Encode.forHtml() (or Encode.forHtmlAttribute() for any attribute context) to
prevent injection. Update the occurrences where demographic_no and each mergedId
are rendered in the href and between the <a> tags (the scriptlet expressions
referencing demographic_no and mergedId) to call the appropriate OWASP Encoder
methods (e.g., Encode.forUriComponent(demographic_no) for the URL param and
Encode.forHtml(demographic_no) for the link text) and do the same for mergedId
iterations.
- Line 1353: The code is incorrectly using demographic.getPatientStatus() ==
"IN" to block edits, which hides editing for any inactive record; instead change
the condition to explicitly detect merged-child records (for example, use a
dedicated flag or relation such as demographic.isMergedChild(),
demographic.getMergeParentId() != null, or demographic.getMergeState() ==
MergeState.MERGED_CHILD) and only block edits when that merge-specific check is
true; leave other inactive statuses unaffected so non-merged inactive
demographics still follow the original edit rules.
- Around line 1336-1345: The current code uses
demographicManager.getMergedDemographicIds(loggedInInfo,
Integer.parseInt(demographic_no)) which reads raw event_type='MERGE' rows and
therefore can show stale or one-way links; replace that call with the helper
that resolves the current active merge relationships (e.g.,
demographicManager.getActiveMergedDemographicIds or a provided
resolveActiveMergeForDemographic helper) so the JSP renders only currently
active merged IDs and includes the link back to the active primary when viewing
a secondary; update the call site in demographiceditdemographic.jsp where
mergedIds is obtained and ensure the returned list is filtered to exclude
unmerged/stale entries before rendering the anchor links.
---
Nitpick comments:
In
`@src/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeAction.java`:
- Line 57: Rename the new Struts2 action class DemographicMergeAction to follow
the *2Action convention (e.g., DemographicMerge2Action): update the public class
declaration from DemographicMergeAction to DemographicMerge2Action, rename the
containing file accordingly, and adjust all references/usages (imports,
instantiations, struts config/action mappings, tests) to the new class name so
compilation and wiring remain correct; ensure any constructors, serialVersionUID
or annotations remain intact after the rename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3eee10c-0ac1-4b12-bf9a-817577cc2a24
📒 Files selected for processing (5)
src/main/java/ca/openosp/openo/commn/dao/DemographicMergeOperationDaoImpl.javasrc/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.javasrc/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeAction.javasrc/main/webapp/WEB-INF/classes/struts.xmlsrc/main/webapp/demographic/demographiceditdemographic.jsp
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/WEB-INF/classes/struts.xml
…demographic_merged
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.java (1)
1908-1910:⚠️ Potential issue | 🔴 CriticalParameterize the provider filter instead of concatenating it.
assignedToProviderNocomes from the search form and is interpolated intosqlRestriction, which reopens SQL injection on a PHI search path. This remains the same issue previously flagged in this area.🛡️ Proposed fix using a bound SQL restriction parameter
+import org.hibernate.type.StandardBasicTypes;- sql = " demographic_no in (select distinct a.client_id from admission a where a.primaryWorker='" - + assignedToProviderNo + "')"; - criteria.add(Restrictions.sqlRestriction(sql)); + sql = " demographic_no in (select distinct a.client_id from admission a where a.primaryWorker=?)"; + criteria.add(Restrictions.sqlRestriction(sql, assignedToProviderNo, StandardBasicTypes.STRING));As per coding guidelines, "Use parameterized queries ONLY - never use string concatenation in SQL queries."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.java` around lines 1908 - 1910, The code concatenates assignedToProviderNo into a SQL fragment which enables SQL injection; update the DemographicDaoImpl usage to a parameterized sqlRestriction by replacing the string concat with a placeholder and passing assignedToProviderNo as a bound value (e.g., criteria.add(Restrictions.sqlRestriction("demographic_no in (select distinct a.client_id from admission a where a.primaryWorker=?)", new Object[]{assignedToProviderNo}, new Type[]{StringType.INSTANCE}))); ensure you import the appropriate Hibernate Type (StringType) and use the Restrictions.sqlRestriction overload that accepts values and types instead of building the SQL via string concatenation.
🤖 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/java/ca/openosp/openo/commn/dao/DemographicDao.java`:
- Around line 285-336: Add missing `@since` Javadoc tags to each of the new public
DAO methods: findActiveMergedDemographicByName,
findActiveMergedDemographicByDOB, findActiveMergedDemographicByPhone,
findActiveMergedDemographicByHIN, and findActiveMergedDemographicByAddress;
obtain the correct date for each tag from git history using the recommended
command (git log --follow --format="%ai"
src/main/java/ca/openosp/openo/commn/dao/DemographicDao.java | tail -1) and
insert that date into each method's JavaDoc `@since` line so every public method
has the required `@since` entry.
In `@src/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.java`:
- Around line 3132-3141: The SQL in findActiveMergedDemographicByPhone only
matches de.phone so merged records with phone2 or demo_cell are missed; update
the query string to include a phone OR condition (e.g. WHERE (de.phone LIKE
:phone OR de.phone2 LIKE :phone OR de.demo_cell LIKE :phone) ) before APPENDING
ACTIVE_MERGED_SUBQUERY so the active-merged filtering remains intact, keeping
the parameter name "phone" and existing pagination/SQLQuery usage (method
findActiveMergedDemographicByPhone, constant ACTIVE_MERGED_SUBQUERY).
- Around line 107-109: The current SQL in DemographicDaoImpl that selects
merged_demographic_no from demographic_merged_event with only event_type =
'MERGE' can return historical merges that were later reversed; change the query
to only return merged_demographic_no values that are currently active by
excluding records that have a later UNMERGE for the same
parentId/merged_demographic_no (or by selecting the latest event per
parentId/merged_demographic_no and ensuring its event_type = 'MERGE'). Update
the SQLQuery (the session.createSQLQuery call and its parameter
setInteger("parentId", demographicNo)) to use a subquery or aggregation (e.g.,
max(event_time) per pair or not exists a later UNMERGE) so only active merges
are returned.
---
Duplicate comments:
In `@src/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.java`:
- Around line 1908-1910: The code concatenates assignedToProviderNo into a SQL
fragment which enables SQL injection; update the DemographicDaoImpl usage to a
parameterized sqlRestriction by replacing the string concat with a placeholder
and passing assignedToProviderNo as a bound value (e.g.,
criteria.add(Restrictions.sqlRestriction("demographic_no in (select distinct
a.client_id from admission a where a.primaryWorker=?)", new
Object[]{assignedToProviderNo}, new Type[]{StringType.INSTANCE}))); ensure you
import the appropriate Hibernate Type (StringType) and use the
Restrictions.sqlRestriction overload that accepts values and types instead of
building the SQL via string concatenation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1b14d42-7ccf-4e21-816f-71a86f096700
📒 Files selected for processing (5)
database/mysql/oscarinit_2025.sqldatabase/mysql/updates/update-2026-04-10-demographic-merged-extend.sqlsrc/main/java/ca/openosp/openo/commn/dao/DemographicDao.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.javasrc/main/java/ca/openosp/openo/commn/model/DemographicMerge.java
🚧 Files skipped from review as they are similar to previous changes (2)
- database/mysql/updates/update-2026-04-10-demographic-merged-extend.sql
- database/mysql/oscarinit_2025.sql
…ng to merge links
…fo on demographic edit
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.java (1)
97-147:⚠️ Potential issue | 🟠 MajorPHI still emitted to stdout and debug logs throughout merge/unmerge.
The numerous
System.out.println(...)blocks (and thelogger.debug(...)calls at Lines 118, 174, 233–234, 329, 379) still embedprimaryDemographicNo,secondaryDemographicNos,targetDemographicNo,mergedDemographicNoandloggedInInfo.getLoggedInProviderNo(). These identifiers are PHI under the project's coding guidelines, and stdout in particular is captured by most container/log-aggregation pipelines outside of the intended audit trail (which you already persist correctly viasaveMergeEvent+LogAction.addLog).Please strip these before merging rather than relying on a post-test cleanup pass — the existing
saveMergeEvent/LogActionwrites already give you the durable, access-controlled record. As per coding guidelines, "PHI (Patient Health Information) must NEVER be logged or exposed."Also applies to: 163-173, 184-243, 325-328, 347-347, 370-370, 373-373, 380-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.java` around lines 97 - 147, The merge method in DemographicMergeManagerImpl is emitting PHI via System.out.println and logger.debug calls (referencing primaryDemographicNo, secondaryDemographicNos, targetDemographicNo, mergedDemographicNo, loggedInInfo.getLoggedInProviderNo()) — remove or sanitize those outputs and keep only non-PHI diagnostics (e.g., status messages or counts). Specifically, in merge (and surrounding blocks that call cloneDemographic, saveMergeEvent, copyAllDataForSource, writeAuditEntriesForMerge) replace all System.out.println(...) usages and any logger.debug(...) that includes identifiers with either no-op or generic messages like "Merge started/completed" and "N secondaries processed" (do not log demographicNos or provider IDs); leave the audit persistence calls (saveMergeEvent and LogAction.addLog / writeAuditEntriesForMerge) intact so the required audit trail remains. Ensure no PHI variables (primaryDemographicNo, secondaryDemographicNos, targetDemographicNo, mergedDemographicNo, loggedInInfo.getLoggedInProviderNo()) are concatenated into any stdout/stderr/log statements.
🧹 Nitpick comments (4)
src/main/java/ca/openosp/openo/commn/dao/DemographicMergeDaoImpl.java (2)
106-117: Primary-match query lacks deterministic ordering.
findActiveMergeEventForSourceprimary branch limits to 1 but does notORDER BY e.eventDate DESC. If a demographic were ever the primary of two merge events where only one has a subsequent UNMERGE and the subquery cannot distinguish which merged record the active one refers to, the returned row is non‑deterministic. The sibling methodfindActiveMergeEventForMergedRecordat Line 94 already includesorder by e.eventDate desc; mirror that here for consistency.♻️ Proposed change
Query primaryQ = entityManager.createQuery( - "select e from DemographicMerge e where e.primaryDemographicNo = :demoNo and e.eventType = :mergeType and e.mergedDemographicNo not in (select u.mergedDemographicNo from DemographicMerge u where u.eventType = :unmergeType)"); + "select e from DemographicMerge e where e.primaryDemographicNo = :demoNo and e.eventType = :mergeType and e.mergedDemographicNo not in (select u.mergedDemographicNo from DemographicMerge u where u.eventType = :unmergeType) order by e.eventDate desc");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/dao/DemographicMergeDaoImpl.java` around lines 106 - 117, The primary-branch JPQL in findActiveMergeEventForSource uses setMaxResults(1) without ordering, causing nondeterministic results; update the Query (primaryQ) to include "order by e.eventDate desc" so the newest merge wins (mirror the ordering used in findActiveMergeEventForMergedRecord), leaving the rest of the parameters and maxResults intact.
79-89: N+1 native queries in a loop — consider a single set-based query.For each unmatched id, the secondary branch issues a separate
find_in_setnative query. For small search-result page sizes this is fine, but callers such asfindMergedSourcesAmongare invoked against the recently-accessed list / search result sets. If that set grows, this loop becomes an N‑round‑trip scan. A single native query usingfind_in_setagainst the whole collection (e.g.sum(find_in_set(id, sec)) > 0grouped over the IN list) or a derived table of the candidate ids would collapse this to one round‑trip. Acceptable as‑is for admin‑only flows; flagging only as a recommended improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/commn/dao/DemographicMergeDaoImpl.java` around lines 79 - 89, The loop issuing a separate native query per id (using createNativeQuery with find_in_set) should be replaced by a single set-based native query to avoid N+1 round trips; collect the unmatched demographicIds (the ones not in result) and execute one native query that uses a derived table or IN-list of those ids and FIND_IN_SET/REPLACE on secondary_demographic_no (e.g. join a VALUES-list of ids or use WHERE id IN (...) combined with SUM(FIND_IN_SET(id, REPLACE(secondary_demographic_no,' ',''))) > 0 grouped by id) to return all matching ids in one call, then add those returned ids to result (preserving use sites like findMergedSourcesAmong and referencing demographicIds, result, createNativeQuery, and the secondary_demographic_no FIND_IN_SET logic).src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.java (1)
302-309: UseSecurityExceptionfor privilege failures, not bareRuntimeException.
checkPrivilegethrowsRuntimeExceptionon missing_demographic/_adminprivilege. Callers (the Struts action, REST service, any upstream interceptor) cannot distinguish these from any other runtime error, which makes it easy to silently swallow an authorization failure. ASecurityException(or a dedicated authorization exception) fails more clearly and aligns with the coding guideline to "Never skip or ignore SecurityException or security checks - always fail securely." The message should also not include the action details verbatim if any remapping is added later.🛡️ Proposed change
- private void checkPrivilege(LoggedInInfo loggedInInfo, String privilege) { - if (!securityInfoManager.hasPrivilege(loggedInInfo, "_demographic", privilege, null)) { - throw new RuntimeException("missing required sec object (_demographic)"); - } - if (!securityInfoManager.hasPrivilege(loggedInInfo, "_admin", privilege, null)) { - throw new RuntimeException("missing required sec object (_admin)"); - } - } + private void checkPrivilege(LoggedInInfo loggedInInfo, String privilege) { + if (!securityInfoManager.hasPrivilege(loggedInInfo, "_demographic", privilege, null)) { + throw new SecurityException("missing required sec object (_demographic)"); + } + if (!securityInfoManager.hasPrivilege(loggedInInfo, "_admin", privilege, null)) { + throw new SecurityException("missing required sec object (_admin)"); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.java` around lines 302 - 309, The method checkPrivilege(LoggedInInfo, String) currently throws bare RuntimeException on failed securityInfoManager.hasPrivilege checks; change these to throw a SecurityException (or a project-specific AuthorizationException if available) with a concise, non-action-specific message like "missing required privilege" (avoid embedding action names verbatim), and update any callers or signatures if they need to declare/handle the checked exception; ensure both privilege checks (for "_demographic" and "_admin") use the new exception type so privilege failures are clearly distinguishable from other runtime errors.src/main/webapp/demographic/demographiceditdemographic.jsp (1)
1337-1340: Move the merge-header lookup out of the JSP.This adds more service-locator and control-flow logic directly in the view. Please resolve the merge header state before rendering and pass it in via request attributes instead of scriptlets.
As per coding guidelines, "JSP views should use JSTL tags and avoid Java scriptlets - use request.getAttribute() for passing data".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/demographic/demographiceditdemographic.jsp` around lines 1337 - 1340, The JSP currently performs service-locator and control-flow work: retrieving DemographicMergeManager via SpringUtils, parsing demographic_no, and calling DemographicMergeManager.findActiveMergeEventForMergedRecord and findActiveMergeEventForSource to set mergeEventAsMergedRecord/mergeEventAsSource; move this logic into the controller/servlet (or a request preprocessor) so the controller fetches DemographicMergeManager, parses demographic_no, resolves the merge header by calling findActiveMergeEventForMergedRecord and (if null) findActiveMergeEventForSource, and then puts the resulting mergeEventAsMergedRecord and mergeEventAsSource into request attributes; update the JSP to remove these scriptlets and read the prepared objects via request.getAttribute().
🤖 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/demographic/demographiceditdemographic.jsp`:
- Line 1353: Remove the stray closing parenthesis rendered at line with "%>)" in
demographiceditdemographic.jsp that produces a dangling ")" in the patient
detail header; locate the literal "%>)" (or the lone ")" output near the patient
detail header rendering) and delete the extraneous ")" so the header text no
longer shows the dangling character.
---
Duplicate comments:
In
`@src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.java`:
- Around line 97-147: The merge method in DemographicMergeManagerImpl is
emitting PHI via System.out.println and logger.debug calls (referencing
primaryDemographicNo, secondaryDemographicNos, targetDemographicNo,
mergedDemographicNo, loggedInInfo.getLoggedInProviderNo()) — remove or sanitize
those outputs and keep only non-PHI diagnostics (e.g., status messages or
counts). Specifically, in merge (and surrounding blocks that call
cloneDemographic, saveMergeEvent, copyAllDataForSource,
writeAuditEntriesForMerge) replace all System.out.println(...) usages and any
logger.debug(...) that includes identifiers with either no-op or generic
messages like "Merge started/completed" and "N secondaries processed" (do not
log demographicNos or provider IDs); leave the audit persistence calls
(saveMergeEvent and LogAction.addLog / writeAuditEntriesForMerge) intact so the
required audit trail remains. Ensure no PHI variables (primaryDemographicNo,
secondaryDemographicNos, targetDemographicNo, mergedDemographicNo,
loggedInInfo.getLoggedInProviderNo()) are concatenated into any
stdout/stderr/log statements.
---
Nitpick comments:
In `@src/main/java/ca/openosp/openo/commn/dao/DemographicMergeDaoImpl.java`:
- Around line 106-117: The primary-branch JPQL in findActiveMergeEventForSource
uses setMaxResults(1) without ordering, causing nondeterministic results; update
the Query (primaryQ) to include "order by e.eventDate desc" so the newest merge
wins (mirror the ordering used in findActiveMergeEventForMergedRecord), leaving
the rest of the parameters and maxResults intact.
- Around line 79-89: The loop issuing a separate native query per id (using
createNativeQuery with find_in_set) should be replaced by a single set-based
native query to avoid N+1 round trips; collect the unmatched demographicIds (the
ones not in result) and execute one native query that uses a derived table or
IN-list of those ids and FIND_IN_SET/REPLACE on secondary_demographic_no (e.g.
join a VALUES-list of ids or use WHERE id IN (...) combined with
SUM(FIND_IN_SET(id, REPLACE(secondary_demographic_no,' ',''))) > 0 grouped by
id) to return all matching ids in one call, then add those returned ids to
result (preserving use sites like findMergedSourcesAmong and referencing
demographicIds, result, createNativeQuery, and the secondary_demographic_no
FIND_IN_SET logic).
In
`@src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.java`:
- Around line 302-309: The method checkPrivilege(LoggedInInfo, String) currently
throws bare RuntimeException on failed securityInfoManager.hasPrivilege checks;
change these to throw a SecurityException (or a project-specific
AuthorizationException if available) with a concise, non-action-specific message
like "missing required privilege" (avoid embedding action names verbatim), and
update any callers or signatures if they need to declare/handle the checked
exception; ensure both privilege checks (for "_demographic" and "_admin") use
the new exception type so privilege failures are clearly distinguishable from
other runtime errors.
In `@src/main/webapp/demographic/demographiceditdemographic.jsp`:
- Around line 1337-1340: The JSP currently performs service-locator and
control-flow work: retrieving DemographicMergeManager via SpringUtils, parsing
demographic_no, and calling
DemographicMergeManager.findActiveMergeEventForMergedRecord and
findActiveMergeEventForSource to set
mergeEventAsMergedRecord/mergeEventAsSource; move this logic into the
controller/servlet (or a request preprocessor) so the controller fetches
DemographicMergeManager, parses demographic_no, resolves the merge header by
calling findActiveMergeEventForMergedRecord and (if null)
findActiveMergeEventForSource, and then puts the resulting
mergeEventAsMergedRecord and mergeEventAsSource into request attributes; update
the JSP to remove these scriptlets and read the prepared objects via
request.getAttribute().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2782bdd6-27ca-4f0d-9df2-cd73942a53fc
📒 Files selected for processing (6)
src/main/java/ca/openosp/openo/commn/dao/DemographicMergeDao.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicMergeDaoImpl.javasrc/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManager.javasrc/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.javasrc/main/webapp/demographic/demographiceditdemographic.jspsrc/main/webapp/demographic/demographicsearchresults.jsp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/demographic/demographicsearchresults.jsp
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/webapp/demographic/demographicsearchresults.jsp">
<violation number="1" location="src/main/webapp/demographic/demographicsearchresults.jsp:336">
P1: This new merge-source lookup can throw authorization exceptions for non-admin users on the patient search page, because it calls a manager method that requires `_admin` read privilege while the page only requires `_search`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…icMergeManagerImpl.checkPrivilege
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/managers/DemographicManagerImpl.java (1)
1365-1383: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd JavaDoc documentation to this public method.
As per coding guidelines, all public methods must have comprehensive JavaDoc documentation including
@paramtags with specific data types,@returntags with exact return types, and@throwstags for any exceptions.📝 Proposed JavaDoc
+ /** + * Searches active demographics for potential merge candidates. + * + * `@param` loggedInInfo LoggedInInfo the current user session context + * `@param` keyword String the search term to match against demographics + * `@param` searchMode String the search field type (search_name, search_dob, search_phone, search_hin, search_address) + * `@param` limit int maximum number of results to return + * `@param` offset int starting position for pagination + * `@return` List<Demographic> list of matching active demographics + */ `@Override` public List<Demographic> searchDemographicsForMerge(LoggedInInfo loggedInInfo, String keyword,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ca/openosp/openo/managers/DemographicManagerImpl.java` around lines 1365 - 1383, Add a JavaDoc block to the public method DemographicManagerImpl.searchDemographicsForMerge describing its purpose and each parameter (LoggedInInfo loggedInInfo, String keyword, String searchMode, int limit, int offset) with concrete types and expected values/semantics, include an explicit `@return` tag documenting the exact return type List<Demographic> and contents, and add `@throws` tags for any runtime exceptions the method can surface (e.g. SecurityException from checkPrivilege or any DAO-related exceptions) so callers know error conditions; mention side-effects such as logging via LogAction.addLogSynchronous and any preconditions (e.g. non-null loggedInInfo).
♻️ Duplicate comments (1)
src/main/java/ca/openosp/openo/managers/DemographicManagerImpl.java (1)
1385-1403: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd JavaDoc documentation to this public method.
As per coding guidelines, all public methods must have comprehensive JavaDoc documentation including
@paramtags with specific data types,@returntags with exact return types, and@throwstags for any exceptions.Additionally, the
providerNovariable on line 1389 is unused and should be removed.📝 Proposed fix
+ /** + * Searches merged demographics for potential unmerge candidates. + * + * `@param` loggedInInfo LoggedInInfo the current user session context + * `@param` keyword String the search term to match against demographics + * `@param` searchMode String the search field type (search_name, search_dob, search_phone, search_hin, search_address) + * `@param` limit int maximum number of results to return + * `@param` offset int starting position for pagination + * `@return` List<Demographic> list of matching active merged demographics + */ `@Override` public List<Demographic> searchMergedDemographicsForUnmerge(LoggedInInfo loggedInInfo, String keyword, String searchMode, int limit, int offset) { checkPrivilege(loggedInInfo, SecurityInfoManager.READ); - String providerNo = loggedInInfo.getLoggedInProviderNo(); List<Demographic> results;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ca/openosp/openo/managers/DemographicManagerImpl.java` around lines 1385 - 1403, The public method searchMergedDemographicsForUnmerge is missing JavaDoc and also declares an unused local variable providerNo; add comprehensive JavaDoc above the method describing its purpose and include `@param` tags for loggedInInfo (LoggedInInfo), keyword (String), searchMode (String), limit (int), offset (int), an `@return` tag specifying List<Demographic>, and an `@throws` tag for any security or DAO exceptions thrown by checkPrivilege or demographicDao calls; then remove the unused providerNo variable declaration to eliminate the dead local variable (ensure you keep the call to checkPrivilege and the demographicDao.* calls intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/ca/openosp/openo/lab/ca/all/upload/MessageUploader.java`:
- Around line 477-509: The executeHinMatchQuery method currently opens
PreparedStatement and ResultSet twice and closes them manually, risking leaks on
exceptions; refactor it to use try-with-resources for both the initial query and
the retry query so PreparedStatement and ResultSet are auto-closed, preserving
the same logic that builds PatientLabRoutingResult (using Misc.getString for
demographic_no and provider_no) and returns the single match or null; ensure the
second try-with-resources block appends " and patient_status = 'AC' " to the sql
and computes countAc the same way before returning countAc == 1 ? result : null.
In `@src/main/java/ca/openosp/openo/lab/ca/on/CML/ABCDParser.java`:
- Around line 145-173: The executeHinMatchQuery method opens PreparedStatement
and ResultSet twice without try-with-resources, risking leaks if an exception
occurs; refactor the method to use try-with-resources for each query execution
(wrap the PreparedStatement and ResultSet in a single try(...) block), e.g. for
the initial pstmt/rs and for the retry pstmt/rs with " and patient_status = 'AC'
", ensure both ResultSet and PreparedStatement are declared in the try header so
they are always closed, and preserve the same logic/return values (use the same
local variables demo and count inside each try block).
---
Outside diff comments:
In `@src/main/java/ca/openosp/openo/managers/DemographicManagerImpl.java`:
- Around line 1365-1383: Add a JavaDoc block to the public method
DemographicManagerImpl.searchDemographicsForMerge describing its purpose and
each parameter (LoggedInInfo loggedInInfo, String keyword, String searchMode,
int limit, int offset) with concrete types and expected values/semantics,
include an explicit `@return` tag documenting the exact return type
List<Demographic> and contents, and add `@throws` tags for any runtime exceptions
the method can surface (e.g. SecurityException from checkPrivilege or any
DAO-related exceptions) so callers know error conditions; mention side-effects
such as logging via LogAction.addLogSynchronous and any preconditions (e.g.
non-null loggedInInfo).
---
Duplicate comments:
In `@src/main/java/ca/openosp/openo/managers/DemographicManagerImpl.java`:
- Around line 1385-1403: The public method searchMergedDemographicsForUnmerge is
missing JavaDoc and also declares an unused local variable providerNo; add
comprehensive JavaDoc above the method describing its purpose and include `@param`
tags for loggedInInfo (LoggedInInfo), keyword (String), searchMode (String),
limit (int), offset (int), an `@return` tag specifying List<Demographic>, and an
`@throws` tag for any security or DAO exceptions thrown by checkPrivilege or
demographicDao calls; then remove the unused providerNo variable declaration to
eliminate the dead local variable (ensure you keep the call to checkPrivilege
and the demographicDao.* calls intact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05c6d6bc-62dd-4619-9876-0b4377e227e5
📒 Files selected for processing (14)
src/main/java/ca/openosp/openo/commn/dao/DemographicDao.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicMergeDao.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicMergeDaoImpl.javasrc/main/java/ca/openosp/openo/commn/dao/DemographicMergeOperationDaoImpl.javasrc/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManager.javasrc/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManagerImpl.javasrc/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeAction.javasrc/main/java/ca/openosp/openo/lab/ca/all/upload/MessageUploader.javasrc/main/java/ca/openosp/openo/lab/ca/on/CML/ABCDParser.javasrc/main/java/ca/openosp/openo/managers/DemographicManagerImpl.javasrc/main/java/ca/openosp/openo/webserv/rest/DemographicMergeService.javasrc/main/webapp/demographic/demographicsearchresults.jspsrc/test-modern/java/ca/openosp/openo/managers/DemographicManagerUnitTest.java
💤 Files with no reviewable changes (5)
- src/main/java/ca/openosp/openo/commn/dao/DemographicDao.java
- src/main/java/ca/openosp/openo/webserv/rest/DemographicMergeService.java
- src/main/java/ca/openosp/openo/demographic/pageUtil/DemographicMergeAction.java
- src/main/java/ca/openosp/openo/demographic/merge/DemographicMergeManager.java
- src/main/java/ca/openosp/openo/commn/dao/DemographicDaoImpl.java
There was a problem hiding this comment.
5 issues found across 13 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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
2 participants
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
DemographicMergeEventaudit trail tracking which records were mergedSummary by cubic
Implements a full patient demographic merge/unmerge with an event-based audit trail and a high‑throughput copy engine. Ships a single‑page admin UI and REST endpoints that return the merged demographic ID; large merges complete in seconds and are fully reversible.
New Features
admin/DemographicMerge.do: search, pick a primary, merge/unmerge; adds a spinner and a success modal linking to the merged patient; unmerge view shows a collapsible merge history; nav links and i18n keys updated.demographic_merged_eventwith domainDemographicMerge; REST (DemographicMergeService,DemographicMergedConverter,DemographicMergedTo1) to list events and run merge/unmerge; merge returns the merged demographic ID.DemographicMergeManager,DemographicMergeOperationDao/DemographicMergeOperationDaoImpl) copies and remaps notes, eForms/values, drugs/reasons, measurements (incl. ext/deleted), labs, consultations (incl. archives/docs/ext), billing (incl. child rows), ticklers (incl. links/comments), case management (CaseMgmt*), documents, email (incl. attachments), preventions (incl. ext), OLIS, consent, and more; fully remapscasemgmt_note_link.tableIdfor appointments, allergies, drugs, eForms, email, preventions, ticklers, and self‑links; unmerge restores the prior state.IN/already‑merged records; after merge mark sourcesIN; on unmerge restore primary/secondaries toACand set the merged record (C) toIN.tickler_comments) and guards for legacy nulls; very large merges complete in seconds.ACwhen duplicates exist after a merge.Migration
database/mysql/updates/update-2026-04-10-demographic-merged-extend.sqlto createdemographic_merged_event(legacydemographic_mergedremains for rollback).Written for commit 24e9141. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Database
UI
Refactor