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.

Sign up for GitHub

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

Jump to bottom

Moving before_destroy callbacks because of rails/rails#3458 #2030

Merged
merged 1 commit into from
Apr 23, 2013
Merged

Moving before_destroy callbacks because of rails/rails#3458 #2030

merged 1 commit into from
Apr 23, 2013

Conversation

daviddavis
Copy link
Contributor

Moving before_destroy callbacks because of rails/rails#3458

ehelms
ehelms reviewed Apr 22, 2013
View reviewed changes
src/app/models/kt_environment.rb
@@ -21,6 +21,9 @@ class KTEnvironment < ActiveRecord::Base
include Ext::PermissionTagCleanup
acts_as_reportable

before_destroy :confirm_last_env
before_destroy :delete_default_view_version
Copy link
Member

Choose a reason for hiding this comment

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

Should these two include comments like the one below?

@ehelms
Copy link
Member

ehelms commented Apr 22, 2013

What do you think about trying to annotate comments like that with a common word/phrase to find them easily in the future with rake notes ?

@daviddavis
Copy link
Contributor Author

Done.

@ehelms
Copy link
Member

ehelms commented Apr 22, 2013

ACK

@pitr-ch
Copy link
Member

pitr-ch commented Apr 23, 2013

@daviddavis could you please add test to https://github.com/Katello/katello/blob/master/src/test/source_code_test.rb to ensure before_destroy is always before an_assoc :dependent => :destroy

@daviddavis
Copy link
Contributor Author

@pitr-ch before_destroy doesn't necessarily need to proceed :dependent => :destroy. However, I guess we could force that though since we don't currently have a case where we need a before_destroy after a dependent destroy.

This commit doesn't move all before_destroys to be before :dependent => :destroy so I'll open another PR to do that along with adding that test. I'm going to go ahead and merge this though because stuff is actually broken currently in master/1.3 until this change is merged.

daviddavis pushed a commit that referenced this pull request Apr 23, 2013
Merge pull request #2030 from daviddavis/temp_1366666322
87685ca 
Moving before_destroy callbacks because of rails/rails#3458
@daviddavis daviddavis merged commit 87685ca into Katello:master Apr 23, 2013
@daviddavis daviddavis deleted the temp_1366666322 branch April 23, 2013 10:54
@pitr-ch
Copy link
Member

pitr-ch commented Apr 23, 2013

@daviddavis Thanks.

daviddavis pushed a commit that referenced this pull request Apr 27, 2013
Merge pull request #2030 from daviddavis/temp_1366666322
d50b1dd 
Moving before_destroy callbacks because of rails/rails#3458
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.
3 participants
@daviddavis @ehelms @pitr-ch