Admins should not see the "Your comment has been queued for moderation by site administrators and will be published after approval." message after unpublishing a comment and/or updating an unpublished comment.

CommentFileSizeAuthor
#12 302438_comment_queued_for_approval_admin_message.patch2.21 KBcatch
#5 302438_comment_queued_for_approval_admin_message.patch2.2 KBmaartenvg
#4 comment_message_test_01.patch1.21 KBcwgordon7
#1 comment_message_test.patch1.21 KBcwgordon7

Comments

cwgordon7’s picture

cwgordon7 commented
StatusFileSize
new comment_message_test.patch1.21 KB

Test case attached to demonstrate point.

lilou’s picture

lilou commented
Status: Active » Needs work

Patch no longer applies.

cwgordon7’s picture

cwgordon7 commented
Status: Needs work » Active

Except that it does.

cwgordon7’s picture

cwgordon7 commented
StatusFileSize
new comment_message_test_01.patch1.21 KB

(It's just fuzz, but reroll attached. :P)

maartenvg’s picture

maartenvg commented
Status: Active » Needs review
StatusFileSize
new 302438_comment_queued_for_approval_admin_message.patch2.2 KB

I wrapped the message in a user_access('administer comments') check, which solves this little issue and should not change the comment approval process. BTW, I left the test.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

The last submitted patch failed testing.

maartenvg’s picture

maartenvg commented

The automated testing failed because of #334826: Editing of anonymous comments is broken, so review that patch to get it in and set this back to CNR.

maartenvg’s picture

maartenvg commented
Status: Needs work » Needs review

That bug was just committed, so it's reviewing time again!

catch’s picture

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

Patch passes tests, and it's lovely. That message is really annoying if you've just unpublished a comment.

dries’s picture

dries commented

This is one of those cases where I'm not 100% convinced we need to have a test.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work 
+        if(!user_access('administer comments')) {

tsk, tsk. ;)

I think having the test is good, both because it helps http://acquia.com/files/test-results/modules_comment_comment.module.html get to 100%, but also because it's one of those things that's not tested in the day-to-day operation of Drupal very often, so is a good candidate for something that might subtly break 60 patches down the line.

How about we re-roll for the missing space and see if we come to consensus about the tests in the meantime? ;)

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 302438_comment_queued_for_approval_admin_message.patch2.21 KB

The shame! Re-rolled for the space. I don't think it hurts to have trivial-ish tests in there - when we get close to 100% coverage, we'll hopefully be trying to fill in the gaps, and knowing that an area is 'don't bother testing' will be near impossible - so better to have a test than not have one and have the same thing duplicated again in 3 months.

cwgordon7’s picture

cwgordon7 commented

I actually have to agree with Dries in this case. I don't think the test case is at all applicable for any bug other than this one: it's a good way to determine whether or not the patch actually fixes the bug in question, but it doesn't have any real use in core. All it does is make sure that after an administrator updates an unpublished comment, this specific message does not appear. I highly doubt that that will ever fail again unless this patch is reverted after applied. I don't think the hunk of this patch in comment.test should be committed, at least not in the current state. If we want to have a test case for this, it needs to be much more generalized—checking for a specific bug doesn't help our test coverage in the long run.

maartenvg’s picture

maartenvg commented

oh my, a (more or less) 1 line patch and I manage to get even that not perfect :P

I agree with Dries and Charlie, the test case is too specific for this bug, which probably will never surface again. It has nothing to do with general functionality of Drupal as a whole.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Fixed

Ok, I committed #12 without the test case. If even the author of the test case agrees it is unnecessary, then.. ;P

Thanks folks! :)

Status: Fixed » Closed (fixed)

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