Hide reply link for unpublished comments

It strikes me that from this view, it just takes one click to approve a comment, therefore, the first solution makes most sense to me.

Although this runs into the issue raised at fix comment_form_submit redirect across multiple pages, I think it is less confusing to separate the approve/reply functions. To go with the second suggestion, I think you risk either people replying to comments and approving them without realising that they are doing so, or allowing them to go to a lot of effort to write a reply before realising that replying would approve the comment, that they do not want to happen.

I propose this patch, which simply hides the reply link on unapproved comments, as it is a simple, easy to understand fix, and resolves the problem. As this issue hasn't seen much attention in 12+ months, I wonder whether a a quick, simple fix, followed by a feature request for a more complex solution, similar to the second option suggested above, would be most appropriate?

One variation on my patch could be to alter the 'Approve' link to read 'Approve this comment to reply' or something like that.

there is some codestyle problems here is the the patch which solves this.

One short question: why do you use style font-weight:bold ?

Thanks very much dereine.

You know, I have no idea why I used font-weight:bold - I think I might have been pondering the idea of using the text 'approve to reply' in a different font or something like that, but then realised that just hiding the reply link probably made just as much sense.

This path has that line removed.

Attaching a new patch updated for HEAD. The last patch does the reported job of removing the reply link on comments that are not published.

Although I did just upload the last patch it was nothing more than updating the patch for head, setting this to reviewed & tested.

.

Not sure -- why should it be impossible to reply to an unpublished comment? That sounds like it could be a valid use case.

I'd say that the "The comment you are replying to does not exist"-message is invalid.

Good catch.

Is there a status on this? I'm using drupal 6.15 and would like users to be able to comment on unpublished content.

Still a valid issue in Drupal 7.2.

#9: drupal.comment-unpublished.patch queued for re-testing.

This is a tricky one. By allowing one to reply on unpublished comments, the display of that reply will be a bit off for people that can't see unpublished comments. It will look like the reply was a reply on another node above (see screenshots). I don't know, Dries says in #8 that it could be a valid use, so maybe it is? :)

Anyway, here is a re-roll of sun's patch from #9 that should apply with Git (the last one was from CVS). I also improved the code comment a tiny bit.

