🐛 Make cmd+s work for all save-buttons (#700)

closes TryGhost/Ghost#8443 - Fixes a bug where the keyboard shortcut `cmd+s` would cause a `Maximum call stack size` error and not save. - Wherever there is a `save` button, the keyboard shortcut to save works now.
TryGhost · May 18, 2017 · 371b9fd · 371b9fd
1 parent ef9381b
commit 371b9fd
Show file tree

Hide file tree

Showing 17 changed files with 229 additions and 18 deletions.
diff --git a/app/controllers/settings/apps/amp.js b/app/controllers/settings/apps/amp.js
@@ -27,6 +27,10 @@ export default Controller.extend({
     actions: {
         update(value) {
             this.set('model', value);
+        },
+
+        save() {
+            this.get('save').perform();
         }
     }
 });
diff --git a/app/controllers/settings/apps/slack.js b/app/controllers/settings/apps/slack.js
@@ -52,7 +52,7 @@ export default Controller.extend({
 
     actions: {
         save() {
-            return this.get('save').perform();
+            this.get('save').perform();
         },
 
         updateURL(value) {

diff --git a/app/controllers/settings/code-injection.js b/app/controllers/settings/code-injection.js
@@ -14,5 +14,11 @@ export default Controller.extend({
             notifications.showAPIError(error, {key: 'code-injection.save'});
             throw error;
         }
-    })
+    }),
+
+    actions: {
+        save() {
+            this.get('save').perform();
+        }
+    }
 });
