[DT-3063, DT-3065] Store relationship between DARs/Progress Reports and DAAs, Flag DARs/PRs for SO approval when user not pre authorized for DAAs by otchet-broad · Pull Request #2859 · DataBiosphere/consent

otchet-broad · 2026-04-08T21:20:09Z

Addresses

https://broadworkbench.atlassian.net/browse/DT-3063
https://broadworkbench.atlassian.net/browse/DT-3065

Summary

Provides an API for obtaining the mapping of the DAAs to a list of datasets so that the client can display the relationship
Validates the DAAs submitted match the DAR or Progress Report (when not a closeout) expected DAAs and routes the DAR to either the SO or the DAC.
Stores the relationship between data_access_request table entries with DAAs on DAR or Progress Report (when not a closeout) submission

Have you read CONTRIBUTING.md lately? If not, do that first.

Label PR with a Jira ticket number and include a link to the ticket
Label PR with a security risk modifier [no, low, medium, high]
PR describes scope of changes
Get a minimum of one thumbs worth of review, preferably two if enough team members are available
Get PO sign-off for all non-trivial UI or workflow changes
Verify all tests go green
Test this change deployed correctly and works on dev environment after deployment

…pproval.

…ve DAAs.

…after closeouts or DMIs are SO approved.

…se where system is configured without DAAs for DACs.

rushtong

Still reviewing functionally, but wanted to get this first pass of feedback out in the meantime.

rushtong · 2026-04-10T11:53:26Z

+  }
+
+  @Override
+  public Stream<Entry<Integer, Set<Integer>>> stream(Map<Integer, Set<Integer>> container) {


This looks super useful 👍🏽

rushtong · 2026-04-10T11:59:14Z

+      Integer darId =
+          dataAccessRequestDAO.insertDataAccessRequest(
+              collectionId,
+              referenceId,
+              user.getUserId(),
+              now,
+              now,
+              now,
+              darData,
+              user.getEraCommonsId());


It would be nice if these were both in a single transaction. I have some WITH examples that might serve as a template for this in one of my open PRs.

I might go with a slightly different approach to see how we like it, but it will be in one transaction.

Coming back to this one. It looks like there were already multiple database updates in this method that should effectively unwind if there's an error here, but because of the number of elements involved, I'm wondering if we should make a ticket for this and come back to it later. Would that be acceptable? There are also other methods in this class (and related services) that need to be linked as well, hence my hesitation to add this to the scope of this ticket.

Sure thing - it would be nice to tighten this up separately

DT-3176 created.

rushtong · 2026-04-10T12:04:56Z

+    // We need to set the new DAAs that were in place on the DAR because the DAAs may have been
+    // updated
+    // from the original DAR.
+    originalDataCopy.setDaaIds(newData.getDaaIds());


Just clarifying that I think we're planning on the FE to populate these values on submission. If not, these will be empty.

Exactly. It's the next ticket I'm grabbing. :-)

rushtong · 2026-04-10T12:12:59Z

+      """
+        INSERT INTO dar_daa (dar_id, daa_id) VALUES (:darId, :daaId)
+    """)
+  void insertDarDAARelationship(@Bind("darId") Integer darId, @Bind("daaId") Set<Integer> daaIds);


Copilot caught this for me - I was unaware of this detail. See current jdbi doc for reference.

In JDBI 3, all @SqlBatch parameters are expected to be iterable. A non-iterable scalar must be annotated with @SingleValue to be held constant per row. Without it, behavior is undefined across JDBI point-releases and the batch will fail if the set has more than one element.

Fix:

void insertDarDAARelationship(@Bind("darId") @SingleValue Integer darId, @Bind("daaId") Set<Integer> daaIds);

I want to spend some time looking at this because I think I wrote a test that ends up proving this works. See testStoreDarDAARelationshipForPR in DaaDAOTest.java

I've updated the method and added the @SingleValue decoration. No change in the test result. Looking at the docs, they all are using the @SingleValue decoration on Collection parameters. That's different than what I've got in the code. darId is a single integer value. I've added an assertion to the testStoreDarDAARelationshipForPR to confirm multiple rows are being inserted as expected.

