[toasts] migrate toastNotifications to the new platform #21772

spalger · 2018-08-08T02:52:22Z

Fixes #20698

As part of the transition of APIs necessary for migrating the Chrome to the new platform, this moves the core logic for the toastNotifications out of ui/notify and into the new platform as the core.notifications.toasts service. I chose to use the notifications namespace here as I plan for the banners service from ui/notify to eventually live at core.notifications.banners. If you disagree with this strategy and would prefer that we use something like core.toastNotifications let me know.

For the most part this service just does the same thing as the ui service did, so functionality should be exactly the same. To test the notifications I suggest using the testbed like so: https://gist.github.com/spalger/81097177c88dee142700fab25de88932

cjcenizal · 2018-08-08T14:30:55Z

About the name, this might cause confusion with the notifications service that Chris E. added (#19236). We could resolve it by renaming that one (maybe to something like outboundMessages) or renaming this one (maybe to something like inAppAlerts or inAppNotifications).

elasticmachine · 2018-08-11T02:49:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin

LGTM, I've tested this PR with the testbed snipped for success, warning and danger toasts - everything worked as expected.

azasypkin · 2018-08-13T10:40:36Z

src/core/public/core_system.test.ts

@@ -52,6 +53,18 @@ jest.mock('./fatal_errors', () => ({
  FatalErrorsService: MockFatalErrorsService,
 }));

+const mockNotificationStartContract = {};


note: I'm experimenting with mocking right now, trying to find the ideal way to mock modules :) There is also another alternative I use, it's not better, just wanted to share:

jest.mock('./notifications'); ...... import { NotificationsService } from './notifications'; const MockNotificationsService = NotificationsService as jest.Mock<NotificationsService>;

Yeah, I've played with options similar to that, and functions that use generics and require() to do the same thing, but ultimately found this to be relatively elegant. Definitely think we could improve it still.

azasypkin · 2018-08-13T10:49:54Z

src/core/public/legacy_platform/legacy_platform_service.test.ts

      });

      expect(mockFatalErrorInit).toHaveBeenCalledTimes(1);
      expect(mockFatalErrorInit).toHaveBeenCalledWith(fatalErrorsStartContract);
    });

+    it('passes toasts service to ui/notify/fatal_errors', () => {


typo: ui/notify/fatal_errors ---> ui/notify/toasts.

azasypkin · 2018-08-13T10:55:13Z

src/ui/public/notify/toasts/toasts.ts

+export let toastNotifications: ToastNotifications;
+
+export function __newPlatformInit__(toasts: ToastsStartContract) {
+  toastNotifications = new ToastNotifications(toasts);


question: in other services you throw if __newPlatformInit__ is called twice, is there any reason to not do the same here as well?

azasypkin · 2018-08-13T10:58:39Z

src/ui/public/notify/toasts/toast_notifications.ts

+
+  private onChangeCallback?: () => void;
+
+  constructor(private toasts: ToastsStartContract) {


nit: private readonly toasts: ...?

azasypkin · 2018-08-13T11:01:19Z

src/ui/public/notify/toasts/toast_notifications.d.ts

@@ -18,7 +18,7 @@
 */


question: why do need this separate .d.ts file when we have toast_notifications.ts? Can we get rid of it?

Forgot to delete when I converted the stub service to TS

azasypkin · 2018-08-13T11:29:36Z

src/core/public/notifications/toasts/global_toast_list.tsx

+      <EuiGlobalToastList
+        toasts={this.state.toasts}
+        dismissToast={this.props.dismissToast}
+        toastLifeTimeMs={6000}


question: why don't you want to make it configurable as it was before? I'd agree that it will help us to have consistent look and feel across Kibana, but I see APM uses 5000 with EuiGlobalToastList for some reason. Maybe we can move this magic number to a constant closer to the top of this file at least?

Technically, the original implementation is hard-coded too, it's just not as obvious. I chose to hard-code this value to prevent toasts from behaving differently throughout Kibana, and to prevent people from loading them with too much content (allowing longer lifetimes allows for longer content).

Hmm, didn't realize that APM had its own implementation of the global notifications list... I assume I can remove those and they can use the notifications from the new platform...

@sqren now that the toast notifications are available from the new platform you won't have to use angular to use them, and I won't have to worry that your global toast list will conflict with the new platform's global toast list, would you mind if I migrated APM to use that rather than its own instance?

@spalger APM is using the EUI component so no Angular. What are the advantages to having this in the platform instead of using EUI directly?

Oh, are you saying that there might be multiple EuiGlobalToastList on the page, because the platform also adds one?

Exactly, the new platform will now be adding the <EuiGlobalToastList/> to every page, not just the Angular ones.

Since APM isn't using our notification APIs, and there isn't anything that will cause double notifications today, I'm going to go ahead with this as-is and work on #21948 after this is merged.

azasypkin · 2018-08-13T11:39:27Z

src/core/public/notifications/toasts/global_toast_list.test.tsx

+import { GlobalToastList } from './global_toast_list';
+
+function render(props: Partial<GlobalToastList['props']> = {}) {
+  return <GlobalToastList dismissToast={jest.fn()} toasts$={Rx.empty()} {...props} />;


nit: empty() is deprecated https://github.com/ReactiveX/rxjs/blob/8fbb15e7f4b699fe5c5d10cb31bfe81595a51697/src/internal/observable/empty.ts#L57

azasypkin · 2018-08-13T11:44:59Z

src/core/public/notifications/toasts/global_toast_list.test.tsx

+  expect(shallow(render())).toMatchSnapshot();
+});
+
+it('subscribes to toasts$ on mount and unsubs on unmount', () => {


typo: unsubs ---> unsubscribes? Or just subs to ..... and unsubs .... to keep symmetry 🤣

azasypkin · 2018-08-13T11:58:41Z

src/core/public/notifications/toasts/toasts_service.test.tsx

+    }).not.toThrowError();
+  });
+
+  it('emptys the content of the targetDomElement', () => {


typo: empties?

cjcenizal · 2018-08-13T13:41:30Z

Anybody have any thoughts on the naming similarity with the other notifications service?

…latform/toasts

spalger · 2018-08-13T20:04:27Z

@cjcenizal sorry, I lost your earlier comment in the shuffle while I tried to figure out what was happening on CI.

Ultimately, I would rather not replace the core.notifications.toasts with core.inAppNotifications.toasts because I think that core.notifications is a good namespace for APIs that core wants to expose. Notice the format I'm using here though, I'm kind of testing this format out right now as the "canonical" way to refer to a new platform service specifically to reduce ambiguity between core/new-platform services and the legacy platform equivalents that will continue to exist in most cases.

I like this strategy because it matches the way that people are going to be consuming these services, and reflects the directory structure where this service can be found, which I think is really valuable for casual observers or new-comers to the project.

site note: I kind of wish it also signaled that we're referring to the browser version of the service, but that is likely relevant from context in most situations...

I think if we get in the habit of referring to the core/new-platform services as core.{{service}} it will take care of disambiguating it from other services that exist, along with the stub services that are being left behind in ui/{{service}}, and allows us to choose names that make the most sense for the core API without needing to consider all the ways this name might conflict with existing services that will at some point be migrated to the new platform and get a root level prefix of their own, like xpack.notifications.email and xpack.notification.slack... Once that is complete we'll have the core.notifications service and the xpack.notifications service, getting rid of the ambiguity around which notifications service someone might be referring to.

All that said, I give my vote to @epixa because he has probably thought this through already and his reasoning likely makes more sense than mine.

cjcenizal · 2018-08-13T20:12:16Z

Thanks for sharing your reasoning @spalger, I think all of that makes sense! When we migrate over the X-Pack notifications I think we'll need to change the name of that module or else conversations about "Kibana notifications" vs "X-Pack notifications" will just become confusing. Though I don't think that should affect this PR at all.

elasticmachine · 2018-08-13T21:38:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…21892) While debugging failures I saw in #21772 I found myself encountering failure messages like `Error: expected undefined to sort of equal true`, and other more cryptic errors caused by methods like `PageObjects.dashboard.saveDashboard()` not ensuring that the dashboard was actually saved before resolving. As part of the debugging effort I noticed that the `saveDashboard()` method does have some awareness of the success condition, but rather than asserting success within the method it returns a success boolean for the caller to check, which was only being done in a handful of tests in `test/functional/apps/dashboard/_dashboard_time.js` but was ignored the vast majority of the time. I think that most of the time we are calling `PageObjects.dashboard.saveDashboard()` we correctly assume that if the dashboard couldn't be saved for some reason the promise will be rejected and the test would fail. If the method was called `maybeSaveDashboard()` or `tryToSaveDashboard()` there might be a signal to consumers that they should check for success conditions, but that would also lead to the same checks all over the place. Instead, this PR reverses the responsibility of checking for success so that code calling `PageObjects.dashboard.saveDashboard()` can continue to assume that if something went wrong their test will fail. It also improves the error message by not using `expect(boolean).to.equal(boolean)`, instead implementing a basic `if()` statement and throwing an error with a meaningful message when something goes wrong. ```js const isDashboardSaved = await testSubjects.exists('saveDashboardSuccess'); expect(isDashboardSaved).to.eql(true); ``` is now ```js if (!await testSubjects.exists('saveDashboardSuccess')) { throw new Error('Expected to find "saveDashboardSuccess" toast after saving dashboard'); } ``` --- I think this type of change could be made to a lot of methods, and would make failures a lot easier to debug and possibly a lot less flaky if we were checking for success conditions in nearly every method we put in our PageObjects. I think it's safe to say that most of the methods we have in PageObjects do not check for actual success criteria, and sometimes that's okay: a method called `clickButton()` can safely resolve once the click method has been called, but a method like `addSampleDataSet()` should be verifying that the sample data set it set out to add was actually added.

…lastic#21892) While debugging failures I saw in elastic#21772 I found myself encountering failure messages like `Error: expected undefined to sort of equal true`, and other more cryptic errors caused by methods like `PageObjects.dashboard.saveDashboard()` not ensuring that the dashboard was actually saved before resolving. As part of the debugging effort I noticed that the `saveDashboard()` method does have some awareness of the success condition, but rather than asserting success within the method it returns a success boolean for the caller to check, which was only being done in a handful of tests in `test/functional/apps/dashboard/_dashboard_time.js` but was ignored the vast majority of the time. I think that most of the time we are calling `PageObjects.dashboard.saveDashboard()` we correctly assume that if the dashboard couldn't be saved for some reason the promise will be rejected and the test would fail. If the method was called `maybeSaveDashboard()` or `tryToSaveDashboard()` there might be a signal to consumers that they should check for success conditions, but that would also lead to the same checks all over the place. Instead, this PR reverses the responsibility of checking for success so that code calling `PageObjects.dashboard.saveDashboard()` can continue to assume that if something went wrong their test will fail. It also improves the error message by not using `expect(boolean).to.equal(boolean)`, instead implementing a basic `if()` statement and throwing an error with a meaningful message when something goes wrong. ```js const isDashboardSaved = await testSubjects.exists('saveDashboardSuccess'); expect(isDashboardSaved).to.eql(true); ``` is now ```js if (!await testSubjects.exists('saveDashboardSuccess')) { throw new Error('Expected to find "saveDashboardSuccess" toast after saving dashboard'); } ``` --- I think this type of change could be made to a lot of methods, and would make failures a lot easier to debug and possibly a lot less flaky if we were checking for success conditions in nearly every method we put in our PageObjects. I think it's safe to say that most of the methods we have in PageObjects do not check for actual success criteria, and sometimes that's okay: a method called `clickButton()` can safely resolve once the click method has been called, but a method like `addSampleDataSet()` should be verifying that the sample data set it set out to add was actually added.

Fixes elastic#20698 As part of the transition of APIs necessary for migrating the Chrome to the new platform, this moves the core logic for the toastNotifications out of `ui/notify` and into the new platform as the `core.notifications.toasts` service. I chose to use the `notifications` namespace here as I plan for the [`banners` service](https://github.com/elastic/kibana/blob/494c267cd97ba0c48c2acd10a8fff2c27defd0bb/src/ui/public/notify/banners/banners.js) from `ui/notify` to eventually live at `core.notifications.banners`. If you disagree with this strategy and would prefer that we use something like `core.toastNotifications` let me know. For the most part this service just does the same thing as the ui service did, so functionality should be exactly the same. To test the notifications I suggest using the testbed like so: https://gist.github.com/spalger/81097177c88dee142700fab25de88932

#21963) Backports the following commits to 6.x: - [toasts] migrate toastNotifications to the new platform (#21772)

spalger · 2018-08-14T22:31:07Z

6.5/6.x: 27bc4d2

spalger added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v7.0.0 v6.5.0 labels Aug 8, 2018

spalger requested review from epixa and azasypkin August 8, 2018 02:52

spalger force-pushed the migrate-new-platform/toasts branch from 19b7178 to c4d9b86 Compare August 8, 2018 18:15

spalger removed request for epixa and azasypkin August 9, 2018 15:34

spalger added WIP Work in progress and removed review labels Aug 9, 2018

spalger force-pushed the migrate-new-platform/toasts branch 2 times, most recently from 9162dd8 to c22b828 Compare August 10, 2018 18:00

elastic deleted a comment from elasticmachine Aug 10, 2018

This comment has been minimized.

Sign in to view

spalger mentioned this pull request Aug 10, 2018

[pageObjects/dashboard] check that save is complete before resolving #21892

Merged

spalger force-pushed the migrate-new-platform/toasts branch from c22b828 to 6993cfd Compare August 10, 2018 20:56

This comment has been minimized.

Sign in to view

[toasts] migrate toastNotifications to the new platform

5ce3b58

spalger force-pushed the migrate-new-platform/toasts branch from 6993cfd to 5ce3b58 Compare August 11, 2018 01:10

spalger requested review from azasypkin and epixa August 11, 2018 16:17

spalger added the review label Aug 11, 2018

spalger added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc and removed Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc WIP Work in progress labels Aug 11, 2018

azasypkin approved these changes Aug 13, 2018

View reviewed changes

spalger added 2 commits August 13, 2018 11:51

Merge branch 'master' of github.com:elastic/kibana into migrate-new-p…

ed2996c

…latform/toasts

address review feedback

321b273

spalger mentioned this pull request Aug 14, 2018

Migrate non-angular plugins to new platform toasts #21948

Closed

spalger merged commit c01c2f9 into elastic:master Aug 14, 2018

spalger mentioned this pull request Aug 14, 2018

[6.x] [toasts] migrate toastNotifications to the new platform (#21772) #21963

Merged

spalger pushed a commit that referenced this pull request Aug 14, 2018

[6.x] [toasts] migrate toastNotifications to the new platform (#21772) (

27bc4d2

#21963) Backports the following commits to 6.x: - [toasts] migrate toastNotifications to the new platform (#21772)

spalger deleted the migrate-new-platform/toasts branch August 14, 2018 22:31

ogupte mentioned this pull request Sep 25, 2019

[APM] New platform migration update toast notifications #46550

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[toasts] migrate toastNotifications to the new platform #21772

[toasts] migrate toastNotifications to the new platform #21772

spalger commented Aug 8, 2018

cjcenizal commented Aug 8, 2018

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Aug 11, 2018

azasypkin left a comment

azasypkin Aug 13, 2018

spalger Aug 13, 2018

azasypkin Aug 13, 2018

azasypkin Aug 13, 2018

azasypkin Aug 13, 2018

azasypkin Aug 13, 2018

spalger Aug 13, 2018

azasypkin Aug 13, 2018

cjcenizal Aug 13, 2018

spalger Aug 13, 2018

sorenlouv Aug 13, 2018 •

edited

Loading

sorenlouv Aug 13, 2018

spalger Aug 14, 2018

spalger Aug 14, 2018

azasypkin Aug 13, 2018

azasypkin Aug 13, 2018

spalger Aug 13, 2018

azasypkin Aug 13, 2018

cjcenizal commented Aug 13, 2018

spalger commented Aug 13, 2018

cjcenizal commented Aug 13, 2018 •

edited

Loading

elasticmachine commented Aug 13, 2018

spalger commented Aug 14, 2018


		private onChangeCallback?: () => void;

		constructor(private toasts: ToastsStartContract) {

[toasts] migrate toastNotifications to the new platform #21772

[toasts] migrate toastNotifications to the new platform #21772

Conversation

spalger commented Aug 8, 2018

cjcenizal commented Aug 8, 2018

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Aug 11, 2018

💚 Build Succeeded

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Aug 13, 2018

spalger commented Aug 13, 2018

cjcenizal commented Aug 13, 2018 • edited Loading

elasticmachine commented Aug 13, 2018

💚 Build Succeeded

spalger commented Aug 14, 2018

sorenlouv Aug 13, 2018 •

edited

Loading

cjcenizal commented Aug 13, 2018 •

edited

Loading