WordPress comments package

luisherranz · 3 August 2020 12:28

Ok, that’s a much better idea. Great work @SantosGuillamot

I guess we could add that line to our plugin in the future, so the only requirement for the wp-comments package would be to have our Frontity plugin installed.

David · 3 August 2020 12:34

I’m glad you like Mario’s idea, @luisherranz.

I guess it won’t take long so I’ll try to implement it during this week.

luisherranz · 4 August 2020 07:41

Should we create a PHP plugin for this, like what we did for the Head Tags plugin?

SantosGuillamot · 4 August 2020 10:55

Yes, I think it makes sense to consider it. Although for this first beta version it’s okay if users add that line into their functions.php.

I’ve been doing more tests using WordPress REST API and these are my findings so far:

Even after adding the PHP code mentioned above, the setting "Users must be registered and logged in to comment " keeps working as expected. This means that if you enable it from the WordPress dashboard, users won’t be able to comment if they are not logged in.
It seems the author_name and the author_email are not required. If you don’t pass them it just creates an "Anonymous comment. We can find the fields accepted by the comments endpoint at the REST API handbook. I think the only required fields are content and post.
If you pass an author id, you have to be logged in to be allowed to comment.
If you want to associate a comment with an author, you have to pass the author id, just with the name and the email doesn’t seem to be enough, even if you’re logged in. If you pass only the name and the email, it allows you to create the comment, but the author id field will be 0. It seems it doesn’t detect automatically the author id just with the email and name.
It allows you to post a comment with an incorrect parent (an id that doesn’t exist yet).
All the different errors have the same format, so it’d be easier to handle them. We could reuse the message WordPress is sending they are pretty descriptive. These are some examples:

Author id that doesn’t exist:

{
    "code": "rest_comment_author_invalid",
    "message": "Invalid comment author ID.",
    "data": {
        "status": 400
    }
}

Duplicate comment:

{
    "code": "comment_duplicate",
    "message": "Duplicate comment detected; it looks as though you&#8217;ve already said that!",
    "data": {
        "status": 409
    }
}

Empty comment:

{
    "code": "rest_comment_content_invalid",
    "message": "Invalid comment content.",
    "data": {
        "status": 400
    }
}

Pass a non-existing post:

{
    "code": "rest_comment_invalid_post_id",
    "message": "Sorry, you are not allowed to create this comment without a post.",
    "data": {
        "status": 403
    }
}

Post too many comments:

{
    "code": "comment_flood",
    "message": "You are posting comments too quickly. Slow down.",
    "data": {
        "status": 400
    }
}

Comments are closed for an specific post:

{
    "code": "rest_comment_closed",
    "message": "Sorry, comments are closed for this item.",
    "data": {
        "status": 403
    }
}

mmczaplinski · 15 August 2020 00:39

My main concern with how to move forward with this is how to test it well. I see three options:

Just make the required changes and test it manually using a local instance of wordpress with the add_filter( 'rest_allow_anonymous_comments', '__return_true' ); added.
Create e2e tests (using the current e2e cypress tests) that use a “remote” WP instance at test.frontity.org. This remote instance would also have to allow comments and add the rest_allow_anonymous_comments filter.
Finish the new e2e testing system that uses the WP instances and write the tests using the new system.

I don’t think that 1 & 2 are very good options and I would vote for 3.

The only issue I see with 3. though is that currently the e2e tests do not support adding custom code to the WP instances. But, we could mitigate that by creating a simple 1-file frontity plugin which just includes the add_filter('rest_allow_anonymous_comments', '__return_true'); and nothing else. Then we can install this plugin in the WP instance and run tests against it.

The downside of 3. is that, of course, this is going to take longer than just 1. and 2.

mmczaplinski · 25 August 2020 21:01

@SantosGuillamot

In my tests it seems that both are indeed required

Otherwise, it fails with:

{
  "code": "rest_comment_author_data_required",
  "message": "Creating a comment requires valid author name and email values.",
   "data": { "status": 400 }
}

Can you verify this?

mmczaplinski · 26 August 2020 00:29

