Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

Status: Active » Needs review
moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

We're using this script on Economist so I'll RTBC it.

I like this change in README: +VotingAPI requires Drupal.

stewsnooze’s picture

subscribe

Junro’s picture

subscribe :)

Scott Reynolds’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.92 KB

So orginal patch works, but this attached patch is a "I think this is a better approach, no offense" patch. This patch makes the operation Batch API'd and adds a form button to the votingapi settings page. Click the button and rebuild your cache.

I have tested it with a combination of about 100,000 votes on a mixture of nodes, comments, and users

greggles’s picture

I think the warning that it could take a long time should be on the main form in addition to the confirmation form. People generally ignore text on confirm forms.

Other than that, shell scripts are easier to write and less likely to be run by accident. I'm not sure this needs to be truly "hard to run by accident" so I see this as an improvement. Didn't test or do a thorough review of the code, though..

Scott Reynolds’s picture

Also think that i probably should do a batch_set() for each content type. Removes a lot of the bookkeeping in there.

No harm in adding the warning message again.

Scott Reynolds’s picture

FileSize
6.36 KB

attached patch does the two afore mentioned things.
1.) adds the message to the button
2.) uses an operation per content_type in the batch api call

Interestingly,

$form['results']['help'] = array(
    '#prefix' => '<div>',
    '#type' => 'markup',
    '#value' => t('This action rebuilds all voting results, and may be a lengthy process.'),
    '#attributes' => array('class' => 'description'),
    '#suffix' => '</div>',
  );

didn't work even though 'markup' is listed to respect #attributes: http://api.drupal.org/api/file/developer/topics/forms_api_reference.html...

@eaton ^^ ?

Junro’s picture

Hi,

just tried the last patch #8 but He doesn't work for me.

I have in /admin/settings/votingapi/rebuild the button "Rebuild all Voting Results" but when I click on it, the page reload without effects. I don't have anything that confirms any changes.

Anyway Voting results is not rebuilt.

And what about 2.) uses an operation per content_type in the batch api call ?
I can't choose node type.

Scott Reynolds’s picture

pretty strange. When you run this query: SELECT DISTINCT(content_type) FROM votingapi_vote does it return anything? Is your votingapi_vote table empty?

Junro’s picture

My content type is "fiche"

When I run this query: SELECT DISTINCT(fiche) FROM votingapi_vote, it return me:

Erreur

requête SQL:

SELECT DISTINCT (
fiche
)
FROM votingapi_vote
LIMIT 0 , 30

MySQL a répondu:
#1054 - Unknown column 'fiche' in 'field list'

I'm not a big expert in SQL

Scott Reynolds’s picture

right, you need to run this query exactly
SELECT DISTINCT(content_type) FROM votingapi_vote

Not
SELECT DISTINCT(fiche) FROM votingapi_vote

Junro’s picture

oh ok, sorry

here the results:

Affichage des enregistrements 0 - 29 (1 306 total, Traitement en 0.0003 sec.)

SELECT DISTINCT (
content_type
)
FROM votingapi_vote
LIMIT 0 , 30

Scott Reynolds’s picture

Status: Needs review » Needs work

guess its not working still ok.

Junro’s picture

Nothing new since the last post?

I'm still here to test some patches :) Really needs to rebuild the votingapi cache ^^

Scott Reynolds’s picture

o duh! you have to clear your menu cache. try existing patch, and visit admin/build/modules first before hitting that button

/me kicks himself forgetting that

Junro’s picture

Status: Needs work » Needs review

Perfect! This patch is great! I don't know why it didn't work the first time I apllied it...

Scott Reynolds’s picture

Status: Needs review » Reviewed & tested by the community

sweet good to here, marking as reviewed and tested. Im using in other places as well.

Junro’s picture

