Dataverse: Add ability to "Return to Author" via API using JSON, read by author via email

[pameyer]

Highly related to #3702; but separate in the spirit of allowing the API to do anything the UI can do (and to allow development prior to UI development):

1. User submits dataset for review [already exists]
2. Reviewer/script identifies problems, uses API to return dataset to author w\ message "please fix problem $x we have identified" [does not exist yet - that's this issue]
3. User fixes problems, submits for review, no problems identified, dataset gets published [already exists].

[djbrooke]

• Where to store message?
• How long to store message (should we keep the feedback forever)?
• How do we handle free text - we don't have other notifications that have free text
• Should handle multiple reviewers/authors

[pdurbin]

1684 by @mercecrosas says, "For some features that generate a notification (such as clicking "Return to Author" in a dataset in review, or assigning a role to a user), we should allow to add a custom note that the user gets with the notification." There's the potential for consolidating these issues into one.

[stevenmce]

@pdurbin @scolapasta Australian Data Archive has a major interest in this requirement for custom notes - particularly to be useable across multiple notifications.

We would use this in several notifications if available:

• Return to authors for review
• refusal of a request for access for restricted datasets
• review of a restricted access request (e.g. I need more info on INSERTPROBLEMHERE).
• referral of a request to another user (another function we would like to see - I'll add an issue on this to GitHub)

We would be happy to contribute to the development effort here - we intend to work on developing this functionality ourselves in Q3 2017 anyhow, as we need it for extending our restricted access customisation.

Cheers, Steve

[pdurbin]

@stevenmce cool. Can you please \@mention the developer(s) who would be interested in helping? Please be advised that the scope of this issue is to build the backend, accessible via API only. The note will be put in JSON format and sent at a new API endpoint. In the future we'll be working on the front end so that submitting a note is possible from the Dataverse GUI (I assume we'll use #3702 to work on the frontend and you are welcome to leave a comment there to explain your requirements).

[pdurbin]

@scolapasta @pameyer I just made pull request #3962 to demonstrating how I'd like to test this feature with REST Assured and stubbed out some of the methods I'll need. Unfortunately, both "Submit for Review" and "Return to Author" are GUI-only features right now (#3440). Feedback from you, @stevenmce 's developers, and others is welcome but I'll keep coding away.

[pameyer]

@pdurbin :

• Do you think it makes sense to test the author "submit for review" -> curator "publishes" path here as well?
• RequestAccessIT.java confused me for a moment, until I realized it was access to deposit/publish (not request download access)
• Would it help to have scripts to test GUI "submit for review" and "return to author"? I'd guess probably not (more effort to integration into current test framework than it's worth for this).

[pdurbin]

@pameyer yeah, in the tests I wrote the author doesn't have permission to publish, which is my understanding of how you plan to run your installation of Dataverse, so I was planning on having the curator publish the dataset after the author responds to the comments made by the curator during review. Thanks for noticing that I botched the name of the test class. Will fix!

No, I don't plan on including any browser-based tests for this issue but I have recently listed browser-based tests under "Future Work" at http://guides.dataverse.org/en/4.7/developers/testing.html#future-work .

[pdurbin]

@pameyer I fixed the name of the test file in d1da0d5. Thanks for spotting that.

Also, this issue is highly related: Submit for Review workflow via API #3144

[pdurbin]

In 02fb4d1 I added notifications to the new submitForReview and returnToAuthor endpoints so I could see what the notifications look like in the GUI as of Dataverse 4.7. That is to say, I'm capturing screenshots of how things look in production before we make any changes to the UI. I believe that UI changes are out of scope for this issue and that they'll be worked on in #3702.

Here are the screenshots. (Sorry for the ugly names but I use these to make objects unique in API tests.) Keep in mind that an dataset can have multiple authors/contributors and multiple curators:

What the curators see when an author/contributor clicks "Submit for Review"

What the authors/contributors see when one of the curators clicks "Return to Author"

[pdurbin]

I chatted with @pameyer in Slack and we agree that the curator's comments need to be persisted somewhere, and I picked a place in 78de1cb mostly to show the places where the code might need to change. I'm certainly open to changing how we persist things.

@pameyer seemed fine with me adding an API endpoint for users to read their notifications, which I stubbed out in 55bf5ba . Right now you can only see the robotic looking "type" of notification (CREATEACC, RETURNEDDS, etc.) and the comment the curator submitted using JSON when calling the new returnToAuthor API endpoint.

I'm not married to this approach so I'm putting this issue in Code Review at https://waffle.io/IQSS/dataverse (pull request #3962) but I hope that @pameyer agrees that the fundamental requirements for this issue have been met. (In sprint planning we gave this issue an 8 so I expect a certain amount of scope creep.) I tried to write my API tests like a user story that goes like this:

• curator creates dataverse
• author immediately tries to create dataset (fails)
• curator give author access to create dataset but not publish
• author creates dataset
• author tries to publish (fails, the curator has chosen permissions that only allow the curator to publish)
• author does the equivalent of clicking "Submit for Review" via API
• author checks for comments (no comments yet)
• curator leaves a comment ("You forgot to upload your files")
• author checks for comments and knows what to fix to make the curator happy

Here's the API test file I'm talking about with the user story above:

https://github.com/IQSS/dataverse/blob/78de1cb3d363d84f80cecfa52bdf8ee6333b48be/src/test/java/edu/harvard/iq/dataverse/api/InReviewWorkflowIT.java

[pameyer]

@pdurbin What do you think about adding three steps to your user story (author adds files, re-submits, curator publishes)?

This user story reminds the that there's a closely related one (author submits for review, curator sends messages "yeah, it's good", curator later publishes) - but that may be out of scope for this one.

[pdurbin]

@pameyer sure I can add those steps in the tests and yes the curator is welcome to sit on the dataset for a while and publish datasets only on Wednesdays or whatever.

Today I had a fantastic conversation with @mheppler and then did a brain dump on @dlmurphy while drawing on his whiteboard:

The important points:

• Communication is one way. The curator asserts what's missing or wrong. "You forgot to upload your files." For now I'm thinking of this as "reason for rejection". Maybe that's too harsh. I don't know. (The curator in the drawing is grumpy-looking and absolutely is not intended to represent @pameyer .) The curator isn't supposed to ask question like, "Is there a third author?" because the author can't reply. We are not building chat within Dataverse. If the curator needs to chat with the author, use a chat application for goodness sake. Or email. The author is receiving instructions or requirements or advice from the curator, not questions.
• Down the road, in the UI (#3702 probably), we'll need to figure how how the author receives the message. The consensus so far is that the comments from the curator appears on the dataset page itself. That way the author can see it, co-authors can see it. Other curators can see it. It's obvious that the dataset hasn't been published because the author needs to take corrective action. The comment is not deleted until the dataset is published.
• "Reason for rejection" or whatever we call it is a required field. Making it option means zero progress. Without any reason why the author has to attempt to read the mind of the curator. No fun.
• In pull request #3962 as of 78de1cb I'm storing the comments from the curator in the notification table. This is a bad idea because if the author clicks the "x" on the notification, the notification is deleted along with the comment. What if the curator wrote five paragraphs? The comment needs to be stored elsewhere.
• There can only be one comment at a time and we aren't going to persist a history of comments. The author needs to live in the now and take corrective action based on the comment that's in from of them. No digging though past comments. There's only ever one comment to deal with, even if the dataset ended up going back and forth between author and curator a few times before publication. If you need to see an old comment, hold on to all the emails that are sent because each time the curator clicks "Reason for rejection" (or whatever) an email will be sent. I'm sort of wondering if the curator should be cc'ed on this email. I guess not. Just send it to the author.
• @dlmurphy needs a new black whiteboard marker

I'm sure I have a few open questions but this is enough of an update for now. @stevenmce and @amberleahey if you have any thoughts on this, please don't hold back.

[pdurbin]

@pameyer I just changed the title of this issue to 'Add ability to "Return to Author" via API using JSON, read by author via email'. If this makes no sense or is unwise, please let me know!

[pdurbin]

I just posted 'Help Wanted: stories related to "Submit for Review" and "Return to Author"' to https://groups.google.com/d/msg/dataverse-community/bGlCU2pbQpE/h3d9enX9AAAJ encouraging the community to join the conversation.

[stevenmce]

@pdurbin I like where this is going - this is an important workflow for an archive like ADA where the curator often makes a number of edits and modifications prior to publishing, in conjunction with the author. I'm going to digest this over the next couple of days and respond further

[stevenmce]

@pdurbin I've just mentioned a related user story for this type of workflow - approving requests for access to Restricted Access data - on the Google Groups discussion at https://groups.google.com/d/msg/dataverse-community/bGlCU2pbQpE/GlPo33dWBAAJ

@mdmADA you may be interested in this for ADA's Restricted Access workflow.

[stevenmce]

Comments on the "Important points" whiteboard discussion from @pdurbin above:

_a) Communication is one way. The curator asserts what's missing or wrong. "You forgot to upload your files." For now I'm thinking of this as "reason for rejection". Maybe that's too harsh. I don't know. (The curator in the drawing is grumpy-looking and absolutely is not intended to represent @pameyer .) The curator isn't supposed to ask question like, "Is there a third author?" because the author can't reply. We are not building chat within Dataverse. If the curator needs to chat with the author, use a chat application for goodness sake. Or email. The author is receiving instructions or requirements or advice from the curator, not questions._

@stevenmce Comment:

From the archive's point of view, we DO want to capture these comms in an effective way. I agree we don't want live chat, but we do liaise with the author via email. Embedding this in the notifications would actually be quite useful for us (particularly where those notifications are preserved). That way they can be captured and maintained for the record later.

Also, is there a reason the author couldn't/shouldn't respond? The author sends a "new" version, and the reviewer responds with their reasons for accept/reject.

_b) Down the road, in the UI (#3702 probably), we'll need to figure how how the author receives the message. The consensus so far is that the comments from the curator appears on the dataset page itself. That way the author can see it, co-authors can see it. Other curators can see it. It's obvious that the dataset hasn't been published because the author needs to take corrective action. The comment is not deleted until the dataset is published.
"Reason for rejection" or whatever we call it is a required field. Making it option means zero progress. Without any reason why the author has to attempt to read the mind of the curator. No fun._

@stevenmce Comment:

agreed - good approach

_c) In pull request #3962 as of 78de1cb I'm storing the comments from the curator in the notification table. This is a bad idea because if the author clicks the "x" on the notification, the notification is deleted along with the comment. What if the curator wrote five paragraphs? The comment needs to be stored elsewhere._

