diff mbox series

[v2,4/5] patch-detail: add functionality for comment status updates

Message ID 20210728181718.3613124-5-raxel@google.com
State Changes Requested
Headers show
Series patch-detail: add unaddressed/addressed status to patch comments | expand
Related show

Commit Message

Raxel Gutierrez July 28, 2021, 6:17 p.m. UTC 
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.

Signed-off-by: Raxel Gutierrez <raxel@google.com>
---
 htdocs/css/style.css                          | 15 ++++-
 htdocs/js/submission.js                       | 52 ++++++++++++++++
 patchwork/templates/patchwork/submission.html | 61 ++++++-------------
 templates/base.html                           |  2 +-
 4 files changed, 85 insertions(+), 45 deletions(-)
 create mode 100644 htdocs/js/submission.js

Comments

Daniel Axtens Aug. 6, 2021, 2:56 a.m. UTC | #1 
Raxel Gutierrez <raxel@google.com> writes:

> 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.

As a general note, if you're able to split the code movement parts out
from the new code parts, I am happy to merge the refactoring earlier.
That will also make it easier for me to do reviews.

Overall this patch (and by extension the series) seems to work in a
reasonable way.

I go back and forth on whether this needs to sit behind
a project gate flag; on one hand it's possibly confusing for a submitter
to have state tracked here that the maintainer doesn't care about. On
the other hand, it isn't a massive UI change and maybe there's no real
harm in leaving a feature around that people don't use. Let's stick with
having it on everywhere for now.

> Signed-off-by: Raxel Gutierrez <raxel@google.com>
> ---
>  htdocs/css/style.css                          | 15 ++++-
>  htdocs/js/submission.js                       | 52 ++++++++++++++++
>  patchwork/templates/patchwork/submission.html | 61 ++++++-------------
>  templates/base.html                           |  2 +-
>  4 files changed, 85 insertions(+), 45 deletions(-)
>  create mode 100644 htdocs/js/submission.js
>
> diff --git a/htdocs/css/style.css b/htdocs/css/style.css
> index ff34ff5..2a5c81f 100644
> --- a/htdocs/css/style.css
> +++ b/htdocs/css/style.css
> @@ -27,6 +27,10 @@ pre {
>      top: 17em;
>  }
>  
> +.hidden {
> +    visibility: hidden;
> +}
> +
>  /* Bootstrap overrides */
>  
>  .navbar-inverse .navbar-brand > a {
> @@ -306,7 +310,7 @@ table.patchmeta tr th, table.patchmeta tr td {
>      font-family: "DejaVu Sans Mono", fixed;
>  }
>  
> -#comment-status-bar {
> +div[id^="comment-status-bar-"] {
>      display: flex;
>      flex-wrap: wrap;
>      align-items: center;
> @@ -316,7 +320,7 @@ table.patchmeta tr th, table.patchmeta tr td {
>      margin: 0px 8px;
>  }
>  
> -#comment-action-addressed, #comment-action-unaddressed {
> +button[id^=comment-action] {
>      background-color: var(--light-color);
>      border-radius: 4px;
>  }
> @@ -329,11 +333,16 @@ table.patchmeta tr th, table.patchmeta tr td {
>      border-color: var(--warning-color);
>  }
>  
> -#comment-action-addressed:hover, #comment-action-unaddressed:hover {
> +#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
> new file mode 100644
> index 0000000..8894890
> --- /dev/null
> +++ b/htdocs/js/submission.js
> @@ -0,0 +1,52 @@
> +import { updateProperty } from "./rest.js";
> +
> +$( document ).ready(function() {
> +    function toggleDiv(link_id, headers_id, label_show, label_hide) {
> +        const link = document.getElementById(link_id)
> +        const headers = document.getElementById(headers_id)
> +
> +        const hidden = headers.style['display'] == 'none';
> +
> +        if (hidden) {
> +            link.innerHTML = label_hide || 'hide';
> +            headers.style['display'] = 'block';
> +        } else {
> +            link.innerHTML = label_show || 'show';
> +            headers.style['display'] = 'none';
> +        }
> +    }
> +
> +    $("button[id^='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 updated",
> +        };
> +        updateProperty(url, data, updateMessage);
> +        $("div[id^='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");
> +    });
> +
> +    // Click listener to expand/collapse series
> +    document.getElementById("toggle-patch-series").addEventListener("click", function() {
> +        toggleDiv("toggle-patch-series", "patch-series", "expand", "collapse");
> +    });
> +
> +    // Click listener to show/hide related patches
> +    document.getElementById("toggle-related").addEventListener("click", function() {
> +        toggleDiv("toggle-related", "related");
> +    });
> +
> +    // Click listener to show/hide related patches from different projects
> +    document.getElementById("toggle-related-outside").addEventListener("click", function() {
> +        toggleDiv("toggle-related-outside", "related-outside", "show from other projects");
> +    });
> +
> +});

This code threw a bunch of JS errors for me because I looked at a patch
that didn't have any related patches. I ended up doing the following:

diff --git a/htdocs/js/submission.js b/htdocs/js/submission.js
index 88948902ec56..56fbac64269e 100644
--- a/htdocs/js/submission.js
+++ b/htdocs/js/submission.js
@@ -40,13 +40,18 @@ $( document ).ready(function() {
     });
 
     // Click listener to show/hide related patches
-    document.getElementById("toggle-related").addEventListener("click", function() {
-        toggleDiv("toggle-related", "related");
-    });
-
+    var related = document.getElementById("toggle-related");
+    if (related) {
+        related.addEventListener("click", function() {
+            toggleDiv("toggle-related", "related");
+        });
+    }
     // Click listener to show/hide related patches from different projects
-    document.getElementById("toggle-related-outside").addEventListener("click", function() {
-        toggleDiv("toggle-related-outside", "related-outside", "show from other projects");
-    });
+    var related_outside = document.getElementById("toggle-related-outside");
+    if (related_outside) {
+        related_outside.addEventListener("click", function() {
+            toggleDiv("toggle-related-outside", "related-outside", "show from other projects");
+        });
+    }
 
 });

Apologies for the shocking JS, I'm not a front-end person at all.


> \ No newline at end of file
> diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
> index 4109442..e3a8909 100644
> --- a/patchwork/templates/patchwork/submission.html
> +++ b/patchwork/templates/patchwork/submission.html
> @@ -5,29 +5,15 @@
>  {% load syntax %}
>  {% load person %}
>  {% load patch %}
> +{% load static %}
> +
> +{% block headers %}
> +  <script type="module" src="{% static "js/submission.js" %}"></script>
> +{% endblock %}
>  
>  {% block title %}{{submission.name}}{% endblock %}
>  
>  {% block body %}
> -<script>
> -function toggle_div(link_id, headers_id, label_show, label_hide)
> -{
> -    var link = document.getElementById(link_id)
> -    var headers = document.getElementById(headers_id)
> -
> -    var hidden = headers.style['display'] == 'none';
> -
> -    if (hidden) {
> -        link.innerHTML = label_hide || 'hide';
> -        headers.style['display'] = 'block';
> -    } else {
> -        link.innerHTML = label_show || 'show';
> -        headers.style['display'] = 'none';
> -    }
> -
> -}
> -</script>
> -
>  <div>
>    {% include "patchwork/partials/download-buttons.html" %}
>    <h1>{{ submission.name }}</h1>
> @@ -63,10 +49,7 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
>   <tr>
>    <th>Headers</th>
>    <td>
> -    <button
> -      id="toggle-patch-headers"
> -      onclick="javascript:toggle_div('toggle-patch-headers', 'patch-headers')"
> -      >show</button>
> +    <button id="toggle-patch-headers">show</button>
>      <div id="patch-headers" class="patch-headers" style="display:none;">
>        <pre>{{submission.headers}}</pre>
>      </div>
> @@ -79,10 +62,7 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
>     <a href="{% url 'patch-list' project_id=project.linkname %}?series={{ submission.series.id }}">
>      {{ submission.series.name }}
>     </a>
> -   <button
> -    id="toggle-patch-series"
> -    onclick="javascript:toggle_div('toggle-patch-series', 'patch-series', 'expand', 'collapse')"
> -    >expand</button>
> +   <button id="toggle-patch-series">expand</button>
>     <div id="patch-series" class="submission-list" style="display:none;">
>      <ul>
>       {% with submission.series.cover_letter as cover %}
> @@ -118,10 +98,7 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
>   <tr>
>    <th>Related</th>
>    <td>
> -    <button
> -      id="toggle-related"
> -      onclick="javascript:toggle_div('toggle-related', 'related')"
> -      >show</button>
> +    <button id="toggle-related">show</button>
>      <div id="related" class="submission-list" style="display:none;">
>        <ul>
>        {% for sibling in related_same_project %}
> @@ -134,10 +111,7 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
>          </li>
>        {% endfor %}
>        {% if related_different_project %}
> -        <button
> -          id="toggle-related-outside"
> -          onclick="javascript:toggle_div('toggle-related-outside', 'related-outside', 'show from other projects')"
> -          >show from other projects</button>
> +        <button id="toggle-related-outside">show from other projects</button>
>          <div id="related-outside" class="submission-list" style="display:none;">
>          {% for sibling in related_outside %}
>            <li>
> @@ -306,32 +280,37 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
>        <a href="{% url 'comment-redirect' comment_id=item.id %}">#{{ forloop.counter }}</a>
>      </span>
>      {% if item.addressed %}
> -      <div id="comment-status-bar">
> +      <div id="comment-status-bar-addressed" data-comment-id={{item.id}}>
> +    {% else %}
> +      <div id="comment-status-bar-addressed" class="hidden" data-comment-id={{item.id}}>
> +    {% endif %}
>          <div id="comment-status-label" class="text-success mx-3">
>            <span id="comment-status-icon" class="glyphicon glyphicon-ok-circle" aria-hidden="true"></span>
>            Addressed
>          </div>
>          {% if editable %}
> -          <button id="comment-action-unaddressed" class="text-warning">
> +          <button id="comment-action-unaddressed" class="text-warning" value="false">
>              <span id="comment-action-icon" class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span>
>              Unaddressed
>            </button>
>          {% endif %}
>        </div>
> +    {% if item.addressed %}
> +      <div id="comment-status-bar-unaddressed" class="hidden" data-comment-id={{item.id}}>
>      {% else %}
> -      <div id="comment-status-bar">
> +      <div id="comment-status-bar-unaddressed" data-comment-id={{item.id}}>
> +    {% endif %}
>          <div id="comment-status-label" class="text-warning mx-3">
>            <span id="comment-status-icon" class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span>
>            Unaddressed
>          </div>
>          {% if editable %}
> -          <button id="comment-action-addressed" class="text-success">
> +          <button id="comment-action-addressed" class="text-success" value="true">
>              <span id="comment-action-icon" class="glyphicon glyphicon-ok-circle" aria-hidden="true"></span>
>              Addressed
>            </button>
>          {% endif %}
>        </div>
> -    {% endif %}
>    </div>
>    <pre class="content">
>    {{ item|commentsyntax }}
> @@ -344,7 +323,7 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
>    {% 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/templates/base.html b/templates/base.html
> index 8accb4c..a95a11b 100644
> --- a/templates/base.html
> +++ b/templates/base.html
> @@ -113,7 +113,7 @@
>    {% endfor %}
>    </div>
>  {% endif %}
> -  <div class="container-fluid">
> +  <div id="main-content" class="container-fluid">
>  {% block body %}
>  {% endblock %}
>    </div>
> -- 
> 2.32.0.554.ge1b32706d8-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 ff34ff5..2a5c81f 100644
--- a/htdocs/css/style.css
+++ b/htdocs/css/style.css
@@ -27,6 +27,10 @@  pre {
     top: 17em;
 }
 
+.hidden {
+    visibility: hidden;
+}
+
 /* Bootstrap overrides */
 
 .navbar-inverse .navbar-brand > a {
@@ -306,7 +310,7 @@  table.patchmeta tr th, table.patchmeta tr td {
     font-family: "DejaVu Sans Mono", fixed;
 }
 
-#comment-status-bar {
+div[id^="comment-status-bar-"] {
     display: flex;
     flex-wrap: wrap;
     align-items: center;
@@ -316,7 +320,7 @@  table.patchmeta tr th, table.patchmeta tr td {
     margin: 0px 8px;
 }
 
-#comment-action-addressed, #comment-action-unaddressed {
+button[id^=comment-action] {
     background-color: var(--light-color);
     border-radius: 4px;
 }
@@ -329,11 +333,16 @@  table.patchmeta tr th, table.patchmeta tr td {
     border-color: var(--warning-color);
 }
 
-#comment-action-addressed:hover, #comment-action-unaddressed:hover {
+#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
new file mode 100644
index 0000000..8894890
--- /dev/null
+++ b/htdocs/js/submission.js
@@ -0,0 +1,52 @@ 
+import { updateProperty } from "./rest.js";
+
+$( document ).ready(function() {
+    function toggleDiv(link_id, headers_id, label_show, label_hide) {
+        const link = document.getElementById(link_id)
+        const headers = document.getElementById(headers_id)
+
+        const hidden = headers.style['display'] == 'none';
+
+        if (hidden) {
+            link.innerHTML = label_hide || 'hide';
+            headers.style['display'] = 'block';
+        } else {
+            link.innerHTML = label_show || 'show';
+            headers.style['display'] = 'none';
+        }
+    }
+
+    $("button[id^='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 updated",
+        };
+        updateProperty(url, data, updateMessage);
+        $("div[id^='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");
+    });
+
+    // Click listener to expand/collapse series
+    document.getElementById("toggle-patch-series").addEventListener("click", function() {
+        toggleDiv("toggle-patch-series", "patch-series", "expand", "collapse");
+    });
+
+    // Click listener to show/hide related patches
+    document.getElementById("toggle-related").addEventListener("click", function() {
+        toggleDiv("toggle-related", "related");
+    });
+
+    // Click listener to show/hide related patches from different projects
+    document.getElementById("toggle-related-outside").addEventListener("click", function() {
+        toggleDiv("toggle-related-outside", "related-outside", "show from other projects");
+    });
+
+});
\ No newline at end of file
diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
index 4109442..e3a8909 100644
--- a/patchwork/templates/patchwork/submission.html
+++ b/patchwork/templates/patchwork/submission.html
@@ -5,29 +5,15 @@ 
 {% load syntax %}
 {% load person %}
 {% load patch %}
+{% load static %}
+
+{% block headers %}
+  <script type="module" src="{% static "js/submission.js" %}"></script>
+{% endblock %}
 
 {% block title %}{{submission.name}}{% endblock %}
 
 {% block body %}
-<script>
-function toggle_div(link_id, headers_id, label_show, label_hide)
-{
-    var link = document.getElementById(link_id)
-    var headers = document.getElementById(headers_id)
-
-    var hidden = headers.style['display'] == 'none';
-
-    if (hidden) {
-        link.innerHTML = label_hide || 'hide';
-        headers.style['display'] = 'block';
-    } else {
-        link.innerHTML = label_show || 'show';
-        headers.style['display'] = 'none';
-    }
-
-}
-</script>
-
 <div>
   {% include "patchwork/partials/download-buttons.html" %}
   <h1>{{ submission.name }}</h1>
@@ -63,10 +49,7 @@  function toggle_div(link_id, headers_id, label_show, label_hide)
  <tr>
   <th>Headers</th>
   <td>
-    <button
-      id="toggle-patch-headers"
-      onclick="javascript:toggle_div('toggle-patch-headers', 'patch-headers')"
-      >show</button>
+    <button id="toggle-patch-headers">show</button>
     <div id="patch-headers" class="patch-headers" style="display:none;">
       <pre>{{submission.headers}}</pre>
     </div>
@@ -79,10 +62,7 @@  function toggle_div(link_id, headers_id, label_show, label_hide)
    <a href="{% url 'patch-list' project_id=project.linkname %}?series={{ submission.series.id }}">
     {{ submission.series.name }}
    </a>
-   <button
-    id="toggle-patch-series"
-    onclick="javascript:toggle_div('toggle-patch-series', 'patch-series', 'expand', 'collapse')"
-    >expand</button>
+   <button id="toggle-patch-series">expand</button>
    <div id="patch-series" class="submission-list" style="display:none;">
     <ul>
      {% with submission.series.cover_letter as cover %}
@@ -118,10 +98,7 @@  function toggle_div(link_id, headers_id, label_show, label_hide)
  <tr>
   <th>Related</th>
   <td>
-    <button
-      id="toggle-related"
-      onclick="javascript:toggle_div('toggle-related', 'related')"
-      >show</button>
+    <button id="toggle-related">show</button>
     <div id="related" class="submission-list" style="display:none;">
       <ul>
       {% for sibling in related_same_project %}
@@ -134,10 +111,7 @@  function toggle_div(link_id, headers_id, label_show, label_hide)
         </li>
       {% endfor %}
       {% if related_different_project %}
-        <button
-          id="toggle-related-outside"
-          onclick="javascript:toggle_div('toggle-related-outside', 'related-outside', 'show from other projects')"
-          >show from other projects</button>
+        <button id="toggle-related-outside">show from other projects</button>
         <div id="related-outside" class="submission-list" style="display:none;">
         {% for sibling in related_outside %}
           <li>
@@ -306,32 +280,37 @@  function toggle_div(link_id, headers_id, label_show, label_hide)
       <a href="{% url 'comment-redirect' comment_id=item.id %}">#{{ forloop.counter }}</a>
     </span>
     {% if item.addressed %}
-      <div id="comment-status-bar">
+      <div id="comment-status-bar-addressed" data-comment-id={{item.id}}>
+    {% else %}
+      <div id="comment-status-bar-addressed" class="hidden" data-comment-id={{item.id}}>
+    {% endif %}
         <div id="comment-status-label" class="text-success mx-3">
           <span id="comment-status-icon" class="glyphicon glyphicon-ok-circle" aria-hidden="true"></span>
           Addressed
         </div>
         {% if editable %}
-          <button id="comment-action-unaddressed" class="text-warning">
+          <button id="comment-action-unaddressed" class="text-warning" value="false">
             <span id="comment-action-icon" class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span>
             Unaddressed
           </button>
         {% endif %}
       </div>
+    {% if item.addressed %}
+      <div id="comment-status-bar-unaddressed" class="hidden" data-comment-id={{item.id}}>
     {% else %}
-      <div id="comment-status-bar">
+      <div id="comment-status-bar-unaddressed" data-comment-id={{item.id}}>
+    {% endif %}
         <div id="comment-status-label" class="text-warning mx-3">
           <span id="comment-status-icon" class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span>
           Unaddressed
         </div>
         {% if editable %}
-          <button id="comment-action-addressed" class="text-success">
+          <button id="comment-action-addressed" class="text-success" value="true">
             <span id="comment-action-icon" class="glyphicon glyphicon-ok-circle" aria-hidden="true"></span>
             Addressed
           </button>
         {% endif %}
       </div>
-    {% endif %}
   </div>
   <pre class="content">
   {{ item|commentsyntax }}
@@ -344,7 +323,7 @@  function toggle_div(link_id, headers_id, label_show, label_hide)
   {% 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/templates/base.html b/templates/base.html
index 8accb4c..a95a11b 100644
--- a/templates/base.html
+++ b/templates/base.html
@@ -113,7 +113,7 @@ 
   {% endfor %}
   </div>
 {% endif %}
-  <div class="container-fluid">
+  <div id="main-content" class="container-fluid">
 {% block body %}
 {% endblock %}
   </div>