Skip to content

Commit

Permalink
🐛 fix Cmd-S save with cursor in slug field
Browse files Browse the repository at this point in the history 
refs TryGhost/Ghost#8551
- renames editor-base-controller.generateSlug to generateSlugFromTitle
- moves `updateSlug` logic from PSM to editor base controller
- moves editor base controller save logic into a `save` task
- adds `updateSlug` and `save` into a `saveTasks` task group so that calls are queued rather than running concurrently
- sets the editor controller on the PSM when accessing the new/edit routes - we use a base controller mixin so the slug/save logic is duplicated on new/edit controllers meaning we need to add a reference manually rather than relying on dependency injection
- remove titleObserver checks for non-existent titleObserver
  • Loading branch information
@kevinansfield
kevinansfield committed Jun 13, 2017
1 parent 0935508 commit 8e340a4
Show file tree
Hide file tree
Showing 4 changed files with 169 additions and 149 deletions.
69 changes: 11 additions & 58 deletions app/controllers/post-settings-menu.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import observer from 'ember-metal/observer';
import {parseDateString} from 'ghost-admin/utils/date-formatting';
import SettingsMenuMixin from 'ghost-admin/mixins/settings-menu-controller';
import boundOneWay from 'ghost-admin/utils/bound-one-way';
import isNumber from 'ghost-admin/utils/isNumber';

const {ArrayProxy, Handlebars, PromiseProxyMixin} = Ember;

