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

Allow model to set table_name after acts_as_paranoid macro #131

Conversation

AlexWheeler
Copy link
Contributor

@AlexWheeler AlexWheeler commented Sep 25, 2019

I originally opened #117 without tests, and since then have deleted my fork, so reforked and opened this PR with a spec @mvz

Thought about also asserting that ParanoidWithExplicitTableNameAfterMacro.only_deleted is working since that's how I found the error, but I think this test is clearer what exactly we're testing. Lemme know what ya think

  • paranoid_column_reference is currently a class attribute which is set
    when a class is loaded and the acts_as_paranoid method is invoked. A
    rails model can set its table_name after it declares itself as
    acts_as_paranoid. This means that whenever paranoid_column_reference is
    referenced in the gem it does not match the table_name for the model
    which can cause errors.

  • e.g. ActsAsParanoid::Core#only_deleted references
    paranoid_column_reference so in the below examples when we call this
    method on the paranoid class it tries to look up a table_name matching
    the model name. If the table doesn't exist a sql error is raised because
    the table does not exist. If the table does exist (maybe you're moving
    data to a new table and deprecating old ones) you'll end up returning
    the old records.

  • This change makes it a method so that it returns whatever the current
    value of table_name is

  # current incorrect behavior
    class User
      acts_as_paranoid
      self.table_name = "Account"
    end

    User.paranoid_column_reference
    => "user.deleted_at"
   # new correct behavior
    class User
      acts_as_paranoid
      self.table_name = "Account"
    end

    User.paranoid_column_reference
    => "account.deleted_at"

@AlexWheeler AlexWheeler force-pushed the acts_as_paranoid_model_can_set_table_name_explicitly branch from d3a0deb to 25d1c20 Compare September 25, 2019 01:56
@mvz mvz self-requested a review September 25, 2019 06:44
@AlexWheeler AlexWheeler force-pushed the acts_as_paranoid_model_can_set_table_name_explicitly branch from 25d1c20 to df6e3d3 Compare September 25, 2019 16:01
* paranoid_column_reference is currently a class attribute which is set
when a class is loaded and the acts_as_paranoid method is invoked. A
rails model can set its table_name after it declares itself as
acts_as_paranoid. This means that whenever paranoid_column_reference is
referenced in the gem it does not match the table_name for the model
which can cause errors.

* e.g. ActsAsParanoid::Core#only_deleted references
`paranoid_column_reference` so in the below examples when we call this
method on the paranoid class it tries to look up a table_name matching
the model name. If the table doesn't exist a sql error is raised because
the table does not exist. If the table does exist (maybe you're moving
data to a new table and deprecating old ones) you'll end up returning
the old records.

* This change makes it a method so that it returns whatever the current
value of table_name is

```ruby

class User
  acts_as_paranoid
  self.table_name = "Account"
end

User.paranoid_column_reference
=> "user.deleted_at"
```

```ruby

class User
  acts_as_paranoid
  self.table_name = "Account"
end

User.paranoid_column_reference
=> "account.deleted_at"
```
@mvz mvz merged commit aab5abc into ActsAsParanoid:master Oct 22, 2019
@mvz
Copy link
Contributor

mvz commented Oct 22, 2019

Thanks, @AlexWheeler!

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

Successfully merging this pull request may close these issues.

None yet

2 participants