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 <step...@that.guru>
Cc: Raxel Gutierrez <ra...@google.com>
Cc: Daniel Axtens <d...@axtens.net>
---
I think it's essential we make this change in order for this patch to be
useful. I also think it's okay to modify the migration in place, since
(a) we don't support deployment from master in production and (b) to the
best of my knowledge, setting a default, non-NULL value on a new column
is an expensive operation on certain databases (MySQL?) while changing
a column value for all rows is *definitely* expensive. The template work
could possibly do with tweaking. Feel free to advise if so.
---
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