rushtong

Thank you for the updates 👍🏽

…lationships and record new ones based on the object submitted.

Copilot

Pull request overview

Adds DAA↔dataset mapping support and persists DAR/Progress Report ↔ DAA relationships, while updating DAR/PR routing logic to require Signing Official (SO) approval when users are not pre-authorized for the required DAAs.

Changes:

Add dar_daa persistence (Liquibase + DAO methods) and store DAR/PR↔DAA relationships on submission.
Add a new DAA API endpoint to return DAA→datasets mapping for a provided set of dataset IDs.
Update DAR/Progress Report workflow logic to validate required DAA acknowledgements and to flag SO approval when the submitter is not pre-authorized.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/main/java/org/broadinstitute/consent/http/service/DataAccessRequestService.java	Stores DAR/PR↔DAA relationships and adjusts SO-approval routing/validation logic.
src/main/java/org/broadinstitute/consent/http/service/DarCollectionService.java	Skips automation-rule triggering for closeouts/DMIs on SO approval.
src/main/java/org/broadinstitute/consent/http/service/DaaService.java	Adds service method to fetch DAA→dataset mapping.
src/main/java/org/broadinstitute/consent/http/resources/DaaResource.java	Adds `/api/daa/datasets` endpoint to expose DAA→dataset mapping.
src/main/java/org/broadinstitute/consent/http/db/DaaDAO.java	Adds mapping query + DAR/PR↔DAA insert/delete operations.
src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java	Returns generated IDs for DAR/PR inserts so relationships can be stored.
src/main/java/org/broadinstitute/consent/http/db/mapper/DaaDatasetReducer.java	Reduces rows into a DAA→datasets map for the new query.
src/main/java/org/broadinstitute/consent/http/models/DataAccessRequestData.java	Ensures `daaIds` getter returns an empty set when unset.
src/main/java/org/broadinstitute/consent/http/models/DataAccessRequest.java	Copies DAA IDs when populating a progress report from JSON.
src/main/resources/changesets/changelog-consent-2026-04-08-dar-daa.xml	Introduces new `dar_daa` table.
src/main/resources/changelog-master.xml	Includes the new Liquibase changeset.
src/main/resources/assets/api-docs.yaml	Registers the new API path.
src/main/resources/assets/paths/daaDatasets.yaml	Documents the new DAA→datasets mapping endpoint.
src/test/java/... (multiple test files)	Updates/adds tests for DAA validation, SO-approval routing, and new endpoint/DAO behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-21T18:40:49Z

+      <column name="daa_id" type="bigint">
+        <constraints nullable="false" foreignKeyName="fkDaa_Daa_DaaId" referencedTableName="data_access_agreement" referencedColumnNames="daa_id" />
+      </column>
+    </createTable>


The new dar_daa join table is missing a composite primary key / unique constraint on (dar_id, daa_id). Without it, duplicate relationships can be inserted, and the DAO’s INSERT ... ON CONFLICT DO NOTHING won’t prevent duplicates because there is no conflict target to trigger on. Consider adding an addPrimaryKey (or addUniqueConstraint) for (dar_id, daa_id) similar to dac_daa and lc_daa join tables.

Suggested change

</createTable>

</createTable>

<addPrimaryKey tableName="dar_daa" columnNames="dar_id, daa_id" constraintName="pk_dar_daa"/>

Copilot · 2026-04-21T18:40:50Z