diff --git a/app/controllers/settings/design.js b/app/controllers/settings/design.js
@@ -79,6 +79,10 @@ export default Controller.extend({
     },
 
     actions: {
+        save() {
+            this.get('save').perform();
+        },
+
         addNavItem() {
             let newNavItem = this.get('newNavItem');
 

diff --git a/app/controllers/settings/general.js b/app/controllers/settings/general.js
@@ -81,6 +81,10 @@ export default Controller.extend({
     }),
 
     actions: {
+        save() {
+            this.get('save').perform();
+        },
+
         setTimezone(timezone) {
             this.set('model.activeTimezone', timezone.name);
         },

diff --git a/app/routes/settings/apps/amp.js b/app/routes/settings/apps/amp.js
@@ -4,5 +4,12 @@ import styleBody from 'ghost-admin/mixins/style-body';
 export default AuthenticatedRoute.extend(styleBody, {
     titleToken: 'Settings - Apps - AMP',
 
-    classNames: ['settings-view-apps-amp']
+    classNames: ['settings-view-apps-amp'],
+
+    actions: {
+        save() {
+            this.get('controller').send('save');
+        }
+    }
+
 });
diff --git a/app/routes/settings/apps/slack.js b/app/routes/settings/apps/slack.js
@@ -4,5 +4,11 @@ import styleBody from 'ghost-admin/mixins/style-body';
 export default AuthenticatedRoute.extend(styleBody, {
     titleToken: 'Settings - Apps - Slack',
 
-    classNames: ['settings-view-apps-slack']
+    classNames: ['settings-view-apps-slack'],
+
+    actions: {
+        save() {
+            this.get('controller').send('save');
+        }
+    }
 });
diff --git a/app/templates/settings/apps/amp.hbs b/app/templates/settings/apps/amp.hbs
@@ -22,7 +22,7 @@
             <div class="form-group for-checkbox">
                 <label for="amp">AMP support for your publications</label>
                 <label class="checkbox" for="amp">
-                    {{one-way-checkbox model id="amp" name="amp" type="checkbox" update=(action "update")}}
+                    {{one-way-checkbox model id="amp" name="amp" type="checkbox" update=(action "update") data-test-amp-checkbox=true}}
                     <span class="input-toggle-component"></span>
                     <p>Enable AMP support</p>
                 </label>

diff --git a/app/templates/settings/apps/slack.hbs b/app/templates/settings/apps/slack.hbs
@@ -22,7 +22,7 @@
         <form class="app-config-form" id="slack-settings" novalidate="novalidate" {{action "save" on="submit"}}>
             {{#gh-form-group errors=model.errors hasValidated=model.hasValidated property="url"}}
                 <label for="url">Webhook URL</label>
-                {{gh-input model.url name="slack[url]" update=(action "updateURL") onenter=(action "save") placeholder="https://hooks.slack.com/services/..."}}
+                {{gh-input model.url name="slack[url]" update=(action "updateURL") onenter=(action "save") placeholder="https://hooks.slack.com/services/..." data-test-slack-url-input=true}}
                 {{#unless model.errors.url}}
                     <p>Set up a new incoming webhook <a href="https://my.slack.com/apps/new/A0F7XDUAZ-incoming-webhooks" target="_blank">here</a>, and grab the URL.</p>
                 {{else}}

diff --git a/app/templates/settings/design.hbs b/app/templates/settings/design.hbs
@@ -2,7 +2,7 @@
     <header class="gh-canvas-header">
         <h2 class="gh-canvas-title">Design</h2>
         <section class="view-actions">
-            {{gh-task-button task=save class="gh-btn gh-btn-blue gh-btn-icon"}}
+            {{gh-task-button task=save class="gh-btn gh-btn-blue gh-btn-icon" data-test-save-button=true}}
         </section>
     </header>
 

diff --git a/app/templates/team/user.hbs b/app/templates/team/user.hbs
@@ -56,7 +56,7 @@
                 </span>
             {{/if}}
 
-            {{gh-task-button class="gh-btn gh-btn-blue gh-btn-icon" task=save}}
+            {{gh-task-button class="gh-btn gh-btn-blue gh-btn-icon" task=save data-test-save-button=true}}
 
             {{#if showDeleteUserModal}}
                 {{gh-fullscreen-modal "delete-user"

diff --git a/tests/acceptance/settings/amp-test.js b/tests/acceptance/settings/amp-test.js
@@ -0,0 +1,98 @@
+/* jshint expr:true */
+import {
+    describe,
+    it,
+    beforeEach,
+    afterEach
+} from 'mocha';
+import {expect} from 'chai';
+import startApp from '../../helpers/start-app';
+import destroyApp from '../../helpers/destroy-app';
+import {invalidateSession, authenticateSession} from 'ghost-admin/tests/helpers/ember-simple-auth';
+import testSelector from 'ember-test-selectors';
+import ctrlOrCmd from 'ghost-admin/utils/ctrl-or-cmd';
+
+describe('Acceptance: Settings - Apps - AMP', function () {
+    let application;
+
+    beforeEach(function() {
+        application = startApp();
+    });
+
+    afterEach(function() {
+        destroyApp(application);
+    });
+
+    it('redirects to signin when not authenticated', async function () {
+        invalidateSession(application);
+        await visit('/settings/apps/amp');
+
+        expect(currentURL(), 'currentURL').to.equal('/signin');
+    });
+
+    it('redirects to team page when authenticated as author', async function () {
+        let role = server.create('role', {name: 'Author'});
+        server.create('user', {roles: [role], slug: 'test-user'});
+
+        authenticateSession(application);
+        await visit('/settings/apps/amp');
+
+        expect(currentURL(), 'currentURL').to.equal('/team/test-user');
+    });
+
+    it('redirects to team page when authenticated as editor', async function () {
+        let role = server.create('role', {name: 'Editor'});
+        server.create('user', {roles: [role], slug: 'test-user'});
+
+        authenticateSession(application);
+        await visit('/settings/apps/amp');
+
+        expect(currentURL(), 'currentURL').to.equal('/team');
+    });
+
+    describe('when logged in', function () {
+        beforeEach(function () {
+            let role = server.create('role', {name: 'Administrator'});
+            server.create('user', {roles: [role]});
+
+            return authenticateSession(application);
+        });
+
+        it('it enables or disables AMP properly and saves it', async function () {
+            await visit('/settings/apps/amp');
+
+            // has correct url
+            expect(currentURL(), 'currentURL').to.equal('/settings/apps/amp');
+
+            // AMP is enabled by default
+            expect(find(testSelector('amp-checkbox')).prop('checked'), 'AMP checkbox').to.be.true;
+
+            await click(testSelector('amp-checkbox'));
+
+            expect(find(testSelector('amp-checkbox')).prop('checked'), 'AMP checkbox').to.be.false;
+
+            await click(testSelector('save-button'));
+
+            let [lastRequest] = server.pretender.handledRequests.slice(-1);
+            let params = JSON.parse(lastRequest.requestBody);
+
+            expect(params.settings.findBy('key', 'amp').value).to.equal(false);
+
+            // CMD-S shortcut works
+            await click(testSelector('amp-checkbox'));
+            await triggerEvent('.gh-app', 'keydown', {
+                keyCode: 83, // s
+                metaKey: ctrlOrCmd === 'command',
+                ctrlKey: ctrlOrCmd === 'ctrl'
+            });
+
+            // we've already saved in this test so there's no on-screen indication
+            // that we've had another save, check the request was fired instead
+            let [newRequest] = server.pretender.handledRequests.slice(-1);
+            params = JSON.parse(newRequest.requestBody);
+
+            expect(find(testSelector('amp-checkbox')).prop('checked'), 'AMP checkbox').to.be.true;
+            expect(params.settings.findBy('key', 'amp').value).to.equal(true);
+        });
+    });
+});
diff --git a/tests/acceptance/settings/code-injection-test.js b/tests/acceptance/settings/code-injection-test.js
@@ -11,6 +11,7 @@ import startApp from '../../helpers/start-app';
 import destroyApp from '../../helpers/destroy-app';
 import {invalidateSession, authenticateSession} from 'ghost-admin/tests/helpers/ember-simple-auth';
 import testSelector from 'ember-test-selectors';
+import ctrlOrCmd from 'ghost-admin/utils/ctrl-or-cmd';
 
 describe('Acceptance: Settings - Code-Injection', function() {
     let application;
@@ -58,7 +59,7 @@ describe('Acceptance: Settings - Code-Injection', function() {
             return authenticateSession(application);
         });
 
-        it('it renders, loads editors correctly', async function () {
+        it('it renders, loads and saves editors correctly', async function () {
             await visit('/settings/code-injection');
 
             // has correct url
@@ -78,6 +79,28 @@ describe('Acceptance: Settings - Code-Injection', function() {
 
             expect(find('#ghost-foot .CodeMirror').length, 'ghost head codemirror element').to.equal(1);
             expect($('#ghost-foot .CodeMirror').hasClass('cm-s-xq-light'), 'ghost head editor theme').to.be.true;
+
+            await click(testSelector('save-button'));
+
+            let [lastRequest] = server.pretender.handledRequests.slice(-1);
+            let params = JSON.parse(lastRequest.requestBody);
+
+            expect(params.settings.findBy('key', 'ghost_head').value).to.equal('');
+            expect(find(testSelector('save-button')).text().trim(), 'save button text').to.equal('Saved');
+
+            // CMD-S shortcut works
+            await triggerEvent('.gh-app', 'keydown', {
+                keyCode: 83, // s
+                metaKey: ctrlOrCmd === 'command',
+                ctrlKey: ctrlOrCmd === 'ctrl'
+            });
+            // we've already saved in this test so there's no on-screen indication
+            // that we've had another save, check the request was fired instead
+            let [newRequest] = server.pretender.handledRequests.slice(-1);
+            params = JSON.parse(newRequest.requestBody);
+
+            expect(params.settings.findBy('key', 'ghost_head').value).to.equal('');
+            expect(find(testSelector('save-button')).text().trim(), 'save button text').to.equal('Saved');
         });
     });
 });
diff --git a/tests/acceptance/settings/design-test.js b/tests/acceptance/settings/design-test.js
@@ -13,6 +13,7 @@ import {invalidateSession, authenticateSession} from 'ghost-admin/tests/helpers/
 import Mirage from 'ember-cli-mirage';
 import mockThemes from 'ghost-admin/mirage/config/themes';
 import testSelector from 'ember-test-selectors';
+import ctrlOrCmd from 'ghost-admin/utils/ctrl-or-cmd';
 
 describe('Acceptance: Settings - Design', function () {
     let application;
@@ -54,6 +55,7 @@ describe('Acceptance: Settings - Design', function () {
             await visit('/settings/design');
 
             expect(currentPath()).to.equal('settings.design.index');
+            expect(find(testSelector('save-button')).text().trim(), 'save button text').to.equal('Save');
 
             // fixtures contain two nav items, check for three rows as we
             // should have one extra that's blank
@@ -69,7 +71,7 @@ describe('Acceptance: Settings - Design', function () {
             await fillIn('.gh-blognav-url:first input', '/test');
             await triggerEvent('.gh-blognav-url:first input', 'blur');
 
-            await click('.gh-btn-blue');
+            await click(testSelector('save-button'));
 
             let [navSetting] = server.db.settings.where({key: 'navigation'});
 
@@ -86,7 +88,7 @@ describe('Acceptance: Settings - Design', function () {
         it('validates new item correctly on save', async function () {
             await visit('/settings/design');
 
-            await click('.gh-btn-blue');
+            await click(testSelector('save-button'));
 
             expect(
                 find('.gh-blognav-item').length,
@@ -97,7 +99,7 @@ describe('Acceptance: Settings - Design', function () {
             await fillIn('.gh-blognav-url:last input', 'http://invalid domain/');
             await triggerEvent('.gh-blognav-url:last input', 'blur');
 
-            await click('.gh-btn-blue');
+            await click(testSelector('save-button'));
 
             expect(
                 find('.gh-blognav-item').length,
@@ -182,7 +184,12 @@ describe('Acceptance: Settings - Design', function () {
                 'number of nav items after successful remove'
             ).to.equal(3);
 
-            await click('.gh-btn-blue');
+            // CMD-S shortcut works
+            await triggerEvent('.gh-app', 'keydown', {
+                keyCode: 83, // s
+                metaKey: ctrlOrCmd === 'command',
+                ctrlKey: ctrlOrCmd === 'ctrl'
+            });
 
             let [navSetting] = server.db.settings.where({key: 'navigation'});
 
@@ -649,6 +656,5 @@ describe('Acceptance: Settings - Design', function () {
             // restore default mirage handlers
             mockThemes(server);
         });
-
     });
 });
diff --git a/tests/acceptance/settings/general-test.js b/tests/acceptance/settings/general-test.js
@@ -11,6 +11,7 @@ import $ from 'jquery';
 import startApp from '../../helpers/start-app';
 import destroyApp from '../../helpers/destroy-app';
 import {invalidateSession, authenticateSession} from 'ghost-admin/tests/helpers/ember-simple-auth';
+import ctrlOrCmd from 'ghost-admin/utils/ctrl-or-cmd';
 
 describe('Acceptance: Settings - General', function () {
     let application;
@@ -105,6 +106,19 @@ describe('Acceptance: Settings - General', function () {
 
             await click(testSelector('modal-accept-button'));
             expect(find('.fullscreen-modal').length).to.equal(0);
+
+            // CMD-S shortcut works
+            await fillIn(testSelector('title-input'), 'CMD-S Test');
+            await triggerEvent('.gh-app', 'keydown', {
+                keyCode: 83, // s
+                metaKey: ctrlOrCmd === 'command',
+                ctrlKey: ctrlOrCmd === 'ctrl'
+            });
+            // we've already saved in this test so there's no on-screen indication
+            // that we've had another save, check the request was fired instead
+            let [lastRequest] = server.pretender.handledRequests.slice(-1);
+            let params = JSON.parse(lastRequest.requestBody);
+            expect(params.settings.findBy('key', 'title').value).to.equal('CMD-S Test');
         });
 
         it('renders timezone selector correctly', async function () {

diff --git a/tests/acceptance/settings/slack-test.js b/tests/acceptance/settings/slack-test.js
@@ -11,6 +11,7 @@ import destroyApp from '../../helpers/destroy-app';
 import Mirage from 'ember-cli-mirage';
 import {invalidateSession, authenticateSession} from 'ghost-admin/tests/helpers/ember-simple-auth';
 import testSelector from 'ember-test-selectors';
+import ctrlOrCmd from 'ghost-admin/utils/ctrl-or-cmd';
 
 describe('Acceptance: Settings - Apps - Slack', function () {
     let application;
@@ -70,6 +71,22 @@ describe('Acceptance: Settings - Apps - Slack', function () {
             expect(find('#slack-settings .error .response').text().trim(), 'inline validation response')
                 .to.equal('The URL must be in a format like https://hooks.slack.com/services/<your personal key>');
 
+            // CMD-S shortcut works
+            await fillIn(testSelector('slack-url-input'), 'https://hooks.slack.com/services/1275958430');
+            await triggerEvent('.gh-app', 'keydown', {
+                keyCode: 83, // s
+                metaKey: ctrlOrCmd === 'command',
+                ctrlKey: ctrlOrCmd === 'ctrl'
+            });
+
+            let [newRequest] = server.pretender.handledRequests.slice(-1);
+            let params = JSON.parse(newRequest.requestBody);
+            let [result] = JSON.parse(params.settings.findBy('key', 'slack').value);
+
+            expect(result.url).to.equal('https://hooks.slack.com/services/1275958430');
+            expect(find('#slack-settings .error .response').text().trim(), 'inline validation response')
+                .to.equal('');
+
             await fillIn('#slack-settings input[name="slack[url]"]', 'https://hooks.slack.com/services/1275958430');
             await click(testSelector('send-notification-button'));
 
@@ -96,6 +113,5 @@ describe('Acceptance: Settings - Apps - Slack', function () {
             expect(lastRequest.url).to.not.match(/\/slack\/test/);
             expect(find('.gh-alert-blue').length, 'check slack alert after api validation error').to.equal(0);
         });
-
     });
 });