-
Notifications
You must be signed in to change notification settings - Fork 358
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
Credential Mapping Form #8905
Credential Mapping Form #8905
Conversation
fd52d11
to
d9e1f0f
Compare
@miq-bot cross-repo-tests ManageIQ/manageiq#22685 |
From Pull Request: ManageIQ/manageiq-ui-classic#8905
5cd5286
to
628e9ba
Compare
Hey @DavidResende0 , could you please check the failing spec. |
@DavidResende0 can you rebase this? |
7579d2b
to
637658d
Compare
@miq-bot cross-repo-tests ManageIQ/manageiq#22685 |
From Pull Request: ManageIQ/manageiq-ui-classic#8905
Kicking after merge of ManageIQ/manageiq#22685 |
bfbbb07
to
a38cdf6
Compare
So for the record UX-wise it feels weird the way you select the credentials reference and do the mapping and it automatically gets set above as opposed to e.g. an explicit Set button it feels like you should have to hit something to "commit" the dropdown and then once you're done you would save the whole form. |
@DavidResende0 This happens whenever the credentials column is |
Backend API PR is merged which fixes the "password" fields showing up. |
I pushed a commit to use RBAC for looking up the workflow by |
Do we have to solve the nested credentials keys first? |
@Fryguy that was fixed by @DavidResende0 (at least the API sees the correct payload now)
|
b4a1375
to
363f677
Compare
@agrare can you test this again, pretty sure I fixed it but want to make sure |
Checked commit DavidResende0@363f677 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint app/views/workflow/map_credentials.html.haml |
/* | ||
Creates the list of credential references from the workflow payload by parsing each state and saving them to payloadCredentials. | ||
Duplicate references get overridden. | ||
*/ | ||
const jsonPayload = JSON.parse(payload).States; | ||
let payloadCredentials = {}; | ||
|
||
Object.keys(jsonPayload).forEach((key1) => { | ||
if (jsonPayload[key1].Credentials != null) { | ||
Object.keys(jsonPayload[key1].Credentials).forEach((key2) => { | ||
payloadCredentials = { | ||
[key2]: jsonPayload[key1].Credentials[key2], | ||
...payloadCredentials, | ||
}; | ||
}); | ||
} | ||
}); |
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.
@agrare Discussed this with @DavidResende0 and while I'm ok with this for this PR, it seems weird to me that the UI would have to parse the payload since that's really the backend's job. I'm wondering if we should extract any credentials into a separate column, so they can be more easily accessed here. Thoughts?
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.
One way we could do this is pre-parse the payload and store those in the credentials column, but with a nil
value. That way, this form would only show the keys and if those keys have non-nil values, then they must be mapped.
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 I agree, this was actually something I discussed with @DavidResende0 a few weeks ago but I never got to implementing, I was thinking that when we load the payload and create the record we could pre-populate the credentials record with the keys that have to be filled in by the user
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.
Ha just edged me out
@DavidResende0 yes confirmed this is fixed now thanks |
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.
Overall LGTM
@agrare On the API side I think we need to ensure that the user can only set those 4 fields. i.e. we don't want them hacking in other fields.
I will put in a PR to return a Bad Request if any credential_fields are requested which don't match the credential class' API_ATTRIBUTES |
Backported to
|
Credential Mapping Form (cherry picked from commit f91fdac)
Depends On: ManageIQ/manageiq#22685
Adds the new Credential Mapping form to the Embedded Workflows page.
Preview: