From b172ef2fdc2a12de67bc8228ef156d08b63e9bf6 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Fri, 26 May 2017 15:26:06 +0100 Subject: [PATCH 1/4] Restore notifications to MR widget --- app/assets/javascripts/lib/utils/notify.js | 79 +++++++++---------- app/assets/javascripts/main.js | 1 - .../vue_merge_request_widget/dependencies.js | 1 + .../mr_widget_options.js | 12 +++ .../stores/mr_widget_store.js | 2 + lib/gitlab/gon_helper.rb | 1 + 6 files changed, 53 insertions(+), 43 deletions(-) diff --git a/app/assets/javascripts/lib/utils/notify.js b/app/assets/javascripts/lib/utils/notify.js index 66f39122a66..2aa1bb00093 100644 --- a/app/assets/javascripts/lib/utils/notify.js +++ b/app/assets/javascripts/lib/utils/notify.js @@ -1,47 +1,42 @@ /* eslint-disable func-names, space-before-function-paren, wrap-iife, no-var, one-var, one-var-declaration-per-line, consistent-return, prefer-arrow-callback, no-return-assign, object-shorthand, comma-dangle, no-param-reassign, max-len */ -(function() { - (function(w) { - var notificationGranted, notifyMe, notifyPermissions; - notificationGranted = function(message, opts, onclick) { - var notification; - notification = new Notification(message, opts); - setTimeout(function() { - return notification.close(); - // Hide the notification after X amount of seconds - }, 8000); - if (onclick) { - return notification.onclick = onclick; - } - }; - notifyPermissions = function() { - if ('Notification' in window) { - return Notification.requestPermission(); - } - }; - notifyMe = function(message, body, icon, onclick) { - var opts; - opts = { - body: body, - icon: icon - }; - // Let's check if the browser supports notifications - if (!('Notification' in window)) { +function notificationGranted(message, opts, onclick) { + var notification; + notification = new Notification(message, opts); + setTimeout(function() { + // Hide the notification after X amount of seconds + return notification.close(); + }, 8000); - // do nothing - } else if (Notification.permission === 'granted') { - // If it's okay let's create a notification + return notification.onclick = onclick || notification.close; +} + +function notifyPermissions() { + if ('Notification' in window) { + return Notification.requestPermission(); + } +} + +function notifyMe(message, body, icon, onclick) { + var opts; + opts = { + body: body, + icon: icon + }; + // Let's check if the browser supports notifications + if (!('Notification' in window)) { + // do nothing + } else if (Notification.permission === 'granted') { + // If it's okay let's create a notification + return notificationGranted(message, opts, onclick); + } else if (Notification.permission !== 'denied') { + return Notification.requestPermission(function(permission) { + // If the user accepts, let's create a notification + if (permission === 'granted') { return notificationGranted(message, opts, onclick); - } else if (Notification.permission !== 'denied') { - return Notification.requestPermission(function(permission) { - // If the user accepts, let's create a notification - if (permission === 'granted') { - return notificationGranted(message, opts, onclick); - } - }); } - }; - w.notify = notifyMe; - return w.notifyPermissions = notifyPermissions; - })(window); -}).call(window); + }); + } +} + +export { notifyMe as default, notifyPermissions, notificationGranted }; diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index f0958972130..1ac82b7e291 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -56,7 +56,6 @@ import './lib/utils/animate'; import './lib/utils/bootstrap_linked_tabs'; import './lib/utils/common_utils'; import './lib/utils/datetime_utility'; -import './lib/utils/notify'; import './lib/utils/pretty_time'; import './lib/utils/text_utility'; import './lib/utils/url_utility'; diff --git a/app/assets/javascripts/vue_merge_request_widget/dependencies.js b/app/assets/javascripts/vue_merge_request_widget/dependencies.js index bfe30ee4c08..994e3bff550 100644 --- a/app/assets/javascripts/vue_merge_request_widget/dependencies.js +++ b/app/assets/javascripts/vue_merge_request_widget/dependencies.js @@ -41,3 +41,4 @@ export { default as getStateKey } from './stores/get_state_key'; export { default as mrWidgetOptions } from './mr_widget_options'; export { default as stateMaps } from './stores/state_maps'; export { default as SquashBeforeMerge } from './components/states/mr_widget_squash_before_merge'; +export { default as notifyMe } from '../lib/utils/notify'; diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js index 99600b6664e..0578598c772 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js +++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js @@ -29,6 +29,7 @@ import { eventHub, stateMaps, SquashBeforeMerge, + notifyMe, } from './dependencies'; export default { @@ -77,8 +78,10 @@ export default { this.service.checkStatus() .then(res => res.json()) .then((res) => { + this.handleNotification(res); this.mr.setData(res); this.setFavicon(); + if (cb) { cb.call(null, res); } @@ -136,6 +139,15 @@ export default { new Flash('Something went wrong. Please try again.'); // eslint-disable-line }); }, + handleNotification(data) { + if (data.ci_status === this.mr.ciStatus) return; + + const label = data.pipeline.details.status.label; + const title = `Pipeline ${label}`; + const message = `Pipeline ${label} for "${data.title}"`; + + notifyMe(title, message, this.mr.gitlabLogo); + }, resumePolling() { this.pollingInterval.resume(); }, diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index c07bd25e6fd..5423613ff49 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -5,6 +5,8 @@ export default class MergeRequestStore { constructor(data) { this.sha = data.diff_head_sha; + this.gitlabLogo = gon.gitlab_logo; + this.setData(data); } diff --git a/lib/gitlab/gon_helper.rb b/lib/gitlab/gon_helper.rb index 21f2e6b6970..ab5d9a69b6f 100644 --- a/lib/gitlab/gon_helper.rb +++ b/lib/gitlab/gon_helper.rb @@ -13,6 +13,7 @@ module Gitlab gon.sentry_dsn = current_application_settings.clientside_sentry_dsn if current_application_settings.clientside_sentry_enabled gon.gitlab_url = Gitlab.config.gitlab.url gon.revision = Gitlab::REVISION + gon.gitlab_logo = ActionController::Base.helpers.asset_path('gitlab_logo.png') if current_user gon.current_user_id = current_user.id From 71d03e7e18827e811a3995925de37e080c3e9723 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Fri, 26 May 2017 15:37:09 +0100 Subject: [PATCH 2/4] Move gon ref to the index to avoid depending on gon in any mr widget objects --- app/assets/javascripts/vue_merge_request_widget/index.js | 2 ++ .../vue_merge_request_widget/stores/mr_widget_store.js | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/vue_merge_request_widget/index.js b/app/assets/javascripts/vue_merge_request_widget/index.js index cd65ac069c5..43ef468c303 100644 --- a/app/assets/javascripts/vue_merge_request_widget/index.js +++ b/app/assets/javascripts/vue_merge_request_widget/index.js @@ -4,6 +4,8 @@ import { } from './dependencies'; document.addEventListener('DOMContentLoaded', () => { + gl.mrWidgetData.gitlabLogo = gon.gitlab_logo; + const vm = new Vue(mrWidgetOptions); window.gl.mrWidget = { diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index 5423613ff49..be2f489df1a 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -5,7 +5,7 @@ export default class MergeRequestStore { constructor(data) { this.sha = data.diff_head_sha; - this.gitlabLogo = gon.gitlab_logo; + this.gitlabLogo = data.gitlabLogo; this.setData(data); } From 870005c432be685434e3435f53ebe69371475e62 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Fri, 26 May 2017 16:47:34 +0100 Subject: [PATCH 3/4] Added specs for handleNotifications in mr_widget_options_spec.js --- app/assets/javascripts/lib/utils/notify.js | 8 +++- .../vue_merge_request_widget/dependencies.js | 2 +- .../mr_widget_options.js | 4 +- .../vue_mr_widget/mr_widget_options_spec.js | 37 +++++++++++++++++++ 4 files changed, 47 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/lib/utils/notify.js b/app/assets/javascripts/lib/utils/notify.js index 2aa1bb00093..973d6119158 100644 --- a/app/assets/javascripts/lib/utils/notify.js +++ b/app/assets/javascripts/lib/utils/notify.js @@ -39,4 +39,10 @@ function notifyMe(message, body, icon, onclick) { } } -export { notifyMe as default, notifyPermissions, notificationGranted }; +const notify = { + notificationGranted, + notifyPermissions, + notifyMe, +}; + +export default notify; diff --git a/app/assets/javascripts/vue_merge_request_widget/dependencies.js b/app/assets/javascripts/vue_merge_request_widget/dependencies.js index 994e3bff550..fe5e1bbb55c 100644 --- a/app/assets/javascripts/vue_merge_request_widget/dependencies.js +++ b/app/assets/javascripts/vue_merge_request_widget/dependencies.js @@ -41,4 +41,4 @@ export { default as getStateKey } from './stores/get_state_key'; export { default as mrWidgetOptions } from './mr_widget_options'; export { default as stateMaps } from './stores/state_maps'; export { default as SquashBeforeMerge } from './components/states/mr_widget_squash_before_merge'; -export { default as notifyMe } from '../lib/utils/notify'; +export { default as notify } from '../lib/utils/notify'; diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js index 0578598c772..2339a00ddd0 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js +++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js @@ -29,7 +29,7 @@ import { eventHub, stateMaps, SquashBeforeMerge, - notifyMe, + notify, } from './dependencies'; export default { @@ -146,7 +146,7 @@ export default { const title = `Pipeline ${label}`; const message = `Pipeline ${label} for "${data.title}"`; - notifyMe(title, message, this.mr.gitlabLogo); + notify.notifyMe(title, message, this.mr.gitlabLogo); }, resumePolling() { this.pollingInterval.resume(); diff --git a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js index bdc18243a15..3a0c50b750f 100644 --- a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js +++ b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js @@ -2,6 +2,7 @@ import Vue from 'vue'; import MRWidgetService from '~/vue_merge_request_widget/services/mr_widget_service'; import mrWidgetOptions from '~/vue_merge_request_widget/mr_widget_options'; import eventHub from '~/vue_merge_request_widget/event_hub'; +import notify from '~/lib/utils/notify'; import mockData from './mock_data'; const createComponent = () => { @@ -107,6 +108,8 @@ describe('mrWidgetOptions', () => { it('should tell service to check status', (done) => { spyOn(vm.service, 'checkStatus').and.returnValue(returnPromise(mockData)); spyOn(vm.mr, 'setData'); + spyOn(vm, 'handleNotification'); + let isCbExecuted = false; const cb = () => { isCbExecuted = true; @@ -117,6 +120,7 @@ describe('mrWidgetOptions', () => { setTimeout(() => { expect(vm.service.checkStatus).toHaveBeenCalled(); expect(vm.mr.setData).toHaveBeenCalled(); + expect(vm.handleNotification).toHaveBeenCalledWith(mockData); expect(isCbExecuted).toBeTruthy(); done(); }, 333); @@ -254,6 +258,39 @@ describe('mrWidgetOptions', () => { }); }); + describe('handleNotification', () => { + const data = { + ci_status: 'running', + title: 'title', + pipeline: { details: { status: { label: 'running-label' } } }, + }; + + beforeEach(() => { + spyOn(notify, 'notifyMe'); + + vm.mr.ciStatus = 'failed'; + vm.mr.gitlabLogo = 'logo.png'; + }); + + it('should call notifyMe', () => { + vm.handleNotification(data); + + expect(notify.notifyMe).toHaveBeenCalledWith( + 'Pipeline running-label', + 'Pipeline running-label for "title"', + 'logo.png', + ); + }); + + it('should not call notifyMe if the status has not changed', () => { + vm.mr.ciStatus = data.ci_status; + + vm.handleNotification(data); + + expect(notify.notifyMe).not.toHaveBeenCalled(); + }); + }); + describe('resumePolling', () => { it('should call stopTimer on pollingInterval', () => { spyOn(vm.pollingInterval, 'resume'); From 13dd82b5de6a2076f33defc9f12bc05f98891423 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Fri, 26 May 2017 20:37:29 +0000 Subject: [PATCH 4/4] Add rubocop:disable to gon_helper.rb --- lib/gitlab/gon_helper.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/gitlab/gon_helper.rb b/lib/gitlab/gon_helper.rb index ab5d9a69b6f..319633656ff 100644 --- a/lib/gitlab/gon_helper.rb +++ b/lib/gitlab/gon_helper.rb @@ -1,3 +1,5 @@ +# rubocop:disable Metrics/AbcSize + module Gitlab module GonHelper def add_gon_variables