Making Done button not checking Resolved by default

[Björn Stenborg]

When refining our Gerrit review workflow we've found that it's unsuitable for us to have the Done button check the Resolved check box by default, as is hard-coded today. The reason is that we define Done as an action that is done by the Author to indicate that a comment has been handled, while the Reviewer should verify these changes and then accept them by marking the comment thread as Resolved (most often using the Ack button). Currently the Author can use the Done button but must then always remember to un-check Resolved, which is of course really hard to remember.

So we'd like either to change the default behavior of Done so that it doesn't check Resolved. This is trivial in the code, just changing from false to true in /gerrit/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.js:369 (current master).

Alternatively, we could add a configuration option to select this per-project or per-installation. Either solution works as well.

Of course, we don't want to break anyone else's work flow! So, is anyone depending on Done marking Resolved as well? Is there a better way to accomplish this? Thoughts and ideas?

[Ben Rohlfs]

Hi Björn, sorry for the late reply.

what you are describing is a very special workflow that is not supported by Gerrit. Changing the default in Gerrit is not an option, because the usual workflow is that owners take care of resolving threads and that is what all our users are currently used to.

We are even pushing back on making this an official config option, because we want to avoid adding config options for all tweaks that individual companies are making. To a certain extent, Gerrit is a code review product with some well defined concepts and workflows that you cannot change, even though Gerrit does already have quite a lot of config options and plugin extension points. We have a lot of users that are using Gerrit on multiple hosts/projects. For them it would be quite confusing, if the review process behaves slightly differently on each host.

There are two options for creating your very own company experience:

1. You can maintain a fork or better a series of patches that you apply on top of each official release and build the release on your own. I think for something like the one-line that you propose this should not be too difficult and still maintainable.

2. You can write a browser extension that you recommend to all your users, which tweaks the product to your needs.

Sorry for not having a better answer for you.

Happy holidays!

-Ben

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/9d89b24d-2120-462b-aeb0-b865a8c6ebd8%40googlegroups.com.

[Björn Stenborg]

Oh please, I'm happy to get such an extensive an thought-through answer. :)

I understand your reasoning and indeed there are pros and cons involved in any little tweak you do to the review flow. The main driver for us wanting the reviewer to close threads instead of the author is to provide that extra safety mechanism. Comment -> proposed correction -> agreement. To reduce the risk of a reviewer not actually reviewing the changes done as a result of their comments. But I realize that changing current and expected behavior is a non-trivial change. We'll have to go the patch route I think.

Thank you for your attention and have a great 2020.

/Björn

On Friday, December 20, 2019 at 11:52:12 AM UTC+1, Ben Rohlfs wrote:

Hi Björn, sorry for the late reply.

what you are describing is a very special workflow that is not supported by Gerrit. Changing the default in Gerrit is not an option, because the usual workflow is that owners take care of resolving threads and that is what all our users are currently used to.

We are even pushing back on making this an official config option, because we want to avoid adding config options for all tweaks that individual companies are making. To a certain extent, Gerrit is a code review product with some well defined concepts and workflows that you cannot change, even though Gerrit does already have quite a lot of config options and plugin extension points. We have a lot of users that are using Gerrit on multiple hosts/projects. For them it would be quite confusing, if the review process behaves slightly differently on each host.

There are two options for creating your very own company experience:

1. You can maintain a fork or better a series of patches that you apply on top of each official release and build the release on your own. I think for something like the one-line that you propose this should not be too difficult and still maintainable.

2. You can write a browser extension that you recommend to all your users, which tweaks the product to your needs.

Sorry for not having a better answer for you.

Happy holidays!

-Ben

On Wed, Dec 4, 2019 at 5:16 PM Björn Stenborg <bjorn....@gmail.com> wrote:

When refining our Gerrit review workflow we've found that it's unsuitable for us to have the Done button check the Resolved check box by default, as is hard-coded today. The reason is that we define Done as an action that is done by the Author to indicate that a comment has been handled, while the Reviewer should verify these changes and then accept them by marking the comment thread as Resolved (most often using the Ack button). Currently the Author can use the Done button but must then always remember to un-check Resolved, which is of course really hard to remember.

So we'd like either to change the default behavior of Done so that it doesn't check Resolved. This is trivial in the code, just changing from false to true in /gerrit/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.js:369 (current master).

Alternatively, we could add a configuration option to select this per-project or per-installation. Either solution works as well.

Of course, we don't want to break anyone else's work flow! So, is anyone depending on Done marking Resolved as well? Is there a better way to accomplish this? Thoughts and ideas?

--
--

To unsubscribe, email repo-d...@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-d...@googlegroups.com.

[Oswald Buddenhagen]

On Tue, Jan 07, 2020 at 05:12:51AM -0800, Björn Stenborg wrote:
>But I realize that changing current and expected behavior is a
>non-trivial change. We'll have to go the patch route I think.
>

i currently don't have a stake in this discussion, but otherwise i'd
find that outcome somewhat unsatisfactory. ^^

i think you're doing your users a disservice by flipping the flag, both
for the reasons ben explains, and because you're making it harder for a
review owner to track what they still need to respond to.

the workflow you're describing isn't uncommon at all; for example many
bug trackers (including the one gerrit uses, heh) have separate states
for "resolved" and "released" (or "verified", or whatever). similarly,
translation assitance tools (which are basically just integrated editing
and review systems) have states that reflect these separate phases of
individual translation units (strings).

so i think what you are really asking for is a separate state for the
comments/threads, and i'd encourage you to make sure that there is a
respective feature request on the issue tracker.

[Martin Fick]

On Tuesday, January 7, 2020 5:12:51 AM MST Björn Stenborg wrote:
> Oh please, I'm happy to get such an extensive an thought-through answer. :)
>
> I understand your reasoning and indeed there are pros and cons involved in
> any little tweak you do to the review flow. The main driver for us wanting
> the reviewer to close threads instead of the author is to provide that
> extra safety mechanism. Comment -> proposed correction -> agreement. To
> reduce the risk of a reviewer not actually reviewing the changes done as a
> result of their comments. But I realize that changing current and expected
> behavior is a non-trivial change. We'll have to go the patch route I think.

An alternative approach that we use internally is to make CR-1 sticky. I
believe that this would be a good default improvement to Gerrit.

With such a workflow, a comment can be marked "done" to signal that the author
believes it has been resolved (as opposed to them not agreeing with the
proposed changed), and then the original reviewer can signal their "Ack" of
the "done" by removing their -1 (and optionally adding a +1/+2).

There is a change up for review with a proposal to be able to make more than
just the -2 sticky, see here:

https://gerrit-review.googlesource.com/c/homepage/+/247379

-Martin

> >> ---
> >> You received this message because you are subscribed to the Google Groups
> >> "Repo and Gerrit Discussion" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an

> >> email to repo-d...@googlegroups.com <javascript:>.

> >> To view this discussion on the web visit
> >> https://groups.google.com/d/msgid/repo-discuss/9d89b24d-2120-462b-aeb0-b8
> >> 65a8c6ebd8%40googlegroups.com

> >> <https://groups.google.com/d/msgid/repo-discuss/9d89b24d-2120-462b-aeb0-> >> b865a8c6ebd8%40googlegroups.com?utm_medium=email&utm_source=footer> .


--
The Qualcomm Innovation Center, Inc. is a member of Code
Aurora Forum, hosted by The Linux Foundation