Code review feedback

- Remove extra space in before endLink
- Remove handleAuthenticate and call createRole directly
- Call createRole as a form submission.
- Store api paths in the store state

app/assets/javascripts/create_cluster/eks_cluster/components/service_credentials_form.vue

@@ -46,7 +46,7 @@ export default {
 
       return sprintf(
         s__(
-          'ClusterIntegration|Create a provision role on %{startAwsLink}Amazon Web Services %{externalLinkIcon} %{endLink} using the account and external ID above. %{startMoreInfoLink}More information%{endLink}',
+          'ClusterIntegration|Create a provision role on %{startAwsLink}Amazon Web Services %{externalLinkIcon}%{endLink} using the account and external ID above. %{startMoreInfoLink}More information%{endLink}',
         ),
         {
           startAwsLink:
@@ -63,7 +63,7 @@ export default {
 
       return sprintf(
         s__(
-          'ClusterIntegration|The Amazon Resource Name (ARN) associated with your role. If you do not have a provision role, first create one on  %{startAwsLink}Amazon Web Services %{externalLinkIcon} %{endLink} using the above account and external IDs. %{startMoreInfoLink}More information%{endLink}',
+          'ClusterIntegration|The Amazon Resource Name (ARN) associated with your role. If you do not have a provision role, first create one on  %{startAwsLink}Amazon Web Services %{externalLinkIcon}%{endLink} using the above account and external IDs. %{startMoreInfoLink}More information%{endLink}',
         ),
         {
           startAwsLink:
@@ -78,16 +78,11 @@ export default {
   },
   methods: {
     ...mapActions(['createRole']),
-    handleAuthenticate() {
-      const { roleArn, externalId } = this;
-
-      this.createRole({ roleArn, externalId });
-    },
   },
 };
 </script>
 <template>
-  <form name="service-credentials-form">
+  <form name="service-credentials-form" @submit.prevent="createRole({ roleArn, externalId })">
     <h2>{{ s__('ClusterIntegration|Authenticate with Amazon Web Services') }}</h2>
     <p>
       {{
@@ -137,10 +132,10 @@ export default {
     </div>
     <loading-button
       class="js-submit-service-credentials"
+      type="submit"
       :disabled="submitButtonDisabled"
       :loading="isCreatingRole"
       :label="submitButtonLabel"
-      @click="handleAuthenticate($event)"
     />
   </form>
 </template>

app/assets/javascripts/create_cluster/eks_cluster/index.js

@@ -26,8 +26,6 @@ export default el => {
         hasCredentials: parseBoolean(hasCredentials),
         externalId,
         accountId,
-      },
-      apiPaths: {
         createRolePath,
       },
     }),

app/assets/javascripts/create_cluster/eks_cluster/store/actions.js

 import * as types from './mutation_types';
 import axios from '~/lib/utils/axios_utils';
 
-export default apiPaths => ({
-  setClusterName({ commit }, payload) {
-    commit(types.SET_CLUSTER_NAME, payload);
-  },
-  setEnvironmentScope({ commit }, payload) {
-    commit(types.SET_ENVIRONMENT_SCOPE, payload);
-  },
-  setKubernetesVersion({ commit }, payload) {
-    commit(types.SET_KUBERNETES_VERSION, payload);
-  },
-  createRole({ dispatch }, payload) {
-    dispatch('requestCreateRole');
-
-    return axios
-      .post(apiPaths.createRolePath, {
-        role_arn: payload.roleArn,
-        role_external_id: payload.externalId,
-      })
-      .then(() => dispatch('createRoleSuccess'))
-      .catch(error => dispatch('createRoleError', { error }));
-  },
-  requestCreateRole({ commit }) {
-    commit(types.REQUEST_CREATE_ROLE);
-  },
-  createRoleSuccess({ commit }) {
-    commit(types.CREATE_ROLE_SUCCESS);
-  },
-  createRoleError({ commit }, payload) {
-    commit(types.CREATE_ROLE_ERROR, payload);
-  },
-  setRegion({ commit }, payload) {
-    commit(types.SET_REGION, payload);
-  },
-  setKeyPair({ commit }, payload) {
-    commit(types.SET_KEY_PAIR, payload);
-  },
-  setVpc({ commit }, payload) {
-    commit(types.SET_VPC, payload);
-  },
-  setSubnet({ commit }, payload) {
-    commit(types.SET_SUBNET, payload);
-  },
-  setRole({ commit }, payload) {
-    commit(types.SET_ROLE, payload);
-  },
-  setSecurityGroup({ commit }, payload) {
-    commit(types.SET_SECURITY_GROUP, payload);
-  },
-  setGitlabManagedCluster({ commit }, payload) {
-    commit(types.SET_GITLAB_MANAGED_CLUSTER, payload);
-  },
-});
+export const setClusterName = ({ commit }, payload) => {
+  commit(types.SET_CLUSTER_NAME, payload);
+};
+
+export const setEnvironmentScope = ({ commit }, payload) => {
+  commit(types.SET_ENVIRONMENT_SCOPE, payload);
+};
+
+export const setKubernetesVersion = ({ commit }, payload) => {
+  commit(types.SET_KUBERNETES_VERSION, payload);
+};
+
+export const createRole = ({ dispatch, state: { createRolePath } }, payload) => {
+  dispatch('requestCreateRole');
+
+  return axios
+    .post(createRolePath, {
+      role_arn: payload.roleArn,
+      role_external_id: payload.externalId,
+    })
+    .then(() => dispatch('createRoleSuccess'))
+    .catch(error => dispatch('createRoleError', { error }));
+};
+
+export const requestCreateRole = ({ commit }) => {
+  commit(types.REQUEST_CREATE_ROLE);
+};
+
+export const createRoleSuccess = ({ commit }) => {
+  commit(types.CREATE_ROLE_SUCCESS);
+};
+
+export const createRoleError = ({ commit }, payload) => {
+  commit(types.CREATE_ROLE_ERROR, payload);
+};
+
+export const setRegion = ({ commit }, payload) => {
+  commit(types.SET_REGION, payload);
+};
+
+export const setKeyPair = ({ commit }, payload) => {
+  commit(types.SET_KEY_PAIR, payload);
+};
+
+export const setVpc = ({ commit }, payload) => {
+  commit(types.SET_VPC, payload);
+};
+
+export const setSubnet = ({ commit }, payload) => {
+  commit(types.SET_SUBNET, payload);
+};
+
+export const setRole = ({ commit }, payload) => {
+  commit(types.SET_ROLE, payload);
+};
+
+export const setSecurityGroup = ({ commit }, payload) => {
+  commit(types.SET_SECURITY_GROUP, payload);
+};
+
+export const setGitlabManagedCluster = ({ commit }, payload) => {
+  commit(types.SET_GITLAB_MANAGED_CLUSTER, payload);
+};

app/assets/javascripts/create_cluster/eks_cluster/store/index.js

 import Vuex from 'vuex';
-import actions from './actions';
+import * as actions from './actions';
 import * as getters from './getters';
 import mutations from './mutations';
 import state from './state';
@@ -8,9 +8,9 @@ import clusterDropdownStore from './cluster_dropdown';
 
 import * as awsServices from '../services/aws_services_facade';
 
-const createStore = ({ initialState, apiPaths }) =>
+const createStore = ({ initialState }) =>
   new Vuex.Store({
-    actions: actions(apiPaths),
+    actions,
     getters,
     mutations,
     state: Object.assign(state(), initialState),

app/assets/javascripts/create_cluster/eks_cluster/store/state.js

 import { KUBERNETES_VERSIONS } from '../constants';
 
 export default () => ({
+  createRolePath: null,
+
   isCreatingRole: false,
   roleCreated: false,
   createRoleError: false,

app/views/clusters/clusters/cloud_providers/_cloud_provider_button.html.haml

@@ -4,7 +4,7 @@
 - last = local_assigns.fetch(:last, false)
 - classes = ['btn btn-light btn-outline flex-fill d-inline-flex flex-column justify-content-center align-items-center', ('mr-3' unless last)]
 
-= link_to clusterable.new_path(provider: provider), class: 'btn btn-light btn-outline flex-fill d-inline-flex flex-column mr-3 justify-content-center align-items-center' do
+= link_to clusterable.new_path(provider: provider), class: classes do
   .svg-content.p-2= image_tag logo_path, alt: label, class: 'gl-w-64 gl-h-64'
   %span
     = label

locale/gitlab.pot

@@ -1489,7 +1489,7 @@ msgstr ""
 msgid "Amazon EKS integration allows you to provision EKS clusters from GitLab."
 msgstr ""
 
-msgid "Amazon authentication is not %{link_start}property configured%{link_end}. Ask your GitLab administrator if you want to use this service."
+msgid "Amazon authentication is not %{link_start}correctly configured%{link_end}. Ask your GitLab administrator if you want to use this service."
 msgstr ""
 
 msgid "Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication"
@@ -2173,6 +2173,9 @@ msgstr ""
 msgid "Authenticate with GitHub"
 msgstr ""
 
+msgid "Authenticating"
+msgstr ""
+
 msgid "Authentication Log"
 msgstr ""
 
@@ -3607,7 +3610,7 @@ msgstr ""
 msgid "ClusterIntegration|Create Kubernetes cluster"
 msgstr ""
 
-msgid "ClusterIntegration|Create a provision role on %{startAwsLink}Amazon Web Services %{externalLinkIcon} %{endLink} using the account and external ID above. %{startMoreInfoLink}More information%{endLink}"
+msgid "ClusterIntegration|Create a provision role on %{startAwsLink}Amazon Web Services %{externalLinkIcon}%{endLink} using the account and external ID above. %{startMoreInfoLink}More information%{endLink}"
 msgstr ""
 
 msgid "ClusterIntegration|Create cluster on"
@@ -4045,7 +4048,7 @@ msgstr ""
 msgid "ClusterIntegration|Subnet"
 msgstr ""
 
-msgid "ClusterIntegration|The Amazon Resource Name (ARN) associated with your role. If you do not have a provision role, first create one on  %{startAwsLink}Amazon Web Services %{externalLinkIcon} %{endLink} using the above account and external IDs. %{startMoreInfoLink}More information%{endLink}"
+msgid "ClusterIntegration|The Amazon Resource Name (ARN) associated with your role. If you do not have a provision role, first create one on  %{startAwsLink}Amazon Web Services %{externalLinkIcon}%{endLink} using the above account and external IDs. %{startMoreInfoLink}More information%{endLink}"
 msgstr ""
 
 msgid "ClusterIntegration|The Kubernetes certificate used to authenticate to the cluster."

spec/frontend/create_cluster/eks_cluster/components/service_credentials_form_spec.js

@@ -47,6 +47,7 @@ describe('ServiceCredentialsForm', () => {
   const findCopyExternalIdButton = () => vm.find('.js-copy-external-id-button');
   const findInvalidCredentials = () => vm.find('.js-invalid-credentials');
   const findSubmitButton = () => vm.find(LoadingButton);
+  const findForm = () => vm.find('form[name="service-credentials-form"]');
 
   it('displays provided account id', () => {
     expect(findAccountIdInput().attributes('value')).toBe(accountId);
@@ -74,8 +75,8 @@ describe('ServiceCredentialsForm', () => {
     expect(findSubmitButton().attributes('disabled')).toBeFalsy();
   });
 
-  it('dispatches authenticate action when submit button is clicked', () => {
-    findSubmitButton().vm.$emit('click');
+  it('dispatches createRole action when form is submitted', () => {
+    findForm().trigger('submit');
 
     expect(createRoleAction).toHaveBeenCalled();
   });

spec/frontend/create_cluster/eks_cluster/store/actions_spec.js

 import testAction from 'helpers/vuex_action_helper';
 
 import createState from '~/create_cluster/eks_cluster/store/state';
-import actionsFactory from '~/create_cluster/eks_cluster/store/actions';
+import * as actions from '~/create_cluster/eks_cluster/store/actions';
 import {
   SET_CLUSTER_NAME,
   SET_ENVIRONMENT_SCOPE,
@@ -31,9 +31,8 @@ describe('EKS Cluster Store Actions', () => {
   let keyPair;
   let securityGroup;
   let gitlabManagedCluster;
-  let actions;
-  let apiPaths;
   let mock;
+  let state;
 
   beforeEach(() => {
     clusterName = 'my cluster';
@@ -47,11 +46,10 @@ describe('EKS Cluster Store Actions', () => {
     securityGroup = { name: 'default group' };
     gitlabManagedCluster = true;
 
-    apiPaths = {
+    state = {
+      ...createState(),
       createRolePath: '/clusters/roles/',
     };
-
-    actions = actionsFactory(apiPaths);
   });
 
   beforeEach(() => {
@@ -77,7 +75,7 @@ describe('EKS Cluster Store Actions', () => {
   `(`$action commits $mutation with $payloadDescription payload`, data => {
     const { action, mutation, payload } = data;
 
-    testAction(actions[action], payload, createState(), [{ type: mutation, payload }]);
+    testAction(actions[action], payload, state, [{ type: mutation, payload }]);
   });
 
   describe('createRole', () => {
@@ -89,7 +87,7 @@ describe('EKS Cluster Store Actions', () => {
     describe('when request succeeds', () => {
       beforeEach(() => {
         mock
-          .onPost(apiPaths.createRolePath, {
+          .onPost(state.createRolePath, {
             role_arn: payload.roleArn,
             role_external_id: payload.externalId,
           })
@@ -100,7 +98,7 @@ describe('EKS Cluster Store Actions', () => {
         testAction(
           actions.createRole,
           payload,
-          createState(),
+          state,
           [],
           [{ type: 'requestCreateRole' }, { type: 'createRoleSuccess' }],
         ));
@@ -112,7 +110,7 @@ describe('EKS Cluster Store Actions', () => {
       beforeEach(() => {
         error = new Error('Request failed with status code 400');
         mock
-          .onPost(apiPaths.createRolePath, {
+          .onPost(state.createRolePath, {
             role_arn: payload.roleArn,
             role_external_id: payload.externalId,
           })
@@ -123,7 +121,7 @@ describe('EKS Cluster Store Actions', () => {
         testAction(
           actions.createRole,
           payload,
-          createState(),
+          state,
           [],
           [{ type: 'requestCreateRole' }, { type: 'createRoleError', payload: { error } }],
         ));
@@ -132,13 +130,13 @@ describe('EKS Cluster Store Actions', () => {
 
   describe('requestCreateRole', () => {
     it('commits requestCreaterole mutation', () => {
-      testAction(actions.requestCreateRole, null, createState(), [{ type: REQUEST_CREATE_ROLE }]);
+      testAction(actions.requestCreateRole, null, state, [{ type: REQUEST_CREATE_ROLE }]);
     });
   });
 
   describe('createRoleSuccess', () => {
     it('commits createRoleSuccess mutation', () => {
-      testAction(actions.createRoleSuccess, null, createState(), [{ type: CREATE_ROLE_SUCCESS }]);
+      testAction(actions.createRoleSuccess, null, state, [{ type: CREATE_ROLE_SUCCESS }]);
     });
   });
 
@@ -148,9 +146,7 @@ describe('EKS Cluster Store Actions', () => {
         error: new Error(),
       };
 
-      testAction(actions.createRoleError, payload, createState(), [
-        { type: CREATE_ROLE_ERROR, payload },
-      ]);
+      testAction(actions.createRoleError, payload, state, [{ type: CREATE_ROLE_ERROR, payload }]);
     });
   });
 });