Hum that's wird, it didn't work for one vote (maybe more? I haven't see another one yet).

I cleared view cache, performance cache...

What do you call "menu cache"?

you say "and visit admin/build/modules first before hitting that button", I visit admin/build/modules, to do what? Maybe I haven"t carry out all spots before hitting thaht button! lol

Scott Reynolds’s picture

Status: Reviewed & tested by the community » Needs work

ok...

The menu callbacks and urls are cached away to the database. This patch adds a new url, so that cache needs to be rebuilt

By going to admin/build/modules, that cache is rebuilt. You don't have todo anything.

But I use devel module to clear the caches (http://drupal.org/project/devel)

Junro’s picture

Hum ok, I don't really understand why this vote is still here if the votingapi cache has been rebuilt successfully. Is this vote was hidding somewhere? lol

I've got votes from 3 users to erase before launch my website, it was votes for tests.

I will report you if I have some news. :)

ps: I don't use Devel module.

thekayra’s picture

The patch at #8 worked quite well for me at first glance. Will report if I come accross something strange.

hefox’s picture

Hm, it worked, but it did produce the whole double-menu-rebuild warnings and I wasn't clearing cache elsewhere, so I think it's being a bit too much cache clearing though I don't see where normal drupal cache was being cleared atm.

Scott Reynolds’s picture

Status: Needs work » Needs review

I have sucessfully used this all the time. The major need for it was developing a voting function for a social news site. I have not run into any of the problems mentioned above. It doesn't do anything to menu_rebuild. And it doesn't miscalculate.

Junro’s picture

Hello,

Any news about this patch? Any modification?

Every time I rebuilt the cache api, only 3008 pieces of content is recalculated.

My votingapi_cache table contains 8 861 pieces.

So I think the recalculaion is not complete.

And some votes are not erased on my site.

Any ideas? :)

Scott Reynolds’s picture

3008 nodes were recalculated and resulted in 8,861 different cache results. This is expected.

This has nothing to do with vote erasing....

Essentially, when you calculate vote totals on a node, there are several different totals that are calculated. The standard ones are 'average', 'sum' and 'count'.

Do you have different calculations? Are some points and others percent ? Points have different ones.

Again, I use this almost daily at this point, as I import my database without the votingapi cache table and then rebuild it.

Junro’s picture

Oh ok, thanks

Do you have an idea why some votes are still in the voting api cache table? Or some tricks to do about it?

Junro’s picture

I don't use percent but points with 'average', 'sum' and 'count'.

Can I erase things directly in the table (phpmyadmin) without problems? The Voting api cache will be rebuild automatically?

kruser’s picture

subscribing - I did a manual table conversion from Wordpress WP-PostRatings now I just need to rebuild the votingapi cache to get the imported data to show up.

edit: running the #1 php script got it working for me. The patch didn't seem to do anything.

Scott Reynolds’s picture

The patch didn't seem to do anything.

Please explain. did you apply the patch, then clear you cache (it adds a new menu item). Did you go to the Votingapi settings and click the big button ?

Did the patch not apply?

kruser’s picture

Okay, I see it now. I just had to clear cache and the new menu link came up for admin/settings/votingapi/rebuild - so the patch does work!

Junro’s picture

Hello,

I still have this problem:

Every time I rebuilt the cache api, only 3008 pieces of content is recalculated.
My votingapi_cache table contains 8 861 pieces.
So I think the recalculaion is not complete.
And some votes are not erased on my site.

My rebuild ajax bar stop à 10% and directly go at 100%

Maybe it's why I don't have all votingapi cache rebuild.

Someone could tell me if he has the same behaviour? or the ajax bar go from 0% to 100%, step by step (with 65%, 66%, 67%...)?

Scott Reynolds’s picture

3008 x 3 ~= 8 861

the 3 would be 'count', 'sum', 'average'. As mentioned this is expected.

And some votes are not erased on my site.

What does that mean ? Votes erased? This doesn't erase votes, it recalculates the totals from all the votes cast by users.

Junro’s picture

And some votes are not erased on my site.

I mean some votes are still in the votingapi cache table. They still appear in views.

I clear the cache so many times. (Site cache, view cache, run cron...) No way, still have them in votingapi cache.

the 3 would be 'count', 'sum', 'average'. As mentioned this is expected.

So, you're agree there is only one of those that is recalculated. Is is expected, why? All three should be recalculated, doesn't it?

In my views, I'm using "count" and "average". Not "sum" and not the value of a single vote.

And there is a problem here too: I've got 3800 pieces of content recalculated, but I have 7000 votes on my website. All these votes should be recalculated?

Scott Reynolds’s picture

votingapi cache isn't cleared by "clearing the cache". Its a badly name table because its not really a cache table.

So, you're agree there is only one of those that is recalculated.

Huh? I don't understand. Every piece of content that has a vote in votingapi_cache table has their three vote results recalculated (count, sum, average). It doesn't delete anything.

In my views, I'm using "count" and "average". Not "sum" and not the value of a single vote.

There is no way for VotingAPI to know this.

This patch goes through every content that has received at least one vote and updates the sum, average and count values in the votingapi_cache table.

Hopefully that helps you understand. And maybe there is a bug in there.

Junro’s picture

Ok, all is cleared for me now.

So the bug is:

Not all nodes are recalculated. Only 50% are recalculated.

Rebuild the voting api cache should affect all votes, isn't it?

Scott Reynolds’s picture

So your saying that there are pieces of content in the system that have at least one vote that are not recalculated ?

It doesn't go "Give me all nodes and recalculate" it does "Give me all nodes that received at least on vote and recalculate".

Junro’s picture

So your saying that there are pieces of content in the system that have at least one vote that are not recalculated ?

Exactly, no doubt about that.

It doesn't go "Give me all nodes and recalculate" it does "Give me all nodes that received at least on vote and recalculate".

Yes, sure.

If you need some proof, details, screenshots or anything that could be helpfull to understant this problem, let me know :)

Scott Reynolds’s picture

Status: Needs review » Needs work
+    $context['sandbox']['max'] = db_result(db_query("SELECT MAX(content_id) FROM {votingapi_vote} WHERE content_type = '%s'", $type));

needs to be

+    $context['sandbox']['max'] = db_result(db_query("SELECT COUNT(content_id) FROM {votingapi_vote} WHERE content_type = '%s'", $type));
Junro’s picture

The process stopped at 10% before.

Now, it continues until 51% and nothing, I had wait 15 minutes but still nothing, still at 51%.

When I want to go to another page, i have:

An error has occured.
Thanks to continue to the error page.
An error HTTP 0 has occured. /batch?id=815&op=do

When I reload the page:

Finished with an error.
on admin/settings/votingapi page.

DamienMcKenna’s picture

This would be *great* as a Drush command :)

Junro’s picture

It sill doesn't work well for me. The provessus stoppes at 29% now. More I have votes, more the processus stop early (normal).
After x votes recalculated, the processus stoppes.

DamienMcKenna’s picture

Status: Needs work » Reviewed & tested by the community

@Scott Reynolds: the correct query should be MAX(), not COUNT(), you're trying to find the highest content_id value, not the number of values.

In my case I had ~1,400 votes across two different "content_types" (one was 'node', another the name of an actual content type) and it worked fine.

Am changing this to RTBC.

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Needs work

Changing back to Needs Work after Junro's comment.

jwilson3’s picture

If votes are attached to cck fields via, for example, fivestar module that stores the votes in the content_type_X tables for voting on other nodes... then this script will not find those votes, and thus the cache may be rebuilt without these tallies.

This could explain why some users (@Junro perhaps?) are not seeing all their votes rebuilt after running this script. This may be related to whats also being described in #1027146: Aggregate functions in views.

jwilson3’s picture

To summarize...

The remaining issues that need work include the following:

  • Don't require moving a PHP script to the Drupal root to run the update.
  • Create a Batch API-compatible update script for rebuilding the votingapi_cache table (capable of actually completing without timeouts), that could be run from the admin UI, drush, or elsewhere (eg cron).
  • Support both normal votes (from the votingapi_vote table), as well as votes stored in other places. This implies that the VotingAPI cache rebuild function should itself be an API, for modules like fivestar to extend and implement for their own tallying needs.
  • Incorporate cache-rebuild instructions into module documentation (as proposed in OP).

Have I missed anything? (reserve the right to come back and update this comment ;)

incaic’s picture

subscribing

Junro’s picture

@ jwilson3

All my votes (even those with fivestars module) are stored in the votingapi table.

Junro’s picture

Status: Needs work » Reviewed & tested by the community

10736 pieces of content recalculated

First time I have the recalculation completed!! :)

I don't know why I didn't have it before...

InTheLyonsDen’s picture

Has anyone ported this to D7? *banging head on table*

mmilo’s picture

Version: 6.x-2.x-dev » 7.x-2.x-dev
Status: Reviewed & tested by the community » Active

Indeed, would be great to have this ported to D7. An update:

