-
Notifications
You must be signed in to change notification settings - Fork 91
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
Conversation
@eclarizio if yah get a chance... can ya fix the indentation errors that travis is 😭ing about? |
6acbcf5
to
98d36aa
Compare
@chalettu could ya review this one when you get a chance? |
This pull request is not mergeable. Please rebase and repush. |
@eclarizio @chalettu any chance we can gettttttttt some conflicts resolved and traction on whether or not this pr can be merged in? 🙇♀️ 💌 |
98d36aa
to
e684219
Compare
e684219
to
9538059
Compare
Jumping in to check this out presently, will merge when it all checks out 🙇♀️ 🌊 |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eclarizio ☝️ 🤔
There was a problem hiding this comment.
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
.
There was a problem hiding this 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
9538059
to
892cd2b
Compare
Checked commit eclarizio@892cd2b with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 |
There was a problem hiding this 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.
🍍
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
Gaprindashvili backport details:
|
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!