From 0025f3d6de3c943568e55f391ea699f66c524867 Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Tue, 30 Sep 2014 00:45:58 +0200 Subject: [PATCH] Shorter user slugs (if possible) Closes #4211 --- core/server/models/base.js | 16 ++++++- core/server/models/user.js | 3 +- core/test/functional/base.js | 4 +- core/test/functional/client/settings_test.js | 16 +++---- core/test/integration/api/api_slugs_spec.js | 2 +- .../test/integration/model/model_tags_spec.js | 35 +++++++++++++++ .../integration/model/model_users_spec.js | 44 +++++++++++++++++++ 7 files changed, 107 insertions(+), 13 deletions(-) diff --git a/core/server/models/base.js b/core/server/models/base.js index a7b77cefc2..55e3867ab5 100644 --- a/core/server/models/base.js +++ b/core/server/models/base.js @@ -316,7 +316,7 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ slugTryCount = 1, baseName = Model.prototype.tableName.replace(/s$/, ''), // Look for a matching slug, append an incrementing number if so - checkIfSlugExists; + checkIfSlugExists, longSlug; checkIfSlugExists = function (slugToFind) { var args = {slug: slugToFind}; @@ -333,6 +333,14 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ slugTryCount += 1; + // If we shortened, go back to the full version and try again + if (slugTryCount === 2 && longSlug) { + slugToFind = longSlug; + longSlug = null; + slugTryCount = 1; + return checkIfSlugExists(slugToFind); + } + // If this is the first time through, add the hyphen if (slugTryCount === 2) { slugToFind += '-'; @@ -353,6 +361,12 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ // Remove trailing hyphen slug = slug.charAt(slug.length - 1) === '-' ? slug.substr(0, slug.length - 1) : slug; + // If it's a user, let's try to cut it down (unless this is a human request) + if (baseName === 'user' && options && options.shortSlug && slugTryCount === 1 && slug !== 'ghost-owner') { + longSlug = slug; + slug = (slug.indexOf('-') > -1) ? slug.substr(0, slug.indexOf('-')) : slug; + } + // Check the filtered slug doesn't match any of the reserved keywords return filters.doFilter('slug.reservedSlugs', config.slugs.reserved).then(function (slugList) { // Some keywords cannot be changed diff --git a/core/server/models/user.js b/core/server/models/user.js index 906ebfff72..aeed9437d1 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -52,7 +52,7 @@ User = ghostBookshelf.Model.extend({ if (this.hasChanged('slug') || !this.get('slug')) { // Generating a slug requires a db call to look for conflicting slugs return ghostBookshelf.Model.generateSlug(User, this.get('slug') || this.get('name'), - {transacting: options.transacting}) + {transacting: options.transacting, shortSlug: !this.get('slug')}) .then(function (slug) { self.set({slug: slug}); }); @@ -493,6 +493,7 @@ User = ghostBookshelf.Model.extend({ options = this.filterOptions(options, 'setup'); options.withRelated = _.union(['roles'], options.include); + options.shortSlug = true; return validatePasswordLength(userData.password).then(function () { // Generate a new password hash diff --git a/core/test/functional/base.js b/core/test/functional/base.js index 538c740d2f..e49d58931c 100644 --- a/core/test/functional/base.js +++ b/core/test/functional/base.js @@ -30,7 +30,7 @@ var DEBUG = false, // TOGGLE THIS TO GET MORE SCREENSHOTS url = 'http://' + host + (noPort ? '/' : ':' + port + '/'), newUser = { name: 'Test User', - slug: 'test-user', + slug: 'test', email: email, password: password }, @@ -90,7 +90,7 @@ screens = { selector: '.settings-menu-users.active' }, 'settings.users.user': { - url: 'ghost/settings/users/test-user', + url: 'ghost/settings/users/test', linkSelector: '.user-menu-profile', selector: '.user-profile' }, diff --git a/core/test/functional/client/settings_test.js b/core/test/functional/client/settings_test.js index 8433dfe793..bd0ae952fc 100644 --- a/core/test/functional/client/settings_test.js +++ b/core/test/functional/client/settings_test.js @@ -199,7 +199,7 @@ CasperTest.begin('Users screen is correct', 9, function suite(test) { CasperTest.begin('Can save settings', 7, function suite(test) { casper.thenOpenAndWaitForPageLoad('settings.users.user', function testTitleAndUrl() { test.assertTitle('Ghost Admin', 'Ghost Admin title is GhostAdmin'); - test.assertUrlMatch(/ghost\/settings\/users\/test-user\/$/, 'settings.users.user has correct URL'); + test.assertUrlMatch(/ghost\/settings\/users\/test\/$/, 'settings.users.user has correct URL'); }); function handleUserRequest(requestData) { @@ -274,7 +274,7 @@ CasperTest.begin('User settings screen resets all whitespace slug to original va casper.thenOpenAndWaitForPageLoad('settings.users.user', function testTitleAndUrl() { test.assertTitle('Ghost Admin', 'Ghost admin has no title'); - test.assertUrlMatch(/ghost\/settings\/users\/test-user\/$/, 'Ghost doesn\'t require login this time'); + test.assertUrlMatch(/ghost\/settings\/users\/test\/$/, 'Ghost doesn\'t require login this time'); }); casper.then(function setSlugToAllWhitespace() { @@ -298,7 +298,7 @@ CasperTest.begin('User settings screen change slug handles duplicate slug', 4, f casper.thenOpenAndWaitForPageLoad('settings.users.user', function testTitleAndUrl() { test.assertTitle('Ghost Admin', 'Ghost admin has no title'); - test.assertUrlMatch(/ghost\/settings\/users\/test-user\/$/, 'Ghost doesn\'t require login this time'); + test.assertUrlMatch(/ghost\/settings\/users\/test\/$/, 'Ghost doesn\'t require login this time'); }); casper.then(function changeSlug() { @@ -325,7 +325,7 @@ CasperTest.begin('User settings screen validates email', 6, function suite(test) casper.thenOpenAndWaitForPageLoad('settings.users.user', function testTitleAndUrl() { test.assertTitle('Ghost Admin', 'Ghost admin has no title'); - test.assertUrlMatch(/ghost\/settings\/users\/test-user\/$/, 'Ghost doesn\'t require login this time'); + test.assertUrlMatch(/ghost\/settings\/users\/test\/$/, 'Ghost doesn\'t require login this time'); }); casper.then(function setEmailToInvalid() { @@ -366,7 +366,7 @@ CasperTest.begin('User settings screen validates email', 6, function suite(test) CasperTest.begin('User settings screen shows remaining characters for Bio properly', 4, function suite(test) { casper.thenOpenAndWaitForPageLoad('settings.users.user', function testTitleAndUrl() { test.assertTitle('Ghost Admin', 'Ghost admin has no title'); - test.assertUrlMatch(/ghost\/settings\/users\/test-user\/$/, 'Ghost doesn\'t require login this time'); + test.assertUrlMatch(/ghost\/settings\/users\/test\/$/, 'Ghost doesn\'t require login this time'); }); function getRemainingBioCharacterCount() { @@ -391,7 +391,7 @@ CasperTest.begin('User settings screen shows remaining characters for Bio proper CasperTest.begin('Ensure user bio field length validation', 3, function suite(test) { casper.thenOpenAndWaitForPageLoad('settings.users.user', function testTitleAndUrl() { test.assertTitle('Ghost Admin', 'Ghost admin has no title'); - test.assertUrlMatch(/ghost\/settings\/users\/test-user\/$/, 'Ghost doesn\'t require login this time'); + test.assertUrlMatch(/ghost\/settings\/users\/test\/$/, 'Ghost doesn\'t require login this time'); }); casper.then(function setBioToInvalid() { @@ -410,7 +410,7 @@ CasperTest.begin('Ensure user bio field length validation', 3, function suite(te CasperTest.begin('Ensure user url field validation', 3, function suite(test) { casper.thenOpenAndWaitForPageLoad('settings.users.user', function testTitleAndUrl() { test.assertTitle('Ghost Admin', 'Ghost admin has no title'); - test.assertUrlMatch(/ghost\/settings\/users\/test-user\/$/, 'Ghost doesn\'t require login this time'); + test.assertUrlMatch(/ghost\/settings\/users\/test\/$/, 'Ghost doesn\'t require login this time'); }); casper.then(function setWebsiteToInvalid() { @@ -429,7 +429,7 @@ CasperTest.begin('Ensure user url field validation', 3, function suite(test) { CasperTest.begin('Ensure user location field length validation', 3, function suite(test) { casper.thenOpenAndWaitForPageLoad('settings.users.user', function testTitleAndUrl() { test.assertTitle('Ghost Admin', 'Ghost admin has no title'); - test.assertUrlMatch(/ghost\/settings\/users\/test-user\/$/, 'Ghost doesn\'t require login this time'); + test.assertUrlMatch(/ghost\/settings\/users\/test\/$/, 'Ghost doesn\'t require login this time'); }); casper.then(function setLocationToInvalid() { diff --git a/core/test/integration/api/api_slugs_spec.js b/core/test/integration/api/api_slugs_spec.js index c010904112..1cbf36c36c 100644 --- a/core/test/integration/api/api_slugs_spec.js +++ b/core/test/integration/api/api_slugs_spec.js @@ -39,7 +39,7 @@ describe('Slug API', function () { }); it('can generate user slug', function (done) { - SlugAPI.generate({context: {user: 1}, type: 'tag', name: 'user name'}) + SlugAPI.generate({context: {user: 1}, type: 'user', name: 'user name'}) .then(function (results) { should.exist(results); testUtils.API.checkResponse(results, 'slugs'); diff --git a/core/test/integration/model/model_tags_spec.js b/core/test/integration/model/model_tags_spec.js index 08f36492c4..8f5c29ba90 100644 --- a/core/test/integration/model/model_tags_spec.js +++ b/core/test/integration/model/model_tags_spec.js @@ -329,6 +329,41 @@ describe('Tag Model', function () { }).catch(done); }); + it('correctly creates non-conflicting slugs', function (done) { + var seededTagNames = ['tag1'], + postModel; + + seedTags(seededTagNames).then(function (_postModel) { + postModel = _postModel; + return TagModel.add({name: 'tagc'}, context); + }).then(function () { + // the tag API expects tags to be provided like {id: 1, name: 'draft'} + var tagData = []; + + tagData.push({id: 2, name: 'tagc'}); + tagData.push({id: 3, name: 'tagc++'}); + + postModel = postModel.toJSON(); + postModel.tags = tagData; + + return PostModel.edit(postModel, _.extend(context, {id: postModel.id, withRelated: ['tags']})); + }).then(function () { + return PostModel.findOne({id: postModel.id, status: 'all'}, {withRelated: ['tags']}); + }).then(function (reloadedPost) { + var tagModels = reloadedPost.related('tags').models, + tagSlugs = tagModels.map(function (t) { return t.get('slug'); }), + tagIds = _.pluck(tagModels, 'id'); + + tagSlugs.sort().should.eql(['tagc', 'tagc-2']); + + // make sure it hasn't just added a new tag with the same name + // Don't expect a certain order in results - check for number of items! + Math.max.apply(Math, tagIds).should.eql(3); + + done(); + }).catch(done); + }); + it('can add a tag to a post on creation', function (done) { var newPost = _.extend(testUtils.DataGenerator.forModel.posts[0], {tags: [{name: 'test_tag_1'}]}); diff --git a/core/test/integration/model/model_users_spec.js b/core/test/integration/model/model_users_spec.js index 7191c7057b..4a7f8c6da6 100644 --- a/core/test/integration/model/model_users_spec.js +++ b/core/test/integration/model/model_users_spec.js @@ -45,6 +45,50 @@ describe('User Model', function run() { }).catch(done); }); + it('shortens slug if possible', function (done) { + var userData = testUtils.DataGenerator.forModel.users[2]; + + sandbox.stub(UserModel, 'gravatarLookup', function (userData) { + return Promise.resolve(userData); + }); + + UserModel.add(userData, context).then(function (createdUser) { + should.exist(createdUser); + createdUser.has('slug').should.equal(true); + createdUser.attributes.slug.should.equal('jimothy'); + done(); + }).catch(done); + }); + + it('does not short slug if not possible', function (done) { + var userData = testUtils.DataGenerator.forModel.users[2]; + + sandbox.stub(UserModel, 'gravatarLookup', function (userData) { + return Promise.resolve(userData); + }); + + UserModel.add(userData, context).then(function (createdUser) { + should.exist(createdUser); + createdUser.has('slug').should.equal(true); + createdUser.attributes.slug.should.equal('jimothy'); + }).then(function () { + userData.email = 'newmail@mail.com'; + UserModel.add(userData, context).then(function (createdUser) { + should.exist(createdUser); + createdUser.has('slug').should.equal(true); + createdUser.attributes.slug.should.equal('jimothy-bogendath'); + }).then(function () { + userData.email = 'newmail2@mail.com'; + UserModel.add(userData, context).then(function (createdUser) { + should.exist(createdUser); + createdUser.has('slug').should.equal(true); + createdUser.attributes.slug.should.equal('jimothy-bogendath-2'); + done(); + }); + }); + }).catch(done); + }); + it('does NOT lowercase email', function (done) { var userData = testUtils.DataGenerator.forModel.users[2];