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.

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

Fix races that lead to duplicate votes creation #211

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fatkodima
Copy link
Contributor

Closes #181
Closes #127

I have also modernized a bit migration file style, validations and added a changelog file (style is adapted from rubocop).

The most complicated part of this would be for existing users upgrade their apps. I have added a detailed enough description to the changelog, but it would be nice for someone to try that on a production app and see how that goes.

@ryanto
Copy link
Owner

ryanto commented Dec 19, 2020

Ok I think this one is next! I'm excited to get your fix published.

Want to rebase it for specs?

@fatkodima
Copy link
Contributor Author

Rebased.

@yrashk
Copy link

yrashk commented Sep 3, 2021

Useful change!

@ryanto Any plans to get this in?

@ryanto
Copy link
Owner

ryanto commented Nov 8, 2021

Heya!

Yes, I want to get this in. This approach is great and it's what we will be using. If you want to use a fork of this PR I'd say go for it!

This PR is a big change, and these sorts of change always end up generating a ton of support issues from folks that don't read the upgrade instructions. There's a tradeoff I need to balance: not supporting concurrency vs all the people that will ignore the upgrade guides, break their acts_as_votable installs once this is merged, and then yell at me :).

I think the best way to deal with this is to put this PR in its own big release. Here's my currently plan.

  • When rails 7 comes out release acts_as_votable 1.0
  • Shortly after release acts_as_votable 2.0 with this PR + upgrade guide.

This will help make it clear that upgrading from 1.0 to 2.0 has breaking changes and users need to follow the upgrade instructions.

Sorry it has taken so long to get this merged. No good excuse... will happen soon(ish).

@oyenmwen
Copy link

oyenmwen commented May 7, 2024

Heya!

Yes, I want to get this in. This approach is great and it's what we will be using. If you want to use a fork of this PR I'd say go for it!

This PR is a big change, and these sorts of change always end up generating a ton of support issues from folks that don't read the upgrade instructions. There's a tradeoff I need to balance: not supporting concurrency vs all the people that will ignore the upgrade guides, break their acts_as_votable installs once this is merged, and then yell at me :).

I think the best way to deal with this is to put this PR in its own big release. Here's my currently plan.

  • When rails 7 comes out release acts_as_votable 1.0
  • Shortly after release acts_as_votable 2.0 with this PR + upgrade guide.

This will help make it clear that upgrading from 1.0 to 2.0 has breaking changes and users need to follow the upgrade instructions.

Sorry it has taken so long to get this merged. No good excuse... will happen soon(ish).

any update on this?

@anil-adepu
Copy link

Any updates here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After so many years, concurrency problem still exists Duplicates (race condition)
5 participants