[DUOS-2781][DUOS-2782] DAC rejects + accepts datasets #2215

hams7504 · 2023-12-15T16:02:43Z

Addresses

https://broadworkbench.atlassian.net/browse/DUOS-2781
https://broadworkbench.atlassian.net/browse/DUOS-2782

Summary

Changed email content for the case where a DAC accepts or rejects a dataset

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

rushtong

👍🏽

src/main/java/org/broadinstitute/consent/http/mail/freemarker/DatasetApprovedModel.java

src/main/java/org/broadinstitute/consent/http/service/DatasetService.java

src/test/java/org/broadinstitute/consent/http/service/DatasetServiceTest.java

rjohanek

Looks good! I have a question about the test but not a blocker

pshapiro4broad

Just a few comments, looks OK overall

pshapiro4broad · 2023-12-20T15:03:57Z

src/main/java/org/broadinstitute/consent/http/mail/freemarker/DatasetDeniedModel.java

@@ -5,11 +5,13 @@ public class DatasetDeniedModel {
  private String dataSubmitterName;
  private String datasetName;
  private String dacName;
+  private String dacEmail;


It looks like these fields can be declared as final, since they're not modified once the object is constructed. If they were, then this could become a record:

public record DatasetDeniedModel(String dataSubmitterName, String datasetName, String dacName) { }

I like the idea of moving to record classes, this is a pretty old part of the code base.

pshapiro4broad · 2023-12-20T15:11:10Z

src/main/java/org/broadinstitute/consent/http/service/DatasetService.java

-          user,
-          dac.getName(),
-          dataset.getDatasetIdentifier());
+      if (!Objects.isNull(dac.getEmail())) {


It looks this use of Objects.isNull() is common in this codebase, and it works, but it wasn't intended for this use. It was intended to be used as a predicate, such as for a streaming operation. See https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Objects.html#isNull(java.lang.Object). Personally I would compare against null directly instead:

Suggested change

if (!Objects.isNull(dac.getEmail())) {

if (dac.getEmail() != null) {

If you do want to use an Objects method, I would use nonNull() instead as then you don't have to negate the result:

Suggested change

if (!Objects.isNull(dac.getEmail())) {

if (Objects.nonNull(dac.getEmail())) {

Thanks for the link to the doc, don't think I've ever read that. I've been suggesting the Objects methods for some time now.

If you'd rather not compare against null directly, a lot of people like using Optional.ofNullable() instead. So you'd write something like:

Optional.ofNullable(dac.getEmail()).ifPresentOrElse(dacEmail -> emailService.sendDatasetDeniedMessage( user, dac.getName(), dataset.getDatasetIdentifier(), dacEmail)).orElse(), () -> logWarn("Unable to send dataset denied email to DAC: " + dac.getDacId());

pshapiro4broad · 2023-12-20T15:19:24Z

src/main/resources/freemarker/dataset-approved.html

@@ -22,7 +22,7 @@
        <tr>
            <td style="padding: 15px 15px 0px; font-family: 'Montserrat', sans-serif; font-size: 16px; color: #1F3B50; text-align: justify; line-height: 25px;">
                <p id="content" style="margin: 0;">
-                    Thank you for using DUOS to register your dataset(s). We’re happy to inform you that ${dacName} has accepted your dataset. The ${datasetName} dataset should now be visible in the DUOS dataset catalog. Researchers will now be able to submit data access requests through DUOS for access to your dataset.
+                  Your dataset, ${datasetName}, submitted to the ${dacName} by ${dataSubmitterName} for management of future data access requests has been <b>accepted</b> and can be found in the DUOS Data Library with DUOS ID: ${datasetName}


I think the phrase submitted to the ${dacName} by ${dataSubmitterName} for management of future data access requests is an appositive clause so it also should have commas around it, in addition to datasetName since it's also an appositive for Your dataset. Also maybe it should be split into two sentences with a period at the end.

Suggested change

Your dataset, ${datasetName}, submitted to the ${dacName} by ${dataSubmitterName} for management of future data access requests has been <b>accepted</b> and can be found in the DUOS Data Library with DUOS ID: ${datasetName}

Your dataset, ${datasetName}, submitted to the ${dacName} by ${dataSubmitterName} for management of future data access requests, has been <b>accepted</b>. It can be found in the DUOS Data Library with DUOS ID: ${datasetName}.

As I understand it, the rationale for separating the appositive clause like this is to make it easer for a reader to find the verb and direct object of a sentence.

The language is specified by product - I'd prefer to leave this decision to @solideoglori for language choice in these cases.

pshapiro4broad · 2023-12-20T15:23:25Z

src/test/java/org/broadinstitute/consent/http/service/DatasetServiceTest.java

+    User user = new User();
+    user.setUserId(1);
+    user.setEmail("[email protected]");
+    user.setDisplayName("John Doe");


User has a constructor that can be used here

Suggested change

User user = new User();

user.setUserId(1);

user.setEmail("[email protected]");

user.setDisplayName("John Doe");

User user = new User(1, "[email protected], "John Doe", null);

pshapiro4broad · 2023-12-20T15:25:37Z

src/test/java/org/broadinstitute/consent/http/service/DatasetServiceTest.java

+    assertFalse(returnedDataset.getDacApproval());
+
+    // do not send denied email
+    verify(emailService, times(0)).sendDatasetDeniedMessage(


A nice alias for times(0) is never()

Suggested change

verify(emailService, times(0)).sendDatasetDeniedMessage(

verify(emailService, never()).sendDatasetDeniedMessage(

src/test/java/org/broadinstitute/consent/http/service/DatasetServiceTest.java

change email contents

a11f7e3

hams7504 changed the title ~~change email contents~~ [DUOS-2782] change email contents Dec 18, 2023

hams7504 changed the title ~~[DUOS-2782] change email contents~~ [DUOS-2782] DAC accepts datasets Dec 18, 2023

hams7504 marked this pull request as ready for review December 18, 2023 14:34

hams7504 requested a review from a team as a code owner December 18, 2023 14:34

rushtong approved these changes Dec 18, 2023

View reviewed changes

[DUOS-2781] DAC rejects datasets (#2216)

b4580e3

hams7504 changed the title ~~[DUOS-2782] DAC accepts datasets~~ [DUOS-2781][DUOS-2782] DAC rejects + accepts datasets Dec 20, 2023

rjohanek reviewed Dec 20, 2023

View reviewed changes

src/main/java/org/broadinstitute/consent/http/mail/freemarker/DatasetApprovedModel.java Show resolved Hide resolved

rjohanek reviewed Dec 20, 2023

View reviewed changes

src/main/java/org/broadinstitute/consent/http/service/DatasetService.java Show resolved Hide resolved

rjohanek reviewed Dec 20, 2023

View reviewed changes

src/test/java/org/broadinstitute/consent/http/service/DatasetServiceTest.java Show resolved Hide resolved

rjohanek approved these changes Dec 20, 2023

View reviewed changes

hams7504 merged commit 8c6af83 into develop Dec 20, 2023
11 checks passed

hams7504 deleted the hm-duos-2782-dac-accepts-datasets branch December 20, 2023 15:28

pshapiro4broad reviewed Dec 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DUOS-2781][DUOS-2782] DAC rejects + accepts datasets #2215

[DUOS-2781][DUOS-2782] DAC rejects + accepts datasets #2215

hams7504 commented Dec 15, 2023 •

edited

Loading

rushtong left a comment

rjohanek left a comment

pshapiro4broad left a comment

pshapiro4broad Dec 20, 2023

rushtong Dec 20, 2023

pshapiro4broad Dec 20, 2023

rushtong Dec 20, 2023

pshapiro4broad Dec 20, 2023

pshapiro4broad Dec 20, 2023

rushtong Dec 20, 2023

pshapiro4broad Dec 20, 2023

pshapiro4broad Dec 20, 2023

rushtong Dec 20, 2023

	if (!Objects.isNull(dac.getEmail())) {
	if (dac.getEmail() != null) {

	if (!Objects.isNull(dac.getEmail())) {
	if (Objects.nonNull(dac.getEmail())) {

	Your dataset, ${datasetName}, submitted to the ${dacName} by ${dataSubmitterName} for management of future data access requests has been <b>accepted</b> and can be found in the DUOS Data Library with DUOS ID: ${datasetName}
	Your dataset, ${datasetName}, submitted to the ${dacName} by ${dataSubmitterName} for management of future data access requests, has been <b>accepted</b>. It can be found in the DUOS Data Library with DUOS ID: ${datasetName}.

	verify(emailService, times(0)).sendDatasetDeniedMessage(
	verify(emailService, never()).sendDatasetDeniedMessage(

[DUOS-2781][DUOS-2782] DAC rejects + accepts datasets #2215

[DUOS-2781][DUOS-2782] DAC rejects + accepts datasets #2215

Conversation

hams7504 commented Dec 15, 2023 • edited Loading

Addresses

Summary

rushtong left a comment

Choose a reason for hiding this comment

rjohanek left a comment

Choose a reason for hiding this comment

pshapiro4broad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hams7504 commented Dec 15, 2023 •

edited

Loading