Skip to content

GH Issue 1619: Fix cross-container handling in assay run QC history and QC state update page #8217

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

Merged
cnathe merged 3 commits into from
Oct 23, 2026
Merged

GH Issue 1619: Fix cross-container handling in assay run QC history and QC state update page #8217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52e56b8
Show cross-container QC state changes in run QC history
cnathe Oct 22, 2026
a40821a
Use validated runs on the QC state update page
cnathe Oct 22, 2026
47601a1
Claude CR feedback: hide container filter menu option from QC history…
cnathe Oct 22, 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
11 changes: 11 additions & 0 deletions api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,17 @@ protected Container createContainer(String name, org.labkey.api.module.Module...
return c;
}

/**
* Create a throwaway subfolder of {@code parent} (for example, one from {@link #createContainer(String, Module...)}),
* registered for cleanup ahead of its parent.
*/
protected Container createChildContainer(Container parent, String name, Module... enabledModules)
{
Container c = activateModules(ContainerManager.createContainer(parent, name, getAdmin()), enabledModules);
_containers.addFirst(c);
return c;
}

/**
* Like {@link #createContainer(String, Module...)}, but created once and reused by every test in the class, then
* deleted after the last one. Container deletes are slow, so prefer this whenever a test only asserts on data it
Expand Down
24 changes: 22 additions & 2 deletions assay/src/org/labkey/assay/AssayController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Stream;
import java.util.zip.ZipEntry;
import java.util.zip.ZipOutputStream;

Expand Down Expand Up @@ -1384,6 +1385,9 @@ public Object execute(Object form, BindException errors) throws Exception
}
}

// Runs are the ones QCStateAction already resolved and permission-checked in their own containers
public record QCStateBean(UpdateQCStateForm form, List<ExpRun> runs) {}

public static class UpdateQCStateForm extends ReturnUrlForm
{
private Long _state;
Expand Down Expand Up @@ -1455,7 +1459,7 @@ public ModelAndView getView(UpdateQCStateForm form, BindException errors) throws

if (getContainer().hasPermission(getUser(), QCAnalystPermission.class))
{
JspView<UpdateQCStateForm> jspView = new JspView<>("/org/labkey/assay/view/updateQCState.jsp", form, errors);
JspView<QCStateBean> jspView = new JspView<>("/org/labkey/assay/view/updateQCState.jsp", new QCStateBean(form, runs), errors);
jspView.setFrame(WebPartView.FrameType.PORTAL);
view.addView(jspView);
}
Expand All @@ -1476,6 +1480,8 @@ public ModelAndView getView(UpdateQCStateForm form, BindException errors) throws
settings.setQueryName("ExperimentAuditEvent");

QueryView auditView = schema.createView(getViewContext(), settings, errors);
auditView.setContainerFilter(getRunQCHistoryContainerFilter(run, getUser()));
auditView.disableContainerFilterSelection();
auditView.setTitle("QC History");

view.addView(auditView);
Expand Down Expand Up @@ -1510,7 +1516,7 @@ public ApiResponse execute(RunQCHistoryForm form, BindException errors)
// Resolve the run and confirm the (non-elevated) user can read its container BEFORE elevating, so this
// action can't be used to read audit events from a container the user otherwise can't access.
ExpRun run = ExperimentService.get().getExpRun(form.getRunId());
if (run == null || !run.getContainer().hasPermission(getUser(), ReadPermission.class))
if (run == null || !run.getContainer().hasPermission(getUser(), AssayReadPermission.class))
throw new NotFoundException("Run " + form.getRunId() + " not found.");

Container runContainer = run.getContainer();
Expand All @@ -1527,10 +1533,24 @@ public ApiResponse execute(RunQCHistoryForm form, BindException errors)
settings.setBaseSort(new Sort("-Created"));

QueryView view = schema.createView(getViewContext(), settings, errors);
view.setContainerFilter(getRunQCHistoryContainerFilter(run, getUser()));

@labkey-klum labkey-klum Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice solution.

return new ExtendedApiQueryResponse(view, false, false, "auditLog", "ExperimentAuditEvent", 0, null, false, false, false);
}
}

// GH Issue 1619: older QC events may be stamped with an ancestor or descendant of the run's container (the folder the
// change was made from). Use an explicit container filter instead based on the original user's permissions.
private static ContainerFilter getRunQCHistoryContainerFilter(ExpRun run, User user)
{
Container runContainer = run.getContainer();
List<Container> containers = Stream.concat(
Stream.iterate(runContainer.getParent(), c -> c != null && !c.isRoot(), Container::getParent)
.filter(c -> c.hasPermission(user, AssayReadPermission.class)),
ContainerManager.getAllChildren(runContainer, user, AssayReadPermission.class).stream()
).toList();
return new ContainerFilter.SimpleContainerFilter(containers);
}

public static class RunQCHistoryForm
{
private Long _runId;
Expand Down
30 changes: 12 additions & 18 deletions assay/src/org/labkey/assay/view/updateQCState.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,42 +16,36 @@
*/
%>
<%@ page import="org.labkey.api.assay.AssayQCService" %>
<%@ page import="org.labkey.api.data.Container" %>
<%@ page import="org.labkey.api.exp.api.ExpRun" %>
<%@ page import="org.labkey.api.exp.api.ExperimentService" %>
<%@ page import="org.labkey.api.qc.DataState" %>
<%@ page import="org.labkey.api.util.InputBuilder.Input" %>
<%@ page import="org.labkey.api.util.SelectBuilder" %>
<%@ page import="org.labkey.api.util.TextAreaBuilder" %>
<%@ page import="org.labkey.api.view.HttpView" %>
<%@ page import="org.labkey.api.view.JspView" %>
<%@ page import="org.labkey.assay.AssayController.QCStateBean" %>
<%@ page import="org.labkey.assay.AssayController.UpdateQCStateForm" %>
<%@ page extends="org.labkey.api.jsp.FormPage" %>
<%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %>
<%
JspView<UpdateQCStateForm> view = HttpView.currentView();
UpdateQCStateForm form = view.getModelBean();
JspView<QCStateBean> view = HttpView.currentView();
QCStateBean bean = view.getModelBean();
UpdateQCStateForm form = bean.form();
ExpRun firstRun = bean.runs().getFirst();
String currentState = null;
String protocolContainerPath = null;
boolean requireComment = false;

