From c544f81fba71befb7e5731614bc5e21e04daaeb7 Mon Sep 17 00:00:00 2001
From: ivaosthu <ivar.conradi.osthus@finn.no>
Date: Sat, 20 Jan 2018 13:28:04 +0100
Subject: [PATCH] Bugfix: more informative name validation errors

Will know tell the user if the toggle name is already
in use by an active feature toggle or an archived toggle.

Also brings lates unleash-frontend fix, which prevents an
invalid form from submitting.

closes: #290, #291
---
 lib/db/feature-toggle-store.js             | 14 ++++--
 lib/routes/admin-api/feature.js            | 23 +++++-----
 lib/routes/admin-api/feature.test.js       | 50 ++++++++++++++++++++++
 test/fixtures/fake-feature-toggle-store.js | 12 +++++-
 4 files changed, 82 insertions(+), 17 deletions(-)

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