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

Rails 4 support #43

Merged
merged 7 commits into from Oct 1, 2014

Conversation

Projects
None yet
@NARKOZ
Copy link
Contributor

NARKOZ commented Apr 3, 2013

No description provided.

@kn

This comment has been minimized.

Copy link

kn commented Apr 15, 2013

Hi @NARKOZ

Thank for the PR. Mostly looks good.

Two questions:

  • Did all specs passed in both Rails 3 & 4? and ruby 1.8.7 and 1.9.3?
  • Can we use attr_accessible if Rails is version 3?
@NARKOZ

This comment has been minimized.

Copy link
Contributor

NARKOZ commented Apr 15, 2013

Did all specs passed in both Rails 3 & 4? and ruby 1.8.7 and 1.9.3?

All specs passed except one. See https://travis-ci.org/twitter/activerecord-reputation-system/jobs/6014002#L232
I don't know why though, the output and expected string looks the same.

Can we use attr_accessible if Rails is version 3?

Yes, I'll update it tomorrow.

@NARKOZ

This comment has been minimized.

Copy link
Contributor

NARKOZ commented Apr 17, 2013

Got the idea why build fails: Arel places extra space before ORDER BY

@NARKOZ

This comment has been minimized.

Copy link
Contributor

NARKOZ commented Apr 29, 2013

I've revisited this PR.
The only remaining deprecation warnings are in finder_methods https://github.com/twitter/activerecord-reputation-system/pull/43/files#diff-6, because of find_scope.

@lunks

This comment has been minimized.

Copy link

lunks commented May 22, 2013

You should probably squash these commits, @NARKOZ.

@NARKOZ

This comment has been minimized.

Copy link
Contributor

NARKOZ commented May 24, 2013

As soon as @kn comments I will squash, but thank you so much.

The only remaining deprecation warnings are in finder_methods https://github.com/twitter/activerecord-reputation-system/pull/43/files#diff-6, because of find_scope.

I kept it for backwards compatibility, but I can remove that.

@bjackson

This comment has been minimized.

Copy link

bjackson commented Jul 3, 2013

I'm getting this warning in this branch. WARN -- : WARNING: Can't mass-assign protected attributes for ReputationSystem::Reputation: reputation_name, target_id, target_type, aggregated_by

It might be related to the error I'm getting here:

screen shot 2013-07-02 at 11 40 52 pm

Am I doing something wrong, or is this a bug?

@NARKOZ

This comment has been minimized.

Copy link
Contributor

NARKOZ commented Jul 3, 2013

@bjackson be sure that gem entry in your Gemfile looks like this:

gem 'activerecord-reputation-system', github: 'NARKOZ/activerecord-reputation-system', branch: 'rails4'
@bjackson

This comment has been minimized.

Copy link

bjackson commented Jul 3, 2013

It does. I don't get this error using the master branch.

@antonpaisov

This comment has been minimized.

Copy link

antonpaisov commented Dec 3, 2013

@NARKOZ tnx! I hope it will be merged at last! 👍

@caniszczyk

This comment has been minimized.

Copy link
Contributor

caniszczyk commented Jan 4, 2014

@kn are you OK with this change?

@@ -27,7 +27,7 @@ def find_with_reputation(*args)
options[:select] = build_select_statement(table_name, reputation_name, options[:select])
options[:joins] = build_join_statement(table_name, name, srn, options[:joins])
options[:conditions] = build_condition_statement(reputation_name, options[:conditions])
find(find_scope, options)
joins(options[:joins]).select(options[:select]).where(options[:conditions]).send(find_scope)

This comment has been minimized.

@kn

kn Jan 6, 2014

Perhaps, we don't need to create options hash in this case?

This comment has been minimized.

@NARKOZ

NARKOZ Jan 6, 2014

Contributor

Not sure about that.

@@ -35,7 +35,7 @@ def count_with_reputation(*args)
options[:joins] = build_join_statement(table_name, name, srn, options[:joins])
options[:conditions] = build_condition_statement(reputation_name, options[:conditions])
options[:conditions][0].gsub!(reputation_name.to_s, "COALESCE(rs_reputations.value, 0)")
count(find_scope, options)
joins(options[:joins]).select(options[:select]).where(options[:conditions]).send(find_scope).count

This comment has been minimized.

@kn
@@ -43,7 +43,7 @@ def find_with_normalized_reputation(*args)
options[:select] = build_select_statement(table_name, reputation_name, options[:select], srn, true)
options[:joins] = build_join_statement(table_name, name, srn, options[:joins])
options[:conditions] = build_condition_statement(reputation_name, options[:conditions], srn, true)
find(find_scope, options)
joins(options[:joins]).select(options[:select]).where(options[:conditions]).send(find_scope)

This comment has been minimized.

@kn
@@ -177,6 +177,7 @@
"FROM \"questions\" JOIN users ON questions.author_id = users.id "\
"LEFT JOIN rs_reputations ON questions.id = rs_reputations.target_id AND rs_reputations.target_type = 'Question' AND rs_reputations.reputation_name = 'total_votes' AND rs_reputations.active = 't' "\
"WHERE (COALESCE(rs_reputations.value, 0) > 0.6) "\
" " if ActiveRecord::VERSION::STRING >= '4' \

This comment has been minimized.

