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

Fix small TtlPIcker2 bug #17376

Merged
merged 4 commits into from
Sep 30, 2022
Merged

Conversation

Monkeychip
Copy link
Contributor

@Monkeychip Monkeychip commented Sep 30, 2022

This fixes two specific situations:

  1. a model using openApi with the following model attr.
  2. a model NOT using openApi with the following model attr.
  @attr({
    label: 'someLabel',
    editType: 'ttl',
    hideToggle: true,
  })
  someParamName

The problem is that if a user does not set the defaultValue in situation 1 the openApi sends a value (if it has one defined). And in the case for PKI roles that was an integer. This may change after a discussion with the backend because that's not the value the API docs declare.

In situation 2 this fix makes is so if you don't pass any defaultValue then the TtlPicker2 doesn't show you passing a value. It shows a blank too.

*Note, the previous PKI engine has been sending the integer 30 for the param notBeforeDuration even though the docs don't state it can handle an integer.

This was an issue in two models:
-pki-role-engine.js
-mfa-methods.js

For both of those models, because the docs state a default, I amended their models to show them setting this defaultValue so the user knows that no matter what, if they press "Create" a defaultValue is being set.

@Monkeychip Monkeychip marked this pull request as ready for review September 30, 2022 20:01
@Monkeychip Monkeychip added ui bug Used to indicate a potential bug labels Sep 30, 2022
@Monkeychip Monkeychip added this to the 1.13.0-rc1 milestone Sep 30, 2022
@Monkeychip Monkeychip removed the bug Used to indicate a potential bug label Sep 30, 2022
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.

thanks for tackling! just some small questions/fixes for comments

@@ -123,6 +123,7 @@ export default class MfaMethod extends Model {
editType: 'ttl',
helperTextEnabled: 'How long each generated TOTP is valid.',
hideToggle: true,
defaultValue: 30, // API accepts both an integer as seconds and sting with unit e.g 30 || '30s'
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for adding this comment! - small typo here! sting

@@ -13,7 +13,7 @@
* @param helperTextDisabled="Allow tokens to be used indefinitely" {String} - This helper text is shown under the label when the toggle is switched off
* @param helperTextEnabled="Disable the use of the token after" {String} - This helper text is shown under the label when the toggle is switched on
* @param description="Longer description about this value, what it does, and why it is useful. Shows up in tooltip next to helpertext"
* @param time=30 {Number} - The time (in the default units) which will be adjustable by the user of the form
* @param time='' {Number} - The time (in the default units) which will be adjustable by the user of the form
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for remembering to update the docs! I was going to suggest updating the format to @param {type} paramName

buuut...I think we tend to do that when glimmerizing so probably fine to skip for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, I actually already attempted to glimmerize at my look into the problem. Had the exact same thought you did, but thought just in case any backporting needs to be done, I'll keep these two separate.

@@ -50,7 +50,7 @@ export default TtlForm.extend({
helperTextDisabled: 'Allow tokens to be used indefinitely',
helperTextEnabled: 'Disable the use of the token after',
description: '',
time: 30,
time: '', // if defaultValue is NOT set, then do not display a defaultValue. This causes the param on the model to be different then what the component shows on init.
Copy link
Contributor

Choose a reason for hiding this comment

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

this second part of this is a little unclear to me

if defaultValue is NOT set, do not display defaultValue

does this mean if defaultValue is NOT set, time input will be left blank?

Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: small typo here, should be: "than what the component shows on init."

but on second thought, not sure if we need to have that second sentence as it might cause confusion later. especially now that the problem is being solved, plus we have the git history to return to if we are ever wondering why we removed 30

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me, I'll take out the second sentence.

@Monkeychip Monkeychip enabled auto-merge (squash) September 30, 2022 22:12
@Monkeychip Monkeychip merged commit 8694944 into main Sep 30, 2022
@Monkeychip Monkeychip deleted the ui/VAULT-8748/ttlpicker2-remove-default-time branch October 1, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants