From cd5d87ddf2c67beb6d8031adebc2dfe904bbd33e Mon Sep 17 00:00:00 2001 From: cobbspur Date: Wed, 10 Dec 2014 14:03:39 +0000 Subject: [PATCH] Return computed url with post object closes #4445 - post model gets permalink format - post model queries urlPathForPost to return computed url - url helper modified to use post url - urlForPost method abolished and replaced where necessary - updated tests --- core/server/api/index.js | 6 +- core/server/config/index.js | 2 +- core/server/config/url.js | 27 +--- core/server/helpers/url.js | 3 +- core/server/models/post.js | 36 ++++- core/server/xmlrpc.js | 73 +++++---- core/test/integration/api/api_users_spec.js | 24 +-- .../integration/model/model_posts_spec.js | 53 ++++++- core/test/unit/config_spec.js | 138 ++---------------- core/test/unit/server_helpers/url_spec.js | 5 +- core/test/unit/xmlrpc_spec.js | 19 +-- core/test/utils/api.js | 2 +- 12 files changed, 165 insertions(+), 223 deletions(-) diff --git a/core/server/api/index.js b/core/server/api/index.js index 1fb6c6ddfe..21a1438735 100644 --- a/core/server/api/index.js +++ b/core/server/api/index.js @@ -82,10 +82,8 @@ cacheInvalidationHeader = function (req, result) { // Don't set x-cache-invalidate header for drafts if (hasStatusChanged || wasDeleted || wasPublishedUpdated) { cacheInvalidate = '/, /page/*, /rss/, /rss/*, /tag/*, /author/*, /sitemap-*.xml'; - if (id && post.slug) { - return config.urlForPost(settings, post).then(function (postUrl) { - return cacheInvalidate + ', ' + postUrl; - }); + if (id && post.slug && post.url) { + cacheInvalidate += ', ' + post.url; } } } diff --git a/core/server/config/index.js b/core/server/config/index.js index 805a224134..5f721f6d36 100644 --- a/core/server/config/index.js +++ b/core/server/config/index.js @@ -31,7 +31,7 @@ function ConfigManager(config) { // Allow other modules to be externally accessible. this.urlFor = configUrl.urlFor; - this.urlForPost = configUrl.urlForPost; + this.urlPathForPost = configUrl.urlPathForPost; // If we're given an initial config object then we can set it. if (config && _.isObject(config)) { diff --git a/core/server/config/url.js b/core/server/config/url.js index 6e11583bfb..0d78b58660 100644 --- a/core/server/config/url.js +++ b/core/server/config/url.js @@ -53,7 +53,7 @@ function createUrl(urlPath, absolute, secure) { // Creates the url path for a post, given a post and a permalink // Parameters: // - post - a json object representing a post -// - permalinks - a json object containing the permalinks setting +// - permalinks - a string containing the permalinks setting function urlPathForPost(post, permalinks) { var output = '', tags = { @@ -68,7 +68,7 @@ function urlPathForPost(post, permalinks) { if (post.page) { output += '/:slug/'; } else { - output += permalinks.value; + output += permalinks; } // replace tags like :slug or :year with actual values @@ -124,9 +124,9 @@ function urlFor(context, data, absolute) { urlPath = context.relativeUrl; } else if (_.isString(context) && _.indexOf(knownObjects, context) !== -1) { // trying to create a url for an object - if (context === 'post' && data.post && data.permalinks) { - urlPath = urlPathForPost(data.post, data.permalinks); - secure = data.post.secure; + if (context === 'post' && data.post) { + urlPath = data.post.url; + secure = data.secure; } else if (context === 'tag' && data.tag) { urlPath = '/tag/' + data.tag.slug + '/'; secure = data.tag.secure; @@ -161,21 +161,6 @@ function urlFor(context, data, absolute) { return createUrl(urlPath, absolute, secure); } -// ## urlForPost -// This method is async as we have to fetch the permalinks -// Get the permalink setting and then get a URL for the given post -// Parameters -// - settings - passed reference to api.settings -// - post - a json object representing a post -// - absolute (optional, default:false) - boolean whether or not the url should be absolute -function urlForPost(settings, post, absolute) { - return settings.read('permalinks').then(function (response) { - var permalinks = response.settings[0]; - - return urlFor('post', {post: post, permalinks: permalinks}, absolute); - }); -} - module.exports.setConfig = setConfig; module.exports.urlFor = urlFor; -module.exports.urlForPost = urlForPost; +module.exports.urlPathForPost = urlPathForPost; diff --git a/core/server/helpers/url.js b/core/server/helpers/url.js index 788769ebd3..ee291e2844 100644 --- a/core/server/helpers/url.js +++ b/core/server/helpers/url.js @@ -6,7 +6,6 @@ var Promise = require('bluebird'), config = require('../config'), - api = require('../api'), schema = require('../data/schema').checks, url; @@ -14,7 +13,7 @@ url = function (options) { var absolute = options && options.hash.absolute; if (schema.isPost(this)) { - return config.urlForPost(api.settings, this, absolute); + return Promise.resolve(config.urlFor('post', {post: this}, absolute)); } if (schema.isTag(this)) { diff --git a/core/server/models/post.js b/core/server/models/post.js index b8bbc40e97..8813226207 100644 --- a/core/server/models/post.js +++ b/core/server/models/post.js @@ -12,9 +12,29 @@ var _ = require('lodash'), xmlrpc = require('../xmlrpc'), sitemap = require('../data/sitemap'), + config = require('../config'), + permalinkSetting = '', + getPermalinkSetting, Post, Posts; +// Stores model permalink format + +getPermalinkSetting = function (model, attributes, options) { + /*jshint unused:false*/ + + // Transactions are used for bulk deletes and imports which don't need this anyway + if (options.transacting) { + return Promise.resolve(); + } + return ghostBookshelf.model('Settings').findOne({key: 'permalinks'}).then(function (response) { + if (response) { + response = response.toJSON(); + permalinkSetting = response.hasOwnProperty('value') ? response.value : ''; + } + }); +}; + Post = ghostBookshelf.Model.extend({ tableName: 'posts', @@ -38,6 +58,9 @@ Post = ghostBookshelf.Model.extend({ return self.updateTags(model, attributes, options); }); + // Ensures local copy of permalink setting is kept up to date + this.on('fetching', getPermalinkSetting); + this.on('created', function (model) { var isPage = !!model.get('page'); if (isPage) { @@ -137,7 +160,7 @@ Post = ghostBookshelf.Model.extend({ ghostBookshelf.Model.prototype.creating.call(this, newPage, attr, options); }, - /** + /** * ### updateTags * Update tags that are attached to a post. Create any tags that don't already exist. * @param {Object} newPost @@ -230,11 +253,11 @@ Post = ghostBookshelf.Model.extend({ var attrs = ghostBookshelf.Model.prototype.toJSON.call(this, options); attrs.author = attrs.author || attrs.author_id; + attrs.url = config.urlPathForPost(attrs, permalinkSetting); delete attrs.author_id; return attrs; } - }, { /** @@ -625,7 +648,14 @@ Post = ghostBookshelf.Model.extend({ }); Posts = ghostBookshelf.Collection.extend({ - model: Post + model: Post, + + initialize: function () { + ghostBookshelf.Collection.prototype.initialize.apply(this, arguments); + + // Ensures local copy of permalink setting is kept up to date + this.on('fetching', getPermalinkSetting); + } }); module.exports = { diff --git a/core/server/xmlrpc.js b/core/server/xmlrpc.js index 6a088de73d..69585c896c 100644 --- a/core/server/xmlrpc.js +++ b/core/server/xmlrpc.js @@ -1,7 +1,6 @@ var _ = require('lodash'), http = require('http'), xml = require('xml'), - api = require('./api'), config = require('./config'), errors = require('./errors'), pingList; @@ -17,7 +16,8 @@ pingList = [{ function ping(post) { var pingXML, - title = post.title; + title = post.title, + url = config.urlFor('post', {post: post}, true); // Only ping when in production and not a page if (process.env.NODE_ENV !== 'production' || post.page || config.isPrivacyDisabled('useRpcPing')) { @@ -32,49 +32,46 @@ function ping(post) { return; } - // Need to require here because of circular dependency - return config.urlForPost(api.settings, post, true).then(function (url) { - // Build XML object. - pingXML = xml({ - methodCall: [{ - methodName: 'weblogUpdate.ping' - }, { - params: [{ - param: [{ - value: [{ - string: title - }] + // Build XML object. + pingXML = xml({ + methodCall: [{ + methodName: 'weblogUpdate.ping' + }, { + params: [{ + param: [{ + value: [{ + string: title }] - }, { - param: [{ - value: [{ - string: url - }] + }] + }, { + param: [{ + value: [{ + string: url }] }] }] - }, {declaration: true}); + }] + }, {declaration: true}); - // Ping each of the defined services. - _.each(pingList, function (pingHost) { - var options = { - hostname: pingHost.host, - path: pingHost.path, - method: 'POST' - }, - req; + // Ping each of the defined services. + _.each(pingList, function (pingHost) { + var options = { + hostname: pingHost.host, + path: pingHost.path, + method: 'POST' + }, + req; - req = http.request(options); - req.write(pingXML); - req.on('error', function (error) { - errors.logError( - error, - 'Pinging services for updates on your blog failed, your blog will continue to function.', - 'If you get this error repeatedly, please seek help from https://ghost.org/forum.' - ); - }); - req.end(); + req = http.request(options); + req.write(pingXML); + req.on('error', function (error) { + errors.logError( + error, + 'Pinging services for updates on your blog failed, your blog will continue to function.', + 'If you get this error repeatedly, please seek help from https://ghost.org/forum.' + ); }); + req.end(); }); } diff --git a/core/test/integration/api/api_users_spec.js b/core/test/integration/api/api_users_spec.js index ea3998dc99..65425b87ee 100644 --- a/core/test/integration/api/api_users_spec.js +++ b/core/test/integration/api/api_users_spec.js @@ -1,20 +1,20 @@ /*globals describe, before, beforeEach, afterEach, it */ /*jshint expr:true*/ -var testUtils = require('../../utils'), - should = require('should'), - sinon = require('sinon'), - Promise = require('bluebird'), - _ = require('lodash'), +var testUtils = require('../../utils'), + should = require('should'), + sinon = require('sinon'), + Promise = require('bluebird'), + _ = require('lodash'), // Stuff we are testing - ModelUser = require('../../../server/models'), - UserAPI = require('../../../server/api/users'), - mail = require('../../../server/api/mail'), + ModelUser = require('../../../server/models'), + UserAPI = require('../../../server/api/users'), + mail = require('../../../server/api/mail'), - context = testUtils.context, - userIdFor = testUtils.users.ids, - roleIdFor = testUtils.roles.ids, - sandbox = sinon.sandbox.create(); + context = testUtils.context, + userIdFor = testUtils.users.ids, + roleIdFor = testUtils.roles.ids, + sandbox = sinon.sandbox.create(); describe('Users API', function () { // Keep the DB clean diff --git a/core/test/integration/model/model_posts_spec.js b/core/test/integration/model/model_posts_spec.js index c6fe26eae8..8ee84efaeb 100644 --- a/core/test/integration/model/model_posts_spec.js +++ b/core/test/integration/model/model_posts_spec.js @@ -4,12 +4,16 @@ var testUtils = require('../../utils'), should = require('should'), sequence = require('../../../server/utils/sequence'), _ = require('lodash'), + Promise = require('bluebird'), + sinon = require('sinon'), + SettingsModel = require('../../../server/models/settings').Settings, // Stuff we are testing ghostBookshelf = require('../../../server/models/base'), PostModel = require('../../../server/models/post').Post, DataGenerator = testUtils.DataGenerator, - context = testUtils.context.owner; + context = testUtils.context.owner, + sandbox = sinon.sandbox.create(); describe('Post Model', function () { // Keep the DB clean @@ -17,9 +21,12 @@ describe('Post Model', function () { describe('Single author posts', function () { before(testUtils.teardown); afterEach(testUtils.teardown); + afterEach(function () { + sandbox.restore(); + }); beforeEach(testUtils.setup('owner', 'posts', 'apps')); - before(function () { + beforeEach(function () { should.exist(PostModel); }); @@ -135,6 +142,48 @@ describe('Post Model', function () { }).catch(done); }); + it('can findOne, returning a slug only permalink', function (done) { + var firstPost = 1; + sandbox.stub(SettingsModel, 'findOne', function () { + return Promise.resolve({toJSON: function () { + return {value: '/:slug/'}; + }}); + }); + + PostModel.findOne({id: firstPost}) + .then(function (result) { + should.exist(result); + firstPost = result.toJSON(); + firstPost.url.should.equal('/html-ipsum/'); + + done(); + }).catch(done); + }); + + it('can findOne, returning a dated permalink', function (done) { + var firstPost = 1, + today = new Date(), + dd = ('0' + today.getDate()).slice(-2), + mm = ('0' + (today.getMonth() + 1)).slice(-2), + yyyy = today.getFullYear(), + postLink = '/' + yyyy + '/' + mm + '/' + dd + '/html-ipsum/'; + + sandbox.stub(SettingsModel, 'findOne', function () { + return Promise.resolve({toJSON: function () { + return {value: '/:year/:month/:day/:slug/'}; + }}); + }); + + PostModel.findOne({id: firstPost}) + .then(function (result) { + should.exist(result); + firstPost = result.toJSON(); + firstPost.url.should.equal(postLink); + + done(); + }).catch(done); + }); + it('can edit', function (done) { var firstPost = 1; diff --git a/core/test/unit/config_spec.js b/core/test/unit/config_spec.js index 955d7f63b8..434145df6b 100644 --- a/core/test/unit/config_spec.js +++ b/core/test/unit/config_spec.js @@ -189,10 +189,12 @@ describe('Config', function () { config.urlFor(testContext, true).should.equal('http://my-ghost-blog.com/blog/about/'); }); - it('should return url for a post when asked for', function () { + it('should return url for a post from post object', function () { var testContext = 'post', - testData = {post: testUtils.DataGenerator.Content.posts[2], permalinks: {value: '/:slug/'}}; + testData = {post: testUtils.DataGenerator.Content.posts[2]}; + // url is now provided on the postmodel, permalinkSetting tests are in the model_post_spec.js test + testData.post.url = '/short-and-sweet/'; config.set({url: 'http://my-ghost-blog.com'}); config.urlFor(testContext, testData).should.equal('/short-and-sweet/'); config.urlFor(testContext, testData, true).should.equal('http://my-ghost-blog.com/short-and-sweet/'); @@ -202,27 +204,6 @@ describe('Config', function () { config.urlFor(testContext, testData, true).should.equal('http://my-ghost-blog.com/blog/short-and-sweet/'); }); - it('should return url for a dated post when asked for', function () { - var testContext = 'post', - testData = { - post: testUtils.DataGenerator.Content.posts[2], - permalinks: {value: '/:year/:month/:day/:slug/'} - }, - today = new Date(), - dd = ('0' + today.getDate()).slice(-2), - mm = ('0' + (today.getMonth() + 1)).slice(-2), - yyyy = today.getFullYear(), - postLink = '/' + yyyy + '/' + mm + '/' + dd + '/short-and-sweet/'; - - config.set({url: 'http://my-ghost-blog.com'}); - config.urlFor(testContext, testData).should.equal(postLink); - config.urlFor(testContext, testData, true).should.equal('http://my-ghost-blog.com' + postLink); - - config.set({url: 'http://my-ghost-blog.com/blog'}); - config.urlFor(testContext, testData).should.equal('/blog' + postLink); - config.urlFor(testContext, testData, true).should.equal('http://my-ghost-blog.com/blog' + postLink); - }); - it('should return url for a tag when asked for', function () { var testContext = 'tag', testData = {tag: testUtils.DataGenerator.Content.tags[0]}; @@ -237,59 +218,19 @@ describe('Config', function () { }); }); - describe('urlForPost', function () { - var sandbox; - - beforeEach(function () { - sandbox = sinon.sandbox.create(); - }); - - afterEach(function () { - sandbox.restore(); - config.set({url: defaultConfig.url}); - }); - - it('should output correct url for post', function (done) { - var settings = {read: function read() {}}, - settingsStub = sandbox.stub(settings, 'read', function () { - return Promise.resolve({settings: [{value: '/:slug/'}]}); - }), + describe('urlPathForPost', function () { + it('should output correct url for post', function () { + var permalinkSetting = '/:slug/', /*jshint unused:false*/ testData = testUtils.DataGenerator.Content.posts[2], postLink = '/short-and-sweet/'; - config.set({url: 'http://my-ghost-blog.com'}); - // next test - config.urlForPost(settings, testData).then(function (url) { - url.should.equal(postLink); - - // next test - return config.urlForPost(settings, testData, true); - }).then(function (url) { - url.should.equal('http://my-ghost-blog.com' + postLink); - - return config.set({url: 'http://my-ghost-blog.com/blog'}); - }).then(function () { - // next test - return config.urlForPost(settings, testData); - }).then(function (url) { - url.should.equal('/blog' + postLink); - - // next test - return config.urlForPost(settings, testData, true); - }).then(function (url) { - url.should.equal('http://my-ghost-blog.com/blog' + postLink); - - done(); - }).catch(done); + config.urlPathForPost(testData, permalinkSetting).should.equal(postLink); }); - it('should output correct url for post with date permalink', function (done) { - var settings = {read: function read() {}}, - settingsStub = sandbox.stub(settings, 'read', function () { - return Promise.resolve({settings: [{value: '/:year/:month/:day/:slug/'}]}); - }), + it('should output correct url for post with date permalink', function () { + var permalinkSetting = '/:year/:month/:day/:slug/', /*jshint unused:false*/ testData = testUtils.DataGenerator.Content.posts[2], today = new Date(), @@ -297,68 +238,17 @@ describe('Config', function () { mm = ('0' + (today.getMonth() + 1)).slice(-2), yyyy = today.getFullYear(), postLink = '/' + yyyy + '/' + mm + '/' + dd + '/short-and-sweet/'; - - config.set({url: 'http://my-ghost-blog.com'}); - // next test - config.urlForPost(settings, testData).then(function (url) { - url.should.equal(postLink); - - // next test - return config.urlForPost(settings, testData, true); - }).then(function (url) { - url.should.equal('http://my-ghost-blog.com' + postLink); - - return config.set({url: 'http://my-ghost-blog.com/blog'}); - }).then(function () { - // next test - return config.urlForPost(settings, testData); - }).then(function (url) { - url.should.equal('/blog' + postLink); - - // next test - return config.urlForPost(settings, testData, true); - }).then(function (url) { - url.should.equal('http://my-ghost-blog.com/blog' + postLink); - - done(); - }).catch(done); + config.urlPathForPost(testData, permalinkSetting).should.equal(postLink); }); - it('should output correct url for page with date permalink', function (done) { - var settings = {read: function read() {}}, - settingsStub = sandbox.stub(settings, 'read', function () { - return Promise.resolve({settings: [{value: '/:year/:month/:day/:slug/'}]}); - }), + it('should output correct url for page with date permalink', function () { + var permalinkSetting = '/:year/:month/:day/:slug/', /*jshint unused:false*/ testData = testUtils.DataGenerator.Content.posts[5], postLink = '/static-page-test/'; - - config.set({url: 'http://my-ghost-blog.com'}); - // next test - config.urlForPost(settings, testData).then(function (url) { - url.should.equal(postLink); - - // next test - return config.urlForPost(settings, testData, true); - }).then(function (url) { - url.should.equal('http://my-ghost-blog.com' + postLink); - - return config.set({url: 'http://my-ghost-blog.com/blog'}); - }).then(function () { - // next test - return config.urlForPost(settings, testData); - }).then(function (url) { - url.should.equal('/blog' + postLink); - - // next test - return config.urlForPost(settings, testData, true); - }).then(function (url) { - url.should.equal('http://my-ghost-blog.com/blog' + postLink); - - done(); - }).catch(done); + config.urlPathForPost(testData, permalinkSetting).should.equal(postLink); }); }); diff --git a/core/test/unit/server_helpers/url_spec.js b/core/test/unit/server_helpers/url_spec.js index 0445319413..22238cc556 100644 --- a/core/test/unit/server_helpers/url_spec.js +++ b/core/test/unit/server_helpers/url_spec.js @@ -44,7 +44,8 @@ describe('{{url}} helper', function () { markdown: 'ff', title: 'title', slug: 'slug', - created_at: new Date(0) + created_at: new Date(0), + url: '/slug/' }).then(function (rendered) { should.exist(rendered); rendered.should.equal('/slug/'); @@ -54,7 +55,7 @@ describe('{{url}} helper', function () { it('should output an absolute URL if the option is present', function (done) { helpers.url.call( - {html: 'content', markdown: 'ff', title: 'title', slug: 'slug', created_at: new Date(0)}, + {html: 'content', markdown: 'ff', title: 'title', slug: 'slug', url: '/slug/', created_at: new Date(0)}, {hash: {absolute: 'true'}} ).then(function (rendered) { should.exist(rendered); diff --git a/core/test/unit/xmlrpc_spec.js b/core/test/unit/xmlrpc_spec.js index 263f4dc006..67fc83aa73 100644 --- a/core/test/unit/xmlrpc_spec.js +++ b/core/test/unit/xmlrpc_spec.js @@ -1,11 +1,9 @@ /*globals describe, beforeEach, afterEach, it*/ /*jshint expr:true*/ var nock = require('nock'), - settings = require('../../server/api').settings, should = require('should'), sinon = require('sinon'), testUtils = require('../utils'), - Promise = require('bluebird'), xmlrpc = require('../../server/xmlrpc'), // storing current environment currentEnv = process.env.NODE_ENV; @@ -28,20 +26,15 @@ describe('XMLRPC', function () { process.env.NODE_ENV = currentEnv; }); - it('should execute two pings', function (done) { + it('should execute two pings', function () { var ping1 = nock('http://blogsearch.google.com').post('/ping/RPC2').reply(200), ping2 = nock('http://rpc.pingomatic.com').post('/').reply(200), - testPost = testUtils.DataGenerator.Content.posts[2], - settingsStub = sandbox.stub(settings, 'read', function () { - return Promise.resolve({settings: [{value: '/:slug/'}]}); - }); + testPost = testUtils.DataGenerator.Content.posts[2]; + /*jshint unused:false */ - xmlrpc.ping(testPost).then(function () { - ping1.isDone().should.be.true; - ping2.isDone().should.be.true; - - done(); - }).catch(done); + xmlrpc.ping(testPost); + ping1.isDone().should.be.true; + ping2.isDone().should.be.true; }); }); diff --git a/core/test/utils/api.js b/core/test/utils/api.js index 770c116585..6c74392c5a 100644 --- a/core/test/utils/api.js +++ b/core/test/utils/api.js @@ -13,7 +13,7 @@ var url = require('url'), pagination: ['page', 'limit', 'pages', 'total', 'next', 'prev'], post: ['id', 'uuid', 'title', 'slug', 'markdown', 'html', 'meta_title', 'meta_description', 'featured', 'image', 'status', 'language', 'created_at', 'created_by', 'updated_at', - 'updated_by', 'published_at', 'published_by', 'page', 'author' + 'updated_by', 'published_at', 'published_by', 'page', 'author', 'url' ], settings: ['settings', 'meta'], setting: ['id', 'uuid', 'key', 'value', 'type', 'created_at', 'created_by', 'updated_at', 'updated_by'],