this is a bug against all versions, not just 4.6.x, but i figured i should mark it as the earliest version still being maintained...

in testing http://drupal.org/node/51561, in noticed that when you edit a poll node, all records from the {poll_votes} table are immediately deleted. :( this means that after you edit a poll, users can vote again, etc, etc.

i believe this behavior is based on the idea that while editing a node, you could change which values are what, etc. however, it's evil and wrong that fixing a typo in an existing poll option removes all historical votes on the whole poll...

i think we need to remove this behavior from the default edit, and add a new button (which must be confirmed) to "clear all votes" or something...

i'll work on a patch at some point, just recording it here so i don't forget, and/or so if someone else is inspired to fix this sooner, they can. ;) if a core maintainer thinks this should only be fixed in 4.7.x or head, please move the version. otherwise, i'll plan to roll patches against all 3.

thanks,
-derek

Comments

stevenpatz’s picture

is this still an issue for cvs?

dww’s picture

yes. no one's fixed this, yet. (and while i understand the bug, i've had no time to write a patch for it). :(

magico’s picture

Version: 4.6.8 » 4.7.3

This problem does not happen and the table {poll_votes} does not exist in 4.6.x
So I suppose this is a 4.7.x bug

infojunkie’s picture

Status: Active » Needs review
StatusFileSize
new1.04 KB

Attached is a patch that resolves this bug. The strategy used is somewhat different from the solution proposed above. Initially, I was planning on adding a checkbox "Reset voters" to control the clearing of {poll_votes}, but then I realized that the vote editor has the ability to reset the vote counts anyway. So what I do now is to clear {poll_votes} *only if all the vote counts have been reset to 0*. The rationale behind it is that if I leave the vote counts as they are, then I don't want to lose the votes. On the other hand, if I reset the counts, then I want to let users vote again. Of course, if I change the vote counts but don't reset them all to zero, my intention is ambiguous and currently the patch does not clear the voters.

rkerr’s picture

The patch looks good, and is reported by killes to work :)
I would personally change the "0 == $total_votes" line 21 to be if (!$total_votes) to as a matter of code style.

killes@www.drop.org’s picture

Version: 4.7.3 » 5.x-dev

applied to 4.7, please check for relevance on 5-dev.

bloomaniac’s picture

should be commited to 5.x.
I have the same problem.

bloomaniac’s picture

Version: 5.x-dev » 6.0-beta1

the same in 6.x.

gábor hojtsy’s picture

Wow, this looks like a nasty bug. But what if I remove poll options? The votes will be kept for them as it seems. This might have side effects elsewhere (ie. in a SQL based summary calculation based on nid or something like that). I am not sure chvotes is used in all cases and the node_votes table is only used to not allow users to vote more than once.

catch’s picture

Version: 6.0-beta1 » 6.x-dev
Status: Needs review » Needs work

setting version and patch status.

the node_votes table is only used to not allow users to vote more than once.

My experience has been that authenticated users can vote more than once in polls since it does the check on session rather than uid (iirc) - this leads to some odd behaviour with the poll block changing back and forth between form and results depending on browser session rather than voting. Will double check that's still in 6.x but a different issue to this so will open one if necessary.

moshe weitzman’s picture

Would be nice to fix this before release

dww’s picture

Either we fix this or just remove poll.module from core. ;)

gábor hojtsy’s picture

So the problem again with this latest patch is that

- you can remove poll choices
- poll choices are renumbered, when saved

As poll_votes has a nid, uid, chorder, hostname combo saved (chorder is the number of the vote casted), users who vote previously will have their vote moved to a different option. That's not exactly desirable. Votes cast previously to the removed option should be deleted, the other votes need to be renumbered. Otherwise we leave the vote table in an inconsistent state.

dww’s picture

@Gabor: my point is that it's not worth our time to fix this... Apologies for the duplicate, see http://drupal.org/node/61285 instead.

gábor hojtsy’s picture

Assigned: Unassigned » gábor hojtsy
Priority: Normal » Critical

We are way past the time to make radical changes to core, we are in bugfixing (I wish to say release, but it is not true yet) mode. I claim this task ;) and will provide a fix in a day hopefully. Marked critical as suggested in the other issue dww mentions, although I don't agree it is critical.

gábor hojtsy’s picture

Status: Needs work » Needs review

