Merge branch '333807-runners-ui-error-handling-improvements' into 'master'

Update error handling and display in runners UI

See merge request gitlab-org/gitlab!64517

app/assets/javascripts/runner/components/cells/runner_actions_cell.vue

 <script>
 import { GlButton, GlButtonGroup, GlTooltipDirective } from '@gitlab/ui';
+import createFlash from '~/flash';
 import { getIdFromGraphQLId } from '~/graphql_shared/utils';
 import { __, s__ } from '~/locale';
-import deleteRunnerMutation from '~/runner/graphql/delete_runner.mutation.graphql';
+import runnerDeleteMutation from '~/runner/graphql/runner_delete.mutation.graphql';
 import runnerUpdateMutation from '~/runner/graphql/runner_update.mutation.graphql';
+import { captureException } from '~/runner/sentry_utils';
 
 const i18n = {
   I18N_EDIT: __('Edit'),
@@ -14,6 +16,7 @@ const i18n = {
 };
 
 export default {
+  name: 'RunnerActionsCell',
   components: {
     GlButton,
     GlButtonGroup,
@@ -86,7 +89,7 @@ export default {
         });
 
         if (errors && errors.length) {
-          this.onError(new Error(errors[0]));
+          throw new Error(errors.join(' '));
         }
       } catch (e) {
         this.onError(e);
@@ -109,7 +112,7 @@ export default {
             runnerDelete: { errors },
           },
         } = await this.$apollo.mutate({
-          mutation: deleteRunnerMutation,
+          mutation: runnerDeleteMutation,
           variables: {
             input: {
               id: this.runner.id,
@@ -119,7 +122,7 @@ export default {
           refetchQueries: ['getRunners'],
         });
         if (errors && errors.length) {
-          this.onError(new Error(errors[0]));
+          throw new Error(errors.join(' '));
         }
       } catch (e) {
         this.onError(e);
@@ -129,9 +132,13 @@ export default {
     },
 
     onError(error) {
-      // TODO Render errors when "delete" action is done
-      // `active` toggle would not fail due to user input.
-      throw error;
+      const { message } = error;
+      createFlash({ message });
+
+      this.reportToSentry(error);
+    },
+    reportToSentry(error) {
+      captureException({ error, component: this.$options.name });
     },
   },
   i18n,

app/assets/javascripts/runner/components/runner_registration_token_reset.vue

@@ -3,9 +3,11 @@ import { GlButton } from '@gitlab/ui';
 import createFlash, { FLASH_TYPES } from '~/flash';
 import { __, s__ } from '~/locale';
 import runnersRegistrationTokenResetMutation from '~/runner/graphql/runners_registration_token_reset.mutation.graphql';
+import { captureException } from '~/runner/sentry_utils';
 import { INSTANCE_TYPE, GROUP_TYPE, PROJECT_TYPE } from '../constants';
 
 export default {
+  name: 'RunnerRegistrationTokenReset',
   components: {
     GlButton,
   },
@@ -52,8 +54,7 @@ export default {
           },
         });
         if (errors && errors.length) {
-          this.onError(new Error(errors[0]));
-          return;
+          throw new Error(errors.join(' '));
         }
         this.onSuccess(token);
       } catch (e) {
@@ -65,6 +66,8 @@ export default {
     onError(error) {
       const { message } = error;
       createFlash({ message });
+
+      this.reportToSentry(error);
     },
     onSuccess(token) {
       createFlash({
@@ -73,6 +76,9 @@ export default {
       });
       this.$emit('tokenReset', token);
     },
+    reportToSentry(error) {
+      captureException({ error, component: this.$options.name });
+    },
   },
 };
 </script>

app/assets/javascripts/runner/components/runner_update_form.vue

@@ -9,6 +9,7 @@ import {
 } from '@gitlab/ui';
 import createFlash, { FLASH_TYPES } from '~/flash';
 import { __ } from '~/locale';
+import { captureException } from '~/runner/sentry_utils';
 import { ACCESS_LEVEL_NOT_PROTECTED, ACCESS_LEVEL_REF_PROTECTED, PROJECT_TYPE } from '../constants';
 import runnerUpdateMutation from '../graphql/runner_update.mutation.graphql';
 
@@ -37,6 +38,7 @@ const runnerToModel = (runner) => {
 };
 
 export default {
+  name: 'RunnerUpdateForm',
   components: {
     GlButton,
     GlForm,
@@ -104,25 +106,28 @@ export default {
         });
 
         if (errors?.length) {
-          this.onError(new Error(errors[0]));
+          // Validation errors need not be thrown
+          createFlash({ message: errors[0] });
           return;
         }
 
         this.onSuccess();
-      } catch (e) {
-        this.onError(e);
+      } catch (error) {
+        const { message } = error;
+        createFlash({ message });
+
+        this.reportToSentry(error);
       } finally {
         this.saving = false;
       }
     },
-    onError(error) {
-      const { message } = error;
-      createFlash({ message });
-    },
     onSuccess() {
       createFlash({ message: __('Changes saved.'), type: FLASH_TYPES.SUCCESS });
       this.model = runnerToModel(this.runner);
     },
+    reportToSentry(error) {
+      captureException({ error, component: this.$options.name });
+    },
   },
   ACCESS_LEVEL_NOT_PROTECTED,
   ACCESS_LEVEL_REF_PROTECTED,

