From 890f11e608523f4487bc3e5cd78f2875d6d0244a Mon Sep 17 00:00:00 2001
From: Filipa Lacerda <filipa@gitlab.com>
Date: Mon, 11 Mar 2019 12:46:26 +0000
Subject: [PATCH] Fixes broken form for feature flags

The edit/remove feature was updating the scopes
based on the array's index.

This commit creates an internal id and uses it
to update the scopes.

Removes the internal id before sending data
to the server
---
 .../feature_flags/components/form.vue         | 29 +++++++----
 .../feature_flags/store/modules/helpers.js    | 13 ++++-
 ee/changelogs/unreleased/10233-ff-form.yml    |  5 ++
 .../feature_flags/components/form_spec.js     | 48 ++++++++++---------
 .../feature_flags/store/helpers_spec.js       | 42 ++++++++++++++++
 5 files changed, 104 insertions(+), 33 deletions(-)
 create mode 100644 ee/changelogs/unreleased/10233-ff-form.yml
 create mode 100644 ee/spec/javascripts/feature_flags/store/helpers_spec.js

diff --git a/ee/app/assets/javascripts/feature_flags/components/form.vue b/ee/app/assets/javascripts/feature_flags/components/form.vue
index 70033c2e251beb..93cebf67c2b7e1 100644
--- a/ee/app/assets/javascripts/feature_flags/components/form.vue
+++ b/ee/app/assets/javascripts/feature_flags/components/form.vue
@@ -1,9 +1,11 @@
 <script>
+import _ from 'underscore';
 import { GlButton } from '@gitlab/ui';
 import { s__, sprintf } from '~/locale';
 import ToggleButton from '~/vue_shared/components/toggle_button.vue';
 import Icon from '~/vue_shared/components/icon.vue';
 import EnvironmentsDropdown from './environments_dropdown.vue';
+import { internalKeyID } from '../store/modules/helpers';
 
 export default {
   components: {
@@ -83,7 +85,8 @@ export default {
      * @param {Number} index
      * @param {Boolean} status
      */
-    onUpdateScopeStatus(scope, index, status) {
+    onUpdateScopeStatus(scope, status) {
+      const index = this.formScopes.findIndex(el => el.id === scope.id);
       this.formScopes.splice(index, 1, Object.assign({}, scope, { active: status }));
     },
     /**
@@ -94,7 +97,8 @@ export default {
      * @param {Object} scope
      * @param {Number} index
      */
-    updateScope(name, scope, index) {
+    updateScope(name, scope) {
+      const index = this.formScopes.findIndex(el => el.id === scope.id);
       this.formScopes.splice(index, 1, Object.assign({}, scope, { environment_scope: name }));
     },
     /**
@@ -107,6 +111,7 @@ export default {
       this.formScopes.push({
         active: value,
         environment_scope: this.newScope,
+        id: _.uniqueId(internalKeyID),
       });
 
       this.newScope = '';
@@ -121,13 +126,15 @@ export default {
      * @param {Number} index
      * @param {Object} scope
      */
-    removeScope(index, scope) {
-      if (scope.id) {
-        this.formScopes.splice(index, 1, Object.assign({}, scope, { _destroy: true }));
-      } else {
+    removeScope(scope) {
+      const index = this.formScopes.findIndex(el => el.id === scope.id);
+      if (_.isString(scope.id) && scope.id.indexOf(internalKeyID) !== -1) {
         this.formScopes.splice(index, 1);
+      } else {
+        this.formScopes.splice(index, 1, Object.assign({}, scope, { _destroy: true }));
       }
     },
+
     /**
      * When the user selects a value or creates a new value in the environments
      * dropdown in the creation row, we push a new entry with the selected value.
@@ -135,7 +142,11 @@ export default {
      * @param {String}
      */
     createNewEnvironment(name) {
-      this.formScopes.push({ environment_scope: name, active: false });
+      this.formScopes.push({
+        environment_scope: name,
+        active: false,
+        id: _.uniqueId(internalKeyID),
+      });
       this.newScope = '';
     },
     /**
@@ -225,7 +236,7 @@ export default {
                 <div class="table-mobile-content js-feature-flag-status">
                   <toggle-button
                     :value="scope.active"
-                    @change="status => onUpdateScopeStatus(scope, index, status)"
+                    @change="status => onUpdateScopeStatus(scope, status)"
                   />
                 </div>
               </div>
@@ -238,7 +249,7 @@ export default {
                   <gl-button
                     v-if="!isAllEnvironment(scope.environment_scope)"
                     class="js-delete-scope btn-transparent"
-                    @click="removeScope(index, scope)"
+                    @click="removeScope(scope)"
                   >
                     <icon name="clear" />
                   </gl-button>
diff --git a/ee/app/assets/javascripts/feature_flags/store/modules/helpers.js b/ee/app/assets/javascripts/feature_flags/store/modules/helpers.js
index 37b2f759729c17..9e7c5c0124fcb7 100644
--- a/ee/app/assets/javascripts/feature_flags/store/modules/helpers.js
+++ b/ee/app/assets/javascripts/feature_flags/store/modules/helpers.js
@@ -1,8 +1,17 @@
-// eslint-disable-next-line import/prefer-default-export
+import _ from 'underscore';
+
+export const internalKeyID = 'internal_';
+
 export const parseFeatureFlagsParams = params => ({
   operations_feature_flag: {
     name: params.name,
     description: params.description,
-    scopes_attributes: params.scopes,
+    scopes_attributes: params.scopes.map(scope => {
+      const scopeCopy = Object.assign({}, scope);
+      if (_.isString(scopeCopy.id) && scopeCopy.id.indexOf(internalKeyID) !== -1) {
+        delete scopeCopy.id;
+      }
+      return scopeCopy;
+    }),
   },
 });
diff --git a/ee/changelogs/unreleased/10233-ff-form.yml b/ee/changelogs/unreleased/10233-ff-form.yml
new file mode 100644
index 00000000000000..aac6c26dc76235
--- /dev/null
+++ b/ee/changelogs/unreleased/10233-ff-form.yml
@@ -0,0 +1,5 @@
+---
+title: Fixes Broken new/edit feature flag form
+merge_request:
+author:
+type: fixed
diff --git a/ee/spec/javascripts/feature_flags/components/form_spec.js b/ee/spec/javascripts/feature_flags/components/form_spec.js
index f62bceb314f02b..0318281b0d34c4 100644
--- a/ee/spec/javascripts/feature_flags/components/form_spec.js
+++ b/ee/spec/javascripts/feature_flags/components/form_spec.js
@@ -1,7 +1,9 @@
+import _ from 'underscore';
 import { createLocalVue, mount } from '@vue/test-utils';
 import Form from 'ee/feature_flags/components/form.vue';
 import ToggleButton from '~/vue_shared/components/toggle_button.vue';
 import EnvironmentsDropdown from 'ee/feature_flags/components/environments_dropdown.vue';
+import { internalKeyID } from 'ee/feature_flags/store/modules/helpers';
 
 describe('feature flag form', () => {
   let wrapper;
@@ -64,7 +66,10 @@ describe('feature flag form', () => {
           it('should add a new scope with the text value empty and the status', () => {
             wrapper.find(ToggleButton).vm.$emit('change', true);
 
-            expect(wrapper.vm.formScopes).toEqual([{ active: true, environment_scope: '' }]);
+            expect(wrapper.vm.formScopes.length).toEqual(1);
+            expect(wrapper.vm.formScopes[0].active).toEqual(true);
+            expect(wrapper.vm.formScopes[0].environment_scope).toEqual('');
+
             expect(wrapper.vm.newScope).toEqual('');
           });
         });
@@ -84,6 +89,11 @@ describe('feature flag form', () => {
             active: false,
             id: 2,
           },
+          {
+            environment_scope: 'review',
+            active: true,
+            id: 4,
+          },
         ],
       });
     });
@@ -104,6 +114,7 @@ describe('feature flag form', () => {
 
             expect(wrapper.vm.formScopes).toEqual([
               { active: true, environment_scope: 'production', id: 2 },
+              { active: true, environment_scope: 'review', id: 4 },
             ]);
 
             expect(wrapper.vm.newScope).toEqual('');
@@ -124,11 +135,14 @@ describe('feature flag form', () => {
               _destroy: true,
               id: 2,
             },
+            { active: true, environment_scope: 'review', id: 4 },
           ]);
         });
 
         it('should not render deleted scopes', () => {
-          expect(wrapper.vm.filteredScopes).toEqual([]);
+          expect(wrapper.vm.filteredScopes).toEqual([
+            { active: true, environment_scope: 'review', id: 4 },
+          ]);
         });
       });
 
@@ -142,6 +156,7 @@ describe('feature flag form', () => {
               {
                 environment_scope: 'new_scope',
                 active: false,
+                id: _.uniqueId(internalKeyID),
               },
             ],
           });
@@ -203,26 +218,15 @@ describe('feature flag form', () => {
 
         wrapper.vm.handleSubmit();
 
-        expect(wrapper.emitted().handleSubmit[0]).toEqual([
-          {
-            name: 'feature_flag_2',
-            description: 'this is a feature flag',
-            scopes: [
-              {
-                environment_scope: 'production',
-                active: false,
-              },
-              {
-                environment_scope: 'review',
-                active: false,
-              },
-              {
-                environment_scope: '',
-                active: true,
-              },
-            ],
-          },
-        ]);
+        const data = wrapper.emitted().handleSubmit[0][0];
+
+        expect(data.name).toEqual('feature_flag_2');
+        expect(data.description).toEqual('this is a feature flag');
+        expect(data.scopes.length).toEqual(3);
+        expect(data.scopes[0]).toEqual({
+          active: false,
+          environment_scope: 'production',
+        });
       });
     });
   });
diff --git a/ee/spec/javascripts/feature_flags/store/helpers_spec.js b/ee/spec/javascripts/feature_flags/store/helpers_spec.js
new file mode 100644
index 00000000000000..04478f53b81353
--- /dev/null
+++ b/ee/spec/javascripts/feature_flags/store/helpers_spec.js
@@ -0,0 +1,42 @@
+import _ from 'underscore';
+import { parseFeatureFlagsParams, internalKeyID } from 'ee/feature_flags/store/modules/helpers';
+
+describe('feature flags helpers spec', () => {
+  describe('parseFeatureFlagsParams', () => {
+    describe('with internalKeyId', () => {
+      it('removes id', () => {
+        const scopes = [
+          {
+            active: true,
+            created_at: '2019-01-17T17:22:07.625Z',
+            environment_scope: '*',
+            id: 2,
+            updated_at: '2019-01-17T17:22:07.625Z',
+          },
+          {
+            active: true,
+            created_at: '2019-03-11T11:18:42.709Z',
+            environment_scope: 'review',
+            id: 29,
+            updated_at: '2019-03-11T11:18:42.709Z',
+          },
+          {
+            active: true,
+            created_at: '2019-03-11T11:18:42.709Z',
+            environment_scope: 'review',
+            id: _.uniqueId(internalKeyID),
+            updated_at: '2019-03-11T11:18:42.709Z',
+          },
+        ];
+
+        const parsedScopes = parseFeatureFlagsParams({
+          name: 'review',
+          scopes,
+          description: 'feature flag',
+        });
+
+        expect(parsedScopes.operations_feature_flag.scopes_attributes[2].id).toEqual(undefined);
+      });
+    });
+  });
+});
-- 
GitLab