Add permissions to conversation resolution

[jakeleventhal]

There should be some sort of permission system on conversation resolution. For instance, there should be an option to not allow the author of a PR to resolve conversations.

Example/use case:
John makes a PR
Bob leaves feedback in a conversation "you should fix this line"
John resolves the conversation without addressing the feedback
Bob forgets about the conversation since it is one of many conversations on the PR

If John was not able to resolve the conversation (only the PR reviewers can resolve) then this can't happen

[PavelA85]

agree, this would make life much easier.

[jdevfullstack]

but the owner of the repo still has the full rights whether to address the PR or not,

if that is in a company, then it's their internal communication that will matter

[cferreira-fuze]

Which is why devs usually aren't the admins of the repo.

[cferreira-fuze]

I also agree that such feature would provide better PR handling. But this should only be valid for comments added in request for revision. Comments added only in the spirit of comments, should not be included in this restriction.

[cgaldiolo-splashlight]

Sometime while reviewing a PR I start a conversation to address a problem or a doubt.
If I start a conversation I would like to be the only one to have the permission to resolve that conversation (not the PR author and not another reviewer).
Many times my conversations are being resolved by someone else and I have to click "Show Resolved" just to check if it was closed
by me or someone else and why

[jakeleventhal]

For this reason, perhaps this feature can be implemented as a "Mark needs work" button that can be applied to any comment. essentially a task feature. The button could create 3 buttons (only one selectable at a time).

Needs work (selected)
Addressed
Closed

If the needs work button was never clicked, the three buttons never appear. If it is clicked, needs work is selected.

The reviewers can click each option. The PR author can only click needs work and addressed.

[DanJBower]

This sort of feature would be really helpful for my organisation. For our review process we'd like it so that all comments that might need a problem addressing should only be resolvable by the author of the comment or by the review moderator in times of deadlock between the comment author and the review owner. This alongside requiring all comments to be resolved before the PR can be complete would be perfect

[cobaltplay]

I would really like this feature. tracking down my conversations to see if they were answered is a pain

[TimCTP]

It's incredibly clunky, and painful. And the UI even hides some conversations in the middle. When you have a dev on the team that does some changes, ignores others, self-resolves etc, it's 3x as much work to figure out what's going on, and makes one consider getting a food truck.

[jeniabrook]

Is there any news regarding this awesome feature?

[jakeleventhal]

This is one of those features worth applying to work at GitHub, implementing, then quitting lol

[kdurando]

I completely agree that there's a need. I'm just about to unresolve 175 conversations to verify the implementation of what has been done.

[willsully]

Definitely in favour of this - but obviously it should be a configurable option to be tailored to each organisation/individual's needs.

I'd like the ability to lock it down so that only the person who raised the comment is able to mark it as resolved - that's the process we follow but unfortunately there's no way to enforce this. Also an admin override of some kind would probably be necessary.

[SaulBurgos]

I need this feature. Big companies with different timezone is vert handy

[jagerman]

This is definitely needed.

Additionally the "Require conversation resolution before merging" branch protection rule on a repository becomes fairly useless without comment resolution permissions because someone can trivially just resolve all conversations to bypass the protection rule.

[mroy-seedbox]

This 👆

[hohwille]

In general the permissions of github are way too corsare grained.
I do not want to give write permissions to my repository to (all) developers.
However, with triage permission developers cannot resolve conversations that they started if the PR is not created by them (in the initial example Bob does not have the permission to resolve his own review comment what IMHO does not make any sense to me).
Maybe you also want to allow some developers to run a workflow manually, etc. but you do not want to grant Admin permissions...
Even if you pay a silver or gold subscription you still need github enterprise to make use of fine-grained permissions.
That does not really make sense to me.

[amatiash]

I feel a need to restrict a person who made a PR to resolve your comments as a reviewer.
Recently had a case of a developer that closed my comment without actually making the fix.

[cferreira-fuze]

Unfortunately, dysfunctional teams are a reality and it's precisely because of such reality that these features are needed.

[mroy-seedbox]

Even it they resolve it themselves after making the correct fix, it can be hell for reviewers to track when there are dozens of comments (or more).

I approach resolving conversations like checking off items on a list (but it does not work if someone else starts checking off items for me, even if they did it).

[pristinesource]

It would be great if we had a permission/setting to only allow the reviewer to resolve conversations on a pull request when requesting changes. We just recently ran into a situation where this would have been very beneficial.

[TimCTP]

For starters (and I mean bare minimum, I strongly agree with the suggestion here), it would be great if the UI showed Resolved - username here, instead of just this:

Then you could at least tell the situation at a glance without having to expand each one (i.e. identify did the reviewer resolve, or the PR author).

[mroy-seedbox]

Oh yeah, that alone would be a huge improvement! 👌

Then we could un-resolve and re-resolve ourselves if it is indeed fixed. 😅

