Merge branch '287846-create-edit-compliance-framework-page' into 'master'

Create edit compliance framework page

See merge request gitlab-org/gitlab!55096

ee/app/assets/javascripts/groups/settings/compliance_frameworks/components/list.vue

@@ -7,6 +7,7 @@ import { s__ } from '~/locale';
 
 import { DANGER, INFO } from '../constants';
 import getComplianceFrameworkQuery from '../graphql/queries/get_compliance_framework.query.graphql';
+import { injectIdIntoEditPath } from '../utils';
 import DeleteModal from './delete_modal.vue';
 import EmptyState from './list_empty_state.vue';
 import ListItem from './list_item.vue';
@@ -27,6 +28,10 @@ export default {
       type: String,
       required: true,
     },
+    editFrameworkPath: {
+      type: String,
+      required: true,
+    },
     emptyStateSvgPath: {
       type: String,
       required: true,
@@ -56,10 +61,15 @@ export default {
       update(data) {
         const nodes = data.namespace?.complianceFrameworks?.nodes;
         return (
-          nodes?.map((framework) => ({
-            ...framework,
-            parsedId: getIdFromGraphQLId(framework.id),
-          })) || []
+          nodes?.map((framework) => {
+            const parsedId = getIdFromGraphQLId(framework.id);
+
+            return {
+              ...framework,
+              parsedId,
+              editPath: injectIdIntoEditPath(this.editFrameworkPath, parsedId),
+            };
+          }) || []
         );
       },
       error(error) {

ee/app/assets/javascripts/groups/settings/compliance_frameworks/components/list_item.vue

 <script>
 import { GlLabel, GlButton, GlTooltipDirective } from '@gitlab/ui';
-import { s__ } from '~/locale';
+import { DELETE_BUTTON_LABEL, EDIT_BUTTON_LABEL } from '../constants';
 
 export default {
   directives: {
@@ -26,17 +26,18 @@ export default {
     },
   },
   i18n: {
-    deleteFramework: s__('ComplianceFrameworks|Delete framework'),
+    editFramework: EDIT_BUTTON_LABEL,
+    deleteFramework: DELETE_BUTTON_LABEL,
   },
 };
 </script>
 <template>
   <div
-    class="gl-display-flex gl-align-items-center gl-justify-content-space-between gl-inset-border-1-gray-100 gl-px-5 gl-p-6 gl-mb-4"
+    class="gl-display-flex gl-align-items-center gl-justify-content-space-between gl-inset-border-1-gray-100 gl-px-5 gl-p-6 gl-mb-4 gl-rounded-base"
   >
     <div class="gl-w-quarter gl-mr-3 gl-flex-shrink-0">
       <gl-label
-        target="#"
+        :target="framework.editPath"
         :background-color="framework.color"
         :title="framework.name"
         :scoped="isScoped"
@@ -46,9 +47,20 @@ export default {
     <p class="gl-w-full gl-m-0!" data-testid="compliance-framework-description">
       {{ framework.description }}
     </p>
-    <div>
+    <div class="gl-display-flex">
+      <gl-button
+        v-gl-tooltip="$options.i18n.editFramework"
+        :loading="loading"
+        :disabled="loading"
+        :aria-label="$options.i18n.editFramework"
+        :href="framework.editPath"
+        data-testid="compliance-framework-edit-button"
+        icon="pencil"
+        category="tertiary"
+      />
       <gl-button
         v-gl-tooltip="$options.i18n.deleteFramework"
+        class="gl-ml-3"
         :loading="loading"
         :disabled="loading"
         :aria-label="$options.i18n.deleteFramework"

ee/app/assets/javascripts/groups/settings/compliance_frameworks/constants.js

@@ -4,12 +4,16 @@ export const DANGER = 'danger';
 export const INFO = 'info';
 
 export const FETCH_ERROR = s__(
-  'ComplianceFrameworks|Error fetching compliance frameworks data. Please refresh the page',
+  'ComplianceFrameworks|Error fetching compliance frameworks data. Please refresh the page or try a different framework',
 );
-
 export const SAVE_ERROR = s__(
   'ComplianceFrameworks|Unable to save this compliance framework. Please try again',
 );
 
+export const EDIT_BUTTON_LABEL = s__('ComplianceFrameworks|Edit framework');
+export const DELETE_BUTTON_LABEL = s__('ComplianceFrameworks|Delete framework');
+
+export const EDIT_PATH_ID_FORMAT = /\/id\//;
+
 // Check that it matches the format [FILE].y(a)ml@[GROUP]/[PROJECT]
 export const PIPELINE_CONFIGURATION_PATH_FORMAT = /^([^@]*\.ya?ml)@([^/]*)\/(.*)$/;

ee/app/assets/javascripts/groups/settings/compliance_frameworks/init_list.js

@@ -15,7 +15,7 @@ const createComplianceFrameworksListApp = (el) => {
     return false;
   }
 
-  const { addFrameworkPath, emptyStateSvgPath, groupPath } = el.dataset;
+  const { addFrameworkPath, editFrameworkPath, emptyStateSvgPath, groupPath } = el.dataset;
 
   return new Vue({
     el,
@@ -24,6 +24,7 @@ const createComplianceFrameworksListApp = (el) => {
       return createElement(Form, {
         props: {
           addFrameworkPath,
+          editFrameworkPath,
           emptyStateSvgPath,
           groupPath,
         },

ee/app/assets/javascripts/groups/settings/compliance_frameworks/utils.js

 import Api from '~/api';
 import httpStatus from '~/lib/utils/http_status';
-import { PIPELINE_CONFIGURATION_PATH_FORMAT } from './constants';
+import { EDIT_PATH_ID_FORMAT, PIPELINE_CONFIGURATION_PATH_FORMAT } from './constants';
+
+const isNumeric = (value) => {
+  return !Number.isNaN(parseInt(value, 10));
+};
+
+export const injectIdIntoEditPath = (path, id) => {
+  if (!path.match(EDIT_PATH_ID_FORMAT) || !isNumeric(id)) {
+    return '';
+  }
+
+  return path.replace(EDIT_PATH_ID_FORMAT, `/${id}/`);
+};
 
 export const initialiseFormData = () => ({
   name: null,

ee/app/assets/javascripts/pages/groups/compliance_frameworks/edit/index.js

+import { createComplianceFrameworksFormApp } from 'ee/groups/settings/compliance_frameworks/init_form';
+
+createComplianceFrameworksFormApp(document.getElementById('js-compliance-frameworks-form'));

ee/app/controllers/groups/compliance_frameworks_controller.rb

@@ -11,6 +11,9 @@ class Groups::ComplianceFrameworksController < Groups::ApplicationController
   def new
   end
 
+  def edit
+  end
+
   protected
 
   def check_group_compliance_frameworks_available!

ee/app/helpers/compliance_management/compliance_framework/group_settings_helper.rb

@@ -3,31 +3,33 @@
 module ComplianceManagement
   module ComplianceFramework
     module GroupSettingsHelper
-      def show_compliance_frameworks?
-        can?(current_user, :admin_compliance_framework, @group)
+      def show_compliance_frameworks?(group)
+        can?(current_user, :admin_compliance_framework, group)
       end
 
-      def compliance_frameworks_list_data
+      def compliance_frameworks_list_data(group)
         {
           empty_state_svg_path: image_path('illustrations/welcome/ee_trial.svg'),
-          group_path: @group.full_path,
-          add_framework_path: new_group_compliance_framework_path(@group)
+          group_path: group.full_path,
+          add_framework_path: new_group_compliance_framework_path(group),
+          edit_framework_path: edit_group_compliance_framework_path(group, :id)
         }
       end
 
-      def compliance_frameworks_new_form_data
+      def compliance_frameworks_form_data(group, framework_id = nil)
         {
-          group_path: @group.full_path,
-          group_edit_path: edit_group_path(@group, anchor: 'js-compliance-frameworks-settings'),
+          framework_id: framework_id,
+          group_path: group.full_path,
+          group_edit_path: edit_group_path(group, anchor: 'js-compliance-frameworks-settings'),
           graphql_field_name: ComplianceManagement::Framework.name,
-          pipeline_configuration_full_path_enabled: pipeline_configuration_full_path_enabled?.to_s
+          pipeline_configuration_full_path_enabled: pipeline_configuration_full_path_enabled?(group).to_s
         }
       end
 
       private
 
-      def pipeline_configuration_full_path_enabled?
-        can?(current_user, :admin_compliance_pipeline_configuration, @group)
+      def pipeline_configuration_full_path_enabled?(group)
+        can?(current_user, :admin_compliance_pipeline_configuration, group)
       end
     end
   end

ee/app/views/groups/_compliance_frameworks.html.haml

 - expanded = expanded_by_default?
 
-- if show_compliance_frameworks?
+- if show_compliance_frameworks?(@group)
   %section.settings.no-animate#js-compliance-frameworks-settings{ class: ('expanded' if expanded) }
     .settings-header
       %h4
@@ -10,4 +10,4 @@
       %p
         = s_('GroupSettings|Configure frameworks to apply enforceable rules to projects.')
     .settings-content
-      #js-compliance-frameworks-list{ data: compliance_frameworks_list_data }
+      #js-compliance-frameworks-list{ data: compliance_frameworks_list_data(@group) }

ee/app/views/groups/compliance_frameworks/edit.html.haml

+- add_to_breadcrumbs _('General Settings'), edit_group_path(@group)
+- title = s_('ComplianceFramework|Edit Compliance Framework')
+- page_title title
+
+%h3.page-title= title
+
+#js-compliance-frameworks-form{ data: compliance_frameworks_form_data(@group, params[:id]) }

ee/app/views/groups/compliance_frameworks/new.html.haml

@@ -4,4 +4,4 @@
 
 %h3.page-title= title
 
-#js-compliance-frameworks-form{ data: compliance_frameworks_new_form_data }
+#js-compliance-frameworks-form{ data: compliance_frameworks_form_data(@group) }

ee/config/routes/group.rb

@@ -11,7 +11,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
       patch :override, on: :member
     end
 
-    resources :compliance_frameworks, only: [:new]
+    resources :compliance_frameworks, only: [:new, :edit]
 
     get '/analytics', to: redirect('groups/%{group_id}/-/analytics/value_stream_analytics')
     resource :contribution_analytics, only: [:show]

ee/spec/frontend/groups/settings/compliance_frameworks/components/list_item_spec.js

@@ -2,33 +2,59 @@ import { GlLabel } from '@gitlab/ui';
 import { shallowMount } from '@vue/test-utils';
 
 import ListItem from 'ee/groups/settings/compliance_frameworks/components/list_item.vue';
+import {
+  DELETE_BUTTON_LABEL,
+  EDIT_BUTTON_LABEL,
+} from 'ee/groups/settings/compliance_frameworks/constants';
 import { createMockDirective, getBinding } from 'helpers/vue_mock_directive';
+import { extendedWrapper } from 'helpers/vue_test_utils_helper';
 
 describe('ListItem', () => {
   let wrapper;
 
-  const framework = { name: 'framework', description: 'a framework', color: '#112233' };
-  const findLabel = () => wrapper.find(GlLabel);
-  const findDescription = () => wrapper.find('[data-testid="compliance-framework-description"]');
-  const findDeleteButton = () => wrapper.find('[data-testid="compliance-framework-delete-button"]');
+  const framework = {
+    parsedId: 1,
+    name: 'framework',
+    description: 'a framework',
+    color: '#112233',
+    editPath: 'group/framework/1/edit',
+  };
+  const findLabel = () => wrapper.findComponent(GlLabel);
+  const findDescription = () => wrapper.findByTestId('compliance-framework-description');
+  const findEditButton = () => wrapper.findByTestId('compliance-framework-edit-button');
+  const findDeleteButton = () => wrapper.findByTestId('compliance-framework-delete-button');
 
   const createComponent = (props = {}) => {
-    wrapper = shallowMount(ListItem, {
-      propsData: {
-        framework,
-        loading: false,
-        ...props,
-      },
-      directives: {
-        GlTooltip: createMockDirective(),
-      },
-    });
+    wrapper = extendedWrapper(
+      shallowMount(ListItem, {
+        propsData: {
+          framework,
+          loading: false,
+          ...props,
+        },
+        directives: {
+          GlTooltip: createMockDirective(),
+        },
+      }),
+    );
   };
 
   afterEach(() => {
     wrapper.destroy();
   });
 
+  const displaysTheButton = (button, icon, ariaLabel) => {
+    expect(button.props('icon')).toBe(icon);
+    expect(button.props('disabled')).toBe(false);
+    expect(button.props('loading')).toBe(false);
+    expect(button.attributes('aria-label')).toBe(ariaLabel);
+  };
+
+  const disablesTheButton = (button) => {
+    expect(button.props('disabled')).toBe(true);
+    expect(button.props('loading')).toBe(true);
+  };
+
   it('displays the description defined by the framework', () => {
     createComponent();
 
@@ -39,6 +65,7 @@ describe('ListItem', () => {
     createComponent();
 
     expect(findLabel().props('title')).toBe('framework');
+    expect(findLabel().props('target')).toBe(framework.editPath);
     expect(findLabel().props('scoped')).toBe(false);
   });
 
@@ -46,20 +73,27 @@ describe('ListItem', () => {
     createComponent({ framework: { ...framework, name: 'scoped::framework' } });
 
     expect(findLabel().props('title')).toBe('scoped::framework');
+    expect(findLabel().props('target')).toBe(framework.editPath);
     expect(findLabel().props('scoped')).toBe(true);
     expect(findLabel().props('disabled')).toBe(false);
   });
 
+  it('displays the edit button', () => {
+    createComponent();
+
+    const button = findEditButton();
+
+    displaysTheButton(button, 'pencil', EDIT_BUTTON_LABEL);
+    expect(button.attributes('href')).toBe('group/framework/1/edit');
+  });
+
   it('displays a delete button', () => {
     createComponent();
 
     const button = findDeleteButton();
     const tooltip = getBinding(button.element, 'gl-tooltip');
 
-    expect(button.props('icon')).toBe('remove');
-    expect(button.props('disabled')).toBe(false);
-    expect(button.props('loading')).toBe(false);
-    expect(button.attributes('aria-label')).toBe('Delete framework');
+    displaysTheButton(button, 'remove', DELETE_BUTTON_LABEL);
     expect(tooltip.value).toBe('Delete framework');
   });
 
@@ -81,8 +115,11 @@ describe('ListItem', () => {
     });
 
     it('disables the delete button and shows loading', () => {
-      expect(findDeleteButton().props('disabled')).toBe(true);
-      expect(findDeleteButton().props('loading')).toBe(true);
+      disablesTheButton(findDeleteButton());
+    });
+
+    it('disables the edit button and shows loading', () => {
+      disablesTheButton(findEditButton());
     });
   });
 });

ee/spec/frontend/groups/settings/compliance_frameworks/components/list_spec.js

@@ -27,14 +27,14 @@ describe('List', () => {
   const fetchLoading = jest.fn().mockResolvedValue(new Promise(() => {}));
   const fetchWithErrors = jest.fn().mockRejectedValue(sentryError);
 
-  const findAlert = () => wrapper.find(GlAlert);
+  const findAlert = () => wrapper.findComponent(GlAlert);
   const findDeleteModal = () => wrapper.findComponent(DeleteModal);
-  const findLoadingIcon = () => wrapper.find(GlLoadingIcon);
-  const findEmptyState = () => wrapper.find(EmptyState);
-  const findTabs = () => wrapper.findAll(GlTab);
-  const findAddBtn = () => wrapper.find(GlButton);
-  const findTabsContainer = () => wrapper.find(GlTabs);
-  const findListItems = () => wrapper.findAll(ListItem);
+  const findLoadingIcon = () => wrapper.findComponent(GlLoadingIcon);
+  const findEmptyState = () => wrapper.findComponent(EmptyState);
+  const findTabs = () => wrapper.findAllComponents(GlTab);
+  const findAddBtn = () => wrapper.findComponent(GlButton);
+  const findTabsContainer = () => wrapper.findComponent(GlTabs);
+  const findListItems = () => wrapper.findAllComponents(ListItem);
 
   function createMockApolloProvider(resolverMock) {
     localVue.use(VueApollo);
@@ -50,6 +50,7 @@ describe('List', () => {
       apolloProvider: createMockApolloProvider(resolverMock),
       propsData: {
         addFrameworkPath: 'group/framework/new',
+        editFrameworkPath: 'group/framework/id/edit',
         emptyStateSvgPath: 'dir/image.svg',
         groupPath: 'group-1',
       },
@@ -170,7 +171,7 @@ describe('List', () => {
       expect(findListItems()).toHaveLength(2);
 
       findListItems().wrappers.forEach((item) =>
-        expect(item.props()).toEqual(
+        expect(item.props()).toStrictEqual(
           expect.objectContaining({
             framework: {
               id: expect.stringContaining('gid://gitlab/ComplianceManagement::Framework/'),
@@ -181,6 +182,7 @@ describe('List', () => {
                 PIPELINE_CONFIGURATION_PATH_FORMAT,
               ),
               color: expect.stringMatching(/^#([0-9A-F]{3}){1,2}$/i),
+              editPath: expect.stringMatching(/^group\/framework\/[0-9+]\/edit$/i),
             },
             loading: false,
           }),

ee/spec/frontend/groups/settings/compliance_frameworks/utils_spec.js

@@ -16,6 +16,20 @@ describe('Utils', () => {
     mock.restore();
   });
 
+  describe('injectIdIntoEditPath', () => {
+    it.each`
+      path                          | id           | output
+      ${'group/framework/abc/edit'} | ${1}         | ${''}
+      ${'group/framework/id/edit'}  | ${undefined} | ${''}
+      ${'group/framework/id/edit'}  | ${null}      | ${''}
+      ${'group/framework/id/edit'}  | ${'abc'}     | ${''}
+      ${'group/framework/id/edit'}  | ${'1'}       | ${'group/framework/1/edit'}
+      ${'group/framework/id/edit'}  | ${1}         | ${'group/framework/1/edit'}
+    `('should return $output when $path and $id are given', ({ path, id, output }) => {
+      expect(Utils.injectIdIntoEditPath(path, id)).toStrictEqual(output);
+    });
+  });
+
   describe('initialiseFormData', () => {
     it('returns the initial form data object', () => {
       expect(Utils.initialiseFormData()).toStrictEqual({

ee/spec/helpers/compliance_management/compliance_framework/group_settings_helper_spec.rb

@@ -7,12 +7,11 @@ RSpec.describe ComplianceManagement::ComplianceFramework::GroupSettingsHelper do
   let_it_be(:current_user) { build(:admin) }
 
   before do
-    assign(:group, group)
     allow(helper).to receive(:current_user) { current_user }
   end
 
   describe '#show_compliance_frameworks?' do
-    subject { helper.show_compliance_frameworks? }
+    subject { helper.show_compliance_frameworks?(group) }
 
     context 'the user has permission' do
       before do
@@ -33,30 +32,48 @@ RSpec.describe ComplianceManagement::ComplianceFramework::GroupSettingsHelper do
 
   describe '#compliance_frameworks_list_data' do
     it 'returns the correct data' do
-      expect(helper.compliance_frameworks_list_data).to contain_exactly(
+      expect(helper.compliance_frameworks_list_data(group)).to contain_exactly(
         [:empty_state_svg_path, ActionController::Base.helpers.image_path('illustrations/welcome/ee_trial.svg')],
         [:group_path, group.full_path],
-        [:add_framework_path, new_group_compliance_framework_path(group)]
+        [:add_framework_path, new_group_compliance_framework_path(group)],
+        [:edit_framework_path, edit_group_compliance_framework_path(group, :id)]
       )
     end
   end
 
-  describe '#compliance_frameworks_new_form_data' do
-    subject { helper.compliance_frameworks_new_form_data }
+  describe '#compliance_frameworks_form_data' do
+    let(:framework_id) { nil }
+
+    subject { helper.compliance_frameworks_form_data(group, framework_id) }
 
     shared_examples 'returns the correct data' do |pipeline_configuration_enabled|
       before do
         allow(helper).to receive(:can?).with(current_user, :admin_compliance_pipeline_configuration, group).and_return(pipeline_configuration_enabled)
       end
 
-      it {
+      it 'does not contain a framework ID' do
         is_expected.to contain_exactly(
+          [:framework_id, nil],
           [:group_path, group.full_path],
           [:group_edit_path, edit_group_path(group, anchor: 'js-compliance-frameworks-settings')],
           [:graphql_field_name, ComplianceManagement::Framework.name],
           [:pipeline_configuration_full_path_enabled, pipeline_configuration_enabled.to_s]
         )
-      }
+      end
+
+      context 'with a framework ID' do
+        let(:framework_id) { 12345 }
+
+        it {
+          is_expected.to contain_exactly(
+            [:framework_id, framework_id],
+            [:group_path, group.full_path],
+            [:group_edit_path, edit_group_path(group, anchor: 'js-compliance-frameworks-settings')],
+            [:graphql_field_name, ComplianceManagement::Framework.name],
+            [:pipeline_configuration_full_path_enabled, pipeline_configuration_enabled.to_s]
+          )
+        }
+      end
     end
 
     context 'the user has pipeline configuration permission' do

ee/spec/requests/groups/compliance_frameworks_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 RSpec.describe 'group compliance frameworks' do
   let_it_be(:group) { create(:group) }
   let_it_be(:user) { create(:user) }
+  let_it_be(:framework) { create(:compliance_framework, namespace: group, name: 'Framework') }
 
   before do
     login_as(user)
@@ -23,6 +24,14 @@ RSpec.describe 'group compliance frameworks' do
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
+
+    describe 'GET /groups/:group/-/compliance_frameworks/:id/edit' do
+      it 'returns 404 not found' do
+        get edit_group_compliance_framework_path(group, framework)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
   end
 
   context 'when compliance frameworks feature is enabled' do
@@ -47,5 +56,22 @@ RSpec.describe 'group compliance frameworks' do
         end
       end
     end
+
+    describe 'GET /groups/:group/-/compliance_frameworks/:id/edit' do
+      it 'renders template' do
+        group.add_owner(user)
+        get edit_group_compliance_framework_path(group, framework)
+
+        expect(response).to render_template 'groups/compliance_frameworks/edit'
+      end
+
+      context 'with unauthorized user' do
+        it 'returns 404 not found' do
+          get edit_group_compliance_framework_path(group, framework)
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
   end
 end

ee/spec/views/groups/compliance_frameworks/edit.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'groups/compliance_frameworks/edit.html.haml' do
+  let_it_be(:group) { build(:group) }
+  let_it_be(:user) { build(:user) }
+
+  before do
+    assign(:group, group)
+
+    allow(view).to receive(:current_user).and_return(user)
+    allow(user).to receive(:can?).with(:admin_compliance_pipeline_configuration, group).and_return(true)
+    allow(view).to receive(:params).and_return(id: 1)
+  end
+
+  it 'shows the compliance frameworks form', :aggregate_failures do
+    render
+
+    expect(rendered).to have_content('Edit Compliance Framework')
+    expect(rendered).to have_css('#js-compliance-frameworks-form')
+    expect(rendered).to have_css('[data-framework-id="1"]')
+  end
+end

locale/gitlab.pot

@@ -7729,12 +7729,18 @@ msgstr ""
 msgid "ComplianceFrameworks|Delete framework"
 msgstr ""
 
+msgid "ComplianceFrameworks|Edit framework"
+msgstr ""
+
 msgid "ComplianceFrameworks|Error deleting the compliance framework. Please try again"
 msgstr ""
 
 msgid "ComplianceFrameworks|Error fetching compliance frameworks data. Please refresh the page"
 msgstr ""
 
+msgid "ComplianceFrameworks|Error fetching compliance frameworks data. Please refresh the page or try a different framework"
+msgstr ""
+
 msgid "ComplianceFrameworks|Invalid format: it should follow the format [PATH].y(a)ml@[GROUP]/[PROJECT]"
 msgstr ""
 
@@ -7759,6 +7765,9 @@ msgstr ""
 msgid "ComplianceFrameworks|e.g. include-gitlab.ci.yml@group-name/project-name"
 msgstr ""
 
+msgid "ComplianceFramework|Edit Compliance Framework"
+msgstr ""
+
 msgid "ComplianceFramework|GDPR"
 msgstr ""