[PATCH 1/4] Make addressed/unaddressed workflow opt-in
Daniel Axtens
dja at axtens.net
Tue Aug 31 22:56:54 AEST 2021
>> >> 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 :)
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