Status message on comment edit is 'Your comment has been posted.'

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

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

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.

@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.');

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

@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 → closed merge request !3631

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

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

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.

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

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

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

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

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

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

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

Done

Can we rerun the Jenkins pipeline?

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.

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

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

done!

Thx!

Back to RTBC

Posted a couple minor suggestions on the MR. Thanks!

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

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.

MR appears to have failures 4845

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

Feedback appears to have been addressed.

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

@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

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

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

Left a minor comment on the MR

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

Feedback appears to be addressed.

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

This looks great now. I made a small change to the docblock to add an explanation of $is_new, since @catch question was a totally sensible one.

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 → closed merge request !4845

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