@stevenmce Comment:

This is nice - good approach. Definitely don't want to comments deleted accidentally - and I'm interested in what those 5 paragraphs say.

_d) There can only be one comment at a time and we aren't going to persist a history of comments. The author needs to live in the now and take corrective action based on the comment that's in from of them. No digging though past comments. There's only ever one comment to deal with, even if the dataset ended up going back and forth between author and curator a few times before publication. If you need to see an old comment, hold on to all the emails that are sent because each time the curator clicks "Reason for rejection" (or whatever) an email will be sent. I'm sort of wondering if the curator should be cc'ed on this email. I guess not. Just send it to the author._

@stevenmce Comment:

No, I don't like this. See my comments above on why we would want to preserve the comments. They are an important part of the archival record. You could think of them as similar to a git commit message - https://chris.beams.io/posts/git-commit/ . The current approach is from the reviewer to the author, but actually a comment back from the author on the next submitted version would also be very helpful - "here's what we changed to address your issues".

[pdurbin]

In 67a7dfc I moved the "reason for return" from the usernotification table to the datasetversion table. This means the five paragraphs the curator writes can't be so easily deleted by the author.

I'm a little puzzled by why @stevenmce is so interested in keeping every single comment when a dataset goes back and forth a dozen times. Shouldn't the curator just summarize the tennis match in a note as he's hitting "Publish"? @stevenmce I might be asking you to create a GitHub issue for future work depending on the design decision we make about "water under the bridge" for the scope of this issue. The main value add we're adding for this issue is that the author will be able to check her email and see the five paragraphs that the curator wrote rather than having to read the curator's mind. 😄

