diff mbox series

[v4,8/9] patch-detail: add label and button for comment addressed status

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

Commit Message

Raxel Gutierrez Aug. 20, 2021, 4:50 a.m. UTC 
Add new label to patch and cover 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 patch comment. For cover comments,
there are no delegates, so only maintainers and cover/cover comment
authors can edit the status of the cover comment. Before [1] and after
[2] images for reference.

Use new comment detail REST API endpoint to update the addressed field
value when "Addressed" or "Unaddressed" buttons are clicked. After a
successful request is made, the appearance of the comment status label
and buttons are toggled appropriately. For unsuccessful requests (e.g.
network errors prevents reaching the server), the error message is
populated to the page. A future improvement on this behavior is to add
a spinner to the button to provide a feedback that the request is in a
pending state until it's handled.

[1] https://imgur.com/3ZKzgjN
[2] https://imgur.com/hWZrrnM

Signed-off-by: Raxel Gutierrez <raxel@google.com>
---
 htdocs/css/style.css                          | 38 ++++++++++++
 htdocs/js/submission.js                       | 20 +++++++
 patchwork/templates/patchwork/submission.html | 60 +++++++++++++++----
 patchwork/views/patch.py                      |  4 +-
 4 files changed, 110 insertions(+), 12 deletions(-)

Comments

Daniel Axtens Aug. 23, 2021, 12:13 p.m. UTC | #1 
Raxel Gutierrez <raxel@google.com> writes:

> Add new label to patch and cover 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 patch comment. For cover comments,
> there are no delegates, so only maintainers and cover/cover comment
> authors can edit the status of the cover comment. Before [1] and after
> [2] images for reference.
>
> Use new comment detail REST API endpoint to update the addressed field
> value when "Addressed" or "Unaddressed" buttons are clicked. After a
> successful request is made, the appearance of the comment status label
> and buttons are toggled appropriately. For unsuccessful requests (e.g.
> network errors prevents reaching the server), the error message is
> populated to the page. A future improvement on this behavior is to add
> a spinner to the button to provide a feedback that the request is in a
> pending state until it's handled.

My only complaint with the current design is by the time I've scrolled
down far enough to see the comments, I can no longer see the top bar
where the messages appear. But hopefully we can sort that out later.

