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.

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

Add ability to "Return to Author" via API using JSON, read by author via email #3943

Closed
pameyer opened this issue Jun 21, 2017 · 47 comments
Closed

Comments

@pameyer
Copy link
Contributor

pameyer commented Jun 21, 2017

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
Copy link
Contributor

djbrooke commented Jun 21, 2017

  • 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
Copy link
Member

pdurbin commented Jun 22, 2017

#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
Copy link

@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
Copy link
Member

pdurbin commented Jun 26, 2017

@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
Copy link
Member

pdurbin commented Jun 26, 2017

@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
Copy link
Contributor Author

pameyer commented Jun 26, 2017

@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
Copy link
Member

pdurbin commented Jun 26, 2017

@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 added a commit that referenced this issue Jun 27, 2017
In my brain, wires are crossed such that I'm forever confusing the
"Request Access" workflow and the "Submit for Review" workflow.
@pdurbin
Copy link
Member

pdurbin commented Jun 27, 2017

@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 added a commit that referenced this issue Jun 27, 2017
@pdurbin
Copy link
Member

pdurbin commented Jun 27, 2017

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"

curator

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

author

@pdurbin
Copy link
Member

pdurbin commented Jun 27, 2017

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

dlmurphy added a commit that referenced this issue Jul 14, 2017
Typo fixes, rephrasing for clarity
dlmurphy added a commit that referenced this issue Jul 14, 2017
Almost missed one!
@dlmurphy dlmurphy removed their assignment Jul 14, 2017
dlmurphy added a commit that referenced this issue Jul 14, 2017
Changed references to authors to be plural, based on a conversation with @pdurbin - there are many cases where multiple authors will be affected and notified when a dataset is returned, so it makes sense to refer to them in the plural here.
@scolapasta scolapasta removed their assignment Jul 18, 2017
sekmiller added a commit that referenced this issue Jul 18, 2017
Also, completely remove Workflow Action.
sekmiller added a commit that referenced this issue Jul 18, 2017
Remove references to getting "Reasons for Return" from the documentation of the Notifications API
sekmiller added a commit that referenced this issue Jul 19, 2017
@pameyer pameyer self-assigned this Jul 19, 2017
@kcondon kcondon self-assigned this Jul 19, 2017
sekmiller added a commit that referenced this issue Jul 19, 2017
@djbrooke djbrooke added this to the 4.8 - Large Data Upload Integration milestone Jul 20, 2017
@kcondon
Copy link
Contributor

kcondon commented Jul 20, 2017

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
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests