[PATCH 1/4] Make addressed/unaddressed workflow opt-in
Raxel Gutierrez
raxelgutierrez09 at gmail.com
Tue Aug 31 07:22:52 AEST 2021
Hi,
On Sun, Aug 29, 2021 at 9:05 AM Daniel Axtens <dja at axtens.net> wrote:
>
> Stephen Finucane <stephen at that.guru> writes:
>
> > On Fri, 2021-08-27 at 13:50 +1000, Daniel Axtens wrote:
> >> Stephen Finucane <stephen at that.guru> writes:
> >>
> >> > The current workflow for the address/unaddressed attribute of comments
> >> > sets all comments to unaddressed by default. This is unintuitive, as it
> >> > assumes that all comments are actionable items. It also imposes a
> >> > massive burden on maintainers, who will need to manually sift through
> >> > every single comment received to a list and manually set the
> >> > non-actionable items as "addressed".
> >>
> >> I agree that not every email is an actionable item.
> >>
> >> I'm not convinced it's a burden on maintainers specifically. The comment
> >> can also be marked as addressed by the patch submitter. Also,
> >> maintainers (and everyone else) are free to ignore the field (and every
> >> other piece of data stored on patchwork).
Agreed, I think patches can be simply marked addressed by the patch
submitter as a way to indicate that it's an actionable comment. If
they are wanting to use the 'addressed' status feature, then I don't
think they mind having to mark something like that.
> >> In general I do think 'unaddressed by default' is a good behaviour. But,
> >> I agree that we can improve the current behaviour.
> >>
> >> I think it makes sense to have it as null for every old patch. So if you
> >> migrate, old patch comments are neither addressed nor
> >> unaddressed. That's something I didn't consider sufficiently earlier on.
>
Agreed that old patches should be set to null.
>
> >> I think it also makes sense for patches that add 'Acked-by:',
> >> 'Reviewed-by:' or 'Tested-by:' to be considered automatically addressed.
I have seen comments that have the 'Reviewed-By' and 'Acked-by'
taglines but still have some questions and stuff to address. Maybe
there could be a behavior to email the user to confirm whether there
is nothing to address? Not sure as that behavior could be cumbersome
and perhaps assuming it's addressed automatically could be a good
default behavior.
> >> But I worry that saying that everything is automatically neither means
> >> that a patch sumbitter could very easily forget to do that and then we
> >> risk losing the value that the feature is supposed to add.
> >
> > Right, but if as you've said this is a feature intended for submitters rather
> > than maintainers, then surely we can assume that they will set the flag as
> > necessary since they'll ultimately benefit from it? I get that nudges (in the
> > psychology sense) are a thing but we shouldn't have to "force" people to use
> > this feature by turning it on for every single non-code submission they make to
> > a list and not providing a way to opt out of it. That's not cool and I don't
> > think it's all that productive either. Until we have sufficiently advanced AI/ML
> > to detect actionable comments, simply encouraging submitters to use this tool as
> > a way to manually track action items (rather than scribbling them in a diary or
> > whatever) seems more than okay.
>
> Maybe? Easy to miss an actionable comment if they're not automatically
> marked, I'd think.
>
> Anyway, I feel like we could go back and forth on this a bit, so maybe
> we should try to explore and see if there's a bigger set of potential
> solutions that might make both of us happier...
>
> How does this strike you?
>
> a) all old mail gets the NULL value.
>
> and
>
> b) Projects get a switch to enable/disable the feature. If you're a
> maintainer and you think these fields are more trouble than they're
> worth, ask your PW admin to make them disappear.
>
> and
>
> b) Users get a switch - maybe with "all automatically unaddressed",
> "NULL until manually marked" and "don't show me any of this ever"
> options (obviously with better names)
>
> That way, with basically no extra load on the db:
>
> - you can get comments on your patches only marked as unaddressed if
> you manually do so,
>
> - I can get all of the comments on my patches automatically unaddressed
> (which, in all honestly, is what I want - I absolutely _do_ lose
> track of email comments even just on the PW list!),
>
> - a patchwork project which has tracking mechanisms formalised in
> another way can turn them off entierly. (I'm thinking of the
> kernel-team mailing list in Ubuntu which has a strict
> 2-Acks-from-team-members requirement, and where people will vocally
> nack.)
>
> Thoughts?
I'm all for customization as it helps fit more users' needs. In
general, I think this is good behavior to follow. From your
description, I see that there should be precedence in the 'addressed'
comments system. As you describe, those user configs would apply to
comments of the submitter's patch. In the case that the patch
submitter replies, I believe the 'addressed' status of the comment by
the patch submitter should follow the behavior of the in-reply-to
comment submitter's preference. For example, consider the two
scenarios given that I have the feature configured to be disabled for
the sake of these examples:
Scenario 1:
1) Daniel sends out patch
2) Stephen replies ---> (comment is unaddressed automatically as per
Daniel's settings)
3) Daniel replies ---> (addressed is NULL until manually marked as per
Stephen's settings)
4) Raxel replies ---> (comment is unaddressed automatically as per
Daniel's settings)
Scenario 2:
1) Stephen sends out patch
2) Daniel replies ---> (addressed is NULL until manually marked as per
Stephen's settings)
3) Raxel replies ---> (addressed is NULL until manually marked as per
Stephen's settings)
4) Stephen replies ---> (addressed status for comment is disabled as
per Raxel's settings)
Based on the two scenarios, the settings of the patch submitter should
take absolute precedence in determining the 'addressed' status of the
comments to their patch. If the patch submitter replies to a comment,
then that 'addressed' status should be determined by the settings of
the in-reply-to comment submitter.
> Kind regards,
> Daniel
> >
> >> Another thing we could consider doing is making it opt-in by
> >> project. For projects that keep pw very tidy (I'm thinking
> >> e.g. https://patchwork.kernel.org/project/netdevbpf/list/) then
> >> the addressed/unaddressed thing might be more useful and less noisy than
> >> e.g. linuxppc which is a bit less well pruned.
> >
> > This does sound initially reasonable, but if these things are opt-in and
> > intended for the submitter then the value of making this configurable on a per-
> > project basis is significantly lower, right? In fact, it might even be actively
> > harmful since an opinionated maintainer (let's say you or I) could disable it
> > for an entire project (patchwork) meaning no submitter (Raxel?) could use this
> > supposedly submitter-focused feature to track action items even if they wanted
> > to.
> >
> > If we were going to do a global'ish config option, I'd probably make it a user
> > preference like the "show patch IDs" feature, so submitters that wanted to make
> > use of the feature would see the various flags while maintainer's who didn't
> > care for it could remain blissfully unaware. That assumes that the feature has
> > no value whatsoever for maintainers though, which I'm not sure is entirely true?
> >
> >> But, again, I see the un/addressed field as being for the submitter, not
> >> the maintainer. The maintainer can't trust it anyway because what the
> >> submitter considers 'addressed' and the maintainer considers 'addressed'
> >> could be very different. So really I see this as helping a submitter to
> >> track that there is nothing waiting on them.
> >
> > No arguments from me: I'm totally behind the feature as whole and understand the
> > motivation. I'm saying that submitters should be able to choose to set this
> > "action required" flag on individual comments as action items pop up, rather
> > than it being forced onto every single comment they send to the list. There are
> > a variety of ways they could do this, be it via the web UI, a custom tool run
> > locally ('pw-mark-actionable <msgid>'?), a script in your mail client, etc. It
> > won't be an issue.
> >
> >> > Change this workflow so that the 'addressed' field defaults to NULL.
> >> > This means maintainers or users must manually set this to False when
> >> > they're requesting additional feedback. This is currently possible via
> >> > the web UI or REST API. A future change will make it possible via a
> >> > custom mail header.
> >> >
> >> > Signed-off-by: Stephen Finucane <stephen at that.guru>
> >> > Cc: Raxel Gutierrez <raxel at google.com>
> >> > Cc: Daniel Axtens <dja at axtens.net>
> >> > ---
> >> > I think it's essential we make this change in order for this patch to be
> >> > useful. I also think it's okay to modify the migration in place, since
> >> > (a) we don't support deployment from master in production and (b) to the
> >> > best of my knowledge, setting a default, non-NULL value on a new column
> >> > is an expensive operation on certain databases (MySQL?) while changing
> >> > a column value for all rows is *definitely* expensive. The template work
> >> > could possibly do with tweaking. Feel free to advise if so.
> >>
> >> We totally can change the migration in place.
> >
> > Sweet.
> >
> > Stephen
> >
> >>
> >> Kind regards,
> >> Daniel
> >>
> >> > ---
> >> > docs/api/schemas/latest/patchwork.yaml | 2 ++
> >> > docs/api/schemas/patchwork.j2 | 2 ++
> >> > docs/api/schemas/v1.3/patchwork.yaml | 2 ++
> >> > htdocs/js/submission.js | 14 +++++++++++--
> >> > patchwork/migrations/0045_addressed_fields.py | 4 ++--
> >> > patchwork/models.py | 4 ++--
> >> > patchwork/templates/patchwork/submission.html | 20 +++++++++++++++----
> >> > 7 files changed, 38 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git docs/api/schemas/latest/patchwork.yaml docs/api/schemas/latest/patchwork.yaml
> >> > index e3bff990..2a98c179 100644
> >> > --- docs/api/schemas/latest/patchwork.yaml
> >> > +++ docs/api/schemas/latest/patchwork.yaml
> >> > @@ -1669,12 +1669,14 @@ components:
> >> > addressed:
> >> > title: Addressed
> >> > type: boolean
> >> > + nullable: true
> >> > CommentUpdate:
> >> > type: object
> >> > properties:
> >> > addressed:
> >> > title: Addressed
> >> > type: boolean
> >> > + nullable: true
> >> > CoverList:
> >> > type: object
> >> > properties:
> >> > diff --git docs/api/schemas/patchwork.j2 docs/api/schemas/patchwork.j2
> >> > index 3b4ad2f6..02aa9f72 100644
> >> > --- docs/api/schemas/patchwork.j2
> >> > +++ docs/api/schemas/patchwork.j2
> >> > @@ -1734,12 +1734,14 @@ components:
> >> > addressed:
> >> > title: Addressed
> >> > type: boolean
> >> > + nullable: true
> >> > CommentUpdate:
> >> > type: object
> >> > properties:
> >> > addressed:
> >> > title: Addressed
> >> > type: boolean
> >> > + nullable: true
> >> > {% endif %}
> >> > CoverList:
> >> > type: object
> >> > diff --git docs/api/schemas/v1.3/patchwork.yaml docs/api/schemas/v1.3/patchwork.yaml
> >> > index 6cbba646..0a9046a5 100644
> >> > --- docs/api/schemas/v1.3/patchwork.yaml
> >> > +++ docs/api/schemas/v1.3/patchwork.yaml
> >> > @@ -1669,12 +1669,14 @@ components:
> >> > addressed:
> >> > title: Addressed
> >> > type: boolean
> >> > + nullable: true
> >> > CommentUpdate:
> >> > type: object
> >> > properties:
> >> > addressed:
> >> > title: Addressed
> >> > type: boolean
> >> > + nullable: true
> >> > CoverList:
> >> > type: object
> >> > properties:
> >> > diff --git htdocs/js/submission.js htdocs/js/submission.js
> >> > index 47cffc82..c93c36ec 100644
> >> > --- htdocs/js/submission.js
> >> > +++ htdocs/js/submission.js
> >> > @@ -29,7 +29,17 @@ $( document ).ready(function() {
> >> > };
> >> > updateProperty(url, data, updateMessage).then(isSuccess => {
> >> > if (isSuccess) {
> >> > - $("div[class^='comment-status-bar-'][data-comment-id='"+commentId+"']").toggleClass("hidden");
> >> > + // The API won't accept anything but true or false, so we
> >> > + // always hide the -action-required element
> >> > + $("div[class='comment-status-bar-action-required'][data-comment-id='"+commentId+"']").addClass("hidden");
> >> > +
> >> > + if (event.target.value === "true") {
> >> > + $("div[class^='comment-status-bar-addressed'][data-comment-id='"+commentId+"']").removeClass("hidden");
> >> > + $("div[class^='comment-status-bar-unaddressed'][data-comment-id='"+commentId+"']").addClass("hidden");
> >> > + } else if (event.target.value === "false") {
> >> > + $("div[class^='comment-status-bar-addressed'][data-comment-id='"+commentId+"']").addClass("hidden");
> >> > + $("div[class^='comment-status-bar-unaddressed'][data-comment-id='"+commentId+"']").removeClass("hidden");
> >> > + }
> >> > }
> >> > })
> >> > });
> >> > @@ -59,4 +69,4 @@ $( document ).ready(function() {
> >> > toggleDiv("toggle-related-outside", "related-outside", "show from other projects");
> >> > });
> >> > }
> >> > -});
> >> > \ No newline at end of file
> >> > +});
> >> > diff --git patchwork/migrations/0045_addressed_fields.py patchwork/migrations/0045_addressed_fields.py
> >> > index ed3527bc..22887c33 100644
> >> > --- patchwork/migrations/0045_addressed_fields.py
> >> > +++ patchwork/migrations/0045_addressed_fields.py
> >> > @@ -13,11 +13,11 @@ class Migration(migrations.Migration):
> >> > migrations.AddField(
> >> > model_name='covercomment',
> >> > name='addressed',
> >> > - field=models.BooleanField(default=False),
> >> > + field=models.BooleanField(null=True),
> >> > ),
> >> > migrations.AddField(
> >> > model_name='patchcomment',
> >> > name='addressed',
> >> > - field=models.BooleanField(default=False),
> >> > + field=models.BooleanField(null=True),
> >> > ),
> >> > ]
> >> > diff --git patchwork/models.py patchwork/models.py
> >> > index 58e4c51e..6304b34d 100644
> >> > --- patchwork/models.py
> >> > +++ patchwork/models.py
> >> > @@ -657,7 +657,7 @@ class CoverComment(EmailMixin, models.Model):
> >> > related_query_name='comment',
> >> > on_delete=models.CASCADE,
> >> > )
> >> > - addressed = models.BooleanField(default=False)
> >> > + addressed = models.BooleanField(null=True)
> >> >
> >> > @property
> >> > def list_archive_url(self):
> >> > @@ -708,7 +708,7 @@ class PatchComment(EmailMixin, models.Model):
> >> > related_query_name='comment',
> >> > on_delete=models.CASCADE,
> >> > )
> >> > - addressed = models.BooleanField(default=False)
> >> > + addressed = models.BooleanField(null=True)
> >> >
> >> > @property
> >> > def list_archive_url(self):
> >> > diff --git patchwork/templates/patchwork/submission.html patchwork/templates/patchwork/submission.html
> >> > index 2238e82e..2814f3d5 100644
> >> > --- patchwork/templates/patchwork/submission.html
> >> > +++ patchwork/templates/patchwork/submission.html
> >> > @@ -285,7 +285,19 @@
> >> > <span class="message-date">{{ item.date }} UTC |
> >> > <a href="{% url 'comment-redirect' comment_id=item.id %}">#{{ forloop.counter }}</a>
> >> > </span>
> >> > - {% if item.addressed %}
> >> > + {% if item.addressed == None %}
> >> > + <div class="comment-status-bar-action-required" data-comment-id={{item.id}}>
> >> > + {% else %}
> >> > + <div class="comment-status-bar-action-required hidden" data-comment-id={{item.id}}>
> >> > + {% endif %}
> >> > + {% if editable or comment_is_editable %}
> >> > + <button class="comment-action-unaddressed text-warning" value="false">
> >> > + <span class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span>
> >> > + Mark Action Required
> >> > + </button>
> >> > + {% endif %}
> >> > + </div>
> >> > + {% if item.addressed == True %}
> >> > <div class="comment-status-bar-addressed" data-comment-id={{item.id}}>
> >> > {% else %}
> >> > <div class="comment-status-bar-addressed hidden" data-comment-id={{item.id}}>
> >> > @@ -301,10 +313,10 @@
> >> > </button>
> >> > {% endif %}
> >> > </div>
> >> > - {% if item.addressed %}
> >> > - <div class="comment-status-bar-unaddressed hidden" data-comment-id={{item.id}}>
> >> > - {% else %}
> >> > + {% if item.addressed == False %}
> >> > <div class="comment-status-bar-unaddressed" data-comment-id={{item.id}}>
> >> > + {% else %}
> >> > + <div class="comment-status-bar-unaddressed hidden" data-comment-id={{item.id}}>
> >> > {% endif %}
> >> > <div class="comment-status-label text-warning mx-3">
> >> > <span class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span>
> >> > --
> >> > 2.31.1
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list