Well, I did not have time through the day, due to core patches flowing in an unprecedented pace, but just sat down with this to keep my promise. It was not a big deal at all though. Let's see why stuff is removed at all:

- you can remove any of the poll options by emptying their titles
- poll options are renumbered starting from 0 each time you edit and save a poll

So if you remove options from the middle, the recorded votes will point to bugos options or will point to missing options which were moved to smaller indexes.

My patch does the following:

- renumbers the votes as an option is renumbered, so votes go with options
- *removes* votes for an option, when an option gets removed

I thought some about actually keeping the votes for removed options, and assigning them to choice number -1 or something. But frankly, people who voted earlier to an option which got removed later should have their chance to vote again. Anyway, if you think we should keep votes for removed options, we can, but as we have no option to relate the votes to, we need to point them to the same nonexistent choice, eg choice -1. That looks quite ugly, and IMHO not fair to those who had their options removed.

dww’s picture

Status: Needs review » Needs work

You forgot to attach the patch. ;)

gábor hojtsy’s picture

No, I did not forget, but it does not work right now:

[10:48pm] emsearcy: catch: right now the webroot's are in a `split-brain' state without replication, so file uploads are intentionally broken because they would only show up on certain servers. we're restarting the nfs server in order for an upgrade to solve this very problem
[10:48pm] catch: emsearcy: cool. just checkin'
[10:48pm] nnewton: we apologize for any inconvenience, but this will eventually enhance performance
[10:49pm] • catch does not currently have a patch to upload
[10:57pm] goba: emsearcy: I do have a patch, so awaiting what happens

gábor hojtsy’s picture

Status: Needs work » Needs review
StatusFileSize
new2.82 KB
gábor hojtsy’s picture

BTW I talked about UI improvements in the "remove poll from core" thread. What I meant is that we can add a delete checkbox column to the poll table, which would allow users to find out that they can actually delete options. Now the deletion by title empty workflow is not even documented. The checkbox column could work/look like in upload module. At the end I did however concentrate on fixing the actual bug at hand, so we can discuss that. If this is in, we can lament about possible UI improvements.

JirkaRybka’s picture

Status: Needs review » Reviewed & tested by the community

I tested this patch in all combinations I can think of (6.x-dev, 4 users; voting, adding/removing/renaming voted/unvoted/first/last options, changing vote counts, previews, inspecting the list of all votes all the time, observing how I'm not/allowed to vote under different logins...) All work pretty fine. The fact that removing an option also removes it's votes, allowing the affected users to vote for some other then (i.e. allocate their now free votes elsewhere), makes good sense. I also examined the code carefully, all fine.

So setting RTBC.

As for the UI for removing choices, I think removing by emptying the title is sufficient as-is (happens only rarely - I never needed that yet - so no need for a column of checkboxes IMO - after all, this is facing poor authenticated users while submitting, quite probably not allowed to edit even). I would suggest to put a little bit of description to that form, explaining that - but that's UI, and so doesn't hold the patch, I quite agree.

blackdog’s picture

Tested:

Created a new Poll, voted, edited it, votes gone.

Patched poll.module next.

Created a new poll, voted, edited, votes still there. The patch also fixed issues with cancelling votes.

gábor hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Great, thanks for testing, committed.

gábor hojtsy’s picture

Next in line BTW is disallow anonymous voting automatically when page caching is turned on, patch here: http://drupal.org/node/39432#comment-662630

Anonymous’s picture

Status: Fixed » Closed (fixed)

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

dww’s picture

Version: 6.x-dev » 5.x-dev
Assigned: gábor hojtsy » Unassigned
Status: Closed (fixed) » Patch (to be ported)

Whoops, this was never backported. I *almost* edited a poll on g.d.o just now, and remembered this bug, and wondered if it was going to nail me on g.d.o. Seems like it will. Yikes. Love that poll module... ;)

dww’s picture

Status: Patch (to be ported) » Needs work

FYI:
a) g.d.o does indeed use core poll module, and is susceptible to this bug.
b) the patch from #19 doesn't apply cleanly to D5, and needs to be rerolled/backported.

dww’s picture

Assigned: Unassigned » dww
Status: Needs work » Needs review
StatusFileSize
new2.88 KB

Here's a lightly tested reroll. Seems to work just fine.

drumm’s picture

Status: Needs review » Fixed

Committed to 5.x.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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