Update node_comment_statistics during "Publish comment" action

looks good. maybe swap the doxygen for each param so they match the order in the function signature

Nice doxy cleanup.

1. parens indentation mistake below our declaration of the comment_publish_action action

I will rtbc after that.

Re-rolled with Moshe's recommendation along w/ commas after the closing ) for the hooks arrays per the Arrays coding standards (comma at the end of the last array element).

Patch re-roll.

and a Drupal 6 version

Yay! I pored over this excellent patch in search of typos and actually found something! :)

+  db_update('comment')
+    ->fields(array('status' => COMMENT_PUBLISHED,))
+    ->condition('cid', $cid)
+    ->execute();

Trailing commas are only supposed to be added to multi-line arrays, so the one after COMMENT_UNPUBLISHED should go.

Got nothing else, though.

Patch re-roll, thanks Arancaytar for the review!

Please add a test for this to actions.test. Thanks!

Marking as postponed as it's currently not possible to configure multiple actions in test cases because simpletest is unable to handle multiple forms on the same page all with the same submit button value. The admin/build/trigger/comment page is one such page. I created a patch to address this issue in simpletest in #421084: Allow drupalPost() to specify what form_id to submit to if multiple, so marking as postponed until that's committed.

Also, don't you think it might be better to add those tests to comment.test as it is the comment module that defines them? Failing that in trigger.test as that's currently where the node level actions are tested?

Thank you so much for the D6 version! Just what I needed.

Thanks so much for the D6 version. This seems to me to be more of a bug report than a feature request, since it seems like a strange omission from the list of actions.

Is this still postponed for D7, and can it still be considered even though Sept. 7 has passed? It would be a shame to have to keep patching this in or else add it in a custom module.

We badly need this.

Tagging.

Regarding tests, please see #582316: Add tests for actions. It makes no sense to hold off this patch, because we have no tests and especially no pattern for tests for actions yet.

code and docs look great to me

'description' is 'label' now.

Did I mark #582316: Add tests for actions as critical bug report? Yes, I did. ;)

Also, please make sure before committing this:

Passed: 185 passes, 0 fails, 0 exceptions

I see no reason for why this patch could kill 12,000+ tests, so I hope the new patch will do better.

Better title. Last patch also passed tests properly now.

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

Re-rolled against latest HEAD.

Yeah. I agree with kicking tests to another issue, given the point in the release cycle we're at now. It's completely silly that this action isn't there, given that unpublish is. Hope to see some of you in the actions.inc test issue!

Committed to HEAD. Please document this in the D6 => D7 module upgrade guide, so contrib modules that are currently offering their own know they can remove them.

.

Thanks, sun for advocating for this & webchick for committing it! Would it be too much to ask for this to be backported to D6? If it is, I can just continue patching it in each time a minor version upgrade comes out.

sub

is #21 patch ready to back port to D6?

Tests already in and a bit tuned at #699596: (Un)publishing several comments doesn't work

Here's a backport of #21 for D6

I think it's a bug and also great fix for docs!

re#27

publish comment appears on "action" "view bulk operation", "rules"

and tested in "view bulk operation", so i think it behaves correctly. Thanks for the patch!

maybe left next tester to mark this "reviewed and tested"?

subscribe

#27,

Your patch is good, but I was unable to apply it with just one command.

I had to split into two patch files and then apply each individually. :(

@nilesh.3892 do you patch against current CVS? I've tested this and patch still cleanly applies

Could someone confirm and RTBC this

I patched it against D6.16

tested against 6.16 and it's fine

What patch command did you issue ?

As always http://drupal.org/patch/apply

wget http://drupal.org/files/issues/360023-action-comment-publish-d6.patch
patch -p0 < 360023-action-comment-publish-d6.patch

Hmm, I think I missed that -p0

So could you RTBC this? or we need more reviews?

Its working with -p0.

Oh, great, thanks, committed.

Does anyone have VBO installed and is already using VBO's comment publish action? To my knowledge, VBO is the only contrib module that provides the missing action.

I didn't test updating to 6.x-dev + VBO myself yet, and can only hope that we didn't break anything with this commit.

I hope that someone else is able to test and report back -- hopefully before 6.17 is released.

I am using VBO, but it didn't provide the action by default. I had to apply the patch.

re@42 agreed. VBO is the org. point where i find this thread and fixed by apply patch.

I'm wondering why you're not updating node_comment_statistics on comment publish/unpublish actions? Current behavior breaks many things if you use this actions in Rules like unpublish comment after 30 days (just an example) - node comment statistics is not updated and you have outdated info in your blocks, node objects, etc.

Update: checked latest alpha of Drupal 7, functionality is the same. Am i the only one who see this problem?

oops - wrong issue. deleted

@cyberpunk No, you're not the only one. Should be a fairly simple patch. Will need tests too.

Here's a patch that adds the needed calls to the actions, needs tests, and testing.

Code-comment should end with comma

Actually should be a full-stop instead of the colon.

Didn't realize this still needed reviews. Thanks comment spam for reminding.

Fixed tags.

*sigh*

Hi, this is a sort of backport of #50 patch for Drupal 6

Is this still valid?

I believe the tags need to be updated.