@SantosGuillamot Oh, dang it, I’ve just noticed that there is the following setting in WP Dashboard:

So, if a user checks this setting, they will have to put the author_name and author_email in the request.

SantosGuillamot · 26 August 2020 05:56

Yep, that’s it! I think it’s great that setting works by default

mmczaplinski · 28 August 2020 22:22

This was the latest discussion we had on the on the topic:

(This is just for the record, skip to the next post for a better proposal)

mmczaplinski · 29 August 2020 00:20

In the beginning of the video, I’m talking about not storing the comment form data in the frontity state, but I thought about this some more and it’s probably not a good idea so you can forget about it for now .

I have a better idea for improving the API (in the future), but I think we shouldn’t focus on it right now. Skip to the very end for a suggested API.

There are other small changes that I want to propose:

Store the validation errors returned from WordPress in the state. The REST API returns the validation errors for each of the fields in the comment. E.g. if you send a request with a malformed author_email, the API returns:

{ 
  "code": "rest_invalid_param",
  "message": "Invalid parameter(s): author_email",
  "data": {
    "status": 400,
    "params": {
      "author_email": "Invalid email address."
    }
  }
}

With this info available, I think that we should store the validation errors in the state. The error message from data.params.author_email should be stored under form.errors This would look like:

export interface Form {
  fields: Fields;
  submitted?: Submitted;
  errors: CommentErrors; // <--- store the errors here.
}

// It's kind of an awkward type, but it's easier to iterate over all the errors this way.
// if it was an object, we would have to do `Object.entries()` first. 
type CommentErrors = {name: keyof Fields, error: string}[]

Add the newly submitted comment to state.source.comments directly. We can do that because the REST API returns the newly created resource after a successful POST request. This is what I get after posting a new comment.

