[DUOS-1764, DUOS-1765][risk=no] Create a single DAR for N Datasets on submission #1648

rushtong · 2022-07-12T12:25:10Z

Addresses

https://broadworkbench.atlassian.net/browse/DUOS-1764
https://broadworkbench.atlassian.net/browse/DUOS-1765

Changes

Update to the DAR submission process so we only create a single DAR instead of N DARs per dataset requested.
Updates to resource class and tests to cover this change.

Note that we should immediately follow up on https://broadworkbench.atlassian.net/browse/DUOS-1961 as that story is blocked by this one and we will no longer need to archive a group of DARs (DARCollection) and recreate a new draft from them.

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

# Conflicts: # src/main/java/org/broadinstitute/consent/http/service/DataAccessRequestService.java # src/main/java/org/broadinstitute/consent/http/service/ElectionService.java # src/test/java/org/broadinstitute/consent/http/db/DataAccessRequestDAOTest.java

# Conflicts: # src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java # src/main/java/org/broadinstitute/consent/http/db/mapper/DataAccessRequestReducer.java

rushtong · 2022-07-20T14:11:04Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-  /**
-   * Returns all dataset_ids that match any of the referenceIds inside of the "referenceIds" list
-   *
-   * @param referenceIds List<String>
-   */
-  @RegisterRowMapper(DarDatasetMapper.class)
-  @SqlQuery("SELECT distinct reference_id, dataset_id FROM dar_dataset WHERE reference_id IN (<referenceIds>)")
-  List<DarDataset> findAllDARDatasets(@BindList("referenceIds") List<String> referenceIds);
-
-  @SqlQuery(
-      " SELECT distinct d.reference_id "
-          + " FROM dar_dataset d "
-          + " INNER JOIN data_access_request dar ON dar.reference_id = d.reference_id AND dar.collection_id = :collectionId "
-          + " WHERE d.dataset_id IN <datasetIds> ")
-  List<String> findReferenceIdsForDatasetIdsWithCollectionId(
-      @BindList("datasetIds") List<Integer> datasetIds, @Bind("collectionId") Integer collectionId);


…on type, and dataset id.

JVThomas · 2022-07-26T13:21:38Z

src/main/java/org/broadinstitute/consent/http/db/ElectionDAO.java

+    @SqlQuery(
+        " SELECT e.* " +
+        " FROM election e " +
+        " INNER JOIN (SELECT referenceid, datasetid, MAX(createdate) max_date FROM election WHERE LOWER(electiontype) = lower(:type) AND datasetid = :datasetId GROUP BY referenceid, datasetid) election_view " +


Why use MAX(createdate) over something like MAX(electionid)?

Mostly because we use it a lot in other queries. I believe both approaches will work equally well.

shaemarks

Looks good overall! When run locally with the ui, I submitted a multi-dataset dar, and the resulting collection contained the single dar with all of the datasets.

However, I think some changes will need to be made to the bucketing logic on the frontend to support this change. The collection I created contained 4 datause buckets, each containing all of the elections since they are processed by dar. The votes are then processed by elections, so voting shouldn't be accurate with these changes.

rushtong · 2022-07-26T17:26:31Z

However, I think some changes will need to be made to the bucketing logic on the frontend to support this change. The collection I created contained 4 datause buckets, each containing all of the elections since they are processed by dar. The votes are then processed by elections, so voting shouldn't be accurate with these changes.

I did actually test this case, but the bucketing and votes looked correct to me when I did so. That could have been an edge case where it worked, and there are others where it doesn't ... I'll try to dig more into the front end and figure something out.

JVThomas · 2022-07-26T17:56:32Z

src/main/java/org/broadinstitute/consent/http/service/dao/DarCollectionServiceDAO.java

+                dar.getDatasetIds().forEach(datasetId -> {
+                    // If there is an existing open election for this DAR+Dataset, we can ignore it
+                    Election lastDataAccessElection = electionDAO.findLastElectionByReferenceIdDatasetIdAndType(dar.getReferenceId(), datasetId, ElectionType.DATA_ACCESS.getValue());
+                    boolean ignore = Objects.nonNull(lastDataAccessElection) && lastDataAccessElection.getStatus().equals(ElectionStatus.OPEN.getValue());


Nit: Just to be safe, can you update the old equals usage with equalsIgnoreCase?

JVThomas

Minor nitpick, but the code looks good. That said, Shae is right about the bucketing logic on the front-end. It needs to be updated to account for the "one dar to many elections" relationship. An update there needs to happen before this PR can go to staging/production.

rushtong added 9 commits July 11, 2022 12:23

refactor deprecated update method

a328a3d

fix compile

91303bb

test fixes

73857c3

minor renaming

97f0da6

add, don't replace

0277575

ensure population on update draft

074edc9

cleanup

c25b783

Merge branch 'develop' into DUOS-1763-v3

2e91c5e

first pass

b592882

rushtong changed the base branch from develop to DUOS-1763-v3 July 12, 2022 12:25

rushtong added 17 commits July 12, 2022 08:48

update match service to process all datasets on the DAR

68f69a3

use reducer and remove limit on find dar by reference id

015d97a

unused

7439a33

first pass at creating elections correctly

a6c975e

fix query, add test

6173b74

Merge branch 'develop' into DUOS-1763-v3

a401302

Merge branch 'DUOS-1763-v3' into DUOS-1764

59a0233

expand tests

9adb260

imports

f3d3c1a

cleanup

1867a2b

fix vote insert

20fffc8

todos

81e4187

import

1577f29

latest election exclusions on dataset id for dar collections

43f2540

cleanup dao call

2ae49c2

data is populated in the mapper already.

4bf09bc

update todos

5663259

rushtong changed the title ~~[DUOS-1764][risk=no] Create a single DAR for N Datasets on submission~~ [DUOS-1764, DUOS-1765][risk=no] Create a single DAR for N Datasets on submission Jul 14, 2022

rushtong mentioned this pull request Jul 14, 2022

[DUOS-1764][risk=no] Loop over all dataset ids instead of assuming the first DataBiosphere/duos-ui#1677

Merged

Base automatically changed from DUOS-1763-v3 to develop July 15, 2022 13:37

rushtong added 4 commits July 15, 2022 11:18

merge fix

eac4223

Merge branch 'develop' into DUOS-1764

ca340fc

Merge branch 'develop' into DUOS-1764

a07eb68

# Conflicts: # src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java # src/main/java/org/broadinstitute/consent/http/db/mapper/DataAccessRequestReducer.java

rushtong commented Jul 20, 2022

View reviewed changes

rushtong added 3 commits July 20, 2022 10:20

cleanup

c4f9ec1

Merge branch 'develop' into DUOS-1764

f591217

fix version calculation - it should always be by reference id, electi…

a023232

…on type, and dataset id.

rushtong marked this pull request as ready for review July 21, 2022 20:03

rushtong requested a review from a team as a code owner July 21, 2022 20:03

rushtong requested review from JVThomas and shaemarks July 26, 2022 13:01

Merge branch 'develop' into DUOS-1764

818a24e

JVThomas reviewed Jul 26, 2022

View reviewed changes

shaemarks reviewed Jul 26, 2022

View reviewed changes

minor logic update to prevent multiple collections for a dar

0891ff7

JVThomas reviewed Jul 26, 2022

View reviewed changes

JVThomas approved these changes Jul 27, 2022

View reviewed changes

PR feedback

d73751b

shaemarks approved these changes Jul 27, 2022

View reviewed changes

rushtong merged commit 492354e into develop Jul 27, 2022

rushtong deleted the DUOS-1764 branch July 27, 2022 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DUOS-1764, DUOS-1765][risk=no] Create a single DAR for N Datasets on submission #1648

[DUOS-1764, DUOS-1765][risk=no] Create a single DAR for N Datasets on submission #1648

rushtong commented Jul 12, 2022 •

edited

Loading

rushtong Jul 20, 2022

JVThomas Jul 26, 2022

rushtong Jul 26, 2022

shaemarks left a comment

rushtong commented Jul 26, 2022

JVThomas Jul 26, 2022 •

edited

Loading

JVThomas left a comment •

edited

Loading

[DUOS-1764, DUOS-1765][risk=no] Create a single DAR for N Datasets on submission #1648

[DUOS-1764, DUOS-1765][risk=no] Create a single DAR for N Datasets on submission #1648

Conversation

rushtong commented Jul 12, 2022 • edited Loading

Addresses

Changes

rushtong Jul 20, 2022

Choose a reason for hiding this comment

JVThomas Jul 26, 2022

Choose a reason for hiding this comment

rushtong Jul 26, 2022

Choose a reason for hiding this comment

shaemarks left a comment

Choose a reason for hiding this comment

rushtong commented Jul 26, 2022

JVThomas Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

JVThomas left a comment • edited Loading

Choose a reason for hiding this comment

rushtong commented Jul 12, 2022 •

edited

Loading

JVThomas Jul 26, 2022 •

edited

Loading

JVThomas left a comment •

edited

Loading