Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Dec 20, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a11f7e3
change email contents
hams7504 Dec 15, 2023
b4580e3
[DUOS-2781] DAC rejects datasets (#2216)
hams7504 Dec 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/main/java/org/broadinstitute/consent/http/mail/freemarker/DatasetApprovedModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,5 @@ public String getDatasetName() {
public String getDacName() {
return dacName;
}

hams7504 marked this conversation as resolved.
Show resolved Hide resolved
}
9 changes: 8 additions & 1 deletion src/main/java/org/broadinstitute/consent/http/mail/freemarker/DatasetDeniedModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ public class DatasetDeniedModel {
private String dataSubmitterName;
private String datasetName;
private String dacName;
private String dacEmail;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public DatasetDeniedModel(String dataSubmitterName, String datasetName, String dacName) {
public DatasetDeniedModel(String dataSubmitterName, String datasetName, String dacName, String dacEmail) {
this.dataSubmitterName = dataSubmitterName;
this.datasetName = datasetName;
this.dacName = dacName;
this.dacEmail = dacEmail;
}

public String getDataSubmitterName() {
Expand All @@ -23,4 +25,9 @@ public String getDatasetName() {
public String getDacName() {
return dacName;
}

public String getDacEmail() {
return dacEmail;
}

}
8 changes: 4 additions & 4 deletions src/main/java/org/broadinstitute/consent/http/mail/freemarker/FreeMarkerTemplateHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ public Writer getDatasetApprovedTemplate(String dataSubmitterName, String datase
}

public Writer getDatasetDeniedTemplate(String dataSubmitterName, String datasetName,
String dacName) throws IOException, TemplateException {
String dacName, String dacEmail) throws IOException, TemplateException {
Template temp = freeMarkerConfig.getTemplate("dataset-denied.html");
return generateDatasetDeniedTemplate(dataSubmitterName, datasetName, dacName, temp);
return generateDatasetDeniedTemplate(dataSubmitterName, datasetName, dacName, dacEmail, temp);
}

public Writer getNewResearcherLibraryRequestTemplate(String researcherName, String serverUrl)
Expand All @@ -90,8 +90,8 @@ private Writer generateDatasetApprovedTemplate(String dataSubmitterName, String
}

private Writer generateDatasetDeniedTemplate(String dataSubmitterName, String datasetName,
String dacName, Template temp) throws IOException, TemplateException {
DatasetDeniedModel model = new DatasetDeniedModel(dataSubmitterName, datasetName, dacName);
String dacName, String dacEmail, Template temp) throws IOException, TemplateException {
DatasetDeniedModel model = new DatasetDeniedModel(dataSubmitterName, datasetName, dacName, dacEmail);
Writer out = new StringWriter();
temp.process(model, out);
return out;
Expand Down
18 changes: 13 additions & 5 deletions src/main/java/org/broadinstitute/consent/http/service/DatasetService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,12 @@
import org.broadinstitute.consent.http.models.dto.DatasetDTO;
import org.broadinstitute.consent.http.models.dto.DatasetPropertyDTO;
import org.broadinstitute.consent.http.service.dao.DatasetServiceDAO;
import org.broadinstitute.consent.http.util.ConsentLogger;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;


public class DatasetService {
public class DatasetService implements ConsentLogger {

private final Logger logger = LoggerFactory.getLogger(this.getClass());
public static final String DATASET_NAME_KEY = "Dataset Name";
Expand Down Expand Up @@ -345,10 +346,17 @@ private void sendDatasetApprovalNotificationEmail(Dataset dataset, User user, Bo
dac.getName(),
dataset.getDatasetIdentifier());
} else {
emailService.sendDatasetDeniedMessage(
user,
dac.getName(),
dataset.getDatasetIdentifier());
if (!Objects.isNull(dac.getEmail())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

String dacEmail = dac.getEmail();
emailService.sendDatasetDeniedMessage(
user,
dac.getName(),
dataset.getDatasetIdentifier(),
dacEmail);
}
else {
logWarn("Unable to send dataset denied email to DAC: " + dac.getDacId());
hams7504 marked this conversation as resolved.
Show resolved Hide resolved
}
}

}
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/broadinstitute/consent/http/service/EmailService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,9 +268,10 @@ public void sendDatasetApprovedMessage(User user,

public void sendDatasetDeniedMessage(User user,
String dacName,
String datasetName) throws Exception {
String datasetName,
String dacEmail) throws Exception {
Writer template = templateHelper.getDatasetDeniedTemplate(user.getDisplayName(), datasetName,
dacName);
dacName, dacEmail);
Optional<Response> response = sendGridAPI.sendDatasetDeniedMessage(user.getEmail(), template);
saveEmailAndResponse(
response.orElse(null),
Expand Down
2 changes: 1 addition & 1 deletion src/main/resources/freemarker/dataset-approved.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</p>
<p>
Please reach out to <a href="mailto:[email protected]" style="text-decoration: none; font-family: 'Montserrat', sans-serif; color: #00609F;">[email protected]</a> if you have any questions or concerns.
Expand Down
2 changes: 1 addition & 1 deletion src/main/resources/freemarker/dataset-denied.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 your interest in using DUOS to register your dataset(s). Unfortunately, ${dacName} has rejected your ${datasetName} dataset. Based on the feedback your received from this DAC, you can choose to alter your data submission form and re-submit the dataset to ${dacName}. Alternatively, you can submit the dataset to a different DAC for review.
Your dataset, ${datasetName}, submitted to the ${dacName} by ${dataSubmitterName} for management of future data access requests has been <b>rejected</b>. Please contact the DAC directly at ${dacEmail} for questions.
</p>
<p>
Please reach out to <a href="mailto:[email protected]" style="text-decoration: none; font-family: 'Montserrat', sans-serif; color: #00609F;">[email protected]</a> if you have any questions or concerns.
Expand Down
39 changes: 38 additions & 1 deletion src/test/java/org/broadinstitute/consent/http/service/DatasetServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,7 @@ void testApproveDataset_DenyDataset() throws Exception {
dataset.setDacId(3);
Dac dac = new Dac();
dac.setName("DAC NAME");
dac.setEmail("[email protected]");
initService();
when(dacDAO.findById(3)).thenReturn(dac);

Expand All @@ -767,10 +768,46 @@ void testApproveDataset_DenyDataset() throws Exception {
verify(emailService, times(1)).sendDatasetDeniedMessage(
user,
"DAC NAME",
"DUOS-000001"
"DUOS-000001",
"[email protected]"
);
}

@Test
void testApproveDataset_DenyDataset_WithNoDACEmail() throws Exception {
Dataset dataset = new Dataset();
dataset.setDataSetId(1);
User user = new User();
user.setUserId(1);
user.setEmail("[email protected]");
user.setDisplayName("John Doe");
Comment on lines +780 to +783
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Boolean payloadBool = false;
Dataset updatedDataset = new Dataset();
updatedDataset.setDataSetId(1);
updatedDataset.setDacApproval(payloadBool);

when(datasetDAO.findDatasetById(any())).thenReturn(updatedDataset);
dataset.setAlias(1);
dataset.setDacId(3);
Dac dac = new Dac();
dac.setName("DAC NAME");
initService();
when(dacDAO.findById(3)).thenReturn(dac);

Dataset returnedDataset = datasetService.approveDataset(dataset, user, payloadBool);
assertEquals(dataset.getDataSetId(), returnedDataset.getDataSetId());
assertFalse(returnedDataset.getDacApproval());

// do not send denied email
verify(emailService, times(0)).sendDatasetDeniedMessage(
hams7504 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

user,
"DAC NAME",
"DUOS-000001",
""
);

}

@Test
void testSyncDataUseTranslation() {
Dataset ds = new Dataset();
Expand Down
Loading