I'm also still leaning toward the author not being able to write back from Dataverse. She still just clicks "Submit for Review". It would be easy to change the button to "Resubmit for Review" on subsequent go arounds.

Please keep up the chatter. I want to write more but I'm suppose to watch some fireworks tonight. 😄

[stevenmce]

Hi @pdurbin. Thanks for prompting me on this. The use case for us retaining all comments is two-fold:

1. It pertains to the type of data we support - and in particular data deposited by government departments. They often want an audit trail of the process - hence the retention of all comments.

2. As an archive, we will also often curate the data further, and may actually send it back to the author again for further review of changes the archive has made. As such, having the author be able to send comments in their submission would be advantageous for us.

I'd be happy to put together a use case document on this, and also our Restricted Access workflow. Let me know if this would help.

And in the meantime, enjoy the fireworks. I once spent 4th of July canoeing down the Charles River to watch the fireworks - it's a great tradition you have there!!

[amberleahey]

I agree with Steve on this, but I’m not sure if the messages are what you’d necessarily want to include alongside versioning information for public view, however I can see it as useful way to collect documentation about versioning and review processes to support reuse or audit trail.

Another though, if wording is added to the commenting box that states “this information will be provided to the data curator for review and will be added to the dataset’s versioning notes for documentation purposes” – would that suffice? I wouldn’t want to unknowingly store or publish information without consent. This information could be presented in context associated with versions.

[pdurbin]

As such, having the author be able to send comments in their submission would be advantageous for us. -- @stevenmce

the commenting box that states “this information will be provided to the data curator for review -- @amberleahey

Wait, what happened to one way communication from the curator to the author? You both want us to build chat? :smile: I'm worried about scope creep.

[amberleahey]

It could be one way, but it could also be more complicated than that. Maybe there is an issue with doing with what the curator/reviewer says, the data manager / PI may have some comments, feedback, explanations, etc.. I don't think it should be thought of as "chat" but could be time stamped version review notes? It does sound a little like scope creep now, but I think if we are really interested in documenting data management these are the realities.

[pdurbin]

This just in from @tlchristian at https://groups.google.com/d/msg/dataverse-community/X1fWTiX4-74/4m3w5gJNAgAJ ... I keep whiteboarding with two personas, author and curator, but she brings up a third, a journal editor. The plot thickens! She writes:

I would love to discuss this further as well. The tennis analogy is certainly apt for our use case. I would love to be able to customize the email messages to include specific information we need to convey to the authors. Also, getting the editor in the communication mix would solve some of our workflow inefficiencies, which is another reason for customized email messages. Being able to manage communications among author, editor, and data curator within the Dataverse system would significantly streamline the workflow.

I'm getting the distinct impression that rather than a bunch of emails on the side between author, curator, and journal editor, @tlchristian @amberleahey and @stevenmce envision a future where these messages are captured within Dataverse. That's a fine vision but may be out of scope for this issue.

Maybe folks have seen this cartoon of going from a skateboard to a car:

In this issue I'm going to try to bring everybody from the skateboard to a scooter or bicycle. You won't get a car. That said, please keep telling me if you want leather seats in your car or not.

Another thought. Above @stevenmce said,

They often want an audit trail of the process - hence the retention of all comments.

This reminds me of the actionlogrecord database table. It's sort of mean to be ephemeral, but if I refactor the code into "commands" like I'd like to, there would be an entry for each SubmitForReviewCommand and each ReturnToAuthorCommand. Of course, there still seems to be some controversy over whether authors get a box to type into or not (in the future when we build a UI). For more on the actionlogrecord database table, please see http://guides.dataverse.org/en/4.7/installation/administration.html#monitoring

@amberleahey I don't see any mention of authors being able to write comments in #3702 so if you and @stevenmce and @tlchristian want to leave a comment there (don't forget journal editors too), it might be helpful. Or we can keep talking here but we are not building a GUI in this issue. The user experience changes for the author in the sense that the email contains the five paragraphs that the curator writes but that's about it for now. Again, a scooter, not a car. 😄

Oh, I'm also conscious of the fact that I mentioned "water under the bridge" above but didn't explain what I mean. What I was trying to say is that for this issue I'm leaning toward saying, "Only one 'reason for return' from the curator is stored at a time (in the datasetversion table). If the dataset goes back and forth a dozen times, we only have the most recent 'reason for return' in the database. And when the dataset is finally published, we delete the 'reason for return'. It's water under the bridge. No one cares about those dozen back and forths now that the dataset is published. It's time to celebrate. Maybe the curator can summarize the back and forth into a note that appears in the DDI." Again, for this issue, that's what I'm thinking.

When a dataset is in the "draft" state, we are continually overwriting it with each edit. There was an idea floated or recording every single edit while in draft at #1002 (yes, @stevenmce this would be more like git) but we don't do this now and I can't see us changing this in the near future. It's too much work.

I hope this brain dump helps explain my thinking. I'm not trying to squelch anyone. Please keep the ideas coming and do consider creating a fresh GitHub issue with your requirements for when we build you a car. 😄

[stevenmce]

@pdurbin - there is definitely scope creep here. I think we are basically piling on some of our own brain dumps (I know I am :-). As you note, separating some of these requirements out would be helpful. i might look at #3702 and other issues and see where is the best place to put some of these thoughts.

[bjonnh]

I got a review from one of our users (PI).

This person was asking about the ability to have more options than accept/reject. Including the ability to add new contributors, this person also asked that this inclusion of new contributors could be made by both the initial author and the the curator. From what I understood, the idea was that when major changes are requested (adding new data), some other people could be asked to get involved.

I think that the addition of other contributors by the author is a separate issue though.

[pdurbin]

To summarize the conversation in our our sprint planning meeting just now:

