[PATCH v3 09/10] patch-detail: add label and button for comment addressed status
Daniel Axtens
dja at axtens.net
Tue Aug 17 00:11:22 AEST 2021
Raxel Gutierrez <raxel at google.com> writes:
> Add new label to patch comments to show the status of whether they are
> addressed or not and add an adjacent button to allow users to change the
> status of the comment. Only users that can edit the patch (i.e. patch
> author, delegate, project maintainers) as well as comment authors can
> change the status of a comment. Before [1] and after [2] images for
> reference.
>
> Refactor both the message containers in the "Commit Message" and
> "Comments" to have the same styling through the `patch-message` class so
> that the headers are normalized to be left-aligned. Also, pass context
> about whether the patch is `editable` by the user to the patch-detail
> template.
>
> Use new comment detail REST API endpoint to update the addressed field
> value when "Addressed" or "Unaddressed" buttons are clicked. After, the
> request is made, the appearance of the comment status label and buttons
> are toggled appropriately.
[This comment fits firmly in the category of "things you are not
expected to know because it is part of the dark magic of Patchwork's db
optimisations", so please don't take it as a criticism of your patch!]
Please include the following change in your next revision of this patch:
diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
index ac9cb44c8d9b..00b0147ff57c 100644
--- a/patchwork/views/patch.py
+++ b/patchwork/views/patch.py
@@ -109,7 +109,8 @@ def patch_detail(request, project_id, msgid):
comments = patch.comments.all()
comments = comments.select_related('submitter')
- comments = comments.only('submitter', 'date', 'id', 'content', 'patch')
+ comments = comments.only('submitter', 'date', 'id', 'content', 'patch',
+ 'addressed')
if patch.related:
related_same_project = patch.related.patches.only(
This stops your patch from adding a round trip to the database for every
comment. We restrict the fields of the comments we query to the ones
listed in only(), and you've added a new field we use in the UI so we
need to add that to the list.
I'd really like to have an intermediate spinner state that is resolved
when we get the response back from the server. It's not a dealbreaker,
but if it's not overly complex to add it would be good to have.
I'll continue my review of this patch tomorrow.
Kind regards,
Daniel
>
> [1] https://imgur.com/NDfcPJy
> [2] https://imgur.com/kIhohED
>
> Signed-off-by: Raxel Gutierrez <raxel at google.com>
> ---
> htdocs/css/style.css | 53 +++++++++++++-
> htdocs/js/submission.js | 15 ++++
> patchwork/templates/patchwork/submission.html | 69 ++++++++++++++-----
> patchwork/views/patch.py | 1 +
> 4 files changed, 118 insertions(+), 20 deletions(-)
>
> diff --git a/htdocs/css/style.css b/htdocs/css/style.css
> index 46a91ee..ba83de4 100644
> --- a/htdocs/css/style.css
> +++ b/htdocs/css/style.css
> @@ -1,3 +1,9 @@
> +:root {
> + --light-color: #F7F7F7;
> + --warning-color: #f0ad4e;
> + --success-color: #5cb85c;
> +}
> +
> h2 {
> font-size: 25px;
> margin: 18px 0 18px 0;
> @@ -21,6 +27,10 @@ pre {
> top: 17em;
> }
>
> +.hidden {
> + visibility: hidden;
> +}
> +
> /* Bootstrap overrides */
>
> .navbar-inverse .navbar-brand > a {
> @@ -277,12 +287,18 @@ table.patch-meta tr th, table.patch-meta tr td {
> color: #f7977a;
> }
>
> -.comment .meta {
> +.patch-message .meta {
> + display: flex;
> + align-items: center;
> background: #f0f0f0;
> padding: 0.3em 0.5em;
> }
>
> -.comment .content {
> +.patch-message .message-date {
> + margin-left: 8px;
> +}
> +
> +.patch-message .content {
> border: 0;
> }
>
> @@ -294,6 +310,39 @@ table.patch-meta tr th, table.patch-meta tr td {
> font-family: "DejaVu Sans Mono", fixed;
> }
>
> +div[class^="comment-status-bar-"] {
> + display: flex;
> + flex-wrap: wrap;
> + align-items: center;
> +}
> +
> +.comment-status-label {
> + margin: 0px 8px;
> +}
> +
> +button[class^=comment-action] {
> + background-color: var(--light-color);
> + border-radius: 4px;
> +}
> +
> +.comment-action-addressed {
> + border-color: var(--success-color);
> +}
> +
> +.comment-action-unaddressed {
> + border-color: var(--warning-color);
> +}
> +
> +.comment-action-addressed:hover {
> + background-color: var(--success-color);
> + color: var(--light-color);
> +}
> +
> +.comment-action-unaddressed:hover {
> + background-color: var(--warning-color);
> + color: var(--light-color);
> +}
> +
> .quote {
> color: #007f00;
> }
> diff --git a/htdocs/js/submission.js b/htdocs/js/submission.js
> index 9676f34..27e4a02 100644
> --- a/htdocs/js/submission.js
> +++ b/htdocs/js/submission.js
> @@ -1,3 +1,5 @@
> +import { updateProperty } from "./rest.js";
> +
> $( document ).ready(function() {
> function toggleDiv(link_id, headers_id, label_show, label_hide) {
> const link = document.getElementById(link_id)
> @@ -14,6 +16,19 @@ $( document ).ready(function() {
> }
> }
>
> + $("button[class^='comment-action']").click((event) => {
> + const patchId = document.getElementById("patch").dataset.patchId;
> + const commentId = event.target.parentElement.dataset.commentId;
> + const url = "/api/patches/" + patchId + "/comments/" + commentId + "/";
> + const data = {'addressed': event.target.value} ;
> + const updateMessage = {
> + 'none': "No comments updated",
> + 'some': "1 comment(s) updated",
> + };
> + updateProperty(url, data, updateMessage);
> + $("div[class^='comment-status-bar-'][data-comment-id='"+commentId+"']").toggleClass("hidden");
> + });
> +
> // Click listener to show/hide headers
> document.getElementById("toggle-patch-headers").addEventListener("click", function() {
> toggleDiv("toggle-patch-headers", "patch-headers");
> diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
> index 66efa0b..1ee3436 100644
> --- a/patchwork/templates/patchwork/submission.html
> +++ b/patchwork/templates/patchwork/submission.html
> @@ -257,14 +257,14 @@
> {% else %}
> <h2>Message</h2>
> {% endif %}
> -<div class="comment">
> -<div class="meta">
> - <span>{{ submission.submitter|personify:project }}</span>
> - <span class="pull-right">{{ submission.date }} UTC</span>
> -</div>
> -<pre class="content">
> -{{ submission|commentsyntax }}
> -</pre>
> +<div class="patch-message">
> + <div class="meta">
> + <span>{{ submission.submitter|personify:project }}</span>
> + <span class="message-date">{{ submission.date }} UTC</span>
> + </div>
> + <pre class="content">
> + {{ submission|commentsyntax }}
> + </pre>
> </div>
>
> {% for item in comments %}
> @@ -273,15 +273,48 @@
> {% endif %}
>
> <a name="{{ item.id }}"></a>
> -<div class="comment">
> -<div class="meta">
> - <span>{{ item.submitter|personify:project }}</span>
> - <span class="pull-right">{{ item.date }} UTC | <a href="{% url 'comment-redirect' comment_id=item.id %}"
> - >#{{ forloop.counter }}</a></span>
> -</div>
> -<pre class="content">
> -{{ item|commentsyntax }}
> -</pre>
> +<div class="patch-message">
> + <div class="meta">
> + {{ item.submitter|personify:project }}
> + <span class="message-date">{{ item.date }} UTC |
> + <a href="{% url 'comment-redirect' comment_id=item.id %}">#{{ forloop.counter }}</a>
> + </span>
> + {% if item.addressed %}
> + <div class="comment-status-bar-addressed" data-comment-id={{item.id}}>
> + {% else %}
> + <div class="comment-status-bar-addressed hidden" data-comment-id={{item.id}}>
> + {% endif %}
> + <div class="comment-status-label text-success mx-3">
> + <span class="glyphicon glyphicon-ok-circle" aria-hidden="true"></span>
> + Addressed
> + </div>
> + {% if editable %}
> + <button class="comment-action-unaddressed text-warning" value="false">
> + <span class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span>
> + Unaddressed
> + </button>
> + {% endif %}
> + </div>
> + {% if item.addressed %}
> + <div class="comment-status-bar-unaddressed hidden" data-comment-id={{item.id}}>
> + {% else %}
> + <div class="comment-status-bar-unaddressed" 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>
> + Unaddressed
> + </div>
> + {% if editable %}
> + <button class="comment-action-addressed text-success" value="true">
> + <span class="glyphicon glyphicon-ok-circle" aria-hidden="true"></span>
> + Addressed
> + </button>
> + {% endif %}
> + </div>
> + </div>
> + <pre class="content">
> + {{ item|commentsyntax }}
> + </pre>
> </div>
> {% endfor %}
>
> @@ -290,7 +323,7 @@
> {% include "patchwork/partials/download-buttons.html" %}
> <h2>Patch</h2>
> </div>
> -<div id="patch" class="patch">
> +<div id="patch" data-patch-id={{submission.id}} class="patch">
> <pre class="content">
> {{ submission|patchsyntax }}
> </pre>
> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
> index 3e6874a..ac9cb44 100644
> --- a/patchwork/views/patch.py
> +++ b/patchwork/views/patch.py
> @@ -128,6 +128,7 @@ def patch_detail(request, project_id, msgid):
> patch.check_set.all().select_related('user'),
> )
> context['submission'] = patch
> + context['editable'] = editable
> context['patchform'] = form
> context['createbundleform'] = createbundleform
> context['project'] = patch.project
> --
> 2.33.0.rc1.237.g0d66db33f3-goog
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list