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

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@google.com>
---
 htdocs/js/rest.js | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)
 create mode 100644 htdocs/js/rest.js

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

Hi Stephen,

Thank you for your review! :)

On Wed, Aug 18, 2021 at 7:38 AM Stephen Finucane <stephen@that.guru> wrote:
>
> 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@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'?

Thanks for the feedback. I agree that 'error' and 'success' are much
better names/descriptors, I changed to this naming in the following
revision. I think that separate attributes like 'successMessage' and
'errorMessage' are a bit confusing for me because error messages are
separate from general update messages, hence the 'updateMessage'
object. If the 'updateMessage' update still seems off, let me know and
we could separate it into something different (e.g.
'successUpdateMessage', 'errorUpdateMessage'). I don't have strong
preferences for this, I just like the compact use of objects :)

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

Yes, the Google JS style guide extends off JSDoc [1]. I added a
reference and comment about the guide in my commit message now. Sorry,
I think I mentioned it in the cover letter of my first patch series
[2, ref 1] but I have come to learn that commit messages are more
important than cover letters :) I think it would be nice to have it
formally added for function comments. I don't know or use the entire
style guide but did use it for documenting functions as seen.

[1] https://google.github.io/styleguide/jsguide.html#jsdoc
[2] https://lists.ozlabs.org/pipermail/patchwork/2021-July/006953.html

> > +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\ :)

Added comment for 'js-cookie' module usage. Haha, yes I agree on both fronts.

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

Hm, so this is tricky because I'm not sure what causes the difference
in error messages in the response body. However, I noticed when I was
working on the patch relations table (unreleased, upcoming
feature/patch series) that there can be errors based on the object
field (e.g. "{ 'related': ["Wrong id"] }) that did produce an array
and the fetch request call was not handling those cases.

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

I agree with the comments on renaming. Sorry, I forgot to add the
'success' parameter because I kept going back and forth on whether I
needed it. Thanks for catching this :) Also, I should do this for the
'updateProperty' function as it returns whether the response was
successful or not as well. Fixed the fetch request also when error is
caught (e.g. network issues) to return false.

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

The ellipsis operator the way I'm using it here is commonly known as
the 'spread operator' or 'spread syntax' [3].  It is equivalent with
the Python '*' operator as it can also be used in other similar ways
(e.g. combine arrays). There is also another use for the ellipsis
which is the 'rest syntax' [4] that works like the '**' Python
operator.

[3] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax
[4] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/rest_parameters

> > +}
> > +
> > +/**
> > + * 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?

Agreed. I renamed them in the revision :)

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

Hi,

This fixes all my functional issues. Applying your other patches, this
now counts to at least 102 without issue and displays a network error
appropriately when I kill the server.

At some point I want to turn this all into a tristate: add a 'pending'
state in the frontend while we wait for the network request to resolve,
but that's complex to do and is only really useful in the edgecase where
the request fails for some reason. So it shouldn't block inclusion now.

Pending Stephen's stylistic comments:
Reviewed-by: Daniel Axtens <dja@axtens.net>

Stephen, I'll let you merge this once you're happy with the style.

Kind regards,
Daniel

> 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@google.com>
> ---
>  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.
> + */
> +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
> +        }).catch(error => {
> +            handleErrorMessages(error);
> +        });
> +}
> +
> +/**
> + * Populates update messages for API REST requests.
> + * @param {string} messageContent Text for update message.
> + */
> +function handleUpdateMessages(messageContent, success) {
> +    // 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]);
> +}
> +
> +/**
> + * Populates error messages for API REST requests.
> + * @param {string} errorMessage Text for error message.
> + */
> +function handleErrorMessages(errorMessage) {
> +    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 };
> -- 
> 2.33.0.rc1.237.g0d66db33f3-goog
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork