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
| Comment | File | Size | Author |
|---|---|---|---|
| #28 | 67895_poll_data_loss_d5.28.patch | 2.88 KB | dww |
| #19 | poll-data-loss-fix.patch | 2.82 KB | gábor hojtsy |
| #4 | kratib-poll-module-20061013.patch | 1.04 KB | infojunkie |
Comments
Comment #1
stevenpatzis this still an issue for cvs?
Comment #2
dwwyes. no one's fixed this, yet. (and while i understand the bug, i've had no time to write a patch for it). :(
Comment #3
magico commentedThis 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
Comment #4
infojunkieAttached 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.
Comment #5
rkerr commentedThe 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.
Comment #6
killes@www.drop.org commentedapplied to 4.7, please check for relevance on 5-dev.
Comment #7
bloomaniac commentedshould be commited to 5.x.
I have the same problem.
Comment #8
bloomaniac commentedthe same in 6.x.
Comment #9
gábor hojtsyWow, 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.
Comment #10
catchsetting version and patch status.
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.
Comment #11
moshe weitzman commentedWould be nice to fix this before release
Comment #12
dwwEither we fix this or just remove poll.module from core. ;)
Comment #13
gábor hojtsySo 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.
Comment #14
dww@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.
Comment #15
gábor hojtsyWe 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.
Comment #16
gábor hojtsyWell, 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.
Comment #17
dwwYou forgot to attach the patch. ;)
Comment #18
gábor hojtsyNo, 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
Comment #19
gábor hojtsyComment #20
gábor hojtsyBTW 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.
Comment #21
JirkaRybka commentedI 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.
Comment #22
blackdog commentedTested:
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.
Comment #23
gábor hojtsyGreat, thanks for testing, committed.
Comment #24
gábor hojtsyNext in line BTW is disallow anonymous voting automatically when page caching is turned on, patch here: http://drupal.org/node/39432#comment-662630
Comment #25
(not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #26
dwwWhoops, 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... ;)
Comment #27
dwwFYI:
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.
Comment #28
dwwHere's a lightly tested reroll. Seems to work just fine.
Comment #29
drummCommitted to 5.x.
Comment #30
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.