| Message ID | 20210826171831.547578-2-stephen@that.guru |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Make addressed/unaddressed workflow opt-in | expand |
Stephen Finucane <stephen@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. 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. 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. > 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@that.guru> > Cc: Raxel Gutierrez <raxel@google.com> > Cc: Daniel Axtens <dja@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. 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
On Fri, 2021-08-27 at 13:50 +1000, Daniel Axtens wrote: > Stephen Finucane <stephen@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@that.guru> > > Cc: Raxel Gutierrez <raxel@google.com> > > Cc: Daniel Axtens <dja@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
Stephen Finucane <stephen@that.guru> writes: > On Fri, 2021-08-27 at 13:50 +1000, Daniel Axtens wrote: >> Stephen Finucane <stephen@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. 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? 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@that.guru> >> > Cc: Raxel Gutierrez <raxel@google.com> >> > Cc: Daniel Axtens <dja@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
Hi, On Sun, Aug 29, 2021 at 9:05 AM Daniel Axtens <dja@axtens.net> wrote: > > Stephen Finucane <stephen@that.guru> writes: > > > On Fri, 2021-08-27 at 13:50 +1000, Daniel Axtens wrote: > >> Stephen Finucane <stephen@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. > >> 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) > > 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 <stephen@that.guru> > >> > Cc: Raxel Gutierrez <raxel@google.com> > >> > Cc: Daniel Axtens <dja@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
Sorry for the noise, making some corrections and adding an idea :) On Mon, Aug 30, 2021 at 5:22 PM Raxel Gutierrez <raxelgutierrez09@gmail.com> wrote: > > Hi, > > On Sun, Aug 29, 2021 at 9:05 AM Daniel Axtens <dja@axtens.net> wrote: > > > > Stephen Finucane <stephen@that.guru> writes: > > > > > On Fri, 2021-08-27 at 13:50 +1000, Daniel Axtens wrote: > > >> Stephen Finucane <stephen@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 <stephen@that.guru> > > >> > Cc: Raxel Gutierrez <raxel@google.com> > > >> > Cc: Daniel Axtens <dja@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
>> >> 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@that.guru> >> >> > Cc: Raxel Gutierrez <raxel@google.com> >> >> > Cc: Daniel Axtens <dja@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
On Tue, Aug 31, 2021 at 8:56 AM Daniel Axtens <dja@axtens.net> wrote: > > >> >> 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 :) I agree, that patch submitter's preference seems as the best/safest default. I just don't feel the best about comments from the patch submitter on their own patches because that would mean that the expected behavior could conflict (e.g. if settings differ from patch submitter) with the settings/"concept" set by the person who is supposed to act on the comment (i.e. non-submitters). Now that I think about it, the behavior also safeguards against the patch submitter being the first person to comment (and review?) on their own patch. For now though, I believe that the behavior you described works effectively and can be built off of. We can throw more ideas around on how to make in-reply-to comments actionable as I believe that's something valuable as well when thinking ahead to integrating the 'addressed' status to the TODO list. > 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@that.guru> > >> >> > Cc: Raxel Gutierrez <raxel@google.com> > >> >> > Cc: Daniel Axtens <dja@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
On Sun, 2021-08-29 at 23:04 +1000, Daniel Axtens wrote: > Stephen Finucane <stephen@that.guru> writes: > > > On Fri, 2021-08-27 at 13:50 +1000, Daniel Axtens wrote: > > > Stephen Finucane <stephen@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@that.guru> > > > > Cc: Raxel Gutierrez <raxel@google.com> > > > > Cc: Daniel Axtens <dja@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
Stephen Finucane <stephen@that.guru> writes: > On Sun, 2021-08-29 at 23:04 +1000, Daniel Axtens wrote: >> Stephen Finucane <stephen@that.guru> writes: >> >> > On Fri, 2021-08-27 at 13:50 +1000, Daniel Axtens wrote: >> > > Stephen Finucane <stephen@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. Yeah, that sounds like a good plan. I will have another skim through the actual code of the series now. >> > > > 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> I don't love that this is the same icon and the same colour as marking unaddressed. How do you feel about one of these? From my most preferred to my least preferred: glyphicon-flag, glyphicon-pushpin, glyphicon-bell, glyphicon-exclamation-sign, glyhpicon-hourglass (per https://www.w3schools.com/bootstrap/bootstrap_ref_comp_glyphs.asp) I'm not sure what colour, I'd say probably just regular text rather than coloured text - I just really want to avoid confusing unmarked with unaddressed. Kind regards, Daniel
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>
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". 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@that.guru> Cc: Raxel Gutierrez <raxel@google.com> Cc: Daniel Axtens <dja@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. --- 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(-)