There *is* a Drush command, but to rebuild the entire voting cache for a node type, we need #1399800: Need to Import 9k Votes - Votingapi_cache not being rebuilt committed.

Things that could be worked on...

  • Create a Batch API-compatible update script for rebuilding the votingapi_cache table (capable of actually completing without timeouts), that could be run from the Admin UI
  • Incorporate cache-rebuild instructions into module documentation (as proposed in OP).

Support both normal votes (from the votingapi_vote table), as well as votes stored in other places. This implies that the VotingAPI cache rebuild function should itself be an API, for modules like fivestar to extend and implement for their own tallying needs.

Note sure if this is entirely necessary. The code already has drupal_alter('votingapi_results', $cache, $entity_type, $entity_id);. Is that enough?

mmilo’s picture

Status: Active » Needs review
FileSize
6.06 KB

Attached is a patch for D7 based on #8.

torotil’s picture

Status: Needs review » Needs work

Hi,
thanks for your patches and sorry for the long silence.

I've reviewed the patch:

There is some whitespace errors in this patch - please fix them.

Using the entity_id for navigating through the introduces some flaws:

  1. If some entities have been deleted or entities of multiple types have been voted on the batch size is calculated wrongly. I think it's better to use the vote_cache_id instead.
  2. The batch job doesn't guarantee to recalculate all results if there are two entities with different types but with the same entity_id.
zhuber’s picture

Status: Needs work » Needs review
FileSize
6.03 KB

I use this module on various sites, and really want this to be ported into the module. It seems to work well for me.

Therefore, I have updated the patch to remove the excess whitespace in hopes that it will get included in the dev release.

I didn't update it to use voting_cache_id, since that would require another table join. We can still do this, but for now I am submitting the cleaned-up version of #52.

DamienMcKenna’s picture

Status: Needs review » Needs work

@zhuber: That's a good direction, but the patch needs some updates to match the Drupal coding standards. Also, you might want to rename the new rebuild page to "Rebuild voting results", and votingapi_rebuild_form() needs a comment.

zhuber’s picture

Status: Needs work » Needs review
FileSize
5.93 KB

Is this better?

I cleaned a lot of things up, but I'm not sure if I missed anything.

zhuber’s picture

Sorry for the spam, let me try that again. The last one was missing the updated menu and page titles.

DamienMcKenna’s picture

Status: Needs review » Needs work

@zhuber: This is really close. A few small nitpicks:

  • The closing */ on the comment for votingapi_rebuild_form needs to be indented.
  • FYI votingapi_rebuild_form isn't an implementation of hook_form, it's a Form API callback.
  • In votingapi_rebuild_form there isn't any need to define $form=array() as it's passed in as an argument to the function.
  • The second sentence in the comment for votingapi_rebuild_form_submit needs a period at the end and doensn't need the link to the d.o API docs page.
  • All comments must be proper sentences with a capital on the first letter and a period at the end.
  • All comments must wrap at 80 characters.

Have you tried the patch on D6? I might try a reroll for one of the sites I maintain.

zhuber’s picture

Status: Needs work » Needs review
FileSize
5.86 KB

Here is an updated version. Thanks for all the direction.

I haven't tried anything with the D6 patches, since I am using fivestar and votingapi heavily on a few D7 sites.

GoofyX’s picture

I have applied the patch (by hand, since it threw out some errors) in the latest 2.11 version for the 7.x branch. How am I supposed to invoke the rebuild functionality? I tried to access the link (admin/config/search/votingapi/rebuild) but it shows the general options page. What am I doing wrong?

zhuber’s picture

@GoofyX:

Do you have the 'administer voting api' permisson on your user account?

You should have the 'Rebuild Settings' tab in addition to the 'General Settings' tab. Either you do not have the correct permissions or the patch was not implemented correctly.

torotil’s picture

Thanks to all for putting so much work into this!

+++ votingapi.admin.inc	(working copy)
@@ -78,3 +114,100 @@
+  // Grab all entity_ids from the votingapi_vote table.
+  $result = db_query_range('SELECT entity_type, entity_id FROM {votingapi_vote} WHERE entity_id > 0 AND tag = :tag AND entity_id > :cid ORDER BY entity_id  ASC', 0, $limit, array(':tag' => $tag, ':cid' => $context['sandbox']['current_entity_id']));
+
+  foreach($result as $row) {
+    watchdog('votingapi', 'trying type @type @entity', array('@type' => $type, '@entity' => $row->entity_id), WATCHDOG_ERROR);
+
+    $context['sandbox']['progress']++;
+    $context['sandbox']['current_entity_id'] = $row->entity_id;
+    // Force vote recalculation for this entity.
+    votingapi_recalculate_results($row->entity_type, $row->entity_id, TRUE);
+    $context['results']['count']++;
+  }

