Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid creating duplicate voters in polls #5532

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Avoid creating duplicate voters in polls #5532

wants to merge 9 commits into from

Conversation

javierm
Copy link
Member

@javierm javierm commented May 11, 2024 •
edited by taitus

References

Objectives

  • Prevent the creation of duplicate poll voters
  • Prevent the creation of multiple poll answers on single-answer questions
  • Add a task to delete duplicate voters, so we can later add a unique index
  • Remove unused code

Notes

We can't introduce a unique index yet; otherwise, the migration introducing it will crash if there are duplicate records. So we'll add the unique index to the poll_voters table after releasing version 2.2.0.

Release notes

ℹ️ Due to a race condition, until now, there was about a 1 in 10,000 chance to get duplicate records when people voted in a poll. This release fixes that (see pull requests #5532 and #5539) and, as part of the release tasks, provides tasks to remove the duplicate records. A log file log/duplicate_records.log will contain the information about the deleted records. As usual, backup your database before upgrading.

Sponsored

Functionality developed by

@javierm javierm added Bug Polls security Pull requests that address a security vulnerability labels May 11, 2024
@javierm javierm self-assigned this May 11, 2024
@javierm javierm added this to Reviewing in Consul Democracy May 11, 2024
github-actions[bot]
github-actions bot reviewed May 11, 2024
View reviewed changes
app/models/user.rb Outdated Show resolved Hide resolved
@javierm javierm force-pushed the poll_duplicate_voters branch 3 times, most recently from ff166db to 377e3f2 Compare May 13, 2024 22:31
@javierm javierm changed the base branch from master to rename_question_answer_to_option May 13, 2024 22:31
@javierm javierm force-pushed the rename_question_answer_to_option branch from 433ed8f to a4779f5 Compare May 13, 2024 23:20
@javierm javierm force-pushed the poll_duplicate_voters branch 3 times, most recently from d403177 to 1756667 Compare May 13, 2024 23:22
@javierm javierm mentioned this pull request May 14, 2024
Open
@javierm javierm force-pushed the poll_duplicate_voters branch 7 times, most recently from ffdb562 to d7efc4b Compare May 15, 2024 18:35
@javierm javierm force-pushed the rename_question_answer_to_option branch from a4779f5 to 3284871 Compare May 17, 2024 20:50
@javierm javierm force-pushed the poll_duplicate_voters branch 2 times, most recently from 63b25c4 to be02718 Compare May 17, 2024 21:18
@taitus taitus self-assigned this Jun 7, 2024
@javierm javierm force-pushed the rename_question_answer_to_option branch 2 times, most recently from 87540aa to 0e31139 Compare June 7, 2024 19:42
@javierm javierm force-pushed the rename_question_answer_to_option branch 3 times, most recently from 785e0e4 to 6cb3c14 Compare June 12, 2024 18:17
@javierm javierm force-pushed the rename_question_answer_to_option branch 3 times, most recently from 49ac93b to eb8b66b Compare June 13, 2024 14:50
@javierm javierm force-pushed the rename_question_answer_to_option branch from 8d6cece to 8420ea1 Compare June 13, 2024 17:13
@javierm javierm force-pushed the rename_question_answer_to_option branch from 8420ea1 to ca8329d Compare June 13, 2024 17:38
Consul Democracy automation moved this from Reviewing to Testing Jun 14, 2024
Base automatically changed from rename_question_answer_to_option to master June 14, 2024 13:19
@javierm javierm added the 2.2 label Jun 14, 2024
@javierm
Fix exception creating an answer without an author
b844225 
We were getting `undefined method `lock!' for nil:NilClass` when the
question allowed multiple answers.
@javierm
Remove Questionable concern
73f4a11 
Since we were only using it in one place, it made the code harder to
follow.

We'll extract it again if we ever find a way to reuse it.
@javierm
Add missing validation rule to poll answers
cd52177 
We were checking we didn't have more votes than allowed in the case of
questions with multiple answers, but we weren't checking it in the case
of questions with a single answer. This made it possible to create more
than one answer to the same question. This could happen because the
method `find_or_initialize_user_answer` might initialize two answers in
different threads, due to a race condition.
@javierm
Use with_lock instead of lock!
6897a03 
That way the record is only locked while necessary.
@javierm
Remove no longer needed call to Poll::Answer#touch
8ece336 
This call was added in commit 81f65f1, and the test for its need was
added in commit cb15428. However, both the test and the helper method
relying on the `touch` call were removed in commit f90d0d9.
@javierm
Remove unused answer_id field in poll_voters table
73836c3 
This field isn't used since commit 51be80e, right after being
added in commit 5806d86.
@javierm
Prevent creation of duplicate poll voters
0a63dbf 
Note that, when taking votes from an erased user, since poll answers
don't belong to poll voters, we were not migrating them in the
`take_votes_from` method (and we aren't migrating them now either).
@javierm
Add task to delete duplicate voters
2876aad 
Note that, since poll answers belong to a user and not to a voter, we
aren't doing anything regarding poll answers. This is a separate topic
that might be dealt with in a separate pull request.

Also note that, since there are no records belonging to poll voters, and
poll voters don't use `acts_as_paranoia` and don't have any callbacks on
destroy, it doesn't really matter whether we call `destroy!` or
`delete`. We're using `delete` so there are no unintended side-effects
that might affect voters with the same `user_id` and `poll_id` on
Consul Democracy installations customizing this behavior.
@javierm
Add a log file to track deleted duplicate records
bb0154b 
It might be interesting in some cases to check the information related
to those records.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions left review comments

@taitus taitus taitus approved these changes

Assignees

@taitus taitus

@javierm javierm

Labels
2.2 Bug Polls Release notes security Pull requests that address a security vulnerability
Projects
Consul Democracy
  
Testing
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@javierm @taitus