+  @POST
+  @RolesAllowed({ADMIN, CHAIRPERSON, MEMBER, SIGNINGOFFICIAL, RESEARCHER})
+  @Path("datasets")
+  public Response findDaaForDatasets(@Auth DuosUser duosUser, String json) {
+    try {
+      Gson gson = new Gson();
+      Type setType = new TypeToken<Set<Integer>>() {}.getType();
+      Set<Integer> set = gson.fromJson(json, setType);
+      return Response.ok(daaService.findDaaIdsByDatasetIds(set)).build();
+    } catch (Exception e) {
+      return createExceptionResponse(e);
+    }


findDaaForDatasets catches a generic Exception and delegates to createExceptionResponse, which will return a 500 for JSON parsing errors (e.g., malformed JSON or wrong type). Since this is a client input problem, it should return a 400 (e.g., by validating the parsed set is non-null and throwing BadRequestException, and/or specifically catching JsonSyntaxException / JsonParseException).

Copilot · 2026-04-21T18:40:50Z

+    try {
+      Gson gson = new Gson();
+      Type setType = new TypeToken<Set<Integer>>() {}.getType();
+      Set<Integer> set = gson.fromJson(json, setType);


findDaaForDatasets parses the request body into Set<Integer> set but does not handle the case where fromJson returns null (e.g., empty body) before calling the service. If set is null, this will likely NPE downstream and return a 500; consider explicitly rejecting null with a BadRequestException (or treating it as an empty set if that’s intended).

Suggested change

Set<Integer> set = gson.fromJson(json, setType);

Set<Integer> set = gson.fromJson(json, setType);

if (set == null) {

throw new BadRequestException("Request body must contain a JSON array of dataset IDs.");

}

Copilot · 2026-04-21T18:40:50Z

    return daaDAO.findByDarReferenceId(referenceId);
  }
+
+  public Map<Integer, Set<Integer>> findDaaIdsByDatasetIds(Set<Integer> datasetIds) {


The new API method name findDaaIdsByDatasetIds is misleading: it returns a Map<Integer, Set<Integer>> (DAA -> dataset IDs), not a set/list of DAA IDs. Renaming to something like mapDaaIdsToDatasetIds / findDaaDatasetMapping would make call sites clearer and avoid confusion with DaaDAO.findDaaIdsByDatasetIds (which does return IDs).

Suggested change

public Map<Integer, Set<Integer>> findDaaIdsByDatasetIds(Set<Integer> datasetIds) {

public Map<Integer, Set<Integer>> mapDaaIdsToDatasetIds(Set<Integer> datasetIds) {

Copilot · 2026-04-21T18:40:51Z

+              darData,
+              user.getEraCommonsId());
+    }
+    daaDAO.insertDarDAARelationship(darId, dataAccessRequest.data.getDaaIds());


In createDataAccessRequest, the code inserts the DAR↔DAA relationship using dataAccessRequest.data.getDaaIds() (direct field access) even though the method already uses dataAccessRequest.getData() elsewhere. Using the getter consistently avoids bypassing encapsulation and reduces the risk of null/visibility issues if the model changes.

Suggested change

daaDAO.insertDarDAARelationship(darId, dataAccessRequest.data.getDaaIds());

daaDAO.insertDarDAARelationship(darId, darData.getDaaIds());

sonarqubecloud · 2026-04-27T22:14:15Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

otchet-broad added 5 commits April 8, 2026 17:13

Initial implementation.

ecdf837

Add assertions to tests.

d95f8a5

Remove unused record class.

18f6400

Add coverage and update condition for empty collection, not a null value

4fca5c5

Add swagger docs.

03a4058

otchet-broad marked this pull request as ready for review April 8, 2026 22:24

otchet-broad requested a review from a team as a code owner April 8, 2026 22:24

otchet-broad requested review from eweitz, fboulnois, kevinmarete and rushtong and removed request for a team April 8, 2026 22:24

otchet-broad marked this pull request as draft April 8, 2026 22:41

Allow DAR/PR to submit if user doesn't have DAA and route to SO for a…

07e15d9

…pproval.

otchet-broad removed request for eweitz, fboulnois, kevinmarete and rushtong April 8, 2026 22:48

otchet-broad added 5 commits April 8, 2026 20:27

Update tests to cover DAR submission routing when the user doesn't ha…

599dcbf

…ve DAAs.

Spotless.

c42c7ed

Fix argument matchers

d368689

Add progress reports to the approval flow, make sure rules don't run …

19fe045

…after closeouts or DMIs are SO approved.

Remove throws from method signature.

3671147

otchet-broad changed the title ~~[DT-3063] Store relationship between DARs/Progress Reports and DAAs~~ [DT-3063, DT-3065] Store relationship between DARs/Progress Reports and DAAs Apr 9, 2026

otchet-broad changed the title ~~[DT-3063, DT-3065] Store relationship between DARs/Progress Reports and DAAs~~ [DT-3063, DT-3065] Store relationship between DARs/Progress Reports and DAAs, Flag DARs/PRs for SO approval when user not pre authorized for DAAs Apr 9, 2026

otchet-broad added 4 commits April 9, 2026 15:06

Refactor to use LC DAA information about the user; Add support for ca…

3f2018d

…se where system is configured without DAAs for DACs.

Increase coverage by DarCollectionServiceTest

279c697

Increase test coverage.

0a1b210

After refactoring method it's entirely possible to remove the todo.

e1352ac

otchet-broad marked this pull request as ready for review April 9, 2026 21:56

otchet-broad requested review from eweitz, fboulnois, kevinmarete and rushtong April 9, 2026 21:58

rushtong reviewed Apr 10, 2026

View reviewed changes

otchet-broad added 5 commits April 10, 2026 10:05

Validate two rows were inserted in SQL Batch.

cf04234

Spotless.

5458665

Add basic indexes to table.

f292f6b

Update swagger api per feedback.

c26a32a

Rename method to better describe what it is doing.

f7fc58d

rushtong approved these changes Apr 10, 2026

View reviewed changes

otchet-broad added 2 commits April 10, 2026 13:43

When converting a saved DAR to a submitted DAR, delete the old DAA re…

0f3dfcc

…lationships and record new ones based on the object submitted.

Spotless

f7f3a94

kevinmarete approved these changes Apr 11, 2026

View reviewed changes

Merge branch 'develop' into otchet-dt-3063-find-daas-for-dataset-ids

c0d7710

otchet-broad added the DO NOT MERGE label Apr 13, 2026

otchet-broad mentioned this pull request Apr 21, 2026

[DT-3100] Add DAAs to progress reports; add DAA IDs to submission payload for DARs & PRs DataBiosphere/duos-ui#3443

Merged

Merge branch 'develop' into otchet-dt-3063-find-daas-for-dataset-ids

301291f

Copilot AI review requested due to automatic review settings April 21, 2026 18:34

Copilot started reviewing on behalf of otchet-broad April 21, 2026 18:35 View session

Copilot AI reviewed Apr 21, 2026

View reviewed changes

otchet-broad added 4 commits April 27, 2026 11:59

Merge branch 'develop' into otchet-dt-3063-find-daas-for-dataset-ids

f62fff6

Adjust tests to new method signature.

1381aba

Merge develop.

cd5f5e6

Merge branch 'develop' into otchet-dt-3063-find-daas-for-dataset-ids

1566bdd

[DT-3054] Remove obe endpoints (#2880)

068b1aa

eweitz approved these changes Apr 28, 2026

View reviewed changes

	</createTable>
	</createTable>
	<addPrimaryKey tableName="dar_daa" columnNames="dar_id, daa_id" constraintName="pk_dar_daa"/>

	public Map<Integer, Set<Integer>> findDaaIdsByDatasetIds(Set<Integer> datasetIds) {
	public Map<Integer, Set<Integer>> mapDaaIdsToDatasetIds(Set<Integer> datasetIds) {

	daaDAO.insertDarDAARelationship(darId, dataAccessRequest.data.getDaaIds());
	daaDAO.insertDarDAARelationship(darId, darData.getDaaIds());

Conversation

otchet-broad commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Addresses

Summary

Uh oh!

rushtong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otchet-broad Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rushtong left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Apr 27, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

otchet-broad commented Apr 8, 2026 •

edited

Loading

otchet-broad Apr 10, 2026 •

edited

Loading