@kn

kn Jan 6, 2014

out of curiosity, why do you need to do this?

This comment has been minimized.

@NARKOZ

NARKOZ Jan 6, 2014

Contributor

Build was failing cause extra space. See rails/arel#177

This comment has been minimized.

@kn

kn Jan 8, 2014

Interesting. Do you mind adding a comment for it?

This comment has been minimized.

@NARKOZ

NARKOZ Mar 29, 2014

Contributor

I can't add a comment exactly for that line. It's a concatenated string.
Btw, I already referenced the issue in a commit message (985bfec)

This comment has been minimized.

@kn

kn Mar 30, 2014

Great. Thanks!

@kn

This comment has been minimized.

Copy link

kn commented Jan 6, 2014

Made a few minor comments. o/w lgtm!

@rockaBe

This comment has been minimized.

Copy link

rockaBe commented Mar 28, 2014

Any news on this PR?
When will it be merged if it will?
Thanks in advance

@antonpaisov

This comment has been minimized.

Copy link

antonpaisov commented Mar 28, 2014

👍 cmon guys, merge it will ya?

@camertron

This comment has been minimized.

Copy link

camertron commented Mar 28, 2014

Sorry everyone! @kn no longer works for Twitter, so that's probably why he's not responding. We're currently reviewing this PR and will merge or comment ASAP.

@bigloser

This comment has been minimized.

Copy link
Contributor

bigloser commented Mar 28, 2014

LGTM

@kn

This comment has been minimized.

Copy link

kn commented Mar 29, 2014

I'm still watching :)

@kn

This comment has been minimized.

Copy link

kn commented Mar 30, 2014

:shipit:

@rockaBe

This comment has been minimized.

Copy link

rockaBe commented Mar 31, 2014

For me, when using the rails4-branch under ruby-1.9.3, i get the following error:

1) ReputationSystem::Reputation Callback #set_target_type_for_sti should assign target's ancestors class name where reputation is declared if STI
   Failure/Error: rep.target_type.should == Person.name
     expected: "Person"
     got: "Programmer" (using ==)
     # ./spec/reputation_system/models/reputation_spec.rb:72:in `block (4 levels) in <top (required)>'

Finished in 0.31345 seconds
139 examples, 1 failure

Is this known, does it only happen for me?

@rockaBe

This comment has been minimized.

Copy link

rockaBe commented Apr 8, 2014

Any news?

@antonpaisov

This comment has been minimized.

Copy link

antonpaisov commented Apr 15, 2014

is there anybody out there?

@sungwoncho

This comment has been minimized.

Copy link

sungwoncho commented Oct 1, 2014

What's the news on this pull request?

@antonpaisov

This comment has been minimized.

Copy link

antonpaisov commented Oct 1, 2014

I thought if twitter "supports" for ex a gem, there wouldn't be 1,5 year delays for updating gem for the current Rails version..... ⚓️

@camertron

This comment has been minimized.

Copy link

camertron commented Oct 1, 2014

Can someone at twitter release this update? How 'bout you, @jakl? Maybe @jugyo?

@piratebroadcast

This comment has been minimized.

Copy link

piratebroadcast commented Oct 1, 2014

Just so you all know, there is a Rails 4 branch for this someone made-
https://github.com/caiosba/activerecord-reputation-system/tree/rails4

On Wed, Oct 1, 2014 at 3:26 AM, Cameron Dutro notifications@github.com
wrote:

Can someone at twitter release this update? How 'bout you, @jakl
https://github.com/jakl? Maybe @jugyo https://github.com/jugyo?


Reply to this email directly or view it on GitHub
#43 (comment)
.

Jesse Waites
LinkedIn http://www.linkedin.com/profile/view?id=69029327

@piratebroadcast

This comment has been minimized.

Copy link

piratebroadcast commented Oct 1, 2014

We can also all annoy the Twitter Engineering team on Twitter - I just
asked them to merge. Please do the same. Here is the link to their account

On Wed, Oct 1, 2014 at 9:00 AM, Jesse Waites jesse.waites@gmail.com wrote:

Just so you all know, there is a Rails 4 branch for this someone made-
https://github.com/caiosba/activerecord-reputation-system/tree/rails4

On Wed, Oct 1, 2014 at 3:26 AM, Cameron Dutro notifications@github.com
wrote:

Can someone at twitter release this update? How 'bout you, @jakl
https://github.com/jakl? Maybe @jugyo https://github.com/jugyo?


Reply to this email directly or view it on GitHub
#43 (comment)
.

Jesse Waites
LinkedIn http://www.linkedin.com/profile/view?id=69029327

Jesse Waites
LinkedIn http://www.linkedin.com/profile/view?id=69029327

@antonpaisov

This comment has been minimized.

Copy link

antonpaisov commented Oct 1, 2014

@piratebroadcast I totally forgot about writing them on twitter) already done
:shipit:

caniszczyk added a commit that referenced this pull request Oct 1, 2014

@caniszczyk caniszczyk merged commit 7cc1366 into twitter:master Oct 1, 2014

@caniszczyk

This comment has been minimized.

Copy link
Contributor

caniszczyk commented Oct 1, 2014

@camertron I merged this change in, thanks everyone.

If someone wants to become a maintainer of this project outside of Twitter, let me know.

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