>
> [1] https://imgur.com/3ZKzgjN
> [2] https://imgur.com/hWZrrnM
>
> Signed-off-by: Raxel Gutierrez <raxel@google.com>
> ---
>  htdocs/css/style.css                          | 38 ++++++++++++
>  htdocs/js/submission.js                       | 20 +++++++
>  patchwork/templates/patchwork/submission.html | 60 +++++++++++++++----
>  patchwork/views/patch.py                      |  4 +-
>  4 files changed, 110 insertions(+), 12 deletions(-)
>
> diff --git a/htdocs/css/style.css b/htdocs/css/style.css
> index a2a2e3c3..9156aa6e 100644
> --- a/htdocs/css/style.css
> +++ b/htdocs/css/style.css
> @@ -1,4 +1,5 @@
>  :root {
> +    --light-color:rgb(247, 247, 247);
>      --success-color:rgb(92, 184, 92);
>      --warning-color:rgb(240, 173, 78);
>      --danger-color:rgb(217, 83, 79);
> @@ -27,6 +28,10 @@ pre {
>      top: 17em;
>  }
>  
> +.hidden {
> +    visibility: hidden;
> +}
> +
>  /* Bootstrap overrides */
>  
>  .navbar-inverse .navbar-brand > a {
> @@ -315,6 +320,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 9676f348..47cffc82 100644
> --- a/htdocs/js/submission.js
> +++ b/htdocs/js/submission.js
> @@ -1,4 +1,7 @@
> +import { updateProperty } from "./rest.js";
> +
>  $( document ).ready(function() {
> +    const patchMeta = document.getElementById("patch-meta");
>      function toggleDiv(link_id, headers_id, label_show, label_hide) {
>          const link = document.getElementById(link_id)
>          const headers = document.getElementById(headers_id)
> @@ -14,6 +17,23 @@ $( document ).ready(function() {
>          }
>      }
>  
> +    $("button[class^='comment-action']").click((event) => {
> +        const submissionType = patchMeta.dataset.submissionType;
> +        const submissionId = patchMeta.dataset.submissionId;
> +        const commentId = event.target.parentElement.dataset.commentId;
> +        const url = `/api/${submissionType}/${submissionId}/comments/${commentId}/`;
> +        const data = {'addressed': event.target.value} ;
> +        const updateMessage = {
> +            'error': "No comments updated",
> +            'success': "1 comment(s) updated",
> +        };
> +        updateProperty(url, data, updateMessage).then(isSuccess => {
> +            if (isSuccess) {
> +                $("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 36b15d0e..2238e82e 100644
> --- a/patchwork/templates/patchwork/submission.html
> +++ b/patchwork/templates/patchwork/submission.html
> @@ -5,6 +5,7 @@
>  {% load person %}
>  {% load patch %}
>  {% load static %}
> +{% load utils %}
>  
>  {% block headers %}
>    <script type="module" src="{% static "js/submission.js" %}"></script>
> @@ -19,7 +20,12 @@
>    <h1>{{ submission.name }}</h1>
>  </div>
>  
> -<table class="patch-meta">
> +<table
> +  id="patch-meta"
> +  class="patch-meta"
> +  data-submission-type={{submission|verbose_name_plural|lower}}
> +  data-submission-id={{submission.id}}
> +>
>   <tr>
>    <th>Message ID</th>
>    {% if submission.list_archive_url %}
> @@ -271,18 +277,50 @@
>  {% if forloop.first %}
>  <h2>Comments</h2>
>  {% endif %}
> -
> +{% is_editable item user as comment_is_editable %}
>  <a name="{{ item.id }}"></a>
>  <div class="submission-message">
> -<div class="meta">
> - <span>{{ item.submitter|personify:project }}</span>
> - <span class="message-date">{{ 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="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 or comment_is_editable %}
> +          <button class="comment-action-unaddressed text-warning" value="false">
> +            <span class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span>
> +            Mark 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 or comment_is_editable %}
> +          <button class="comment-action-addressed text-success" value="true">
> +            <span class="glyphicon glyphicon-ok-circle" aria-hidden="true"></span>
> +            Mark Addressed
> +          </button>
> +        {% endif %}
> +      </div>
> +  </div>
> +  <pre class="content">
> +  {{ item|commentsyntax }}
> +  </pre>
>  </div>
>  {% endfor %}
>  
> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
> index 3e6874ae..00b0147f 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(
> @@ -128,6 +129,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.rc2.250.ged5fa647cd-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 a2a2e3c3..9156aa6e 100644
--- a/htdocs/css/style.css
+++ b/htdocs/css/style.css
@@ -1,4 +1,5 @@ 
 :root {
+    --light-color:rgb(247, 247, 247);
     --success-color:rgb(92, 184, 92);
     --warning-color:rgb(240, 173, 78);
     --danger-color:rgb(217, 83, 79);
@@ -27,6 +28,10 @@  pre {
     top: 17em;
 }
 
+.hidden {
+    visibility: hidden;
+}
+
 /* Bootstrap overrides */
 
 .navbar-inverse .navbar-brand > a {
@@ -315,6 +320,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 9676f348..47cffc82 100644
--- a/htdocs/js/submission.js
+++ b/htdocs/js/submission.js
@@ -1,4 +1,7 @@ 
+import { updateProperty } from "./rest.js";
+
 $( document ).ready(function() {
+    const patchMeta = document.getElementById("patch-meta");
     function toggleDiv(link_id, headers_id, label_show, label_hide) {
         const link = document.getElementById(link_id)
         const headers = document.getElementById(headers_id)
@@ -14,6 +17,23 @@  $( document ).ready(function() {
         }
     }
 
+    $("button[class^='comment-action']").click((event) => {
+        const submissionType = patchMeta.dataset.submissionType;
+        const submissionId = patchMeta.dataset.submissionId;
+        const commentId = event.target.parentElement.dataset.commentId;
+        const url = `/api/${submissionType}/${submissionId}/comments/${commentId}/`;
+        const data = {'addressed': event.target.value} ;
+        const updateMessage = {
+            'error': "No comments updated",
+            'success': "1 comment(s) updated",
+        };
+        updateProperty(url, data, updateMessage).then(isSuccess => {
+            if (isSuccess) {
+                $("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 36b15d0e..2238e82e 100644
--- a/patchwork/templates/patchwork/submission.html
+++ b/patchwork/templates/patchwork/submission.html
@@ -5,6 +5,7 @@ 
 {% load person %}
 {% load patch %}
 {% load static %}
+{% load utils %}
 
 {% block headers %}
   <script type="module" src="{% static "js/submission.js" %}"></script>
@@ -19,7 +20,12 @@ 
   <h1>{{ submission.name }}</h1>
 </div>
 
-<table class="patch-meta">
+<table
+  id="patch-meta"
+  class="patch-meta"
+  data-submission-type={{submission|verbose_name_plural|lower}}
+  data-submission-id={{submission.id}}
+>
  <tr>
   <th>Message ID</th>
   {% if submission.list_archive_url %}
@@ -271,18 +277,50 @@ 
 {% if forloop.first %}
 <h2>Comments</h2>
 {% endif %}
-
+{% is_editable item user as comment_is_editable %}
 <a name="{{ item.id }}"></a>
 <div class="submission-message">
-<div class="meta">
- <span>{{ item.submitter|personify:project }}</span>
- <span class="message-date">{{ 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="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 or comment_is_editable %}
+          <button class="comment-action-unaddressed text-warning" value="false">
+            <span class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span>
+            Mark 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 or comment_is_editable %}
+          <button class="comment-action-addressed text-success" value="true">
+            <span class="glyphicon glyphicon-ok-circle" aria-hidden="true"></span>
+            Mark Addressed
+          </button>
+        {% endif %}
+      </div>
+  </div>
+  <pre class="content">
+  {{ item|commentsyntax }}
+  </pre>
 </div>
 {% endfor %}
 
diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
index 3e6874ae..00b0147f 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(
@@ -128,6 +129,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