Skip to content

Scope billing run deletion filters to the current container #1146

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
labkey-martyp wants to merge 2 commits into
base: release25.7-SNAPSHOT
Choose a base branch
from
Open

Scope billing run deletion filters to the current container #1146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5bc210
Scope billing run deletion filters to the current container
labkey-martyp Jun 12, 2026
b0ef34c
Scope misc charges delete by authorized run id only
labkey-martyp Jun 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
224 changes: 219 additions & 5 deletions ehr_billing/src/org/labkey/ehr_billing/EHR_BillingManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
{
Expand All @@ -68,11 +78,24 @@ 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);
Set<String> invoiceRunIds = getInvoiceRunIds(invoiceRuns, objectIdFilter);
if (invoiceRunIds.isEmpty())
{
List<String> ret = new ArrayList<>();
if (testOnly)
{
ret.add("0 records from invoiced items");
ret.add("0 records from invoice");
ret.add("0 invoice 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 invoiceIdFilter = createContainerScopedInFilter(container, "invoiceId", invoiceRunIds);
SimpleFilter invoiceRunIdFilter = createContainerScopedInFilter(container, "invoiceRunId", invoiceRunIds);

SimpleFilter miscChargesFilter = createMiscChargesFilter(invoiceRunIds);

//perform the work
List<String> ret = new ArrayList<>();
Expand Down Expand Up @@ -114,6 +137,33 @@ 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 Set<String> 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<String> invoiceRunIds)
{
// Intentionally NOT container-scoped. Source miscCharges records can live in any number of 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;
}

private void deleteInvoiceRuns(TableInfo tableInfo, Map<String, Object>[] rows, User user, Container container) throws QueryUpdateServiceException, BatchValidationException, InvalidKeyException
{
if(rows.length>0)
Expand Down Expand Up @@ -141,4 +191,168 @@ 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 static final String FOLDER_EHR = "EHRBillingDeleteTestEHR";
private static final String FOLDER_SATELLITE = "EHRBillingDeleteTestSatellite";

private User _user;
private Container _containerA;
private Container _containerB;
private Container _containerEHR;
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);
_containerEHR = createBillingFolder(junit, FOLDER_EHR);
_containerSatellite = createBillingFolder(junit, FOLDER_SATELLITE);
setEHRContainer(_containerA, _containerEHR);

_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));

// WNPRC-style source data: billing artifacts live in the finance container, but miscCharges live in the EHR container
String runIdWithEHRMiscCharge = insertBillingRun(_containerA, _containerEHR);
List<String> preview = manager.deleteBillingRuns(_user, _containerA, List.of(runIdWithEHRMiscCharge), true);
assertTrue("Preview should count miscCharges rows in the EHR source container: " + preview,
preview.stream().anyMatch(summary -> summary.startsWith("1 invoice records from misc charges")));

manager.deleteBillingRuns(_user, _containerA, List.of(runIdWithEHRMiscCharge), false);
assertEquals("miscCharges row in the EHR source container should be detached from the deleted invoice", 0, miscChargesWithInvoiceCount(_containerEHR));
assertEquals("miscCharges row in the EHR source container should not be deleted", 1, containerRowCount(schema.getMiscCharges(), _containerEHR));

// Satellite source data: miscCharges can live in a container that is neither the finance container nor
// the configured EHR study container. The delete is keyed off the authorized run id, so these rows are
// still previewed and detached.
String runIdWithSatelliteMiscCharge = insertBillingRun(_containerA, _containerSatellite);
List<String> 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 invoice 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(schema.getMiscCharges(), _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(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)
{
return insertBillingRun(c, c);
}

private String insertBillingRun(Container billingContainer, Container miscChargesContainer)
{
EHR_BillingSchema schema = EHR_BillingSchema.getInstance();
String runId = GUID.makeGUID();
String invoiceNumber = billingContainer.getName() + "-" + runId;

Map<String, Object> run = new CaseInsensitiveHashMap<>();
run.put("objectid", runId);
run.put("runDate", new Date());
run.put("container", billingContainer.getId());
Table.insert(_user, schema.getTableInvoiceRuns(), run);

Map<String, Object> invoice = new CaseInsensitiveHashMap<>();
invoice.put("invoiceNumber", invoiceNumber);
invoice.put("invoiceRunId", runId);
invoice.put("container", billingContainer.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", billingContainer.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", miscChargesContainer.getId());
Table.insert(_user, schema.getMiscCharges(), miscCharge);

return runId;
}

private void setEHRContainer(Container source, Container ehrContainer)
{
Module ehr = ModuleLoader.getInstance().getModule("EHR");
ModuleProperty mp = ehr.getModuleProperties().get("EHRStudyContainer");
mp.saveValue(_user, source, ehrContainer.getPath());
}

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, FOLDER_EHR, FOLDER_SATELLITE))
{
Container c = junit.getChild(name);
if (c != null)
ContainerManager.delete(c, _user);
}
}
}
}
7 changes: 7 additions & 0 deletions ehr_billing/src/org/labkey/ehr_billing/EHR_BillingModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down