This still works only as long as there aren't two entities with the same entity_id but different entity_types. This could be fixed by also sorting (and filtering) by entity_type. I still prefer a voting_cache_id-based solution though. You don't need an extra join for that either: Just use the votingapi_cache table instead.

Also there is a possible issue with setting setting current_entity_id before the result for the entity is saved. What if votingapi_recalculate_results() somehow fails?

torotil’s picture

Status: Needs review » Needs work
emilymoi’s picture

I also couldn't get [#59] to apply--Malformed patch. I ended up applying by hand and recreated the patch so others won't have to do the same.

I've done a bit of testing and it seems like it's doing the job so far. Not sure why votingapi has been without this functionality for so long.

roderik’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.8 KB

I still prefer a voting_cache_id-based solution though. You don't need an extra join for that either: Just use the votingapi_cache table instead.

The initial idea for this patch was to rebuild the votes when the votingapi_cache table was deleted - so we can't do that. I think we need to keep selecting entity_id/entity_type combinations from the votingapi_vote table. (And these are not unique, so we need to use DISTINCT here.)

It's possible... it just doesn't work well with dividing the process up into batches of 100 records. You need to account for the possibility that records 100 & 101 have the same entity ID. I created code to check that, which unfortunately makes things harder to follow...

Also (and I see this was also mentioned in #45 / #46): votes from comments are not re-counted. So I attached help text to say that.

roderik’s picture

FileSize
8.03 KB

removing cruft from previous patch...

torotil’s picture

The initial idea for this patch was to rebuild the votes when the votingapi_cache table was deleted - so we can't do that.

There is another reason: There might be multiple vote_cache_ids for one combination of entity_type-entity_id-tag since the function may vary. This makes vote_cache_ids less handy than they looked like. The empty table could have been solved easily by pre-populating the table.

+++ b/votingapi.admin.inc
@@ -86,3 +86,173 @@ function votingapi_generate_votes_form_submit($form, &$form_state) {
+  $result = db_query_range('SELECT DISTINCT entity_type, entity_id FROM {votingapi_vote} WHERE tag = :tag AND entity_id >= :cid ORDER BY entity_id  ASC',
+    0, $limit, array(':tag' => $tag, ':cid' => $context['sandbox']['current_entity_id']));

What about using ORDER BY entity_id, type together with entity_id>:current_entity_id OR (entity_id=:current_entity_id AND entity_type>:current_entity_type)? That would make each entity_id/entity_type combination unique.

The calculation of the progress-bar is still a bit off. Isn't there a better solution for that?

roderik’s picture

Status: Needs review » Needs work

What about using ORDER BY entity_id, type together with entity_id>:current_entity_id OR (entity_id=:current_entity_id AND entity_type>:current_entity_type)? That would make each entity_id/entity_type combination unique.

Good idea, I think you're right, would make the first 'if' wrapper inside the 'foreach' unnecessary, and 'current_entity_types' array can be simplified to a 'current_entity_type' scalar.

Sadly it looks like I don't have time to look at / test this code more.

The calculation of the progress-bar is still a bit off. Isn't there a better solution for that?

Not one I am aware of. See code comments.

roderik’s picture

Status: Needs work » Needs review
FileSize
7.29 KB
6.28 KB

Incorporated comments. I also added some code (min_entity_id) to make the progress bar seems less random. Still not perfect but should be better.

pifagor’s picture

pifagor’s picture

Version: 7.x-2.x-dev » 7.x-3.x-dev
Status: Needs review » Active
pifagor’s picture

Status: Active » Needs review
FileSize
8.11 KB
pifagor’s picture

Status: Needs review » Needs work

The last submitted patch, 72: votingapi-382866-71.patch, failed testing. View results

  • pifagor committed c9ac757 on 7.x-2.x
    Issue #382866 by zhuber, roderik, Scott Reynolds, greggles, pifagor,...
pifagor’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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