Skip to content

Commit

Permalink
remove jQuery theme injection in ghost_foot helper and add cdn link w…
Browse files Browse the repository at this point in the history
…hen migrating

closes TryGhost#5298
- remove all harcoded instances of jQuery throughout the front-end of the blog
- add migration function to add cdn link to ghost_foot code injection when migrating up from version 003
- migration version bump
  • Loading branch information
acburdine authored and sebgie committed Aug 19, 2015
1 parent e9fbbdc commit 549e30f
Show file tree
Hide file tree
Showing 8 changed files with 121 additions and 125 deletions.
47 changes: 6 additions & 41 deletions Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -403,24 +403,10 @@ var _ = require('lodash'),
// ### grunt-contrib-copy
// Copy files into their correct locations as part of building assets, or creating release zips
copy: {
jquery: {
cwd: 'core/client/bower_components/jquery/dist/',
src: 'jquery.js',
dest: 'core/built/public/',
expand: true,
nonull: true
},
release: {
files: [{
cwd: 'core/client/bower_components/jquery/dist/',
src: 'jquery.js',
dest: 'core/built/public/',
expand: true
}, {
expand: true,
src: buildGlob,
dest: '<%= paths.releaseBuild %>/'
}]
expand: true,
src: buildGlob,
dest: '<%= paths.releaseBuild %>/'
}
},

Expand All @@ -437,27 +423,6 @@ var _ = require('lodash'),
}
},

// ### grunt-contrib-uglify
// Minify concatenated javascript files ready for production
uglify: {
prod: {
options: {
sourceMap: false
},
files: {
'core/built/public/jquery.min.js': 'core/built/public/jquery.js'
}
},
release: {
options: {
sourceMap: false
},
files: {
'core/built/public/jquery.min.js': 'core/built/public/jquery.js'
}
}
},

// ### grunt-update-submodules
// Grunt task to update git submodules
update_submodules: {
Expand Down Expand Up @@ -920,7 +885,7 @@ var _ = require('lodash'),
// ### Basic Asset Building
// Builds and moves necessary client assets. Prod additionally builds the ember app.
grunt.registerTask('assets', 'Basic asset building & moving',
['clean:tmp', 'buildAboutPage', 'copy:jquery']);
['clean:tmp', 'buildAboutPage']);

// ### Default asset build
// `grunt` - default grunt task
Expand All @@ -934,7 +899,7 @@ var _ = require('lodash'),
//
// It is otherwise the same as running `grunt`, but is only used when running Ghost in the `production` env.
grunt.registerTask('prod', 'Build JS & templates for production',
['shell:ember:prod', 'uglify:prod', 'master-warn']);
['shell:ember:prod', 'master-warn']);

// ### Live reload
// `grunt dev` - build assets on the fly whilst developing
Expand All @@ -961,7 +926,7 @@ var _ = require('lodash'),
' - Copy files to release-folder/#/#{version} directory\n' +
' - Clean out unnecessary files (travis, .git*, etc)\n' +
' - Zip files in release-folder to dist-folder/#{version} directory',
['init', 'shell:ember:prod', 'uglify:release', 'clean:release', 'shell:shrinkwrap', 'copy:release', 'compress:release']);
['init', 'shell:ember:prod', 'clean:release', 'shell:shrinkwrap', 'copy:release', 'compress:release']);
};

module.exports = configureGrunt;
7 changes: 6 additions & 1 deletion core/server/api/notifications.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@
// RESTful API for creating notifications
var Promise = require('bluebird'),
_ = require('lodash'),
canThis = require('../permissions').canThis,
permissions = require('../permissions'),
errors = require('../errors'),
utils = require('./utils'),
pipeline = require('../utils/pipeline'),
canThis = permissions.canThis,

// Holds the persistent notifications
notificationsStore = [],
Expand Down Expand Up @@ -57,6 +58,10 @@ notifications = {
* @returns {Object} options
*/
function handlePermissions(options) {
if (permissions.parseContext(options.context).internal) {
return Promise.resolve(options);
}

return canThis(options.context).add.notification().then(function () {
return options;
}, function () {
Expand Down
47 changes: 46 additions & 1 deletion core/server/data/fixtures/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,17 @@ var Promise = require('bluebird'),
sequence = require('../../utils/sequence'),
_ = require('lodash'),
errors = require('../../errors'),
config = require('../../config'),
utils = require('../../utils'),
models = require('../../models'),
fixtures = require('./fixtures'),
permissions = require('./permissions'),
notifications = require('../../api/notifications'),

// Private
logInfo,
to003,
to004,
convertAdminToOwner,
createOwner,
options = {context: {internal: true}},
Expand Down Expand Up @@ -156,13 +159,55 @@ to003 = function () {
});
};

/**
* Update ghost_foot to include a CDN of jquery if the DB is migrating from
* @return {Promise}
*/
to004 = function () {
var value,
jquery = [
'<!-- only delete this line if you are ABSOLUTELY SURE your theme doesn\'t require jQuery -->\n',

This comment has been minimized.

Copy link
@JohnONolan

JohnONolan Aug 24, 2015

Suggest update to:

<!-- You can safely delete this line if your theme does not require jQuery -->\n

'<script type="text/javascript" src="http://code.jquery.com/jquery-1.11.3.min.js"></script>\n\n'
],
privacyMessage = [
'Because jQuery is no longer included for Ghost themes by default,',
'a link to the jQuery library via a CDN has been added to your code-injection setting,',
'If you wish to remove it for privacy reasons, please ensure it is not required by your theme.'

This comment has been minimized.

Copy link
@JohnONolan

JohnONolan Aug 24, 2015

This should be a notification, the destination of the notification (what happens when it is clicked) should be the code injection screen. Suggested message:

jQuery has been removed from Ghost core, it is now located within your <strong>Code Injection</strong> settings, where it can be edited or removed entirely.

This comment has been minimized.

Copy link
@ErisDS

ErisDS Aug 24, 2015

I'd like to suggest an amendment, to mention CDN explicitly as this is what I think is the critical info for the people this message is targeted at, and to include that this can be changed:

jQuery has been removed from Ghost core and is now served via a CDN, this can be changed in your Code Injection settings.

Here's what it looks like, just for fun:

This comment has been minimized.

Copy link
@JohnONolan

JohnONolan Aug 24, 2015

Agreed, but I also think which CDN is critical. Suggest:

jQuery has been removed from Ghost core and is now being loaded from the jQuery Foundation's CDN. This can be changed or removed entirely in your <strong>Code Injection</strong> settings area.

This comment has been minimized.

Copy link
@ErisDS

ErisDS Aug 24, 2015

Very much 👍

];

return models.Settings.findOne('ghost_foot').then(function (setting) {
if (setting) {
value = setting.attributes.value;
value = jquery.join('') + value;

This comment has been minimized.

Copy link
@ErisDS

ErisDS Aug 23, 2015

Thinking out loud, should this perhaps be .join('\n') ?

This comment has been minimized.

Copy link
@ErisDS

ErisDS Aug 23, 2015

Ignore me - the snippet ends with two \n's, didn't see that

return models.Settings.edit({key: 'ghost_foot', value: value}, options).then(function () {
if (_.isEmpty(config.privacy)) {
return Promise.resolve();
}
return notifications.add({notifications: [{
type: 'info',
message: privacyMessage.join(' ')
}]}, options);
});
}

This comment has been minimized.

Copy link
@ErisDS

ErisDS Aug 21, 2015

Quick thought - what happens if you upgrade from pre 0.5.6 (where ghost_head & ghost_foot were introduced) straight to this version? I haven't tested it, but once TryGhost#5703 is in I can package up a zip with this PR & take it for a proper spin

This comment has been minimized.

Copy link
@ErisDS

ErisDS Aug 23, 2015

Turns out my concern here was correct, if you upgrade straight from 0.5.5 to a release generated from this PR, then you do not get jQuery added to ghost_foot because ghost_foot hasn't been created yet.

Upgrading from 0.5.6 (where the setting is already created) works just fine.

I realise supporting upgrades from 0.5.5 -> 0.7.0 is pretty edge case, but I think this is simple enough to warrant fixing.

This comment has been minimized.

Copy link
@ErisDS

ErisDS Aug 23, 2015

Wondering if we can just switch around the order in which we do populateDefaultSettings vs fixtures.update in migration/index.js#180

That would be a super simple fix, to ensure everything happened in the right order. I can't think of any negative side effects as there is no other place where fixture upgrades touches settings as yet. However, I do also want to fix the settings type for isPrivate and password as part of this migration (see TryGhost#5614 / TryGhost#5503). I think this would work either way around, so I believe it is safe to make the switch... going to have a play around.

});
};

update = function (fromVersion, toVersion) {
var ops = [];

logInfo('Updating fixtures');
// Are we migrating to, or past 003?
if ((fromVersion < '003' && toVersion >= '003') ||
fromVersion === '003' && toVersion === '003' && process.env.FORCE_MIGRATION) {
return to003();
ops.push(to003);
}

if (fromVersion < '004' && toVersion === '004' ||
fromVersion === '004' && toVersion === '004' && process.env.FORCE_MIGRATION) {
ops.push(to004);
}

return sequence(ops);
};

module.exports = {
Expand Down
10 changes: 1 addition & 9 deletions core/server/helpers/ghost_foot.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,13 @@

var hbs = require('express-hbs'),
_ = require('lodash'),
config = require('../config'),
filters = require('../filters'),
api = require('../api'),
utils = require('./utils'),
ghost_foot;

ghost_foot = function (options) {
/*jshint unused:false*/
var jquery = utils.isProduction ? 'jquery.min.js' : 'jquery.js',
foot = [];

foot.push(utils.scriptTemplate({
source: config.paths.subdir + '/public/' + jquery,
version: config.assetHash
}));
var foot = [];

return api.settings.read({key: 'ghost_foot'}).then(function (response) {
foot.push(response.settings[0].value);
Expand Down
6 changes: 5 additions & 1 deletion core/test/integration/api/api_tags_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,11 @@ describe('Tags API', function () {
});

describe('Browse', function () {
beforeEach(testUtils.setup('tags'));
beforeEach(function (done) {
testUtils.fixtures.insertMoreTags().then(function () {
done();
});
});

it('can browse (internal)', function (done) {
TagAPI.browse(testUtils.context.internal).then(function (results) {
Expand Down
40 changes: 40 additions & 0 deletions core/test/integration/migration_spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*globals describe, before, beforeEach, afterEach, it */
/*jshint expr:true*/
var testUtils = require('../utils'),
should = require('should'),

migration = require('../../server/data/migration/index'),
Models = require('../../server/models');

describe('Database Migration (special functions)', function () {
before(testUtils.teardown);
afterEach(testUtils.teardown);

describe('004', function () {
beforeEach(testUtils.setup('settings'));

it('should add jQuery to ghost_foot injection setting', function (done) {
Models.Settings.findOne('ghost_foot').then(function (setting) {
should.exist(setting);
should.exist(setting.attributes);
setting.attributes.value.should.equal('');

process.env.FORCE_MIGRATION = true; // force a migration
migration.init().then(function () {
Models.Settings.findOne('ghost_foot').then(function (result) {
var jquery = [
'<!-- only delete this line if you are ABSOLUTELY SURE your theme doesn\'t require jQuery -->\n',
'<script type="text/javascript" src="http://code.jquery.com/jquery-1.11.3.min.js"></script>\n\n'
];

should.exist(result);
should.exist(result.attributes);
result.attributes.value.should.equal(jquery.join(''));

done();
});
});
});
});
});
});
88 changes: 17 additions & 71 deletions core/test/unit/server_helpers/ghost_foot_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,85 +19,31 @@ describe('{{ghost_foot}} helper', function () {
utils.loadHelpers();
});

afterEach(function () {
sandbox.restore();
utils.restoreConfig();
helpers.__set__('utils.isProduction', false);
beforeEach(function () {
sandbox = sinon.sandbox.create();
});

describe('without Code Injection', function () {
beforeEach(function () {
sandbox = sinon.sandbox.create();
sandbox.stub(api.settings, 'read', function () {
return Promise.resolve({
settings: [{value: ''}]
});
});
});

it('has loaded ghost_foot helper', function () {
should.exist(handlebars.helpers.ghost_foot);
});

it('outputs correct jquery for development mode', function (done) {
utils.overrideConfig({assetHash: 'abc'});

helpers.ghost_foot.call().then(function (rendered) {
should.exist(rendered);
rendered.string.should.match(/<script src=".*\/public\/jquery.js\?v=abc"><\/script>/);

done();
}).catch(done);
});

it('outputs correct jquery for production mode', function (done) {
utils.overrideConfig({assetHash: 'abc'});
helpers.__set__('utils.isProduction', true);

helpers.ghost_foot.call().then(function (rendered) {
should.exist(rendered);
rendered.string.should.match(/<script src=".*\/public\/jquery.min.js\?v=abc"><\/script>/);

done();
}).catch(done);
});
it('has loaded ghost_foot helper', function () {
should.exist(handlebars.helpers.ghost_foot);
});

describe('with Code Injection', function () {
beforeEach(function () {
sandbox = sinon.sandbox.create();
sandbox.stub(api.settings, 'read', function () {
return Promise.resolve({
settings: [{value: '<script></script>'}]
});
it('outputs correct injected code', function (done) {
sandbox.stub(api.settings, 'read', function () {
return Promise.resolve({
settings: [{value: '<script type="text/javascript">var test = \'I am a variable!\'</script>'}]
});
});

afterEach(function () {
sandbox.restore();
});

it('outputs correct jquery for development mode', function (done) {
utils.overrideConfig({assetHash: 'abc'});

helpers.ghost_foot.call().then(function (rendered) {
should.exist(rendered);
rendered.string.should.match(/<script src=".*\/public\/jquery.js\?v=abc"><\/script> <script><\/script>/);

done();
}).catch(done);
});
helpers.ghost_foot.call().then(function (rendered) {
should.exist(rendered);
rendered.string.should.match(/<script type="text\/javascript">var test = 'I am a variable!'<\/script>/);

it('outputs correct jquery for production mode', function (done) {
utils.overrideConfig({assetHash: 'abc'});
helpers.__set__('utils.isProduction', true);

helpers.ghost_foot.call().then(function (rendered) {
should.exist(rendered);
rendered.string.should.match(/<script src=".*\/public\/jquery.min.js\?v=abc"><\/script> <script><\/script>/);
done();
}).catch(done);
});

done();
}).catch(done);
});
afterEach(function () {
sandbox.restore();
utils.restoreConfig();
});
});
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@
"grunt-contrib-compress": "0.13.0",
"grunt-contrib-copy": "0.8.0",
"grunt-contrib-jshint": "0.11.2",
"grunt-contrib-uglify": "0.9.1",
"grunt-contrib-watch": "0.6.1",
"grunt-docker": "0.0.10",
"grunt-express-server": "0.5.1",
Expand Down

0 comments on commit 549e30f

Please sign in to comment.