Expand All @@ -26,6 +25,11 @@ export default Controller.extend(SettingsMenuMixin, {
slugGenerator: injectService(),
timeZone: injectService(),

// HACK: the edit controller will be different for new/edit so we can't use
// injectController. Instead we set this value in the routes so that we can
// call the editorControllers tasks. Fixed in 1.0 as the PSM is a component
editorController: null,

initializeSelectedAuthor: observer('model', function () {
return this.get('model.author').then((author) => {
this.set('selectedAuthor', author);
Expand Down Expand Up @@ -172,63 +176,12 @@ export default Controller.extend(SettingsMenuMixin, {
* triggered by user manually changing slug
*/
updateSlug(newSlug) {
let slug = this.get('model.slug');

newSlug = newSlug || slug;
newSlug = newSlug && newSlug.trim();

// Ignore unchanged slugs or candidate slugs that are empty
if (!newSlug || slug === newSlug) {
// reset the input to its previous state
this.set('slugValue', slug);

return;
}

this.get('slugGenerator').generateSlug('post', newSlug).then((serverSlug) => {
// If after getting the sanitized and unique slug back from the API
// we end up with a slug that matches the existing slug, abort the change
if (serverSlug === slug) {
return;
}

// Because the server transforms the candidate slug by stripping
// certain characters and appending a number onto the end of slugs
// to enforce uniqueness, there are cases where we can get back a
// candidate slug that is a duplicate of the original except for
// the trailing incrementor (e.g., this-is-a-slug and this-is-a-slug-2)

// get the last token out of the slug candidate and see if it's a number
let slugTokens = serverSlug.split('-');
let check = Number(slugTokens.pop());

// if the candidate slug is the same as the existing slug except
// for the incrementor then the existing slug should be used
if (isNumber(check) && check > 0) {
if (slug === slugTokens.join('-') && serverSlug !== newSlug) {
this.set('slugValue', slug);

return;
}
}

this.set('model.slug', serverSlug);

if (this.hasObserverFor('model.titleScratch')) {
this.removeObserver('model.titleScratch', this, 'titleObserver');
}

// If this is a new post. Don't save the model. Defer the save
// to the user pressing the save button
if (this.get('model.isNew')) {
return;
}

return this.get('model').save();
}).catch((error) => {
this.showError(error);
this.get('model').rollbackAttributes();
});
return this.get('editorController.updateSlug')
.perform(newSlug)
.catch((error) => {
this.showError(error);
this.get('model').rollbackAttributes();
});
},

/**
Expand Down
239 changes: 153 additions & 86 deletions app/mixins/editor-base-controller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@ import run from 'ember-runloop';
import {isEmberArray} from 'ember-array/utils';
import {isBlank} from 'ember-utils';

import {task, timeout} from 'ember-concurrency';
import {task, timeout, taskGroup} from 'ember-concurrency';

import PostModel from 'ghost-admin/models/post';
import boundOneWay from 'ghost-admin/utils/bound-one-way';
import {isVersionMismatchError} from 'ghost-admin/services/ajax';

const {resolve} = RSVP;
import isNumber from 'ghost-admin/utils/isNumber';

// this array will hold properties we need to watch
// to know if the model has been changed (`controller.hasDirtyAttributes`)
Expand Down Expand Up @@ -341,10 +340,10 @@ export default Mixin.create({
// debounce for 700 milliseconds
yield timeout(700);

yield this.get('generateSlug').perform();
yield this.get('generateSlugFromTitle').perform();
}).restartable(),

generateSlug: task(function* () {
generateSlugFromTitle: task(function* () {
let title = this.get('model.titleScratch');

// Only set an "untitled" slug once per post
Expand All @@ -368,113 +367,181 @@ export default Mixin.create({
}
}).enqueue(),

actions: {
cancelTimers() {
let autoSaveId = this._autoSaveId;
let timedSaveId = this._timedSaveId;
// updateSlug and save should always be enqueued so that we don't run into
// problems with concurrency, for example when Cmd-S is pressed whilst the
// cursor is in the slug field - that would previously trigger a simultaneous
// slug update and save resulting in ember data errors and inconsistent save
// results
saveTasks: taskGroup().enqueue(),

if (autoSaveId) {
run.cancel(autoSaveId);
this._autoSaveId = null;
}
updateSlug: task(function* (_newSlug) {
let slug = this.get('model.slug');
let newSlug, serverSlug;

if (timedSaveId) {
run.cancel(timedSaveId);
this._timedSaveId = null;
}
},
newSlug = _newSlug || slug;
newSlug = newSlug && newSlug.trim();

save(options) {
let prevStatus = this.get('model.status');
let isNew = this.get('model.isNew');
let psmController = this.get('postSettingsMenuController');
let promise, status;

options = options || {};

// don't auto-save save if nothing has changed, this prevents
// unnecessary collision errors when reading a post that another
// user is editing
if (options.backgroundSave && !this.get('hasDirtyAttributes')) {
this.send('cancelTimers');
// Ignore unchanged slugs or candidate slugs that are empty
if (!newSlug || slug === newSlug) {
// reset the input to its previous state
this.set('slugValue', slug);

return;
}

serverSlug = yield this.get('slugGenerator').generateSlug('post', newSlug);

// If after getting the sanitized and unique slug back from the API
// we end up with a slug that matches the existing slug, abort the change
if (serverSlug === slug) {
return;
}

// Because the server transforms the candidate slug by stripping
// certain characters and appending a number onto the end of slugs
// to enforce uniqueness, there are cases where we can get back a
// candidate slug that is a duplicate of the original except for
// the trailing incrementor (e.g., this-is-a-slug and this-is-a-slug-2)

// get the last token out of the slug candidate and see if it's a number
let slugTokens = serverSlug.split('-');
let check = Number(slugTokens.pop());

// if the candidate slug is the same as the existing slug except
// for the incrementor then the existing slug should be used
if (isNumber(check) && check > 0) {
if (slug === slugTokens.join('-') && serverSlug !== newSlug) {
this.set('slugValue', slug);
return;
}
}

this.toggleProperty('submitting');
if (options.backgroundSave) {
// do not allow a post's status to be set to published by a background save
status = 'draft';
this.set('model.slug', serverSlug);

// If this is a new post. Don't save the model. Defer the save
// to the user pressing the save button
if (this.get('model.isNew')) {
return;
}

return yield this.get('model').save();
}).group('saveTasks'),

save: task(function* (options) {
let prevStatus = this.get('model.status');
let isNew = this.get('model.isNew');
let psmController = this.get('postSettingsMenuController');
let status;

options = options || {};

// don't auto-save save if nothing has changed, this prevents
// unnecessary collision errors when reading a post that another
// user is editing
if (options.backgroundSave && !this.get('hasDirtyAttributes')) {
this.send('cancelTimers');
return;
}

this.toggleProperty('submitting');
if (options.backgroundSave) {
// do not allow a post's status to be set to published by a background save
status = 'draft';
} else {
if (this.get('scheduledWillPublish')) {
status = (!this.get('willSchedule') && !this.get('willPublish')) ? 'draft' : 'published';
} else {
if (this.get('scheduledWillPublish')) {
status = (!this.get('willSchedule') && !this.get('willPublish')) ? 'draft' : 'published';
if (this.get('willPublish') && !this.get('model.isScheduled') && !this.get('statusFreeze')) {
status = 'published';
} else if (this.get('willSchedule') && !this.get('model.isPublished') && !this.get('statusFreeze')) {
status = 'scheduled';
} else {
if (this.get('willPublish') && !this.get('model.isScheduled') && !this.get('statusFreeze')) {
status = 'published';
} else if (this.get('willSchedule') && !this.get('model.isPublished') && !this.get('statusFreeze')) {
status = 'scheduled';
} else {
status = 'draft';
}
status = 'draft';
}
}
}

this.send('cancelTimers');
this.send('cancelTimers');

// Set the properties that are indirected
// set markdown equal to what's in the editor, minus the image markers.
this.set('model.markdown', this.get('model.scratch'));
this.set('model.status', status);
// Set the properties that are indirected
// set markdown equal to what's in the editor, minus the image markers.
this.set('model.markdown', this.get('model.scratch'));
this.set('model.status', status);

// Set a default title
if (!this.get('model.titleScratch').trim()) {
this.set('model.titleScratch', '(Untitled)');
}
// Set a default title
if (!this.get('model.titleScratch').trim()) {
this.set('model.titleScratch', '(Untitled)');
}

this.set('model.title', this.get('model.titleScratch'));
this.set('model.metaTitle', psmController.get('metaTitleScratch'));
this.set('model.metaDescription', psmController.get('metaDescriptionScratch'));
this.set('model.title', this.get('model.titleScratch'));
this.set('model.metaTitle', psmController.get('metaTitleScratch'));
this.set('model.metaDescription', psmController.get('metaDescriptionScratch'));

try {
if (!this.get('model.slug')) {
this.get('updateTitle').cancelAll();

promise = this.get('generateSlug').perform();
yield this.get('generateSlugFromTitle').perform();
}

return resolve(promise).then(() => {
return this.get('model').save(options).then((model) => {
if (!options.silent) {
this.showSaveNotification(prevStatus, model.get('status'), isNew ? true : false);
}
let model = yield this.get('model').save(options);

this.toggleProperty('submitting');
if (!options.silent) {
this.showSaveNotification(prevStatus, model.get('status'), isNew ? true : false);
}

// reset the helper CP back to false after saving and refetching the new model
// which is published by the scheduler process on the server now
if (this.get('scheduledWillPublish')) {
this.set('scheduledWillPublish', false);
}
return model;
});
}).catch((error) => {
// re-throw if we have a general server error
// TODO: use isValidationError(error) once we have
// ember-ajax/ember-data integration
if (error && error.errors && error.errors[0].errorType !== 'ValidationError') {
this.toggleProperty('submitting');
this.send('error', error);
return;
}
this.toggleProperty('submitting');

if (!options.silent) {
error = error || this.get('model.errors.messages');
this.showErrorAlert(prevStatus, this.get('model.status'), error);
}
// reset the helper CP back to false after saving and refetching the new model
// which is published by the scheduler process on the server now
if (this.get('scheduledWillPublish')) {
this.set('scheduledWillPublish', false);
}

this.set('model.status', prevStatus);
return model;

} catch (_error) {
let error = _error;

// re-throw if we have a general server error
// TODO: use isValidationError(error) once we have
// ember-ajax/ember-data integration
if (error && error.errors && error.errors[0].errorType !== 'ValidationError') {
this.toggleProperty('submitting');
return this.get('model');
});
this.send('error', error);
return;
}

if (!options.silent) {
error = error || this.get('model.errors.messages');
this.showErrorAlert(prevStatus, this.get('model.status'), error);
}

this.set('model.status', prevStatus);

this.toggleProperty('submitting');
return this.get('model');
}
}).group('saveTasks'),

actions: {
cancelTimers() {
let autoSaveId = this._autoSaveId;
let timedSaveId = this._timedSaveId;

if (autoSaveId) {
run.cancel(autoSaveId);
this._autoSaveId = null;
}

if (timedSaveId) {
run.cancel(timedSaveId);
this._timedSaveId = null;
}
},

save(options) {
this.get('save').perform(options);
},

setSaveType(newType) {
Expand Down
3 changes: 3 additions & 0 deletions app/routes/editor/edit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ export default AuthenticatedRoute.extend(base, {
this._super(...arguments);

controller.set('shouldFocusEditor', this.get('_transitionedFromNew'));

let psm = this.controllerFor('post-settings-menu');
psm.set('editorController', controller);
},

actions: {
Expand Down
Loading

0 comments on commit 8e340a4

Please sign in to comment.