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

Daniel Axtens dja at axtens.net
Fri Aug 20 10:22:04 AEST 2021


Daniel Axtens <dja at axtens.net> writes:

> 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.
>>
>> [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");
>> +    });
>> +
>
> 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 at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/patchwork


More information about the Patchwork mailing list