Send reminders to submitters of private data on Panorama Public #566
Conversation
…r job can be turned on via the admin console
- Made SendPrivateDataRemindersAction a FormViewAction - DatasetStatus columns can be accessed via the ExperimentAnnotations table - Added constructor to pass the Journal (e.g. Panorama Public) and a list of experiments to PrivateDataReminderJob
- "Panorama Public" journal folder can be selected in a drop-down.
…cript. - Fixed RequestExtensionAction and RequestDeletionAction. - Bumped schema cersion in PanoramaPublicModule. - PrivateDataReminderJob skips experiments that should be be getting reminders. - Fixed column name in DatasetStatusTableInfo. - Added more testing in PrivateDataReminderTest.
…tildes (e.g., ~strikethrough~) is rendered as strikethrough.
- Some cleanup.
- Moved date calculations to PrivateDateReminderSettings. Added unit tests - Improvements to PrivateDataReminderJob - Fixed PrivateDataReminderTest
labkey-jeckels
left a comment
labkey-jeckels
left a comment
There was a problem hiding this comment.
This is a big PR. I reviewed the code, but let me know if there are specific areas where you want extra attention. Also, I didn't do any testing on the work. Let me know if you're wanting help on that.
| } | ||
| } | ||
|
|
||
| public static class DatasetStatusColumn extends DataColumn |
There was a problem hiding this comment.
I suggest overriding isSortable() and isFilterable() to return false. It tends to be confusing when you're sorting/filtering on a value different from the one being rendered.
| { | ||
| errors.reject(ERROR_MSG, "Please enter a value for 'Reminder frequency'."); | ||
| } | ||
| else if (form.getReminderFrequency() < 0) |
There was a problem hiding this comment.
Does 0 make sense here, or should this ensure that the value >= 1?
There was a problem hiding this comment.
Does 0 make sense here, or should this ensure that the value >= 1?
0 is allowed to support automated testing in PrivateDataReminderTest. In practice on Panorama Public, we wouldn’t set it below 1 unless we intentionally want reminders to start immediately after submission. I’ve added clarifying notes on the form to make this behavior clear.
panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java
Outdated
Show resolved
Hide resolved
| HtmlView view = new HtmlView(DIV( | ||
| DIV(getConfirmViewMessage()), | ||
| DIV("Title: " + _exptAnnotations.getTitle()), | ||
| DIV("Submitted on: " + DateUtil.formatDateTime(_exptAnnotations.getCreated(), "MMMM d, yyyy")), |
There was a problem hiding this comment.
This date format is hard-coded in a number of places. Extract as a constant?
| } | ||
| else | ||
| { | ||
| messageBody.append("We were unable to locate the source folder for this data in your project. ") |
There was a problem hiding this comment.
Will this be clear enough to the recipient? Do we want to encourage them to check and delete if appropriate, as perhaps it was renamed or moved? Or is the assumption that they should keep it around?
There was a problem hiding this comment.
Will this be clear enough to the recipient? Do we want to encourage them to check and delete if appropriate, as perhaps it was renamed or moved? Or is the assumption that they should keep it around?
The message should only appear if the source folder has actually been deleted (or the corresponding row in the experimentannotations table was removed). If the folder was simply moved or renamed, we can still locate it through the container column in experimentannotations.
I haven’t encountered the deletion case yet, but if it happens, the message is probably OK. Also, if the user is requesting deletion of their data on Panorama Public, the data in the source folder is likely no longer important to them.
I am open to ideas on improving the message to make it clearer to the data submitter.
- Use table name constant and date format string constant. - isSortable and isFilterable return false for DatasetStatusColumn. - Added clarifying text for form input elements in privateDataRemindersSettingsForm.jsp
Thanks for the review! If you could have another look at I have a added an automated test -
If you are able to do additional testing, that would be very welcome. |
| catch(Exception e) | ||
| { | ||
| _log.error("Error queuing PrivateDataReminderJob", e); | ||
| ExceptionUtil.logExceptionToMothership(null, e); |
There was a problem hiding this comment.
You're already logging the exception to the local log file, so no need for ExceptionUtil to log it again.
| ExceptionUtil.logExceptionToMothership(null, e); | |
| ExceptionUtil.logExceptionToMothership(null, e, false); |
|
I did a little manual testing, largely based on the setup done in PrivateDataReminderTest. I wasn't able to get a manually created submission to send reminders. It got skipped because it didn't think that it was the latest version. I didn't have all the data needed for a ProteomeExchange submission so I skipped that. Is that why the version was null? Should the reminder code fire on submissions in that state? |
- Added an option in the Private Date Reminder Setting form to enter a time for reminder jobs to run.
…of RequestDeletionAction. - Fixed SQLException by updating DatasetStatus handling: - Delete row in DatasetStatus when data is resubmitted and a new Panorama Public copy is created. - Delete row in DatasetStatus when the Panorama Public copy is deleted. - Prevents SQLException from shortUrl FK constraint when deleting Panorama Public copy followed by source data folder, or vice versa. - Cleanup user accounts in PrivateDataReminderTest.
…tationsId. DatasetStatus is associated with a particular copy of the data. If a new copy is made, the data status resets. Status associated with the previous copy is no longer relevant.
Thanks for testing this! All experiments copied to a Panorama Public project using the data copy pipeline job should have a The reminder job iterates over all private datasets (or the ones you select if running manually), and posts reminders only for those that meet the conditions in Could you please try this workflow for testing manually:
NOTE: I committed some changes:
|
Sorry it took me a while to respond. I followed those test steps and everything looked fine when I worked from the folders that that test set up for me. Let me know if you still want to find a time to connect on what went wrong in my original test pass. I imagine I did something wrong in submitting the data, but I'm not sure what. |
|
Thanks of testing. It will be good to understand what went wrong in your initial tests. I will follow over email to schedule a time. |
Rationale
Submitters often forget to make their data public even after the associated paper is published. We should send them reminders periodically.
Changes
strikethrough.