Votes tab show 20 votes, not 20 rows.

Thanks for the bug report and patch. You're right that the incorrect number of votes are being displayed and that this is a bit better, although it will display too many votes if anyone did not submit a complete ranking and therefore the list can still be cut off mid-vote.

Taking a quick look, it seems like what we really need is a unique ID per vote that we can aggregate by - we should probably try to get this added to VotingAPI. Another option would be to include a timestamp with each vote, although this would be slightly unreliable on high-traffic sites with multiple anonymous votes from the same IP (at the same second).

This still needs to be fixed. And I noticed a few weeks back that Voting API already sets a unique id per vote, so this won't be too hard to implement.

Bump - I'm going to do this sometime in the next week. It'll make it easy to add a delete vote link for each row too.

Still need to fix this.

Okay here is a patch.

Btw, here is one possible testing procedure: create a poll.module poll with 3 choices and pre-populate each choice with 6 votes (18 in total), convert it to an Advanced Poll, change the choice limit back to 0, then vote on all the available choices (so that there 19 voters but 21 voted on choices). With this patch the votes tab should only show one page, but without the page the votes will run over to two pages.

Following those steps, only two choices are listed for the advpoll voter.

Ugh, I could have sworn there was one vote_id per vote... but apparently I am crazy. Maybe this issue will motivate me to drop VotingAPI and just reimpliment the few features that we specifically use.

No, the content_id is the node id. There isn't anything that signifies a vote by a user - you have to just get all the rows for a uid-hostname-timestamp combination. Not fun.

For non-anonymous users, isn't content_type + content_id + uid a single vote by a user in our case? It's anonymous users that's the special case here, right?

Right

Here is a revised patch that actually works for the test procedure I proposed, imagine that.

Indeed, the testcase works.

Errors are generated if a poll doesn't have a vote.

That last chunk of code is slightly scary. Feel free to expand that code comment about security a little bit, e.g. what we are trying to protect ourselves from.

So basically what you are doing here is to check by id for logged in users and by timestamp + hostname for anonymous (?). Also feel free to elaborate on what is missing in VotingAPI to deal with this more cleanly (just an extra column for an id per vote?). How serious are you about ditching VotingAPI, is it something that should be considered for 6.x? (sorry for getting slightly off topic).

Great work as always Chris!

Thanks for the solid review.

Fixed the no-votes error - sorry, should have caught that myself.

Added clearer code comments; this will definitely be handy when reviewing the code in the future. I found out how to use placeholders for array data at http://drupal.org/writing-secure-code#comment-185423 (kind of hard to find).

Yep, VotingAPI just needs another column that is a unique id for the set of rankings a user submits as their vote. The current conception assumes that a vote consists of only a single value, rather than potentially a group of values. I'm not too set on ditching VotingAPI, it's just frustrating at times. Maybe I'll restart submitting patches to it so I can fix its problems myself.

Those code comments are great!

The check doesn't work though. How about using if (count($uids) > 0) instead and remove $votes = array(); which doesn't seem to be used. I'd roll a new patch but think there might be another issue:

Tried my old nemesis ranking poll and all I get is "=" as seperator. Looked at the code but couldn't figure it out...

Ugh buggy mc. bugster. Should be fixed.

Nice.

Committed: http://drupal.org/cvs?commit=91491