diff --git a/lib/db/feature-toggle-store.js b/lib/db/feature-toggle-store.js index c09fc470c0..4604039e14 100644 --- a/lib/db/feature-toggle-store.js +++ b/lib/db/feature-toggle-store.js @@ -51,12 +51,20 @@ class FeatureToggleStore { .then(this.rowToFeature); } - hasFeatureName(name) { + hasFeature(name) { return this.db - .first('name') + .first('name', 'archived') .from(TABLE) .where({ name }) - .then(n => !!n); + .then(row => { + if (!row) { + throw new NotFoundError('No feature toggle found'); + } + return { + name: row.name, + archived: row.archived === 1, + }; + }); } getArchivedFeatures() { diff --git a/lib/routes/admin-api/feature.js b/lib/routes/admin-api/feature.js index f3e09becb3..c20d7edc14 100644 --- a/lib/routes/admin-api/feature.js +++ b/lib/routes/admin-api/feature.js @@ -23,12 +23,7 @@ const handleErrors = (req, res, error) => { case NameExistsError: return res .status(403) - .json([ - { - msg: - 'A feature with this name already exists. Try re-activating it from the archive.', - }, - ]) + .json([{ msg: error.message }]) .end(); case ValidationError: return res @@ -97,13 +92,15 @@ module.exports.router = function(config) { function validateUniqueName(req) { return new Promise((resolve, reject) => { - featureToggleStore.hasFeatureName(req.body.name).then(hasName => { - if (hasName) { - reject(new NameExistsError('Feature name already exist')); - } else { - resolve(req); - } - }); + featureToggleStore + .hasFeature(req.body.name) + .then(definition => { + const msg = definition.archived + ? 'An archived toggle with that name already exist' + : 'A toggle with that name already exist'; + reject(new NameExistsError(msg)); + }) + .catch(() => resolve(req)); }); } diff --git a/lib/routes/admin-api/feature.test.js b/lib/routes/admin-api/feature.test.js index c540f3ef48..2a49bbdb45 100644 --- a/lib/routes/admin-api/feature.test.js +++ b/lib/routes/admin-api/feature.test.js @@ -81,6 +81,56 @@ test('should require at least one strategy when creating a feature toggle', t => .expect(400); }); +test('should be allowed to use new toggle name', t => { + t.plan(0); + const { request, base } = getSetup(); + + return request + .post(`${base}/api/admin/features/validate`) + .send({ name: 'new.name' }) + .set('Content-Type', 'application/json') + .expect(201); +}); + +test('should not be allowed to reuse active toggle name', t => { + t.plan(1); + const { request, featureToggleStore, base } = getSetup(); + featureToggleStore.addFeature({ + name: 'ts', + strategies: [{ name: 'default' }], + }); + + return request + .post(`${base}/api/admin/features/validate`) + .send({ name: 'ts' }) + .set('Content-Type', 'application/json') + .expect(403) + .expect(res => { + t.true(res.body[0].msg === 'A toggle with that name already exist'); + }); +}); + +test('should not be allowed to reuse archived toggle name', t => { + t.plan(1); + const { request, featureToggleStore, base } = getSetup(); + featureToggleStore.addArchivedFeature({ + name: 'ts.archived', + strategies: [{ name: 'default' }], + }); + + return request + .post(`${base}/api/admin/features/validate`) + .send({ name: 'ts.archived' }) + .set('Content-Type', 'application/json') + .expect(403) + .expect(res => { + t.true( + res.body[0].msg === + 'An archived toggle with that name already exist' + ); + }); +}); + test('should require at least one strategy when updating a feature toggle', t => { t.plan(0); const { request, featureToggleStore, base } = getSetup(); diff --git a/test/fixtures/fake-feature-toggle-store.js b/test/fixtures/fake-feature-toggle-store.js index 54aba77f1e..fdd4946f15 100644 --- a/test/fixtures/fake-feature-toggle-store.js +++ b/test/fixtures/fake-feature-toggle-store.js @@ -12,8 +12,18 @@ module.exports = () => { return Promise.reject(); } }, + hasFeature: name => { + const toggle = _features.find(f => f.name === name); + const archived = _archive.find(f => f.name === name); + if (toggle) { + return Promise.resolve({ name, archived: false }); + } else if (archived) { + return Promise.resolve({ name, archived: true }); + } else { + return Promise.reject(); + } + }, getFeatures: () => Promise.resolve(_features), - hasFeatureName: () => Promise.resolve(false), addFeature: feature => _features.push(feature), getArchivedFeatures: () => Promise.resolve(_archive), addArchivedFeature: feature => _archive.push(feature),