Problem/Motivation

When a comment is edited and saved the user gets a status message that reads: 'Your comment has been posted.'

That is confusing because it suggests that a new comment has been created.

Proposed resolution

Use a status message like 'The comment has been updated.' on comment edit.

CommentFileSizeAuthor
#20 Screen Recording After.mov23.16 MBrinku jacob 13
#20 Screen Recording Before.mov16.29 MBrinku jacob 13
#18 3346218-after-patch.png184.33 KBnayana_mvr
#18 3346218-before-patch.png169.42 KBnayana_mvr

Issue fork drupal-3346218

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Harlor created an issue. See original summary.

urvashi_vora made their first commit to this issue’s fork.

urvashi_vora’s picture

urvashi_vora commented

Hi,

From the issue, I came to know that the message "Your comment has been posted" arrives from

/core/modules/comment/src/CommentForm.php line number 396

$this->messenger()->addStatus($this->t('Your comment has been posted.'));

Other than that, there are two more files which contains the same message but when I edited comment in my backend and did several tests, always the results landed me on CommentForm.php file.

Also, when we add a new comment or we edit an existing comment, always the message comes from the same file.

Thanks.

harlor’s picture

harlor commented

Yeah exactly - do you think this is reasonable?

Because I think we should change this.

urvashi_vora’s picture

urvashi_vora commented

Indeed, I agree.

We should change this so that the user may be notified with the difference of "Posted" and "Updated"

harlor’s picture

harlor commented

The NodeForm.php uses a simple if clause for that:

    if ($insert) {
      $this->logger('content')->notice('@type: added %title.', $context);
      $this->messenger()->addStatus($this->t('@type %title has been created.', $t_args));
    }
    else {
      $this->logger('content')->notice('@type: updated %title.', $context);
      $this->messenger()->addStatus($this->t('@type %title has been updated.', $t_args));
    }

I guess in the CommentForm we could do the same.

urvashi_vora’s picture

urvashi_vora commented

I believe, this will be a valid approach to meet the requirement.

Kaushik1216 made their first commit to this issue’s fork.

Kaushik1216 opened merge request !3603

urvashi_vora’s picture

urvashi_vora commented

The MR changes looks good to me, but further I would definitely like to test it thoroughly in different setups before moving it to RTBC.

At present, setting the issue status as "Needs Review".

Thanks @Kaushik1216 for the work.

I appreciate your efforts.

harlor’s picture

harlor commented
Status: Active » Needs work

@Kaushik1216, Look at the tests ;)

Also I'd expect that we have to adjust this test:

    // Check that saving a comment produces a success message.
    $this->drupalGet('comment/' . $comment->id() . '/edit');
    $this->submitForm($edit, 'Save');
    $this->assertSession()->pageTextContains('Your comment has been posted.');
Kaushik1216’s picture

Kaushik1216 commented

@Harlor please more elaborate your comment #11 on adjust test . ie where and why we should add this code as I am new here .Thanks in advance !

harlor’s picture

harlor commented

@Kaushik1216,

If you haven't already make your self familiar with automated testing in drupal. You can find the error I expected to happen in #11 now in https://www.drupal.org/pift-ci-job/2611730.

Kaushik1216 opened merge request !3631

Kaushik1216 closed merge request !3631

Kaushik1216’s picture

Kaushik1216 commented

@ Harlor Please help me to add test for this merge request

harlor’s picture

harlor commented
Status: Needs work » Needs review

@Kaushik1216, Ah the two messages just needed to be interchanged so some of the failing tests were actually right ;)

nayana_mvr’s picture

nayana_mvr commented
StatusFileSize
new 3346218-before-patch.png169.42 KB
new 3346218-after-patch.png184.33 KB

Verified the MR!3603 on Drupal version 10.1.x. The patch applied cleanly and now the status message on comment edit is changed to 'Your comment has been updated.' I have added the before and after screenshots for reference. Need RTBC+1.

rinku jacob 13’s picture

rinku jacob 13 commented

Reviewed Merge requests!3603 . After applying the Merge request edit comment message changed to "Your comment has been updated". Need RTBC +1.

rinku jacob 13’s picture

rinku jacob 13 commented
StatusFileSize
new Screen Recording Before.mov16.29 MB
new Screen Recording After.mov23.16 MB
Kaushik1216’s picture

Kaushik1216 commented

