Rails 4 support #43

NARKOZ · 2013-04-03T11:39:07Z

No description provided.

kn · 2013-04-15T18:44:42Z

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 · 2013-04-15T19:21:08Z

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 · 2013-04-17T13:20:31Z

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

See: rails/arel#177

NARKOZ · 2013-04-29T18:15:34Z

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 · 2013-05-22T03:09:05Z

You should probably squash these commits, @NARKOZ.

NARKOZ · 2013-05-24T02:56:08Z

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 · 2013-07-03T04:42:11Z

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:

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

NARKOZ · 2013-07-03T13:47:23Z

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

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

bjackson · 2013-07-03T21:51:45Z

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

antonpaisov · 2013-12-03T23:02:53Z

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

caniszczyk · 2014-01-04T17:10:40Z

@kn are you OK with this change?

kn · 2014-01-06T06:07:46Z

lib/reputation_system/finder_methods.rb

@@ -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)


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

Not sure about that.

kn · 2014-01-06T06:17:00Z

Made a few minor comments. o/w lgtm!

rockaBe · 2014-03-28T17:53:32Z

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

antonpaisov · 2014-03-28T22:31:59Z

👍 cmon guys, merge it will ya?

camertron · 2014-03-28T23:49:35Z

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 · 2014-03-28T23:54:40Z

LGTM

kn · 2014-03-29T20:28:20Z

I'm still watching :)

kn · 2014-03-30T02:55:25Z

rockaBe · 2014-03-31T12:22:05Z

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 · 2014-04-08T08:42:31Z

Any news?

antonpaisov · 2014-04-15T21:38:35Z

is there anybody out there?

sungwoncho · 2014-10-01T06:17:44Z

What's the news on this pull request?

antonpaisov · 2014-10-01T06:22:02Z

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 · 2014-10-01T07:26:16Z

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

jessewaites · 2014-10-01T13:01:00Z

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

jessewaites · 2014-10-01T13:18:05Z

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

https://twitter.com/TwitterEng/

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 · 2014-10-01T13:23:12Z

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

Rails 4 support

caniszczyk · 2014-10-01T14:09:34Z

@camertron I merged this change in, thanks everyone.

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

fix deprecated bundler source

053c619

NARKOZ added 5 commits April 29, 2013 22:53

bring ActiveRecord 4 compatibility

b35ff8a

test ActiveRecord 4 on travis-ci

bffe456

set attr_accessible for ActiveRecord < 4

a8c1d84

bump activerecord from beta to rc

4f1d2b2

fix failing spec with ActiveRecord 4

985bfec

See: rails/arel#177

check for strong_parameters module instead of ActiveRecord version

01197ad

kn reviewed Jan 6, 2014
View reviewed changes

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

Merge pull request #43 from NARKOZ/rails4

7cc1366

Rails 4 support

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rails 4 support #43

Rails 4 support #43

NARKOZ commented Apr 3, 2013

kn commented Apr 15, 2013

NARKOZ commented Apr 15, 2013

NARKOZ commented Apr 17, 2013

NARKOZ commented Apr 29, 2013

lunks commented May 22, 2013

NARKOZ commented May 24, 2013

bjackson commented Jul 3, 2013

NARKOZ commented Jul 3, 2013

bjackson commented Jul 3, 2013

antonpaisov commented Dec 3, 2013

caniszczyk commented Jan 4, 2014

kn Jan 6, 2014

NARKOZ Jan 6, 2014

kn commented Jan 6, 2014

rockaBe commented Mar 28, 2014

antonpaisov commented Mar 28, 2014

camertron commented Mar 28, 2014

bigloser commented Mar 28, 2014

kn commented Mar 29, 2014

kn commented Mar 30, 2014

rockaBe commented Mar 31, 2014

rockaBe commented Apr 8, 2014

antonpaisov commented Apr 15, 2014

sungwoncho commented Oct 1, 2014

antonpaisov commented Oct 1, 2014

camertron commented Oct 1, 2014

jessewaites commented Oct 1, 2014

jessewaites commented Oct 1, 2014

antonpaisov commented Oct 1, 2014

caniszczyk commented Oct 1, 2014

Rails 4 support #43

Rails 4 support #43

Conversation

NARKOZ commented Apr 3, 2013

kn commented Apr 15, 2013

NARKOZ commented Apr 15, 2013

NARKOZ commented Apr 17, 2013

NARKOZ commented Apr 29, 2013

lunks commented May 22, 2013

NARKOZ commented May 24, 2013

bjackson commented Jul 3, 2013

NARKOZ commented Jul 3, 2013

bjackson commented Jul 3, 2013

antonpaisov commented Dec 3, 2013

caniszczyk commented Jan 4, 2014

kn Jan 6, 2014

Choose a reason for hiding this comment

NARKOZ Jan 6, 2014

Choose a reason for hiding this comment

kn commented Jan 6, 2014

rockaBe commented Mar 28, 2014

antonpaisov commented Mar 28, 2014

camertron commented Mar 28, 2014

bigloser commented Mar 28, 2014

kn commented Mar 29, 2014

kn commented Mar 30, 2014

rockaBe commented Mar 31, 2014

rockaBe commented Apr 8, 2014

antonpaisov commented Apr 15, 2014

sungwoncho commented Oct 1, 2014

antonpaisov commented Oct 1, 2014

camertron commented Oct 1, 2014

jessewaites commented Oct 1, 2014

jessewaites commented Oct 1, 2014

antonpaisov commented Oct 1, 2014

caniszczyk commented Oct 1, 2014