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 5.2 Fixing has one callbacks #3

Merged
merged 4 commits into from
Oct 10, 2019

Conversation

NikoRoberts
Copy link

No description provided.

Copy link

@danieltheodosius danieltheodosius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the approach seems okay considering the behaviour introduced by active record 5.2, I just have minor comments

case options[:dependent]
when :destroy
target.destroy
if target.respond_to?(:soft_destroyed?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does permanent_records define soft_destroyed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I believe it is our concern that defines this method. But as permanent_records is basically unmaintained I thought it best to cover more possibilities in case our implementation becomes the actual way we do this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is good to cover it in our case, but in the context of this gem, it is not provided, so thanks for removing it

when :destroy
target.destroyed_by_association = reflection
target.destroy
if target.respond_to?(:soft_destroyed?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

target.destroy
if target.respond_to?(:soft_destroyed?)
raise ActiveRecord::Rollback unless target.soft_destroyed?
elsif target.respond_to?(:destroyed?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, seems like we only need to target deleted? since permanent_records handle the deleted as

def deleted?
  if is_permanent?
    !!deleted_at # rubocop:disable Style/DoubleNegation
  else
    destroyed?
  end
end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the deleted? method gets added to all AR models right?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume so

@NikoRoberts
Copy link
Author

@danieltheodosius I've updated to remove the extraneous method checks

@NikoRoberts NikoRoberts merged commit 129b4c4 into master Oct 10, 2019
@NikoRoberts NikoRoberts deleted the rails52-fixing-has-one-callbacks branch October 10, 2019 00:18
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.

2 participants