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

Rails 4.1 - Issue updating STI column #404

Closed
jmirmina opened this issue Jul 31, 2014 · 15 comments
Closed

Rails 4.1 - Issue updating STI column #404

jmirmina opened this issue Jul 31, 2014 · 15 comments
Milestone

Comments

@jmirmina
Copy link

I've run into this weird issue running Rails 4.1.4 and paper_trail 3.0.3. Ill try and simplify my code to explain what is happening.

class Page < ActiveRecord::Base
  class Draft < ActiveRecord::Base
    include PaperTrail
    has_paper_trail
  end
end

class MainPage < Page
  class Draft < Page::Draft; end
end

class SubPage < MainPage
  class Draft < MainPage::Draft; end
end

The page and page_drafts table both have the 'type' inheritance column. There are some instances where I need to change the type. Previously, I was able to do something like this:

my_page = MainPage::Draft.create
my_page.update type: 'SubPage::Draft'

The update call would then update the type and updated_at columns. When upgrading to Rails 4.1.4, the update call is only updating the updated_at field. I noticed that when removing paper_trail from the Page::Draft class, it works as expected.

The weird thing is that it seems to work as expected if my_page is an instance of the Page::Draft class where has_paper_trail is called. If my_page is an instance of a subclass of Page::Draft, thats when I notice this issue.

I apologize if this is hard to follow. I've been trying to use the template at https://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_master.rb to create a failing test, but I am having issues getting PaperTrail included.

I originally thought this was an issue with Rails 4.1 until I discovered that removing PaperTrail fixed my issue. Reverting to Rails 4.0.8 and leaving in PaperTrail also fixes the issue. Any idea what could be going on?

@alienatix
Copy link

I have the same issue.

@batter
Copy link
Collaborator

batter commented Aug 4, 2014

You shouldn't need to have the include PaperTrail line. If you aren't using bundler, you might need to add a require 'paper_trail' to the top of your template.. (after require 'active_record'). I don't understand why an upgrade to Rails would start causing issues though.

@batter
Copy link
Collaborator

batter commented Aug 4, 2014

You also shouldn't be nesting one model within another model the way you are doing it unless I'm mistaken. If you want to namespace your models, I believe you want to set them up like this:

module Page
  class Draft < ActiveRecord::Base
    has_paper_trail
  end
end

OR

class Page < ActiveRecord::Base
  has_paper_trail
end

class Draft < Page
end

module MainPage
  class Draft < Draft; end
end

etc...

In my experience with Ruby, Modules can/should be used for namespacing children (modules and/or classes), but it is unusual to have a class that is a child of another class in Ruby. After doing some reading just now, it appears as though the pattern you are using is doable, however, I'm not sure that that is proper Rails convention for setting up STI models, which may be what is causing your issues. I would try posting to the Rails issues repo or StackOverflow and establishing if that is a setup that works first, because it sounds like it may be an issue with the setup of the classes there causing this. Or are you saying that that setup works perfectly until you have PaperTrail involved?

@batter batter added Not a bug and removed Not a bug labels Aug 4, 2014
@rdubya
Copy link

rdubya commented Aug 5, 2014

As a preface, the reason we (I work with jmirmina) are manually setting the type is because ActiveRecord's becomes! method is not working correctly. It looks like they have a fix in for 4.2 that should make what we are doing here unnecessary though. This ticket can be closed as it isn't an issue for us anymore.

In case other's aren't so lucky:

The setup that jmirmina shows was working for us until we added the PaperTrail gem. I simplified it down to being a basic test case.

I believe I've tracked it down to the item_before_change callback. It appears that ActiveRecord's dup() method is now wiping the changed_attributes hash so it doesn't look like the type was changed. If others need the functionality, it might be better to file a bug with the rails core team.

unless File.exist?('Gemfile')
    File.write('Gemfile', <<-GEMFILE)
    source 'https://rubygems.org'
    gem 'rails', '~> 4.1.1'
    gem 'paper_trail'
    gem 'sqlite3'
    GEMFILE

    system 'bundle'
end

require 'bundler'
Bundler.setup(:default)

require 'active_record'
require 'paper_trail'
require 'minitest/autorun'
require 'logger'

