diff --git a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingController.java b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingController.java index 8d6db17d8..efebbc441 100644 --- a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingController.java +++ b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingController.java @@ -162,7 +162,7 @@ public ModelAndView getConfirmView(QueryForm form, BindException errors) throws Set ids = DataRegionSelection.getSelected(form.getViewContext(), true); StringBuilder msg = new StringBuilder("You have selected " + ids.size() + " billing runs to delete. This will also delete:

"); - for (String m : ONPRC_BillingManager.get().deleteBillingRuns(getUser(), ids, true)) + for (String m : ONPRC_BillingManager.get().deleteBillingRuns(getUser(), getContainer(), ids, true)) { msg.append(m).append("
"); } @@ -176,7 +176,7 @@ public ModelAndView getConfirmView(QueryForm form, BindException errors) throws public boolean handlePost(QueryForm form, BindException errors) throws Exception { Set ids = DataRegionSelection.getSelected(form.getViewContext(), true); - ONPRC_BillingManager.get().deleteBillingRuns(getUser(), ids, false); + ONPRC_BillingManager.get().deleteBillingRuns(getUser(), getContainer(), ids, false); return true; } diff --git a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingManager.java b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingManager.java index c2898de06..068468c26 100644 --- a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingManager.java +++ b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingManager.java @@ -15,6 +15,10 @@ */ package org.labkey.onprc_billing; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.data.CompareType; import org.labkey.api.data.Container; @@ -31,12 +35,19 @@ import org.labkey.api.query.FieldKey; import org.labkey.api.query.Queryable; import org.labkey.api.security.User; +import org.labkey.api.util.GUID; +import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.TestContext; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.Date; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; /** * User: bimber @@ -71,16 +82,28 @@ public static ONPRC_BillingManager get() return _instance; } - public List deleteBillingRuns(User user, Collection pks, boolean testOnly) + public List deleteBillingRuns(User user, Container container, Collection pks, boolean testOnly) { TableInfo invoiceRuns = ONPRC_BillingSchema.getInstance().getSchema().getTable(ONPRC_BillingSchema.TABLE_INVOICE_RUNS); TableInfo invoicedItems = ONPRC_BillingSchema.getInstance().getSchema().getTable(ONPRC_BillingSchema.TABLE_INVOICED_ITEMS); TableInfo miscCharges = ONPRC_BillingSchema.getInstance().getSchema().getTable(ONPRC_BillingSchema.TABLE_MISC_CHARGES); //create filters - SimpleFilter invoiceRunFilter = new SimpleFilter(FieldKey.fromString("invoiceId"), pks, CompareType.IN); + SimpleFilter invoiceRunsFilter = createContainerScopedInFilter(container, "objectid", pks); + Set invoiceRunIds = getInvoiceRunIds(invoiceRuns, invoiceRunsFilter); + if (invoiceRunIds.isEmpty()) + { + List ret = new ArrayList<>(); + if (testOnly) + { + ret.add("0 records from invoiced items"); + ret.add("0 records from misc charges will be removed from the deleted invoice, which means they will be picked up by the next billing period. They are not deleted."); + } + return ret; + } - SimpleFilter miscChargesFilter = new SimpleFilter(FieldKey.fromString("invoiceId"), pks, CompareType.IN); + SimpleFilter invoiceRunFilter = createContainerScopedInFilter(container, "invoiceId", invoiceRunIds); + SimpleFilter miscChargesFilter = createMiscChargesFilter(invoiceRunIds); //perform the work List ret = new ArrayList<>(); @@ -96,7 +119,7 @@ public List deleteBillingRuns(User user, Collection pks, boolean { try (DbScope.Transaction transaction = ExperimentService.get().ensureTransaction()) { - long deleted1 = Table.delete(invoicedItems, invoiceRunFilter); + Table.delete(invoicedItems, invoiceRunFilter); TableSelector tsMiscCharges2 = new TableSelector(miscCharges, Collections.singleton("objectid"), miscChargesFilter, null); String[] miscChargesIds = tsMiscCharges2.getArray(String.class); @@ -107,7 +130,7 @@ public List deleteBillingRuns(User user, Collection pks, boolean Table.update(user, miscCharges, map, objectid); } - long deleted3 = Table.delete(invoiceRuns, new SimpleFilter(FieldKey.fromString("objectid"), pks, CompareType.IN)); + Table.delete(invoiceRuns, invoiceRunsFilter); transaction.commit(); } @@ -116,6 +139,33 @@ public List deleteBillingRuns(User user, Collection pks, boolean return ret; } + private SimpleFilter createContainerScopedInFilter(Container container, String columnName, Collection values) + { + return SimpleFilter.createContainerFilter(container).addInClause(FieldKey.fromString(columnName), values); + } + + private Set getInvoiceRunIds(TableInfo invoiceRuns, SimpleFilter objectIdFilter) + { + TableSelector tsInvoiceRuns = new TableSelector(invoiceRuns, Collections.singleton("objectid"), objectIdFilter, null); + String[] invoiceRunIds = tsInvoiceRuns.getArray(String.class); + return new HashSet<>(Arrays.asList(invoiceRunIds)); + } + + private SimpleFilter createMiscChargesFilter(Collection invoiceRunIds) + { + // Intentionally NOT container-scoped. Source miscCharges records can live in different containers + // (the billing/finance container, the configured EHR study container, or other satellite containers that + // feed charges into a billing run), so there is no reliable, complete set of "source" containers to filter on. + // + // This is not a cross-container security issue: invoiceRunIds has already been narrowed by getInvoiceRunIds() + // to the run ids that actually exist in the requesting container (see the container-scoped objectIdFilter). + // A forged or out-of-container run id never reaches this filter, so matching miscCharges solely by + // invoiceId only ever touches charges belonging to runs the caller is already authorized to delete. + SimpleFilter filter = new SimpleFilter(); + filter.addInClause(FieldKey.fromString("invoiceId"), invoiceRunIds); + return filter; + } + public Container getBillingContainer(Container c) { Module billing = ModuleLoader.getInstance().getModule(ONPRC_BillingModule.NAME); @@ -139,4 +189,145 @@ public Container getSLADataFolder(Container c) return ContainerManager.getForPath(path); } + + public static class TestCase extends Assert + { + private static final String FOLDER_A = "ONPRCBillingDeleteTestA"; + private static final String FOLDER_B = "ONPRCBillingDeleteTestB"; + private static final String FOLDER_SATELLITE = "ONPRCBillingDeleteTestSatellite"; + + private User _user; + private Container _containerA; + private Container _containerB; + private Container _containerSatellite; + private String _runIdA; + private String _runIdB; + + @Before + public void setUp() + { + _user = TestContext.get().getUser(); + deleteTestFolders(); + + Container junit = JunitUtil.getTestContainer(); + _containerA = createBillingFolder(junit, FOLDER_A); + _containerB = createBillingFolder(junit, FOLDER_B); + _containerSatellite = createBillingFolder(junit, FOLDER_SATELLITE); + + _runIdA = insertBillingRun(_containerA); + _runIdB = insertBillingRun(_containerB); + } + + @After + public void tearDown() + { + deleteTestFolders(); + } + + @Test + public void testDeleteBillingRunsIsContainerScoped() + { + ONPRC_BillingManager manager = ONPRC_BillingManager.get(); + ONPRC_BillingSchema schema = ONPRC_BillingSchema.getInstance(); + + // A testOnly preview issued from container A targeting container B's run must not see container B's rows. + for (String summary : manager.deleteBillingRuns(_user, _containerA, List.of(_runIdB), true)) + assertTrue("Preview from another container should count 0 rows, but got: " + summary, summary.startsWith("0 ")); + + // An actual delete issued from container A targeting container B's run must leave container B untouched. + manager.deleteBillingRuns(_user, _containerA, List.of(_runIdB), false); + assertEquals("invoiceRuns row in container B should survive a delete issued from container A", 1, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_INVOICE_RUNS), _containerB)); + assertEquals("invoicedItems row in container B should survive a delete issued from container A", 1, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_INVOICED_ITEMS), _containerB)); + assertEquals("miscCharges row in container B should still reference its invoice", 1, miscChargesWithInvoiceCount(_containerB)); + + // Satellite source data: miscCharges can live in a container that is neither the requesting/finance + // container nor the run's own container. The delete is keyed off the authorized run id, so these rows are + // still previewed and detached. + String runIdWithSatelliteMiscCharge = insertBillingRun(_containerA, _containerSatellite); + List satellitePreview = manager.deleteBillingRuns(_user, _containerA, List.of(runIdWithSatelliteMiscCharge), true); + assertTrue("Preview should count miscCharges rows in an unrelated source container: " + satellitePreview, + satellitePreview.stream().anyMatch(summary -> summary.startsWith("1 records from misc charges"))); + + manager.deleteBillingRuns(_user, _containerA, List.of(runIdWithSatelliteMiscCharge), false); + assertEquals("miscCharges row in the satellite source container should be detached from the deleted invoice", 0, miscChargesWithInvoiceCount(_containerSatellite)); + assertEquals("miscCharges row in the satellite source container should not be deleted", 1, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_MISC_CHARGES), _containerSatellite)); + + // Positive control: deleting a run from its own container removes its rows. + manager.deleteBillingRuns(_user, _containerA, List.of(_runIdA), false); + assertEquals("invoiceRuns row in container A should be deleted", 0, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_INVOICE_RUNS), _containerA)); + assertEquals("invoicedItems row in container A should be deleted", 0, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_INVOICED_ITEMS), _containerA)); + assertEquals("miscCharges row in container A should be detached from the deleted invoice", 0, miscChargesWithInvoiceCount(_containerA)); + assertEquals("miscCharges row in container A should not be deleted", 1, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_MISC_CHARGES), _containerA)); + } + + private Container createBillingFolder(Container parent, String name) + { + Container c = ContainerManager.createContainer(parent, name, _user); + Set active = new HashSet<>(c.getActiveModules()); + active.add(ModuleLoader.getInstance().getModule(ONPRC_BillingModule.NAME)); + c.setActiveModules(active, _user); + return c; + } + + private String insertBillingRun(Container c) + { + return insertBillingRun(c, c); + } + + private String insertBillingRun(Container billingContainer, Container miscChargesContainer) + { + ONPRC_BillingSchema schema = ONPRC_BillingSchema.getInstance(); + String runId = GUID.makeGUID(); + + Map run = new CaseInsensitiveHashMap<>(); + run.put("objectid", runId); + run.put("runDate", new Date()); + run.put("billingPeriodStart", new Date()); + run.put("billingPeriodEnd", new Date()); + run.put("container", billingContainer.getId()); + Table.insert(_user, getTable(schema, ONPRC_BillingSchema.TABLE_INVOICE_RUNS), run); + + Map invoicedItem = new CaseInsensitiveHashMap<>(); + invoicedItem.put("objectid", GUID.makeGUID()); + invoicedItem.put("invoiceId", runId); + invoicedItem.put("container", billingContainer.getId()); + Table.insert(_user, getTable(schema, ONPRC_BillingSchema.TABLE_INVOICED_ITEMS), invoicedItem); + + Map miscCharge = new CaseInsensitiveHashMap<>(); + miscCharge.put("objectid", GUID.makeGUID()); + miscCharge.put("invoiceId", runId); + miscCharge.put("container", miscChargesContainer.getId()); + Table.insert(_user, getTable(schema, ONPRC_BillingSchema.TABLE_MISC_CHARGES), miscCharge); + + return runId; + } + + private TableInfo getTable(ONPRC_BillingSchema schema, String tableName) + { + return schema.getSchema().getTable(tableName); + } + + private long containerRowCount(TableInfo table, Container c) + { + return new TableSelector(table, SimpleFilter.createContainerFilter(c), null).getRowCount(); + } + + private long miscChargesWithInvoiceCount(Container c) + { + SimpleFilter filter = SimpleFilter.createContainerFilter(c); + filter.addCondition(FieldKey.fromParts("invoiceId"), null, CompareType.NONBLANK); + return new TableSelector(getTable(ONPRC_BillingSchema.getInstance(), ONPRC_BillingSchema.TABLE_MISC_CHARGES), filter, null).getRowCount(); + } + + private void deleteTestFolders() + { + Container junit = JunitUtil.getTestContainer(); + for (String name : List.of(FOLDER_A, FOLDER_B, FOLDER_SATELLITE)) + { + Container c = junit.getChild(name); + if (c != null) + ContainerManager.delete(c, _user); + } + } + } } \ No newline at end of file diff --git a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingModule.java b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingModule.java index 69fa84695..544fa7724 100644 --- a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingModule.java +++ b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingModule.java @@ -166,6 +166,13 @@ public Set getSchemaNames() return Collections.singleton(ONPRC_BillingSchema.NAME); } + @Override + @NotNull + public Set> getIntegrationTests() + { + return Collections.singleton(ONPRC_BillingManager.TestCase.class); + } + @Override protected void registerSchemas() { diff --git a/onprc_ehr/resources/queries/study/ParentageDamMismatch.sql b/onprc_ehr/resources/queries/study/ParentageDamMismatch.sql index 6ae43f26a..25cb20738 100644 --- a/onprc_ehr/resources/queries/study/ParentageDamMismatch.sql +++ b/onprc_ehr/resources/queries/study/ParentageDamMismatch.sql @@ -18,49 +18,64 @@ * 6. Excludes old parentage entries, enddate IS BLANK */ +/* + Modifications by Kolli, June 2026 + Refer tkt# 14891 for details + + * 1. Remove cases in which the ID has a surrogate dam entered in prime. (There are several but 41043 is one example) + * 2. Remove cases in which the observed dam is "unknown" + * 3. Remove cases in which the observed dam ID is not an ONPRC. E.g. there are several NEPRC IDs of the format ###-####. + */ + SELECT d.Id, d.Id.curLocation.area AS Area, - coalesce(p2.parent, '') as geneticDam, - coalesce(b.dam, '') as observedDam + gd.geneticDam, + b.dam AS observedDam + FROM study.demographics d - LEFT JOIN ( +LEFT JOIN ( SELECT - p2.Id, - MAX(p2.parent) AS parent - FROM study.parentage p2 - WHERE (p2.method = 'Genetic' OR p2.method = 'Provisional Genetic') - AND p2.relationship = 'Dam' - AND p2.enddate IS NULL - GROUP BY p2.Id -) p2 ON d.Id = p2.Id + p.Id, + MAX(p.parent) AS geneticDam + FROM study.parentage p + WHERE p.relationship = 'Dam' + AND p.method IN ('Genetic', 'Provisional Genetic') + AND p.enddate IS NULL + GROUP BY p.Id +) gd + ON gd.Id = d.Id - LEFT JOIN ( - SELECT - p3.Id, - MAX(p3.parent) AS parent - FROM study.parentage p3 - WHERE p3.relationship = 'Foster Dam' - AND p3.enddate IS NULL - GROUP BY p3.Id -) p3 ON d.Id = p3.Id +LEFT JOIN study.birth b + ON b.Id = d.Id - LEFT JOIN study.birth b - ON b.Id = d.Id +WHERE d.calculated_status.code IN ('Alive', 'Dead') + AND d.qcstate = 18 -WHERE d.calculated_status.code IN ('Alive', 'Dead') AND d.qcstate = 18 - /* exclude foster-dam cases (NULL or blank only) */ - AND COALESCE(RTRIM(LTRIM(CAST(p3.parent AS VARCHAR(50)))), '') = '' + /* exclude animals with active Foster Dam */ + /* And, exclude animals with active Surrogate Dam */ + AND NOT EXISTS ( + SELECT 1 + FROM study.parentage fd + WHERE fd.Id = d.Id + AND fd.relationship IN ('Foster Dam', 'Surrogate Dam') + AND fd.enddate IS NULL + AND COALESCE(RTRIM(LTRIM(CAST(fd.parent AS VARCHAR(50)))), '') <> '' + ) - /* exclude blank observed dam */ + /* observed dam is not blank */ AND COALESCE(RTRIM(LTRIM(CAST(b.dam AS VARCHAR(50)))), '') <> '' - /* exclude blank genetic dam */ - AND COALESCE(RTRIM(LTRIM(CAST(p2.parent AS VARCHAR(50)))), '') <> '' + /* observed dam is not unknown */ + AND LOWER(COALESCE(RTRIM(LTRIM(CAST(b.dam AS VARCHAR(50)))), '')) <> 'unknown' - /* mismatch observed vs genetic */ - AND COALESCE(RTRIM(LTRIM(CAST(b.dam AS VARCHAR(50)))), '') <> - COALESCE(RTRIM(LTRIM(CAST(p2.parent AS VARCHAR(50)))), '') + /* observed dam is an ONPRC ID, not external format like ###-#### */ + AND COALESCE(RTRIM(LTRIM(CAST(b.dam AS VARCHAR(50)))), '') NOT LIKE '%-%' + /* genetic dam is not blank */ + AND COALESCE(RTRIM(LTRIM(CAST(gd.geneticDam AS VARCHAR(50)))), '') <> '' + /* observed dam does not match genetic dam */ + AND COALESCE(RTRIM(LTRIM(CAST(b.dam AS VARCHAR(50)))), '') <> + COALESCE(RTRIM(LTRIM(CAST(gd.geneticDam AS VARCHAR(50)))), '');