diff mbox series

[v3,06/10] static: add rest.js to handle PATCH requests & respective responses

Message ID 20210813053127.2160595-7-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 `rest.js` file to have a utilities JavaScript module that can be
reused by any Patchwork JS files that make requests to the REST API. In
particular, this patch provides the following function:

 - `updateProperty`: make PATCH requests that partially update the
   fields of an object given it's REST API endpoint specified by the
   caller. Also, the caller can specify the field(s) to modify and the
   associated content for update messages in the case of both failed
   successful requests that render to the current webpage.

The `rest.js` module can be further expanded to support and provide
functions that allow for other requests (e.g. GET, POST, PUT) to the
REST API.

Also, add functions that handle update & error messages for these PATCH
requests that match the Django messages framework format and form error
styling. These functions are internal to the module and aren't exposed
outside of the `rest.js` file.

Error and accompanying failed update messages are replaced by successful
update messages and vice versa. Consecutive successful update messages
add to counter of updated objects. Consecutive error messages stack up.

Signed-off-by: Raxel Gutierrez <raxel@google.com>
---
 htdocs/README.rst   |   7 +++
 htdocs/js/rest.js   | 119 ++++++++++++++++++++++++++++++++++++++++++++
 templates/base.html |   3 ++
 3 files changed, 129 insertions(+)
 create mode 100644 htdocs/js/rest.js

Comments

Daniel Axtens Aug. 17, 2021, 12:57 a.m. UTC | #1 
Raxel Gutierrez <raxel@google.com> writes:

> Add `rest.js` file to have a utilities JavaScript module that can be
> reused by any Patchwork JS files that make requests to the REST API. In
> particular, this patch provides the following function:
>
>  - `updateProperty`: make PATCH requests that partially update the
>    fields of an object given it's REST API endpoint specified by the
>    caller. Also, the caller can specify the field(s) to modify and the
>    associated content for update messages in the case of both failed
>    successful requests that render to the current webpage.
>
> The `rest.js` module can be further expanded to support and provide
> functions that allow for other requests (e.g. GET, POST, PUT) to the
> REST API.
>
> Also, add functions that handle update & error messages for these PATCH
> requests that match the Django messages framework format and form error
> styling. These functions are internal to the module and aren't exposed
> outside of the `rest.js` file.
>
> Error and accompanying failed update messages are replaced by successful
> update messages and vice versa. Consecutive successful update messages
> add to counter of updated objects. Consecutive error messages stack up.
>
> Signed-off-by: Raxel Gutierrez <raxel@google.com>
> ---
>  htdocs/README.rst   |   7 +++
>  htdocs/js/rest.js   | 119 ++++++++++++++++++++++++++++++++++++++++++++
>  templates/base.html |   3 ++
>  3 files changed, 129 insertions(+)
>  create mode 100644 htdocs/js/rest.js
>
> diff --git a/htdocs/README.rst b/htdocs/README.rst
> index 128dc7c..ced6bb8 100644
> --- a/htdocs/README.rst
> +++ b/htdocs/README.rst
> @@ -131,6 +131,13 @@ js
>    :GitHub: https://github.com/js-cookie/js-cookie/
>    :Version: 3.0.0
>  
> +``rest.js.``
> +
> +  Utility module for REST API requests to be used by other Patchwork js files
> +  (fetch requests, handling update & error messages).
> +
> +  Part of Patchwork.
> +
>  ``selectize.min.js``
>  
>    Selectize is the hybrid of a ``textbox`` and ``<select>`` box. It's jQuery
> diff --git a/htdocs/js/rest.js b/htdocs/js/rest.js
> new file mode 100644
> index 0000000..faac05d
> --- /dev/null
> +++ b/htdocs/js/rest.js
> @@ -0,0 +1,119 @@
> +/**
> + * Sends PATCH requests to update objects' properties using the REST API.
> + * @param {string} url Path to the REST API endpoint.
> + * @param {{field: string, value: string}} data
> + *     field: Name of the property field to update.
> + *     value: Value to update the property field to.
> + * @param {{none: string, some: string}} updateMessage
> + *     none: Message when object update failed due to errors.
> + *     some: Message when object update successful.
> + */
> +async function updateProperty(url, data, updateMessage) {
> +    const request = new Request(url, {
> +        method: 'PATCH',
> +        mode: "same-origin",
> +        headers: {
> +            "X-CSRFToken": Cookies.get("csrftoken"),
> +            "Content-Type": "application/json",
> +        },
> +        body: JSON.stringify(data),
> +    });
> +
> +    return await fetch(request)


I tried simulating network issues by loading the page, then killing the
local server, then pressing the button a few times.

No error message was displayed, but there were the following messages in
the JS console:

PATCH http://localhost:8000/api/patches/17/comments/9/ net::ERR_CONNECTION_REFUSED

rest.js:46 Uncaught (in promise) TypeError: Failed to fetch

I'm not sure what the proper way to deal with this is in JS, so I will
leave it in your capable hands.

> +        .then(response => {
> +            let message = updateMessage.some;
> +            let success = true;
> +            if (!response.ok) {
> +                response.text().then(text => {
> +                    const responseObject = JSON.parse(text);
> +                    // Add error messages from response to page
> +                    for (const [key,value] of Object.entries(responseObject)) {

Should there be a space between the comma and value in `const [key,value]`?

> +                        if (Array.isArray(value)) {
> +                            for (const error of value) {
> +                                handleErrorMessages(key + ": " + error);
> +                            }
> +                        } else {
> +                            handleErrorMessages(key + ": " + value);
> +                        }
> +                    }
> +                    // Update message to be unsuccessful
> +                    message = updateMessage.none;
> +                    success = false;
> +                });
> +            }
> +            handleUpdateMessages(message, success);
> +            return response.ok
> +        });
> +}
> +
> +/**
> + * Populates update messages for API REST requests.
> + * @param {string} messageContent Text for update message.
> + */
> +function handleUpdateMessages(messageContent, success=true) {
> +    // Replace error and failure update messages with success update message
> +    const errorList = document.getElementsByClassName("error-list")[0];
> +    const errorContainer = document.getElementById("errors");
> +    let messages = document.getElementById("messages");
> +    if (success && errorList != null) {
> +        errorContainer.replaceChildren();
> +        messages.replaceChildren();
> +    }
> +
> +    // Create messages container if it doesn't exist
> +    if (messages == null) {
> +        messages = document.createElement("div");
> +        messages.setAttribute("id", "messages");
> +        $(messages).insertAfter("nav");
> +    } else {
> +        // Increment counter of consecutive success update messages
> +        if (messages.firstChild != null) {
> +            const newMessageCount = parseInt(messages.firstChild.textContent.slice(0,1)) + 1
> +            messageContent = newMessageCount + messageContent.slice(1);
> +        }
> +    }
> +

This counter counts 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 2,... You can't
assume the whole number is in textContent.slice(0,1) - the number could
be multiple digits. 

> +    // Create new message element and add to list
> +    const message = document.createElement("div");
> +    message.setAttribute("class", "message");
> +    message.textContent = messageContent;
> +    messages.replaceChildren(...[message]);
> +}
> +
> +/**
> + * Populates error messages for API REST requests.
> + * @param {string} errorMessage Text for error message.
> + */
> +function handleErrorMessages(errorMessage) {
> +    // Replace success update message with error and failure update messages
> +    const messages = document.getElementById("messages");
> +    messages.replaceChildren()
> +
> +    let errorContainer = document.getElementById("errors");
> +    let errorHeader = document.getElementById("errors-header");
> +    let errorList = document.getElementsByClassName("error-list")[0];
> +    // Create errors container if it doesn't exist
> +    if (errorContainer == null) {
> +        errorContainer = document.createElement("div");
> +    }
> +    // Create errors list and error header if container contents removed
> +    if (errorList == null) {
> +        errorHeader = document.createElement("p");
> +        errorList = document.createElement("ul");
> +        errorContainer.setAttribute("id", "errors")
> +        errorHeader.setAttribute("id", "errors-header")
> +        errorHeader.textContent = "The following errors were encountered while updating comments:";
> +        errorList.setAttribute("class", "error-list");
> +        errorContainer.appendChild(errorHeader);
> +        errorContainer.appendChild(errorList);
> +    }
> +
> +    const contentContainer = document.getElementById("main-content");
> +    const error = document.createElement("li");
> +
> +    error.textContent = errorMessage;
> +    errorList.appendChild(error);
> +    contentContainer.prepend(errorContainer);
> +}
> +
> +export { updateProperty };
> diff --git a/templates/base.html b/templates/base.html
> index e57e2d5..313a72d 100644
> --- a/templates/base.html
> +++ b/templates/base.html
> @@ -105,7 +105,9 @@
>      </div>
>     </div>
>    </nav>
> +
>  {% if messages %}
> +  {# TODO(raxel): Change to always have messages container #}
>    <div id="messages">
>    {% for message in messages %}
>    {# TODO(stephenfin): Make use of message.tags when completely #}
> @@ -115,6 +117,7 @@
>    </div>
>  {% endif %}
>    <div id="main-content" class="container-fluid">
> +  {# TODO(raxel): Change to always have errors container #}
>  {% block body %}
>  {% endblock %}
>    </div>

Can you explain a bit more what you mean by this? If I've understood
correctly you want the template to always have the container divs for
messages and errors even if there are no messages or errors, so that you
can just add to them in JS without having to worry about creating them
if they're absent? I'm guessing it requires a lot of fiddling with the
HTML/CSS and maybe JS to get the divs not to show up as weird empty
boxes when there are no messages?

While I'm on the topic of error divs, in upstream master + your latest
patchset the display of errors is a bit broken: (ignore the content of
the error, I was just doing something to force an error to occur)

https://imgur.com/a/yxcZRRb

(not sure if that's an issue with this patch or another, apologies)

Kind regards,
Daniel

> -- 
> 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/README.rst b/htdocs/README.rst
index 128dc7c..ced6bb8 100644
--- a/htdocs/README.rst
+++ b/htdocs/README.rst
@@ -131,6 +131,13 @@  js
   :GitHub: https://github.com/js-cookie/js-cookie/
   :Version: 3.0.0
 
+``rest.js.``
+
+  Utility module for REST API requests to be used by other Patchwork js files
+  (fetch requests, handling update & error messages).
+
+  Part of Patchwork.
+
 ``selectize.min.js``
 
   Selectize is the hybrid of a ``textbox`` and ``<select>`` box. It's jQuery
diff --git a/htdocs/js/rest.js b/htdocs/js/rest.js
new file mode 100644
index 0000000..faac05d
--- /dev/null
+++ b/htdocs/js/rest.js
@@ -0,0 +1,119 @@ 
+/**
+ * Sends PATCH requests to update objects' properties using the REST API.
+ * @param {string} url Path to the REST API endpoint.
+ * @param {{field: string, value: string}} data
+ *     field: Name of the property field to update.
+ *     value: Value to update the property field to.
+ * @param {{none: string, some: string}} updateMessage
+ *     none: Message when object update failed due to errors.
+ *     some: Message when object update successful.
+ */
+async function updateProperty(url, data, updateMessage) {
+    const request = new Request(url, {
+        method: 'PATCH',
+        mode: "same-origin",
+        headers: {
+            "X-CSRFToken": Cookies.get("csrftoken"),
+            "Content-Type": "application/json",
+        },
+        body: JSON.stringify(data),
+    });
+
+    return await fetch(request)
+        .then(response => {
+            let message = updateMessage.some;
+            let success = true;
+            if (!response.ok) {
+                response.text().then(text => {
+                    const responseObject = JSON.parse(text);
+                    // Add error messages from response to page
+                    for (const [key,value] of Object.entries(responseObject)) {
+                        if (Array.isArray(value)) {
+                            for (const error of value) {
+                                handleErrorMessages(key + ": " + error);
+                            }
+                        } else {
+                            handleErrorMessages(key + ": " + value);
+                        }
+                    }
+                    // Update message to be unsuccessful
+                    message = updateMessage.none;
+                    success = false;
+                });
+            }
+            handleUpdateMessages(message, success);
+            return response.ok
+        });
+}
+
+/**
+ * Populates update messages for API REST requests.
+ * @param {string} messageContent Text for update message.
+ */
+function handleUpdateMessages(messageContent, success=true) {
+    // Replace error and failure update messages with success update message
+    const errorList = document.getElementsByClassName("error-list")[0];
+    const errorContainer = document.getElementById("errors");
+    let messages = document.getElementById("messages");
+    if (success && errorList != null) {
+        errorContainer.replaceChildren();
+        messages.replaceChildren();
+    }
+
+    // Create messages container if it doesn't exist
+    if (messages == null) {
+        messages = document.createElement("div");
+        messages.setAttribute("id", "messages");
+        $(messages).insertAfter("nav");
+    } else {
+        // Increment counter of consecutive success update messages
+        if (messages.firstChild != null) {
+            const newMessageCount = parseInt(messages.firstChild.textContent.slice(0,1)) + 1
+            messageContent = newMessageCount + messageContent.slice(1);
+        }
+    }
+
+    // Create new message element and add to list
+    const message = document.createElement("div");
+    message.setAttribute("class", "message");
+    message.textContent = messageContent;
+    messages.replaceChildren(...[message]);
+}
+
+/**
+ * Populates error messages for API REST requests.
+ * @param {string} errorMessage Text for error message.
+ */
+function handleErrorMessages(errorMessage) {
+    // Replace success update message with error and failure update messages
+    const messages = document.getElementById("messages");
+    messages.replaceChildren()
+
+    let errorContainer = document.getElementById("errors");
+    let errorHeader = document.getElementById("errors-header");
+    let errorList = document.getElementsByClassName("error-list")[0];
+    // Create errors container if it doesn't exist
+    if (errorContainer == null) {
+        errorContainer = document.createElement("div");
+    }
+    // Create errors list and error header if container contents removed
+    if (errorList == null) {
+        errorHeader = document.createElement("p");
+        errorList = document.createElement("ul");
+        errorContainer.setAttribute("id", "errors")
+        errorHeader.setAttribute("id", "errors-header")
+        errorHeader.textContent = "The following errors were encountered while updating comments:";
+        errorList.setAttribute("class", "error-list");
+        errorContainer.appendChild(errorHeader);
+        errorContainer.appendChild(errorList);
+    }
+
+    const contentContainer = document.getElementById("main-content");
+    const error = document.createElement("li");
+
+    error.textContent = errorMessage;
+    errorList.appendChild(error);
+    contentContainer.prepend(errorContainer);
+}
+
+export { updateProperty };
diff --git a/templates/base.html b/templates/base.html
index e57e2d5..313a72d 100644
--- a/templates/base.html
+++ b/templates/base.html
@@ -105,7 +105,9 @@ 
     </div>
    </div>
   </nav>
+
 {% if messages %}
+  {# TODO(raxel): Change to always have messages container #}
   <div id="messages">
   {% for message in messages %}
   {# TODO(stephenfin): Make use of message.tags when completely #}
@@ -115,6 +117,7 @@ 
   </div>
 {% endif %}
   <div id="main-content" class="container-fluid">
+  {# TODO(raxel): Change to always have errors container #}
 {% block body %}
 {% endblock %}
   </div>