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

Refactor letsrate model to fix rating on inherited models #60

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@3den

3den commented Jan 9, 2014

This PR is need to fix a problem on on the rating cache that was saving the wrong cacheable_type for models. I also cleaned up the code to make it easier to understand. Please merge this for the next version.

if can_rate? user, dimension
rates(dimension).create! do |r|
r.stars = stars

This comment has been minimized.

@3den

3den Jan 9, 2014

this code was moved to a private method

@3den

3den Jan 9, 2014

this code was moved to a private method

@@ -66,7 +46,7 @@ def average(dimension=nil)
end
def can_rate?(user, dimension=nil)
user.ratings_given.where(dimension: dimension, rateable_id: id, rateable_type: self.class.name).size.zero?
rates(dimension).where(rater_id: user.id).size.zero?

This comment has been minimized.

@3den

3den Jan 9, 2014

This is much simpler than the previous code and dont violate the "Law of Demeter"

@3den

3den Jan 9, 2014

This is much simpler than the previous code and dont violate the "Law of Demeter"

@3den

This comment has been minimized.

Show comment
Hide comment
@3den

3den Jan 9, 2014

@muratguzel please merge this PR so I can use the oficial version. Thanks for publishing this gem I really liked it.

3den commented Jan 9, 2014

@muratguzel please merge this PR so I can use the oficial version. Thanks for publishing this gem I really liked it.

@3den

This comment has been minimized.

Show comment
Hide comment
@3den

3den Jan 10, 2014

@muratguzel btw why do you use travis if you have no tests?

3den commented Jan 10, 2014

@muratguzel btw why do you use travis if you have no tests?

@muratguzel

This comment has been minimized.

Show comment
Hide comment
@muratguzel

muratguzel Jan 21, 2014

Owner

I will add tests :) But yes you are right. And also I will check and merge you commit. Thanks

Owner

muratguzel commented Jan 21, 2014

I will add tests :) But yes you are right. And also I will check and merge you commit. Thanks

@3den

This comment has been minimized.

Show comment
Hide comment
@3den

3den commented Jan 21, 2014

Thanks @muratguzel :)

@berkos

This comment has been minimized.

Show comment
Hide comment
@berkos

berkos Jun 10, 2014

hi all!
i was wondering,
is this still valid for merging?

berkos commented Jun 10, 2014

hi all!
i was wondering,
is this still valid for merging?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment