Merge branch...

Merge branch '346059-create-configuration-security-training-add-primary-training-support' into 'master'

Add primary training support to security training config

See merge request gitlab-org/gitlab!81204

app/assets/javascripts/security_configuration/components/training_provider_list.vue

@@ -2,7 +2,7 @@
 import { GlAlert, GlCard, GlToggle, GlLink, GlSkeletonLoader } from '@gitlab/ui';
 import * as Sentry from '@sentry/browser';
 import Tracking from '~/tracking';
-import { __ } from '~/locale';
+import { __, s__ } from '~/locale';
 import {
   TRACK_TOGGLE_TRAINING_PROVIDER_ACTION,
   TRACK_TOGGLE_TRAINING_PROVIDER_LABEL,
@@ -10,9 +10,12 @@ import {
   TRACK_PROVIDER_LEARN_MORE_CLICK_LABEL,
 } from '~/security_configuration/constants';
 import dismissUserCalloutMutation from '~/graphql_shared/mutations/dismiss_user_callout.mutation.graphql';
-import { updateSecurityTrainingOptimisticResponse } from '~/security_configuration/graphql/utils/optimistic_response';
-import securityTrainingProvidersQuery from '../graphql/security_training_providers.query.graphql';
-import configureSecurityTrainingProvidersMutation from '../graphql/configure_security_training_providers.mutation.graphql';
+import securityTrainingProvidersQuery from '~/security_configuration/graphql/security_training_providers.query.graphql';
+import configureSecurityTrainingProvidersMutation from '~/security_configuration/graphql/configure_security_training_providers.mutation.graphql';
+import {
+  updateSecurityTrainingCache,
+  updateSecurityTrainingOptimisticResponse,
+} from '~/security_configuration/graphql/cache_utils';
 
 const i18n = {
   providerQueryErrorMessage: __(
@@ -21,6 +24,7 @@ const i18n = {
   configMutationErrorMessage: __(
     'Could not save configuration. Please refresh the page, or try again later.',
   ),
+  primaryTraining: s__('SecurityTraining|Primary Training'),
 };
 
 export default {
@@ -57,6 +61,9 @@ export default {
     };
   },
   computed: {
+    enabledProviders() {
+      return this.securityTrainingProviders.filter(({ isEnabled }) => isEnabled);
+    },
     isLoading() {
       return this.$apollo.queries.securityTrainingProviders.loading;
     },
@@ -91,14 +98,42 @@ export default {
         Sentry.captureException(e);
       }
     },
-    toggleProvider(provider) {
-      const { isEnabled } = provider;
+    async toggleProvider(provider) {
+      const { isEnabled, isPrimary } = provider;
       const toggledIsEnabled = !isEnabled;
 
       this.trackProviderToggle(provider.id, toggledIsEnabled);
-      this.storeProvider({ ...provider, isEnabled: toggledIsEnabled });
+
+      // when the current primary provider gets disabled then set the first enabled to be the new primary
+      if (!toggledIsEnabled && isPrimary && this.enabledProviders.length > 1) {
+        const firstOtherEnabledProvider = this.enabledProviders.find(
+          ({ id }) => id !== provider.id,
+        );
+        this.setPrimaryProvider(firstOtherEnabledProvider);
+      }
+
+      this.storeProvider({
+        ...provider,
+        isEnabled: toggledIsEnabled,
+      });
+    },
+    setPrimaryProvider(provider) {
+      this.storeProvider({ ...provider, isPrimary: true });
     },
-    async storeProvider({ id, isEnabled, isPrimary }) {
+    async storeProvider(provider) {
+      const { id, isEnabled, isPrimary } = provider;
+      let nextIsPrimary = isPrimary;
+
+      // if the current provider has been disabled it can't be primary
+      if (!isEnabled) {
+        nextIsPrimary = false;
+      }
+
+      // if the current provider is the only enabled provider it should be primary
+      if (isEnabled && !this.enabledProviders.length) {
+        nextIsPrimary = true;
+      }
+
       try {
         const {
           data: {
@@ -111,13 +146,17 @@ export default {
               projectPath: this.projectFullPath,
               providerId: id,
               isEnabled,
-              isPrimary,
+              isPrimary: nextIsPrimary,
             },
           },
           optimisticResponse: updateSecurityTrainingOptimisticResponse({
             id,
             isEnabled,
-            isPrimary,
+            isPrimary: nextIsPrimary,
+          }),
+          update: updateSecurityTrainingCache({
+            query: securityTrainingProvidersQuery,
+            variables: { fullPath: this.projectFullPath },
           }),
         });
 
@@ -188,6 +227,27 @@ export default {
                   {{ __('Learn more.') }}
                 </gl-link>
               </p>
+              <!-- Note: The following `div` and it's content will be replaced by 'GlFormRadio' once https://gitlab.com/gitlab-org/gitlab-ui/-/issues/1720#note_857342988 is resolved -->
+              <div
+                class="gl-form-radio custom-control custom-radio"
+                data-testid="primary-provider-radio"
+              >
+                <input
+                  :id="`security-training-provider-${provider.id}`"
+                  type="radio"
+                  :checked="provider.isPrimary"
+                  name="radio-group-name"
+                  class="custom-control-input"
+                  :disabled="!provider.isEnabled"
+                  @change="setPrimaryProvider(provider)"
+                />
+                <label
+                  class="custom-control-label"
+                  :for="`security-training-provider-${provider.id}`"
+                >
+                  {{ $options.i18n.primaryTraining }}
+                </label>
+              </div>
             </div>
           </div>
         </gl-card>

app/assets/javascripts/security_configuration/graphql/utils/optimistic_response.js → app/assets/javascripts/security_configuration/graphql/cache_utils.js

+import produce from 'immer';
+
 export const updateSecurityTrainingOptimisticResponse = (changes) => ({
   // False positive i18n lint: https://gitlab.com/gitlab-org/frontend/eslint-plugin-i18n/issues/26
   // eslint-disable-next-line @gitlab/require-i18n-strings
@@ -11,3 +13,28 @@ export const updateSecurityTrainingOptimisticResponse = (changes) => ({
     errors: [],
   },
 });
+
+export const updateSecurityTrainingCache = ({ query, variables }) => (cache, { data }) => {
+  const {
+    securityTrainingUpdate: { training: updatedProvider },
+  } = data;
+  const { project } = cache.readQuery({ query, variables });
+  if (!updatedProvider.isPrimary) {
+    return;
+  }
+
+  // when we set a new primary provider, we need to unset the previous one(s)
+  const updatedProject = produce(project, (draft) => {
+    draft.securityTrainingProviders.forEach((provider) => {
+      // eslint-disable-next-line no-param-reassign
+      provider.isPrimary = provider.id === updatedProvider.id;
+    });
+  });
+
+  // write to the cache
+  cache.writeQuery({
+    query,
+    variables,
+    data: { project: updatedProject },
+  });
+};

ee/app/graphql/mutations/security/training_provider_update.rb

@@ -45,7 +45,7 @@ module Mutations
         return unless training.provider
 
         training.provider.tap do |provider|
-          provider.assign_attributes(is_enabled: !training.destroyed?, is_primary: training.is_primary)
+          provider.assign_attributes(is_enabled: !training.destroyed?, is_primary: !training.destroyed? && training.is_primary)
         end
       end
     end

ee/app/models/security/training.rb

@@ -20,14 +20,14 @@ module Security
     # if there are other trainings enabled for the project.
     # Users have to select another primary before deleting trainings.
     def prevent_deleting_primary
-      return unless is_primary? && only_training_available?
+      return unless is_primary? && other_trainings_available?
 
       errors.add(:base, _("Can not delete primary training"))
 
       throw :abort # rubocop:disable Cop/BanCatchThrow
     end
 
-    def only_training_available?
+    def other_trainings_available?
       project.security_trainings.not_including(self).exists?
     end
   end

ee/spec/frontend/vulnerabilities/vulnerability_training_spec.js

@@ -20,10 +20,7 @@ import {
 } from '~/security_configuration/constants';
 import createMockApollo from 'helpers/mock_apollo_helper';
 import waitForPromises from 'helpers/wait_for_promises';
-import {
-  securityTrainingProvidersResponse,
-  disabledSecurityTrainingProvidersResponse,
-} from 'jest/security_configuration/mock_data';
+import { getSecurityTrainingProvidersData } from 'jest/security_configuration/mock_data';
 
 const defaultProps = {
   identifiers: [
@@ -36,6 +33,12 @@ const mockSuccessTrainingUrl = 'training/path';
 
 Vue.use(VueApollo);
 
+const TEST_TRAINING_PROVIDERS_ALL_DISABLED = getSecurityTrainingProvidersData();
+const TEST_TRAINING_PROVIDERS_FIRST_ENABLED = getSecurityTrainingProvidersData({
+  providerOverrides: { first: { isEnabled: true } },
+});
+const TEST_TRAINING_PROVIDERS_DEFAULT = TEST_TRAINING_PROVIDERS_FIRST_ENABLED;
+
 describe('VulnerabilityTraining component', () => {
   let wrapper;
   let apolloProvider;
@@ -45,7 +48,7 @@ describe('VulnerabilityTraining component', () => {
     apolloProvider = createMockApollo([
       [
         securityTrainingProvidersQuery,
-        queryHandler || jest.fn().mockResolvedValue(securityTrainingProvidersResponse),
+        queryHandler || jest.fn().mockResolvedValue(TEST_TRAINING_PROVIDERS_DEFAULT.response),
       ],
     ]);
   };
@@ -106,7 +109,7 @@ describe('VulnerabilityTraining component', () => {
 
       it('does not render component when there are no enabled securityTrainingProviders', async () => {
         createApolloProvider({
-          queryHandler: jest.fn().mockResolvedValue(disabledSecurityTrainingProvidersResponse),
+          queryHandler: jest.fn().mockResolvedValue(TEST_TRAINING_PROVIDERS_ALL_DISABLED.response),
         });
         createComponent();
         await waitForQueryToBeLoaded();

ee/spec/graphql/mutations/security/training_provider_update_spec.rb

@@ -46,11 +46,22 @@ RSpec.describe Mutations::Security::TrainingProviderUpdate do
           subject { mutation_result[:training] }
 
           context 'when the training is deleted' do
-            before do
-              training.destroy!
+            context 'when training is not primary' do
+              before do
+                training.destroy!
+              end
+
+              it { is_expected.to have_attributes(is_enabled: false, is_primary: false) }
             end
 
-            it { is_expected.to have_attributes(is_enabled: false, is_primary: false) }
+            context 'when training is primary' do
+              before do
+                training.update!(is_primary: true)
+                training.destroy!
+              end
+
+              it { is_expected.to have_attributes(is_enabled: false, is_primary: false) }
+            end
           end
 
           context 'when the training is not deleted' do

locale/gitlab.pot

@@ -33094,6 +33094,9 @@ msgstr ""
 msgid "SecurityReports|scanned resources"
 msgstr ""
 
+msgid "SecurityTraining|Primary Training"
+msgstr ""
+
 msgid "See example DevOps Score page in our documentation."
 msgstr ""
 

spec/frontend/security_configuration/graphql/cache_utils_spec.js

+import {
+  updateSecurityTrainingCache,
+  updateSecurityTrainingOptimisticResponse,
+} from '~/security_configuration/graphql/cache_utils';
+
+describe('EE - Security configuration graphQL cache utils', () => {
+  describe('updateSecurityTrainingOptimisticResponse', () => {
+    it('returns an optimistic response in the correct shape', () => {
+      const changes = { isEnabled: true, isPrimary: true };
+      const mutationResponse = updateSecurityTrainingOptimisticResponse(changes);
+
+      expect(mutationResponse).toEqual({
+        __typename: 'Mutation',
+        securityTrainingUpdate: {
+          __typename: 'SecurityTrainingUpdatePayload',
+          training: {
+            __typename: 'ProjectSecurityTraining',
+            ...changes,
+          },
+          errors: [],
+        },
+      });
+    });
+  });
+
+  describe('updateSecurityTrainingCache', () => {
+    let mockCache;
+
+    beforeEach(() => {
+      // freezing the data makes sure that we don't mutate the original project
+      const mockCacheData = Object.freeze({
+        project: {
+          securityTrainingProviders: [
+            { id: 1, isEnabled: true, isPrimary: true },
+            { id: 2, isEnabled: true, isPrimary: false },
+            { id: 3, isEnabled: false, isPrimary: false },
+          ],
+        },
+      });
+
+      mockCache = {
+        readQuery: () => mockCacheData,
+        writeQuery: jest.fn(),
+      };
+    });
+
+    it('does not update the cache when the primary provider is not getting disabled', () => {
+      const providerAfterUpdate = {
+        id: 2,
+        isEnabled: true,
+        isPrimary: false,
+      };
+
+      updateSecurityTrainingCache({
+        query: 'GraphQL query',
+        variables: { fullPath: 'gitlab/project' },
+      })(mockCache, {
+        data: {
+          securityTrainingUpdate: {
+            training: {
+              ...providerAfterUpdate,
+            },
+          },
+        },
+      });
+
+      expect(mockCache.writeQuery).not.toHaveBeenCalled();
+    });
+
+    it('sets the previous primary provider to be non-primary when another provider gets set as primary', () => {
+      const providerAfterUpdate = {
+        id: 2,
+        isEnabled: true,
+        isPrimary: true,
+      };
+
+      const expectedTrainingProvidersWrittenToCache = [
+        // this was the previous primary primary provider and it should not be primary any longer
+        { id: 1, isEnabled: true, isPrimary: false },
+        { id: 2, isEnabled: true, isPrimary: true },
+        { id: 3, isEnabled: false, isPrimary: false },
+      ];
+
+      updateSecurityTrainingCache({
+        query: 'GraphQL query',
+        variables: { fullPath: 'gitlab/project' },
+      })(mockCache, {
+        data: {
+          securityTrainingUpdate: {
+            training: {
+              ...providerAfterUpdate,
+            },
+          },
+        },
+      });
+
+      expect(mockCache.writeQuery).toHaveBeenCalledWith(
+        expect.objectContaining({
+          data: {
+            project: {
+              securityTrainingProviders: expectedTrainingProvidersWrittenToCache,
+            },
+          },
+        }),
+      );
+    });
+  });
+});

spec/frontend/security_configuration/mock_data.js

 export const testProjectPath = 'foo/bar';
 
-export const testProviderIds = [101, 102];
+export const testProviderIds = [101, 102, 103];
 
-export const securityTrainingProviders = [
+const createSecurityTrainingProviders = ({ providerOverrides = {} }) => [
   {
     id: testProviderIds[0],
     name: 'Vendor Name 1',
@@ -10,33 +10,43 @@ export const securityTrainingProviders = [
     url: 'https://www.example.org/security/training',
     isEnabled: false,
     isPrimary: false,
+    ...providerOverrides.first,
   },
   {
     id: testProviderIds[1],
     name: 'Vendor Name 2',
     description: 'Security training with guide and learning pathways.',
     url: 'https://www.vendornametwo.com/',
-    isEnabled: true,
+    isEnabled: false,
+    isPrimary: false,
+    ...providerOverrides.second,
+  },
+  {
+    id: testProviderIds[2],
+    name: 'Vendor Name 3',
+    description: 'Security training for the everyday developer.',
+    url: 'https://www.vendornamethree.com/',
+    isEnabled: false,
     isPrimary: false,
+    ...providerOverrides.third,
   },
 ];
 
-export const securityTrainingProvidersResponse = {
-  data: {
-    project: {
-      id: 1,
-      securityTrainingProviders,
+export const getSecurityTrainingProvidersData = (providerOverrides = {}) => {
+  const securityTrainingProviders = createSecurityTrainingProviders(providerOverrides);
+  const response = {
+    data: {
+      project: {
+        id: 1,
+        securityTrainingProviders,
+      },
     },
-  },
-};
+  };
 
-export const disabledSecurityTrainingProvidersResponse = {
-  data: {
-    project: {
-      id: 1,
-      securityTrainingProviders: [securityTrainingProviders[0]],
-    },
-  },
+  return {
+    response,
+    data: securityTrainingProviders,
+  };
 };
 
 export const dismissUserCalloutResponse = {