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

Show generate creds for static-roles when you have read permissions #19190

Merged
merged 7 commits into from
Feb 16, 2023

Conversation

Monkeychip
Copy link
Contributor

@Monkeychip Monkeychip commented Feb 14, 2023

Bug fix for showing the Generate Creds/Get Credentials button for both dynamic and static roles. Before it was only showing the button for dynamic roles.

Backporting note: If this PR is approved, I'll remove the test coverage and backport everything else 3 releases. Then I'll make another PR for just the test coverage that will not get backported. I cannot backport the test coverage because I'm using a very handy (thank you Chelsea) method for stubbing the capabilities-self endpoint that was added in the 1.13. I could have added to the existing acceptance test, but we've had problems with race conditions in that test. Additionally, the actual fix was better tested in a component test.

I won't add the backport labels until after I remove the test.

creds.mov

@Monkeychip Monkeychip added ui bug Used to indicate a potential bug labels Feb 14, 2023
@Monkeychip Monkeychip modified the milestones: 1.13.0, 1.13.1 Feb 15, 2023
@@ -0,0 +1,45 @@
import { module, test } from 'qunit';
Copy link
Contributor Author

@Monkeychip Monkeychip Feb 15, 2023

Choose a reason for hiding this comment

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

I'll remove this file and put in another pr that will not get backported.

@Monkeychip Monkeychip marked this pull request as ready for review February 15, 2023 23:16
Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

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

Just a question about computed vs alias. Thanks for adding a test!

@@ -130,6 +130,7 @@ export default Model.extend({
credentialPath: lazyCapabilities(apiPath`${'backend'}/creds/${'id'}`, 'backend', 'id'),
staticCredentialPath: lazyCapabilities(apiPath`${'backend'}/static-creds/${'id'}`, 'backend', 'id'),
canGenerateCredentials: alias('credentialPath.canRead'),
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be a computed instead and true if staticCredentialPath.canRead OR
credentialPath.canRead is true? that way the HBS template could still be just @model.canGenerateCredentials?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. The type matters here. So if type is static then we have to check the static path and if type is dynamic check the dynamic path. We could still do a computed given this logic but I go back and forth on this. I think because this is a backport, and I just noticed (my bad) that the the static param already existed canGetCredentials I'll keep the changes to a minimum. The canGenerateCreds and canGetCredentials are used in another template so to use a computed I would need to amend the logic there as well. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch! I see that now, good call removing!

{{#if @model.canGenerateCredentials}}
{{#if
(or
(and (eq @model.type "static") @model.canGenerateStaticCredentials)
Copy link
Contributor

Choose a reason for hiding this comment

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

can be reverted if the above is possible!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to indicate a potential bug ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants