Change reconfigure setup to include values configured with originally #4226
Conversation
| _.forEach(allErrorMessages, (function(errorMessage) { | ||
| add_flash(errorMessage, 'error'); | ||
| })); | ||
| console.log(err); |
There was a problem hiding this comment.
I'd use console.error instead, to match what miqService.handleFailure does
There was a problem hiding this comment.
This was copied from https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/assets/javascripts/controllers/dialog_user/dialog_user_controller.js#L68. I can change it in both places so that it matches.
There was a problem hiding this comment.
Well in that case, maybe you can move it to a shared service?
|
Other than the one silly nit, looks great. |
| var vm = this; | ||
|
|
||
| vm.$onInit = function() { | ||
| var apiCall = new Promise(function(resolve) { |
There was a problem hiding this comment.
isn't this whole thing just
API.get(url)
.then(init)
.then(miqService.refreshSelectpicker);
?
There was a problem hiding this comment.
Yeah I thought so too, but when I tried to use .then(miqService.refreshSelectpicker) in #4063, it did not work and had to manually resolve the promise first.
|
Not seeing any angular problems either :) 👍 |
eclarizio
force-pushed
the
BZ1591484-Reconfigure
branch
from
88ba5f9 to
f895305
Compare
|
@eclarizio can you see if you can fix some of the CC issues? Thx, Dan |
|
@eclarizio If you're going to push another commit, please update BZ link to the 'master' BZ (https://bugzilla.redhat.com/show_bug.cgi?id=1580987), so the correct BZ will be commented. |
eclarizio
force-pushed
the
BZ1591484-Reconfigure
branch
from
87796af to
4b7bfdb
Compare
|
Checked commits eclarizio/manageiq-ui-classic@f11151d~...4b7bfdb with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
Just confirming that this PR is complete, and that we're just waiting on PR17647 to be merge first. |
|
Backend PR is merged. |
|
@mzazrivec with the PR Greg mentioned merged, and the approvals on this PR, can you merge this asap. |
I'll merge this PR once I also review it. Not ASAP. |
|
Thanks @mzazrivec that's perfect. 👍 |
|
Thanks @mzazrivec !! Appreciate your help. |
|
@eclarizio I see a conflict backporting to Gaprindashvili for compress id 156 <<<<<<< HEAD
157 s = Service.find_by_id(from_cid(params[:id]))
.....
172 =======
173 service = Service.find_by(:id => params[:id])
.....
180 >>>>>>> ffa5c39... Merge pull request #4226 from eclarizio/BZ1591484-ReconfigureCan you clarify there will be no spec change needed for backport? I've seen compress id conflicts requiring spec change in the past, so double-checking... |
|
@simaishi The spec for the method that conflict is in was created with this PR. I don't know exactly how lookup with the compressed ids works, so we may need to leave the |
|
@eclarizio In that case... please create a separate PR for G-branch. Thanks! |
|
Backported to Gaprindashvili via #4236 |
|
Looks like ManageIQ/manageiq#17647 was merged 10 days ago, removing pending core ;). @miq-bot remove_label pending core |
This PR is the UI counterpart to the backend PR found here. I decided to write a new javascript controller and haml file corresponding to the reconfigure action instead of reusing
dialogUserControllerbecause there were enough differences that I felt it would be much cleaner and easier to understand this way.Essentially, when clicking on the reconfigure button, it now will make an API call to get the reconfigure dialog, which will return the dialog with the correct "default" values (they are not actually the default values, they are the values which the service was originally configured with).
https://bugzilla.redhat.com/show_bug.cgi?id=1591484
@miq-bot add_label gaprindashvili/yes, bug, blocker
/cc @gmcculloug @tinaafitz
@d-m-u Can you test this in conjunction with the main repo PR please?
@syncrou Can you review this as well please?