Repository navigation
[DT-4062] Add data access history and research outputs - #3932
Merged
otchet-broad merged 26 commits intoSep 23, 2026
Merged
otchet-broad merged 26 commits into
otchet-broad merged 26 commits into
Conversation
…rch outputs Adds two sections. Data Access History lists the granted requests across every dataset in the study - PI, institution, submission date, project title, and an expandable research use statement - with a current/expired chip. Secondary Research Outputs lists the presentations, publications and intellectual property that researchers reported back through progress reports on the study's datasets, grouped by type. The DAR summary is fetched once for the whole study rather than once per dataset, and DatasetStatisticsDar gains piName, institutionName and submissionDate, which the study view shows and the existing per-dataset view simply ignores. Both sections stay on the page when they are empty, saying so, rather than disappearing: on a study page an absent section reads as a loading failure. Fifth of the DT-4062 stack. **Pairs with consent #3053** (otchet-dt-4061-study-dar-metrics), which adds /api/metrics/dar-summaries/study/{studyId} and /api/metrics/research-outputs/study/{studyId}. Until that deploys both requests fail and each section shows its error state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y-dar-history # Conflicts: # test/components/study_details/StudyDetails.spec.tsx
consent #3053 gates GET /api/metrics/dar-summaries/{datasetId} on being able to read the dataset, because the summaries carry the project titles and research use statements of approved DARs and the route previously checked only that the dataset existed. A dataset whose study is unpublished now answers 403 to everyone but the study's creator, its custodians, and admins. The dataset itself still loads, so a refusal costs only this section. Caught where the request is made rather than by the page's outer handler, it leaves the rest of the page intact and says which of the two things happened - the history is withheld, not absent - where the generic "unable to retrieve dataset statistics from server" read as a fault. extractStatus reads the status fetchAdapter attaches to a failed request, so an authorization refusal can be told from a server fault; both read identically through extractError. A 500 still surfaces as an error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Sonar: "Use <output> instead of the 'status' role to ensure accessibility across all devices." <output> carries an implicit status role and is announced more reliably than a div wearing role="status". It is inline by default, so the notice takes display:block to keep the spacing its neighbours have. The test now finds the notice by its role rather than by text, so a revert to a plain div fails here instead of silently ceasing to announce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
otchet-broad
marked this pull request as ready for review
September 15, 2026 02:06
otchet-broad
requested review from
fboulnois and
kevinmarete
and removed request for
a team
September 15, 2026 02:06
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical dataset-switching state and moderate error-handling and coverage findings remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds study-wide Data Access History and Secondary Research Outputs, with updated DAR metadata, API hooks, and authorization-aware dataset handling.
Changes:
- Adds study metrics APIs and React Query hooks.
- Adds DAR history and grouped research-output sections.
- Adds restricted-history handling and related tests.
File summaries
| File | Review summary |
|---|---|
test/utils/ErrorUtils.spec.ts |
Covers status extraction; no final comments. |
test/pages/DatasetStatistics.spec.tsx |
Covers restricted and error states; no final comments. |
test/components/study_details/StudyDetails.spec.tsx |
Exercises the new study sections; no final comments. |
src/utils/ErrorUtils.ts |
Moderate (1 vote): Fall back to cause.response when extracting status so fetchAdapter-wrapped 403 responses are recognized. |
src/types/model.ts |
Adds DAR metadata fields; no final comments. |
src/pages/DatasetStatistics.tsx |
Critical (3 votes): Reset DAR state on dataset changes to prevent stale history or incorrectly hidden history. Nit (1 vote): Use the shared MUI Alert for the informational restriction notice. |
src/libs/ajax/DatasetMetrics.ts |
Moderate (1 vote): Add focused tests for endpoint paths, auth options, and response shapes. Moderate (2 votes): Add direct success/failure tests for the study DAR endpoint. |
src/hooks/useStudyDetailsData.ts |
Adds study metrics queries; no final comments. |
src/components/study_details/StudySecondaryResearchOutputs.tsx |
Moderate (1 vote): Assert the empty message for each zero-count output group. |
src/components/study_details/StudyDetails.tsx |
Moderate (1 vote): Add coverage for empty DAR/output arrays and rejected study queries. |
src/components/study_details/StudyDarHistory.tsx |
Moderate (1 vote): Add empty-history and applicable error-state assertions. |
Review details
Suppressed comments (7)
src/components/study_details/StudyDarHistory.tsx:57
- The new StudyDetails tests exercise only a populated DAR history. The default mock returns an empty array, but no assertion covers this
isEmptybranch, so a regression could remove the section or lose its required empty message without failing tests. Add an empty-history assertion (and an error-state assertion if the section's failure behavior is part of this change).
error={error}
isEmpty={data.length === 0}
emptyMessage="No granted data access requests yet."
errorMessage="Unable to load data access requests."
src/components/study_details/StudyDetails.tsx:198
- The added StudyDetails tests cover only successful non-empty responses. They do not assert the required empty-state messages or the failure path that must keep each section visible while showing an error, so regressions in
StudyQueryResultusage could pass unnoticed. Add cases for empty DAR/output arrays and rejected study queries.
<StudyDarHistory studyId={studyId} />
<StudySecondaryResearchOutputs studyId={studyId} />
src/components/study_details/StudySecondaryResearchOutputs.tsx:21
- The new component test covers only non-empty output groups. The empty path here is required to keep each type visible and explain that no output was reported, but the default empty mock is not asserted, so this behavior can regress unnoticed. Add an assertion that the empty group message is rendered for each zero-count type.
{items.length === 0
? <Typography color="text.secondary">No {title.toLowerCase()} reported yet.</Typography>
src/libs/ajax/DatasetMetrics.ts:30
- The two new study-level AJAX methods are not covered by
test/libs/ajax/DatasetMetrics.spec.ts, which only exercisesgetDatasetStats. Add focused tests for both endpoint paths, auth options, and returned response shapes so a route or payload regression is caught independently of mocked component tests.
getStudyStats: async (studyId: number | string): Promise<DatasetStatisticsDar[]> => {
const url = `${await Config.getApiUrl()}/api/metrics/dar-summaries/study/${studyId}`
const res = await fetchGet<DatasetStatisticsDar[]>(url, Config.authOpts())
src/libs/ajax/DatasetMetrics.ts:42
- This new API wrapper is not covered by the existing
test/libs/ajax/DatasetMetrics.spec.ts, and the StudyDetails tests mockDatasetMetrics, so an incorrect research-output URL, auth-options call, or response propagation would pass. Add direct success/failure tests for this endpoint.
getResearchOutputs: async (studyId: number | string): Promise<StudyResearchOutputs> => {
const url = `${await Config.getApiUrl()}/api/metrics/research-outputs/study/${studyId}`
const res = await fetchGet<StudyResearchOutputs>(url, Config.authOpts())
return res.data
src/pages/DatasetStatistics.tsx:297
- This is a newly added user-facing status/callout, so it should use the shared MUI
Alert(withseverity="info", retainingrole="status"if required) rather than a raw<output>. Alert supplies the standard visual treatment and accessible semantics used for notices in this codebase.
<output style={{ display: 'block', paddingTop: '20px', fontStyle: 'italic' }}>
The study this dataset belongs to has not been published, so its data access request
history is available only to the study's creator, its custodians, and admins.
</output>
src/utils/ErrorUtils.ts:32
fetchRequestcatches thehandleResponseerror and rethrows a newErrorwith the original response only incause(src/libs/ajax/fetchAdapter.ts:305-319). Consequently, a real 403 fromgetDatasetStatsreaches this helper without a top-levelresponse, soextractStatus(error)returnsundefinedand the intended restricted-history message is never shown; it is reported as a generic server error instead. Read the status fromcause.responseas a fallback (and add a regression case using the fetchAdapter-shaped error).
if (typeof error === 'object' && error !== null && 'response' in error) {
const response = (error as { response?: { status?: number } }).response
return typeof response?.status === 'number' ? response.status : undefined
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Clears the per-dataset answers before each request. This page is not remounted between datasets - the route parameter changes and the effect re-runs against the same state - so a 403 on an unpublished study left the previous dataset's request history on screen beside the restriction notice, and returning to a readable dataset kept the notice. A test navigates between the two through the router, since MemoryRouter reads initialEntries only on mount. Adds direct coverage for the two study-scoped metrics calls. Both were mocked wholesale by the feature tests, so an incorrect url, missing auth options, or swallowed response could not be caught. The DAR summary case also asserts that a rejection keeps its status, which is what lets the page tell a refusal from a fault. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kevinmarete
reviewed
Sep 15, 2026
kevinmarete
left a comment
kevinmarete
left a comment
Contributor
There was a problem hiding this comment.
fetchRequest wraps non-OK errors in a new Error whose cause carries response.status, so extractStatus(error) returns undefined for a real 403. Please inspect cause.response as a fallback or preserve the response on the wrapper, and add a regression test using the actual fetch-adapter-shaped error.
Review reported that fetchRequest rewraps the error so extractStatus returns undefined for a real 403. Driving the actual adapter shows it returns 403 today: handleResponse attaches .response and throws, and fetchRequest reaches it through `return handleResponse(...)` with no await, so the rejection never enters the surrounding catch that would have rewrapped it. The report is worth a test anyway, for the reason it is not currently true. Adding `await` there - an ordinary tidy-up - routes the error through that catch, drops .response to .cause, and the study page silently stops telling a refusal from a fault. The test fails on exactly that change; it was written against it rather than asserted from reading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fboulnois
approved these changes
Sep 15, 2026
fboulnois
left a comment
fboulnois
left a comment
Contributor
There was a problem hiding this comment.
A few suggestions identified with Claude. Some of these may be intended behavior:
-
Dataset reset misses
datasetTerm:src/pages/DatasetStatistics.tsx:107
The new reset clearsdars/darsRestrictedbut notdatasetTerm. On in-app navigation to a second dataset whose index lookup throws or returns ≠1 hit, both paths setisLoading=falseand return, so!isLoading && datasetTermstill renders the previous dataset's page under the new URL, and Apply for Access drafts a DAR against the olddatasetTerm.datasetId. -
No cancellation latch in
init():src/pages/DatasetStatistics.tsx:106
Two quick navigations that resolve out of order let the older request'ssetDatasetTerm/setDars/setDarsRestrictedland last, reintroducing exactly the cross-dataset bleed this hunk is meant to fix. The added resets run before the stale writes, so they don't help. -
Every 403 is reported as "not published":
src/pages/DatasetStatistics.tsx:139
A 403 for any other reason (ToS, disabled account, proxy) makes the page assert something false about the study. -
Study page and dataset page disagree on 403:
src/components/study_details/StudyDarHistory.tsx:57
The study-page section has no 403 branch, so the same refusal reads as "Unable to load data access requests." The two surfaces now present an authorization refusal differently, despiteextractStatusbeing added for precisely that distinction. -
Privacy: requester identity shown unconditionally:
src/components/study_details/StudyDarHistory.tsx:21
Requester PI name and institution (newDatasetStatisticsDarfields) are shown alongside the research use statement to any authenticated viewer of a study page, which is broader than the dataset page's project-title-only view.
Four findings from review on the data access history. The reset cleared the history and the restriction flag but not the dataset itself. If the next lookup throws or matches no single dataset, both paths stop without setting one, so the previous dataset's page was still on screen under the new URL - and Apply for Access would have drafted a request against that stale dataset. Nothing stopped an older in-flight request from writing last, either. Two quick navigations that resolve out of order reintroduced exactly the cross-dataset bleed the resets exist to prevent, since clearing state first does nothing about a write that arrives afterwards. The effect now latches and ignores answers to a question the page has stopped asking. The restriction notice asserted the study was unpublished, but a 403 can also come from a disabled account, terms not accepted, or a proxy. It now describes the refusal and offers the unpublished case as the common reason rather than stating it as fact. The study page had no 403 branch at all, so the same refusal read there as "unable to load data access requests" - two descriptions of one authorization decision, which is the distinction extractStatus was added to make. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sonar flags the effect's init at cognitive complexity 17 against a limit of 15 (typescript:S3776). The cancellation guards added to stop an older navigation writing last are what pushed it over. The request-history fetch moves to its own function. It was always a separate concern - a refusal there withholds one section rather than failing the page - and taking its try/catch and 403 branch out of the effect removes two levels of nesting from the part that is really just "look up the dataset, then load its history". No behaviour change; the existing dataset-switching and 403 tests cover both paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The section carried the requester's name beside their affiliation. An institution says where a grant landed; the name identifies the person who holds it, which the study page does not need to publish to every reader. The backend stops sending it either way, so this would otherwise have rendered 'Not provided' for every row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Auditing the class rather than the reported instance turned up two more components doing what review had already flagged twice elsewhere. StudySecondaryResearchOutputs passed the error through unconditionally, so a failed background refetch replaced outputs that had loaded fine. StudyDarHistory looked guarded but was not: its gate asked whether the failure was a 403, not whether anything was cached, so an ordinary 500 on refetch still cleared the rows. A refusal keeps its precedence - once the server says no, the history comes off the page rather than lingering - and every other failure now defers to what is already loaded. Three tests, one per path: a transient failure keeps the rows, a 403 still removes them, and the outputs section keeps its groups. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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
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.
Addresses
https://broadworkbench.atlassian.net/browse/DT-4062
Summary
Two sections. Data Access History lists the granted requests across every dataset in the study —
PI, institution, submission date, project title, and an expandable research use statement — with a
current/expired chip. Secondary Research Outputs lists the presentations, publications and
intellectual property researchers reported back through progress reports, grouped by type.
The DAR summary is fetched once for the whole study rather than once per dataset.
DatasetStatisticsDargainspiName,institutionNameandsubmissionDate, which the studyview shows and the existing per-dataset view ignores.
Both sections stay on the page when empty, saying so, rather than disappearing: on a study page an
absent section reads as a loading failure.
Also updates
DatasetStatistics.tsx, which is outside the study page but shares the backendchange. consent #3053 gates
GET /api/metrics/dar-summaries/{datasetId}on being able to read thedataset, because those summaries carry the project titles and research use statements of approved
DARs and the route previously checked only that the dataset existed. A dataset whose study is
unpublished now answers 403 to everyone but the study's creator, its custodians, and admins. The
dataset itself still loads, so the refusal is caught where the request is made rather than by the
page's outer handler: the rest of the page stays intact and the section says the history is
withheld rather than absent, where the generic "unable to retrieve" read as a server fault. A 500
still surfaces as an error.
extractStatusreads the statusfetchAdapterattaches to a failed request, so an authorizationrefusal can be told from a server fault — both read identically through
extractError.Have you read Terra's Contributing Guide lately? If not, do that first.