if (form.getRuns().size() == 1)
if (bean.runs().size() == 1)
{
// for single row selections, display the current QC state
ExpRun run = ExperimentService.get().getExpRun(getContainer(), form.getRuns().stream().findFirst().get());
if (run != null)
{
DataState state = AssayQCService.getProvider().getQCState(run.getProtocol(), run.getRowId());
currentState = state != null ? state.getLabel() : null;
}
DataState state = AssayQCService.getProvider().getQCState(firstRun.getProtocol(), firstRun.getRowId());
currentState = state != null ? state.getLabel() : null;
}

// get the protocol container to determine the folder where QC states are defined
ExpRun expRun = ExperimentService.get().getExpRun(getContainer(), form.getRuns().stream().findFirst().get());
if (expRun != null)
{
protocolContainerPath = expRun.getProtocol().getContainer().getPath();
requireComment = AssayQCService.getProvider().isRequireCommentOnQCStateChange(expRun.getProtocol().getContainer());
}
Container protocolContainer = firstRun.getProtocol().getContainer();
String protocolContainerPath = protocolContainer.getPath();
boolean requireComment = AssayQCService.getProvider().isRequireCommentOnQCStateChange(protocolContainer);
%>

<script type="text/javascript" nonce="<%=getScriptNonce()%>">
Expand Down
11 changes: 6 additions & 5 deletions experiment/src/org/labkey/experiment/api/ExpQCFlagTableImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,27 +221,27 @@ public UpdateService(TableInfo queryTable)
protected Map<String, Object> _insert(User user, Container c, Map<String, Object> row) throws SQLException, ValidationException
{
Map<String, Object> newRow = super._insert(user, c, row);
addAuditEvent(c, user, INSERT, row);
addAuditEvent(user, INSERT, row);
return newRow;
}

@Override
protected Map<String, Object> _update(User user, Container c, Map<String, Object> row, Map<String, Object> oldRow, Object[] keys) throws SQLException, ValidationException
{
Map<String, Object> newRow = super._update(user, c, row, oldRow, keys);
addAuditEvent(c, user, UPDATE, row);
addAuditEvent(user, UPDATE, row);
return newRow;
}

@Override
protected Map<String, Object> deleteRow(User user, Container container, Map<String, Object> oldRowMap) throws QueryUpdateServiceException, SQLException, InvalidKeyException
{
Map<String, Object> newRow = super.deleteRow(user, container, oldRowMap);
addAuditEvent(container, user, DELETE, oldRowMap);
addAuditEvent(user, DELETE, oldRowMap);
return newRow;
}

private void addAuditEvent(Container container, @Nullable User user, int action, Map<String, Object> row)
private void addAuditEvent(@Nullable User user, int action, Map<String, Object> row)
{
// for now we are only auditing QCState related flag events
if (row.containsKey("IntKey1") && row.get("IntKey1") != null)
Expand All @@ -259,7 +259,8 @@ private void addAuditEvent(Container container, @Nullable User user, int action,
DataState state = qcId != null ? DataStateManager.getInstance().getStateForRowId(run.getProtocol().getContainer(), qcId) : null;
if (state != null)
{
ExperimentAuditEvent event = new ExperimentAuditEvent(container, comment);
// GH Issue 1619: use the run's container, not the request's container
ExperimentAuditEvent event = new ExperimentAuditEvent(run.getContainer(), comment);

event.setProtocolLsid(run.getProtocol().getLSID());
event.setRunLsid(run.getLSID());
Expand Down
Loading