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

Allow Self Love with Thumbs Up #65

Merged
merged 4 commits into from Feb 15, 2013

Conversation

pboling
Copy link

@pboling pboling commented Feb 14, 2013

Where is the love?

I have Users that need to vote on other Users (hypothetically, of course).

class User
  acts_as_voter     # People vote ...
  acts_as_voteable  # On other people
end

This adds two has_many relationships to the class with the second obscuring the first:

  has_many :votes, :as => :voter, :dependent => :destroy
  has_many :votes, :as => :voteable, :dependent => :destroy

With this pull request the relationship name is configurable, though by default stays as :votes for both, so it is fully backward compatible. Now you can do the following in an initializer:

ThumbsUp.configure do |config|
  config.voteable_relationship_name = :votes_by
  config.voter_relationship_name    = :votes_on
end

Now on any given user:

u = User.first
u.votes_on # => the votes that have been cast on this user
u.votes_by # => the votes that have been cast by this user

Thanks for the great voting tool!

Backwards Compatability

  • 100%
  • Didn't even need to alter any test code
  • Defaults preserve the old behavior

Tests

  • They all pass
  • I updated the README instructions with what I had to do to get it running on latest postgres, and latest mysql

@bouchard bouchard merged commit 8035f07 into bouchard:master Feb 15, 2013
@bouchard
Copy link
Owner

This is a very well written pull request, thanks for contributing! Merged.

@pboling
Copy link
Author

pboling commented Feb 16, 2013

I missed another typo. I re-used the configuration I added to I18n::Airbrake as the template - and I miseed an I18n in a comment. :( Submitted another PR with typo fix.

@pboling
Copy link
Author

pboling commented Apr 2, 2013

@bouchard - I have realized that, although the primary relationship name change is working fine here, the votes method is used directly in most of the methods in ActsAsVoteable. I need to re-direct those calls to the dynamic _votes_by which will call the configured relationship. Once I do that, this will be fully working.

As long as people don't use this new configuration yet this merge won't cause any issues. But the new configurable relationship names are not fully working yet. I'll fix it soon and send another PR.

@bouchard
Copy link
Owner

bouchard commented Apr 2, 2013

Sounds good! Would you mind adding failing tests for the current configuration when you do, please?

On 2013-04-02, at 7:13 PM, Peter Boling notifications@github.com wrote:

@bouchard - I have realized that, although the primary relationship name change is working fine here, the votes method is used directly in most of the methods in ActsAsVoteable. I need to re-direct those calls to the dynamic _votes_by which will call the configured relationship. Once I do that, this will be fully working. As long as people don't use this new configuration yet this merge won't cause any issues. But the configuratio nis not fully working. I'll fix it soon and send another PR.


Reply to this email directly or view it on GitHub.

@pboling
Copy link
Author

pboling commented Apr 2, 2013

Yep!

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.

None yet

2 participants