pkp-lib icon indicating copy to clipboard operation
pkp-lib copied to clipboard

In OJS, a reviewer cannot access his/her recommendations from a previous round.

Open pilasou opened this issue 1 year ago • 40 comments

As a reviewer reinvited to a new review round, I would like to be able the see the evaluation I provided in previous rounds.

In OJS, reviewers that are being invited to different review rounds on the same submission cannot access what was their previous recmmandations.

This improvement will offer the opportunity to reviewers to access their previous review reports and thus, better determine how the author made his/her changes to the manuscript.

The reviewer will be able to access his/her Previous round reports by clicking on a Round button on top of the reviewer process page (as shown below). image

When clicking on a cycle button a new window open containing the following information:

  • Recommandation
  • Forms informations
  • Review file sent
  • Cycle dates
  • Article version reviewed during preceding round

image

PRs:

Update (2024-04-02) pkp/ui-library#313 (jardakotesovec repository) pkp/pkp-lib#9526 pkp/ojs#4148

Initial: #9526

How to test?

  1. Make a submission and send it to the Review step.
  2. Invite one reviewer.
  3. Connect as reviewer and complete the review (fill the form and add a file).
  4. As editor, Request revision and select Revisions will be subject to a new round of peer reviews.
  5. As an author, submit revision.
  6. As editor, open a 2nd round of review.
  7. Invite the 1st round reviewer.
  8. Connect as the reviewer.
  9. On top of the review page, you should see the Round 1 bouton.
  10. In the new window, the reviewer can access its answers (forms and files).

pilasou avatar Oct 23 '23 18:10 pilasou

PR: https://github.com/pkp/pkp-lib/pull/9526

@Vitaliy-1, could you review this? Thanks!

asmecher avatar Nov 23 '23 00:11 asmecher

Thanks, @nibou230! I left a couple of comments.

Vitaliy-1 avatar Dec 11 '23 16:12 Vitaliy-1

Hi, @Vitaliy-1, just to let you know, I am a colleage of @nibou230, he will be on vacation for the next few weeks. Back in January. We will follow up when he comes back. Thanks!

pilasou avatar Dec 11 '23 21:12 pilasou

@nibou230 Hi, thanks for the contribution, its certainly useful feature. Only drawback is that this is built on legacy stack and we aim to avoid adding new functionality on this old stack, because that means that we would need to migrate it to newer stack at some point anyway.

I would like to ask whether you would have some dev capacity & interest to build this feature on newer stack?

It would be creating frontend part in Vue.js (significant portion of OJS is already on Vue.js) and creating relevant API endpoints that the frontend would use.

If so, we would provide support and guidance how to do it. In particular frontend part: @jardakotesovec api: @defstat UI/UX: @Devika008

Let us know what you think

Thank you!

Jarda

jardakotesovec avatar Jan 03 '24 13:01 jardakotesovec

Hi @jardakotesovec, if our planning let me some spare time, I will do my best to convert this feature on the newer Vue.js stack. Do you have any portion of the application (on Vue.js) that would be similar so I could take some inspiration from it?

Hello @Vitaliy-1, thanks for the comments, if they are still relevant, I will take a look at those once the refactor is done.

nibou230 avatar Jan 15 '24 14:01 nibou230

@nibou230 That would be great! Some of the things that we are currently using are quite new, therefore there is not enough examples, so I decided to create boilerplate to show how things should come together. New composition API from vue3 might look bit intimidating at first, but once you get use to it, its very expressive way to write logic for components.

Once the tests run, I will merge it to main (probably tomorrow morning), so you can rebase against main and start moving things over.

PR: ui-library, pkp-lib, ojs

Since I will be merging unfinished work to the main branch, its behind feature flag, to enable displaying of 'History buttons' adjust your config.inc.php to include:

[features]
enable_review_round_history = On

Now there are two big parts to tackle:

API

We already have many API endpoints to serve data - https://docs.pkp.sfu.ca/dev/api/ojs/3.4 . Unfortunately the data you are after are not exposed yet. So it will be necessary to introduce new ones that would cover needed metadata.

Can someone more familiar with that provide bit of guidance and suggest some good modern examples to follow? Pinging @asmecher @Vitaliy-1 @defstat

In boilerplate for demonstration purposes I am using the /submissions/{submissionId} endpoint to get submission metadata.

Styling

Thats probably last thing to tackle. For new UI we are using TailwindCSS - please checkout our documentation in storybook.

Pinging @Devika008 in case she would have capacity to mock what the UI could look like. If not - we will improvise :-).

Also note that one of the parts is listing of reviewer files - I will be building same Vue.js component hopefully in ~2weeks for new submissions listing. So thats something I would provide as well (but that handles just rendering, its still necessary to create API to fetch them).

Other notes

  • Yes I noticed that buttons shows Round 1, and I am tempted to adjust the translations, but want to check with the team first, rather than fallback to v-html which is way too permissive, just to be able pass the html entity.
  • Also feel free to explore the storybook that I linked above for additional technical details - I have not promoted it, as I am still working on documentation, but its lots of info already there. More officially it will be introduced in ~2/3weeks.