• We're building this "Add ability to 'Return to Author' via API using JSON, read by author via email and in-app notification" feature primary for @sbgrid (courtesy of @pameyer) but we are still very interested in input from the community! Please keep up the chatter here and on the Help Wanted: stories related to "Submit for Review" and "Return to Author" thread @stevenmce @amberleahey @philippconzett @adam3smith @tlchristian @bjonnh and others!
• Today I spoke with @sbarbosadataverse about issues #2526 #2128 and an apparently untracked issue about the "Submit for Review" button not going away. I'm hoping to fix all these bugs while I'm in this code but no promises.
• There are still lots of open design questions.
• We decided to add the "reason for return" to the notification, implemented in ddca20d and the screenshot below, vetted by @pameyer .

[pdurbin]

We decided to add the "reason for return" to the notification

At standup this morning we heard some concerns about showing too many characters in an in-app notification. For example, a thousand characters doesn't look great:

@pameyer how many characters to do you need when typing the "reason for return"? 1000? 255? 140? Please advise. For the scope of this issue, we're pretty sure we'd like to slap a limit on it. Thanks!

Also @sekmiller is refactoring the code I copied and pasted into common business logic (command pattern). We're going to make the "reason for return" a required field when using the API. When we build a proper UI, we'll make the reason for return required in the UI as well.

[pameyer]

@pdurbin are these size limits for display or storage? 200 characters should be enough (short description and link to more detailed description if necessary).

FYI - that screenshot looks reasonable to me; although it makes me wonder how line breaks are handled in notification display.

[pdurbin]

@pameyer storage. To limit to 255 characters, we would add a constraint in the database like this:

ALTER TABLE datasetversion ADD COLUMN returnReason character varying(255) default NULL

Line breaks are not preserved in the in-app notification but they are preserved in the email. Here's how both look so you can compare:

in-app notification

email notification

Subject: Root: Your dataset has been returned

Hello,
Darwin's Finches (view at http://localhost:8080/dataset.xhtml?persistentId=doi:10.5072/FK2/ZQHGNG&version=DRAFT&faces-redirect=true) was returned by the curator of dv4c0410f0 (view at http://localhost:8080/dataverse/dv4c0410f0) with a return reason of "Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor. Aenean massa.

Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim.

Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu. In enim justo, rhoncus ut, imperdiet a, venenatis vitae, justo. Nullam dictum felis eu pede mollis pretium. Integer tincidunt.

Cras dapibus. Vivamus elementum semper nisi. Aenean vulputate eleifend tellus. Aenean leo ligula, porttitor eu, consequat vitae, eleifend ac, enim. Aliquam lorem ante, dapibus in, viverra quis, feugiat a, tellus.

Phasellus viverra nulla ut metus varius laoreet. Quisque rutrum. Aenean imperdiet. Etiam ultricies nisi vel augue. Curabitur ullamcorper ultricies nisi. Nam eget dui. Etiam rhoncus. Maecenas tempus, tellus eget condimentum rhoncus, sem quam semper libero, sit amet adipiscing sem neque sed foo."

Thank you,
Root

[pdurbin]

I just moved this issue and pull request #3962 to Code Review at https://waffle.io/IQSS/dataverse

Again, we are delivering value in small chunks. We're trying to go from a skateboard to a scooter or bicycle rather than going from a skateboard to a car. As of 8f30b45 in pull request #3962 this is the value we're delivering:

• The curator can specify a "reason for return" via API. This is documented at https://github.com/IQSS/dataverse/blob/8f30b455288386c8ea24c44fb93a7237029c4ce0/doc/sphinx-guides/source/api/native-api.rst#return-to-author
• The author and the curator can see the reason for return in notifications both in the form of email messages and in-app notifications. Because we are showing the "reason for return" in the in-app notification, we limit the the reason for return to 200 characters.
• Authors who are API users can "Submit for Review" via API. This is documented at https://github.com/IQSS/dataverse/blob/8f30b455288386c8ea24c44fb93a7237029c4ce0/doc/sphinx-guides/source/api/native-api.rst#submit-for-review
• All users can check their notifications via API. This is documented at https://github.com/IQSS/dataverse/blob/8f30b455288386c8ea24c44fb93a7237029c4ce0/doc/sphinx-guides/source/api/native-api.rst#notifications
• This bug has been fixed: Dataset In Review Notifications - Admin not notified if dataset is "resubmitted" #2526
• This bug has a been addressed: dataset has been submitted for review: change text with automated message #2128

Personally, I think we should go ahead and let the curator type the "reason for return" in a box for this issue. Code for the box was already in the app as of Dataverse 4.0 and it was cleaned up in 152a772 and then removed again in 728562b. When we add the box for good, we'll just need to adjust the validation a bit to centralize the rule that the "reason for return" is required in the new ReturnDatasetToAuthorCommand.

