diff mbox series

[4/5] static: utils.js for modular handling of requests & messages

Message ID 20210719233428.2045872-5-raxel@google.com
State Superseded
Headers show
Series patch-list: improve usability of list action bar | expand

Commit Message

Raxel Gutierrez July 19, 2021, 11:34 p.m. UTC
Added file to have a general utilities JavaScript module file for
functions that are reused throughout various Patchwork js files. First
added function for making fetch requests to update properties through
'PATCH' requests with the REST api endpoints. Also, added functions for
handling update & error messages for these requests. The subsequent
patch will make use of these functions which will be also reused in
future features the make use fetch requests to update object fields.

Signed-off-by: Raxel Gutierrez <raxel@google.com>
---
 htdocs/README.rst  |  7 +++++
 htdocs/js/utils.js | 71 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)
 create mode 100644 htdocs/js/utils.js

Comments

Jonathan Nieder July 20, 2021, 2:13 a.m. UTC | #1
Hi,

Raxel Gutierrez wrote:

> Added file to have a general utilities JavaScript module file for
> functions that are reused throughout various Patchwork js files. First
> added function for making fetch requests to update properties through
> 'PATCH' requests with the REST api endpoints. Also, added functions for
> handling update & error messages for these requests. The subsequent
> patch will make use of these functions which will be also reused in
> future features the make use fetch requests to update object fields.
>
> Signed-off-by: Raxel Gutierrez <raxel@google.com>
> ---
>  htdocs/README.rst  |  7 +++++
>  htdocs/js/utils.js | 71 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+)
>  create mode 100644 htdocs/js/utils.js

I think I'd prefer to see this squashed into the same patch as patch
5/5, because that way it's easier to evaluate the interface it exposes
in the context of what its callers need.  I don't feel strongly about
that, though.  (It mostly comes down to whether the API exposed here
is obvious enough when viewed without that context.)

> diff --git a/htdocs/README.rst b/htdocs/README.rst
> index 4441bf3..2bae34c 100644
> --- a/htdocs/README.rst
> +++ b/htdocs/README.rst
> @@ -147,3 +147,10 @@ js
>    :Website: https://selectize.github.io/selectize.js/
>    :GitHub: https://github.com/selectize/selectize.js
>    :Version: 0.11.2
> +
> +``utils.js.``
> +
> +  General utility module for functions used throughout other static Patchwork
> +  js files (fetch requests, handling update & error messages).
> +
> +  Part of Patchwork.

These appear to be specifically about making REST API requests to the
patchwork backend, so how about a name like rest.js?

That way, if we come up with other utility functions then we can group
them into their own utility library.

Thanks,
Jonathan
Raxel Gutierrez July 20, 2021, 5:24 p.m. UTC | #2
Hi,

> I think I'd prefer to see this squashed into the same patch as patch
> 5/5, because that way it's easier to evaluate the interface it exposes
> in the context of what its callers need.  I don't feel strongly about
> that, though.  (It mostly comes down to whether the API exposed here
> is obvious enough when viewed without that context.)

I feel that the file is well documented enough and the methods it
exposes are also given enough context for the patch to be on its own.
Patch 5/5 adds a whole new feature to Patchwork which I feel should be
evaluated by itself. Without squashing, both patches can be more
self-contained. Ultimately, I feel that the following patch is more
than just a proof of concept, it's a new feature. This patch simply is
for healthier code as the module's functions will be used elsewhere
later.
diff mbox series

Patch

diff --git a/htdocs/README.rst b/htdocs/README.rst
index 4441bf3..2bae34c 100644
--- a/htdocs/README.rst
+++ b/htdocs/README.rst
@@ -147,3 +147,10 @@  js
   :Website: https://selectize.github.io/selectize.js/
   :GitHub: https://github.com/selectize/selectize.js
   :Version: 0.11.2
+
+``utils.js.``
+
+  General utility module for functions used throughout other static Patchwork
+  js files (fetch requests, handling update & error messages).
+
+  Part of Patchwork.
diff --git a/htdocs/js/utils.js b/htdocs/js/utils.js
new file mode 100644
index 0000000..34d41f5
--- /dev/null
+++ b/htdocs/js/utils.js
@@ -0,0 +1,71 @@ 
+/**
+ * Sends fetch requests to update objects' properties using REST api endpoints.
+ * @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 unsuccessful 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),
+    });
+
+    await fetch(request)
+        .then(response => {
+            if (!response.ok) {
+                response.text().then(text => {
+                    // Error occurred, so update message specifies no objects updated
+                    handleUpdateMessages(updateMessage.none);
+                    handleErrorMessages(JSON.parse(text).detail);
+                });
+            } else {
+                // Update message for successful changes
+                handleUpdateMessages(updateMessage.some);
+            }
+        });
+}
+
+/**
+ * Populates update messages for fetch requests.
+ * @param {string} messageContent Text for update message.
+ */
+function handleUpdateMessages(messageContent) {
+    let messages = document.getElementById("messages");
+    if (messages == null) {
+        messages = document.createElement("div");
+        messages.setAttribute("id", "messages");
+    }
+    let message = document.createElement("div");
+    message.setAttribute("class", "message");
+    message.textContent = messageContent;
+    messages.appendChild(message);
+    if (messages) $(messages).insertAfter("nav");
+}
+
+/**
+ * Populates error messages for fetch requests.
+ * @param {string} errorMessage Text for error message.
+ */
+function handleErrorMessages(errorMessage) {
+    let container = document.getElementById("main-content");
+    let errorHeader = document.createElement("p");
+    let errorList = document.createElement("ul");
+    let error = document.createElement("li");
+    errorHeader.textContent = "The following error was encountered while updating comments:";
+    errorList.setAttribute("class", "errorlist");
+    error.textContent = errorMessage;
+    errorList.appendChild(error);
+    container.prepend(errorList);
+    container.prepend(errorHeader);
+}
+
+export { updateProperty, handleUpdateMessages, handleUpdateMessages};
\ No newline at end of file