[PATCH 1/4] Make addressed/unaddressed workflow opt-in

Stephen Finucane stephen at that.guru
Fri Aug 27 18:26:48 AEST 2021


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.

> 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