Posted by dixon_ on May 7, 2010 at 9:06pm
5 followers
| Project: | Vote Up/Down |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | postponed |
Issue Summary
I'll start to say that I really like the 2.x branch of this module. Really nice!
I have one feature request, though. And that is to have the possibility to switch voting on/off per individual content. I think that it would be a great addition to this module.
vud.module provides generic methods for saving and deleting the status per content piece. The attached patch isn't complete, but working for nodes. I decided to submit this patch early to get the ball rolling. I'll add support for comments and terms very soon.
I think we'll need to add an upgrade path too, even though this module is in active development, I think it's a good idea.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| vub-per-content.patch | 7.95 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vub-per-content.patch. | View details | Re-test |
Comments
#1
Here is a new version with better performance and better documentation of our new API functions.
I haven't been able to test my new additions (since I'm on a train without a dev environment). But I'll give a test as soon as possible.
#2
There were some logical errors in the last patch. Try this one instead.
#3
Ok, I'm off the train and back to my dev environment. I've now tested and adjusted my patch a bit (the patch was made from the wrong path previously). And everything seems to work as expected now.
#4
Thanks for the patch, but I'm really not sure if we want this feature, it would be great to hear about what other maintainers think about it.
#5
let's postpone this for the next major version
#6
#7
bad status, sorry
#8
The last submitted patch, vub-per-content-3.patch, failed testing.
#9
Here is a much cleaner patch that also let's you set the default voting status (on/off) for each enabled content type.
The patch applies to DRUPAL-6--2.
#10
Actually, this patch only *changes* one line of code. The rest is new backward compatible code. So it would be possible to get this into the 6.x-2.x branch without much hassle I think.
#11
The last submitted patch, vud-per-content-9.patch, failed testing.
#12
+++ vud.module 9 Jun 2010 11:51:13 -0000@@ -193,3 +193,30 @@ function vud_ctools_plugin_directory($mo
+/**
+ * Helper function to save status for a piece of content.
+ */
+function vud_status_save($type, $content_id, $status) {
+ db_query("SELECT status FROM {vud_status} WHERE type = '%s' AND content_id = %d", $type, $content_id);
+ if (db_affected_rows() > 0) {
+ db_query("UPDATE {vud_status} SET status = %d WHERE type = '%s' AND content_id = %d", $status, $type, $content_id);
+ }
+ else {
+ db_query("INSERT INTO {vud_status} (type, content_id, status) VALUES ('%s', %d, %d)", $type, $content_id, $status);
+ }
+}
I think you want to use drupal_write_record()
+++ vud_node/vud_node.module 9 Jun 2010 11:51:13 -0000@@ -132,7 +155,7 @@ function vud_node_nodeapi(&$node, $op, $
+ if ((($can_edit=user_access('use vote up/down on nodes')) || user_access('view vote up/down count on nodes')) && vud_node_is_valid_node($node)) {
can you please explain why we need that extra validation there.. I think we can just call the function and validate it later, since in that way we are changing the logic. Not sure about it, so if you can explain it would be great.
+++ vud_node/vud_node.module 9 Jun 2010 11:51:13 -0000@@ -293,3 +316,131 @@ function vud_node_link($type, $object, $
+function vud_node_is_valid_node($node, $reset = FALSE) {
I'm not sure about the name of this function, since it is interacting with cache. BTW why are we using
static $cacheinstead ofcache_get/cache_set. It would be great if you can elaborate about the cache for node types and nids associated with vud_node.+++ vud_node/vud_node.module 9 Jun 2010 11:51:13 -0000@@ -293,3 +316,131 @@ function vud_node_link($type, $object, $
+/**
+ * Implementation of hook_content_extra_fields().
+ */
+function vud_node_content_extra_fields($type_name) {
+ if (vud_node_is_valid_type($type_name)) {
+ $extra = array();
+ $extra['vud'] = array(
+ 'label' => t('Vote Up/Down'),
+ 'description' => t('Vote Up/Down settings'),
+ 'weight' => 0,
+ );
+ }
+ return $extra;
+}
I think this piece is another patch for #582624: Expose widget to CCK fields reordering
Maybe we also want a hook_update_N to populate the vud_status table?
Powered by Dreditor.
#13
Here is a new patch that is a little better documented and with some motivations:
+++ vud_node/vud_node.module 10 Jun 2010 15:02:47 -0000@@ -132,8 +153,7 @@ function vud_node_nodeapi(&$node, $op, $
+ if ((($can_edit=user_access('use vote up/down on nodes')) || user_access('view vote up/down count on nodes')) && vud_node_is_valid_node($node)) {
Since voting can be disabled for some nodes, we have to check that it's a valid node we are working with. That function does two thing: 1) checks to see if the node is of a valid type 2) checks if voting is enabled for that particular node.
+++ vud_node/vud_node.module 10 Jun 2010 15:02:47 -0000@@ -293,3 +311,119 @@ function vud_node_link($type, $object, $
+function vud_node_is_valid_node($node, $reset = FALSE) {
I'm using
static $cachejust to cache the result of a validation during that request. If, for some reason, you have to validate a node twice during the same request, the result is being statically cached inside the function. And when working with static variables it's always good practice to have a$resetparameter if you explicitly what to skip the cache and revalidate the node. Usingcache_set()andcache_get()would result in more of an overhead I think. The same goes forvud_node_is_valid_type().Here are some new stuff too:
+++ vud_node/vud_node.module 10 Jun 2010 15:02:47 -0000
@@ -248,8 +264,10 @@ function vud_node_link($type, $object, $
+ if (!vud_node_is_valid_node($node)) {
+ return;
+ }
$votes_display_mode = variable_get('vud_node_votes', VUD_NODE_DISPLAY_BOTH);
- $node_type = in_array($node->type, variable_get('vud_node_types', array()), TRUE);
$widget_theme = variable_get('vud_node_widget', 'plain');
$tag = variable_get('vud_tag', 'vote');
$view_vud_node_votes_count = user_access('view vote up/down count on nodes') || user_access('use vote up/down on nodes');
@@ -257,7 +275,7 @@ function vud_node_link($type, $object, $
@@ -257,7 +275,7 @@ function vud_node_link($type, $object, $
case VUD_NODE_DISPLAY_NO:
break;
case VUD_NODE_DISPLAY_TEASER_ONLY:
- if (($teaser == 1) && $node_type && $view_vud_node_votes_count) {
+ if (($teaser == 1) && $view_vud_node_votes_count) {
I've removed the the check for
$node_typein bothvud_node_nodeapi()andvud_node_links()since that check now is done by the newvud_node_is_valid_node(). This also does result in a little better performing code because the check is made earlier.+++ vud.module 10 Jun 2010 15:02:46 -0000@@ -193,3 +193,35 @@ function vud_ctools_plugin_directory($mo
+function vud_status_save($type, $content_id, $status) {
+ $object = array(
+ 'type' => $type,
+ 'content_id' => $content_id,
+ 'status' => $status,
+ );
+ db_query("SELECT status FROM {vud_status} WHERE type = '%s' AND content_id = %d", $type, $content_id);
+ if (db_affected_rows() > 0) {
+ drupal_write_record('vud_status', $object, array('type', 'content_id'));
+ }
+ else {
+ drupal_write_record('vud_status', $object);
+ }
+}
I actually didn't know (for some strange reason)
drupal_write_record()could take an array of primary keys on which to update the record on. We naturally doesn't have access to the real primary key on{vud_status}, which I thought was needed. This is now fixed.I've also riped out the part with the ability to reorder the output through CCK. As you say, that is better handled here: #582624: Expose widget to CCK fields reordering.
I see that the tests doesn't pass. That's probably because the tests isn't adjusted to this new functionality. I'll look into this in a new roll later on.
#14
I know that the patch won't pass tests yet. But I'll mark this as NR to have human eyes on it.
#15
The last submitted patch, vud-per-content-13.patch, failed testing.
#16
dixon_: Thanks for the updates!
mm strange error or testbot, but the patch looks good, let me comment a little more
+++ vud.install 10 Jun 2010 15:02:46 -0000@@ -17,7 +17,41 @@ function vud_install() {
+ 'vudid_type_content_id' => array('vudid', 'type', 'content_id'),
since vudid is serial, we only need the other two as one unique
+++ vud_node/vud_node.module 10 Jun 2010 15:02:47 -0000@@ -132,8 +153,7 @@ function vud_node_nodeapi(&$node, $op, $
+ if ((($can_edit=user_access('use vote up/down on nodes')) || user_access('view vote up/down count on nodes')) && vud_node_is_valid_node($node)) {
since in this place we have
$node->vud_statusset(on 'load'), maybe is better to validate that value and the$node_type/call-the-new-type-cached-validate ;-)+++ vud_node/vud_node.module 10 Jun 2010 15:02:47 -0000@@ -248,8 +264,10 @@ function vud_node_link($type, $object, $
+ if (!vud_node_is_valid_node($node)) {
+ return;
+ }
IMHO it's cleaner to return an empty array
+++ vud_node/vud_node.module 10 Jun 2010 15:02:47 -0000@@ -293,3 +311,119 @@ function vud_node_link($type, $object, $
+function vud_node_is_valid_node($node, $reset = FALSE) {
A little naming change, IMHO it's better to use
vud_node_is_valid_node.+++ vud_node/vud_node.module 10 Jun 2010 15:02:47 -0000@@ -293,3 +311,119 @@ function vud_node_link($type, $object, $
+function vud_node_is_valid_type($type_name, $reset = FALSE) {
The same little naming change, IMHO it's better to use
vud_node_is_valid_node_type.On other comments:
we need to make a
hook_update_N()implementation to add the new table.Powered by Dreditor.
#17
Is anyone working on doing this for comments too?
#18
I like this idea, and wanted to chime in with a feature request. It would be nice to have per content type access according to role, letting anonymous users vote on some content types but not others for instance.
#19
I have this working on a per node basis but would love to have it work this way for the comments too. I haven't quite figured out how to apply it to the comments yet. Is this something you have worked on yet?
#20
Do not hijack issues, please read the guidelines.
What you want is probably solved by other issue #877392: Add hook that allows modules to alter voting permissions already committed to 6.x-3.x
#21
Thanks marvil and apologies. I will make a new thread next time
#22
No more features for 6.x-3.x now that it is the stable branch, moving to 7.x-1.x as postponed until basic port is ready.