{
 "id": 3,
 "post": 1,
 "parent": 0,
 "author": 0,
 "author_name": "Michal",
 "author_url": "",
 "date": "2020-08-28T23:19:15",
 "date_gmt": "2020-08-28T23:19:14",
 "content": {
     "rendered": "<p>hello this is a postman test</p>\n"
 },
 "link": "http://localhost:8080/hello-world/#comment-3",
 "status": "hold",
 "type": "comment",
 "author_avatar_urls": {
     "24": "http://1.gravatar.com/avatar/160475e57835d6b02f6bfadbb05d853c?s=24&d=mm&r=g",
     "48": "http://1.gravatar.com/avatar/160475e57835d6b02f6bfadbb05d853c?s=48&d=mm&r=g",
     "96": "http://1.gravatar.com/avatar/160475e57835d6b02f6bfadbb05d853c?s=96&d=mm&r=g"
 },
 "meta": [],
 "_links": {
  //...  cut out for brevity

This would also mean that we would not need to store the comment data on the submitted property. We can rename this property to status and use it to store only the submission status.

export interface Form {
  fields: Fields;
  status?: Status;
  errors: CommentErrors;
}

export interface Status {
  isPending: boolean;
  // isOnHold: boolean;     not needed, because we have that in the Response
  // isApproved: boolean;   not needed either.
  isError: boolean;
  
 /**
  * The error message that was returned by the REST API
  */
  errorMessage: string;
  
 /**
  * The error code. Those are defined internally in the WordPress REST API.
  * @example rest_comment_invalid_post_id
  */
  errorCode: string;
   
 /**
  * The HTTP status code that might have been received 
  * from the WordPress REST API.
  */
  errorStatusCode?: number;

  // timestamp: number;    I think that is also not needed anymore
  // id?: number;          Not needed anymore either
}

Then, we would also have to actually populate the data in the state using populate() and insert the new comment in the existing tree of comments:

const populated = await populate({ response, state });

/* This is the part that I'm not sure about. 
   I think that the comments are sorted ascending by the `id` right? 
   So, the comments (on the same level) will always be sorted by id?
   Something like:
   
   comment 1
      comment 4
      comment 5
   comment 2
   comment 3
*/

// Then we can iterate over the data.items to insert the new comment here.

const { id } = await response.json();
const data = state.source.data[route];

Provide an updateField() action.
I think we in addition to the updateFields() (plural) we should have an action that can update an individual field. This would have better ergonomics for the user and also result in only rerendering one field instead of the the whole form when updating a field value. It could be simply something like:
```
actions: {
	updateField: ({ state }) => (postId, name, value) => {
		state.comments.form[postId]?.fields?[name] = value;
	}
}
```

Future API improvement

I was concerned about interoperability with popular form libraries like formik or react-hook-form, which are the idiomatic way to create forms in react. Rolling your own forms in react is a pain in the ass without those .

I was also thinking how to generally improve the developer experience of frontity.

We are going to provide the basic primitives for working with the WP comments which are:

form state,
submit() action
updateFields() action
updateField()` action

I propose that we also provide a useComments() hook which should cover 90% of the most basic cases which is just wiring some fields to the state, showing errors and submitting the form.

This way, we can recommend the users to use the hook for most cases and only reach to use the underlying primitives directly if the hook is not enough!

(the ref-based API is inspired by the react-hook-form )

const useComments = () => {
  const { state, actions } = useConnect();
  const { id: postId } = state.source.get(state.router.link);

  // It's a bit of a hack but it works well!
  const register = useCallback((ref) => {
    if (ref) {
      ref.addEventListener("input", ({ target: { name, value } }) => {
        actions.updateField(postId, name, value);
      });
    }
  }, []);

  const submit = (e) => {
    e.preventDefault();
    actions.comments.submit(postId);
  };

  return { submit, register, errors: state.comments.form.errors };
};

And for an example of how you’d use it:

const CommentComponent = connect(() => {
  const { register, submit, errors } = useComments();
  
  return (
    <form onSubmit={submit}>
      <input name="author_name" ref={register} />
      <input name="author_email" ref={register} />
      
      {/* We could show both client-side validation and backend errors here. */}
      {errors && errors.map(err => 
        <span>{err.name} : {err.errorMessage} </span>
      )}
      
      <input type="submit" />
    </form>
  );
})

However, I think we can work on this in the future because:

It’s not necessary to use the comments.
It only works if you attach the ref directly to the DOM element so wont work out of the box with most component libraries like Chakra, Bootstrap, etc. I think we would need to mess with forwardRef to make it work.
There are probably many other edge cases that I didnt think of yet

David · 31 August 2020 07:53

Some feedback for you, Michal!

Store the validation errors returned from WordPress in the state .

mmczaplinski:

With this info available, I think that we should store the validation errors in the state. The error message from data.params.author_email should be stored under form.errors

Yup, I also thought of storing that information in the state when I talked with Mario about using the REST API. My initial idea was to store the same error object received as it is in the submission status so you would access the validation errors doing something like this:
```
form.status.error.data.params
```
But if you think there is a benefit of creating a specific property in the state, that’s OK for me.

I’m not sure about it is better for that property to be an array. How are those errors going to be consumed? Just printing an enumeration of problems for the whole form? Or an error message in each field?
Add the newly submitted comment to state.source.comments directly .

mmczaplinski:

This would also mean that we would not need to store the comment data on the submitted property. We can rename this property to status and use it to store only the submission status.

I agree with that. isOnHold and isApproved would not be needed anymore but at least we need a property to know if there was a successful request so we can print a message in React that the comment was published correctly. If not, there would not be any difference in the state if you have published comments or you have never done it.

As I said before, I would store the same error object received from WordPress so I would change the status property to have the following structure:
```
export interface Status {
  /**
  * Request has not finished yet.
  */
  isPending: boolean;

  /**
  * Request has finished successfully.
  */
  isSubmitted: boolean;

  /**
  * Request has failed.
  */
  isError: boolean;

  /**
  * The error object that was returned by the REST API.
  * (having WpError the same structure than the error response).
  */
  error: WpError;
}
```
Again, if you think it is better to not use the same structure WordPress is using for errors I have no problem with using our own schema.

mmczaplinski:

Then, we would also have to actually populate the data in the state using populate() and insert the new comment in the existing tree of comments:

That would be really great.
Provide an updateField() action .

mmczaplinski:

I think we in addition to the updateFields() (plural) we should have an action that can update an individual field.

Actually, the current implementation of updateFields() allows you to update a single field, you just need to pass an object with that property, e.g.
```
updateFields({ email: "my.other@mail.com" });
```
If you think it is confusing we can add the updateField() action, no problem with that.

BTW, I really like your proposal for future API improvement.

luisherranz · 31 August 2020 07:58

I like the idea of useComments for the future

I’ve renamed it to useComments instead of useWPComments because that hook would need to by provided by any comments package.

One question: Can we add the unapproved comments to state.source.comments as well? Are they returned properly from the the REST API?

In that case, I think we wouldn’t need an array of submissions because the unapproved comments would be stored in state.source.comments, is that right?

Another question: Maybe due to that we don’t need a submitted or status field anymore and we can have those props in Form:

export interface Form {
  fields: Fields;
  isSubmitting: boolean;
  isSubmitted: boolean;
  isError: boolean;
  errorMessage?: string;
  errorCode?: string;
  errorStatusCode?: number;
  errors: Errors;
}

David · 31 August 2020 08:01

The unapproved comments are returned by the REST API with the same format as accepted comments. The only difference is the value in the status field, that would be hold in that case.

luisherranz · 31 August 2020 08:05

Awesome. Then I guess we don’t need an array of submissions anymore

By the way, what do you think about renaming isPending to isSubmitting because the action would be actions.comments.submit and I’d like to keep consistency with isFetching that relates to actions.source.fetch.

Yeah, I would also try to keep the errors schema in the state as close as possible to what is returned by WordPress.

mmczaplinski · 31 August 2020 16:22

Yeah, I think I will change it to an object. This way, yes you have to do a Object.entries() when enumerating errors, but I think normally you will want show an error individually for each field an it’s easier with an object.

The unapproved comments are only returned by the REST API if you are authenticated though… If you are not authenticated, you cannot change the default status=approve :

Would this be a problem? I think it’s fine:

If the user is not authenticated then once they submit a comment we can add it to state.source.comments. This way, we can show a comment that is on hold at least temporarily. Then:

The comment can get either approved or thrashed in the meantime but the user has to refresh the page to see the “updated” state.
If the comment is still on hold, the user will not see it unless we store it in localstorage, but again I think that it’s fine.

That sounds fine to me, I agree that a more flat schema is better.

mmczaplinski · 14 September 2020 20:50

This has finally been implemented in https://github.com/frontity/frontity/pull/542

This is the recap of the changes:

Update the wp-comments package to use the REST API directly
Add the data returned by the REST API when POSTing a new comment directly to state.source.data and state.source.comment . This is handled inside of the actions.comments.submit() action.
Store the validation errors returned from WordPress in the state. The REST API returns the validation errors for each of the fields in the comment. E.g. if you send a request with a malformed author_email
Remove the query , route & link from the non-URL entities (like the comments) that are fetched from the REST API. The link for those entities starts with a @, e.g. @comments/42

You can now use the package to fetch and create new comments.

A pretty full usage example (please note that it’s not tested, so it might contain typos):

const Comments = ({ actions, state, postId }) => {
  useEffect(() => {
    actions.source.fetch(`@comments/${postId}`);
  }, []);

  const data = state.source.get(`@comments/${postId}`);
  const form = state.comments.forms[postId];

  return data.isReady ? (
    <>
      <form
        onSubmit={(e) => {
          e.preventDefault();

          // Post the comment using that values present in
          // state.comments.forms[postId].fields
          //
          // You can also pass the values as a parameter to `submit()`, like:
          // submit(1, {content: '', authorName: "Mike", authorEmail: hi@test.com })
          actions.comments.submit(postId);
        }}
      >
        {/* If the form is submitting, we can show some kind of a loading state */}
        {form.isSubmitting && <Loading />}

        <input
          name="content"
          onChange={(e) =>
            actions.comments.updateFields(postId, { content: e.target.value })
          }
          value={state.comments.forms[postId]?.fields?.content || ""}
        />

        {/* Show the errors for the individual fields.
            E.g. content of a comment cannot be empty and the email must be valid */}
        {form.errors?.content}

        <input
          name="author_name"
          onChange={(e) =>
            actions.comments.updateFields(postId, {
              authorName: e.target.value,
            })
          }
          value={state.comments.forms[postId]?.fields?.authorName || ""}
        />
        {form.errors?.authorName}

        <input
          name="author_email"
          onChange={(e) =>
            actions.comments.updateFields(postId, {
              authorEmail: e.target.value,
            })
          }
          value={state.comments.forms[postId]?.fields?.authorEmail || ""}
        />
        {form.errors?.authorEmail}

        {/* Show the REST API error messages.
            E.g. "Sorry, you must be logged in to comment." */}
        <div>ERROR: {form.errorMessage}</div>

        {/* If the form was submitted successfully we can show a confirmation */}
        <div>{form.isSubmitted && "The comment submitted successfully!"}</div>

        <input type="submit" />
      </form>

      {/* Display the list of comments */}
      <div>
        {data.items.map(({ id, children }) => {
          if (children) {
            // Iterate over the sub-comments jand render them...
          }
          return (
            <>
              <div> {state.source.comment[id].author_name} said: </div>
              <div> {state.source.comment[id].content} </div>
            </>
          );
        })}
      </div>
    </>
  ) : null;
};

luisherranz · 15 September 2020 07:26

Hey @mmczaplinksi, amazing work

I have a comment about this, though. I think we are assuming how non-URL data objects are going to be used, and restricting their usage too early.

As a general rule, I’d prefer trying not to restrict the APIs as much as we can.

In this specific case, I also see possible uses for the removed fields.

link: The goal of introducing this field was to make it easier to use it as a self-reference, useful when passing objects around.

For example, imagine a function that receives the data object.
```
const sendSomeEvent = (data) => {
  someAnalyticsLibrary.send({ name: data.link, payload: data });
};

sendSomeEvent(data);
```
I think that usage still applies for non-URL data objects.
route: This is required for pagination, the same way page is required.

Imagine we introduce pagination in the comments, for example.
```
await actions.source.fetch("@comments/10/page/2");
const data = state.souce.get("@comments/10/page/2");
```
It would make sense to have both data.page (2) and data.route ("@comments/10") in the data object, just like in any other paginated data object.
query: This gives an opportunity to add options.

We decided to use a string for the index of the data object. I think that’s a great choice because they are much simpler to query and identify than JavaScript objects. But these string indexes need to be very versatile.

Thankfully, the URL-format is, in a sense, similar to a JavaScript object. For optional options, the query is ideal, and people are quite used to on the web, so it’s easy to understand.

Imagine we want to introduce some options to the @comments requests.
- Possibility to filter by the author of the comments.
- Possibility to filter the nested level of the comments fetched.
If we don’t have the query, we need to bake those in the path:
```
pattern: `@comments/:postId/:author/:nestedLevel`
```
But if those options are optional, maybe I don’t want to filter by an author, I only want to specify a nested level. I have no way to do so. It will also become quite convoluted when there are many options.

With queries, it becomes much simpler. The pattern is back to:
```
pattern: `@comments/:postId`
```
And queries are handled inside the handler. If I want to use the nested level option but not the author one, I could do:
```
actions.source.fetch("@comments/60?nestedLevel=2")
```

Sorry for the long explanation I was trying to make my point clear.

mmczaplinski · 15 September 2020 15:55

Those are all really good points, Luis, thank you!

I’m happy to undo that change now or we could wait for the next opportunity when we are doing further work on wp-comments.

luisherranz · 15 September 2020 19:34

@SantosGuillamot what do you think?

SantosGuillamot · 16 September 2020 09:06

If it’s a small change I would do it now. I haven’t released this version yet, so if you think it’s something we have to do, I’d include it before announcing it. I was planning to release and announce it once the new Yoast package is merged, but it can wait until tomorrow if this change doesn’t take long.