Skip to content

Commit 4a20b8d

Browse files
vagishavagisha
authored
PanoramaPublic bug fixes (#580)
- Do not send private data reminders for datasets that may have been resubmitted and are pending copy. - Fix NPE in `PrivateDataReminderJob` when a submitted experiment has no associated `announcementId`. - Allow re-submitting data that is already public but not yet published in a peer-reviewed journal e.g., manuscript is on pre-print servers like bioRxiv or medRxiv. - Removed logic that prevented making data public when full publication details were entered at submission time but the data was kept private. - Include experiment title in the private data reminder message - Updated `PanoramaPublicMakePublicTest` to verify that data pending copy cannot be made public by entering the MakePublicAction URL in the browser
1 parent c0197fa commit 4a20b8d

File tree

10 files changed

+155
-34
lines changed

10 files changed

+155
-34
lines changed

‎panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java‎

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6876,6 +6876,11 @@ private ExperimentAnnotations getCopiedExperimentFor(ExperimentAnnotations expAn
68766876
return null;
68776877
}
68786878

6879+
if (_journalSubmission.hasPendingSubmission())
6880+
{
6881+
errors.reject(ERROR_MSG, String.format("There is a pending re-submit request for this experiment on '%s'", _journal.getName()));
6882+
return null;
6883+
}
68796884
if (expAnnot.isJournalCopy())
68806885
{
68816886
if (!_journalSubmission.isLatestExperimentCopy(_expAnnot.getId()))
@@ -7007,12 +7012,10 @@ public void validateCommand(PublicationDetailsForm form, Errors errors)
70077012
return;
70087013
}
70097014
}
7010-
if (!form.hasNewPublicationDetails(_copiedExperiment))
7011-
{
7012-
errors.reject(ERROR_MSG, String.format("Publication details are the same as the ones associated with the data on %s at %s",
7013-
_journal.getName(), _copiedExperiment.getShortUrl().renderShortURL()));
7014-
return;
7015-
}
7015+
// Validation removed: no need to ensure that publication details entered in the form differ from the
7016+
// publication details already associated with the experiment. The user may have entered the
7017+
// correct PubMedID etc. when submitting but forgot to make the data public.
7018+
// (See commit history for the original logic.)
70167019
}
70177020
else if (_copiedExperiment.isPublic())
70187021
{
@@ -7356,13 +7359,6 @@ public boolean hasLinkAndCitation()
73567359
{
73577360
return !(StringUtils.isBlank(_link) || StringUtils.isBlank(_citation));
73587361
}
7359-
7360-
public boolean hasNewPublicationDetails(ExperimentAnnotations copiedExperiment)
7361-
{
7362-
return !(Objects.equals(_pubmedId, copiedExperiment.getPubmedId())
7363-
&& Objects.equals(_link, copiedExperiment.getPublicationLink())
7364-
&& Objects.equals(_citation, copiedExperiment.getCitation()));
7365-
}
73667362
}
73677363

73687364
public static abstract class PanoramaPublicExperimentAction<T extends ExperimentIdForm> extends FormViewAction<T>
@@ -9849,7 +9845,7 @@ private MessageExampleBean createExampleMessage(List<Integer> experimentIds, Pan
98499845
{
98509846
continue;
98519847
}
9852-
Announcement announcement = announcementSvc.getAnnouncement(announcementsContainer, getUser(), submission.getAnnouncementId());
9848+
Announcement announcement = submission.getAnnouncement(announcementSvc, announcementsContainer, getUser());
98539849
if (announcement == null)
98549850
{
98559851
continue; // old data before we started posting submission requests to a message board

‎panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicNotification.java‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -396,14 +396,15 @@ public static String getDataStatusReminderMessage(@NotNull ExperimentAnnotations
396396
message.append("Dear ").append(getUserName(submitter)).append(",").append(NL2)
397397
.append("We are reaching out regarding your data on Panorama Public (").append(shortUrl).append("), which has been private since ")
398398
.append(dateString).append(".")
399-
.append("\n\n**Is the paper associated with this work already published?**")
400-
.append("\n- If yes: Please make your data public by clicking the \"Make Public\" button in your folder or by clicking this link: ")
399+
.append(NL2).append(bold("Title:")).append(" ").append(escape(exptAnnotations.getTitle()))
400+
.append(NL2).append(bold("Is the paper associated with this work already published?"))
401+
.append(NL).append("- If yes: Please make your data public by clicking the \"Make Public\" button in your folder or by clicking this link: ")
401402
.append(bold(link("Make Data Public", makePublicLink)))
402403
.append(". This helps ensure that your valuable research is easily accessible to the community.")
403-
.append("\n- If not: You have a couple of options:")
404-
.append("\n - **Request an Extension** - If your paper is still under review, or you need additional time, please let us know by clicking ")
404+
.append(NL).append("- If not: You have a couple of options:")
405+
.append(NL).append(" - ").append(bold("Request an Extension")).append(" - If your paper is still under review, or you need additional time, please let us know by clicking ")
405406
.append(bold(link("Request Extension", requestExtensionUrl.getURIString()))).append(".")
406-
.append("\n - **Delete from Panorama Public** - If you no longer wish to host your data on Panorama Public, please click ")
407+
.append(NL).append(" - ").append(bold("Delete from Panorama Public")).append(" - If you no longer wish to host your data on Panorama Public, please click ")
407408
.append(bold(link("Request Deletion", requesDeletionUrl.getURIString()))).append(". ")
408409
.append("We will remove your data from Panorama Public.");
409410
if (sourceExperiment != null)
@@ -413,9 +414,9 @@ public static String getDataStatusReminderMessage(@NotNull ExperimentAnnotations
413414
.append(") will remain intact, allowing you to resubmit your data in the future if you wish.");
414415
}
415416

416-
message.append("\n\nIf you have any questions or need further assistance, please do not hesitate to respond to this message by ")
417+
message.append(NL2).append("If you have any questions or need further assistance, please do not hesitate to respond to this message by ")
417418
.append(bold(link("clicking here", respondToMessageUrl.getURIString()))).append(".")
418-
.append("\n\nThank you for sharing your research on Panorama Public. We appreciate your commitment to open science and your contributions to the research community.")
419+
.append(NL2).append("Thank you for sharing your research on Panorama Public. We appreciate your commitment to open science and your contributions to the research community.")
419420
.append(NL2).append("Best regards,")
420421
.append(NL).append(getUserName(journalAdmin));
421422
return message.toString();

‎panoramapublic/src/org/labkey/panoramapublic/model/ExperimentAnnotations.java‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,11 @@ public List<String> getInstruments()
224224
return instruments;
225225
}
226226

227+
public String getInstrumentsCommaSeparated()
228+
{
229+
return StringUtils.join(getInstruments(), ", ");
230+
}
231+
227232
public void setInstrument(String instrument)
228233
{
229234
_instrument = instrument;
@@ -504,12 +509,12 @@ public DataLicense getDataLicense()
504509

505510
/**
506511
* Returns true if the experiment is in an 'Experimental Data' folder that is public and the experiment is
507-
* associated with a published paper.
512+
* associated with a peer-reviewed paper (excludes biorxiv and medrxiv preprint servers).
508513
*/
509514
public boolean isFinal()
510515
{
511516
TargetedMSService.FolderType folderType = TargetedMSService.get().getFolderType(getContainer());
512-
return Experiment.equals(folderType) && isPublic() && isPublished();
517+
return Experiment.equals(folderType) && isPublic() && isPeerReviewed();
513518
}
514519

515520
public boolean hasCompletePublicationInfo()

‎panoramapublic/src/org/labkey/panoramapublic/model/JournalSubmission.java‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22

33
import org.jetbrains.annotations.NotNull;
44
import org.jetbrains.annotations.Nullable;
5+
import org.labkey.api.announcements.api.Announcement;
6+
import org.labkey.api.announcements.api.AnnouncementService;
7+
import org.labkey.api.data.Container;
8+
import org.labkey.api.security.User;
59
import org.labkey.api.view.ShortURLRecord;
610
import org.labkey.panoramapublic.query.SubmissionManager;
711

@@ -69,11 +73,20 @@ public int getModifiedBy()
6973
return _journalExperiment.getModifiedBy();
7074
}
7175

72-
public Integer getAnnouncementId()
76+
public @Nullable Integer getAnnouncementId()
7377
{
7478
return _journalExperiment.getAnnouncementId();
7579
}
7680

81+
public @Nullable Announcement getAnnouncement(@NotNull AnnouncementService announcementSvc,
82+
@NotNull Container announcementsContainer,
83+
@NotNull User user)
84+
{
85+
return (getAnnouncementId() != null)
86+
? announcementSvc.getAnnouncement(announcementsContainer, user, getAnnouncementId())
87+
: null; // old data before we started posting submission requests to a message board
88+
}
89+
7790
public Integer getReviewerId()
7891
{
7992
return _journalExperiment.getReviewer();

‎panoramapublic/src/org/labkey/panoramapublic/pipeline/PostPanoramaPublicMessageJob.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,8 @@ private void postMessage()
103103
continue;
104104
}
105105

106-
Announcement announcement = announcementSvc.getAnnouncement(announcementsContainer, getUser(), submission.getAnnouncementId());
106+
Announcement announcement = submission.getAnnouncement(announcementSvc, announcementsContainer, getUser());
107+
107108
if (announcement == null)
108109
{
109110
getLogger().error("Could not find the message thread for experiment Id: " + experimentAnnotationsId

‎panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,13 @@ private static ReminderDecision getReminderDecision(@NotNull ExperimentAnnotatio
100100
return ReminderDecision.skip("Not the current version of the experiment");
101101
}
102102

103+
// Make sure the data does not have a pending re-submission request
104+
JournalSubmission journalSubmission = SubmissionManager.getSubmissionForJournalCopy(exptAnnotations);
105+
if (journalSubmission != null && journalSubmission.hasPendingSubmission())
106+
{
107+
return ReminderDecision.skip("Data has a pending re-submission request");
108+
}
109+
103110
DatasetStatus datasetStatus = DatasetStatusManager.getForExperiment(exptAnnotations);
104111
if (datasetStatus != null)
105112
{
@@ -244,7 +251,7 @@ private void processExperiment(Integer experimentAnnotationsId, ProcessingContex
244251
}
245252

246253
Container announcementsFolder = context.getAnnouncementsFolder();
247-
Announcement announcement = context.getAnnouncementService().getAnnouncement(announcementsFolder, getUser(), submission.getAnnouncementId());
254+
Announcement announcement = submission.getAnnouncement(context.getAnnouncementService(), context.getAnnouncementsFolder(), getUser());
248255
if (announcement == null)
249256
{
250257
processingResults.addAnnouncementNotFound(experimentAnnotationsId, submission, announcementsFolder);

‎panoramapublic/src/org/labkey/panoramapublic/proteomexchange/PxHtmlWriter.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,7 @@ void writePublicationList(ExperimentAnnotations experimentAnnotations)
300300
HtmlList publicationList = new HtmlList();
301301
if(experimentAnnotations.isPublished())
302302
{
303-
publicationList.addItem("Link: ", experimentAnnotations.getPublicationLink(), false);
303+
publicationList.addItem("Link", experimentAnnotations.getPublicationLink(), false);
304304

305305
if(experimentAnnotations.hasPubmedId())
306306
{

‎panoramapublic/src/org/labkey/panoramapublic/view/expannotations/experimentDetails.jsp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,7 @@
337337
<li><strong>Organism:</strong> <%=h(annot.getOrganismsNoTaxId())%></li>
338338
<%}%>
339339
<%if(annot.getInstrument() != null){%>
340-
<li><strong>Instrument:</strong> <%=h(annot.getInstrument())%></li>
340+
<li><strong>Instrument:</strong> <%=h(annot.getInstrumentsCommaSeparated())%></li>
341341
<%}%>
342342
<%if(annot.getSpikeIn() != null){%>
343343
<li><strong>SpikeIn:</strong>

‎panoramapublic/test/src/org/labkey/test/components/panoramapublic/TargetedMsExperimentWebPart.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,16 @@
22

33
import org.labkey.test.BaseWebDriverTest;
44
import org.labkey.test.Locator;
5+
import org.labkey.test.WebTest;
6+
import org.labkey.test.WebTestHelper;
57
import org.labkey.test.components.BodyWebPart;
68
import org.labkey.test.util.DataRegionTable;
79
import org.openqa.selenium.WebElement;
810

11+
import java.net.MalformedURLException;
12+
import java.net.URL;
13+
import java.util.Map;
14+
915
import static org.junit.Assert.assertNotNull;
1016
import static org.junit.Assert.assertNull;
1117
import static org.junit.Assert.assertTrue;
@@ -83,6 +89,24 @@ public void clickMakePublic()
8389
clickLink(elementCache().makePublicButton, "Expected to see a \"Make Public\" button");
8490
}
8591

92+
public Integer getExperimentAnnotationsId()
93+
{
94+
WebElement moreDetailsLink = elementCache().moreDetailsLink;
95+
if (moreDetailsLink != null)
96+
{
97+
String href = moreDetailsLink.getAttribute("href");
98+
try
99+
{
100+
return Integer.parseInt(WebTestHelper.parseUrlQuery(new URL(href)).get("id"));
101+
}
102+
catch (MalformedURLException e)
103+
{
104+
throw new RuntimeException(e);
105+
}
106+
}
107+
return null;
108+
}
109+
86110
public void clickAddPublication()
87111
{
88112
clickLink(elementCache().addPublicationButton, "Expected to see a \"Add Publication\" button");

‎panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicMakePublicTest.java‎

Lines changed: 80 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,13 @@
1818
import org.openqa.selenium.NoSuchElementException;
1919
import org.openqa.selenium.WebElement;
2020

21+
import java.util.Map;
2122
import java.util.regex.Matcher;
2223
import java.util.regex.Pattern;
2324

2425
import static org.junit.Assert.assertEquals;
2526
import static org.junit.Assert.assertFalse;
27+
import static org.junit.Assert.assertNotNull;
2628
import static org.junit.Assert.assertTrue;
2729

2830
@Category({External.class, MacCossLabModules.class})
@@ -45,24 +47,38 @@ public void testExperimentCopy()
4547
verifyIsPublicColumn(PANORAMA_PUBLIC, experimentTitle, false);
4648
verifyPermissions(projectName, folderName, PANORAMA_PUBLIC, targetFolder);
4749

48-
// Verify that the submitter can make the data public
49-
verifyMakePublic(PANORAMA_PUBLIC, targetFolder, SUBMITTER, true);
5050
// Verify that a folder admin in the source folder, who is not the submitter or lab head will not see the
5151
// "Make Public" button in the Panorama Public copy.
52-
verifyMakePublic(PANORAMA_PUBLIC, targetFolder, ADMIN_2, false);
52+
verifyMakePublicButtonIsNotVisible(PANORAMA_PUBLIC, targetFolder, ADMIN_2,
53+
"Make Public button should not be visible in the Panorama Public copy to a user who is neither submitter nor lab head");
54+
55+
// Verify that the submitter can make the data public
56+
verifyMakePublic(PANORAMA_PUBLIC, targetFolder, SUBMITTER, true);
5357

5458
// Resubmit the folder. This is still possible since the Panorama Public copy is not yet associated with a publication.
5559
resubmitFolder(projectName, folderName, SUBMITTER, true);
5660

61+
// Data copy is pending. Verify that the "Make Public" button in not visible in the source folder or the copied folder
62+
verifyMakePublicButtonIsNotVisible(PANORAMA_PUBLIC, targetFolder, SUBMITTER,
63+
"Make Public button should not be visible in the Panorama Public copy if data copy is pending");
64+
verifyMakePublicButtonIsNotVisible(projectName, folderName, SUBMITTER,
65+
"Make Public button should not be visible in the source folder if data copy is pending");
66+
67+
// Verify that the submitter cannot enter the MakePublicAction URL in the browser to make the data public
68+
verifyCannotMakePublicPendingResubmit(PANORAMA_PUBLIC, targetFolder, SUBMITTER); // In the target folder
69+
verifyCannotMakePublicPendingResubmit(projectName, folderName, SUBMITTER); // In the source folder
70+
5771
// Re-copy the experiment to the Panorama Public project. Do not delete the previous copy
5872
makeCopy(shortAccessUrl, experimentTitle, targetFolder, true, false);
5973

6074
// Verify that the "Make Public button is not visible in the older copy of the data.
6175
String v1Folder = targetFolder + " V.1";
62-
verifyMakePublic(PANORAMA_PUBLIC, v1Folder, SUBMITTER, true);
63-
// Verify that the submitter can make data public, and add publication details
76+
verifyMakePublicButtonIsNotVisible(PANORAMA_PUBLIC, v1Folder, SUBMITTER,
77+
"Make Public button should not be visible in an older copy of the data");
78+
verifyCannotMakePublicOldCopy(PANORAMA_PUBLIC, v1Folder, SUBMITTER);
79+
80+
// Verify that the submitter can make the latest copy of the data public, and add publication details
6481
verifyMakePublic(PANORAMA_PUBLIC, targetFolder, SUBMITTER, true, true);
65-
verifyMakePublic(PANORAMA_PUBLIC, v1Folder, ADMIN_2, false);
6682

6783
verifyIsPublicColumn(PANORAMA_PUBLIC, experimentTitle, true);
6884

@@ -281,6 +297,64 @@ private void verifyMakePublic(String projectName, String folderName, String user
281297
stopImpersonating();
282298
}
283299

300+
private void verifyMakePublicButtonVisible(boolean expectVisible, String projectName, String folderName, String user, String errorMessage)
301+
{
302+
if (isImpersonating())
303+
{
304+
stopImpersonating(true);
305+
}
306+
goToProjectFolder(projectName, folderName);
307+
impersonate(user);
308+
goToDashboard();
309+
TargetedMsExperimentWebPart expWebPart = new TargetedMsExperimentWebPart(this);
310+
311+
if (expectVisible)
312+
{
313+
assertTrue(errorMessage, expWebPart.hasMakePublicButton());
314+
}
315+
else
316+
{
317+
assertFalse(errorMessage, expWebPart.hasMakePublicButton());
318+
}
319+
}
320+
321+
private void verifyMakePublicButtonIsVisible(String projectName, String folderName, String user, String errorMessage)
322+
{
323+
verifyMakePublicButtonVisible(true, projectName, folderName, user, errorMessage);
324+
}
325+
326+
private void verifyMakePublicButtonIsNotVisible(String projectName, String folderName, String user, String errorMessage)
327+
{
328+
verifyMakePublicButtonVisible(false, projectName, folderName, user, errorMessage);
329+
}
330+
331+
private void verifyCannotMakePublicOldCopy(String projectName, String folderName, String user)
332+
{
333+
verifyCannotMakePublic(projectName, folderName, user, "not the most recent copy of the data");
334+
}
335+
336+
private void verifyCannotMakePublicPendingResubmit(String projectName, String folderName, String user)
337+
{
338+
verifyCannotMakePublic(projectName, folderName, user, "There is a pending re-submit request for this experiment");
339+
}
340+
341+
private void verifyCannotMakePublic(String projectName, String folderName, String user, String expectedMessage)
342+
{
343+
if (isImpersonating())
344+
{
345+
stopImpersonating(true);
346+
}
347+
goToProjectFolder(projectName, folderName);
348+
impersonate(user);
349+
goToDashboard();
350+
TargetedMsExperimentWebPart expWebPart = new TargetedMsExperimentWebPart(this);
351+
Integer expAnnotationsId = expWebPart.getExperimentAnnotationsId();
352+
assertNotNull(expAnnotationsId);
353+
beginAt(WebTestHelper.buildURL("panoramapublic", getCurrentContainerPath(), "makePublic", Map.of("id", expAnnotationsId)));
354+
waitForText(expectedMessage);
355+
}
356+
357+
284358
private void makeDataPublic()
285359
{
286360
makeDataPublic(true);

0 commit comments

Comments
 (0)