Widget ignores settings

heres a more elaborate patch that takes into account your fixes as well
the other fixes are in fivestar_nodeapi(), if you dont enable the default "vote"-tag, the other ones will not be enabled as well, also position is taken from the default one, ive fixed these 2 issues, so you can just display your own custom tags without the default one, and positions are independent of each other (one can be static, one above, one below, it doesnt take these from the default "vote" one anymore)

--- fivestar.module_dist        2010-05-06 12:57:21.000000000 +0000
+++ fivestar.module     2010-05-06 13:50:57.000000000 +0000
@@ -256,8 +256,8 @@ function fivestar_vote($type, $cid, $tag

   $suffix = fivestar_get_suffix((!isset($node) ? 'default' : $node->type), $tag);

-  $stars = variable_get('fivestar_stars' . $suffix, 5);
-  $feedback_enable = variable_get('fivestar_feedback' . $suffix, 1);
+  $stars = variable_get('fivestar_stars_' . $suffix, 5);
+  $feedback_enable = variable_get('fivestar_feedback_' . $suffix, 1);

   $output .= '<xml><result>';

@@ -493,52 +493,66 @@ function fivestar_nodeapi(&$node, $op, $
         NODE_BUILD_SEARCH_RESULT,
         NODE_BUILD_RSS,
       );
-      if (!in_array($node->build_mode, $exclude_modes) && !isset($node->modr8_form_teaser) && variable_get('fivestar_'. $node->type, 0)) {
-        if ($teaser) {
-          $position = variable_get('fivestar_position_teaser_'. $node->type, 'above');
-        }
-        else {
-          $position = variable_get('fivestar_position_'. $node->type, 'above');
+
+      $fivestar_node_enabled = 0;
+      foreach (fivestar_get_tags() as $tag) {
+        if (fivestar_validate_target('node', $node->nid, $tag)) {
+          $fivestar_node_enabled = 1;
         }
+      }

-        switch ($position) {
-          case 'above':
-          case 'below':
-            if (user_access('rate content')) {
-              $content = '';
-              foreach (fivestar_get_tags() as $tag) {
-                if (fivestar_validate_target('node', $node->nid, $tag)) {
-                  $content .= fivestar_widget_form($node, $tag);
-                }
-              }
-              if ($content) {
-                $node->content['fivestar_widget'] = array(
-                  '#value' => $content,
-                  '#weight' => $position == 'above' ? -10 : 50,
-                );
-              }
+      if (!in_array($node->build_mode, $exclude_modes) && !isset($node->modr8_form_teaser) && $fivestar_node_enabled) {
+
+        $content_above = $content_below = '';
+        foreach (fivestar_get_tags() as $tag) {
+          if (!fivestar_validate_target('node', $node->nid, $tag)) {
+            continue ;
+          }
+          $suffix = fivestar_get_suffix($node->type, $tag);
+
+          if ($teaser) {
+            $position = variable_get('fivestar_position_teaser_'. $suffix, 'above');
+          }
+          else {
+            $position = variable_get('fivestar_position_'. $suffix, 'above');
+          }
+
+          switch ($position) {
+            case 'above':
+            case 'below':
+              if (user_access('rate content')) {
+                if (strpos($position, 'above') === 0)
+                  $content_above .= fivestar_widget_form($node, $tag);
+                else
+                  $content_below .= fivestar_widget_form($node, $tag);
+                break;
+              } // Fall through to static if not allowed to rate.
+              $position .= '_static';
+            case 'above_static':
+            case 'below_static':
+              if (strpos($position, 'above') === 0)
+                $content_above .= fivestar_static('node', $node->nid, $node->type, $tag);
+              else
+                $content_below .= fivestar_static('node', $node->nid, $node->type, $tag);
               break;
-            } // Fall through to static if not allowed to rate.
-            $position .= '_static';
-          case 'above_static':
-          case 'below_static':
-            $content = '';
-            foreach (fivestar_get_tags() as $tag) {
-              if (fivestar_validate_target('node', $node->nid, $tag)) {
-                $content .= fivestar_static('node', $node->nid, $node->type, $tag);
-              }
-            }
-            if ($content) {
-              $node->content['fivestar_widget'] = array(
-                '#value' => $content,
-                '#weight' => strpos($position, 'above') === 0 ? -10 : 50,
-              );
-            }
-            break;
-          default:
-            // We'll do nothing.
-            break;
+            default:
+              // We'll do nothing.
+              break;
+          }
         }
+        if ($content_above) {
+          $node->content['fivestar_widget_above'] = array(
+            '#value' => $content_above,
+            '#weight' => -10,
+          );
+        }
+        if ($content_below) {
+          $node->content['fivestar_widget_below'] = array(
+            '#value' => $content_below,
+            '#weight' => 50,
+          );
+        }
+
       }
       break;
   }
@@ -649,8 +663,8 @@ function fivestar_form(&$form_state, $co
   }

   $suffix = fivestar_get_suffix($node->type, $tag);
-  $star_display = variable_get('fivestar_style' . $suffix, 'average');
-  $text_display = variable_get('fivestar_text' . $suffix, 'dual');
+  $star_display = variable_get('fivestar_style_' . $suffix, 'average');
+  $text_display = variable_get('fivestar_text_' . $suffix, 'dual');

   if ($star_display == 'average' && ($text_display == 'average' || $text_display == 'none')) {
     // Save a query and don't retrieve the user vote unnecessarily.
@@ -667,18 +681,17 @@ function fivestar_form(&$form_state, $co
   );

   $settings = array(
-    'stars' => variable_get('fivestar_stars' . $suffix, 5),
-    'allow_clear' => variable_get('fivestar_unvote' . $suffix, FALSE),
+    'stars' => variable_get('fivestar_stars_' . $suffix, 5),
+    'allow_clear' => variable_get('fivestar_unvote_' . $suffix, FALSE),
     'style' => $star_display,
     'text' => $text_display,
     'content_type' => $content_type,
     'content_id' => $content_id,
-    'tag' => 'vote',
     'autosubmit' => TRUE,
-    'title' => variable_get('fivestar_title' . $suffix, 1) ? NULL : FALSE,
-    'feedback_enable' => variable_get('fivestar_feedback' . $suffix, 1),
-    'labels_enable' => variable_get('fivestar_labels_enable' . $suffix, 1),
-    'labels' => variable_get('fivestar_labels' . $suffix, array()),
+    'title' => variable_get('fivestar_title_' . $suffix, 1) ? NULL : FALSE,
+    'feedback_enable' => variable_get('fivestar_feedback_' . $suffix, 1),
+    'labels_enable' => variable_get('fivestar_labels_enable_' . $suffix, 1),
+    'labels' => variable_get('fivestar_labels_' . $suffix, array()),
     'tag' => $tag,
   );

abaddon, your code is good.

But the patch does not work, i get the following output:

patching file fivestar.module
patch unexpectedly ends in middle of line

however, manually making changes in the code solves the problem.
This should be commited ASAP in the dev version, for it is simply not working ATM.

I thank you for the effort!
Regards.

+1

This patch was rolled against DRUPAL-6--2 today. It is the fixes mentioned in #2. I haven't had time to roll through #3.

This patch fixes the primary problem, that of the fivestar widget not accepting the saved settings like for displaying 10 stars instead of just 5.

Index: fivestar.module
===================================================================
RCS file: /cvs/drupal-contrib/contributions/modules/fivestar/fivestar.module,v
retrieving revision 1.32
diff -u -p -r1.32 fivestar.module
--- fivestar.module	3 Nov 2009 04:34:29 -0000	1.32
+++ fivestar.module	22 Dec 2010 04:06:19 -0000
@@ -667,18 +667,17 @@ function fivestar_form(&$form_state, $co
   );
 
   $settings = array(
-    'stars' => variable_get('fivestar_stars' . $suffix, 5),
-    'allow_clear' => variable_get('fivestar_unvote' . $suffix, FALSE),
+    'stars' => variable_get('fivestar_stars_' . $suffix, 5),
+    'allow_clear' => variable_get('fivestar_unvote_' . $suffix, FALSE),
     'style' => $star_display,
     'text' => $text_display,
     'content_type' => $content_type,
     'content_id' => $content_id,
-    'tag' => 'vote',
     'autosubmit' => TRUE,
-    'title' => variable_get('fivestar_title' . $suffix, 1) ? NULL : FALSE,
-    'feedback_enable' => variable_get('fivestar_feedback' . $suffix, 1),
-    'labels_enable' => variable_get('fivestar_labels_enable' . $suffix, 1),
-    'labels' => variable_get('fivestar_labels' . $suffix, array()),
+    'title' => variable_get('fivestar_title_' . $suffix, 1) ? NULL : FALSE,
+    'feedback_enable' => variable_get('fivestar_feedback_' . $suffix, 1),
+    'labels_enable' => variable_get('fivestar_labels_enable_' . $suffix, 1),
+    'labels' => variable_get('fivestar_labels_' . $suffix, array()),
     'tag' => $tag,
   );

This one takes into account both instances. I am seeing trouble with the average vote sent to the theme function. I am not sure it's getting populated (the average vote) in the array.

Anyway, this code appears to solve a majority of the problems. Please test this patch and modify as necessary. patched against DRUPAL6--2-- on CVS. (Head is D7).

Joe

Index: fivestar.module
===================================================================
RCS file: /cvs/drupal-contrib/contributions/modules/fivestar/fivestar.module,v
retrieving revision 1.32
diff -u -p -r1.32 fivestar.module
--- fivestar.module	3 Nov 2009 04:34:29 -0000	1.32
+++ fivestar.module	23 Dec 2010 04:57:57 -0000
@@ -256,8 +256,8 @@ function fivestar_vote($type, $cid, $tag
 
   $suffix = fivestar_get_suffix((!isset($node) ? 'default' : $node->type), $tag);
 
-  $stars = variable_get('fivestar_stars' . $suffix, 5);
-  $feedback_enable = variable_get('fivestar_feedback' . $suffix, 1);
+  $stars = variable_get('fivestar_stars_' . $suffix, 5);
+  $feedback_enable = variable_get('fivestar_feedback_' . $suffix, 1);
 
   $output .= '<xml><result>';
 
@@ -370,10 +370,10 @@ function fivestar_get_votes($type, $cid,
 
   $results = votingapi_select_results($criteria);
   foreach ($results as $result) {
-    if ($result['function'] == 'average') {
+    if ($result->function == 'average') {
       $votes['average'] = $result;
     }
-    if ($result['function'] == 'count') {
+    if (strpos($result->function,'count')) {
       $votes['count'] = $result;
     }
   }
@@ -493,58 +493,65 @@ function fivestar_nodeapi(&$node, $op, $
         NODE_BUILD_SEARCH_RESULT,
         NODE_BUILD_RSS,
       );
-      if (!in_array($node->build_mode, $exclude_modes) && !isset($node->modr8_form_teaser) && variable_get('fivestar_'. $node->type, 0)) {
-        if ($teaser) {
-          $position = variable_get('fivestar_position_teaser_'. $node->type, 'above');
+      $fivestar_node_enabled = 0;
+      foreach (fivestar_get_tags() as $tag) {
+        if (fivestar_validate_target('node', $node->nid, $tag)) {
+          $fivestar_node_enabled = 1;
         }
-        else {
-          $position = variable_get('fivestar_position_'. $node->type, 'above');
-        }
-
-        switch ($position) {
-          case 'above':
-          case 'below':
-            if (user_access('rate content')) {
-              $content = '';
-              foreach (fivestar_get_tags() as $tag) {
-                if (fivestar_validate_target('node', $node->nid, $tag)) {
-                  $content .= fivestar_widget_form($node, $tag);
+      }    
+      if (!in_array($node->build_mode, $exclude_modes) && !isset($node->modr8_form_teaser) && $fivestar_node_enabled) {
+        $content_above = $content_below = '';
+        foreach (fivestar_get_tags() as $tag) {
+          if (!fivestar_validate_target('node', $node->nid, $tag)) {
+            continue;
+          }
+          $suffix = fivestar_get_suffix($node->type, $tag);
+          if ($teaser) {
+            $position = variable_get('fivestar_position_teaser_'. $suffix, 'above');
+          }
+          else {
+            $position = variable_get('fivestar_position_'. $suffix, 'above');
+          }
+          switch ($position) {
+            case 'above':
+            case 'below':
+              if (user_access('rate content')) {
+                if (strpos($position, 'above') === 0) {
+                  $content_above .= fivestar_widget_form($node, $tag);
+                }
+                else {
+                  $content_below .= fivestar_widget_form($node, $tag);
                 }
+                break;
+              } // Fall through to static if not allowed to rate.
+              $position .= '_static';
+            case 'above_static':
+            case 'below_static':
+              if (strpos($position, 'above') === 0) {
+                $content_above .= fivestar_static('node', $node->nid, $node->type, $tag);
               }
-              if ($content) {
-                $node->content['fivestar_widget'] = array(
-                  '#value' => $content,
-                  '#weight' => $position == 'above' ? -10 : 50,
-                );
+              else {
+                $content_below .= fivestar_static('node', $node->nid, $node->type, $tag);
               }
               break;
-            } // Fall through to static if not allowed to rate.
-            $position .= '_static';
-          case 'above_static':
-          case 'below_static':
-            $content = '';
-            foreach (fivestar_get_tags() as $tag) {
-              if (fivestar_validate_target('node', $node->nid, $tag)) {
-                $content .= fivestar_static('node', $node->nid, $node->type, $tag);
-              }
+             default:
+               // We'll do nothing.
+               break;
             }
-            if ($content) {
-              $node->content['fivestar_widget'] = array(
-                '#value' => $content,
-                '#weight' => strpos($position, 'above') === 0 ? -10 : 50,
-              );
-            }
-            break;
-          default:
-            // We'll do nothing.
-            break;
+          }
+          if ($content_above) {
+            $node->content['fivestar_widget_above'] = array(
+              '#value' => $content_above,
+              '#weight' => -10,
+            );
+          }
         }
+        break;
       }
-      break;
-  }
 }
 
 
+
 /**
  * Implementation of hook_link().
  *
@@ -649,8 +656,8 @@ function fivestar_form(&$form_state, $co
   }
 
   $suffix = fivestar_get_suffix($node->type, $tag);
-  $star_display = variable_get('fivestar_style' . $suffix, 'average');
-  $text_display = variable_get('fivestar_text' . $suffix, 'dual');
+  $star_display = variable_get('fivestar_style_' . $suffix, 'average');
+  $text_display = variable_get('fivestar_text_' . $suffix, 'dual');
 
   if ($star_display == 'average' && ($text_display == 'average' || $text_display == 'none')) {
     // Save a query and don't retrieve the user vote unnecessarily.
@@ -667,18 +674,17 @@ function fivestar_form(&$form_state, $co
   );
 
   $settings = array(
-    'stars' => variable_get('fivestar_stars' . $suffix, 5),
-    'allow_clear' => variable_get('fivestar_unvote' . $suffix, FALSE),
+    'stars' => variable_get('fivestar_stars_' . $suffix, 5),
+    'allow_clear' => variable_get('fivestar_unvote_' . $suffix, FALSE),
     'style' => $star_display,
     'text' => $text_display,
     'content_type' => $content_type,
     'content_id' => $content_id,
-    'tag' => 'vote',
     'autosubmit' => TRUE,
-    'title' => variable_get('fivestar_title' . $suffix, 1) ? NULL : FALSE,
-    'feedback_enable' => variable_get('fivestar_feedback' . $suffix, 1),
-    'labels_enable' => variable_get('fivestar_labels_enable' . $suffix, 1),
-    'labels' => variable_get('fivestar_labels' . $suffix, array()),
+    'title' => variable_get('fivestar_title_' . $suffix, 1) ? NULL : FALSE,
+    'feedback_enable' => variable_get('fivestar_feedback_' . $suffix, 1),
+    'labels_enable' => variable_get('fivestar_labels_enable_' . $suffix, 1),
+    'labels' => variable_get('fivestar_labels_' . $suffix, array()),
     'tag' => $tag,
   );
 
@@ -889,7 +895,6 @@ function fivestar_custom_widget(&$form_s
       $form['vote']['#attributes']['class'] .= ' fivestar-combo-text';
       break;
   }
-
   switch ($settings['style']) {
     case 'average':
       $form['vote']['#title'] = t('Average');

The above patches broke the display of the vote totals on the fivestar widget. So this is maintenance on the above, and I think makes this patch ready for primetime. Please test.

This patch fixes the confusion between the votingapi's type of object (or array) returned as the cached votes results. Looks like we've fluctuated between object $results and array $results. Now the vote totals will appear on the fivestar widget as advertised. It was an array() of values, not an object.

This patch rolled against fivestar-DRUPAL-6--2.

Tested against votingapi version 6.x-2.x-dev:

; $Id: votingapi.info,v 1.4 2007/07/06 03:02:34 eaton Exp $
name = Voting API
description = Provides a shared voting API for other modules.
package = Voting
core = 6.x
; Information added by drupal.org packaging script on 2010-11-12
version = "6.x-2.x-dev"
core = "6.x"
project = "votingapi"
datestamp = "1289564961"

Patch in #8 works well.

Thank you!

I think we've got code unrelated to the issue in the #7. This issue is about widget settings. If there are bugs related to the way the VotingAPI formats data, let's address those in another issue. I believe this patch contains the relevant fixes for the scope of this issue.

It gets even better. With some debugging code added I saw that with the "Enable Fivestar rating for the [tag] tag" checkbox unchecked, the submit hanlder for the Fivestar settings form was never called. Craziness.

I tracked down to the theme function theme_fivestar_node_type_tag_form() -- Preventing this from running by commenting out its definition in fivestar_theme() resolved the issue. Mysterious.

Further troubleshooting showed that turning off javascript in the browser allowed the form submit handler to run regardless of the checkbox state.

Break out the Firebug net tool and it turns out in fivestar-admin.js, $options = $('#fivestar-node-type-tag-form input:not(#edit-fivestar, #edit-submit), #fivestar-node-type-tag-form select'); has the effect of unsetting *all* of the form elements, including hidden Form API elements such as the form id that we're submitting, resulting in a virtually empty POST when submitting the form, and thus the reason submit handlers never ran.

Here's a revised patch that fixes the above by adding a containing div and doing a jQuery show() and hide(), rather than setting elements to disabled. This also changes the name of the 'fivestar' setting to 'fivestar_enable' which is somewhat more descriptive.

@ezra-g won't you need an update hook for changing variables?

Yes indeed.

Here's a re-roll without the variable name change, which would also have to be taken into account in at least one other place. Let's keep the convention.

Missed an underscore, which made the above not actually work ;). I've tested a bit this time.

I marked #494150: Fivestar voting widget is only displayed if vote tag is enabled as a duplicate since they're both part of the same symptom "widget ignores settings" and we want to fix the symptom, rather than individual causes separately, which is more difficult over 2 patches.

This still needs an update function for old variables to include the 'tag' name, since this is seems like a better way of providing an upgrade path rather than always looking for outdated variable names.

Marking as NR so folks can verify the overall approach. I've tested with 2 tags and a few different display settings.

With dpm removed :\.

The widget settings are also not respected in the Views integration. I am working on a patch locally.

The overall problem here is that FIvestar 2.x introduced the ability to vote on multiple VotingAPI tags per content type, but the module doesn't take this into account pretty much wherever it loads the settings for the Fivestar widget. So while previously you had one set of settings per node type and one for comments, you now have one set of widget settings per content type *per tag*. This still needs an update function to change old variable names, but at least gets the Views voting widget working in my testing.

And, there's quite a bit of testing required, since you can set the display location of the full node view and teaser (among other settings) for each content type for each tag -- That's a lot of permutations!

In some places we were checking the widget setting value 'style' and in others 'star_display' for the same setting, causing previous vote values to not be loaded into the widget. This patch fixes that. Yay.

Marked #463840: Feedback and Glitches on 2.x-dev as a duplicate.

Here's a revision that fixes a potential foreach error in some views.

Guess who found more broken Views integration :) ? This time in the static widget.

Slight revision.

I found one more case of Views integration not respecting tag settings and fixed it.

Given that there was never a stable release of this branch and that it's difficult to work on other patches when the module at the core doesn't respect its own settings, I've moved old variable cleanup to #1052524: Clean out old variables in an update function and committed. http://drupal.org/cvs?commit=495324

I credited jghyde and googletorp for their work in this and other issues relating to this underlying problem of the 2.x branch having been started but not really finished. I think there was some overlap in identifying problems as a result of not seeing all the comments in all the issues related to this one. In some places we came to somewhat different solutions, in some places they were more similar.

Back in 2009, quicksketch said,

...right now the 2.x version is so broken it's not really usable for any purpose.

Here's to a 6.x-2.x branch that's, dare I say, not totally broken!

Hopefully this is helpful for the 7.x-2.x port that ericduran is leading.

I think there was some overlap in identifying problems as a result of not seeing all the comments in all the issues related to this one.

For example, the fivestar-admin.js javascript preventing the settings form from submitting. Folks identified and proposed solutions to this in at least three different places:

Myself in the present issue: http://drupal.org/node/786224#comment-4043906
#786244: Always enabled tag
#463840: Feedback and Glitches on 2.x-dev

However, it wasn't necessarily clear from issue titles and contents how these issues were relevant to the problem, at least without closer reading through potentially a lot of issues.

just an initial comment to the last updates, ive tried this instead of my own modifications in #3 and the votes get saved ok but the voting widget on the node doesnt have any labels, their order is changed and the cancel button is gone
the settings for the respective tags are to show the widget title and allow undo and i resaved the settings form, but still the same
i never tested the patches from jghyde but they look the same
i dont have more time to review this now, if it helps i can attach my copy of the patched module that works for me
i wont reopen the issue because im not sure about where the problem is