[mroy-seedbox]

And the information is already available!

It just needs to be displayed differently (without having to click "Show resolved" to see it).

[mroy-seedbox]

I am a very thorough reviewer, and it is hell to keep track of things if other people resolve my own comments.

And it's not like we can expect the problem to go away just by saying "please do not resolve my comments".

I'm tired of clicking "show resolved". I am not loving this experience at all. But it would be a lovely experience if I were the only one able to resolve my own comments.

Please fix this.

[mroy-seedbox]

Imagine a marriage like this:

"Honey, we need to talk."

"Resolve conversation."

"What?!???"

[mroy-seedbox]

And if that doesn't work, there's aways "sudo resolve conversation". 😆

[mroy-seedbox]

Right now, I have to track resolution by putting a 👍 emoji reaction at the end of the conversation, because no one else can put one for me. And even if they also put their own 👍 , it shows up differently (not in blue).

Sometimes it looks weird because I'm putting a 👍 on my own comment, but 🤷‍♂️.

But "Require conversation resolution before merging" is still broken since people can self-resolve all issues.

[mroy-seedbox]

Alright, I got tired enough of this that I created a little bookmarklet to handle it. It's not perfect, but it helps a lot.

Basically how it works:

• Change the username to your own.
• Copy-paste the entire code as the URL of a bookmark in your bookmark bar
  • You can look at it, it's not doing anything dangerous.
• Click it while on the "Conversation" tab of a PR.

When clicked:

• It will expand all of the conversations that are not currently expanded.
• It will remove from the page/DOM those that are expanded and resolved by username (yourself).
  • And leave any which are unresolved or resolved by someone else.

So you might need to click it twice in order to have the full effect: first to expand, and then again to delete (because we cannot know who resolved a conversation without expanding it first).

If other people resolve your comments, simply unresolve them and then resolve them yourself.

But of course, "Require conversation resolution before merging" is still broken since people can self-resolve all issues.

javascript:(() => {
  const username = "mroy-seedbox";
  document.querySelectorAll('div.js-timeline-item:has(details.js-resolvable-timeline-thread-container > summary)').forEach(e => {
    if (e.querySelector('details').getAttribute('open') == null) {
      e.querySelector('details span.Details-content--closed').click();
    } else {
      const resolver = e.querySelector('details form.js-resolvable-timeline-thread-form > div > strong')?.innerText;
      if (resolver == username) e.remove();
    }
  });
})();

[zubin]

This is a complicated problem because there are multiple perspectives:

• PR authors want to mark conversations as resolved so they can see what's left to address.
• PR reviewers want to revisit conversations they've raised to see whether they've been resolved.

Therefore, this behaviour may make sense:

• When a PR author marks a conversation as resolved, it's only resolved from their perspective.
  • To everyone else it says "Marked as resolved by author" but it remains unfolded to other reviewers.
• Reviewers can choose "Agreed: conversation is resolved", which will fold the conversation only for them.
  • Otherwise they can comment and optionally unresolve the conversation (for the author).

A grey area is where to apply the non-author logic:

1. to anyone who commented in the conversation
2. to whoever initiated the conversation
3. to all reviewers
4. to everyone except the author

Personally, I prefer option 1. But this leads to another question: unless option 4, what should everyone else see? Probably same as author (ie resolved).

[TimCTP]

Not bad ideas. I hadn't thought about the "PR author tracks things done by using 'resolved'" angle (since I don't do that). Perhaps one "resolved" state isn't enough, like "done/not doing" (author picks) and "resolved" (reviewer or admin picks)?

[mroy-seedbox]

Good point! The behavior you propose makes sense. 👍

One way to make it work for everyone could be for the system to remember the "Hide/show resolved" status for each conversation, for each user. And by default it should remain open (even if resolved) until a user manually hides it. And that could be a behavior toggle for users who find that annoying ("Show resolved conversation until hidden": true|false), which should be false/unchecked by default (to preserve the current behavior).

That way, "Hide resolved" would become a way for each user to mark a conversation as completed for them.

[mroy-seedbox]

Actually, see @jakeleventhal 's comment below. It's an even better solution.

[TimCTP]

^ YES. Ooh la la.

[jakeleventhal]

When I worked at SpaceX, the way we had issue resolution was like this:

Every comment had a button group with 3 options that acted like radio buttons

1. Needs work
2. Addressed
3. Resolved

PR authors could only select 1 or 2
PR reviewers could select 1, 2, or 3

[mroy-seedbox]

👌

Addressed vs Resolved is the perfect way to solve this issue with "resolved by PR author" vs "resolved by PR reviewer".

And if authors can only mark as "Addressed" instead of "Resolved", then they would no longer be able to bypass "Require conversation resolution before merging" by marking everything as resolved themselves.

[zubin]

Agreed: "addressed vs resolved" is simple and intuitive. I like!