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

Returning to author now requires a reason that is sent by email to the author #10137

Merged
merged 11 commits into from
Mar 6, 2024

Conversation

luddaniel
Copy link
Contributor

@luddaniel luddaniel commented Nov 22, 2023

What this PR does / why we need it:

It displays a required textarea inside the "return to author" popup that is used to comment the reason of the return. This comment is sent by email to the author.

Which issue(s) this PR closes:

Closes #3702

Special notes for your reviewer:

This PR does not cover the management of displaying reasons of return in notification page.
Notification does not share the same architecture as WorkflowComment on a DatasetVersion, so it's not easy to join on comment to a notification in case there is multiple return to the author on the same Dataset version.

Bundle.properties new values must be arbitrated, as the closing of #3702

Suggestions on how to test this:

All the XHTML part with required field and remoteCommand seems really "delicate", an attention must be paid to the other commands in the dataset (like editing/saving a dataset).
Also, validation message seems to be removed a bit too fast when tags are updated, it might need a little improvement.

Does this PR introduce a user interface change?

return_to_author.mp4

Is there a release notes update needed for this change?:

Definitely yes.

@coveralls
Copy link

coveralls commented Nov 22, 2023

Coverage Status

coverage: 20.273% (-0.002%) from 20.275%
when pulling 0028b90 on Recherche-Data-Gouv:3702-return-to-author
into de45c13 on IQSS:develop.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Wow, I'm impressed by how little code is needed. Great job, @luddaniel!

Also, I love the video!

Now I think it's a matter of seeing if we want this or not. And if so, where it falls in our priorities.

src/main/java/propertyFiles/Bundle.properties Outdated Show resolved Hide resolved
src/main/webapp/dataset.xhtml Show resolved Hide resolved
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

One more question.

@DS-INRA DS-INRA added this to 🚧 Dev by Recherche Data Gouv in Recherche Data Gouv (formerly Data INRAE) Nov 29, 2023
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Adding feedback from @sbarbosadataverse (from Slack).

src/main/webapp/dataset.xhtml Outdated Show resolved Hide resolved
src/main/java/propertyFiles/Bundle.properties Outdated Show resolved Hide resolved
Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
@luddaniel
Copy link
Contributor Author

Here is the demo of the last version :

demo_return_to_author_v3.mp4

@@ -781,7 +781,8 @@ notification.email.createDataverse=Your new dataverse named {0} (view at {1} ) w
# Bundle file editors, please note that "notification.email.createDataset" is used in a unit test
notification.email.createDataset=Your new dataset named {0} (view at {1} ) was created in {2} (view at {3} ). To learn more about what you can do with a dataset, check out the Dataset Management - User Guide at {4}/{5}/user/dataset-management.html .
notification.email.wasSubmittedForReview={0} (view at {1} ) was submitted for review to be published in {2} (view at {3} ). Don''t forget to publish it or send it back to the contributor, {4} ({5})\!
notification.email.wasReturnedByReviewer={0} (view at {1} ) was returned by the curator of {2} (view at {3} ).
notification.email.wasReturnedByReviewer={0} (view at {1} ) was returned by the curator of {2} (view at {3} ).{4}
Copy link
Member

Choose a reason for hiding this comment

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

Would be a good opportunity to be more precise :)

Suggested change
notification.email.wasReturnedByReviewer={0} (view at {1} ) was returned by the curator of {2} (view at {3} ).{4}
notification.email.wasReturnedByReviewer=Your dataset named {0} (view at {1} ) was returned by the curator of {2} (view at {3} ).{4}

Also in wasSubmittedForReview, see other suggestion

@@ -781,7 +781,8 @@ notification.email.createDataverse=Your new dataverse named {0} (view at {1} ) w
# Bundle file editors, please note that "notification.email.createDataset" is used in a unit test
notification.email.createDataset=Your new dataset named {0} (view at {1} ) was created in {2} (view at {3} ). To learn more about what you can do with a dataset, check out the Dataset Management - User Guide at {4}/{5}/user/dataset-management.html .
notification.email.wasSubmittedForReview={0} (view at {1} ) was submitted for review to be published in {2} (view at {3} ). Don''t forget to publish it or send it back to the contributor, {4} ({5})\!
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
notification.email.wasSubmittedForReview={0} (view at {1} ) was submitted for review to be published in {2} (view at {3} ). Don''t forget to publish it or send it back to the contributor, {4} ({5})\!
notification.email.wasSubmittedForReview=Your dataset named {0} (view at {1} ) was submitted for review to be published in {2} (view at {3} ). Don''t forget to publish it or send it back to the contributor, {4} ({5})\!

see other comment

src/main/java/propertyFiles/Bundle.properties Outdated Show resolved Hide resolved
src/main/java/propertyFiles/Bundle.properties Outdated Show resolved Hide resolved
src/main/webapp/dataset.xhtml Outdated Show resolved Hide resolved
@luddaniel
Copy link
Contributor Author

Hello @pdurbin, it seems like everybody's desire has been listened, how can we move forward on this PR ?

@pdurbin
Copy link
Member

pdurbin commented Dec 8, 2023

@luddaniel yes, thanks for being so responsive! Next we check with @cmbz and @scolapasta to see where this fits into priorities, etc.