jardakotesovec avatar Jan 16 '24 14:01 jardakotesovec

Wow, thank you so much @jardakotesovec!

I was quite lost trying to figure out how I could start this and how to link the component/page to the reviewer handler section. I'm more of a C# type of developer so it's far from my usual code. :-)

This boilerplate will help me a lot. I'll wait until tomorrow for this code and meanwhile I will look at how I should expose the required data.

Should I create a whole new API controller or should I add a method or two inside the current submission one?

nibou230 avatar Jan 16 '24 18:01 nibou230

@nibou230 Hi Nicolas, its now merged on main. If you have any further frontend related questions or feedback, let me know. And hopefully on API side someone will chip in shortly.

jardakotesovec avatar Jan 17 '24 08:01 jardakotesovec

Hi @nibou230! Regarding the API, all the data that you are after seem to be contained into the Review process, so it could make sense to start with working on a separate API Controller and add your endpoints there. One example that you could investigate is the JATS API controller. If you have any questions regarding that don't hesitate to ask.

Depending on the URLs that the newly developed endpoints will listen to, there could be some specific changes that you need to make in order for those to work correctly, but I suggest to see those if there is such a need.

Please take into account possible differences between the OJS and OMP applications, so that each application (OJS or OMP) specific changes will be developed out of the PKP-LIB API Controller, and leave common changes (that cover both OMP and OJS) into the PKP-LIB API Controller.

You could check (for example):

  1. Submission API Controller for OJS
  2. Submission API for OMP No specific changes needed for OMP
  3. Submission API Controller for PKP-LIB - Common for all applications

defstat avatar Jan 18 '24 11:01 defstat

Hi @defstat, thank you for the help.

I'll look at the Jats controller and make another one specific for the reviews.

I understand the point to split what should be common or specific to an application, but I don't know OMP at all. I will do my best and if you guys spot something that should not be inside PKP-LIB, tell me and I will make the appropriate change.

nibou230 avatar Jan 18 '24 13:01 nibou230

Hi @defstat, thank you for the help.

I'll look at the Jats controller and make another one specific for the reviews.

I understand the point to split what should be common or specific to an application, but I don't know OMP at all. I will do my best and if you guys spot something that should not be inside PKP-LIB, tell me and I will make the appropriate change.

Perfect, thanks. I would suggest not bothering with OMP specifics, at this stage. You have already much to consider for this change. :)

The application specifics can be handled during the reviewing stage, and after the feature is in a mature functioning state.

defstat avatar Jan 18 '24 13:01 defstat

Hello,

I can do UI mock-ups for this keeping our updated design system in mind but will require till Tuesday the 23rd of January? Is that okay?

Devika008 avatar Jan 18 '24 17:01 Devika008

HI @Devika008, @defstat, @jardakotesovec Thanks very much for all your help on this!

Given the extensive refactoring and the fact that we need to adopt a very recent new technology, we propose to make a first MVP of this feature with:

  1. Standard UI using OJS components as is (as documented in StoryBook). The page will be structured using the section documented in the issue: Recommendations, Form content, Dates (see image below)
  2. For Author Manuscripts and Reviewer file, we will wait for Jarda works on submission to reuse the component he will have created.
  3. On first iteration, the form section will only display the default review forms, not custom forms

rev-history

Do you agree with this scenario?

pilasou avatar Jan 18 '24 17:01 pilasou

Hi guys @defstat @jardakotesovec,

I worked on the different PRs last week:

https://github.com/pkp/ui-library/pull/313 https://github.com/pkp/pkp-lib/pull/9526 https://github.com/pkp/ojs/pull/4148

It's not 100% done, the files are still missing, but you can already take a look if something needs to be addressed right away.

Thanks!

nibou230 avatar Jan 22 '24 16:01 nibou230

@nibou230 hi! Regarding your question on the reviewerId and whether this is the way to go, or whether you need to add the reviewerId as an input parameter to the request, I think you should consider the specific usecase here - would this issue result to a UI solution that will display "history" information only for the specific reviewer that is currently logged in, or will an editor for example have access to this information also?

If my understanding is correct, each reviewer will have access to his/her individual history. If this is the case then I think that getting the reviewerId from the request is sufficient.

defstat avatar Jan 23 '24 08:01 defstat

@defstat OK, thank you! Yes, you are correct, the history side panel is visible only for the current logged in reviewer for now. I wanted to make sure I followed the standards for API development.

nibou230 avatar Jan 23 '24 15:01 nibou230

Hello @jardakotesovec @nibou230 @pilasou @defstat

Keeping the elements we currently have and the information architecture we are trying to establish through our revised designs (editorial decisions, email templates, submission dashboard, following is the UX/UI i propose for the issue.

image

Specifications:

  1. I have adapted the UX/UI of Revised Editorial Decisions. this includes the stepper mechanism instead of the tabs for a step by step journey. Tabs usually indicate that you can hop between them irrespective of any decision which doesnt support the mental modal here
  2. I have also introduced a white block for Previous Reviews. There could be edge cases where there are more than one review round and hence to make the design scalable I introduced the block.
  3. The information flow and design below the stepper remain constant to what is there currently

image

Specifications:

  1. On clicking the "Read Review" button, this panel will open just like it does in the summary section of new submission dashboard.
  2. The summary section of the dashboard was designed in the way that the grey area was used to support submission information or metadata and the white area to support any information that is important to made a decision. Hence, I've used the grey area here to higlight submission metadata and the general information and the white area to show the recommendations, comments, files and attachments

Let me know if this works for you or if you have any additional comments of feedback on this

Devika008 avatar Jan 29 '24 23:01 Devika008

@Devika008 Thats great update, thank you! Lets keep this focused on the history. Changing tabs to stepper make sense, but is out of the scope of this one. Maybe create new ticket with that idea and we will see whether we can execute it separately or later when the reviewer process will be revamped.

@nibou230 Based on the design from Devika, here is some guidance that should help you to get that going easily, just merged that to main:

  1. 2 Column layout for side modal available in storybook - you will find this example in SideModal.stories.js
  2. Listing files - added component to handle that.. its not pixel perfect per the design, but will refine that later. Here it is in storybook. Source code again in stories file.

Let me know if there is anything else you need. The section with Read Round buttons on top should be quite easy just style with Tailwind.

jardakotesovec avatar Jan 30 '24 14:01 jardakotesovec

@jardakotesovec Thank you so much :D.

What ill do is that ill create a separate ticket for the stepper for this one. However, I feel it will be a good user experience if we retain the white block above the stepper and use it above the tabs to make it more scalable for the future

Devika008 avatar Jan 30 '24 17:01 Devika008

@jardakotesovec The storybook is really helpful! Thank you so much for the help, it makes my life better!

@Devika008 i'll try to follow the design :-) This is what I have for now, i'll continue the work in the following days.

image

nibou230 avatar Jan 30 '24 20:01 nibou230

Hi everyone, I'm mostly done with the development. You can see a preview of the review history here.

@jardakotesovec The URL used to download the file isn't working. Is this normal (still under development) at this point? It's trying to fetch "http://localhost:8000/index.php/partage/reviewer/submission/url".

image

nibou230 avatar Feb 01 '24 20:02 nibou230

This is amazing @nibou230. thank you so much

Devika008 avatar Feb 01 '24 21:02 Devika008

@nibou230 No idea, so far I just made the component that links to whatever url is passed to it. But to be honest not familiar with the endpoints for downloading files. I could look at it on Tuesday, but maybe someone more familiar will know the answer.

jardakotesovec avatar Feb 01 '24 21:02 jardakotesovec

Hi everyone, I'm mostly done with the development. You can see a preview of the review history here.

@jardakotesovec The URL used to download the file isn't working. Is this normal (still under development) at this point? It's trying to fetch "http://localhost:8000/index.php/partage/reviewer/submission/url".

image

@nibou230 the URL that does not work is the one that you prepared for the needs of this development? If so, you may need to have some code written for the URL to work. We can see that together if this is the case.

defstat avatar Feb 02 '24 07:02 defstat

Hi @jardakotesovec, I submitted a fix for the URL, you can see it here.

I also had to push another fix for the SubmissionFileAssignedReviewerAccessPolicy.

@defstat No, the new component is using the URL fetched with the submissionFile repository. I found the error this morning, the File component was trying to set the href instead of :href.

nibou230 avatar Feb 02 '24 14:02 nibou230

@nibou230 Thank you for fixing that. Can you please cherry-pick this commit to your ui-library branch to add this page to storybook? https://github.com/jardakotesovec/ui-library/commit/19200fb83992d5ae7559224a8fb0a25cec28dc14

Would like to try test workflow with Devika, so she can provide feedback in storybook when she can see mostly real thing.

Also you might have better API mocks while testing it - so feel free to replace them with your, especially if it tests wider range of metadata.

jardakotesovec avatar Feb 08 '24 13:02 jardakotesovec

@jardakotesovec OK, I already cherry-picked the commit and I will look if I can improve the mocks.

nibou230 avatar Feb 09 '24 14:02 nibou230

@Vitaliy-1 @defstat @defstat @asmecher I think last opened problem here is whether it needs some adjustments for OPS/OMP. Do anyone of you know from top of your head what differences there might be for this particular feature? I am not familiar with ops/omp yet much.. will be learning more about it once extending new submission listing for ops&omp.

jardakotesovec avatar Feb 27 '24 09:02 jardakotesovec

I would suggest, before a more specific review, to add the application specific (OJS) changes to OMP at least, and add PRs for both OMP and OPS for the tests to run. It will give a hint of whether those changes work as they supposed to on OMP and OPS.

@nibou230 if you have questions on how to prepare the PRs for OMP and OPS (and OJS) so that the tests can run on travis, don't hesitate to ask.

defstat avatar Feb 27 '24 10:02 defstat

Thanks, @defstat, we can make the PRs for OMP and OPS, but we want to say that we have never work with those softwares. If all goes smoothly and all tests pass for both software, does this mean our PRs will be accepted?

pilasou avatar Mar 01 '24 16:03 pilasou