diff mbox series

[v3,09/10] patch-detail: add label and button for comment addressed status

Message ID 20210813053127.2160595-10-raxel@google.com
State Superseded
Headers show
Series patch-detail: add unaddressed/addressed status to patch comments | expand
Related show

Commit Message

Raxel Gutierrez Aug. 13, 2021, 5:31 a.m. UTC 
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.

[1] https://imgur.com/NDfcPJy
[2] https://imgur.com/kIhohED

Signed-off-by: Raxel Gutierrez <raxel@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(-)

Comments

Daniel Axtens Aug. 16, 2021, 2:11 p.m. UTC | #1 
Raxel Gutierrez <raxel@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@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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
Daniel Axtens Aug. 17, 2021, 1:19 a.m. UTC | #2 
Raxel Gutierrez <raxel@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.
>
> [1] https://imgur.com/NDfcPJy
> [2] https://imgur.com/kIhohED
>
> Signed-off-by: Raxel Gutierrez <raxel@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");
> +    });
> +

If I hit this button repeatedly and quickly, I eventually get a 500
error from the server and the following on the console:

rest.js:22 PATCH http://localhost:8000/api/patches/17/comments/undefined/ 500 (Internal Server Error)
VM283:1 Uncaught (in promise) SyntaxError: Unexpected token < in JSON at position 0
    at JSON.parse (<anonymous>)
    at rest.js:28

The django log contains the following revealing snippet (and then we hit
the error from the API patch where we try to look up the linkname field
which doesn't exist):

web_1  | "PATCH /api/patches/17/comments/undefined/ HTTP/1.1" 500 169238
web_1  | Internal Server Error: /api/patches/17/comments/undefined/
web_1  | Traceback (most recent call last):
web_1  |   File "/home/patchwork/patchwork/patchwork/api/comment.py", line 101, in get_object
web_1  |     obj = queryset.get(id=int(comment_id))
web_1  | ValueError: invalid literal for int() with base 10: 'undefined'

Somehow I'm guessing the data field is getting rewritten and we're
losing the comment ID temporarily? I haven't traced through the JS to
figure out how but hopefully you can debug it from here. FWIW I'm using
Chrome on Ubuntu.

Kind regards,
Daniel

>      // 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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
Daniel Axtens Aug. 20, 2021, 12:22 a.m. UTC | #3 
Daniel Axtens <dja@axtens.net> writes:

> Raxel Gutierrez <raxel@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.
>>
>> [1] https://imgur.com/NDfcPJy
>> [2] https://imgur.com/kIhohED
>>
>> Signed-off-by: Raxel Gutierrez <raxel@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");
>> +    });
>> +
>
> If I hit this button repeatedly and quickly, I eventually get a 500
> error from the server and the following on the console:
>
> rest.js:22 PATCH http://localhost:8000/api/patches/17/comments/undefined/ 500 (Internal Server Error)
> VM283:1 Uncaught (in promise) SyntaxError: Unexpected token < in JSON at position 0
>     at JSON.parse (<anonymous>)
>     at rest.js:28
>
> The django log contains the following revealing snippet (and then we hit
> the error from the API patch where we try to look up the linkname field
> which doesn't exist):
>
> web_1  | "PATCH /api/patches/17/comments/undefined/ HTTP/1.1" 500 169238
> web_1  | Internal Server Error: /api/patches/17/comments/undefined/
> web_1  | Traceback (most recent call last):
> web_1  |   File "/home/patchwork/patchwork/patchwork/api/comment.py", line 101, in get_object
> web_1  |     obj = queryset.get(id=int(comment_id))
> web_1  | ValueError: invalid literal for int() with base 10: 'undefined'
>
> Somehow I'm guessing the data field is getting rewritten and we're
> losing the comment ID temporarily? I haven't traced through the JS to
> figure out how but hopefully you can debug it from here. FWIW I'm using
> Chrome on Ubuntu.

This no longer happens with the new rest.js. :D

>
> Kind regards,
> Daniel
>
>>      // 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@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/patchwork
diff mbox series

Patch

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