Sorry for the noise, making some corrections and adding an idea :) On Mon, Aug 30, 2021 at 5:22 PM Raxel Gutierrez <raxelgutierre...@gmail.com> wrote: > > Hi, > > On Sun, Aug 29, 2021 at 9:05 AM Daniel Axtens <d...@axtens.net> wrote: > > > > Stephen Finucane <step...@that.guru> writes: > > > > > On Fri, 2021-08-27 at 13:50 +1000, Daniel Axtens wrote: > > >> Stephen Finucane <step...@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 <step...@that.guru> > > >> > Cc: Raxel Gutierrez <ra...@google.com> > > >> > Cc: Daniel Axtens <d...@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@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/patchwork _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
