Posted by greggles on January 16, 2009 at 11:06pm
19 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | comment.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | actions, needs backport to D6, needs backport to D7, Needs tests |
Issue Summary
I just needed an action with Views Bulk Operations to publish a comment and realized that none exists.
So, I thought I'd add one to 7.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| comment_publish.patch | 1.74 KB | Idle | Failed: Failed to install HEAD. | View details | Re-test |
Comments
#1
looks good. maybe swap the doxygen for each param so they match the order in the function signature
#2
I had just copied this from the other actions in the file which had misleading doxygen.
So, here is a new version of the patch which fixes the doxygen and some trailing whitespace. I apologize to the kittens :/
#3
Nice doxy cleanup.
I will rtbc after that.
#4
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).
#5
The last submitted patch failed testing.
#6
Patch re-roll.
#7
and a Drupal 6 version
#8
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.
#9
Patch re-roll, thanks Arancaytar for the review!
#10
Please add a test for this to actions.test. Thanks!
#11
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?
#12
Thank you so much for the D6 version! Just what I needed.
#13
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.
#14
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.
#15
code and docs look great to me
#16
'description' is 'label' now.
Did I mark #582316: Add tests for actions as critical bug report? Yes, I did. ;)
#17
Also, please make sure before committing this:
I see no reason for why this patch could kill 12,000+ tests, so I hope the new patch will do better.
#18
Better title. Last patch also passed tests properly now.
#19
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.
#20
The last submitted patch failed testing.
#21
Re-rolled against latest HEAD.
#22
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.
#23
.
#24
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.
#25
sub
is #21 patch ready to back port to D6?
#26
Tests already in and a bit tuned at #699596: (Un)publishing several comments doesn't work
#27
Here's a backport of #21 for D6
I think it's a bug and also great fix for docs!
#28
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"?
#29
subscribe
#30
#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. :(
#31
@nilesh.3892 do you patch against current CVS? I've tested this and patch still cleanly applies
Could someone confirm and RTBC this
#32
I patched it against D6.16
#33
tested against 6.16 and it's fine
#34
What patch command did you issue ?
#35
As always http://drupal.org/patch/apply
wget http://drupal.org/files/issues/360023-action-comment-publish-d6.patchpatch -p0 < 360023-action-comment-publish-d6.patch
#36
Hmm, I think I missed that -p0
#37
So could you RTBC this? or we need more reviews?
#38
Its working with -p0.
#39
#40
Oh, great, thanks, committed.
#41
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.
#42
I am using VBO, but it didn't provide the action by default. I had to apply the patch.
#43
re@42 agreed. VBO is the org. point where i find this thread and fixed by apply patch.
#44
Automatically closed -- issue fixed for 2 weeks with no activity.
#46
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?
#47
Better title and version. We should fix in 7.x first and then port back.
#48
oops - wrong issue. deleted
#49
@cyberpunk No, you're not the only one. Should be a fairly simple patch. Will need tests too.
#50
Here's a patch that adds the needed calls to the actions, needs tests, and testing.
#51
Code-comment should end with comma
#52
Actually should be a full-stop instead of the colon.
#57
Didn't realize this still needed reviews. Thanks comment spam for reminding.
#58
Fixed tags.
#59
#60
#61
*sigh*
#62
Hi, this is a sort of backport of #50 patch for Drupal 6