@cmbz
Copy link

cmbz commented Dec 11, 2023

2023/12/11

  • @luddaniel @pdurbin As per conversation with @scolapasta we're adding it to the 6.2 proposals.
  • Forgot to add: this needs sizing; please suggest a size.

@cmbz cmbz added this to the 6.2 milestone Dec 11, 2023
@cmbz cmbz added Type: Feature a feature request Feature: Notifications UX & UI: Design This issue needs input on the design of the UI and from the product owner UX & UI: New React UI Needs enough design work that it should probably go in the new React UI, not JSF Feature: In Review Workflow Component: JSF Involves modifying JSF (Jakarta Server Faces) code, which is being replaced with React. User Role: Curator Curates and reviews datasets, manages permissions labels Dec 11, 2023
@cmbz
Copy link

cmbz commented Dec 11, 2023

2023/12/11: Updated labels to match original issue: #3702

@pdurbin pdurbin added the Size: 3 A percentage of a sprint. 2.1 hours. label Dec 11, 2023
@pdurbin
Copy link
Member

pdurbin commented Dec 11, 2023

@cmbz thanks!

My only reservation is that it's a new required field. Will some curators be annoyed by it? And does the API allow you to get around this requirement? We should probably have consistency between the UI and API, either required or not. And maybe it should be configurable (globally, for now) if it's required or not. @luddaniel do you have thoughts on this?

As for size, if we accept the code as-is with no changes, it's probably a 3. If we need to meet and discuss what the behavior should be, it's probably higher. Maybe we can continue discussing here.

@pdurbin pdurbin changed the title #3702 - Returning to author now requires a commented reason that is sent by email to the author Returning to author now requires a reason that is sent by email to the author Dec 15, 2023
@DS-INRA
Copy link
Member

DS-INRA commented Dec 22, 2023

Mentionning the SPA frontend issue for this PR to appear there :

@cmbz
Copy link

cmbz commented Jan 8, 2024

2024/01/08: Moved to sprint ready as per prioritization meeting

@pdurbin
Copy link
Member

pdurbin commented Feb 22, 2024

@luddaniel over at #3702 (comment) @philippconzett is asking if the feature can be turned off. What do you think? Should we make it configurable? Make it possible to turn it off?

@luddaniel
Copy link
Contributor Author

@luddaniel over at #3702 (comment) @philippconzett is asking if the feature can be turned off. What do you think? Should we make it configurable? Make it possible to turn it off?

  1. I think we can do that with some work; Can we agree and the spec before developments/tests ?
    A new setting must be added, named :ReturnToAuthorReasonRequired, by default true (if absent) and can be disabled using this curl command curl -X PUT -d false http://localhost:8080/api/admin/settings/:ReturnToAuthorReasonRequired.
    This setting set to false will act the same on GUI popin (not displayed therefore not required) and API endpoint (not required).

  2. OR; @philippconzett can you handle this by explain to curators to type something like "A curation report with feedback and suggestions/instructions will follow in another email ?".
    I mean this required reason can be addressed with whatever meaningful informations as long as a minimum of 1 character...
    The release note can be decorated by Do not consider this requirement as a barrier as it can be supplemented with a creative and meaningful comment such as "The author would like to modify his dataset", "Files are missing", "Nothing to report", "A curation report with comments and suggestions/instructions will follow in another email".

As time is precious, I prefer option 2.

Waiting for your response @pdurbin @philippconzett
Regards

@philippconzett
Copy link
Contributor

Thanks, @luddaniel! Option 2 is fine for us. Please make sure to include the instruction in the release notes and the guide update.

@sekmiller sekmiller self-assigned this Feb 29, 2024
Copy link
Contributor

@sekmiller sekmiller left a comment

Choose a reason for hiding this comment

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

Looks like all of the previous update requests have been implemented. @luddaniel can you please make sure that this branch is up to date with the latest from develop? thanks!

@sekmiller sekmiller removed their assignment Feb 29, 2024
@stevenwinship stevenwinship self-assigned this Mar 6, 2024
@stevenwinship stevenwinship merged commit 4716c7a into IQSS:develop Mar 6, 2024
12 checks passed
Recherche Data Gouv (formerly Data INRAE) automation moved this from 🚧 Dev by Recherche Data Gouv to Done Mar 6, 2024
@stevenwinship stevenwinship removed their assignment Mar 6, 2024
@DS-INRA DS-INRA moved this from Done to 🚀 Done (Recherche Data Gouv) in Recherche Data Gouv (formerly Data INRAE) Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: JSF Involves modifying JSF (Jakarta Server Faces) code, which is being replaced with React. Feature: In Review Workflow Feature: Notifications Size: 3 A percentage of a sprint. 2.1 hours. Type: Feature a feature request User Role: Curator Curates and reviews datasets, manages permissions UX & UI: Design This issue needs input on the design of the UI and from the product owner UX & UI: New React UI Needs enough design work that it should probably go in the new React UI, not JSF
Projects
Status: Done 🧹
Recherche Data Gouv (formerly Data IN...
  
🚀 Done (Recherche Data Gouv)
Development

Successfully merging this pull request may close these issues.

Return to Author: When returning datasets that were submitted for review, I want to add a feedback note.
9 participants