[pdurbin]

Oh, I'm also conscious of the fact that I mentioned "water under the bridge" above but didn't explain what I mean. What I was trying to say is that for this issue I'm leaning toward saying, "Only one 'reason for return' from the curator is stored at a time (in the datasetversion table). If the dataset goes back and forth a dozen times, we only have the most recent 'reason for return' in the database. And when the dataset is finally published, we delete the 'reason for return'. It's water under the bridge. No one cares about those dozen back and forths now that the dataset is published. It's time to celebrate. Maybe the curator can summarize the back and forth into a note that appears in the DDI." Again, for this issue, that's what I'm thinking.

Pull request #3962 (the 3943-return-to-author-with-explanation branch) represents the "water under the bridge" design decision described above where the PublishDatasetCommand sets the "reason for return" to null on publish, effectively blanking it out. I believe this is what @pameyer wants.

In pull request #3992 (the 3943-return-to-author-new-table branch) we are playing around with the idea of not deleting the "reason for return" on publish. @pameyer would you be ok with this?

[sekmiller]

So the comment stored in the new table is associated with a Dataset Version, but not the notification?

[pdurbin]

@sekmiller users should continue to be free to delete notifications at will so I'm reluctant to add a foreign key to a notification id. If you delete the notification, the "reason for return" should not be deleted. I don't know if this helps or not, but here's what's on my whiteboard:

I think the real solution is to display the "reason for return" not in notifications but on the dataset page itself. This has UI implications, however, and we're trying to minimize UI impact in this issue. Reason for displaying the "reason for return" on the dataset page include:

• The "reason for return" can be unlimited. It'll have room to breathe. Because there is so little room in the notifications list, we are currently limiting the "reason for return" to 200 characters.
• All contributors and all curators of the dataset can see the reason for return in a central place, the dataset page. All these folks would be free to delete their notifications because they would know they can still see the "reason for return" on the dataset page.

[pameyer]

@pdurbin persisting "reason for return" after publication isn't strictly necessary; but it's also not necessary to go out of the way to remove it after publication (aka - if it sticks around after publication, that's not a problem. having it visible by default after publication is another question though).

[sekmiller]

I thought the whole point of the separate table was to extend the functionality of adding comments to other types of notifications. There is probably a way to make the join to notification and/or dataset version nullable so that a comment could have a link out to one or the other or both.
Also, it seems like the save comment should be called in the Return to Author command.
And when we return the adding comments capability to the UI do we need to display any prior comments in the pop-up?

[adam3smith]

I realize this is mainly @pameyer 's scooter (or skateboard?) at this point, but FWIW, we'd prefer for the curation notes to remain stored after publish -- audit trail and all. I would assume that the other teams who wanted this for more traditional curation such as @stevenmce would agree.

[scolapasta]

@adam3smith (cc: @stevenmce) agreed and the technical approach I gave in my review was to store them. We won't necessarily have the capability yet to view them anywhere except in the db, but we will have them stored for when we do.

@sekmiller you are correct - and the recommended way I had discussed with @pdurbin was to have both the foreign keys (and notification_id to be nullable, so that the notification can be deleted without losing the comment). The approach done by @pdurbin was proffered as a simpler solution to at least solve the "store all comments" functionality.

I think we can probably get away with this for now, knowing that we'll beed to revisit either a) when we add some UI for this or b) when we add these types of comments for other notifications.

(the reason it may be a) is that this solution has one issue - which was also true of the only storing one comment solution) and that is all notifications will have to show the latest comment, even if the notification was created with an earlier comment in mine. This may not matter much, but we should definitely at least ping the UI team about the display of notifications when we add the UI component to this.

[pdurbin]

Thanks, all. Lots of people are expressing their needs in terms of an audit trail so I'm playing around with this idea in e57acda after I drew the following noise on my whiteboard while talking to @sekmiller 😄

[pdurbin]

Ok, I've attempted to organize all that noise on my whiteboard into a spreadsheet that I enabled public comments on: https://docs.google.com/spreadsheets/d/1jRNe6WfGwOCZYRBthI7matgmG38pgSDycn7yTaqEq1o/edit?usp=sharing

Here's a screenshot of what it looks like right now:

I recommend clicking the screenshot or the spreadsheet to actually read it but here a dump in YAML format of its contents:

'Submit [dataset version] for Review':
  Workflow: In Review/Publication
  Item being discussed: dataset version
  Role performing the action: Contributor
  What is being asked for: publication of a dataset version
  Allow edited version in GUI: maybe
  Back and forth expected: yes
  Multiple people may receive notification: yes
  Persist comment forever: 'yes, in the future'
  'Required, optional or disallowed': required on resubmit?
  Send additional emails on edit: no
  Table store comment in: publicationauditentry
'Return [dataset version] to Author':
  Workflow: In Review/Publication
  Item being discussed: dataset version
  Role performing the action: Curator or Admin
  What is being asked for: revisions to a dataset version
  Allow edited version in GUI: maybe
  Back and forth expected: yes
  Multiple people may receive notification: yes
  Persist comment forever: yes
  'Required, optional or disallowed': required
  Send additional emails on edit: no
  Table store comment in: publicationauditentry
'Publish [dataset version]':
  Workflow: In Review/Publication
  Item being discussed: dataset version
  Role performing the action: Curator or Admin
  What is being asked for: nothing
  Allow edited version in GUI: no comments allowed
  Back and forth expected: publishing concludes
  Multiple people may receive notification: yes
  Persist comment forever: no comments allowed
  'Required, optional or disallowed': comment disallowed on publish
  Send additional emails on edit: no comments allowed
  Table store comment in: nowhere
'Request Access [to file]':
  Workflow: Request Access
  Item being discussed: file
  Role performing the action: No role
  What is being asked for: File Downloader role
  Allow edited version in GUI: 'no editing, one shot'
  Back and forth expected: 'no, one shot?'
  Multiple people may receive notification: yes
  Persist comment forever: no?
  'Required, optional or disallowed': optional
  Send additional emails on edit: 'no, one shot'
  Table store comment in: workflowcomment
'Deny Access [to file]':
  Workflow: Request Access
  Item being discussed: file
  Role performing the action: Curator or Admin
  What is being asked for: nothing
  Allow edited version in GUI: 'no editing, one shot'
  Back and forth expected: 'no, one shot?'
  Multiple people may receive notification: no
  Persist comment forever: no?
  'Required, optional or disallowed': configurable?
  Send additional emails on edit: 'no, one shot'
  Table store comment in: workflowcomment
'Grant Access [to file]':
  Workflow: Request Access
  Item being discussed: file
  Role performing the action: Curator or Admin
  What is being asked for: nothing
  Allow edited version in GUI: 'no editing, one shot'
  Back and forth expected: 'no, one shot?'
  Multiple people may receive notification: no
  Persist comment forever: no?
  'Required, optional or disallowed': comment disallowed when granting access
  Send additional emails on edit: 'no, one shot'
  Table store comment in: workflowcomment
'Request Contributor Access [to dataset]':
  Workflow: Request Access
  Item being discussed: dataset
  Role performing the action: No role
  What is being asked for: Contributor role
  Allow edited version in GUI: 'no editing, one shot'
  Back and forth expected: 'no, one shot?'
  Multiple people may receive notification: yes
  Persist comment forever: no?
  'Required, optional or disallowed': optional?
  Send additional emails on edit: 'no, one shot'
  Table store comment in: nowhere

As of e57acda I was experimenting with a new table called publicationauditentry but I think I'll try a new table called workflowcomment to see if the various use cases fit well.

[pdurbin]

Here's our definition of done from our meeting this morning followed by some writing on the whiteboard. I'm sure there are a few other FIXMEs in the code I'll want to look at but this is the most important stuff:

• [x] Persist "reason for return" forever. Working as of 551635d.
• [x] Don't show "reason for return in UI (remove from in-app notifications but keep message in emails). As of 2017-07-13 email messages are buggy per a comment below.
• [x] Allow multiple "reasons for return" from curators per dataset version. Working as of 551635d.
• [x] Put "reasons for return" in a new table probably called workflowcomment. Fixed in 9c0fde6.
• [x] Show errors on exceptions from new commands. Fixed in c6dc562.

[pdurbin]

@TaniaSchlatter is back (yay!) and we just talked about the state of the issue and how I attempted to make some UX observations at https://groups.google.com/d/msg/dataverse-community/bGlCU2pbQpE/10oSARygAQAJ which I will go ahead and copy and paste here:

Thanks, Philipp! These are exactly the sorts of ideas I'm looking for. You seem to be describing several problems or deficiencies. Let me attempt to address them one by one in the form of a "UX observation" like we heard[1] about at the community meeting.

My first observation is that you like the "Publication Status" label and that the author can't easily tell that a dataset has been returned. (Obviously, the notification is not enough.) To solve this problem, you're suggesting we add a new label called "Returned to Author". The Style Guide says[2] we already have an "In Review" label so I'll put it on my list to check when and for whom it appears. I assume only the curator sees it.

