Change message to "Your comment has been updated" on comment update

[alanmels]

Description of the bug

We needed separate created and updated notifications for comments on a new Backdrop project. Unfortunately, the current state is that it always gives the "Your comment has been posted." message regardless if a comment was just created or updated. Nodes, on the other hand, have a separate message for update operation: "Post Your first post! has been updated.". So I believe this needs to be brought into consistency.

Additional information

This can be easily addressed by changing the following code in comment.module:

    if ($comment->status == COMMENT_NOT_PUBLISHED) {
      if (!user_access('administer comments')) {
        backdrop_set_message(t('Your comment has been queued for review by site administrators and will be published after approval.'));
      }
    }
    else {
      backdrop_set_message(t('Your comment has been posted.'));
    }

to

    if ($comment->status == COMMENT_NOT_PUBLISHED) {
      if (!user_access('administer comments')) {
        backdrop_set_message(t('Your comment has been queued for review by site administrators and will be published after approval.'));
      }
    }
    elseif (isset($comment->new)) {
      backdrop_set_message(t('Your comment has been updated.'));
    }
    else {
      backdrop_set_message(t('Your comment has been posted.'));
    }

and I'll be filing a PR soon. However, what I find it strange is that the $comment->new is set and equals to FALSE only on updates whereas on new comment creation it's not there at all. So I am not sure if this is a separate bug, because semantically it makes more sense to use an if condition something like this:

if ($comment->new == TRUE) {
  print created message here
} 
else {
  print updated message here
}

Let me know if we should chase why the $comment->new is not there for new comments or if the suggested PR is good enough.

[argiepiano]

This sounds like a good idea.

After a quick look, I think there is some confusion about the property new in comments. It's used mainly for the UI, to add the new notice indicating that it's a new/unread comment for the user viewing the page. The property does not mean that the comment itself is new. Take a look at node_mark(), which uses the constant NODE_NEW_LIMIT to determine whether the comment is new.

I believe you want to use the method isNew() in your elseif statements. This method, inherited from Entity, checks to see if the entity has an id or a is_new flag.

    elseif (!$comment->isNew()) {
      backdrop_set_message(t('Your comment has been updated.'));
    }
    else {
      backdrop_set_message(t('Your comment has been posted.'));
    }

Also, I would not call this a bug but a feature request.

[indigoxela]

... I believe you want to use the method isNew()

That was exactly what I was thinking. :+1:

Not sure if I'd call this a feature request. It's clearly a UX bug - inconsistency.

[alanmels]

Thanks for the suggestion. I've changed the PR accordingly.

[indigoxela]

I've changed the PR accordingly.

Great, many thanks! Currently at least two comment related tests are failing. Didn't take a closer look, but very likely the expected message text hast to be updated in tests.

[alanmels]

@indigoxela I'm not sure how to re-work the tests part, could you please help me out with this task?

[indigoxela]

@alanmels sure, I can help. But first of all: something's not right yet. If I add a new comment, I get the "updated" message. Seems like $comment->isNew() always returns FALSE. :thinking: (The comment's cid is already set at that point.)

[indigoxela]

I was able to achieve the expected message by using elseif ($comment->date != 'now') {. This property isn't defined or documented anywhere (it gets added on the fly in function comment_submit) and Comment::isNew() clearly does not what it's expected to do.

@argiepiano what do you think about this inconsistency re the entity class?

@alanmels it might be you opened a can of worms. :grin:

[argiepiano]

Yup. Checking the code more closely, comment_form_submit() saves the comment before displaying the message. As soon as you save the entity, the isNew() method will return false. This saving is done prior to displaying the message because, only after saving, can you know whether the comment is published or needs approval.

So, @indigoxela's workaround (checking for the date property to have a now value, which is a temporary property used just for the form), is a valid way to do it.

Another way to do it, which may be clearer for code legibility, is to use a variable such as $is_new that is set before saving. Then checking for that in order to display the proper message. I'll make that suggestion in the PR

[argiepiano]

Just left some suggestions.

Then there is the issue of the tests not passing, which needs to be addressed as well.

[alanmels]

@indigoxela, as far as I like finding newer cans of worms to make Backdrop even better, I am now confused which way is the ideal solution to this one. What about just using the initially proposed $comment->new variable? Even-though it's for different purpose, it works.

[argiepiano]

@alanmels, did you see my suggestion? It's simple, and in my opinion, better from the point of view of readability

[alanmels]

@argiepiano can't right now, but I definitely will do later today.

[alanmels]

@argiepiano I've committed both of your suggestions. Please confirm it's working now as expected.

[indigoxela]

@alanmels maybe it needs a little clarification, what $comment->new means: that the person viewing the node, didn't see that comment yet. Same for listings. Such a comment can be old - but it's still unread.

I like the idea with the $is_new variable - and it works like a charm! @argiepiano good suggestion!

Now for the failing test:

Here in this line the test checks the displayed message. But as this happens on an existing comment, the text is now "Your comment has been updated.". The only thing it needs to let it pass again is to change the expected text. And with that we also check that the updated text gets properly set (win-win). :wink:

[alanmels]

Only now had some time to fix the last issue on the PR and eventhough the https://github.com/backdrop/backdrop/pull/4152 now looks all green, this step https://github.com/backdrop/backdrop/actions/runs/3654156670/jobs/6174329966#step:10:506 is failing for some reason. Any ideas how to fix it?

[alanmels]

Closing and re-opening the PR passed all checks. See https://github.com/backdrop/backdrop/pull/4152

[klonos]

Nice teamwork here! 👏🏼 ❤️

I would have done something like $message_saved = $comment->isNew() ? t('Your comment has been updated.') : t('Your comment has been posted.')?, and would then have avoided the entire elseif/else checks and would have simply changed the existing else to do backdrop_set_message($message_saved);, but what you have done here works too, so 👍🏼

Right above the text were are changing with this PR, there's a watchdog message that says "Comment posted". While we're at it, and improving consistency in the frontend, why not change the watchdog message accordingly too for those that are going though the logs? I'd suggest 'Comment queued for review: %subject.' and 'Comment posted: %subject.'/'Comment updated: %subject.' respectively.

I also think that it might be worth improving the tests by checking for these different cases (we are currently only checking the "Your comment has been posted" scenario).

[klonos]

...and yes, this is a UX task I believe - so it can go into a bug fix release.