From c77c1507454b9bfd181983811ddc3701c4235c7f Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Tue, 4 Oct 2022 17:32:49 +0100 Subject: [PATCH] Refactored facebook/twitter URL inputs refs https://github.com/TryGhost/Ghost/issues/14101 Twitter/facebook URL validation doesn't follow our typical validation and was duplicated across multiple screens making the controllers unnecessarily complex. - extracted url input fields and their validation into separate components - uses tracked scratch values so that the input field values can reset to the saved value on save - twitter/facebook URL inputs are different to our other inputs because invalid values won't prevent saving, instead they are reset to their previous value on save - added `this.validate()` call after a successful save in `settings` service so the service and underlying model validations are both in sync (fixes validation error sticking around after saving with invalid twitter/facebook values that have been reset) --- ghost/admin/.lint-todo | 14 ++ .../app/components/gh-facebook-url-input.hbs | 10 ++ .../app/components/gh-facebook-url-input.js | 68 +++++++++ .../app/components/gh-twitter-url-input.hbs | 10 ++ .../app/components/gh-twitter-url-input.js | 66 +++++++++ .../admin/app/controllers/settings/general.js | 133 +++-------------- .../app/controllers/settings/staff/user.js | 136 +++--------------- ghost/admin/app/services/settings.js | 1 + .../admin/app/templates/settings/general.hbs | 32 ++--- .../app/templates/settings/staff/user.hbs | 36 ++--- 10 files changed, 234 insertions(+), 272 deletions(-) create mode 100644 ghost/admin/app/components/gh-facebook-url-input.hbs create mode 100644 ghost/admin/app/components/gh-facebook-url-input.js create mode 100644 ghost/admin/app/components/gh-twitter-url-input.hbs create mode 100644 ghost/admin/app/components/gh-twitter-url-input.js diff --git a/ghost/admin/.lint-todo b/ghost/admin/.lint-todo index 1d61999165..7405cca33f 100644 --- a/ghost/admin/.lint-todo +++ b/ghost/admin/.lint-todo @@ -887,3 +887,17 @@ remove|ember-template-lint|no-action|125|88|125|88|ea7c6e414cc202b6e4049eae4f60e remove|ember-template-lint|no-action|129|35|129|35|ea7c6e414cc202b6e4049eae4f60eb0829bd3ff0|1662681600000|1673053200000|1678237200000|app/templates/settings/staff/user.hbs remove|ember-template-lint|no-action|136|42|136|42|7a2a7a1b2159d811b310a9b89817108ddff9df88|1662681600000|1673053200000|1678237200000|app/templates/settings/staff/user.hbs remove|ember-template-lint|no-action|140|35|140|35|7a2a7a1b2159d811b310a9b89817108ddff9df88|1662681600000|1673053200000|1678237200000|app/templates/settings/staff/user.hbs +remove|ember-template-lint|no-action|361|43|361|43|1f32c13702a8b46410e9e10f9988fa2e0df41968|1662681600000|1673053200000|1678237200000|app/templates/settings/general.hbs +remove|ember-template-lint|no-action|362|47|362|47|69b8d823159491abd67494eba728616ac0a928a5|1662681600000|1673053200000|1678237200000|app/templates/settings/general.hbs +remove|ember-template-lint|no-action|374|43|374|43|226e659be4c2c10c0e5fbebb40f712f3016c9b90|1662681600000|1673053200000|1678237200000|app/templates/settings/general.hbs +remove|ember-template-lint|no-action|375|47|375|47|d746c5532ce38d23d8537195a3afa6ce7235f577|1662681600000|1673053200000|1678237200000|app/templates/settings/general.hbs +remove|ember-template-lint|no-passed-in-event-handlers|361|36|361|36|c14e20b2b090706f9af9fa416c4300c06e206894|1662681600000|1673053200000|1678237200000|app/templates/settings/general.hbs +remove|ember-template-lint|no-passed-in-event-handlers|374|36|374|36|e286f50c92a17a31581207e4c65054f146cbd533|1662681600000|1673053200000|1678237200000|app/templates/settings/general.hbs +remove|ember-template-lint|no-action|122|82|122|82|e2e24a0e1efe19ed982cc8140248570c03eee7b9|1662681600000|1673053200000|1678237200000|app/templates/settings/staff/user.hbs +remove|ember-template-lint|no-action|262|39|262|39|1f32c13702a8b46410e9e10f9988fa2e0df41968|1662681600000|1673053200000|1678237200000|app/templates/settings/staff/user.hbs +remove|ember-template-lint|no-action|263|43|263|43|69b8d823159491abd67494eba728616ac0a928a5|1662681600000|1673053200000|1678237200000|app/templates/settings/staff/user.hbs +remove|ember-template-lint|no-action|279|39|279|39|226e659be4c2c10c0e5fbebb40f712f3016c9b90|1662681600000|1673053200000|1678237200000|app/templates/settings/staff/user.hbs +remove|ember-template-lint|no-action|280|43|280|43|d746c5532ce38d23d8537195a3afa6ce7235f577|1662681600000|1673053200000|1678237200000|app/templates/settings/staff/user.hbs +remove|ember-template-lint|no-action|398|106|398|106|d2028f094665a3702bbea21159b1c12237c94bc9|1662681600000|1673053200000|1678237200000|app/templates/settings/staff/user.hbs +remove|ember-template-lint|no-passed-in-event-handlers|262|32|262|32|c14e20b2b090706f9af9fa416c4300c06e206894|1662681600000|1673053200000|1678237200000|app/templates/settings/staff/user.hbs +remove|ember-template-lint|no-passed-in-event-handlers|279|32|279|32|e286f50c92a17a31581207e4c65054f146cbd533|1662681600000|1673053200000|1678237200000|app/templates/settings/staff/user.hbs diff --git a/ghost/admin/app/components/gh-facebook-url-input.hbs b/ghost/admin/app/components/gh-facebook-url-input.hbs new file mode 100644 index 0000000000..1765c5298d --- /dev/null +++ b/ghost/admin/app/components/gh-facebook-url-input.hbs @@ -0,0 +1,10 @@ + diff --git a/ghost/admin/app/components/gh-facebook-url-input.js b/ghost/admin/app/components/gh-facebook-url-input.js new file mode 100644 index 0000000000..8f1676239d --- /dev/null +++ b/ghost/admin/app/components/gh-facebook-url-input.js @@ -0,0 +1,68 @@ +import Component from '@glimmer/component'; +import validator from 'validator'; +import {action} from '@ember/object'; +import {get, set} from '@ember/object'; + +export default class GhFacebookUrlInput extends Component { + // NOTE: `get` and `set` are required when reading/writing model properties + // because we can be dealing with proxy objects such as the settings service + get value() { + const {model, modelProperty, scratchValue} = this.args; + return scratchValue || get(model, modelProperty); + } + + @action + setScratchValue(event) { + this.args.setScratchValue?.(event.target.value); + } + + @action + setFacebookUrl(event) { + const {model, modelProperty} = this.args; + + let newUrl = event.target.value; + + // reset errors and validation + model.errors.remove('facebook'); + model.hasValidated.removeObject('facebook'); + + if (!newUrl) { + // Clear out the Facebook url + set(model, modelProperty, null); + this.args.setScratchValue?.(null); + return; + } + + try { + // strip any facebook URLs out + newUrl = newUrl.replace(/(https?:\/\/)?(www\.)?facebook\.com/i, ''); + + // don't allow any non-facebook urls + if (newUrl.match(/^(http|\/\/)/i)) { + throw 'invalid url'; + } + + // strip leading / if we have one then concat to full facebook URL + newUrl = newUrl.replace(/^\//, ''); + newUrl = `https://www.facebook.com/${newUrl}`; + + // don't allow URL if it's not valid + if (!validator.isURL(newUrl)) { + throw 'invalid url'; + } + + set(model, modelProperty, newUrl); + this.args.setScratchValue?.(null); + } catch (e) { + if (e === 'invalid url') { + const message = 'The URL must be in a format like https://www.facebook.com/yourPage'; + model.errors.add('facebook', message); + return; + } + + throw e; + } finally { + model.hasValidated.pushObject('facebook'); + } + } +} diff --git a/ghost/admin/app/components/gh-twitter-url-input.hbs b/ghost/admin/app/components/gh-twitter-url-input.hbs new file mode 100644 index 0000000000..22635bfafc --- /dev/null +++ b/ghost/admin/app/components/gh-twitter-url-input.hbs @@ -0,0 +1,10 @@ + \ No newline at end of file diff --git a/ghost/admin/app/components/gh-twitter-url-input.js b/ghost/admin/app/components/gh-twitter-url-input.js new file mode 100644 index 0000000000..bb64540773 --- /dev/null +++ b/ghost/admin/app/components/gh-twitter-url-input.js @@ -0,0 +1,66 @@ +import Component from '@glimmer/component'; +import {action} from '@ember/object'; +import {get, set} from '@ember/object'; + +export default class GhTwitterUrlInput extends Component { + // NOTE: `get` and `set` are required when reading/writing model properties + // because we can be dealing with proxy objects such as the settings service + get value() { + const {model, modelProperty, scratchValue} = this.args; + return scratchValue || get(model, modelProperty); + } + + @action + setScratchValue(event) { + this.args.setScratchValue?.(event.target.value); + } + + @action + setTwitterUrl(event) { + const {model, modelProperty} = this.args; + + const newUrl = event.target.value; + + // reset errors and validation + model.errors.remove(modelProperty); + model.hasValidated.removeObject(modelProperty); + + if (!newUrl) { + // Clear out the Twitter url + set(model, modelProperty, ''); + this.args.setScratchValue?.(null); + return; + } + + if (newUrl.match(/(?:twitter\.com\/)(\S+)/) || newUrl.match(/([a-z\d.]+)/i)) { + let username = []; + + if (newUrl.match(/(?:twitter\.com\/)(\S+)/)) { + [, username] = newUrl.match(/(?:twitter\.com\/)(\S+)/); + } else { + [username] = newUrl.match(/([^/]+)\/?$/mi); + } + + // check if username starts with http or www and show error if so + if (username.match(/^(http|www)|(\/)/) || !username.match(/^[a-z\d._]{1,15}$/mi)) { + const message = !username.match(/^[a-z\d._]{1,15}$/mi) + ? 'Your Username is not a valid Twitter Username' + : 'The URL must be in a format like https://twitter.com/yourUsername'; + + model.errors.add(modelProperty, message); + model.hasValidated.pushObject(modelProperty); + return; + } + + set(model, modelProperty, `https://twitter.com/${username}`); + this.args.setScratchValue?.(null); + + model.hasValidated.pushObject(modelProperty); + } else { + const message = 'The URL must be in a format like https://twitter.com/yourUsername'; + model.errors.add(modelProperty, message); + model.hasValidated.pushObject(modelProperty); + return; + } + } +} diff --git a/ghost/admin/app/controllers/settings/general.js b/ghost/admin/app/controllers/settings/general.js index 7149b0a331..9e1fe7da3a 100644 --- a/ghost/admin/app/controllers/settings/general.js +++ b/ghost/admin/app/controllers/settings/general.js @@ -4,12 +4,13 @@ import {inject as service} from '@ember/service'; /* eslint-disable ghost/ember/alias-model-in-controller */ import Controller from '@ember/controller'; import generatePassword from 'ghost-admin/utils/password-generator'; -import validator from 'validator'; import { IMAGE_EXTENSIONS, IMAGE_MIME_TYPES } from 'ghost-admin/components/gh-image-uploader'; +import {TrackedObject} from 'tracked-built-ins'; import {task} from 'ember-concurrency'; +import {tracked} from '@glimmer/tracking'; function randomPassword() { let word = generatePassword(6); @@ -28,11 +29,11 @@ export default class GeneralController extends Controller { @service frontend; @service ui; + @tracked scratchValues = new TrackedObject(); + availableTimezones = null; imageExtensions = IMAGE_EXTENSIONS; imageMimeTypes = IMAGE_MIME_TYPES; - _scratchFacebook = null; - _scratchTwitter = null; @computed('config.blogUrl', 'settings.publicHash') get privateRSSUrl() { @@ -102,111 +103,12 @@ export default class GeneralController extends Controller { } @action - validateFacebookUrl() { - let newUrl = this._scratchFacebook; - let oldUrl = this.get('settings.facebook'); - let errMessage = ''; - - // reset errors and validation - this.get('settings.errors').remove('facebook'); - this.get('settings.hasValidated').removeObject('facebook'); - - if (newUrl === '') { - // Clear out the Facebook url - this.set('settings.facebook', ''); - return; - } - - // _scratchFacebook will be null unless the user has input something - if (!newUrl) { - newUrl = oldUrl; - } - - try { - // strip any facebook URLs out - newUrl = newUrl.replace(/(https?:\/\/)?(www\.)?facebook\.com/i, ''); - - // don't allow any non-facebook urls - if (newUrl.match(/^(http|\/\/)/i)) { - throw 'invalid url'; - } - - // strip leading / if we have one then concat to full facebook URL - newUrl = newUrl.replace(/^\//, ''); - newUrl = `https://www.facebook.com/${newUrl}`; - - // don't allow URL if it's not valid - if (!validator.isURL(newUrl)) { - throw 'invalid url'; - } - - this.settings.set('facebook', newUrl); - this.settings.notifyPropertyChange('facebook'); - } catch (e) { - if (e === 'invalid url') { - errMessage = 'The URL must be in a format like ' - + 'https://www.facebook.com/yourPage'; - this.get('settings.errors').add('facebook', errMessage); - return; - } - - throw e; - } finally { - this.get('settings.hasValidated').pushObject('facebook'); - } + setScratchValue(property, value) { + this.scratchValues[property] = value; } - @action - validateTwitterUrl() { - let newUrl = this._scratchTwitter; - let oldUrl = this.get('settings.twitter'); - let errMessage = ''; - - // reset errors and validation - this.get('settings.errors').remove('twitter'); - this.get('settings.hasValidated').removeObject('twitter'); - - if (newUrl === '') { - // Clear out the Twitter url - this.set('settings.twitter', ''); - return; - } - - // _scratchTwitter will be null unless the user has input something - if (!newUrl) { - newUrl = oldUrl; - } - - if (newUrl.match(/(?:twitter\.com\/)(\S+)/) || newUrl.match(/([a-z\d.]+)/i)) { - let username = []; - - if (newUrl.match(/(?:twitter\.com\/)(\S+)/)) { - [, username] = newUrl.match(/(?:twitter\.com\/)(\S+)/); - } else { - [username] = newUrl.match(/([^/]+)\/?$/mi); - } - - // check if username starts with http or www and show error if so - if (username.match(/^(http|www)|(\/)/) || !username.match(/^[a-z\d._]{1,15}$/mi)) { - errMessage = !username.match(/^[a-z\d._]{1,15}$/mi) ? 'Your Username is not a valid Twitter Username' : 'The URL must be in a format like https://twitter.com/yourUsername'; - - this.get('settings.errors').add('twitter', errMessage); - this.get('settings.hasValidated').pushObject('twitter'); - return; - } - - newUrl = `https://twitter.com/${username}`; - - this.settings.get('hasValidated').pushObject('twitter'); - this.settings.set('twitter', newUrl); - this.settings.notifyPropertyChange('twitter'); - } else { - errMessage = 'The URL must be in a format like ' - + 'https://twitter.com/yourUsername'; - this.get('settings.errors').add('twitter', errMessage); - this.get('settings.hasValidated').pushObject('twitter'); - return; - } + clearScratchValues() { + this.scratchValues = new TrackedObject(); } _deleteTheme() { @@ -221,25 +123,23 @@ export default class GeneralController extends Controller { }); } - @task(function* () { + @task + *saveTask() { let notifications = this.notifications; let config = this.config; - if (this.settings.get('twitter') !== this._scratchTwitter) { - this.send('validateTwitterUrl'); - } - - if (this.settings.get('facebook') !== this._scratchFacebook) { - this.send('validateFacebookUrl'); - } - try { let changedAttrs = this.settings.changedAttributes(); let settings = yield this.settings.save(); + + this.clearScratchValues(); + config.set('blogTitle', settings.get('title')); + if (changedAttrs.password) { this.frontend.loginIfNeeded(); } + // this forces the document title to recompute after a blog title change this.ui.updateDocumentTitle(); @@ -250,6 +150,5 @@ export default class GeneralController extends Controller { } throw error; } - }) - saveTask; + } } diff --git a/ghost/admin/app/controllers/settings/staff/user.js b/ghost/admin/app/controllers/settings/staff/user.js index b2a94bba8e..e01ed5f65c 100644 --- a/ghost/admin/app/controllers/settings/staff/user.js +++ b/ghost/admin/app/controllers/settings/staff/user.js @@ -9,13 +9,14 @@ import UploadImageModal from '../../../components/settings/staff/modals/upload-i import boundOneWay from 'ghost-admin/utils/bound-one-way'; import copyTextToClipboard from 'ghost-admin/utils/copy-text-to-clipboard'; import isNumber from 'ghost-admin/utils/isNumber'; -import validator from 'validator'; import windowProxy from 'ghost-admin/utils/window-proxy'; +import {TrackedObject} from 'tracked-built-ins'; import {action, computed} from '@ember/object'; import {alias, and, not, or, readOnly} from '@ember/object/computed'; import {run} from '@ember/runloop'; import {inject as service} from '@ember/service'; import {task, taskGroup, timeout} from 'ember-concurrency'; +import {tracked} from '@glimmer/tracking'; export default Controller.extend({ ajax: service(), @@ -31,8 +32,13 @@ export default Controller.extend({ personalToken: null, personalTokenRegenerated: false, dirtyAttributes: false, - _scratchFacebook: null, - _scratchTwitter: null, + + init() { + this._super(...arguments); + this.clearScratchValues(); + }, + + scratchValues: tracked(), saveHandlers: taskGroup().enqueue(), @@ -85,117 +91,6 @@ export default Controller.extend({ }), actions: { - validateFacebookUrl() { - let newUrl = this._scratchFacebook; - let oldUrl = this.get('user.facebook'); - let errMessage = ''; - - // reset errors and validation - this.get('user.errors').remove('facebook'); - this.get('user.hasValidated').removeObject('facebook'); - - if (newUrl === '') { - // Clear out the Facebook url - this.set('user.facebook', ''); - return; - } - - // _scratchFacebook will be null unless the user has input something - if (!newUrl) { - newUrl = oldUrl; - } - - try { - // strip any facebook URLs out - newUrl = newUrl.replace(/(https?:\/\/)?(www\.)?facebook\.com/i, ''); - - // don't allow any non-facebook urls - if (newUrl.match(/^(http|\/\/)/i)) { - throw 'invalid url'; - } - - // strip leading / if we have one then concat to full facebook URL - newUrl = newUrl.replace(/^\//, ''); - newUrl = `https://www.facebook.com/${newUrl}`; - - // don't allow URL if it's not valid - if (!validator.isURL(newUrl)) { - throw 'invalid url'; - } - - this.set('user.facebook', ''); - run.schedule('afterRender', this, function () { - this.set('user.facebook', newUrl); - }); - } catch (e) { - if (e === 'invalid url') { - errMessage = 'The URL must be in a format like ' - + 'https://www.facebook.com/yourPage'; - this.get('user.errors').add('facebook', errMessage); - return; - } - - throw e; - } finally { - this.get('user.hasValidated').pushObject('facebook'); - } - }, - - validateTwitterUrl() { - let newUrl = this._scratchTwitter; - let oldUrl = this.get('user.twitter'); - let errMessage = ''; - - // reset errors and validation - this.get('user.errors').remove('twitter'); - this.get('user.hasValidated').removeObject('twitter'); - - if (newUrl === '') { - // Clear out the Twitter url - this.set('user.twitter', ''); - return; - } - - // _scratchTwitter will be null unless the user has input something - if (!newUrl) { - newUrl = oldUrl; - } - - if (newUrl.match(/(?:twitter\.com\/)(\S+)/) || newUrl.match(/([a-z\d.]+)/i)) { - let username = []; - - if (newUrl.match(/(?:twitter\.com\/)(\S+)/)) { - [, username] = newUrl.match(/(?:twitter\.com\/)(\S+)/); - } else { - [username] = newUrl.match(/([^/]+)\/?$/mi); - } - - // check if username starts with http or www and show error if so - if (username.match(/^(http|www)|(\/)/) || !username.match(/^[a-z\d._]{1,15}$/mi)) { - errMessage = !username.match(/^[a-z\d._]{1,15}$/mi) ? 'Your Username is not a valid Twitter Username' : 'The URL must be in a format like https://twitter.com/yourUsername'; - - this.get('user.errors').add('twitter', errMessage); - this.get('user.hasValidated').pushObject('twitter'); - return; - } - - newUrl = `https://twitter.com/${username}`; - - this.get('user.hasValidated').pushObject('twitter'); - - this.set('user.twitter', ''); - run.schedule('afterRender', this, function () { - this.set('user.twitter', newUrl); - }); - } else { - errMessage = 'The URL must be in a format like ' - + 'https://twitter.com/yourUsername'; - this.get('user.errors').add('twitter', errMessage); - this.get('user.hasValidated').pushObject('twitter'); - return; - } - }, - // TODO: remove those mutation actions once we have better // inline validations that auto-clear errors on input updatePassword(password) { @@ -285,6 +180,14 @@ export default Controller.extend({ }); }), + setScratchValue: action(function (property, value) { + this.scratchValues[property] = value; + }), + + clearScratchValues() { + this.scratchValues = new TrackedObject(); + }, + reset: action(function () { this.user.rollbackAttributes(); this.user.password = ''; @@ -292,6 +195,7 @@ export default Controller.extend({ this.user.ne2Password = ''; this.set('slugValue', this.user.slug); this.set('dirtyAttributes', false); + this.clearScratchValues(); }), toggleCommentNotifications: action(function (event) { @@ -366,7 +270,9 @@ export default Controller.extend({ } try { - user = yield user.save({format: false}); + user = yield user.save(); + + this.clearScratchValues(); // If the user's slug has changed, change the URL and replace // the history so refresh and back button still work diff --git a/ghost/admin/app/services/settings.js b/ghost/admin/app/services/settings.js index 9e12f5481e..002b030575 100644 --- a/ghost/admin/app/services/settings.js +++ b/ghost/admin/app/services/settings.js @@ -63,6 +63,7 @@ export default class SettingsService extends Service.extend(_ProxyMixin, Validat } await settings.save(); + await this.validate(); this.set('settledIcon', settings.icon); return settings; } diff --git a/ghost/admin/app/templates/settings/general.hbs b/ghost/admin/app/templates/settings/general.hbs index c90d542efb..723ef69a6a 100644 --- a/ghost/admin/app/templates/settings/general.hbs +++ b/ghost/admin/app/templates/settings/general.hbs @@ -359,30 +359,26 @@ {{#liquid-if this.socialOpen}}
- -

URL of your publication's Facebook Page

+

URL of your publication's Facebook Page

 - -

URL of your publication's Twitter profile

+

URL of your publication's Twitter profile
{{/liquid-if}} diff --git a/ghost/admin/app/templates/settings/staff/user.hbs b/ghost/admin/app/templates/settings/staff/user.hbs index fdb60f9569..99d36b0248 100644 --- a/ghost/admin/app/templates/settings/staff/user.hbs +++ b/ghost/admin/app/templates/settings/staff/user.hbs @@ -85,7 +85,7 @@ {{!--
--}}
-