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 new workflow for Request With Review #4187

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

frankduncan
Copy link
Contributor

This workflow is like Request with External Review, in that both internal and external reviewers can review submissions. However, it has a workflow more like Request in that there is only one Review step.

Test Steps

Best way to test/experiment with is to create a fund with this workflow and move through the process, using both internal and external reviewers.

@frankduncan frankduncan force-pushed the request-review-workflow branch 2 times, most recently from e70f5dd to 814eb99 Compare October 26, 2024 13:16
@frjo
Copy link
Contributor

frjo commented Oct 28, 2024

I think this is a good addition, one where internal and external review happens at the same stage. This has a good chance of being useful to many organisations.

Happy to see that it is fairly easy to add a new workflow.

The plan is to make it even easier by splitting them up in separate files.

Copy link
Contributor

@frjo frjo left a comment

Choose a reason for hiding this comment

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

Some minor comments around naming things but overall look good I think.

@@ -1285,7 +1423,7 @@ def phases_matching(phrase, exclude=None):
},
"internal-review": {
"name": _("Internal Review"),
"statuses": phases_matching("internal_review"),
"statuses": phases_matching("rev_review") + phases_matching("internal_review"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not rename rev_review to rev_internal_review instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it's not really internal review. It's external AND internal review.

The problem this solves is that there's a banner on submissions page (maybe gone in future versions) that showed a count of "Internal Reviews". So we lumped this in there rather than creating a new summary tab that would always show up for other workflow types.

Not really sure what's the best way to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you want it to phases_matching with *_internal_review I think it makes sense to name it rev_internal_review. This is a machine name and not displayed anywhere in the UI.

"rev_post_review_discussion": _("Close Review"),
INITIAL_STATE: _("Need screening (revert)"),
},
"display": _("Internal Review"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe only name this stage "Review" since there is only one step and it is both internal and external.

@frjo frjo added Type: Feature This is something new (not an enhancement of an existing thing). Type: Minor Minor change, used in release drafter Status: Needs testing Tickets that need testing/qa Status: Needs dev testing 🧑‍💻 Tasks that should be tested by the dev team labels Nov 4, 2024
@frjo
Copy link
Contributor

frjo commented Nov 14, 2024

"Request with review" is not the best name for this.

We already have "Request" that do have reviews but only internal ones. Then there is "Request with external review" that has internal och external reviews in series.

This new workflow also has internal och external reviews, but in parallel. Or in the same phase; step etc.

So what makes this new workflow unique is that the external and internal reviews are done at the same time. The name should give a clue about that I think.

Any suggestions?

@frjo
Copy link
Contributor

frjo commented Nov 14, 2024

The workflow itself works without issues when I test it. Fix the naming and we can merge it I think.

@frankduncan
Copy link
Contributor Author

frankduncan commented Nov 14, 2024

As they say, naming is one of the two hardest problems in computer science. I have no idea what to call it. If I were starting over, I would have three:

  • Request
  • Request with Only Internal Review
  • Request with Separated External Review

Or, in other words. This one feels like the standard, the current workflows seem like deviations from it, if that makes sense.

@frjo
Copy link
Contributor

frjo commented Nov 15, 2024

What about "Request with same time review"?

RequestRev -> RequestSame

rev_more_info -> same_more_info

etc.

@frankduncan
Copy link
Contributor Author

Update with name changes. I think I didn't miss any.

@frankduncan
Copy link
Contributor Author

Oh, whoops. Thanks for the catch!!

@wes-otf
Copy link
Contributor

wes-otf commented Nov 25, 2024

So in testing with 2 different accounts assigned (one reviewer role, one staff role) when in the review stage I can only see that it's ready for review as a staff member. I get permission denied when trying to access it as the reviewer role, despite getting an email that it's ready for review.

Let me know if there's a detail/config option I missed!

@wes-otf wes-otf added Status: Tested - changes/discussion needed 🔨 and removed Status: Needs testing Tickets that need testing/qa Status: Needs dev testing 🧑‍💻 Tasks that should be tested by the dev team labels Nov 25, 2024
frankduncan and others added 3 commits November 25, 2024 21:58
This workflow is like Request with External Review, in that both
internal and external reviewers can review submissions.  However, it has
a workflow more like Request in that there is only one Review step.
@frjo
Copy link
Contributor

frjo commented Nov 25, 2024

@wes-otf It was the Reviewer settings that was the issue. See https://test-apply.hypha.app/admin/settings/funds/reviewersettings/2/.

I have deactivated the settings now so Hypha reverts back to the default.

@frjo frjo added Status: Needs testing Tickets that need testing/qa Status: Needs dev testing 🧑‍💻 Tasks that should be tested by the dev team and removed Status: Tested - changes/discussion needed 🔨 labels Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs dev testing 🧑‍💻 Tasks that should be tested by the dev team Status: Needs testing Tickets that need testing/qa Type: Feature This is something new (not an enhancement of an existing thing). Type: Minor Minor change, used in release drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants