Merge pull request #2094 from hypothesis/remove-toastr

Replace remaining uses angular-toastr/flash with toastMessenger

gulpfile.js

@@ -158,7 +158,6 @@ const cssBundles = [
   './src/styles/vendor/angular-csp.css',
   './src/styles/vendor/icomoon.css',
   './node_modules/katex/dist/katex.min.css',
-  './node_modules/angular-toastr/dist/angular-toastr.css',
 ];
 
 gulp.task('build-css', function () {

package.json

@@ -14,7 +14,6 @@
     "@sentry/browser": "^5.6.2",
     "angular": "^1.7.5",
     "angular-mocks": "^1.7.5",
-    "angular-toastr": "^2.1.1",
     "autoprefixer": "^9.4.7",
     "aws-sdk": "^2.345.0",
     "axe-core": "^3.4.1",

scripts/gulp/vendor-bundles.js

@@ -9,7 +9,7 @@
 module.exports = {
   bundles: {
     jquery: ['jquery'],
-    angular: ['angular', 'angular-toastr'],
+    angular: ['angular'],
     katex: ['katex'],
     sentry: ['@sentry/browser'],
     showdown: ['showdown'],

src/boot/boot.js

@@ -120,7 +120,6 @@ function bootSidebarApp(doc, config) {
     'scripts/sidebar.bundle.js',
 
     'styles/angular-csp.css',
-    'styles/angular-toastr.css',
     'styles/katex.min.css',
     'styles/sidebar.css',
   ]);

src/boot/test/boot-test.js

@@ -47,7 +47,6 @@ describe('bootstrap', function () {
       'scripts/sidebar.bundle.js',
 
       'styles/angular-csp.css',
-      'styles/angular-toastr.css',
       'styles/katex.min.css',
       'styles/sidebar.css',
     ];
@@ -153,7 +152,6 @@ describe('bootstrap', function () {
         'scripts/showdown.bundle.1234.js',
         'scripts/sidebar.bundle.1234.js',
         'styles/angular-csp.1234.css',
-        'styles/angular-toastr.1234.css',
         'styles/katex.min.1234.css',
         'styles/sidebar.1234.css',
       ].map(assetUrl);

src/sidebar/components/annotation-action-bar.js

@@ -44,7 +44,7 @@ function AnnotationActionBar({
   const onDelete = () => {
     if (window.confirm('Are you sure you want to delete this annotation?')) {
       annotationsService.delete(annotation).catch(err => {
-        toastMessenger.error(err.message, 'Deleting annotation failed');
+        toastMessenger.error(err.message);
       });
     }
   };

src/sidebar/components/hypothesis-app.js

@@ -42,12 +42,12 @@ function HypothesisAppController(
   auth,
   bridge,
   features,
-  flash,
   frameSync,
   groups,
   serviceUrl,
   session,
-  settings
+  settings,
+  toastMessenger
 ) {
   const self = this;
 
@@ -107,7 +107,7 @@ function HypothesisAppController(
         session.reload();
       })
       .catch(err => {
-        flash.error(err.message);
+        toastMessenger.error(err.message);
       });
   };
 

src/sidebar/components/test/hypothesis-app-test.js

@@ -15,7 +15,6 @@ describe('sidebar.components.hypothesis-app', function () {
   let fakeAuth = null;
   let fakeBridge = null;
   let fakeFeatures = null;
-  let fakeFlash = null;
   let fakeFrameSync = null;
   let fakeIsSidebar = null;
   let fakeServiceConfig = null;
@@ -24,6 +23,7 @@ describe('sidebar.components.hypothesis-app', function () {
   let fakeGroups = null;
   let fakeServiceUrl = null;
   let fakeSettings = null;
+  let fakeToastMessenger = null;
   let fakeWindow = null;
 
   let sandbox = null;
@@ -93,10 +93,6 @@ describe('sidebar.components.hypothesis-app', function () {
         flagEnabled: sandbox.stub().returns(false),
       };
 
-      fakeFlash = {
-        error: sandbox.stub(),
-      };
-
       fakeFrameSync = {
         connect: sandbox.spy(),
       };
@@ -123,15 +119,19 @@ describe('sidebar.components.hypothesis-app', function () {
         call: sandbox.stub(),
       };
 
+      fakeToastMessenger = {
+        error: sandbox.stub(),
+      };
+
       $provide.value('store', fakeStore);
       $provide.value('auth', fakeAuth);
       $provide.value('analytics', fakeAnalytics);
       $provide.value('features', fakeFeatures);
-      $provide.value('flash', fakeFlash);
       $provide.value('frameSync', fakeFrameSync);
       $provide.value('serviceUrl', fakeServiceUrl);
       $provide.value('session', fakeSession);
       $provide.value('settings', fakeSettings);
+      $provide.value('toastMessenger', fakeToastMessenger);
       $provide.value('bridge', fakeBridge);
       $provide.value('groups', fakeGroups);
       $provide.value('$window', fakeWindow);
@@ -340,7 +340,7 @@ describe('sidebar.components.hypothesis-app', function () {
       const ctrl = createController();
 
       return ctrl.login().then(null, () => {
-        assert.called(fakeFlash.error);
+        assert.called(fakeToastMessenger.error);
       });
     });
 

src/sidebar/index.js

@@ -34,10 +34,6 @@ disableOpenerForExternalLinks(document.body);
 
 import angular from 'angular';
 
-// Angular addons which export the Angular module name via `module.exports`.
-
-import angularToastr from 'angular-toastr';
-
 // Load polyfill for :focus-visible pseudo-class.
 import 'focus-visible';
 
@@ -60,13 +56,6 @@ const isSidebar = !(
 // Install Preact renderer options to work around IE11 quirks
 rendererOptions.setupIE11Fixes();
 
-// @ngInject
-function configureToastr(toastrConfig) {
-  angular.extend(toastrConfig, {
-    preventOpenDuplicates: true,
-  });
-}
-
 // @ngInject
 function setupApi(api, streamer) {
   api.setClientId(streamer.clientId);
@@ -148,7 +137,6 @@ import apiRoutesService from './services/api-routes';
 import authService from './services/oauth-auth';
 import autosaveService from './services/autosave';
 import featuresService from './services/features';
-import flashService from './services/flash';
 import frameSyncService from './services/frame-sync';
 import groupsService from './services/groups';
 import loadAnnotationsService from './services/load-annotations';
@@ -192,7 +180,6 @@ function startAngularApp(config) {
     .register('autosaveService', autosaveService)
     .register('bridge', bridgeService)
     .register('features', featuresService)
-    .register('flash', flashService)
     .register('frameSync', frameSyncService)
     .register('groups', groupsService)
     .register('loadAnnotationsService', loadAnnotationsService)
@@ -225,10 +212,8 @@ function startAngularApp(config) {
   // constructed them.
   //
   // @ngInject
-  function registerAngularServices($rootScope, toastr) {
-    container
-      .register('toastr', { value: toastr })
-      .register('$rootScope', { value: $rootScope });
+  function registerAngularServices($rootScope) {
+    container.register('$rootScope', { value: $rootScope });
   }
 
   // Run initialization logic that uses constructed services.
@@ -246,7 +231,7 @@ function startAngularApp(config) {
   const wrapComponent = component => wrapReactComponent(component, container);
 
   angular
-    .module('h', [angularToastr])
+    .module('h', [])
 
     // The root component for the application
     .component('hypothesisApp', hypothesisApp)
@@ -277,7 +262,6 @@ function startAngularApp(config) {
     .service('auth', () => container.get('auth'))
     .service('bridge', () => container.get('bridge'))
     .service('features', () => container.get('features'))
-    .service('flash', () => container.get('flash'))
     .service('frameSync', () => container.get('frameSync'))
     .service('groups', () => container.get('groups'))
     .service('loadAnnotationsService', () =>
@@ -289,6 +273,7 @@ function startAngularApp(config) {
     .service('session', () => container.get('session'))
     .service('streamer', () => container.get('streamer'))
     .service('streamFilter', () => container.get('streamFilter'))
+    .service('toastMessenger', () => container.get('toastMessenger'))
 
     // Redux store
     .service('store', () => container.get('store'))
@@ -297,8 +282,6 @@ function startAngularApp(config) {
     .value('isSidebar', container.get('isSidebar'))
     .value('settings', container.get('settings'))
 
-    .config(configureToastr)
-
     // Make Angular built-ins available to services constructed by `container`.
     .run(registerAngularServices)
     .run(initServices);

src/sidebar/services/flash.js

-/**
- * A service for displaying "flash" notification messages.
- */
-
-// @ngInject
-export default function flash(toastr) {
-  return {
-    info: toastr.info.bind(toastr),
-    success: toastr.success.bind(toastr),
-    warning: toastr.warning.bind(toastr),
-    error: toastr.error.bind(toastr),
-  };
-}

src/sidebar/services/oauth-auth.js

@@ -26,9 +26,9 @@ export default function auth(
   $rootScope,
   $window,
   apiRoutes,
-  flash,
   localStorage,
-  settings
+  settings,
+  toastMessenger
 ) {
   /**
    * Authorization code from auth popup window.
@@ -57,10 +57,8 @@ export default function auth(
    * Show an error message telling the user that the access token has expired.
    */
   function showAccessTokenExpiredErrorMessage(message) {
-    flash.error(message, 'Hypothesis login lost', {
-      extendedTimeOut: 0,
-      tapToDismiss: false,
-      timeOut: 0,
+    toastMessenger.error(`Hypothesis login lost: ${message}`, {
+      autoDismiss: false,
     });
   }
 
@@ -306,7 +304,7 @@ auth.$inject = [
   '$rootScope',
   '$window',
   'apiRoutes',
-  'flash',
   'localStorage',
   'settings',
+  'toastMessenger',
 ];

src/sidebar/services/session.js

@@ -26,8 +26,8 @@ export default function session(
   store,
   api,
   auth,
-  flash,
-  settings
+  settings,
+  toastMessenger
 ) {
   // Cache the result of load()
   let lastLoad;
@@ -142,7 +142,7 @@ export default function session(
 
     return loggedOut
       .catch(function (err) {
-        flash.error('Log out failed');
+        toastMessenger.error('Log out failed');
         analytics.track(analytics.events.LOGOUT_FAILURE);
         throw new Error(err);
       })

src/sidebar/services/test/flash-test.js

-import flash from '../flash';
-
-describe('sidebar.flash', () => {
-  ['info', 'success', 'warning', 'error'].forEach(method => {
-    describe(`#${method}`, () => {
-      it(`calls toastr's "${method}" method`, () => {
-        const fakeToastr = {
-          info: sinon.stub(),
-          success: sinon.stub(),
-          warning: sinon.stub(),
-          error: sinon.stub(),
-        };
-
-        const svc = flash(fakeToastr);
-        svc[method]('message', 'title');
-
-        assert.calledWith(fakeToastr[method], 'message', 'title');
-      });
-    });
-  });
-});

src/sidebar/services/test/oauth-auth-test.js

@@ -13,10 +13,10 @@ describe('sidebar.oauth-auth', function () {
   let nowStub;
   let fakeApiRoutes;
   let fakeClient;
-  let fakeFlash;
   let fakeLocalStorage;
   let fakeWindow;
   let fakeSettings;
+  let fakeToastMessenger;
   let clock;
 
   /**
@@ -51,10 +51,6 @@ describe('sidebar.oauth-auth', function () {
       ),
     };
 
-    fakeFlash = {
-      error: sinon.stub(),
-    };
-
     fakeSettings = {
       apiUrl: 'https://hypothes.is/api/',
       oauthClientId: 'the-client-id',
@@ -66,6 +62,10 @@ describe('sidebar.oauth-auth', function () {
       ],
     };
 
+    fakeToastMessenger = {
+      error: sinon.stub(),
+    };
+
     fakeLocalStorage = {
       getObject: sinon.stub().returns(null),
       setObject: sinon.stub(),
@@ -98,9 +98,9 @@ describe('sidebar.oauth-auth', function () {
       .register('$rootScope', { value: $rootScope })
       .register('$window', { value: fakeWindow })
       .register('apiRoutes', { value: fakeApiRoutes })
-      .register('flash', { value: fakeFlash })
       .register('localStorage', { value: fakeLocalStorage })
       .register('settings', { value: fakeSettings })
+      .register('toastMessenger', { value: fakeToastMessenger })
       .register('auth', authFactory)
       .get('auth');
   });
@@ -154,10 +154,13 @@ describe('sidebar.oauth-auth', function () {
 
       it('shows an error message to the user', function () {
         return assertThatAccessTokenPromiseWasRejectedAnd(function () {
-          assert.calledOnce(fakeFlash.error);
-          assert.equal(
-            fakeFlash.error.firstCall.args[0],
-            'You must reload the page to annotate.'
+          assert.calledOnce(fakeToastMessenger.error);
+          assert.calledWith(
+            fakeToastMessenger.error,
+            'Hypothesis login lost: You must reload the page to annotate.',
+            {
+              autoDismiss: false,
+            }
           );
         });
       });

src/sidebar/services/test/session-test.js

@@ -11,11 +11,11 @@ describe('sidebar/services/session', function () {
 
   let fakeAnalytics;
   let fakeAuth;
-  let fakeFlash;
   let fakeSentry;
   let fakeServiceConfig;
   let fakeSettings;
   let fakeStore;
+  let fakeToastMessenger;
   let fakeApi;
   let sandbox;
 
@@ -43,7 +43,6 @@ describe('sidebar/services/session', function () {
       login: sandbox.stub().returns(Promise.resolve()),
       logout: sinon.stub().resolves(),
     };
-    fakeFlash = { error: sandbox.spy() };
     fakeSentry = {
       setUserInfo: sandbox.spy(),
     };
@@ -57,6 +56,7 @@ describe('sidebar/services/session', function () {
     fakeSettings = {
       serviceUrl: 'https://test.hypothes.is/root/',
     };
+    fakeToastMessenger = { error: sandbox.spy() };
 
     $imports.$mock({
       '../service-config': fakeServiceConfig,
@@ -75,9 +75,9 @@ describe('sidebar/services/session', function () {
       .register('store', { value: fakeStore })
       .register('api', { value: fakeApi })
       .register('auth', { value: fakeAuth })
-      .register('flash', { value: fakeFlash })
       .register('settings', { value: fakeSettings })
       .register('session', sessionFactory)
+      .register('toastMessenger', { value: fakeToastMessenger })
       .get('session');
   });
 
@@ -308,6 +308,16 @@ describe('sidebar/services/session', function () {
         assert.calledWith(fakeStore.updateProfile, loggedOutProfile);
       });
     });
+
+    it('displays an error if logging out fails', async () => {
+      fakeAuth.logout.rejects(new Error('Could not revoke token'));
+      try {
+        await session.logout();
+      } catch (e) {
+        // Ignored.
+      }
+      assert.calledWith(fakeToastMessenger.error, 'Log out failed');
+    });
   });
 
   context('when another client changes the current login', () => {

src/sidebar/services/test/toast-messenger-test.js

@@ -100,6 +100,19 @@ describe('toastMessenger', function () {
       assert.calledOnce(fakeStore.getToastMessage);
       assert.notCalled(fakeStore.updateToastMessage);
     });
+
+    it('does not dismiss the message if `autoDismiss` is false', () => {
+      fakeStore.hasToastMessage.returns(false);
+      fakeStore.getToastMessage.returns(undefined);
+
+      service.error('boo', { autoDismiss: false });
+
+      // Move to the first scheduled timeout.
+      clock.next();
+
+      assert.notCalled(fakeStore.getToastMessage);
+      assert.notCalled(fakeStore.updateToastMessage);
+    });
   });
 
   describe('#dismiss', () => {

src/sidebar/services/toast-messenger.js

@@ -10,6 +10,13 @@ const MESSAGE_DISPLAY_TIME = 5000;
 // Delay before removing the message entirely (allows animations to complete)
 const MESSAGE_DISMISS_DELAY = 500;
 
+/**
+ * Additional control over the display of a particular message.
+ *
+ * @typedef {Object} MessageOptions
+ * @prop {boolean} [autoDismiss=true] - Whether the toast message automatically disappears.
+ */
+
 // @ngInject
 export default function toastMessenger(store) {
   /**
@@ -38,8 +45,9 @@ export default function toastMessenger(store) {
    *
    * @param {('error'|'success')} type
    * @param {string} message - The message to be rendered
+   * @param {MessageOptions} [options]
    */
-  const addMessage = (type, message) => {
+  const addMessage = (type, message, { autoDismiss = true } = {}) => {
     // Do not add duplicate messages (messages with the same type and text)
     if (store.hasToastMessage(type, message)) {
       return;
@@ -49,26 +57,34 @@ export default function toastMessenger(store) {
 
     store.addToastMessage({ type, message, id, isDismissed: false });
 
+    if (autoDismiss) {
       // Attempt to dismiss message after a set time period. NB: The message may
       // have been removed by other mechanisms at this point; do not assume its
       // presence.
       setTimeout(() => {
         dismiss(id);
       }, MESSAGE_DISPLAY_TIME);
+    }
   };
 
   /**
    * Add an error toast message with `messageText`
+   *
+   * @param {string} messageText
+   * @param {MessageOptions} options
    */
-  const error = messageText => {
-    addMessage('error', messageText);
+  const error = (messageText, options) => {
+    addMessage('error', messageText, options);
   };
 
   /**
    * Add a success toast message with `messageText`
+   *
+   * @param {string} messageText
+   * @param {MessageOptions} options
    */
-  const success = messageText => {
-    addMessage('success', messageText);
+  const success = (messageText, options) => {
+    addMessage('success', messageText, options);
   };
 
   return {

yarn.lock

@@ -1154,11 +1154,6 @@ angular-mocks@^1.7.5:
   resolved "https://registry.yarnpkg.com/angular-mocks/-/angular-mocks-1.7.9.tgz#0a3b7e28b9a493b4e3010ed2b0f69a68e9b4f79b"
   integrity sha512-LQRqqiV3sZ7NTHBnNmLT0bXtE5e81t97+hkJ56oU0k3dqKv1s6F+nBWRlOVzqHWPGFOiPS8ZJVdrS8DFzHyNIA==
 
-angular-toastr@^2.1.1:
-  version "2.1.1"
-  resolved "https://registry.yarnpkg.com/angular-toastr/-/angular-toastr-2.1.1.tgz#9f8350ca482145a44d011a755b8fb3623d60544c"
-  integrity sha1-n4NQykghRaRNARp1W4+zYj1gVEw=
-
 angular@^1.7.5:
   version "1.7.9"
   resolved "https://registry.yarnpkg.com/angular/-/angular-1.7.9.tgz#e52616e8701c17724c3c238cfe4f9446fd570bc4"