app/assets/javascripts/runner/constants.js

@@ -3,6 +3,7 @@ import { s__ } from '~/locale';
 export const RUNNER_PAGE_SIZE = 20;
 export const RUNNER_JOB_COUNT_LIMIT = 1000;
 
+export const I18N_FETCH_ERROR = s__('Runners|Something went wrong while fetching runner data.');
 export const I18N_DETAILS_TITLE = s__('Runners|Runner #%{runner_id}');
 
 export const RUNNER_TAG_BADGE_VARIANT = 'info';

app/assets/javascripts/runner/runner_details/runner_details_app.vue

 <script>
+import createFlash from '~/flash';
 import { TYPE_CI_RUNNER } from '~/graphql_shared/constants';
 import { convertToGraphQLId } from '~/graphql_shared/utils';
+import { sprintf } from '~/locale';
 import RunnerTypeAlert from '../components/runner_type_alert.vue';
 import RunnerTypeBadge from '../components/runner_type_badge.vue';
 import RunnerUpdateForm from '../components/runner_update_form.vue';
-import { I18N_DETAILS_TITLE } from '../constants';
+import { I18N_DETAILS_TITLE, I18N_FETCH_ERROR } from '../constants';
 import getRunnerQuery from '../graphql/get_runner.query.graphql';
+import { captureException } from '../sentry_utils';
 
 export default {
+  name: 'RunnerDetailsApp',
   components: {
     RunnerTypeAlert,
     RunnerTypeBadge,
     RunnerUpdateForm,
   },
-  i18n: {
-    I18N_DETAILS_TITLE,
-  },
   props: {
     runnerId: {
       type: String,
@@ -35,6 +36,24 @@ export default {
           id: convertToGraphQLId(TYPE_CI_RUNNER, this.runnerId),
         };
       },
+      error(error) {
+        createFlash({ message: I18N_FETCH_ERROR });
+
+        this.reportToSentry(error);
+      },
+    },
+  },
+  computed: {
+    pageTitle() {
+      return sprintf(I18N_DETAILS_TITLE, { runner_id: this.runnerId });
+    },
+  },
+  errorCaptured(error) {
+    this.reportToSentry(error);
+  },
+  methods: {
+    reportToSentry(error) {
+      captureException({ error, component: this.$options.name });
     },
   },
 };
@@ -42,9 +61,7 @@ export default {
 <template>
   <div>
     <h2 class="page-title">
-      {{ sprintf($options.i18n.I18N_DETAILS_TITLE, { runner_id: runnerId }) }}
-
-      <runner-type-badge v-if="runner" :type="runner.runnerType" />
+      {{ pageTitle }} <runner-type-badge v-if="runner" :type="runner.runnerType" />
     </h2>
 
     <runner-type-alert v-if="runner" :type="runner.runnerType" />

app/assets/javascripts/runner/runner_list/runner_list_app.vue

 <script>
-import * as Sentry from '@sentry/browser';
+import createFlash from '~/flash';
 import { fetchPolicies } from '~/lib/graphql';
 import { updateHistory } from '~/lib/utils/url_utility';
 import RunnerFilteredSearchBar from '../components/runner_filtered_search_bar.vue';
@@ -7,8 +7,9 @@ import RunnerList from '../components/runner_list.vue';
 import RunnerManualSetupHelp from '../components/runner_manual_setup_help.vue';
 import RunnerPagination from '../components/runner_pagination.vue';
 import RunnerTypeHelp from '../components/runner_type_help.vue';
-import { INSTANCE_TYPE } from '../constants';
+import { INSTANCE_TYPE, I18N_FETCH_ERROR } from '../constants';
 import getRunnersQuery from '../graphql/get_runners.query.graphql';
+import { captureException } from '../sentry_utils';
 import {
   fromUrlQueryToSearch,
   fromSearchToUrl,
@@ -16,6 +17,7 @@ import {
 } from './runner_search_utils';
 
 export default {
+  name: 'RunnerListApp',
   components: {
     RunnerFilteredSearchBar,
     RunnerList,
@@ -59,8 +61,10 @@ export default {
           pageInfo: runners?.pageInfo || {},
         };
       },
-      error(err) {
-        this.captureException(err);
+      error(error) {
+        createFlash({ message: I18N_FETCH_ERROR });
+
+        this.reportToSentry(error);
       },
     },
   },
@@ -87,15 +91,12 @@ export default {
       },
     },
   },
-  errorCaptured(err) {
-    this.captureException(err);
+  errorCaptured(error) {
+    this.reportToSentry(error);
   },
   methods: {
-    captureException(err) {
-      Sentry.withScope((scope) => {
-        scope.setTag('component', 'runner_list_app');
-        Sentry.captureException(err);
-      });
+    reportToSentry(error) {
+      captureException({ error, component: this.$options.name });
     },
   },
   INSTANCE_TYPE,

app/assets/javascripts/runner/sentry_utils.js

+import * as Sentry from '@sentry/browser';
+
+const COMPONENT_TAG = 'vue_component';
+
+/**
+ * Captures an error in a Vue component and sends it
+ * to Sentry
+ *
+ * @param {Object} options
+ * @param {Error} options.error - Exception or error
+ * @param {String} options.component - Component name in CamelCase format
+ */
+export const captureException = ({ error, component }) => {
+  Sentry.withScope((scope) => {
+    if (component) {
+      scope.setTag(COMPONENT_TAG, component);
+    }
+    Sentry.captureException(error);
+  });
+};

locale/gitlab.pot

@@ -28174,6 +28174,9 @@ msgstr ""
 msgid "Runners|Show Runner installation instructions"
 msgstr ""
 
+msgid "Runners|Something went wrong while fetching runner data."
+msgstr ""
+
 msgid "Runners|Something went wrong while fetching the tags suggestions"
 msgstr ""
 

spec/frontend/runner/components/runner_registration_token_reset_spec.js

@@ -7,8 +7,10 @@ import createFlash, { FLASH_TYPES } from '~/flash';
 import RunnerRegistrationTokenReset from '~/runner/components/runner_registration_token_reset.vue';
 import { INSTANCE_TYPE } from '~/runner/constants';
 import runnersRegistrationTokenResetMutation from '~/runner/graphql/runners_registration_token_reset.mutation.graphql';
+import { captureException } from '~/runner/sentry_utils';
 
 jest.mock('~/flash');
+jest.mock('~/runner/sentry_utils');
 
 const localVue = createLocalVue();
 localVue.use(VueApollo);
@@ -111,25 +113,32 @@ describe('RunnerRegistrationTokenReset', () => {
 
   describe('On error', () => {
     it('On network error, error message is shown', async () => {
-      runnersRegistrationTokenResetMutationHandler.mockRejectedValueOnce(
-        new Error('Something went wrong'),
-      );
+      const mockErrorMsg = 'Token reset failed!';
+
+      runnersRegistrationTokenResetMutationHandler.mockRejectedValueOnce(new Error(mockErrorMsg));
 
       window.confirm.mockReturnValueOnce(true);
       await findButton().vm.$emit('click');
       await waitForPromises();
 
       expect(createFlash).toHaveBeenLastCalledWith({
-        message: 'Network error: Something went wrong',
+        message: `Network error: ${mockErrorMsg}`,
+      });
+      expect(captureException).toHaveBeenCalledWith({
+        error: new Error(`Network error: ${mockErrorMsg}`),
+        component: 'RunnerRegistrationTokenReset',
       });
     });
 
     it('On validation error, error message is shown', async () => {
+      const mockErrorMsg = 'User not allowed!';
+      const mockErrorMsg2 = 'Type is not valid!';
+
       runnersRegistrationTokenResetMutationHandler.mockResolvedValue({
         data: {
           runnersRegistrationTokenReset: {
             token: null,
-            errors: ['Token reset failed'],
+            errors: [mockErrorMsg, mockErrorMsg2],
           },
         },
       });
@@ -139,7 +148,11 @@ describe('RunnerRegistrationTokenReset', () => {
       await waitForPromises();
 
       expect(createFlash).toHaveBeenLastCalledWith({
-        message: 'Token reset failed',
+        message: `${mockErrorMsg} ${mockErrorMsg2}`,
+      });
+      expect(captureException).toHaveBeenCalledWith({
+        error: new Error(`${mockErrorMsg} ${mockErrorMsg2}`),
+        component: 'RunnerRegistrationTokenReset',
       });
     });
   });

spec/frontend/runner/components/runner_update_form_spec.js

@@ -15,9 +15,11 @@ import {
   ACCESS_LEVEL_NOT_PROTECTED,
 } from '~/runner/constants';
 import runnerUpdateMutation from '~/runner/graphql/runner_update.mutation.graphql';
+import { captureException } from '~/runner/sentry_utils';
 import { runnerData } from '../mock_data';
 
 jest.mock('~/flash');
+jest.mock('~/runner/sentry_utils');
 
 const mockRunner = runnerData.data.runner;
 
@@ -232,22 +234,30 @@ describe('RunnerUpdateForm', () => {
     });
 
     it('On network error, error message is shown', async () => {
-      runnerUpdateHandler.mockRejectedValue(new Error('Something went wrong'));
+      const mockErrorMsg = 'Update error!';
+
+      runnerUpdateHandler.mockRejectedValue(new Error(mockErrorMsg));
 
       await submitFormAndWait();
 
       expect(createFlash).toHaveBeenLastCalledWith({
-        message: 'Network error: Something went wrong',
+        message: `Network error: ${mockErrorMsg}`,
+      });
+      expect(captureException).toHaveBeenCalledWith({
+        component: 'RunnerUpdateForm',
+        error: new Error(`Network error: ${mockErrorMsg}`),
       });
       expect(findSubmitDisabledAttr()).toBeUndefined();
     });
 
-    it('On validation error, error message is shown', async () => {
+    it('On validation error, error message is shown and it is not sent to sentry', async () => {
+      const mockErrorMsg = 'Invalid value!';
+
       runnerUpdateHandler.mockResolvedValue({
         data: {
           runnerUpdate: {
             runner: mockRunner,
-            errors: ['A value is invalid'],
+            errors: [mockErrorMsg],
           },
         },
       });
@@ -255,8 +265,9 @@ describe('RunnerUpdateForm', () => {
       await submitFormAndWait();
 
       expect(createFlash).toHaveBeenLastCalledWith({
-        message: 'A value is invalid',
+        message: mockErrorMsg,
       });
+      expect(captureException).not.toHaveBeenCalled();
       expect(findSubmitDisabledAttr()).toBeUndefined();
     });
   });

spec/frontend/runner/runner_detail/runner_details_app_spec.js

@@ -2,14 +2,19 @@ import { createLocalVue, mount, shallowMount } from '@vue/test-utils';
 import VueApollo from 'vue-apollo';
 import createMockApollo from 'helpers/mock_apollo_helper';
 import waitForPromises from 'helpers/wait_for_promises';
+import createFlash from '~/flash';
 
 import { getIdFromGraphQLId } from '~/graphql_shared/utils';
 import RunnerTypeBadge from '~/runner/components/runner_type_badge.vue';
 import getRunnerQuery from '~/runner/graphql/get_runner.query.graphql';
 import RunnerDetailsApp from '~/runner/runner_details/runner_details_app.vue';
+import { captureException } from '~/runner/sentry_utils';
 
 import { runnerData } from '../mock_data';
 
+jest.mock('~/flash');
+jest.mock('~/runner/sentry_utils');
+
 const mockRunnerGraphqlId = runnerData.data.runner.id;
 const mockRunnerId = `${getIdFromGraphQLId(mockRunnerGraphqlId)}`;
 
@@ -23,11 +28,9 @@ describe('RunnerDetailsApp', () => {
   const findRunnerTypeBadge = () => wrapper.findComponent(RunnerTypeBadge);
 
   const createComponentWithApollo = ({ props = {}, mountFn = shallowMount } = {}) => {
-    const handlers = [[getRunnerQuery, mockRunnerQuery]];
-
     wrapper = mountFn(RunnerDetailsApp, {
       localVue,
-      apolloProvider: createMockApollo(handlers),
+      apolloProvider: createMockApollo([[getRunnerQuery, mockRunnerQuery]]),
       propsData: {
         runnerId: mockRunnerId,
         ...props,
@@ -63,4 +66,22 @@ describe('RunnerDetailsApp', () => {
 
     expect(findRunnerTypeBadge().text()).toBe('shared');
   });
+
+  describe('When there is an error', () => {
+    beforeEach(async () => {
+      mockRunnerQuery = jest.fn().mockRejectedValueOnce(new Error('Error!'));
+      await createComponentWithApollo();
+    });
+
+    it('error is reported to sentry', async () => {
+      expect(captureException).toHaveBeenCalledWith({
+        error: new Error('Network error: Error!'),
+        component: 'RunnerDetailsApp',
+      });
+    });
+
+    it('error is shown to the user', async () => {
+      expect(createFlash).toHaveBeenCalled();
+    });
+  });
 });

spec/frontend/runner/runner_list/runner_list_app_spec.js

-import * as Sentry from '@sentry/browser';
 import { createLocalVue, mount, shallowMount } from '@vue/test-utils';
 import VueApollo from 'vue-apollo';
 import createMockApollo from 'helpers/mock_apollo_helper';
 import { TEST_HOST } from 'helpers/test_constants';
 import waitForPromises from 'helpers/wait_for_promises';
+import createFlash from '~/flash';
 import { updateHistory } from '~/lib/utils/url_utility';
 
 import RunnerFilteredSearchBar from '~/runner/components/runner_filtered_search_bar.vue';
@@ -23,13 +23,15 @@ import {
 } from '~/runner/constants';
 import getRunnersQuery from '~/runner/graphql/get_runners.query.graphql';
 import RunnerListApp from '~/runner/runner_list/runner_list_app.vue';
+import { captureException } from '~/runner/sentry_utils';
 
 import { runnersData, runnersDataPaginated } from '../mock_data';
 
 const mockRegistrationToken = 'MOCK_REGISTRATION_TOKEN';
 const mockActiveRunnersCount = 2;
 
-jest.mock('@sentry/browser');
+jest.mock('~/flash');
+jest.mock('~/runner/sentry_utils');
 jest.mock('~/lib/utils/url_utility', () => ({
   ...jest.requireActual('~/lib/utils/url_utility'),
   updateHistory: jest.fn(),
@@ -80,11 +82,6 @@ describe('RunnerListApp', () => {
   beforeEach(async () => {
     setQuery('');
 
-    Sentry.withScope.mockImplementation((fn) => {
-      const scope = { setTag: jest.fn() };
-      fn(scope);
-    });
-
     mockRunnersQuery = jest.fn().mockResolvedValue(runnersData);
     createComponentWithApollo();
     await waitForPromises();
@@ -191,15 +188,21 @@ describe('RunnerListApp', () => {
 
   describe('when runners query fails', () => {
     beforeEach(async () => {
-      mockRunnersQuery = jest.fn().mockRejectedValue(new Error());
+      mockRunnersQuery = jest.fn().mockRejectedValue(new Error('Error!'));
       createComponentWithApollo();
 
       await waitForPromises();
     });
 
     it('error is reported to sentry', async () => {
-      expect(Sentry.withScope).toHaveBeenCalled();
-      expect(Sentry.captureException).toHaveBeenCalled();
+      expect(captureException).toHaveBeenCalledWith({
+        error: new Error('Network error: Error!'),
+        component: 'RunnerListApp',
+      });
+    });
+
+    it('error is shown to the user', async () => {
+      expect(createFlash).toHaveBeenCalledTimes(1);
     });
   });
 

spec/frontend/runner/sentry_utils_spec.js

+import * as Sentry from '@sentry/browser';
+import { captureException } from '~/runner/sentry_utils';
+
+jest.mock('@sentry/browser');
+
+describe('~/runner/sentry_utils', () => {
+  let mockSetTag;
+
+  beforeEach(async () => {
+    mockSetTag = jest.fn();
+
+    Sentry.withScope.mockImplementation((fn) => {
+      const scope = { setTag: mockSetTag };
+      fn(scope);
+    });
+  });
+
+  describe('captureException', () => {
+    const mockError = new Error('Something went wrong!');
+
+    it('error is reported to sentry', () => {
+      captureException({ error: mockError });
+
+      expect(Sentry.withScope).toHaveBeenCalled();
+      expect(Sentry.captureException).toHaveBeenCalledWith(mockError);
+    });
+
+    it('error is reported to sentry with a component name', () => {
+      const mockComponentName = 'MyComponent';
+
+      captureException({ error: mockError, component: mockComponentName });
+
+      expect(Sentry.withScope).toHaveBeenCalled();
+      expect(Sentry.captureException).toHaveBeenCalledWith(mockError);
+
+      expect(mockSetTag).toHaveBeenCalledWith('vue_component', mockComponentName);
+    });
+  });
+});