My second observation is that curators are potentially confused about whether they are being to asked to curate an entirely new dataset that has never been published (a combination of the labels "Unpublished" and "Draft" indicates this brand new, never been published status) versus a dataset that has already been published as 1.0 or whatever but now has a post-publication draft (the label "Draft" is present but the label "Unpublished" is absent). It's probably too subtle to expect people to think this hard about the presence or absence of the "Unpublished" label. For now, I hope this helps explain the existing labels. If this isn't documented in the User Guide please do open a GitHub issue. :) Anyway, your proposed solution is to show the version number to the curator because the curator knows that "1.1" means the dataset has already been published and that the author needs review of a post-publication draft. It's a good idea. There's a related GitHub issue at https://github.com/IQSS/dataverse/issues/2782 about bringing back a version indicator on the dataset page that was lost when we made some performance improvements and I think this would help a ton. This is probably out of scope for https://github.com/IQSS/dataverse/issues/3943 but I can at least look into adding the version number to the notifications that are sent.

My third observation is that at some institutions, when the author clicks "Submit for Review", the dataset needs to be directed to a curator who knows something about the subject (e.g. Mathematical Studies) and that putting the subject in the notification would help. I like this idea.

Also, I did take note of a new bit of feedback from Steve McEachern at https://github.com/IQSS/dataverse/issues/3943#issuecomment-312400480 where he wrote, "I like where this is going - this is an important workflow for an archive like ADA where the curator often makes a number of edits and modifications prior to publishing, in conjunction with the author." My observation here is that it's expected that this process might look a bit like a tennis match with the dataset bouncing between author(s) and curators(s) before the dataset is ultimately published

Thanks again for this new feedback, Philipp and Steve! I'm glad I asked!

[pdurbin]

See the checked boxes above for things @sekmiller and I have fixed today. I also removed the 200 character limit in 053eb65. Good progress.

I know we said we'd keep the 3943-return-to-author-new-table sending email with the "reason for return" in it if it's working, but unfortunately, it's not and there is other weirdness with email. From a quick check:

• The curator sees a subject of "Root: Your dataset has been submitted for review" but it should probably be "A dataset". I believe this is how it is in production too.
• The email to the curator ends with "Don't forget to publish it or send it back to the contributor!." which is too much punctuation.
• After the first "Return to Author" the author(s) don't see the reason for return in the email.

I'm inclined to say we should back out of changes to the email behavior and revert it back to how it works in production but I'm fine with whatever decision.

Other todo's:

• [x] Move the saving of the "reason for return" into the ReturnDatasetToAuthorCommand. This means passing in the reason as a string as well.

[pdurbin]

In a913ff1 I just documented in the API Guide the latest behavior of the code we're working on. Feedback on what I wrote is welcome! Here's a handy screenshot:

[pdurbin]

I'm inclined to say we should back out of changes to the email behavior and revert it back to how it works in production but I'm fine with whatever decision.

@scolapasta @sekmiller @pameyer and I decided to make pull request #3992 send the same emails that we send in production, backing out of any changes we made along that way in 872dc56. That is to say, we'll give the design team time to think about the UX of these emails and decide how best to put the "reason for return" in them. I mentioned this to @TaniaSchlatter this morning and the design team has a Trello card about the future of this feature at https://trello.com/c/BriFJ5mR/35-submit-for-review-return-to-author-3702-3943 .

I just merged the latest code from "develop" into the pull request and moving this issue to Code Review at https://waffle.io/IQSS/dataverse

[kcondon]

Seeing some weirdness:
-Clicking dataverse link on dv card does not take creator to dataverse page, remains on search results page.
-Dataverse page no longer contains add data button for creator (saw at least once after creation but it appears to be there if first navigate to dataset and then use brcrumb.
-Dataverse does not appear on MyData page for creator but appears on search results
(error in server log says could not index dataverse, null value)

First 2 exist only in this branch. The third seems to have found its way into /develop.

[sekmiller]

There's definitely some weirdness in the dataverse cards urls (I don't think it was introduced in this branch)

This works:
http://localhost:8080/dataverse/dvb46ef2ac

This also works:
http://localhost:8080/dataverse.xhtml?alias=dvb46ef2ac

This doesn't (and shows up on dataverse cards in my local):
http://localhost:8080/dataverse/root?alias=dvb46ef2ac

[sekmiller]

Fix for the Dataverse Card Link is in the Return to Author Branch.