# 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 do |t|
        t.string :type
        t.string :title
        t.string :author
    end

    create_table :versions do |t|
        t.string :item_type, :null => false
        t.integer :item_id, :null => false
        t.string :event, :null => false
        t.integer :whodunnit
        t.text :object
        t.text :object_changes
        t.datetime :created_at
    end
end

# Papertrail nested classes
class Post < ActiveRecord::Base
    has_paper_trail
end

class NewsPost < Post; end
class BlogPost < Post; end

class BugTest < Minitest::Test

    def test_changing_type_with_inner_class
        post = NewsPost.create!
        post.update(type: 'BlogPost')
        updated_post = Post.find(post.id)

        assert_instance_of BlogPost, updated_post
    end

end

@sjobe
Copy link

sjobe commented Aug 14, 2014

We're running into this exact same issue.

@JeanMertz
Copy link

Seeing similar problems using AASM and paper_trail. After including has_paper_trail, the states would no longer update as they did before.

@batter
Copy link
Collaborator

batter commented Aug 28, 2014

Do you guys see these issues with PaperTrail version 3.0.2 or just with version 3.0.3?

I'm planning to release a version 3.0.4 by Friday (and yank the 3.0.3 release from Rubygems) which will essentially be almost identical to 3.0.3 but with the Rails::Engine model loading backed out because a lot of people seem to be having issues with it. The Rails::Engine model loading piece of the gem will be re-introduced in 3.1.0, and I may even try to make it optional to load the model in that way.

@JeanMertz
Copy link

I'm having the issues with 3.0.3. Looking forward to trying 3.0.4 when you release it.

@batter
Copy link
Collaborator

batter commented Sep 3, 2014

@JeanMertz - Can you try 3.0.5?

@bwicklund
Copy link

@batter Same Issue here and I am running 3.0.5

@gdomingu
Copy link

Having the same issue with 3.0.6

@TheKidCoder
Copy link

I'm also having this issue with 3.0.1.

Really basic STI though.

class Prompt < ActiveRecord::Base
  has_paper_trail
end

class TextField < Prompt
end

class RadioButton < Prompt
end

Simply trying to do TextField.first.update_attributes type: "RadioButton" does not change the type.

@batter batter added this to the 4.0.0 milestone Feb 3, 2015
@batter
Copy link
Collaborator

batter commented Mar 1, 2015

Sorry for the delayed response here but can one of you try running your tests against PaperTrail version 4.0.0.beta2.. It seems like the issue is solved on PaperTrail 4.x. Here is a template based off @rdubya's submission from this thread that works fine for me locally:

gem 'activerecord', '~> 4.1.1'
gem 'sqlite3'
gem 'paper_trail', '4.0.0.beta2'

require 'active_record'
require 'paper_trail'
require 'minitest/autorun'
require 'logger'

# 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 do |t|
    t.string :type
    t.string :title
    t.string :author
  end

  create_table :versions do |t|
    t.string :item_type, :null => false
    t.integer :item_id, :null => false
    t.string :event, :null => false
    t.integer :whodunnit
    t.text :object
    t.text :object_changes
    t.datetime :created_at
  end
end

# Papertrail nested classes
class Post < ActiveRecord::Base
  has_paper_trail
end

class NewsPost < Post; end
class BlogPost < Post; end

class BugTest < Minitest::Test
  def test_changing_type_with_inner_class
    post = NewsPost.create!
    post.update(type: 'BlogPost')
    updated_post = Post.find(post.id)
    assert_instance_of BlogPost, updated_post
  end
end

batter added a commit that referenced this issue Mar 1, 2015
@batter
Copy link
Collaborator

batter commented Mar 2, 2015

Looks like this was actually fixed as a byproduct of the fix for #428 (contained in the master / PaperTrail 4.x branch). I am going to backport this fix to PaperTrail 3.x, and cut a 3.0.7 release containing the fix too.

batter added a commit that referenced this issue Mar 2, 2015
batter added a commit that referenced this issue Mar 2, 2015
…#404

Conflicts:
	lib/paper_trail/has_paper_trail.rb
@batter batter closed this as completed in c83c409 Mar 2, 2015
@TheKidCoder
Copy link

Just wanted to follow up and confirm 4.x has fixed this issue!

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

No branches or pull requests

9 participants