Skip to content
Snippets Groups Projects

Comment admins cannot reply to unpublished comments (despite link)

Open Comment admins cannot reply to unpublished comments (despite link)
issue/drupal-221761:221761-comment-admins-cannot into 11.x
Open Mohit Aghera requested to merge issue/drupal-221761:221761-comment-admins-cannot into 11.x

Closes #221761

  • Added provisioning for the comment administrator to allow comment
  • comment reply remains unpublished.
  • Added test case.
Edited by Mohit Aghera

Merge request reports

Members who can merge are allowed to add commits.

Merge request pipeline #157762 passed

Stage: 🏗️ Build
Stage: 🪄 Lint
Stage: 🗜️ Test
Pipeline: drupal-221761

#157764

    Merge request pipeline passed for c176ad23

    Approval is optional
    Code Quality is loading
    Test summary results are being parsed
    Full report
    Ready to merge by members who can write to the target branch.
    • The source branch is 1742 commits behind the target branch.
    • 1 commit will be added to 11.x.
    • Source branch will be deleted.

    Activity

    Filter activity
    • Approvals
    • Assignees & reviewers
    • Comments (from bots)
    • Comments (from users)
    • Commits & branches
    • Edits
    • Labels
    • Lock status
    • Mentions
    • Merge request status
    • Tracking
    core/modules/comment/tests/src/Functional/CommentAccessTest.php
    124 118 $assert->statusCodeEquals(200);
    125 119 }
    126 120
    121 /**
    122 * Tests admins can access reply form for unpublished comments.
    123 */
    124 public function testUnpublishedCommentReplyForCommentAdministrators() {
  • Stephen Mustgrave added 1 commit

    added 1 commit

    • c176ad23 - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • Jess
    Jess @xjm started a thread on the diff
    • core/modules/comment/src/Controller/CommentController.php
    295 295
    296 296 // Load the parent comment.
    297 297 $comment = $this->entityTypeManager()->getStorage('comment')->load($pid);
    298 // Check if the parent comment is published and belongs to the entity.
    299 $access = $access->andIf(AccessResult::allowedIf($comment && $comment->isPublished() && $comment->getCommentedEntityId() == $entity->id()));
    298 // Check if the parent comment is published or user can administer
    299 // comments, and parent comment belongs to the entity.
    • Comment on lines +298 to +299
      Jess
      Jess @xjm
      Maintainer
      Suggested change
      298 // Check if the parent comment is published or user can administer
      299 // comments, and parent comment belongs to the entity.
      298 // Access is allowed if:
      299 // - The parent comment is published or the user can administer comments.
      300 // - The parent comment belongs to the entity.
    • Please register or sign in to reply
  • Jess
    Jess @xjm started a thread on the diff
    • core/modules/comment/src/Controller/CommentController.php
    295 295
    296 296 // Load the parent comment.
    297 297 $comment = $this->entityTypeManager()->getStorage('comment')->load($pid);
    298 // Check if the parent comment is published and belongs to the entity.
    299 $access = $access->andIf(AccessResult::allowedIf($comment && $comment->isPublished() && $comment->getCommentedEntityId() == $entity->id()));
    298 // Check if the parent comment is published or user can administer
    299 // comments, and parent comment belongs to the entity.
    300 $access = $access->andIf(AccessResult::allowedIf($comment &&
    301 ($comment->isPublished() || $account->hasPermission('administer comments')) &&
    302 $comment->getCommentedEntityId() == $entity->id()));
    • Comment on lines +300 to +302
      Jess
      Jess @xjm
      Maintainer

      This is trying to win the Longest Line of Code Award.™ Ideally should either spread the logic out onto multiple readable lines, or define single use variables like $is_published, $administer_comments, and $is_entity for readability.

    • Please register or sign in to reply
  • Jess
    Jess @xjm started a thread on the diff
    • core/modules/comment/tests/src/Functional/CommentAccessTest.php
    60 53 'post comments',
    61 54 'access comments',
    62 55 'access content',
    56 'administer comments',
    • Jess
      Jess @xjm
      Maintainer

      Generally, we should add a new test class rather than altering the permissions of an existing test so broadly. Adding a permission like this to an existing test can create false passes for other functionality. We should either create a different test class or use a different user for the new test method. I'd suggest the former.

    • Please register or sign in to reply
  • Jess
    Jess @xjm started a thread on the diff
    • core/modules/comment/tests/src/Functional/CommentAccessTest.php
    124 118 $assert->statusCodeEquals(200);
    125 119 }
    126 120
    121 /**
    122 * Tests admins can access reply form for unpublished comments.
    123 */
    124 public function testUnpublishedCommentReplyForCommentAdministrators(): void {
    125 $assert = $this->assertSession();
    126
    127 // Publish the node.
    128 $this->unpublishedNode->setPublished()->save();
    129 $node_id = $this->unpublishedNode->id();
    130
    131 // Create a comment on an published node.
    • Jess
      Jess @xjm
      Maintainer
      Suggested change
      131 // Create a comment on an published node.
      131 // Create a comment on the published node.
    • Please register or sign in to reply
  • Jess
    Jess @xjm started a thread on the diff
    • core/modules/comment/tests/src/Functional/CommentAccessTest.php
    124 118 $assert->statusCodeEquals(200);
    125 119 }
    126 120
    121 /**
    122 * Tests admins can access reply form for unpublished comments.
    • Jess
      Jess @xjm
      Maintainer
      Suggested change
      122 * Tests admins can access reply form for unpublished comments.
      122 * Tests that admins can access the reply form for unpublished comments.
    • Please register or sign in to reply
  • Jess
    Jess @xjm started a thread on the diff
    • core/modules/comment/tests/src/Functional/CommentAccessTest.php
    149 // Replying to an unpublished node as a user who has "administer comment"
    150 // permissions.
    151 $this->drupalGet($comment_url);
    152 $assert->statusCodeEquals(200);
    153
    154 // Submit the comment and ensure that comment reply remains unpublished.
    155 $edit = [
    156 'subject[0][value]' => 'Comment reply subject',
    157 'comment_body[0][value]' => 'Comment body',
    158 ];
    159 $this->submitForm($edit, 'Save');
    160 $reply_cid = $this->getUnapprovedComment('Comment reply subject');
    161 $reply = Comment::load($reply_cid);
    162 $this->assertEquals(0, $reply->isPublished());
    163
    164 // Logout and visit as an anonymous user.
    • Jess
      Jess @xjm
      Maintainer
      Suggested change
      164 // Logout and visit as an anonymous user.
      164 // Log out and visit as an anonymous user.

      "Log out" is two words when it's a verb, one when it's a noun.

    • Please register or sign in to reply
  • Jess
    Jess @xjm started a thread on the diff
    • core/modules/comment/tests/src/Functional/CommentInterfaceTest.php
    173 173 $reply_loaded->save();
    174 174 $this->drupalGet('comment/reply/node/' . $this->node->id() . '/comment/' . $reply_loaded->id());
    175 175 $this->assertSession()->statusCodeEquals(403);
    176 $this->assertSession()->pageTextContains(t('You are not authorized to access this page.'), 'Replying to an unpublished comment is not possible with insufficient permissions.');
    • Jess
      Jess @xjm
      Maintainer

      Two things:

      1. We avoid using t() or any variant thereof in tests unless it's the translation system being tested.
      2. Using assertion messages is no longer considered a best practice. If the message adds necessary information to the test, it should be converted to an inline comment.
      Suggested change
      176 $this->assertSession()->pageTextContains(t('You are not authorized to access this page.'), 'Replying to an unpublished comment is not possible with insufficient permissions.');
      176
      177 // Replying to an unpublished comment is not possible with insufficient permissions.
      178 $this->assertSession()->pageTextContains('You are not authorized to access this page.');
      Edited by Jess
    • Please register or sign in to reply
  • Jess
    Jess @xjm started a thread on the diff
    • core/modules/comment/tests/src/Functional/CommentInterfaceTest.php
    173 173 $reply_loaded->save();
    174 174 $this->drupalGet('comment/reply/node/' . $this->node->id() . '/comment/' . $reply_loaded->id());
    175 175 $this->assertSession()->statusCodeEquals(403);
    176 $this->assertSession()->pageTextContains(t('You are not authorized to access this page.'), 'Replying to an unpublished comment is not possible with insufficient permissions.');
    177 // Attempt to reply to an unpublished comment as an administrator.
    • Jess
      Jess @xjm
      Maintainer
      Suggested change
      177 // Attempt to reply to an unpublished comment as an administrator.
      177
      178 // Attempt to reply to an unpublished comment as an administrator.
    • Please register or sign in to reply
  • Jess
    Jess @xjm started a thread on the diff
    • core/modules/comment/tests/src/Functional/CommentInterfaceTest.php
    173 173 $reply_loaded->save();
    174 174 $this->drupalGet('comment/reply/node/' . $this->node->id() . '/comment/' . $reply_loaded->id());
    175 175 $this->assertSession()->statusCodeEquals(403);
    176 $this->assertSession()->pageTextContains(t('You are not authorized to access this page.'), 'Replying to an unpublished comment is not possible with insufficient permissions.');
    177 // Attempt to reply to an unpublished comment as an administrator.
    178 $this->drupalLogin($this->adminUser);
    179 $this->drupalGet('comment/reply/node/' . $this->node->id() . '/comment/' . $reply_loaded->id());
    180 $this->assertSession()->statusCodeEquals(200);
    181 $reply = $this->postComment(NULL, $this->randomMachineName(), $this->randomMachineName(), TRUE);
    182 $this->assertTrue($this->commentExists($reply, TRUE), 'An administration user was able to comment to an unpublished comment.');
    • Jess
      Jess @xjm
      Maintainer
      Suggested change
      182 $this->assertTrue($this->commentExists($reply, TRUE), 'An administration user was able to comment to an unpublished comment.');
      182
      183 // Confirm that an administrative user can reply to an unpublished comment.
      184 $this->assertTrue($this->commentExists($reply, TRUE));
    • Please register or sign in to reply
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
    • Please register or sign in to reply