+++ b/modules/comment/comment.pages.inc
@@ -47,15 +47,16 @@ function comment_reply($node, $pid = NULL) {
-        $comment = db_query('SELECT c.*, u.uid, u.name AS registered_name, u.signature, u.signature_format, u.picture, u.data FROM {comment} c INNER JOIN {users} u ON c.uid = u.uid WHERE c.cid = :cid AND c.status = :status', array(
+        $comment = db_query('SELECT c.*, u.uid, u.name AS registered_name, u.signature, u.picture, u.data FROM {comment} c INNER JOIN {users} u ON c.uid = u.uid WHERE c.cid = :cid', array(

I don't think you intended to remove u.signature_format

26 days to next Drupal core point release.

Thank you! I have no idea how that happened...

If we need tests here, more work is also needed. I'll probably take a look at this in the near future.

comment_reply is gone now.

20: 221761-reply-unpublished-comment-20.patch queued for re-testing.

Hi,

Here is a re-roll of the patch for D8.
I have not removed the Needs tests tag, as Im not sure if that is covered or not. Seems to be that there is an existing test, which Attempt to reply to an unpublished comment (CommentInterfaceTest.php).

Thanks

Patch still applies. I think a quick assertion or two could be added to CommentInterfaceTest to confirm this fix.

+++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
@@ -155,9 +155,18 @@ public function testCommentInterface() {
+    $this->drupalLogout();
...
+    $this->drupalLogout();

These calls shouldn't be needed since drupalLogin() will log out the current user.

This is a tricky one. By allowing one to reply on unpublished comments, the display of that reply will be a bit off for people that can't see unpublished comments. It will look like the reply was a reply on another node above (see screenshots). I don't know, Dries says in #8 that it could be a valid use, so maybe it is? :)

I'm not sure if the current patch addresses this issue or not. If an administrator replies to a non-published comment, is that reply also unpublished by default, or will it appear published? Not sure if this needs addressing or not, but it does seem confusing.

Looks fine to me.

This looks fine for me.

Small nitpick. RTBC after that is fixed.

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -119,6 +119,12 @@ public function preSave(EntityStorageInterface $storage) {
+          if(!$parent->isPublished()) {

We need an extra space in this if statement.

Hi @geertvd,
I am re-rolling patch#43 with the modification you mentioned.

Don't forget to set the status to "Needs review" or the testbot won't pick this up.
Change looks good but please consider adding an interdiff in the future.
Will RTBC this when all tests pass.

Ok @geertvd !
Thanks for the advice. I will attach interdiff in the future!

The code and test looks fine to me.

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -120,6 +120,12 @@ public function preSave(EntityStorageInterface $storage) {
+          // If the parent is not published, this comment should be unpublished
+          // as well.
+          if (!$parent->isPublished()) {
+            $this->setPublished(FALSE);
+          }

Assigning to @webchick as product manager to confirm that this is the way things should work. I think the chosen behaviour does make sense but I ponder if it is slightly different to the behaviour when unpublishing a parent comment - do it's children stay published? Looking at the UnpublishComment action it looks like this is the case. Of course if the admin wants their comment to be published then they can just publish their comment. But what's weird is that when the parent comment is published it's children added by the admin will not be.

In fact thinking about

But what's weird is that when the parent comment is published it's children added by the admin will not be.

I'm not sure that we should allow admins to post replies to unpublished comments.

In #8 @Dries says this is a valid use case - but if the unpublished comment is never seen and not auto published if the parent comment is - what's the point?

Removing sprint weekend tag!!
As suggested by @YesCT

Unassigning stale issue. Hopefully someone else will pursue this.

Updated IS with steps to test and removed outdated code to avoid confusion. This issue still occurs in 9.3.x. Bumping to minor because this does not inhibit functionality.

- Re-roll the patch for the latest 9.3.x
- Ensure that comment reply remains unpublished when commenting on the unpublished comment
- Added test case.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Picking up again.
- Update the PR destination and base to 10.1.x
- Add additional test case from past patches. Both looks same to me.
Happy to weigh in other opinions and refactor those.
- All the test cases are passing on local now.

Hiding other patches as the one in #43 seems valid.
PR contains all the changes done in #43

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

So following the steps in the issue summary with the changes from MR 879 I'm still seeing reply button for unpublished comment.

Don't show the link, or allow admins to reply to unpublished comments?

This is a question so is this the approach that was taken?

Since this is changing the UI before/after screenshots in the issue summary will be needed

Thanks.

Tested manually and this does solve the problem, admins can reply to unpublished comments with the patch.

Before:

After:

Took the gifs from #76 and added to the issue summary user interface section

Ran the test-only feature too

1) Drupal\Tests\comment\Functional\CommentAccessTest::testUnpublishedCommentReplyForCommentAdministrators
Behat\Mink\Exception\ExpectationException: Current response status code is 403, but 200 expected.
/builds/issue/drupal-221761/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-221761/vendor/behat/mink/src/WebAssert.php:145
/builds/issue/drupal-221761/core/modules/comment/tests/src/Functional/CommentAccessTest.php:152
/builds/issue/drupal-221761/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
ERRORS!
Tests: 3, Assertions: 54, Errors: 1.
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.
Testing Drupal\Tests\comment\Functional\CommentInterfaceTest

Coverage is definitely there

MR does appear to address the problem described in the summary.

Believe this is good.

Thanks for your work on this issue! Thanks especially @pameeela for the very clear manual testing. I've updated the issue summary with those images.

There are a few coding standards and best practices issues with the current MR (see my comments therein). I do understand that in some cases these are being copied from existing tests and/or surrounding code, but we should not do the wrong thing for the sake of consistency. (If desired, a followup can be filed to improve the best practices in the rest of these tests.)

I think this is pretty close.

Saving issue credits.

I also just noticed no one actually ever answered @alexpott's question in #50 and #51. NW for that also.

FWIW, I think there is a usecase here, but we should articulate specifically why this is the right approach (vs. removing the link in this case).

If we need to escalate it for further feedback once that reply is given, I think it should go to the usability team and/or comment subsystem maintainers; this is not really a product-level decision. Thanks!

I was a bit unsure about the solution myself, in testing it seemed a bit odd to reply to an unpublished comment. I am not super clear on the use case for replying before approving.

If an admin replies to an unpublished comment, the reply is unpublished as well and it has to be approved separately (even if they have permission to comment without approval). This makes sense, but you can then approve it meaning you could have a published reply to an unpublished comment. And if the original comment is deleted rather than approved, the reply is also deleted. (Why would you reply to a comment you were going to delete ? Well, exactly. But also why wouldn't you just approve the comment before replying?)

Having said all that, the stakes here seem extremely low. But it makes more sense to me to hide the reply link until the comment is approved.

Updated based on the latest suggested approach.

Agreed with the new proposed solution, hiding the link seems right.

Put up an MR with test coverage.

Woohoo! Manual test passed, added updated screenshots to the IS.

Solution and test code look good, could maybe check for the reply link to exist and not just the text to be a little more specific but this works.

Committed and pushed 6067ed426c to 11.x and 3b1e465274 to 11.0.x and f096a302e8 to 10.4.x and 71ac91d7ce to 10.3.x. Thanks!