[PATCH 1/4] Make addressed/unaddressed workflow opt-in
Stephen Finucane
stephen at that.guru
Thu Sep 2 02:19:32 AEST 2021
On Sun, 2021-08-29 at 23:04 +1000, Daniel Axtens 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).
> > >
> > > 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.
> > >
> > > I think it also makes sense for patches that add 'Acked-by:',
> > > 'Reviewed-by:' or 'Tested-by:' to be considered automatically addressed.
> > >
> > > 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.
Yes, please.
> 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.
I'm on the fence about this one. As noted in my earlier email, I don't think
allowing maintainers to entirely disable this user-centric feature is a good
precedent. Also, it doesn't seem entirely necessary given the below.
>
> 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)
So to rephrase, this would be a user preference checkbox to allow users to
automatically have any submitted comments marked as unaddressed, yes? If we can
have that default to False (i.e. opt-in) then yes, this would work for me. The
"don't show me any of this option" could/should probably be implemented as a
separate thing though I could live without this if the addressed/unaddressed
thing is something a contributor is opting into and is therefore likely to
actually tend to.
Want me to submit a follow-up patch to add this feature? I think we can merge
the series I have as-is (assuming I haven't missed anything) since we've agreed
that the field should default to NULL, which means we need those migration and
API schema changes.
> 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.)
Yup, all makes sense bar possibly the last point. Let me know what you think of
that. We can focus on the "automatically mark my comments as unaddressed" knob
in the interim.
Stephen
>
> Thoughts?
>
> 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
More information about the Patchwork
mailing list