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

Loading
Loading

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