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

Added Review State #1100

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Added Review State #1100

wants to merge 1 commit into from

Conversation

shreyalpandit
Copy link
Collaborator

Addresses Iss #1097

@amritpurshotam
Copy link
Collaborator

@shreyalpandit I'm not quite convinced about this PR. Having an internal reviewer assigned isn't quite an outcome in my opinion and seems to go against what's happening currently for applications.

The way I see it if we don't add the REVIEW status, we would still be able to tell which submissions are being reviewed by seeing which ones have an assigned reviewer but no review responses and/or an outcome.

Of course I could be missing something obvious. What do you think?

@shreyalpandit
Copy link
Collaborator Author

That makes sense. I was thinking 1) for notifications and 2) for iterations on the submission. 1) It would be helpful to track and remind reviewers to submit responses since there are no deadlines for the continuous journal. However, it seems like this can also be checked by looking at reviewers and outcome for a response. 2) When an applicant gets feedback and then submits a new response, they would no longer be considered the previous outcome but rather, be back in review. I guess this might not apply if the outcome is connected to each individual response...in which case we can still check for assigned reviewers and no outcome for submissions in review.

@avishkar58 had also suggested this addition to outcomes, so I am not sure if he had something else in mind.

@amritpurshotam
Copy link
Collaborator

@shreyalpandit Yeah you make good points and are definitely quite valid. This approach then would indeed solve this. Give me one more day to ponder on this one though.

We do have a concept of a active reviewer on the ResponseReviewer class which we don't seem to be using from a quick glance. Just want to think about if this could be what we need. So no need for action on your side. will get back to this tomorrow.

@avishkar58
Copy link
Contributor

avishkar58 commented Mar 22, 2023 via email

@amritpurshotam
Copy link
Collaborator

amritpurshotam commented Apr 11, 2023

@shreyalpandit @avishkar58 Hey sorry I dropped off on this for a bit.

Yeah I agree being explicit makes more sense. With the implicit approach it could get a bit muddled especially with the bank and forth of the review process. I think I was getting too caught up in the "Outcome" wording which felt like it should be the end of the process (but it doesn't have to be).

P.S looks like there's some merge conflicts now because of my delay so once that's resolved this should be good.

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

Successfully merging this pull request may close these issues.

3 participants