[DT-4061] Consent modifications to support redesigned study pages. - #3047
Closed
otchet-broad wants to merge 5 commits into
Closed
[DT-4061] Consent modifications to support redesigned study pages.#3047otchet-broad wants to merge 5 commits into
otchet-broad wants to merge 5 commits into
Conversation
|
otchet-broad
marked this pull request as ready for review
September 8, 2026 13:26
otchet-broad
requested review from
fboulnois and
rushtong
and
a lite review from Copilot
and removed request for
a team
September 8, 2026 13:26
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed correctness/validation issues (commentText accepts non-string primitives, and POST can throw a contextless NoSuchElementException) plus a test change that introduces Mockito lenient stubbing contrary to repo guidance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds backend support for redesigned study pages by introducing PI profile fields on Study, promoting registration “assets” into first-class study_property rows (with legacy fallback), and adding new study-scoped comments/ratings and metrics/recommendation endpoints with consistent study-visibility gating.
Changes:
- Adds PI institution + profile link columns, patch semantics (absent vs explicit
null), and supporting migrations/tests. - Promotes selected asset lists out of the legacy
assetsblob into dedicated properties, updates search indexing and registration mapping, and adds read endpoints + OpenAPI updates. - Adds study comment CRUD, study-scoped DAR summaries, research outputs aggregation from progress reports, and study recommendation queries/endpoints.
File summaries
| File | Description |
|---|---|
| src/test/java/org/broadinstitute/consent/http/service/StudyCommentServiceTest.java | Unit coverage for comment listing/post/delete behavior and visibility gating. |
| src/test/java/org/broadinstitute/consent/http/service/StudyAssetServiceTest.java | Unit coverage for promoted-vs-legacy asset reads and visibility gating. |
| src/test/java/org/broadinstitute/consent/http/service/MetricsServiceTest.java | Unit coverage for study-scoped metrics, outputs aggregation, and recommendations. |
| src/test/java/org/broadinstitute/consent/http/service/DatasetServiceTest.java | Adds tests for shared study visibility gate and updates for new PI fields. |
| src/test/java/org/broadinstitute/consent/http/service/DatasetRegistrationServiceTest.java | Ensures registration updates preserve PI profile links and set PI institution. |
| src/test/java/org/broadinstitute/consent/http/service/dao/DatasetServiceDAOTest.java | Validates study insert/patch/update behavior including PI institution + PI detail fields. |
| src/test/java/org/broadinstitute/consent/http/resources/StudyResourceTest.java | Updates resource tests for new visibility gate and stricter PATCH authorization. |
| src/test/java/org/broadinstitute/consent/http/resources/StudyCommentResourceTest.java | Resource-level tests for comment payload validation and error mapping. |
| src/test/java/org/broadinstitute/consent/http/resources/StudyAssetResourceTest.java | Resource-level tests for asset subresource delegation and error mapping. |
| src/test/java/org/broadinstitute/consent/http/resources/MetricsResourceTest.java | Resource-level tests for new study metrics endpoints and error mapping. |
| src/test/java/org/broadinstitute/consent/http/models/StudyPatchTest.java | Tests new PI column patch semantics (absent/no-op vs explicit null/clear). |
| src/test/java/org/broadinstitute/consent/http/models/StudyAssetsTest.java | Tests promoted asset behavior, legacy fallback, and assembled legacy shape. |
| src/test/java/org/broadinstitute/consent/http/models/dto/registration/RegistrationRequestMapperTest.java | Ensures promoted asset lists map to their own properties and legacy stripping. |
| src/test/java/org/broadinstitute/consent/http/db/StudyRecommendationDAOTest.java | Integration tests for similarity and co-request recommendation queries. |
| src/test/java/org/broadinstitute/consent/http/db/StudyPiInstitutionBackfillMigrationTest.java | Executes shipped Liquibase SQL against seeded data to validate migration. |
| src/test/java/org/broadinstitute/consent/http/db/StudyDAOTest.java | Verifies PI detail column persistence and join behavior. |
| src/test/java/org/broadinstitute/consent/http/db/StudyCommentDAOTest.java | Integration tests for comment upsert/list/delete semantics. |
| src/test/java/org/broadinstitute/consent/http/db/StudyAssetsMigrationTest.java | Executes shipped Liquibase SQL to validate asset promotion/stripping migration. |
| src/test/java/org/broadinstitute/consent/http/db/DatasetDAOTest.java | Updates dataset/study mapping tests for new PI detail columns. |
| src/test/java/org/broadinstitute/consent/http/db/DataAccessRequestDAOTest.java | Tests new study-scoped DAR summary query and progress report filtering. |
| src/test/java/org/broadinstitute/consent/http/authentication/AuthorizationHelperTest.java | Adds coverage for null/empty-role authorization behavior. |
| src/main/resources/changesets/changelog-consent-2026-09-02-study-pi-institution-backfill.xml | Data migration: backfill study.pi_institution_id from legacy property. |
| src/main/resources/changesets/changelog-consent-2026-09-02-study-assets.xml | Data migration: promote asset keys into properties and strip legacy object. |
| src/main/resources/changesets/changelog-consent-2026-08-18-study-pi-details.xml | Schema change: adds PI detail columns + FK + index. |
| src/main/resources/changesets/changelog-consent-2026-08-18-study-comments.xml | Schema change: introduces study_comment table + constraints. |
| src/main/resources/changelog-master.xml | Wires new changesets into Liquibase master changelog. |
| src/main/resources/assets/schemas/Workspace.yaml | Adds OpenAPI schema for workspace assets. |
| src/main/resources/assets/schemas/StudyResearchOutputs.yaml | Adds OpenAPI schema for aggregated progress-report outputs. |
| src/main/resources/assets/schemas/StudyRecommendation.yaml | Adds OpenAPI schema for recommendation results. |
| src/main/resources/assets/schemas/StudyPatch.yaml | Documents new patchable PI fields and their null/omit semantics. |
| src/main/resources/assets/schemas/StudyCommentsSummary.yaml | Adds OpenAPI schema for comments list + average rating. |
| src/main/resources/assets/schemas/StudyComment.yaml | Adds OpenAPI schema for a study comment. |
| src/main/resources/assets/schemas/Study.yaml | Adds PI institution + profile link fields to study schema. |
| src/main/resources/assets/schemas/Maintainer.yaml | Adds schema used by AI model assets. |
| src/main/resources/assets/schemas/IntellectualProperty.yaml | Fixes filingDate schema type/description. |
| src/main/resources/assets/schemas/FundingResource.yaml | Adds OpenAPI schema for funding assets. |
| src/main/resources/assets/schemas/DatasetRegistrationSchemaV1.yaml | Adds promoted asset list fields and deprecates legacy assets. |
| src/main/resources/assets/schemas/DarMetric.yaml | Adds submissionDate + PI/institution fields to DAR metric schema. |
| src/main/resources/assets/schemas/ClinicalTrial.yaml | Adds OpenAPI schema for clinical trial assets. |
| src/main/resources/assets/schemas/Biospecimen.yaml | Adds OpenAPI schema for biospecimen assets. |
| src/main/resources/assets/schemas/AiModel.yaml | Adds OpenAPI schema for AI model assets. |
| src/main/resources/assets/paths/studyWorkspaces.yaml | New study asset endpoint OpenAPI path (workspaces). |
| src/main/resources/assets/paths/studySimilarRecommendations.yaml | New metrics endpoint OpenAPI path (similar studies). |
| src/main/resources/assets/paths/studyResearchOutputs.yaml | New metrics endpoint OpenAPI path (research outputs). |
| src/main/resources/assets/paths/studyPublications.yaml | New study asset endpoint OpenAPI path (publications). |
| src/main/resources/assets/paths/studyPresentations.yaml | New study asset endpoint OpenAPI path (presentations). |
| src/main/resources/assets/paths/studyModels.yaml | New study asset endpoint OpenAPI path (models). |
| src/main/resources/assets/paths/studyIntellectualProperty.yaml | New study asset endpoint OpenAPI path (intellectual property). |
| src/main/resources/assets/paths/studyFundingResources.yaml | New study asset endpoint OpenAPI path (funding resources). |
| src/main/resources/assets/paths/studyFrequentlyRequestedWith.yaml | New metrics endpoint OpenAPI path (co-requested studies). |
| src/main/resources/assets/paths/studyDarSummaries.yaml | New metrics endpoint OpenAPI path (study DAR summaries). |
| src/main/resources/assets/paths/studyComments.yaml | New comments endpoint OpenAPI path (list + post). |
| src/main/resources/assets/paths/studyCommentById.yaml | New comments endpoint OpenAPI path (delete own comment). |
| src/main/resources/assets/paths/studyClinicalTrials.yaml | New study asset endpoint OpenAPI path (clinical trials). |
| src/main/resources/assets/paths/studyById.yaml | Updates PATCH docs for new authorization and new PI patch fields. |
| src/main/resources/assets/api-docs.yaml | Registers new study asset/comment and study metrics OpenAPI routes. |
| src/main/java/org/broadinstitute/consent/http/service/studytemplate/StudyTemplateV1Fields.java | Allows promoted asset fields in template validation/whitelisting. |
| src/main/java/org/broadinstitute/consent/http/service/StudyCommentService.java | Implements comment list/post/delete with role+library-card checks and visibility gating. |
| src/main/java/org/broadinstitute/consent/http/service/StudyAssetService.java | Reads promoted/legacy assets with shared visibility enforcement. |
| src/main/java/org/broadinstitute/consent/http/service/MetricsService.java | Adds study-scoped DAR summaries, research outputs aggregation, and recommendations. |
| src/main/java/org/broadinstitute/consent/http/service/ElasticSearchService.java | Re-assembles legacy-shaped assets object for search indexing after promotion. |
| src/main/java/org/broadinstitute/consent/http/service/DatasetService.java | Introduces shared verifyStudyVisibilityAccess used across study-derived endpoints. |
| src/main/java/org/broadinstitute/consent/http/service/DatasetRegistrationService.java | Ensures PI institution + existing profile links are handled correctly on registration update/insert. |
| src/main/java/org/broadinstitute/consent/http/service/dao/DatasetServiceDAO.java | Persists PI detail columns on update/patch and records PI institution on insert. |
| src/main/java/org/broadinstitute/consent/http/resources/StudyResource.java | Tightens PATCH authorization to creator/custodian/admin and centralizes visibility check. |
| src/main/java/org/broadinstitute/consent/http/resources/StudyCommentResource.java | Adds comments API with stricter JSON typing checks for rating payload. |
| src/main/java/org/broadinstitute/consent/http/resources/StudyAssetResource.java | Adds study asset subresource endpoints delegating to StudyAssetService. |
| src/main/java/org/broadinstitute/consent/http/resources/MetricsResource.java | Adds study-scoped metrics/recommendations endpoints. |
| src/main/java/org/broadinstitute/consent/http/models/StudyResearchOutputs.java | DTO for aggregated progress report outputs. |
| src/main/java/org/broadinstitute/consent/http/models/StudyRecommendation.java | DTO for recommendation results. |
| src/main/java/org/broadinstitute/consent/http/models/StudyPatch.java | Adds PI fields and explicit-null tracking to distinguish omit vs clear semantics. |
| src/main/java/org/broadinstitute/consent/http/models/StudyCommentsSummary.java | DTO for comment list + average rating. |
| src/main/java/org/broadinstitute/consent/http/models/StudyComment.java | DTO for stored comment with display/institution info. |
| src/main/java/org/broadinstitute/consent/http/models/StudyAssets.java | Centralizes promoted asset read/assemble/strip logic with legacy fallback. |
| src/main/java/org/broadinstitute/consent/http/models/Study.java | Adds PI institution and profile link fields. |
| src/main/java/org/broadinstitute/consent/http/models/dto/registration/StudyRegistrationRequest.java | Adds top-level promoted asset fields to registration request DTO. |
| src/main/java/org/broadinstitute/consent/http/models/dto/registration/RegistrationRequestMapper.java | Promotes asset lists into separate study properties and strips legacy object. |
| src/main/java/org/broadinstitute/consent/http/models/dataset_registration_v1/DatasetRegistrationSchemaV1.java | Adds promoted asset lists to generated schema model. |
| src/main/java/org/broadinstitute/consent/http/models/dataset_registration_v1/builder/SchemaFromStudy.java | Emits both promoted fields and legacy-shaped assembled assets for compatibility. |
| src/main/java/org/broadinstitute/consent/http/models/DarMetricsSummary.java | Extends metrics DTO with submission date and PI/institution fields. |
| src/main/java/org/broadinstitute/consent/http/db/StudyRecommendationDAO.java | Adds queries for similar and frequently co-requested studies. |
| src/main/java/org/broadinstitute/consent/http/db/StudyDAO.java | Adds PI detail columns to updates and joins PI institution name on reads. |
| src/main/java/org/broadinstitute/consent/http/db/StudyCommentDAO.java | Adds DAO for comment list/upsert/delete. |
| src/main/java/org/broadinstitute/consent/http/db/mapper/StudyReducer.java | Maps PI institution (id + optional name) onto Study during row reduction. |
| src/main/java/org/broadinstitute/consent/http/db/mapper/DarMetricsSummaryMapper.java | Maps new summary fields and expired computation based on submission date. |
| src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java | Adds study-scoped metrics/progress-report queries and enriches summary rows. |
| src/main/java/org/broadinstitute/consent/http/ConsentModule.java | Registers new services and updates MetricsService provider signature. |
| src/main/java/org/broadinstitute/consent/http/ConsentApplication.java | Registers new Jersey resources for study assets and comments. |
| src/main/java/org/broadinstitute/consent/http/authentication/AuthorizationHelper.java | Null-guards missing user/roles to avoid 500s on @RolesAllowed endpoints. |
| docs/ai/CLAUDE.md | Updates provider-pattern guidance for Guice singletons. |
Review details
- Files reviewed: 95/95 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+108
to
+111
| if (!element.isJsonPrimitive()) { | ||
| throw new BadRequestException("Comment text must be a string."); | ||
| } | ||
| return element.getAsString(); |
| return commentDAO.findByStudyId(studyId).stream() | ||
| .filter(comment -> comment.studyCommentId().equals(id)) | ||
| .findFirst() | ||
| .orElseThrow(); |
Comment on lines
67
to
+83
| resource = new StudyResource(datasetService, datasetRegistrationService, elasticSearchService); | ||
| // The read-access gate now lives in DatasetService#verifyStudyVisibilityAccess (shared with | ||
| // the study asset, comment, and metrics endpoints). These tests exercise the resource, so the | ||
| // mock replays the real rule against whatever isCreatorCustodianOrAdmin each test stubs. | ||
| // DatasetServiceTest covers the rule itself. | ||
| lenient() | ||
| .when(datasetService.verifyStudyVisibilityAccess(any(), any())) | ||
| .thenAnswer( | ||
| invocation -> { | ||
| Study study = invocation.getArgument(0); | ||
| User requestingUser = invocation.getArgument(1); | ||
| if (!datasetService.isCreatorCustodianOrAdmin(requestingUser, study) | ||
| && !Boolean.TRUE.equals(study.getPublicVisibility())) { | ||
| throw new NotFoundException("Study not found"); | ||
| } | ||
| return study; | ||
| }); |
Contributor
|
Some unfiltered AI findings while I go through this manually. Claude Findings
1. Case-variant legacy asset keys disappear. Take a study that carries assets = {"Publications": [...]} and was never re-registered. Both migration changesets use exact-case JSON operators, so they leave the key alone. Then on every read:
The key then vanishes from the registration GET and from the search index. Before this change, findMapPropValue returned the legacy map as it stood and kept it. Pick one convention for both sides: either read the legacy map case-insensitively, or remove from it case-sensitively. 2. Research outputs skip the approval gate. The sibling query, findSummaryMetricApprovedDARsByStudyIdIncludesExpired, works hard to require a final access vote of TRUE. findProgressReportsByStudyId at DataAccessRequestDAO.java:294 checks only parent_id IS NOT NULL, a submission date, and non-archived status. The new DAO test at DataAccessRequestDAOTest.java:1397 builds a parent DAR with no election or vote and still expects both reports back. So on a public study, any signed-in caller can see outputs from a request nobody granted. If a progress report can only exist after a grant in practice, say so in the Javadoc; otherwise add the vote filter. One nit outside the findings: findProgressReportsByDatasetId at DataAccessRequestDAO.java:318 has no production caller — only the DAO test uses it. Codex PI institution edits can be reverted by registration saves. PATCH updates the institution column but leaves the piInstitution study property unchanged. Registration GET reads that stale property, and a subsequent PUT writes it back into the column. Changing institution A to B—or clearing it—can therefore be undone by an unrelated registration edit. Keep both representations synchronized or make registration reads use the column. Legacy asset edits can be silently discarded. Registration GET returns assets both top-level and inside assets. A legacy client that preserves the response and edits only assets.publications sends the unchanged top-level copy too. promotedValue() always selects that copy, so additions, edits, and deletions are ignored despite a successful save. The compatibility path needs to handle conflicting representations. |
fboulnois
reviewed
Sep 8, 2026
fboulnois
left a comment
fboulnois
left a comment
Contributor
There was a problem hiding this comment.
Can we split this out into smaller PRs? 8k lines is not easily human-reviewable.
Aside from that, at first glance I see that tests have been added using bash scripts. I am concerned about the maintainability of this approach. Additionally, I think given their size, each could be a separate reviewable component.
rushtong
reviewed
Sep 8, 2026
Comment on lines
+111
to
+113
| if (user == null || user.getRoles() == null) { | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
This might also be in User.getRoles() if roles were instantiated with an empty list by default. That would also protect other callers from hitting this edge case.
Contributor
Author
|
I'm going to split this up into multiple PRs and remove the bash scripts. Those scripts have been helpful testing against the variants in dev and prod databases, but aren't needed moving forward. |
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-4061
Summary
Adds the backend for the redesigned study page: PI contact details, researcher ratings/comments, first-class registration assets, and study-level metrics and recommendations.
PI details on Study
Four new study columns (pi_institution_id, pi_orcid, pi_linkedin_url, pi_website_url), plumbed through Study, StudyPatch, StudyDAO, and DatasetServiceDAO. Study reads join institution so the PI institution name comes back with the study. Registration already collected the PI institution as the numeric piInstitution study property; the create path now writes the column as well, and a backfill changeset copies the property into the column for existing studies (skipping non-numeric values and ids with no institution row).
Because these are columns rather than study properties, they follow JSON semantics instead of the blank-string-deletes convention used elsewhere in StudyPatch: absent is a no-op, explicit null clears, a blank string normalizes to a clear, a value sets. StudyPatch now records which fields arrived as explicit JSON nulls to make that distinction possible.
Behavior change: PATCH authorization
StudyResource.patchStudyById now requires creator/custodian/admin on this study, not merely a study-editing role somewhere in DUOS. A role-only callewho could PATCH a publicly visible study on develop now getsstration PUT path.
Comments & ratings
New study_comment table (rating 1–5 enforced by check construdy, FK cascade on study and user) with GET (list + average), POST (upsert), and DELETE (own comment only) under api/dataset/study/{studyId}/comments. Posting requires the Researcher role and a library card; reads are @permitAll subject to study visibility.
Asset promotion
Eight asset lists (models, workspaces, presentations, publiclectualProperties, biospecimens, funding) move out of theclient-managed, unvalidated assets blob into their own study_property rows and become first-class registration fields. Reads fall back to the legacy object for studies not re-registered since; a present promoteven when empty, so removing the last asset of a type isn'tundone by a stale copy. Elasticsearch reassembles the original assets shape, so search consumers are unaffected.
Seven new GET /api/dataset/study/{studyId}/assets/* endpoints. Two naming mismatches worth knowing: /assets/intellectualProperty reads intellectualProperties, /assets/fundingResources reads fundindpoint — it's reachable only through the registration GET.
Metrics & recommendations
Cross-cutting
All twelve new @permitAll GETs go through a shared DatasetSecess, so a non-public study is readable only by creator,custodians, and admins. New services are registered as Guice singletons (docs/ai/CLAUDE.md updated with the provider pattern). AuthorizationHelper now
null-guards a user with no role rows, which previously turne
Reviewer notes
Have you read CONTRIBUTING.md lately? If not, do that first.