From e03c725b8658c812946b7c9f1addd0a9d31886eb Mon Sep 17 00:00:00 2001 From: Alex M Date: Sat, 1 Oct 2016 22:40:21 +0300 Subject: [PATCH 1/7] [Style] Minor style changes --- .../test/NotificationServiceSpec.js | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/platform/commonUI/notification/test/NotificationServiceSpec.js b/platform/commonUI/notification/test/NotificationServiceSpec.js index 92e3542692..b74b9e299a 100644 --- a/platform/commonUI/notification/test/NotificationServiceSpec.js +++ b/platform/commonUI/notification/test/NotificationServiceSpec.js @@ -19,6 +19,7 @@ * this source code distribution or the Licensing information page available * at runtime from the About dialog for additional information. *****************************************************************************/ +/*global describe,it,expect,beforeEach,jasmine*/ define( ['../src/NotificationService'], @@ -53,8 +54,7 @@ define( }; }); - it("gets a new success notification, making" + - " the notification active", function () { + it("activates success notifications", function () { var activeNotification; notificationService.notify(successModel); activeNotification = notificationService.getActiveNotification(); @@ -74,8 +74,7 @@ define( }); - it("allows specification of an info notification given just a" + - " title, making the notification active", function () { + it("activates an info notification built with just the title", function () { var activeNotification, notificationTitle = "Test info notification"; notificationService.info(notificationTitle); @@ -84,8 +83,7 @@ define( expect(activeNotification.model.severity).toBe("info"); }); - it("gets a new success notification with" + - " numerical auto-dismiss specified. ", function () { + it("gets a new success notification with numerical auto-dismiss specified. ", function () { var activeNotification; successModel.autoDismiss = 1000; notificationService.notify(successModel); @@ -98,8 +96,7 @@ define( expect(activeNotification).toBeUndefined(); }); - it("gets a new notification with" + - " boolean auto-dismiss specified. ", function () { + it("gets a new notification with boolean auto-dismiss specified. ", function () { var activeNotification; successModel.autoDismiss = true; notificationService.notify(successModel); @@ -143,9 +140,8 @@ define( expect(notificationService.notifications.length).toBe(0); }); - describe(" gets called with multiple notifications", function () { - it("auto-dismisses the previously active notification, making" + - " the new notification active", function () { + describe("when called with multiple notifications", function () { + it("auto-dismisses the previously active notification, making the new notification active", function () { var activeNotification; //First pre-load with a info message notificationService.notify(successModel); @@ -165,6 +161,7 @@ define( activeNotification = notificationService.getActiveNotification(); expect(activeNotification.model).toBe(errorModel); }); + it("auto-minimizes an active error notification", function () { var activeNotification; //First pre-load with an error message @@ -186,8 +183,8 @@ define( expect(activeNotification.model).toBe(successModel); expect(errorModel.minimized).toEqual(true); }); - it("auto-minimizes errors when a number of them arrive in" + - " short succession ", function () { + + it("auto-minimizes errors when a number of them arrive in short succession", function () { var activeNotification, error2 = { title: "Second Mock Error Notification", @@ -228,7 +225,6 @@ define( notificationService.getActiveNotification(); expect(activeNotification.model).toBe(error3); expect(error2.minimized).toEqual(true); - }); }); }); From 0442a3115395f8949080c5cdd2ec6bfc04fb8b50 Mon Sep 17 00:00:00 2001 From: Alex M Date: Sun, 2 Oct 2016 20:40:53 +0300 Subject: [PATCH 2/7] [Notification] Rename variables for clarity In the spec: NotificationService knows about concepts like "info", "alert" and "error" only, having a plain info notification model called successModel doesn't help much and may be confusing at times (makes you think it's something the service treats separately). --- .../notification/src/NotificationService.js | 18 +++---- .../test/NotificationServiceSpec.js | 50 ++++++++++--------- 2 files changed, 35 insertions(+), 33 deletions(-) diff --git a/platform/commonUI/notification/src/NotificationService.js b/platform/commonUI/notification/src/NotificationService.js index e37571759a..8f5b71ec42 100644 --- a/platform/commonUI/notification/src/NotificationService.js +++ b/platform/commonUI/notification/src/NotificationService.js @@ -109,18 +109,18 @@ define( * @memberof platform/commonUI/notification * @constructor * @param $timeout the Angular $timeout service - * @param DEFAULT_AUTO_DISMISS The period of time that an + * @param defaultAutoDismissTimeout The period of time that an * auto-dismissed message will be displayed for. - * @param MINIMIZE_TIMEOUT When notifications are minimized, a brief + * @param minimizeAnimationTimeout When notifications are minimized, a brief * animation is shown. This animation requires some time to execute, * so a timeout is required before the notification is hidden */ - function NotificationService($timeout, topic, DEFAULT_AUTO_DISMISS, MINIMIZE_TIMEOUT) { + function NotificationService($timeout, topic, defaultAutoDismissTimeout, minimizeAnimationTimeout) { this.notifications = []; this.$timeout = $timeout; this.highest = { severity: "info" }; - this.DEFAULT_AUTO_DISMISS = DEFAULT_AUTO_DISMISS; - this.MINIMIZE_TIMEOUT = MINIMIZE_TIMEOUT; + this.AUTO_DISMISS_TIMEOUT = defaultAutoDismissTimeout; + this.MINIMIZE_ANIMATION_TIMEOUT = minimizeAnimationTimeout; this.topic = topic; /* @@ -162,7 +162,7 @@ define( // in order to allow the minimize animation to run through. service.$timeout(function () { service.setActiveNotification(service.selectNextNotification()); - }, service.MINIMIZE_TIMEOUT); + }, service.MINIMIZE_ANIMATION_TIMEOUT); } }; @@ -325,7 +325,7 @@ define( notificationModel.severity = notificationModel.severity || "info"; if (notificationModel.autoDismiss === true) { - notificationModel.autoDismiss = this.DEFAULT_AUTO_DISMISS; + notificationModel.autoDismiss = this.AUTO_DISMISS_TIMEOUT; } //Notifications support a 'dismissable' attribute. This is a @@ -366,7 +366,7 @@ define( */ this.active.timeout = this.$timeout(function () { activeNotification.dismissOrMinimize(); - }, this.DEFAULT_AUTO_DISMISS); + }, this.AUTO_DISMISS_TIMEOUT); } return notification; @@ -390,7 +390,7 @@ define( if (notification && (notification.model.autoDismiss || this.selectNextNotification())) { - timeout = notification.model.autoDismiss || this.DEFAULT_AUTO_DISMISS; + timeout = notification.model.autoDismiss || this.AUTO_DISMISS_TIMEOUT; this.active.timeout = this.$timeout(function () { notification.dismissOrMinimize(); }, timeout); diff --git a/platform/commonUI/notification/test/NotificationServiceSpec.js b/platform/commonUI/notification/test/NotificationServiceSpec.js index b74b9e299a..715894a077 100644 --- a/platform/commonUI/notification/test/NotificationServiceSpec.js +++ b/platform/commonUI/notification/test/NotificationServiceSpec.js @@ -30,9 +30,9 @@ define( mockTimeout, mockAutoDismiss, mockMinimizeTimeout, - successModel, mockTopicFunction, mockTopicObject, + infoModel, errorModel; beforeEach(function () { @@ -44,27 +44,29 @@ define( mockAutoDismiss = mockMinimizeTimeout = 1000; notificationService = new NotificationService( mockTimeout, mockTopicFunction, mockAutoDismiss, mockMinimizeTimeout); - successModel = { - title: "Mock Success Notification", + + infoModel = { + title: "Mock Info Notification", severity: "info" }; + errorModel = { title: "Mock Error Notification", severity: "error" }; }); - it("activates success notifications", function () { + it("activates info notifications", function () { var activeNotification; - notificationService.notify(successModel); + notificationService.notify(infoModel); activeNotification = notificationService.getActiveNotification(); - expect(activeNotification.model).toBe(successModel); + expect(activeNotification.model).toBe(infoModel); }); it("notifies listeners on dismissal of notification", function () { var notification, dismissListener = jasmine.createSpy("ondismiss"); - notification = notificationService.notify(successModel); + notification = notificationService.notify(infoModel); notification.onDismiss(dismissListener); expect(mockTopicObject.listen).toHaveBeenCalled(); notification.dismiss(); @@ -83,12 +85,12 @@ define( expect(activeNotification.model.severity).toBe("info"); }); - it("gets a new success notification with numerical auto-dismiss specified. ", function () { + it("gets a new info notification with numerical auto-dismiss specified. ", function () { var activeNotification; - successModel.autoDismiss = 1000; - notificationService.notify(successModel); + infoModel.autoDismiss = 1000; + notificationService.notify(infoModel); activeNotification = notificationService.getActiveNotification(); - expect(activeNotification.model).toBe(successModel); + expect(activeNotification.model).toBe(infoModel); mockTimeout.mostRecentCall.args[0](); expect(mockTimeout.calls.length).toBe(2); mockTimeout.mostRecentCall.args[0](); @@ -98,10 +100,10 @@ define( it("gets a new notification with boolean auto-dismiss specified. ", function () { var activeNotification; - successModel.autoDismiss = true; - notificationService.notify(successModel); + infoModel.autoDismiss = true; + notificationService.notify(infoModel); activeNotification = notificationService.getActiveNotification(); - expect(activeNotification.model).toBe(successModel); + expect(activeNotification.model).toBe(infoModel); mockTimeout.mostRecentCall.args[0](); expect(mockTimeout.calls.length).toBe(2); mockTimeout.mostRecentCall.args[0](); @@ -113,11 +115,11 @@ define( var notification, activeNotification; - successModel.autoDismiss = false; - notification = notificationService.notify(successModel); + infoModel.autoDismiss = false; + notification = notificationService.notify(infoModel); activeNotification = notificationService.getActiveNotification(); - expect(activeNotification.model).toBe(successModel); + expect(activeNotification.model).toBe(infoModel); notification.minimize(); mockTimeout.mostRecentCall.args[0](); activeNotification = notificationService.getActiveNotification(); @@ -129,11 +131,11 @@ define( var notification, activeNotification; - successModel.autoDismiss = false; - notification = notificationService.notify(successModel); + infoModel.autoDismiss = false; + notification = notificationService.notify(infoModel); activeNotification = notificationService.getActiveNotification(); - expect(activeNotification.model).toBe(successModel); + expect(activeNotification.model).toBe(infoModel); notification.dismiss(); activeNotification = notificationService.getActiveNotification(); expect(activeNotification).toBeUndefined(); @@ -144,11 +146,11 @@ define( it("auto-dismisses the previously active notification, making the new notification active", function () { var activeNotification; //First pre-load with a info message - notificationService.notify(successModel); + notificationService.notify(infoModel); activeNotification = notificationService.getActiveNotification(); //Initially expect the active notification to be info - expect(activeNotification.model).toBe(successModel); + expect(activeNotification.model).toBe(infoModel); //Then notify of an error notificationService.notify(errorModel); //But it should be auto-dismissed and replaced with the @@ -167,7 +169,7 @@ define( //First pre-load with an error message notificationService.notify(errorModel); //Then notify of info - notificationService.notify(successModel); + notificationService.notify(infoModel); expect(notificationService.notifications.length).toEqual(2); //Mock the auto-minimize mockTimeout.mostRecentCall.args[0](); @@ -180,7 +182,7 @@ define( expect(notificationService.notifications.length).toEqual(2); activeNotification = notificationService.getActiveNotification(); - expect(activeNotification.model).toBe(successModel); + expect(activeNotification.model).toBe(infoModel); expect(errorModel.minimized).toEqual(true); }); From f2fe6a9885d419110e18fd0f4a33f4553d1de1e0 Mon Sep 17 00:00:00 2001 From: Alex M Date: Mon, 3 Oct 2016 04:19:55 +0300 Subject: [PATCH 3/7] [Notification] New tests + bugfix New test suite for the notification service covering common use cases. Fixed the bug where autoDismiss would be set wrong for info notifications; also treating info notifications appropriately now. --- .../notification/src/NotificationService.js | 64 +++-- .../test/NotificationServiceSpec.js | 230 +++++++++++------- 2 files changed, 187 insertions(+), 107 deletions(-) diff --git a/platform/commonUI/notification/src/NotificationService.js b/platform/commonUI/notification/src/NotificationService.js index 8f5b71ec42..39104faca3 100644 --- a/platform/commonUI/notification/src/NotificationService.js +++ b/platform/commonUI/notification/src/NotificationService.js @@ -208,11 +208,11 @@ define( * @private */ NotificationService.prototype.dismissOrMinimize = function (notification) { - - //For now minimize everything, and have discussion around which - //kind of messages should or should not be in the minimized - //notifications list - notification.minimize(); + if (notification.minimizeInsteadOfDismiss) { + notification.minimize(); + } else { + notification.dismiss(); + } }; /** @@ -226,7 +226,9 @@ define( /** * A convenience method for info notifications. Notifications * created via this method will be auto-dismissed after a default - * wait period + * wait period unless explicitly forbidden by the caller through + * the {autoDismiss} property on the {NotificationModel}, in which + * case the notification will be minimized after the wait. * @param {NotificationModel | string} message either a string for * the title of the notification message, or a {@link NotificationModel} * defining the options notification to display @@ -235,7 +237,6 @@ define( */ NotificationService.prototype.info = function (message) { var notificationModel = typeof message === "string" ? {title: message} : message; - notificationModel.autoDismiss = notificationModel.autoDismiss || true; notificationModel.severity = "info"; return this.notify(notificationModel); }; @@ -306,27 +307,52 @@ define( activeNotification = self.active.notification, topic = this.topic(); + notificationModel.severity = notificationModel.severity || "info"; + notification = { model: notificationModel, + minimize: function () { self.minimize(self, notification); }, + dismiss: function () { self.dismiss(self, notification); topic.notify(); }, + dismissOrMinimize: function () { self.dismissOrMinimize(notification); }, + onDismiss: function (callback) { topic.listen(callback); - } - }; + }, - notificationModel.severity = notificationModel.severity || "info"; - if (notificationModel.autoDismiss === true) { - notificationModel.autoDismiss = this.AUTO_DISMISS_TIMEOUT; - } + autoDismiss: (function () { + if (notificationModel.severity === "info") { + return true; + } + return notificationModel.autoDismiss; + })(), + + autoDismissTimeout: (function () { + if (typeof notificationModel.autoDismiss === "number") { + return notificationModel.autoDismiss; + } + return self.AUTO_DISMISS_TIMEOUT; + })(), + + minimizeInsteadOfDismiss: (function () { + if (notificationModel.severity === "info") { + if (notificationModel.autoDismiss === false) { + return true; + } + return false; + } + return true; + })() + }; //Notifications support a 'dismissable' attribute. This is a // convenience to support adding a 'dismiss' option to the @@ -370,27 +396,23 @@ define( } return notification; - }; /** * Used internally by the NotificationService * @private */ - NotificationService.prototype.setActiveNotification = - function (notification) { + NotificationService.prototype.setActiveNotification = function (notification) { var timeout; - this.active.notification = notification; + /* If autoDismiss has been specified, OR there are other notifications queued for display, setup a timeout to dismiss the dialog. */ - if (notification && (notification.model.autoDismiss || - this.selectNextNotification())) { - - timeout = notification.model.autoDismiss || this.AUTO_DISMISS_TIMEOUT; + if (notification && (notification.autoDismiss || this.selectNextNotification())) { + timeout = notification.autoDismissTimeout || this.AUTO_DISMISS_TIMEOUT; this.active.timeout = this.$timeout(function () { notification.dismissOrMinimize(); }, timeout); diff --git a/platform/commonUI/notification/test/NotificationServiceSpec.js b/platform/commonUI/notification/test/NotificationServiceSpec.js index 715894a077..2c90dce621 100644 --- a/platform/commonUI/notification/test/NotificationServiceSpec.js +++ b/platform/commonUI/notification/test/NotificationServiceSpec.js @@ -33,8 +33,13 @@ define( mockTopicFunction, mockTopicObject, infoModel, + alertModel, errorModel; + function elapseTimeout() { + mockTimeout.mostRecentCall.args[0](); + } + beforeEach(function () { mockTimeout = jasmine.createSpy("$timeout"); mockTopicFunction = jasmine.createSpy("topic"); @@ -42,124 +47,180 @@ define( mockTopicFunction.andReturn(mockTopicObject); mockAutoDismiss = mockMinimizeTimeout = 1000; - notificationService = new NotificationService( - mockTimeout, mockTopicFunction, mockAutoDismiss, mockMinimizeTimeout); + notificationService = new NotificationService(mockTimeout, mockTopicFunction, mockAutoDismiss, mockMinimizeTimeout); infoModel = { title: "Mock Info Notification", severity: "info" }; + alertModel = { + title: "Mock Alert Notification", + severity: "alert" + }; + errorModel = { title: "Mock Error Notification", severity: "error" }; }); - it("activates info notifications", function () { - var activeNotification; - notificationService.notify(infoModel); - activeNotification = notificationService.getActiveNotification(); - expect(activeNotification.model).toBe(infoModel); - }); - it("notifies listeners on dismissal of notification", function () { - var notification, - dismissListener = jasmine.createSpy("ondismiss"); - notification = notificationService.notify(infoModel); + var dismissListener = jasmine.createSpy("ondismiss"); + var notification = notificationService.notify(infoModel); notification.onDismiss(dismissListener); expect(mockTopicObject.listen).toHaveBeenCalled(); notification.dismiss(); expect(mockTopicObject.notify).toHaveBeenCalled(); mockTopicObject.listen.mostRecentCall.args[0](); expect(dismissListener).toHaveBeenCalled(); - }); - it("activates an info notification built with just the title", function () { - var activeNotification, - notificationTitle = "Test info notification"; - notificationService.info(notificationTitle); - activeNotification = notificationService.getActiveNotification(); - expect(activeNotification.model.title).toBe(notificationTitle); - expect(activeNotification.model.severity).toBe("info"); + describe("when receiving info notifications", function () { + it("minimizes info notifications if the caller disables auto-dismiss", function () { + infoModel.autoDismiss = false; + var notification = notificationService.info(infoModel); + elapseTimeout(); + // 2nd elapse for the minimize animation timeout + elapseTimeout(); + expect(notificationService.getActiveNotification()).toBeUndefined(); + expect(notificationService.notifications.length).toEqual(1); + expect(notificationService.notifications[0]).toEqual(notification); + }); + + it("dismisses info notifications if the caller ignores auto-dismiss", function () { + notificationService.info(infoModel); + elapseTimeout(); + expect(notificationService.getActiveNotification()).toBeUndefined(); + expect(notificationService.notifications.length).toEqual(0); + }); + + it("dismisses info notifications if the caller requests auto-dismissal", function () { + infoModel.autoDismiss = true; + notificationService.info(infoModel); + elapseTimeout(); + expect(notificationService.getActiveNotification()).toBeUndefined(); + expect(notificationService.notifications.length).toEqual(0); + }); + + it("uses a custom auto-dismiss timeout value if provided", function () { + var timeoutValueInModel = 1500; + var timeoutValueUsedByService = 0; + mockTimeout.andCallFake(function (callback, timeout) { + timeoutValueUsedByService = timeout; + }); + infoModel.autoDismiss = timeoutValueInModel; + notificationService.info(infoModel); + expect(timeoutValueUsedByService).toEqual(timeoutValueInModel); + }); }); - it("gets a new info notification with numerical auto-dismiss specified. ", function () { - var activeNotification; - infoModel.autoDismiss = 1000; - notificationService.notify(infoModel); - activeNotification = notificationService.getActiveNotification(); - expect(activeNotification.model).toBe(infoModel); - mockTimeout.mostRecentCall.args[0](); - expect(mockTimeout.calls.length).toBe(2); - mockTimeout.mostRecentCall.args[0](); - activeNotification = notificationService.getActiveNotification(); - expect(activeNotification).toBeUndefined(); + describe("when receiving alert notifications", function () { + it("minimizes alert notifications if the caller enables auto-dismiss", function () { + alertModel.autoDismiss = true; + var notification = notificationService.alert(alertModel); + elapseTimeout(); + elapseTimeout(); + expect(notificationService.getActiveNotification()).toBeUndefined(); + expect(notificationService.notifications.length).toEqual(1); + expect(notificationService.notifications[0]).toEqual(notification); + }); + + it("keeps alert notifications active if the caller disables auto-dismiss", function () { + mockTimeout.andCallFake(function (callback, time) { + callback(); + }); + alertModel.autoDismiss = false; + var notification = notificationService.alert(alertModel); + expect(notificationService.getActiveNotification()).toEqual(notification); + expect(notificationService.notifications.length).toEqual(1); + expect(notificationService.notifications[0]).toEqual(notification); + }); + + it("keeps alert notifications active if the caller ignores auto-dismiss", function () { + mockTimeout.andCallFake(function (callback, time) { + callback(); + }); + var notification = notificationService.alert(alertModel); + expect(notificationService.getActiveNotification()).toEqual(notification); + expect(notificationService.notifications.length).toEqual(1); + expect(notificationService.notifications[0]).toEqual(notification); + }); + + it("uses a custom auto-dismiss timeout value if provided", function () { + var timeoutValueInModel = 1500; + var timeoutValueUsedByService = 0; + mockTimeout.andCallFake(function (callback, timeout) { + timeoutValueUsedByService = timeout; + }); + alertModel.autoDismiss = timeoutValueInModel; + notificationService.alert(alertModel); + expect(timeoutValueUsedByService).toEqual(timeoutValueInModel); + }); }); - it("gets a new notification with boolean auto-dismiss specified. ", function () { - var activeNotification; - infoModel.autoDismiss = true; - notificationService.notify(infoModel); - activeNotification = notificationService.getActiveNotification(); - expect(activeNotification.model).toBe(infoModel); - mockTimeout.mostRecentCall.args[0](); - expect(mockTimeout.calls.length).toBe(2); - mockTimeout.mostRecentCall.args[0](); - activeNotification = notificationService.getActiveNotification(); - expect(activeNotification).toBeUndefined(); - }); + describe("when receiving error notifications", function () { + it("minimizes error notifications if the caller enables auto-dismiss", function () { + errorModel.autoDismiss = true; + var notification = notificationService.error(errorModel); + elapseTimeout(); + elapseTimeout(); + expect(notificationService.getActiveNotification()).toBeUndefined(); + expect(notificationService.notifications.length).toEqual(1); + expect(notificationService.notifications[0]).toEqual(notification); + }); - it("allows minimization of notifications", function () { - var notification, - activeNotification; + it("keeps error notifications active if the caller disables auto-dismiss", function () { + mockTimeout.andCallFake(function (callback, time) { + callback(); + }); + errorModel.autoDismiss = false; + var notification = notificationService.error(errorModel); + expect(notificationService.getActiveNotification()).toEqual(notification); + expect(notificationService.notifications.length).toEqual(1); + expect(notificationService.notifications[0]).toEqual(notification); + }); - infoModel.autoDismiss = false; - notification = notificationService.notify(infoModel); + it("keeps error notifications active if the caller ignores auto-dismiss", function () { + mockTimeout.andCallFake(function (callback, time) { + callback(); + }); + var notification = notificationService.error(errorModel); + expect(notificationService.getActiveNotification()).toEqual(notification); + expect(notificationService.notifications.length).toEqual(1); + expect(notificationService.notifications[0]).toEqual(notification); + }); - activeNotification = notificationService.getActiveNotification(); - expect(activeNotification.model).toBe(infoModel); - notification.minimize(); - mockTimeout.mostRecentCall.args[0](); - activeNotification = notificationService.getActiveNotification(); - expect(activeNotification).toBeUndefined(); - expect(notificationService.notifications.length).toBe(1); - }); - - it("allows dismissal of notifications", function () { - var notification, - activeNotification; - - infoModel.autoDismiss = false; - notification = notificationService.notify(infoModel); - - activeNotification = notificationService.getActiveNotification(); - expect(activeNotification.model).toBe(infoModel); - notification.dismiss(); - activeNotification = notificationService.getActiveNotification(); - expect(activeNotification).toBeUndefined(); - expect(notificationService.notifications.length).toBe(0); + it("uses a custom auto-dismiss timeout value if provided", function () { + var timeoutValueInModel = 1500; + var timeoutValueUsedByService = 0; + mockTimeout.andCallFake(function (callback, timeout) { + timeoutValueUsedByService = timeout; + }); + errorModel.autoDismiss = timeoutValueInModel; + notificationService.error(errorModel); + expect(timeoutValueUsedByService).toEqual(timeoutValueInModel); + }); }); describe("when called with multiple notifications", function () { it("auto-dismisses the previously active notification, making the new notification active", function () { var activeNotification; + infoModel.autoDismiss = false; //First pre-load with a info message notificationService.notify(infoModel); - activeNotification = - notificationService.getActiveNotification(); + activeNotification = notificationService.getActiveNotification(); //Initially expect the active notification to be info expect(activeNotification.model).toBe(infoModel); //Then notify of an error notificationService.notify(errorModel); //But it should be auto-dismissed and replaced with the // error notification - mockTimeout.mostRecentCall.args[0](); + elapseTimeout(); //Two timeouts, one is to force minimization after // displaying the message for a minimum period, the // second is to allow minimization animation to take place. - mockTimeout.mostRecentCall.args[0](); + elapseTimeout(); activeNotification = notificationService.getActiveNotification(); expect(activeNotification.model).toBe(errorModel); }); @@ -172,16 +233,15 @@ define( notificationService.notify(infoModel); expect(notificationService.notifications.length).toEqual(2); //Mock the auto-minimize - mockTimeout.mostRecentCall.args[0](); + elapseTimeout(); //Two timeouts, one is to force minimization after // displaying the message for a minimum period, the // second is to allow minimization animation to take place. - mockTimeout.mostRecentCall.args[0](); + elapseTimeout(); //Previous error message should be minimized, not // dismissed expect(notificationService.notifications.length).toEqual(2); - activeNotification = - notificationService.getActiveNotification(); + activeNotification = notificationService.getActiveNotification(); expect(activeNotification.model).toBe(infoModel); expect(errorModel.minimized).toEqual(true); }); @@ -204,27 +264,25 @@ define( notificationService.notify(error3); expect(notificationService.notifications.length).toEqual(3); //Mock the auto-minimize - mockTimeout.mostRecentCall.args[0](); + elapseTimeout(); //Two timeouts, one is to force minimization after // displaying the message for a minimum period, the // second is to allow minimization animation to take place. - mockTimeout.mostRecentCall.args[0](); + elapseTimeout(); //Previous error message should be minimized, not // dismissed expect(notificationService.notifications.length).toEqual(3); - activeNotification = - notificationService.getActiveNotification(); + activeNotification = notificationService.getActiveNotification(); expect(activeNotification.model).toBe(error2); expect(errorModel.minimized).toEqual(true); //Mock the second auto-minimize - mockTimeout.mostRecentCall.args[0](); + elapseTimeout(); //Two timeouts, one is to force minimization after // displaying the message for a minimum period, the // second is to allow minimization animation to take place. - mockTimeout.mostRecentCall.args[0](); - activeNotification = - notificationService.getActiveNotification(); + elapseTimeout(); + activeNotification = notificationService.getActiveNotification(); expect(activeNotification.model).toBe(error3); expect(error2.minimized).toEqual(true); }); From 9456370077a1648bba0a67e1c5a1bf2b6f427dec Mon Sep 17 00:00:00 2001 From: Alex M Date: Sat, 8 Oct 2016 21:36:31 +0300 Subject: [PATCH 4/7] [Notifications] Extract minimize and dismiss logic --- .../notification/src/NotificationService.js | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/platform/commonUI/notification/src/NotificationService.js b/platform/commonUI/notification/src/NotificationService.js index 39104faca3..9765698343 100644 --- a/platform/commonUI/notification/src/NotificationService.js +++ b/platform/commonUI/notification/src/NotificationService.js @@ -208,10 +208,15 @@ define( * @private */ NotificationService.prototype.dismissOrMinimize = function (notification) { - if (notification.minimizeInsteadOfDismiss) { - notification.minimize(); + var model = notification.model; + if (model.severity === "info") { + if (model.autoDismiss === false) { + notification.minimize(); + } else { + notification.dismiss(); + } } else { - notification.dismiss(); + notification.minimize(); } }; @@ -341,16 +346,6 @@ define( return notificationModel.autoDismiss; } return self.AUTO_DISMISS_TIMEOUT; - })(), - - minimizeInsteadOfDismiss: (function () { - if (notificationModel.severity === "info") { - if (notificationModel.autoDismiss === false) { - return true; - } - return false; - } - return true; })() }; From d1d2067ad5b2c9ac0b92474fa9d47c338f0f5ed7 Mon Sep 17 00:00:00 2001 From: Alex M Date: Sat, 8 Oct 2016 22:51:01 +0300 Subject: [PATCH 5/7] [Notifications] Extract autoDismiss logic --- .../notification/src/NotificationService.js | 58 +++++++++---------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/platform/commonUI/notification/src/NotificationService.js b/platform/commonUI/notification/src/NotificationService.js index 9765698343..3d9c92db90 100644 --- a/platform/commonUI/notification/src/NotificationService.js +++ b/platform/commonUI/notification/src/NotificationService.js @@ -332,21 +332,7 @@ define( onDismiss: function (callback) { topic.listen(callback); - }, - - autoDismiss: (function () { - if (notificationModel.severity === "info") { - return true; - } - return notificationModel.autoDismiss; - })(), - - autoDismissTimeout: (function () { - if (typeof notificationModel.autoDismiss === "number") { - return notificationModel.autoDismiss; - } - return self.AUTO_DISMISS_TIMEOUT; - })() + } }; //Notifications support a 'dismissable' attribute. This is a @@ -398,23 +384,33 @@ define( * @private */ NotificationService.prototype.setActiveNotification = function (notification) { - var timeout; - this.active.notification = notification; + var timeout; + var shouldAutoDismiss; + var usesCustomTimeout; + this.active.notification = notification; - /* - If autoDismiss has been specified, OR there are other - notifications queued for display, setup a timeout to - dismiss the dialog. - */ - if (notification && (notification.autoDismiss || this.selectNextNotification())) { - timeout = notification.autoDismissTimeout || this.AUTO_DISMISS_TIMEOUT; - this.active.timeout = this.$timeout(function () { - notification.dismissOrMinimize(); - }, timeout); - } else { - delete this.active.timeout; - } - }; + if (!notification) { + delete this.active.timeout; + return; + } + + usesCustomTimeout = typeof notification.model.autoDismiss === "number"; + + if (notification.model.severity === "info" || usesCustomTimeout) { + shouldAutoDismiss = true; + } else { + shouldAutoDismiss = notification.model.autoDismiss; + } + + if (shouldAutoDismiss || this.selectNextNotification()) { + timeout = usesCustomTimeout ? notification.model.autoDismiss : this.AUTO_DISMISS_TIMEOUT; + this.active.timeout = this.$timeout(function () { + notification.dismissOrMinimize(); + }, timeout); + } else { + delete this.active.timeout; + } + }; /** * Used internally by the NotificationService From bfdf7b822fad5c189fe5d997c95d836b17663e0d Mon Sep 17 00:00:00 2001 From: Alex M Date: Sun, 16 Oct 2016 15:34:11 +0300 Subject: [PATCH 6/7] [Notifications] Cover direct dismiss and minimize --- .../test/NotificationServiceSpec.js | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/platform/commonUI/notification/test/NotificationServiceSpec.js b/platform/commonUI/notification/test/NotificationServiceSpec.js index 2c90dce621..5645c185ea 100644 --- a/platform/commonUI/notification/test/NotificationServiceSpec.js +++ b/platform/commonUI/notification/test/NotificationServiceSpec.js @@ -76,6 +76,22 @@ define( expect(dismissListener).toHaveBeenCalled(); }); + it("dismisses a notification when the notification's dismiss method is used", function () { + var notification = notificationService.info(infoModel); + notification.dismiss(); + expect(notificationService.getActiveNotification()).toBeUndefined(); + expect(notificationService.notifications.length).toEqual(0); + }); + + it("minimizes a notification when the notification's minimize method is used", function () { + var notification = notificationService.info(infoModel); + notification.minimize(); + elapseTimeout(); // needed for the minimize animation timeout + expect(notificationService.getActiveNotification()).toBeUndefined(); + expect(notificationService.notifications.length).toEqual(1); + expect(notificationService.notifications[0]).toEqual(notification); + }); + describe("when receiving info notifications", function () { it("minimizes info notifications if the caller disables auto-dismiss", function () { infoModel.autoDismiss = false; @@ -95,7 +111,7 @@ define( expect(notificationService.notifications.length).toEqual(0); }); - it("dismisses info notifications if the caller requests auto-dismissal", function () { + it("dismisses info notifications if the caller requests auto-dismiss", function () { infoModel.autoDismiss = true; notificationService.info(infoModel); elapseTimeout(); From d63c401e4463f6d725f6c079c5217dfbfe06e7ba Mon Sep 17 00:00:00 2001 From: Alex M Date: Sat, 29 Oct 2016 11:18:07 +0300 Subject: [PATCH 7/7] [Notifications] Remove custom autoDismiss --- .../notification/src/NotificationService.js | 11 ++----- .../test/NotificationServiceSpec.js | 33 ------------------- 2 files changed, 3 insertions(+), 41 deletions(-) diff --git a/platform/commonUI/notification/src/NotificationService.js b/platform/commonUI/notification/src/NotificationService.js index 3d9c92db90..3cdc442271 100644 --- a/platform/commonUI/notification/src/NotificationService.js +++ b/platform/commonUI/notification/src/NotificationService.js @@ -61,7 +61,7 @@ define( * @property {boolean} [unknownProgress] a boolean indicating that the * progress of the underlying task is unknown. This will result in a * visually distinct progress bar. - * @property {boolean | number} [autoDismiss] If truthy, dialog will + * @property {boolean} [autoDismiss] If truthy, dialog will * be automatically minimized or dismissed (depending on severity). * Additionally, if the provided value is a number, it will be used * as the delay period before being dismissed. @@ -384,9 +384,7 @@ define( * @private */ NotificationService.prototype.setActiveNotification = function (notification) { - var timeout; var shouldAutoDismiss; - var usesCustomTimeout; this.active.notification = notification; if (!notification) { @@ -394,19 +392,16 @@ define( return; } - usesCustomTimeout = typeof notification.model.autoDismiss === "number"; - - if (notification.model.severity === "info" || usesCustomTimeout) { + if (notification.model.severity === "info") { shouldAutoDismiss = true; } else { shouldAutoDismiss = notification.model.autoDismiss; } if (shouldAutoDismiss || this.selectNextNotification()) { - timeout = usesCustomTimeout ? notification.model.autoDismiss : this.AUTO_DISMISS_TIMEOUT; this.active.timeout = this.$timeout(function () { notification.dismissOrMinimize(); - }, timeout); + }, this.AUTO_DISMISS_TIMEOUT); } else { delete this.active.timeout; } diff --git a/platform/commonUI/notification/test/NotificationServiceSpec.js b/platform/commonUI/notification/test/NotificationServiceSpec.js index 5645c185ea..c722c8e468 100644 --- a/platform/commonUI/notification/test/NotificationServiceSpec.js +++ b/platform/commonUI/notification/test/NotificationServiceSpec.js @@ -118,17 +118,6 @@ define( expect(notificationService.getActiveNotification()).toBeUndefined(); expect(notificationService.notifications.length).toEqual(0); }); - - it("uses a custom auto-dismiss timeout value if provided", function () { - var timeoutValueInModel = 1500; - var timeoutValueUsedByService = 0; - mockTimeout.andCallFake(function (callback, timeout) { - timeoutValueUsedByService = timeout; - }); - infoModel.autoDismiss = timeoutValueInModel; - notificationService.info(infoModel); - expect(timeoutValueUsedByService).toEqual(timeoutValueInModel); - }); }); describe("when receiving alert notifications", function () { @@ -162,17 +151,6 @@ define( expect(notificationService.notifications.length).toEqual(1); expect(notificationService.notifications[0]).toEqual(notification); }); - - it("uses a custom auto-dismiss timeout value if provided", function () { - var timeoutValueInModel = 1500; - var timeoutValueUsedByService = 0; - mockTimeout.andCallFake(function (callback, timeout) { - timeoutValueUsedByService = timeout; - }); - alertModel.autoDismiss = timeoutValueInModel; - notificationService.alert(alertModel); - expect(timeoutValueUsedByService).toEqual(timeoutValueInModel); - }); }); describe("when receiving error notifications", function () { @@ -206,17 +184,6 @@ define( expect(notificationService.notifications.length).toEqual(1); expect(notificationService.notifications[0]).toEqual(notification); }); - - it("uses a custom auto-dismiss timeout value if provided", function () { - var timeoutValueInModel = 1500; - var timeoutValueUsedByService = 0; - mockTimeout.andCallFake(function (callback, timeout) { - timeoutValueUsedByService = timeout; - }); - errorModel.autoDismiss = timeoutValueInModel; - notificationService.error(errorModel); - expect(timeoutValueUsedByService).toEqual(timeoutValueInModel); - }); }); describe("when called with multiple notifications", function () {