>> >> 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 <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
