Merge pull request #292 from Unleash/fix-name-validation

Bugfix: more informative name validation errors

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() {

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));
         });
     }
 

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();

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),