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
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/17376.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
ui: Remove default value of 30 to TtlPicker2 if no value is passed in.
```
1 change: 1 addition & 0 deletions ui/app/models/mfa-method.js
Original file line number Diff line number Diff line change
Expand Up @@ -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

})
period;
@attr('number', {
Expand Down
1 change: 1 addition & 0 deletions ui/app/models/pki/pki-role-engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export default class PkiRoleEngineModel extends Model {
'Also called the not_before_duration property. Allows certificates to be valid for a certain time period before now. This is useful to correct clock misalignment on various systems when setting up your CA.',
editType: 'ttl',
hideToggle: true,
defaultValue: '30s', // ARG TODO follow up with Core re: question openAPI returning an integer.
})
notBeforeDuration;

Expand Down
5 changes: 3 additions & 2 deletions ui/lib/core/addon/components/ttl-picker2.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.

* @param unit="s" {String} - This is the unit key which will show by default on the form. Can be one of `s` (seconds), `m` (minutes), `h` (hours), `d` (days)
* @param recalculationTimeout=5000 {Number} - This is the time, in milliseconds, that `recalculateSeconds` will be be true after time is updated
* @param initialValue=null {String} - This is the value set initially (particularly from a string like '30h')
Expand Down Expand Up @@ -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.

unit: 's',
initialValue: null,
changeOnInit: false,
Expand All @@ -62,6 +62,7 @@ export default TtlForm.extend({
const enable = this.initialEnabled;
const changeOnInit = this.changeOnInit;
// if initial value is unset use params passed in as defaults
// and if no defaultValue is passed in display no time
if (!value && value !== 0) {
return;
}
Expand Down