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

Destroy default content view on cascade when deleting environment #1743

Merged

Conversation

iNecas
Copy link
Member

@iNecas iNecas commented Mar 13, 2013

Also save the default content_view explicitly. Here is what happened without
this changes:

  1. Create environment (default content view and content_view_version was
    crated)
  2. Delete the environment (content_view stayed there because
    :dependent=> :destroy was missing)
  3. When creating an environment with the
    same name, the content_view was not created, because another content view with
    the same name already existed. However, because the content_view was being
    saved as part of saving content_view_version, the exception was not thrown
    (and the content_view_version was saved successfully without association to
    any content_view)
  4. When promoting some time after this incident (that
    happened unnoticed), "nil pointer exception" occurred while promoting, because
    the default content view was not there.

@bbuckingham
Copy link
Member

ACK

2 similar comments
@pitr-ch
Copy link
Member

pitr-ch commented Mar 13, 2013

ACK

@lzap
Copy link
Contributor

lzap commented Mar 13, 2013

ACK

@dmitri-d
Copy link
Contributor

NACK. You don't need to call save! (or update_attributes!, or create!) to validate the object. Call #valid? instead. Also, could you add a test for this?

@iNecas
Copy link
Member Author

iNecas commented Mar 13, 2013

NACKNACK :) valid? returns boolean, I want the exception, since that's how other validations work as well in current status of orchestration.

@iNecas
Copy link
Member Author

iNecas commented Mar 13, 2013

I can add some test thou

@dmitri-d
Copy link
Contributor

NACKNACKNACK: raise RecordInvalid.new(content_view_instance) unless content_view_instance.valid?

@iNecas
Copy link
Member Author

iNecas commented Mar 14, 2013

@witlessbird Could you please be more specific about the advantages of this solution (and in the future, stating that reasons directly in the first NACK comment, it saves some time:)? Implicit saving of associated records just proved to be unreliable: calling content_view_version.save! saved without problems even when the content_view was invalid. I can imagine situation where the content_view could be valid when checking, but something might cause the actual save wont be successful and we wouldn't even notice.

@dmitri-d
Copy link
Contributor

@iNecas The triple NACK was made in jest. On the serious side, the behaviour you are describing is very strange: according to the docs and the code, validations should be run on the children when save! is called on the parent. Your approach is fine as a work-around, but I think we should find the root cause of the issue. Did you look into why this is happening?

@iNecas
Copy link
Member Author

iNecas commented Mar 14, 2013

Tests added

@dmitri-d
Copy link
Contributor

ACK.

@iNecas
Copy link
Member Author

iNecas commented Mar 14, 2013

@witlessbird found the real cause https://github.com/iNecas/rails-fail/pull/1, I will retest it with this approach

Also validate the default content_view explicitly. Here is what happened
without this changes:

1. Create environment (default content view and content_view_version
was crated)
2. Delete the environment (`content_view` stayed there because
`:dependent=> :destroy` was missing)
3. When creating an environment with the same name, the content_view
was not created, because another content view with the same name
already existed. However, because the content_view was being saved as
part of saving content_view_version, the exception was not thrown (and
the content_view_version was saved successfully without association to
any content_view)
4. When promoting some time after this incident (that happened
unnoticed), "nil pointer exception" occurred while promoting, because
the default content view was not there.
@iNecas
Copy link
Member Author

iNecas commented Mar 14, 2013

I've added :validate => true to the default_content_view association so that the validation happens when calling content_view_version.save!. We need to check other associations to make sure similar things doesn't happen somewhere else. This is, however, out of the scope of this PR. We need also handle the validation errors from associations to give a user more descriptive message. Now it would say 'Environments are invalid'.

iNecas added a commit that referenced this pull request Mar 15, 2013
…estroy

Destroy default content view on cascade when deleting environment
@iNecas iNecas merged commit 0e5baae into Katello:master Mar 15, 2013
iNecas added a commit that referenced this pull request Apr 27, 2013
…estroy

Destroy default content view on cascade when deleting environment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants