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

Prefer :dialog_id to :new_dialog_name in config_info #14958

Merged
merged 1 commit into from
May 2, 2017

Conversation

bzwei
Copy link
Contributor

@bzwei bzwei commented May 1, 2017

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

When the request config_info contains both new_dialog_name and dialog_id, the update process will fail with error dialog already exists.

The cause is we will try to create a new service dialog even if the dialog_id is already given.

The fix is to use the given dialog_id and ignore the dialog name.

This problem was discovered while using rails console to test the update. OPS UI does not have the issue.

The fix is more preventive if the config_info comes from a REST API happens to contains both attributes.

@bzwei
Copy link
Contributor Author

bzwei commented May 1, 2017

@miq-bot add_label bug, services, providers/ansible_tower, fine/yes
@miq-bot assign @gmcculloug
@miq-bot review @syncrou

@miq-bot
Copy link
Member

miq-bot commented May 1, 2017

@bzwei unrecognized command 'review', ignoring...

Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

@miq-bot
Copy link
Member

miq-bot commented May 1, 2017

Checked commit bzwei@ee74ab4 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@gmcculloug gmcculloug requested a review from syncrou May 1, 2017 21:49
@@ -225,19 +225,18 @@
expect(service_template.resource_actions.first.dialog.id).to eq new_dialog_record.id
end

it 'uses the existing dialog if :service_dialog_id is passed in' do
it 'uses the existing dialog if :dialog_id is passed in' do
Copy link
Contributor

Choose a reason for hiding this comment

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

@bzwei - It would also be good to cover the case where :new_dialog_name & :dialog_id were passed in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the case both options are present.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh - I see it now. catalog_item_options_three has it there. Then cool - Everything looks good.

Copy link
Contributor

@syncrou syncrou left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@gmcculloug gmcculloug merged commit ec26e2b into ManageIQ:master May 2, 2017
@gmcculloug gmcculloug added this to the Sprint 60 Ending May 8, 2017 milestone May 2, 2017
simaishi pushed a commit that referenced this pull request May 2, 2017
Prefer :dialog_id to :new_dialog_name in config_info
(cherry picked from commit ec26e2b)

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

simaishi commented May 2, 2017

Fine backport details:

$ git log -1
commit 9a71b8e98e842ae7096f2df6b12a5478fc6b6caf
Author: Greg McCullough <[email protected]>
Date:   Tue May 2 13:27:09 2017 -0400

    Merge pull request #14958 from bzwei/dialog_id_and_name
    
    Prefer :dialog_id to :new_dialog_name in config_info
    (cherry picked from commit ec26e2b9e064ecb8cfa3ce52fa8718290ce1748c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1447427

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