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

Clear out all dialog resources before adding/validating. #1306

Merged
merged 1 commit into from
May 8, 2017

Conversation

h-kataria
Copy link
Contributor

Clear out all dialog resources to make sure there is no invalid resources left from previous invalid transaction.

https://bugzilla.redhat.com/show_bug.cgi?id=1448274

@lgalis @eclarizio please test/review.

Clear out all dialog resources to make sure there is no invalid resources left from previous invalid transaction.

https://bugzilla.redhat.com/show_bug.cgi?id=1448274
@h-kataria h-kataria force-pushed the dialog_editor_add_element_fix branch from fde8643 to b66aa8d Compare May 8, 2017 14:01
@miq-bot
Copy link
Member

miq-bot commented May 8, 2017

Checked commit h-kataria@b66aa8d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@lgalis
Copy link
Contributor

lgalis commented May 8, 2017

Looks good - verified in the UI

@h-kataria
Copy link
Contributor Author

@chessbyte can you please merge

Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

The only thing I'm concerned about here is the test. Doing a lot of instance_variable_sets feels a little dirty and that means maybe something needs to be broken out. It is also really long and has a lot of steps in a single it block. It feels more like an acceptance test than a unit test, and I think because of that, again points to some refactoring that could be done. I don't think it's 100% necessary to do it in this PR, but I think it's a good thing to keep in mind going forward.

@chessbyte chessbyte self-assigned this May 8, 2017
@chessbyte chessbyte merged commit 91a9c32 into ManageIQ:master May 8, 2017
@chessbyte chessbyte added this to the Sprint 60 Ending May 8, 2017 milestone May 8, 2017
simaishi pushed a commit that referenced this pull request May 8, 2017
Clear out all dialog resources before adding/validating.
(cherry picked from commit 91a9c32)

https://bugzilla.redhat.com/show_bug.cgi?id=1448943
@simaishi
Copy link
Contributor

simaishi commented May 8, 2017

Fine backport details:

$ git log -1
commit b464832dc438d88df7697a0407d34df91b7699a4
Author: Oleg Barenboim <[email protected]>
Date:   Mon May 8 13:50:57 2017 -0400

    Merge pull request #1306 from h-kataria/dialog_editor_add_element_fix
    
    Clear out all dialog resources before adding/validating.
    (cherry picked from commit 91a9c32ecab7d4fc7faac12391b7ee9ebedaedbb)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1448943

@h-kataria h-kataria deleted the dialog_editor_add_element_fix branch May 10, 2017 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants