Rails 4.2 support (waiting on rails/rails#19042)

[yan-hoose]

[Ed (Radar): We are waiting for this bug to be fixed in rails: rails/rails#19042]

Hi,

I'd like to see Forem updated to support Rails 4.2. I took a look at it and at first glance there does not seem to be very much needed to do. There are a ton of deprecation warnings when running specs but most of them are repetitive. However, there are a few strange issues.

BTW, currently Travis CI is running Forem tests in Rails 4.2 mode, so you can see the warnings easily yourself. In Rails 4.2 mode because in the gemspec there is currently
s.add_dependency 'rails', '~> 4.0', '<= 4.2'
instead of
s.add_dependency 'rails', '~> 4.0', '< 4.2'.

Anyway, when running tests, there is one failing test and three main sources of warnings:

• deprecated use_route in forem/spec/support/controller_hacks.rb:27
• using #deliver instead of #deliver_now or #deliver_later in forem/app/models/forem/subscription.rb:12
• #column_for_attribute change warnings from Simple Form: forem/app/views/forem/admin/forums/_form.html.erb:8. This warning will be resolved if the gem dependency is changed from '> 3.0.1' to '> 3.0'.

I'm not sure if in addition to fixing the use_route deprecation warning, RSpec gem dependency should be raised from '> 2.14.0' to '> 3.1.0' (since 3.1 is the one with official Rails 4.2 support). Somebody with more experience in RSpec might know the answer to this.

There is also a question about how to support both #deliver and #deliver_later depending on the Rails version.

About the failing test:
rspec ./spec/models/topic_spec.rb:87 # Forem::Topic helper methods #register_view_by increments the overall topic view count
It seems that this code in forem/models/forem/concerns/viewable.rb

view = views.find_or_create_by(user_id: user.id)
view.increment!('count')
increment!(:views_count)

is incrementing topic.views_count by 2 instead of 1 - first increment in views.find_or_create_by (I'm not sure why) and the second one in increment!(:views_count).

Has anybody else taken a look at 4.2 support and knows the answers/solutions to these issues already? Unfortunately I don't have too much time to spend on this.

[radar]

Hi yan-hoose,

If you'd like to see it updated then the best way to get that to happen would be to submit a PR which did the updating.

Could you please work on that? I will review it when you're done.

Thanks

On 16 Feb 2015, at 00:19, yan-hoose [email protected] wrote:

Hi,

I'd like to see Forem updated to support Rails 4.2. I took a look at it and at first glance there does not seem to be very much needed to do. There are a ton of deprecation warnings when running specs but most of them are repetitive. However, there are a few strange issues.

BTW, currently Travis CI is running Forem tests in Rails 4.2 mode, so you can see the warnings easily yourself. In Rails 4.2 mode because in the gemspec there is currently
s.add_dependency 'rails', '> 4.0', '<= 4.2'
instead of
s.add_dependency 'rails', '> 4.0', '< 4.2'.

Anyway, when running tests, there is one failing test and three main sources of warnings:

deprecated use_route in forem/spec/support/controller_hacks.rb:27
using #deliver instead of #deliver_now or #deliver_later in forem/app/models/forem/subscription.rb:12
#column_for_attribute change warnings from Simple Form: forem/app/views/forem/admin/forums/_form.html.erb:8. This warning will be resolved if the gem dependency is changed from '> 3.0.1' to '> 3.0'.
I'm not sure if in addition to fixing the use_route deprecation warning, RSpec gem dependency should be raised from '> 2.14.0' to '> 3.1.0' (since 3.1 is the one with official Rails 4.2 support). Somebody with more experience in RSpec might know the answer to this.

There is also a question about how to support both #deliver and #deliver_later depending on the Rails version.

About the failing test:
rspec ./spec/models/topic_spec.rb:87 # Forem::Topic helper methods #register_view_by increments the overall topic view count
It seems that this code in forem/models/forem/concerns/viewable.rb

view = views.find_or_create_by(user_id: user.id)
view.increment!('count')
increment!(:views_count)
is incrementing topic.views_count by 2 instead of 1 - first increment in views.find_or_create_by (I'm not sure why) and the second one in increment!(:views_count).

Has anybody else taken a look at 4.2 support and knows the answers/solutions to these issues already? Unfortunately I don't have too much time to spend on this.

—
Reply to this email directly or view it on GitHub.

[dt1973]

@yan-hoose please find the time to do this!

[yan-hoose]

I have taken this on and it's about half done.

[radar]

Yay! Thanks :)

On 18 Feb 2015, at 06:58, yan-hoose [email protected] wrote:

I have taken this on and it's about half done.

—
Reply to this email directly or view it on GitHub.

[yan-hoose]

About the one failing test. It seems that Rails 4.2 thinks that the views_count column in both topics and forums tables is a counter cache column and that is why it is incrementing it here when creating a new view row and resulting in double incrementation:

#app/models/forem/concerns/viewable.rb:19
view = views.find_or_create_by(user_id: user.id)

That did not happen in Rails 4.1 and lower. Setting the :counter_cache => false option does not work since Rails just looks if views_count column is present and if it is considers it as a counter cache.

I'm not sure if this is intended or not but probably the easiest solution would be to just rename the column to something like view_count. It is probably a good practice to not name columns like counter cache columns when in reality they aren't. What do you think?

Besides this one thing everything else is done.

[radar]

Wat? Can you reproduce this issue in a new app? Counter caches are not mentioned in the 4.2 changelog and so for this to change seems unnatural.

[yan-hoose]

I'll try in the morning.

[yan-hoose]

I can reproduce it with the following test.

# Activate the gem you are reporting the issue against.
gem 'activerecord', '4.1.9'
#gem 'activerecord', '4.2.0'
require 'active_record'
require 'minitest/autorun'
require 'logger'

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.integer :comments_count, default: 0
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

class BugTest < Minitest::Test
  def test_association_stuff
    post = Post.create!
    assert_equal 0, post.comments_count

    post.comments << Comment.create!
    assert_equal 1, post.comments.count
    assert_equal 0, post.comments_count
  end
end

With 4.1.9 it works as expected and with 4.2.0 it fails on the last assertion. I took a look at the 4.2 release notes again and indeed, the only mention of counter caches was about deprecating broken automatic detection when using has_many :through associations, so it seems like a Rails bug.

Let me know if you get the same result with this test.

[yan-hoose]

rails/rails#19042

The bug is marked with the milestone 4.2.2, so we'll have to wait until Rails 4.2.2 to have Forem support for 4.2.x.

[sungwoncho]

Good job investigating this issue. It is very strange that setting counter_cache to false does not turn off this behavior.

[yan-hoose]

Unfortunately Rails 4.2.2 does not include a fix for this issue, so we'll have to wait for 4.2.3.

[sintro]

So, 4.2.2 is here, and looks like the problem with counter_cache is fixed:
"Deprecate automatic counter caches on has_many :through. The behavior was broken and inconsistent."
Can anyone confirm the Forem is working fine now?

[yan-hoose]

Nope, the Rails bug has not been fixed yet and is marked with the milestone 4.2.4. The bug that you are talking about is not the same bug we ran into with Forem (has_many :through vs simple has_many and belongs_to association).

[radar]

Made it more obvious in the issue title and OP.

[yan-hoose]

I went ahead and created the PR so you can start reviewing it when you have time: #668

[radar]

This issue will need to be addressed before a proper gem release (as we talked about in #666)

[radar]

Rails 4.2.4 has been released. Let's get this version bumped! Please submit a PR for that.

[yan-hoose]

Nice, I'll do it in the evening today!

[sintro]

So, did anything get fixed?

[radar]

Radar: asks for pull requests
Community: asks if anything got fixed magically

And so the cycle continues...

On 26 Aug 2015, at 22:06, sintro [email protected] wrote:

So, did anything get fixed?

—
Reply to this email directly or view it on GitHub.

[radar]

@yan-hoose How goes this PR?

[yan-hoose]

It's already done a few days ago: #668
All tests pass and no deprecation warnings with the new Rails version.

[radar]

Merged, thanks!

On 28 Aug 2015, at 16:51, yan-hoose [email protected] wrote:

It's already done a few days ago: #668
All tests pass and no deprecation warnings with the new Rails version.

—
Reply to this email directly or view it on GitHub.