[PATCH 1/4] Make addressed/unaddressed workflow opt-in
Raxel Gutierrez
raxelgutierrez09 at gmail.com
Tue Aug 31 07:32:25 AEST 2021
Sorry for the noise, making some corrections and adding an idea :)
On Mon, Aug 30, 2021 at 5:22 PM Raxel Gutierrez
<raxelgutierrez09 at gmail.com> wrote:
>
> 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.
Sorry for the noise, I need to correct the following. I mean to say
that patch submitters can simply mark a comment as "Addressed" to
signify it's "not an actionable comment."
> > >> 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)
Also, while I'm here I think that these User configs can be
labeled/defined as 'Automatic', 'Manual', and 'Disabled'.
> > 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