[uiSettings]Removes Infinity from notifications lifetimes #97384

TinaHeiligers · 2021-04-16T23:45:50Z

Resolves #30109

Summary

Follows approach 1 outlined below.
(edit) ~~Removes support for 'Infinity' and updates text rendered in Advanced Settings for the notifications lifetimes~~ Retains support for 'Infinity' but removes the sentence mentioning using 'Infinity' from the ui text in Advanced settings for:

notifications:lifetime:banner
notifications:lifetime:error
notifications:lifetime:warning
notifications:lifetime:info

~~Updates the schema for these fields to only allow number and updates the description fields, removing the line that mentions using 'Infinity'.~~

~~If the approach in this PR is approved,~~ A PR to update the docs will follow shortly.

History

Several folks have raised issues (e.g. #30109) that there's no way one can set a value of 'Infinity' in Advanced Settings for the notifications banner, error, warning and info lifetimes even though the ui text mentions that 'Infinity' is supported.
I verified the comment that the code itself supports using 'Infinity' and the schema for these fields also allows it.
I traced the problem back through code history and I think the confusion stems from the fact that the descriptions for these fields haven't changed since the settings were first introduced in Feb 2016.

Prior to this PR, setting the value of the notifications' lifetime to 'Infinity' directly in the notifications code has the following effect:

The value for that setting is empty in the ui.
The public component itself has validation: undefined. (see screenshots attached)
One can still change it in the ui, but only to a positive number.
Resetting to default clears the field again.
Notifications themselves only show for the briefest moment. (see the gif below)

Note that one can only set the value for notifications:lifetime:info to Infinity if the object is moved further up in the file 🤷‍♀️ .

There are two other approaches we can take here:

Keep support for 'Infinity' in the code and simply remove the text mentioning it is a valid value from the ui text and the docs since end users have no way of configuring it to anything other than a positive integer. (will also require an update to the docs)
Use a custom ui component that allows the end user to choose either a positive integer value or 'Infinity'. (won't require an update to the docs)

Option 1 is less effort than this PR, and we could move the text to a code comment to let developers know it is supported.

Option 2 will require more work as we'd need to redesign the component to allow for both positive numeric fields and an absolute value of 'Infinity'.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)

For maintainers

This was checked for breaking API changes and was labeled appropriately

…etimes

TinaHeiligers · 2021-04-17T15:21:37Z

@elasticmachine merge upstream

…emove-notification-lifetime-Infinity-support

elasticmachine · 2021-04-18T21:06:10Z

Pinging @elastic/kibana-core (Team:Core)

pgayvallet · 2021-04-19T06:12:42Z

src/core/server/ui_settings/settings/notifications.ts

-      schema: schema.oneOf([schema.number({ min: 0 }), schema.literal('Infinity')]),
+      schema: schema.number({ min: 0 }),


This is a breaking change for customers, as any Kibana instance currently overriding this value via the uiSettings.overrides config property will no longer start due to a validation error.

kibana/src/core/server/ui_settings/ui_settings_service.ts

Lines 114 to 122 in f1708dd

private validatesOverrides() {

for (const [key, value] of Object.entries(this.overrides)) {

const definition = this.uiSettingsDefaults.get(key);

// overrides might contain UiSettings for a disabled plugin

if (definition?.schema) {

definition.schema.validate(value, {}, `ui settings overrides [${key}]`);

}

}

}

This will also cause warnings every time the uiSettings are fetched:

kibana/src/core/server/ui_settings/ui_settings_client.ts

Lines 174 to 181 in 4584a8b

try {

this.validateKey(key, userValue);

filteredValues[key] = {

userValue: userValue as T,

};

} catch (error) {

this.log.warn(`Ignore invalid UiSettings value. ${error}.`);

}

If we really want to remove this value from these settings, our only option would be to add the associated migration in src/core/server/ui_settings/saved_objects/migrations.ts to migrate the value to a new valid one.

However (and I know we already discussed it in the weekly, but), I'm not sure we should drive a model change based only on a UI limitation.

OTOH, note that

Keep support for 'Infinity' in the code and simply remove the text mentioning it is a valid value from the ui text and the docs since end users have no way of configuring it to anything other than a positive integer. (will also require an update to the docs)

would be awkward for users already using this value, as Infinity simply does not appear in the UI even if already defined via overrides. However, I think this would be acceptable.

I would pragmatically use this solution for now though, as developing a custom component (and adding the associated uiSettings type) seems to much work for the gain here.

I would pragmatically use this solution for now though, as developing a custom component (and adding the associated uiSettings type) seems to much work for the gain here

@pgayvallet I agree, and will take the first approach.

overriding this value via the uiSettings.overrides config property

I can't find where in the docs we mention these settings, they're not in the Configure Kibana section. Do you know where these are in the docs?

Not sure, we may just not have any documentation on it tbh. Greping uiSettings.overrides in /docs has no match... @mshustov maybe you know?

they are not documented in https://www.elastic.co/guide/en/kibana/current/advanced-options.html nor in https://www.elastic.co/guide/en/kibana/current/settings.html

I'd ask Docs team whether we want to announce this API to the wider audience.

we may just not have any documentation on it tbh

Yeah, AFAIK this has always been an undocumented feature since it was added in 6.5. It was in our release notes, but I don't think it was ever added to the documentation.

For now, I'll follow up with a docs PR to remove the sentences mentioning 'Infinity' as an options for the notifications lifetime settings in Advanced Settings.

@elastic/kibana-core it would be nice to have this in for 7.13. If someone has time to have another look, that would be great 😄

…lue does

kibanamachine · 2021-04-19T17:48:49Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💛 Build #120379 was flaky 5aa86e3
💔 Build #120363 failed 91904b3ad715fc2abb83689e3a9f23d46cd9c094

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

pgayvallet

LGTM

)

kibanamachine · 2021-04-20T13:57:20Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…97644) Co-authored-by: Christiane (Tina) Heiligers <christiane.heiligers@elastic.co>

