Project

General

Profile

Task #4238

default reviewers updates

Added by Marcin Kuzminski [staff] almost 3 years ago. Updated almost 3 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
-
Target version:
Start date:
22.09.2016
Due date:
% Done:

0%

Estimated time:
Sorting:
Commit Number:

Description

Some of my notes

  • it would be good to SAVE the reasons for a reviewer, so it's clear to outsiders which roles other members have. It's good to know "WHO" is the author, who is form the team etc
  • it'd be good to have "exclude" author option so it's not a possibility that author gets picked to review his own PR

History

#3 Updated by Daniel D almost 3 years ago

After some thought on second option (exclude pr author from reviewers)

How does this work for when the only person available for review is the pr author (ie. he is repo owner too), this can happen in eg. single user rhodecode instance

Maybe exclude author should be disabled when owner/author is only one available for the review process

Extending this, is there a reason to make this a toggle at all? Why not just exclude pr authors by default (which is what a multi user instance would want) and turn it off in the case of single user?

I guess the main question is: Why would a pull request author want to review his own pr?

#4 Updated by Marcin Kuzminski [staff] almost 3 years ago

Good question,

Main reason to put an author of PR into a review is to "block" the approval. So i often put myself in order to make the PR not mergable.

So maybe the only thing we want is:

  • in case there's only SINGLE reviewer it cannot be the author (the checkbox controls this actually)
  • Author is never picked automatically
  • If you want you can always add yourself to already existing pool of reviewers

I think in case of single instance then you have the author thing unchecked and system will/can pick YOU as reviewer, else it will not pick automatically, it will NOT allow you to create a PR with yourself as a reviewer.
This should imho cover all the cases, for Gemalto, for us, for a single-user-mode.

Thoughts ?

#5 Updated by Redmine Integration almost 3 years ago

  • Status changed from New to Resolved

Also available in: Atom PDF