-
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
Change service dialog api call to match CUI #1438
Change service dialog api call to match CUI #1438
Conversation
@eclarizio can you 👀 please? |
25aeac6
to
4b05bc5
Compare
4b05bc5
to
9a2df89
Compare
@@ -113,7 +113,7 @@ function Controller ($stateParams, CollectionsApi, EventNotifications, ShoppingC | |||
|
|||
init() | |||
function setDialogUrl (serviceTemplateCatalogId) { | |||
vm.dialogUrl = `service_catalogs/${serviceTemplateCatalogId}/service_templates` | |||
vm.dialogUrl = `/api/service_dialogs/` |
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.
@d-m-u
Can we use the href_slug here? it encapsulates the internal names so this doesn't have to be aware if its called service_catalogs or service_dialogs
@miq-bot add_label blocker |
@d-m-u if this can be backported, can you add the gaprindashvili/yes label. |
@eclarizio ya think should this be backported? |
@d-m-u has this been tested with a drop down in a service dialog by chance? |
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.
The spec is broken since you didn't change it to expect the new URL, but otherwise I think it looks fine.
Also, yes, this should be backported since the BZ is requesting a fix for 5.9
@@ -136,7 +136,7 @@ function Controller ($stateParams, CollectionsApi, EventNotifications, ShoppingC | |||
targetId: vm.serviceTemplate.id, | |||
targetType: 'service_template' | |||
} | |||
const url = `${vm.dialogUrl}/${vm.serviceTemplate.id}` | |||
const url = `${vm.dialogUrl}/${vm.dialogId}` |
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.
Ah, this won't work. You'll either have to use the id from one of the previous lines (idList.dialogId
) or use vm.parsedDialogs.id
.
9a2df89
to
70f1c3d
Compare
@miq-bot add_label gaprindashvili/yes |
@@ -113,7 +113,7 @@ function Controller ($stateParams, CollectionsApi, EventNotifications, ShoppingC | |||
|
|||
init() | |||
function setDialogUrl (serviceTemplateCatalogId) { |
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.
We can remove this argument now that we're not using it. Also will need to be removed from any callers.
d19e172
to
266f1d8
Compare
@chalettu yeah, it looks fine |
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.
@d-m-u any chance yah can throw up a gif of the pr in action?
TOTALLY down with the change, just wanta seeeeeeee it... work......
266f1d8
to
6f8d6ea
Compare
f0cc7c0
to
7d34a9f
Compare
c42d731
to
8d9c6b9
Compare
@eclarizio can we address https://github.com/ManageIQ/manageiq-ui-service/pull/1438/files#diff-b46757a979791a43044bafe701ffe4dfR101 in subsequent pr? |
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
@eclarizio @chalettu yah good with this?
8d9c6b9
to
03b12f0
Compare
Checked commit d-m-u@03b12f0 with ruby 2.3.3, rubocop 0.52.1, 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.
Yeah we can address the other issue in a separate PR.
👍
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.
LGTM. Great work
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.
🌮 💃
Change service dialog api call to match CUI (cherry picked from commit c0f89fc) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1585709
Gaprindashvili backport details:
|
Fix for https://bugzilla.redhat.com/show_bug.cgi?id=1578080
The classic ui api call for service dialogs is the one we should be using so we don't end up with eeeeeeeetooooomaaaaaany automate calls.