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

Thu Aug 19 04:33:16 AEST 2021

Hi Stephen,

Thank you for your review! :)

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

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
>