Merge branch '10233-ff-form' into 'master'

Fixes broken form for feature flags

Closes #10233

See merge request gitlab-org/gitlab-ee!9953

ee/app/assets/javascripts/feature_flags/components/form.vue

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

ee/app/assets/javascripts/feature_flags/store/modules/helpers.js

-// 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;
+    }),
   },
 });

ee/changelogs/unreleased/10233-ff-form.yml

+---
+title: Fixes Broken new/edit feature flag form
+merge_request:
+author:
+type: fixed

ee/spec/javascripts/feature_flags/components/form_spec.js

+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',
+        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: '',
-                active: true,
-              },
-            ],
-          },
-        ]);
+          environment_scope: 'production',
+        });
       });
     });
   });

ee/spec/javascripts/feature_flags/store/helpers_spec.js

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