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

BZ#1518390-Pass additional information to the API when refreshing a dialog field #1324

Merged
merged 1 commit into from
Jan 17, 2018

Conversation

eclarizio
Copy link
Member

Very similar to ManageIQ/manageiq-ui-classic#2885, except for refreshing dialogs in the service UI.

The resource action id, target id, and target type need to be passed to the API in order to establish the context for dialog fields when they are refreshed via the /service_dialogs endpoint, otherwise certain automate methods can fail.

@miq-bot add_label gaprindashvili/yes, bug

There is no current BZ that deals with this since https://bugzilla.redhat.com/show_bug.cgi?id=1518390 is talking about the classic UI, but it's the closest relative. I just wanted to put this PR out there to get ahead of a potential BZ popping up with the same issue but for the service UI.

@chalettu Please review!

@AllenBW AllenBW self-assigned this Dec 7, 2017
@AllenBW AllenBW added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 7, 2017
@AllenBW AllenBW requested a review from chalettu December 7, 2017 12:16
@AllenBW AllenBW changed the title Pass additional information to the API when refreshing a dialog field BZ#1518390-Pass additional information to the API when refreshing a dialog field Dec 7, 2017
@AllenBW
Copy link
Member

AllenBW commented Dec 11, 2017

@eclarizio if yah get a chance... can ya fix the indentation errors that travis is 😭ing about?

@AllenBW
Copy link
Member

AllenBW commented Dec 12, 2017

@chalettu could ya review this one when you get a chance?

@miq-bot
Copy link
Member

miq-bot commented Dec 12, 2017

This pull request is not mergeable. Please rebase and repush.

@AllenBW
Copy link
Member

AllenBW commented Jan 8, 2018

@eclarizio @chalettu any chance we can gettttttttt some conflicts resolved and traction on whether or not this pr can be merged in? 🙇‍♀️ 💌

@AllenBW
Copy link
Member

AllenBW commented Jan 16, 2018

Jumping in to check this out presently, will merge when it all checks out 🙇‍♀️ 🌊

AllenBW
AllenBW previously approved these changes Jan 16, 2018
Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

LG2M ❤️ 🙇‍♀️

@chalettu last call, anything on this one?


let idList = {
dialogId: vm.dialogId,
resourceActionId: $stateParams.button.resource_action.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more of a question than anything else. Are we assured this particular state param will always be there? If so, the I am fine with it , I think the other thing to consider is more of a style thing. I tend to like to put things into variables that can be reused. I don't know if we have a use for $stateParams.button more than just this statement but if we do, we should do a
const button = $stateParams.button

resourceActionId: button.resource_action.id,

Copy link
Member

Choose a reason for hiding this comment

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

@eclarizio ☝️ 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

$stateParams.button should always be there, yes, since it is a reference to the custom button that was just clicked. Likewise, resource_action on that custom button should also always be there.

I'll add vm.resourceAction since we already have a vm.button.

Copy link
Contributor

@chalettu chalettu left a comment

Choose a reason for hiding this comment

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

Take a look at my comment and let me know if something should change or if I should just go ahead and approve it.

Resource action id, target id, and target type need to be passed to the
API in order to establish the context for dialog fields when they are
refreshed via the /service_dialogs endpoint
@miq-bot
Copy link
Member

miq-bot commented Jan 16, 2018

Checked commit eclarizio@892cd2b with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

Hot darn, looks like feedback was addressed, will merge once tests pass.
🍍

@AllenBW AllenBW merged commit 3d43dbf into ManageIQ:master Jan 17, 2018
simaishi pushed a commit that referenced this pull request Feb 21, 2018
BZ#1518390-Pass additional information to the API when refreshing a dialog field
(cherry picked from commit 3d43dbf)

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

Gaprindashvili backport details:

$ git log -1
commit 734f7f04c500fef7e5f29c87406837983ee5d583
Author: Allen Wight <[email protected]>
Date:   Wed Jan 17 08:52:31 2018 -0500

    Merge pull request #1324 from eclarizio/dialog_refresh_targets
    
    BZ#1518390-Pass additional information to the API when refreshing a dialog field
    (cherry picked from commit 3d43dbf2cb92cc0751a493940b07eabd0fa648a3)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1543180

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.

5 participants