[PATCH 1/4] Make addressed/unaddressed workflow opt-in
Raxel Gutierrez
raxelgutierrez09 at gmail.com
Wed Sep 1 01:21:07 AEST 2021
On Tue, Aug 31, 2021 at 8:56 AM Daniel Axtens <dja at axtens.net> wrote:
>
> >> >> 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.
> >
>
> (I originally totally misunderstood this, thanks for clarifying it in
> your subsequent email!)
>
> >> >> 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.
> >
>
> Yeah good point. Let's not be overly clever - let's leave tags for now.
>
> >> >> 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.
>
> Hmm interesting. I had only considered it operating on the basis of the
> preferences of the submitter of the patch/cover letter.
>
> Using your example where I want all comments marked as unaddressed,
> Stephen wants NULLs and you want the feature disabled, my thoughts were:
>
> - If I send a patch, all replies to my patch - regardless of who
> sends them - marked as unaddressed.
>
> - If Stephen sends a patch, all replies are marked with the null
> state, regardless of who sends them.
>
> - If you send a patch, the un/addressed states are just not shown,
> regardless of who sends them.
>
> I am a little worried that trying to support threaded comment rules
> starts to operate in surprising ways for people (and would be a
> nightmare to implement) but I'm happy to be persuaded otherwise.
>
> I guess I can muck about with some code for (the simple version of) this
> over the next few days. Like the IETF I like the idea of decisions being
> made via "rough consensus and working code" so I guess it's time to lead
> by example :)
I agree, that patch submitter's preference seems as the best/safest
default. I just don't feel the best about comments from the patch
submitter on their own patches because that would mean that the
expected behavior could conflict (e.g. if settings differ from patch
submitter) with the settings/"concept" set by the person who is
supposed to act on the comment (i.e. non-submitters). Now that I think
about it, the behavior also safeguards against the patch submitter
being the first person to comment (and review?) on their own patch.
For now though, I believe that the behavior you described works
effectively and can be built off of. We can throw more ideas around on
how to make in-reply-to comments actionable as I believe that's
something valuable as well when thinking ahead to integrating the
'addressed' status to the TODO list.
> Kind regards,
> Daniel
>
> >
> >> 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