[PATCH v2 3/3] static: add rest.js to handle PATCH requests & respective responses

[Stephen Finucane]

On Tue, 2021-08-17 at 21:33 +0000, Raxel Gutierrez wrote:
> 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 caller
>  receives whether the request was successful or not.
> 
> 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 a counter of updated objects. Consecutive error messages stack up.
> 
> Signed-off-by: Raxel Gutierrez <raxel at google.com>

FYI, I've focused on style and the likes here since Daniel seems to have a good
handle on the functionality of this already...

> ---
>  htdocs/js/rest.js | 110 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
>  create mode 100644 htdocs/js/rest.js
> 
> diff --git a/htdocs/js/rest.js b/htdocs/js/rest.js
> new file mode 100644
> index 00000000..5be5682e
> --- /dev/null
> +++ b/htdocs/js/rest.js
> @@ -0,0 +1,110 @@
> +/**
> + * 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.
> + */

Any reason for the 'none' and 'some' key names? Is it shorthand for something or
convention? It seems rather unintuitive to my JS newb eyes. Could we use e.g.
'error' and 'success' instead, or even separate attributes such as e.g.
'successMessage' and 'errorMessage'?

Also, is there a name for this format of comment? JSdoc? We should probably get
it formally added as a recommendation in the contributors guide
(docs/development/contributing.rst) at some point...

> +async function updateProperty(url, data, updateMessage) {
> + const request = new Request(url, {
> + method: 'PATCH',
> + mode: "same-origin",
> + headers: {
> + "X-CSRFToken": Cookies.get("csrftoken"),

nit: A small comment above this indicating that we're using the 'js-cookie'
module here might be helpful or it might be redundant. Your call

unrelated: JS' scope rules and packaging system is *the worst* /o\ :)

> + "Content-Type": "application/json",
> + },
> + body: JSON.stringify(data),

note to self: JSON is a builtin object...

> + });
> +
> + 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)) {

What are the conditions that can cause this to be an array vs a string? A small
comment explaining this would be useful, IMO.

> + 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
> + }).catch(error => {
> + handleErrorMessages(error);
> + });
> +}
> +
> +/**
> + * Populates update messages for API REST requests.
> + * @param {string} messageContent Text for update message.
> + */
> +function handleUpdateMessages(messageContent, success) {

Couple of somewhat nitty points:

 * Is 'message' good enough? 'messageContent' feel redundant.
 * There's no docstring for the 'success' parameter. I suspect it's a boolean
   but a note would be helpful
 * This is called 'handleUpdateMessages' yet it accepts a single message. I
   guess the 'Messages' you're referring to is the sum of messages found in the
   'ul.messages' element? Perhaps we could call this 'addUpdateMessage' instead
   since that's what I think you're doing (adding a new message as opposed to
   overwriting everything there).

> + // Replace error and failure update messages with success update message
> + const errorContainer = document.getElementById("errors");
> + let messages = document.getElementsByClassName("messages")[0];
> + if (success && errorContainer.firstChild != null) {
> + messages.replaceChildren();
> + errorContainer.replaceChildren();
> + } else if (!success) {
> + messages.replaceChildren();
> + }
> +
> + // Increment counter of consecutive success update messages
> + if (messages.firstChild != null) {
> + const currentMessageCount = messages.firstChild.textContent.match('^([0-9]+)')[0];
> + const newMessageCount = parseInt(currentMessageCount) + 1;
> + messageContent = newMessageCount + messageContent.slice(1);
> + }
> +
> + // Create new message element and add to list
> + const message = document.createElement("li");
> + message.setAttribute("class", "message");
> + if (success) {
> + message.classList.add("class", "success");
> + } else {
> + message.classList.add("class", "error");
> + }
> + message.textContent = messageContent;
> + messages.replaceChildren(...[message]);

Question: fwict the ellipsis operator is the equivalent of the '*' operator in
Python, i.e.

hello(*['a', 'b'])

is equivalent to:

hello('a', 'b')

yeah? Apologies again for my JS newbness.

> +}
> +
> +/**
> + * Populates error messages for API REST requests.
> + * @param {string} errorMessage Text for error message.
> + */
> +function handleErrorMessages(errorMessage) {

The signature for this makes more sense, though it's still plural and the
'error' in 'errorMessage' feels redundant. Perhaps we could call this
'addErrorMessage(message)' to align with the above, assuming that makes sense?

> + let errorContainer = document.getElementById("errors");
> + let errorHeader = document.getElementById("errors-header");
> + let errorList = document.getElementsByClassName("error-list")[0];
> +
> + // Create errors list and error header if container contents removed
> + if (errorList == null) {
> + errorHeader = document.createElement("p");
> + errorList = document.createElement("ul");
> + errorHeader.setAttribute("id", "errors-header")
> + errorHeader.textContent = "The following errors were encountered while making updates:";
> + errorList.setAttribute("class", "error-list");
> + errorContainer.appendChild(errorHeader);
> + errorContainer.appendChild(errorList);
> + }
> +
> + const error = document.createElement("li");
> + error.textContent = errorMessage;
> + errorList.appendChild(error);
> +}
> +
> +export { updateProperty };

Hopefully none of my comments above are too silly. Let me know if you've any
questions.

Cheers,
Stephen