Removes support for using Infinity for ui-settings' notifications lif…

ca3967b

…etimes

TinaHeiligers added Feature:uiSettings release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.13.0 v8.0.0 labels Apr 16, 2021

TinaHeiligers added 2 commits April 18, 2021 07:19

i18n fix

ead34db

Merge branch 'master' of github.com:elastic/kibana into ui-settings/r…

5aa86e3

…emove-notification-lifetime-Infinity-support

TinaHeiligers force-pushed the ui-settings/remove-notification-lifetime-Infinity-support branch from 91904b3 to 5aa86e3 Compare April 18, 2021 14:22

TinaHeiligers marked this pull request as ready for review April 18, 2021 21:06

TinaHeiligers requested a review from a team as a code owner April 18, 2021 21:06

pgayvallet reviewed Apr 19, 2021

View reviewed changes

Retains support for Infinity and adds a dev note on what using the va…

3e614a7

…lue does

pgayvallet approved these changes Apr 20, 2021

View reviewed changes

TinaHeiligers added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 20, 2021

TinaHeiligers merged commit 948aa3a into elastic:master Apr 20, 2021

TinaHeiligers deleted the ui-settings/remove-notification-lifetime-Infinity-support branch April 20, 2021 13:52

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 20, 2021

[uiSettings]Removes Infinity from notifications lifetimes (elastic#97384

bbb6837

)

kibanamachine mentioned this pull request Apr 20, 2021

[7.x] [uiSettings]Removes Infinity from notifications lifetimes (#97384) #97644

Merged

kibanamachine added a commit that referenced this pull request Apr 20, 2021

[uiSettings]Removes Infinity from notifications lifetimes (#97384) (#…

47e60c9

…97644) Co-authored-by: Christiane (Tina) Heiligers <christiane.heiligers@elastic.co>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[uiSettings]Removes Infinity from notifications lifetimes #97384

[uiSettings]Removes Infinity from notifications lifetimes #97384

TinaHeiligers commented Apr 16, 2021 •

edited

Loading

TinaHeiligers commented Apr 17, 2021

elasticmachine commented Apr 18, 2021

pgayvallet Apr 19, 2021 •

edited

Loading

TinaHeiligers Apr 19, 2021 •

edited

Loading

pgayvallet Apr 19, 2021

mshustov Apr 19, 2021

lukeelmers Apr 19, 2021

TinaHeiligers Apr 19, 2021

TinaHeiligers Apr 19, 2021

kibanamachine commented Apr 19, 2021

pgayvallet left a comment

kibanamachine commented Apr 20, 2021

		schema: schema.oneOf([schema.number({ min: 0 }), schema.literal('Infinity')]),
		schema: schema.number({ min: 0 }),

	private validatesOverrides() {
	for (const [key, value] of Object.entries(this.overrides)) {
	const definition = this.uiSettingsDefaults.get(key);
	// overrides might contain UiSettings for a disabled plugin
	if (definition?.schema) {
	definition.schema.validate(value, {}, `ui settings overrides [${key}]`);
	}
	}
	}

	try {
	this.validateKey(key, userValue);
	filteredValues[key] = {
	userValue: userValue as T,
	};
	} catch (error) {
	this.log.warn(`Ignore invalid UiSettings value. ${error}.`);
	}

[uiSettings]Removes Infinity from notifications lifetimes #97384

[uiSettings]Removes Infinity from notifications lifetimes #97384

Conversation

TinaHeiligers commented Apr 16, 2021 • edited Loading

Summary

History

Checklist

For maintainers

TinaHeiligers commented Apr 17, 2021

elasticmachine commented Apr 18, 2021

pgayvallet Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

pgayvallet Apr 19, 2021

Choose a reason for hiding this comment

mshustov Apr 19, 2021

Choose a reason for hiding this comment

lukeelmers Apr 19, 2021

Choose a reason for hiding this comment

TinaHeiligers Apr 19, 2021

Choose a reason for hiding this comment

TinaHeiligers Apr 19, 2021

Choose a reason for hiding this comment

kibanamachine commented Apr 19, 2021

💚 Build Succeeded

Metrics [docs]

History

pgayvallet left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 20, 2021

💚 Backport successful

TinaHeiligers commented Apr 16, 2021 •

edited

Loading

pgayvallet Apr 19, 2021 •

edited

Loading

TinaHeiligers Apr 19, 2021 •

edited

Loading