Scope billing run deletion filters to the current container

deleteBillingRuns() filtered the invoice, invoicedItems, and miscCharges tables by objectid/invoiceId alone, so a billing admin in one container could delete or detach billing data in any other container by passing foreign objectids. All filters are now container-scoped via SimpleFilter.createContainerFilter().

Adds an EHR_BillingManager.TestCase integration test, registered through EHR_BillingModule.getIntegrationTests(), that seeds a complete billing run in each of two folders and verifies that ids from another container are ignored by both the testOnly preview and the actual delete, while same-container deletion continues to remove the run and detach its misc charges.

Author: labkey-martyp

ehr_billing/src/org/labkey/ehr_billing/EHR_BillingManager.java

@@ -16,6 +16,10 @@
 
 package org.labkey.ehr_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;
@@ -36,14 +40,20 @@
 import org.labkey.api.query.QueryUpdateServiceException;
 import org.labkey.api.security.User;
 import org.labkey.api.data.RuntimeSQLException;
+import org.labkey.api.util.GUID;
+import org.labkey.api.util.JunitUtil;
+import org.labkey.api.util.TestContext;
 
 import java.sql.SQLException;
 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;
 
 public class EHR_BillingManager
 {
@@ -68,11 +78,11 @@ public List<String> deleteBillingRuns(User user, Container container, Collection
         TableInfo miscCharges = EHR_BillingSchema.getInstance().getSchema().getTable(EHR_BillingSchema.TABLE_MISC_CHARGES);
 
         //create filters
-        SimpleFilter objectIdFilter = new SimpleFilter(FieldKey.fromString("objectid"), pks, CompareType.IN);
-        SimpleFilter invoiceIdFilter = new SimpleFilter(FieldKey.fromString("invoiceId"), pks, CompareType.IN);
-        SimpleFilter invoiceRunIdFilter = new SimpleFilter(FieldKey.fromString("invoiceRunId"), pks, CompareType.IN);
+        SimpleFilter objectIdFilter = createContainerScopedInFilter(container, "objectid", pks);
+        SimpleFilter invoiceIdFilter = createContainerScopedInFilter(container, "invoiceId", pks);
+        SimpleFilter invoiceRunIdFilter = createContainerScopedInFilter(container, "invoiceRunId", pks);
 
-        SimpleFilter miscChargesFilter = new SimpleFilter(FieldKey.fromString("invoiceId"), pks, CompareType.IN);
+        SimpleFilter miscChargesFilter = createContainerScopedInFilter(container, "invoiceId", pks);
 
         //perform the work
         List<String> ret = new ArrayList<>();
@@ -114,6 +124,11 @@ public List<String> deleteBillingRuns(User user, Container container, Collection
         return ret;
     }
 
+    private SimpleFilter createContainerScopedInFilter(Container container, String columnName, Collection<String> values)
+    {
+        return SimpleFilter.createContainerFilter(container).addInClause(FieldKey.fromString(columnName), values);
+    }
+
     private void deleteInvoiceRuns(TableInfo tableInfo, Map<String, Object>[] rows, User user, Container container) throws QueryUpdateServiceException, BatchValidationException, InvalidKeyException
     {
         if(rows.length>0)
@@ -141,4 +156,127 @@ public Container getBillingContainer(Container c)
 
     }
 
-}
+    public static class TestCase extends Assert
+    {
+        private static final String FOLDER_A = "EHRBillingDeleteTestA";
+        private static final String FOLDER_B = "EHRBillingDeleteTestB";
+
+        private User _user;
+        private Container _containerA;
+        private Container _containerB;
+        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);
+
+            _runIdA = insertBillingRun(_containerA);
+            _runIdB = insertBillingRun(_containerB);
+        }
+
+        @After
+        public void tearDown()
+        {
+            deleteTestFolders();
+        }
+
+        @Test
+        public void testDeleteBillingRunsIsContainerScoped() throws Exception
+        {
+            EHR_BillingManager manager = EHR_BillingManager.get();
+            EHR_BillingSchema schema = EHR_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(schema.getTableInvoiceRuns(), _containerB));
+            assertEquals("invoice row in container B should survive a delete issued from container A", 1, containerRowCount(schema.getInvoice(), _containerB));
+            assertEquals("invoicedItems row in container B should survive a delete issued from container A", 1, containerRowCount(schema.getTableInvoiceItems(), _containerB));
+            assertEquals("miscCharges row in container B should still reference its invoice", 1, miscChargesWithInvoiceCount(_containerB));
+
+            // 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(schema.getTableInvoiceRuns(), _containerA));
+            assertEquals("invoice row in container A should be deleted", 0, containerRowCount(schema.getInvoice(), _containerA));
+            assertEquals("invoicedItems row in container A should be deleted", 0, containerRowCount(schema.getTableInvoiceItems(), _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(schema.getMiscCharges(), _containerA));
+        }
+
+        private Container createBillingFolder(Container parent, String name)
+        {
+            Container c = ContainerManager.createContainer(parent, name, _user);
+            Set<Module> active = new HashSet<>(c.getActiveModules());
+            active.add(ModuleLoader.getInstance().getModule(EHR_BillingModule.NAME));
+            c.setActiveModules(active, _user);
+            return c;
+        }
+
+        private String insertBillingRun(Container c)
+        {
+            EHR_BillingSchema schema = EHR_BillingSchema.getInstance();
+            String runId = GUID.makeGUID();
+            String invoiceNumber = c.getName();
+
+            Map<String, Object> run = new CaseInsensitiveHashMap<>();
+            run.put("objectid", runId);
+            run.put("runDate", new Date());
+            run.put("container", c.getId());
+            Table.insert(_user, schema.getTableInvoiceRuns(), run);
+
+            Map<String, Object> invoice = new CaseInsensitiveHashMap<>();
+            invoice.put("invoiceNumber", invoiceNumber);
+            invoice.put("invoiceRunId", runId);
+            invoice.put("container", c.getId());
+            Table.insert(_user, schema.getInvoice(), invoice);
+
+            Map<String, Object> invoicedItem = new CaseInsensitiveHashMap<>();
+            invoicedItem.put("objectId", GUID.makeGUID());
+            invoicedItem.put("invoiceId", runId);
+            invoicedItem.put("invoiceNumber", invoiceNumber);
+            invoicedItem.put("container", c.getId());
+            Table.insert(_user, schema.getTableInvoiceItems(), invoicedItem);
+
+            Map<String, Object> miscCharge = new CaseInsensitiveHashMap<>();
+            miscCharge.put("objectid", GUID.makeGUID());
+            miscCharge.put("invoiceId", runId);
+            miscCharge.put("container", c.getId());
+            Table.insert(_user, schema.getMiscCharges(), miscCharge);
+
+            return runId;
+        }
+
+        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(EHR_BillingSchema.getInstance().getMiscCharges(), filter, null).getRowCount();
+        }
+
+        private void deleteTestFolders()
+        {
+            Container junit = JunitUtil.getTestContainer();
+            for (String name : List.of(FOLDER_A, FOLDER_B))
+            {
+                Container c = junit.getChild(name);
+                if (c != null)
+                    ContainerManager.delete(c, _user);
+            }
+        }
+    }
+}

ehr_billing/src/org/labkey/ehr_billing/EHR_BillingModule.java

@@ -127,6 +127,13 @@ public Set<String> getSchemaNames()
         return Collections.singleton(EHR_BillingSchema.NAME);
     }
 
+    @Override
+    @NotNull
+    public Set<Class> getIntegrationTests()
+    {
+        return Collections.singleton(EHR_BillingManager.TestCase.class);
+    }
+
     @NotNull
     @Override
     public JSONObject getPageContextJson(ContainerUser ctx)