@Harlor Thanks for changing MR . A warm Thanks to @nayana_mvr and Rinku Jacob 13 for testing MR

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 10.0.x-dev » 10.1.x-dev
Status: Needs review » Needs work
Issue tags: +String change in 10.1.0

Thanks folks! left some comments on the MR - tl;dr I think this is close, I'd just like to see the message derivation in it's own method to avoid the complexity of if/else/elseif and we need to target 10.1.x because of the string change.

Kaushik1216 closed merge request !3603

Kaushik1216 opened merge request !3982

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

harlor’s picture

harlor commented

I'm a bit confused with the branches on which one are we working now?

Kaushik1216’s picture

Kaushik1216 commented

I am working previously on 10.1 branch but now somehow branch for this issue changed to 11.0.x

Harlor opened merge request !4834

harlor’s picture

harlor commented
Status: Needs work » Needs review

I couldn't figure out how the branch could be fixed so I created a new branch from 10.1.x and cherry-picked the relevant commits...

I also added the doc-block for the getStatusMessage method

smustgrave’s picture

smustgrave commented

Change looks good. Believe @larowlan did a review but branch will have to be updated to 11.x please

Harlor opened merge request !4845

harlor’s picture

harlor commented

Done

harlor’s picture

harlor commented

Can we rerun the Jenkins pipeline?

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Yup, as long as the MR still applies you can click "Add test/retest" underneath it.

Reran for 11.x and all green and confirmed the new message.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -String change in 10.1.0

Can we please close all the MRs except the correct one?

Also, we don't tag string changes anymore.

Thanks!

Harlor closed merge request !4834

harlor’s picture

harlor commented

Sorry, I closed my obsolete MR.

@Kaushik1216 Could you please close https://git.drupalcode.org/project/drupal/-/merge_requests/3982? It seems that I don't have the permission to do that.

Kaushik1216 closed merge request !3982

Kaushik1216’s picture

Kaushik1216 commented

done!

harlor’s picture

harlor commented
Status: Needs work » Reviewed & tested by the community

Thx!

Back to RTBC

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Posted a couple minor suggestions on the MR. Thanks!

Kaushik1216 changed the visibility of the branch 3346218-d11-fix-comment-update-message to hidden.

harlor’s picture

harlor commented
Status: Needs work » Needs review

I committed the changes as suggested ;)

Harlor changed the visibility of the branch 3346218-d11-fix-comment-update-message to active.

Harlor changed the visibility of the branch 3346218--patch-b83b to hidden.

Harlor changed the visibility of the branch 10.1.x to hidden.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

MR appears to have failures 4845

harlor’s picture

harlor commented
Status: Needs work » Needs review

I merged 11.x into the MR's branch - and the tests succeeded.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback appears to have been addressed.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

@xjm's feedback on the MR hasn't been addressed - suggestion was to make $is_new the second parameter. Also I'm not sure we need that parameter at all, since $comment->isNew() should work?

pradhumanjain2311 made their first commit to this issue’s fork.

harlor’s picture

harlor commented

@catch, @pradhumanjain2311, We need the $is_new variable because the status message is generated after $comment->save(); and the comment is no longer new.

Sorry, I forgot to interchange the parameters :S

harlor’s picture

harlor commented
Status: Needs work » Needs review

I reverted to the variant with two parameters and interchanged the parameters.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Sorry for jumping the gun in #49 but seeing $is_new as the 2nd parameter now

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

Left a minor comment on the MR

harlor’s picture

harlor commented
Status: Needs work » Needs review

I applied the suggested changes - this time using gitlab :D

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed.

xjm’s picture

xjm
she/her
Primary language English
commented

Saving issue credits. I granted credit for the manual testing in #20 as well as that in #18, because in this scenario (where the original message is correct sometimes, and the new message is correct others), the video is actually really helpful over screenshots. (Normally we would not grant credit for duplicate manual testing.)

xjm’s picture

xjm
she/her
Primary language English
commented

This looks great now. I added a dockblock explanation of $is_new, since @catch's question was a totally sensible one.

  • xjm committed 67f56fc2 on 11.x 
    Issue #3346218 by Kaushik1216, Harlor, urvashi_vora, xjm,...

  • xjm committed 300f639c on 10.3.x 
    Issue #3346218 by Kaushik1216, Harlor, urvashi_vora, xjm,...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and cherry-picked to 10.3.x. I did not backport it to 10.2.x as it includes a string addition and slight behavior change. Thanks!

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev

xjm closed merge request !4845

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.