From e24b5c3382d92e4547501fef1815f3595f04b17e Mon Sep 17 00:00:00 2001 From: Adam Howard Date: Mon, 2 Sep 2013 02:49:08 +0100 Subject: [PATCH] Proper settings infrastructure, allowing new features without compromising old data. On server load, check for settings which have not been set, and apply a default value to the settings table from a JSON file. --- core/ghost.js | 2 + core/server/data/default-settings.json | 47 ++++ core/server/data/fixtures/001.js | 61 +---- core/server/data/fixtures/002.js | 30 +-- core/server/data/migration/001.js | 2 +- core/server/data/migration/002.js | 11 +- core/server/data/migration/index.js | 1 + core/server/models/settings.js | 30 ++- core/test/unit/api_settings_spec.js | 349 ++++++++++++++----------- core/test/unit/export_spec.js | 9 +- core/test/unit/helpers.js | 7 +- core/test/unit/import_spec.js | 20 +- 12 files changed, 301 insertions(+), 268 deletions(-) create mode 100644 core/server/data/default-settings.json diff --git a/core/ghost.js b/core/ghost.js index d2000b863c..e3cce32475 100644 --- a/core/ghost.js +++ b/core/ghost.js @@ -145,6 +145,8 @@ Ghost.prototype.init = function () { instance.getPaths(), instance.mail.init(self) ).then(function () { + return models.Settings.populateDefaults(); + }).then(function () { return self.initPlugins(); }).then(function () { // Initialize the settings cache diff --git a/core/server/data/default-settings.json b/core/server/data/default-settings.json new file mode 100644 index 0000000000..0800b357e3 --- /dev/null +++ b/core/server/data/default-settings.json @@ -0,0 +1,47 @@ +[ + { + "key": "title", + "value": "Ghost", + "type": "blog" + }, + { + "key": "description", + "value": "Just a blogging platform.", + "type": "blog" + }, + { + "key": "email", + "value": "ghost@example.com", + "type": "general" + }, + { + "key": "activePlugins", + "value": "", + "type": "general" + }, + { + "key": "activeTheme", + "value": "content/themes/casper", + "type": "general" + }, + { + "key": "currentVersion", + "value": "002", + "type": "core" + }, + { + "key": "installedPlugins", + "value": "[]", + "type": "core" + }, + { + "key": "logo", + "value": "", + "type": "blog" + }, + { + "key": "icon", + "value": "", + "type": "blog" + } +] diff --git a/core/server/data/fixtures/001.js b/core/server/data/fixtures/001.js index 3452fad213..432a247c3b 100644 --- a/core/server/data/fixtures/001.js +++ b/core/server/data/fixtures/001.js @@ -25,65 +25,6 @@ module.exports = { } ], - settings: [ - { - "uuid": uuid.v4(), - "key": "url", - "value": "http://localhost:2368", - "created_by": 1, - "updated_by": 1, - "type": "blog" - }, - { - "uuid": uuid.v4(), - "key": "title", - "value": "Ghost", - "created_by": 1, - "updated_by": 1, - "type": "blog" - }, - { - "uuid": uuid.v4(), - "key": "description", - "value": "Just a blogging platform.", - "created_by": 1, - "updated_by": 1, - "type": "blog" - }, - { - "uuid": uuid.v4(), - "key": "email", - "value": "ghost@example.com", - "created_by": 1, - "updated_by": 1, - "type": "general" - }, - { - "uuid": uuid.v4(), - "key": "activePlugins", - "value": "", - "created_by": 1, - "updated_by": 1, - "type": "general" - }, - { - "uuid": uuid.v4(), - "key": "activeTheme", - "value": "content/themes/casper", - "created_by": 1, - "updated_by": 1, - "type": "general" - }, - { - "uuid": uuid.v4(), - "key": "currentVersion", - "value": "001", - "created_by": 1, - "updated_by": 1, - "type": "core" - } - ], - roles: [ { "id": 1, @@ -140,4 +81,4 @@ module.exports = { "role_id": 1 } ] -}; \ No newline at end of file +}; diff --git a/core/server/data/fixtures/002.js b/core/server/data/fixtures/002.js index 92d8324550..8858adc467 100644 --- a/core/server/data/fixtures/002.js +++ b/core/server/data/fixtures/002.js @@ -3,37 +3,9 @@ var uuid = require('node-uuid'); module.exports = { posts: [], - settings: [ - { - "uuid": uuid.v4(), - "key": "installedPlugins", - "value": "[]", - "created_by": 1, - "updated_by": 1, - "type": "core" - }, - { - "uuid": uuid.v4(), - "key": "logo", - "value": "", - "created_by": 1, - "updated_by": 1, - "type": "blog" - }, - { - "uuid": uuid.v4(), - "key": "icon", - "value": "", - "created_by": 1, - "updated_by": 1, - "type": "blog" - } - - ], - roles: [], permissions: [], permissions_roles: [] -}; \ No newline at end of file +}; diff --git a/core/server/data/migration/001.js b/core/server/data/migration/001.js index 82f2c7743c..9634b293e8 100644 --- a/core/server/data/migration/001.js +++ b/core/server/data/migration/001.js @@ -101,7 +101,7 @@ up = function () { // knex('roles_users').insert(fixtures.roles_users), knex('permissions').insert(fixtures.permissions), knex('permissions_roles').insert(fixtures.permissions_roles), - knex('settings').insert(fixtures.settings) + knex('settings').insert({ key: 'currentVersion', 'value': '001', type: 'core' }) ]); }); diff --git a/core/server/data/migration/002.js b/core/server/data/migration/002.js index 35e885e236..f298cba47f 100644 --- a/core/server/data/migration/002.js +++ b/core/server/data/migration/002.js @@ -58,14 +58,6 @@ up = function () { }) ]).then(function () { - - // Once we create all of the initial tables, bootstrap any of the data - return when.all([ - knex('settings').insert(fixtures.settings) - ]); - - }).then(function () { - // Lastly, update the current version settings to reflect this version return knex('settings') .where('key', 'currentVersion') @@ -84,7 +76,8 @@ down = function () { knex.Schema.dropTableIfExists("posts_custom_data") ]); }); + // Should we also drop the currentVersion? }; exports.up = up; -exports.down = down; \ No newline at end of file +exports.down = down; diff --git a/core/server/data/migration/index.js b/core/server/data/migration/index.js index ee7450a6fa..0009b6e371 100644 --- a/core/server/data/migration/index.js +++ b/core/server/data/migration/index.js @@ -7,6 +7,7 @@ var _ = require('underscore'), initialVersion = "001", // This currentVersion string should always be the current version of Ghost, // we could probably load it from the config file. + // - Will be possible after default-settings.json restructure currentVersion = "002"; function getCurrentVersion() { diff --git a/core/server/models/settings.js b/core/server/models/settings.js index 56aa896085..80781bc5b0 100644 --- a/core/server/models/settings.js +++ b/core/server/models/settings.js @@ -3,7 +3,8 @@ var Settings, uuid = require('node-uuid'), _ = require('underscore'), errors = require('../errorHandling'), - when = require('when'); + when = require('when'), + defaultSettings = require('../data/default-settings.json'); // Each setting is saved as a separate row in the database, // but the overlying API treats them as a single key:value mapping @@ -36,7 +37,7 @@ Settings = GhostBookshelf.Model.extend({ saving: function () { // Deal with the related data here - // Remove any properties which don't belong on the post model + // Remove any properties which don't belong on the model this.attributes = this.pick(this.permittedAttributes); } }, { @@ -57,12 +58,33 @@ Settings = GhostBookshelf.Model.extend({ // Accept an array of models as input if (item.toJSON) { item = item.toJSON(); } return settings.forge({ key: item.key }).fetch().then(function (setting) { - return setting.set('value', item.value).save(); + if (setting) { + return setting.set('value', item.value).save(); + } + return settings.forge({ key: item.key, value: item.value }).save(); + }, errors.logAndThrowError); }); + }, + + populateDefaults: function () { + return this.findAll().then(function (allSettings) { + var usedKeys = allSettings.models.map(function (setting) { return setting.get('key'); }), + insertOperations = []; + + defaultSettings.forEach(function (defaultSetting) { + var isMissingFromDB = usedKeys.indexOf(defaultSetting.key) === -1; + if (isMissingFromDB) { + insertOperations.push(Settings.forge(defaultSetting).save()); + } + }); + + return when.all(insertOperations); + }); } + }); module.exports = { Settings: Settings -}; \ No newline at end of file +}; diff --git a/core/test/unit/api_settings_spec.js b/core/test/unit/api_settings_spec.js index edb03b747a..2ef37cbe28 100644 --- a/core/test/unit/api_settings_spec.js +++ b/core/test/unit/api_settings_spec.js @@ -2,7 +2,8 @@ var _ = require("underscore"), should = require('should'), helpers = require('./helpers'), - Models = require('../../server/models'); + Models = require('../../server/models'), + knex = require('../../server/models/base').Knex; describe('Settings Model', function () { @@ -27,165 +28,207 @@ describe('Settings Model', function () { }, done); }); - it('can browse', function (done) { - SettingsModel.browse().then(function (results) { + describe('API', function () { - should.exist(results); + it('can browse', function (done) { + SettingsModel.browse().then(function (results) { - results.length.should.be.above(0); + should.exist(results); - done(); - }).then(null, done); + results.length.should.be.above(0); + + done(); + }).then(null, done); + }); + + it('can read', function (done) { + var firstSetting; + + SettingsModel.browse().then(function (results) { + + should.exist(results); + + results.length.should.be.above(0); + + firstSetting = results.models[0]; + + return SettingsModel.read(firstSetting.attributes.key); + + }).then(function (found) { + + should.exist(found); + + found.attributes.value.should.equal(firstSetting.attributes.value); + + done(); + + }).then(null, done); + }); + + it('can edit single', function (done) { + var firstSetting; + + SettingsModel.browse().then(function (results) { + + should.exist(results); + + results.length.should.be.above(0); + + firstSetting = results.models[0]; + + // The edit method has been modified to take an object of + // key/value pairs + firstSetting.set('value', 'new value'); + + return SettingsModel.edit(firstSetting); + + }).then(function (edited) { + + should.exist(edited); + + edited.length.should.equal(1); + + edited = edited[0]; + + edited.attributes.key.should.equal(firstSetting.attributes.key); + edited.attributes.value.should.equal('new value'); + + done(); + + }).then(null, done); + }); + + it('can edit multiple', function (done) { + var model1, + model2, + editedModel; + + SettingsModel.browse().then(function (results) { + + should.exist(results); + + results.length.should.be.above(0); + + model1 = results.models[0]; + model2 = results.models[1]; + + // The edit method has been modified to take an object of + // key/value pairs + model1.set('value', 'new value1'); + model2.set('value', 'new value2'); + + return SettingsModel.edit([model1, model2]); + + }).then(function (edited) { + + should.exist(edited); + + edited.length.should.equal(2); + + editedModel = edited[0]; + + editedModel.attributes.key.should.equal(model1.attributes.key); + editedModel.attributes.value.should.equal('new value1'); + + editedModel = edited[1]; + + editedModel.attributes.key.should.equal(model2.attributes.key); + editedModel.attributes.value.should.equal('new value2'); + + done(); + + }).then(null, done); + }); + + it('can add', function (done) { + var newSetting = { + key: 'TestSetting1', + value: 'Test Content 1' + }; + + SettingsModel.add(newSetting).then(function (createdSetting) { + + should.exist(createdSetting); + createdSetting.has('uuid').should.equal(true); + createdSetting.attributes.key.should.equal(newSetting.key, "key is correct"); + createdSetting.attributes.value.should.equal(newSetting.value, "value is correct"); + createdSetting.attributes.type.should.equal("general"); + + done(); + }).then(null, done); + }); + + it('can delete', function (done) { + var firstSettingId; + + SettingsModel.browse().then(function (results) { + + should.exist(results); + + results.length.should.be.above(0); + + firstSettingId = results.models[0].id; + + return SettingsModel.destroy(firstSettingId); + + }).then(function () { + + return SettingsModel.browse(); + + }).then(function (newResults) { + + var ids, hasDeletedId; + + ids = _.pluck(newResults.models, "id"); + + hasDeletedId = _.any(ids, function (id) { + return id === firstSettingId; + }); + + hasDeletedId.should.equal(false); + + done(); + + }).then(null, done); + }); }); - it('can read', function (done) { - var firstSetting; + describe('populating defaults from settings.json', function (done) { - SettingsModel.browse().then(function (results) { - - should.exist(results); - - results.length.should.be.above(0); - - firstSetting = results.models[0]; - - return SettingsModel.read(firstSetting.attributes.key); - - }).then(function (found) { - - should.exist(found); - - found.attributes.value.should.equal(firstSetting.attributes.value); - - done(); - - }).then(null, done); - }); - - it('can edit single', function (done) { - var firstSetting; - - SettingsModel.browse().then(function (results) { - - should.exist(results); - - results.length.should.be.above(0); - - firstSetting = results.models[0]; - - // The edit method has been modified to take an object of - // key/value pairs - firstSetting.set('value', 'new value'); - - return SettingsModel.edit(firstSetting); - - }).then(function (edited) { - - should.exist(edited); - - edited.length.should.equal(1); - - edited = edited[0]; - - edited.attributes.key.should.equal(firstSetting.attributes.key); - edited.attributes.value.should.equal('new value'); - - done(); - - }).then(null, done); - }); - - it('can edit multiple', function (done) { - var model1, - model2, - editedModel; - - SettingsModel.browse().then(function (results) { - - should.exist(results); - - results.length.should.be.above(0); - - model1 = results.models[0]; - model2 = results.models[1]; - - // The edit method has been modified to take an object of - // key/value pairs - model1.set('value', 'new value1'); - model2.set('value', 'new value2'); - - return SettingsModel.edit([model1, model2]); - - }).then(function (edited) { - - should.exist(edited); - - edited.length.should.equal(2); - - editedModel = edited[0]; - - editedModel.attributes.key.should.equal(model1.attributes.key); - editedModel.attributes.value.should.equal('new value1'); - - editedModel = edited[1]; - - editedModel.attributes.key.should.equal(model2.attributes.key); - editedModel.attributes.value.should.equal('new value2'); - - done(); - - }).then(null, done); - }); - - it('can add', function (done) { - var newSetting = { - key: 'TestSetting1', - value: 'Test Content 1' - }; - - SettingsModel.add(newSetting).then(function (createdSetting) { - - should.exist(createdSetting); - createdSetting.has('uuid').should.equal(true); - createdSetting.attributes.key.should.equal(newSetting.key, "key is correct"); - createdSetting.attributes.value.should.equal(newSetting.value, "value is correct"); - createdSetting.attributes.type.should.equal("general"); - - done(); - }).then(null, done); - }); - - it('can delete', function (done) { - var firstSettingId; - - SettingsModel.browse().then(function (results) { - - should.exist(results); - - results.length.should.be.above(0); - - firstSettingId = results.models[0].id; - - return SettingsModel.destroy(firstSettingId); - - }).then(function () { - - return SettingsModel.browse(); - - }).then(function (newResults) { - - var ids, hasDeletedId; - - ids = _.pluck(newResults.models, "id"); - - hasDeletedId = _.any(ids, function (id) { - return id === firstSettingId; + beforeEach(function (done) { + knex('settings').truncate().then(function () { + done(); }); + }); - hasDeletedId.should.equal(false); + it('populates any unset settings from the JSON defaults', function (done) { + SettingsModel.findAll().then(function (allSettings) { + console.log(allSettings.models) + allSettings.length.should.equal(0); + return SettingsModel.populateDefaults(); + }).then(function () { + return SettingsModel.findAll(); + }).then(function (allSettings) { + allSettings.length.should.be.above(0); + return SettingsModel.read('description'); + }).then(function (descriptionSetting) { + // Testing against the actual value in default-settings.json feels icky, + // but it's easier to fix the test if that ever changes than to mock out that behaviour + descriptionSetting.get('value').should.equal('Just a blogging platform.'); + done(); + }).then(null, done); + }); - done(); - - }).then(null, done); + it('doesn\'t overwrite any existing settings', function (done) { + SettingsModel.edit({key: 'description', value: 'Adam\'s Blog'}).then(function () { + return SettingsModel.populateDefaults(); + }).then(function () { + return SettingsModel.read('description'); + }).then(function (descriptionSetting) { + descriptionSetting.get('value').should.equal('Adam\'s Blog'); + done(); + }).then(null, done); + }); }); -}); \ No newline at end of file + +}); diff --git a/core/test/unit/export_spec.js b/core/test/unit/export_spec.js index bff0440a86..7674339d85 100644 --- a/core/test/unit/export_spec.js +++ b/core/test/unit/export_spec.js @@ -8,7 +8,8 @@ var _ = require("underscore"), exporter = require('../../server/data/export'), Exporter001 = require('../../server/data/export/001'), Exporter002 = require('../../server/data/export/002'), - errors = require('../../server/errorHandling'); + errors = require('../../server/errorHandling'), + Settings = require('../../server/models/settings').Settings; describe("Export", function () { @@ -43,6 +44,8 @@ describe("Export", function () { // initialise database to version 001 - confusingly we have to set the max version to be one higher // than the migration version we want migration.migrateUpFromVersion('001', '002').then(function () { + return Settings.populateDefaults(); + }).then(function () { return exporter("001"); }).then(function (exportData) { var tables = ['posts', 'users', 'roles', 'roles_users', 'permissions', 'permissions_roles', 'settings']; @@ -88,6 +91,8 @@ describe("Export", function () { // initialise database to version 001 - confusingly we have to set the max version to be one higher // than the migration version we want migration.migrateUpFromVersion('001', '003').then(function () { + return Settings.populateDefaults(); + }).then(function () { return exporter("002"); }).then(function (exportData) { var tables = [ @@ -111,4 +116,4 @@ describe("Export", function () { }).then(null, done); }); }); -}); \ No newline at end of file +}); diff --git a/core/test/unit/helpers.js b/core/test/unit/helpers.js index e34d71912f..247575ed5b 100644 --- a/core/test/unit/helpers.js +++ b/core/test/unit/helpers.js @@ -4,6 +4,7 @@ process.env.NODE_ENV = process.env.TRAVIS ? 'travis' : 'testing'; var knex = require('../../server/models/base').Knex, when = require('when'), migration = require("../../server/data/migration/"), + Settings = require('../../server/models/settings').Settings, helpers, samplePost, sampleUser, @@ -44,7 +45,9 @@ sampleUserRole = function (i) { helpers = { initData: function (done) { - return migration.init(); + return migration.init().then(function () { + return Settings.populateDefaults(); + }); }, clearData: function () { @@ -83,4 +86,4 @@ helpers = { }; -module.exports = helpers; \ No newline at end of file +module.exports = helpers; diff --git a/core/test/unit/import_spec.js b/core/test/unit/import_spec.js index 516f33d06d..cd325cc4de 100644 --- a/core/test/unit/import_spec.js +++ b/core/test/unit/import_spec.js @@ -10,7 +10,8 @@ var _ = require("underscore"), importer = require('../../server/data/import'), Importer001 = require('../../server/data/import/001'), Importer002 = require('../../server/data/import/002'), - errors = require('../../server/errorHandling'); + errors = require('../../server/errorHandling'), + Settings = require('../../server/models/settings').Settings; describe("Import", function () { @@ -50,6 +51,8 @@ describe("Import", function () { // initialise database to version 001 - confusingly we have to set the max version to be one higher // than the migration version we want migration.migrateUpFromVersion('001', '002').then(function () { + return Settings.populateDefaults(); + }).then(function () { // export the version 001 data ready to import // TODO: Should have static test data here? return exporter("001"); @@ -83,8 +86,7 @@ describe("Import", function () { // we always have 0 users as there isn't one in fixtures importedData[0].length.should.equal(0); importedData[1].length.should.equal(exportData.data.posts.length); - // version 001 settings have 7 fields - importedData[2].length.should.equal(7); + importedData[2].length.should.be.above(0); _.findWhere(exportData.data.settings, {key: "currentVersion"}).value.should.equal("001"); @@ -119,6 +121,8 @@ describe("Import", function () { // initialise database to version 001 - confusingly we have to set the max version to be one higher // than the migration version we want migration.migrateUpFromVersion('001', '002').then(function () { + return Settings.populateDefaults(); + }).then(function () { // export the version 001 data ready to import // TODO: Should have static test data here? return exporter("001"); @@ -146,8 +150,7 @@ describe("Import", function () { importedData[0].length.should.equal(0); // import no longer requires all data to be dropped, and adds posts importedData[1].length.should.equal(exportData.data.posts.length + 1); - // version 002 settings have 10 fields, and settings get updated not inserted - importedData[2].length.should.equal(10); + importedData[2].length.should.be.above(0); _.findWhere(importedData[2], {key: "currentVersion"}).value.should.equal("002"); @@ -161,6 +164,8 @@ describe("Import", function () { // initialise database to version 001 - confusingly we have to set the max version to be one higher // than the migration version we want migration.migrateUpFromVersion('001', '003').then(function () { + return Settings.populateDefaults(); + }).then(function () { // export the version 002 data ready to import // TODO: Should have static test data here? return exporter("002"); @@ -184,8 +189,7 @@ describe("Import", function () { importedData[0].length.should.equal(0); // import no longer requires all data to be dropped, and adds posts importedData[1].length.should.equal(exportData.data.posts.length + 1); - // version 002 settings have 10 fields, and settings get updated not inserted - importedData[2].length.should.equal(10); + importedData[2].length.should.be.above(0); _.findWhere(importedData[2], {key: "currentVersion"}).value.should.equal("002"); @@ -193,4 +197,4 @@ describe("Import", function () { }).then(null, done); }); }); -}); \ No newline at end of file +});