Stephen Finucane <step...@that.guru> writes:
> On Sun, 2021-08-29 at 23:04 +1000, Daniel Axtens wrote:
>> Stephen Finucane <step...@that.guru> writes:
>>
>> > On Fri, 2021-08-27 at 13:50 +1000, Daniel Axtens wrote:
>> > > Stephen Finucane <step...@that.guru> writes:
>> > >
>> > > > The current workflow for the address/unaddressed attribute of comments
>> > > > sets all comments to unaddressed by default. This is unintuitive, as it
>> > > > assumes that all comments are actionable items. It also imposes a
>> > > > massive burden on maintainers, who will need to manually sift through
>> > > > every single comment received to a list and manually set the
>> > > > non-actionable items as "addressed".
>> > >
>> > > I agree that not every email is an actionable item.
>> > >
>> > > I'm not convinced it's a burden on maintainers specifically. The comment
>> > > can also be marked as addressed by the patch submitter. Also,
>> > > maintainers (and everyone else) are free to ignore the field (and every
>> > > other piece of data stored on patchwork).
>> > >
>> > > 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
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
