refactor(pipelines): incorporate merge-request review findings

app/assets/javascripts/lib/utils/common_utils.js

@@ -483,6 +483,16 @@ export const historyPushState = newUrl => {
   window.history.pushState({}, document.title, newUrl);
 };
 
+/**
+ * Based on the current location and the string parameters provided
+ * overwrites the current entry in the history without reloading the page.
+ *
+ * @param {String} param
+ */
+export const historyReplaceState = newUrl => {
+  window.history.replaceState({}, document.title, newUrl);
+};
+
 /**
  * Returns true for a String value of "true" and false otherwise.
  * This is the opposite of Boolean(...).toString().

app/assets/javascripts/pages/projects/pipelines/index/index.js

 import Vue from 'vue';
+import { GlToast } from '@gitlab/ui';
+import { doesHashExistInUrl } from '~/lib/utils/url_utility';
+import {
+  parseBoolean,
+  historyReplaceState,
+  buildUrlWithCurrentLocation,
+} from '~/lib/utils/common_utils';
+import { __ } from '~/locale';
 import PipelinesStore from '../../../../pipelines/stores/pipelines_store';
 import pipelinesComponent from '../../../../pipelines/components/pipelines.vue';
 import Translate from '../../../../vue_shared/translate';
-import { parseBoolean } from '../../../../lib/utils/common_utils';
 
 Vue.use(Translate);
+Vue.use(GlToast);
 
 document.addEventListener(
   'DOMContentLoaded',
@@ -21,6 +29,11 @@ document.addEventListener(
       },
       created() {
         this.dataset = document.querySelector(this.$options.el).dataset;
+
+        if (doesHashExistInUrl('delete_success')) {
+          this.$toast.show(__('The pipeline has been deleted'));
+          historyReplaceState(buildUrlWithCurrentLocation());
+        }
       },
       render(createElement) {
         return createElement('pipelines-component', {

app/assets/javascripts/pipelines/components/header_component.vue

@@ -4,6 +4,8 @@ import ciHeader from '../../vue_shared/components/header_ci_component.vue';
 import eventHub from '../event_hub';
 import { __ } from '~/locale';
 
+const DELETE_MODAL_ID = 'pipeline-delete-modal';
+
 export default {
   name: 'PipelineHeaderSection',
   components: {
@@ -34,6 +36,11 @@ export default {
     shouldRenderContent() {
       return !this.isLoading && Object.keys(this.pipeline).length;
     },
+    deleteModalConfirmationText() {
+      return __(
+        'Are you sure you want to delete this pipeline? Doing so will expire all pipeline caches and delete all related objects, such as builds, logs, artifacts, and triggers. This action cannot be undone.',
+      );
+    },
   },
 
   watch: {
@@ -43,6 +50,13 @@ export default {
   },
 
   methods: {
+    onActionClicked(action) {
+      if (action.modal) {
+        this.$root.$emit('bv::show::modal', action.modal);
+      } else {
+        this.postAction(action);
+      }
+    },
     postAction(action) {
       const index = this.actions.indexOf(action);
 
@@ -51,7 +65,7 @@ export default {
       eventHub.$emit('headerPostAction', action);
     },
     deletePipeline() {
-      const index = this.actions.findIndex(action => action.modal === 'pipeline-delete-modal');
+      const index = this.actions.findIndex(action => action.modal === DELETE_MODAL_ID);
 
       this.$set(this.actions[index], 'isLoading', true);
 
@@ -85,9 +99,9 @@ export default {
         actions.push({
           label: __('Delete'),
           path: this.pipeline.delete_path,
-          modal: 'pipeline-delete-modal',
+          modal: DELETE_MODAL_ID,
           cssClass: 'js-btn-delete-pipeline btn btn-danger btn-inverted',
-          type: 'modal-button',
+          type: 'button',
           isLoading: false,
         });
       }
@@ -95,6 +109,7 @@ export default {
       return actions;
     },
   },
+  DELETE_MODAL_ID,
 };
 </script>
 <template>
@@ -107,24 +122,20 @@ export default {
       :user="pipeline.user"
       :actions="actions"
       item-name="Pipeline"
-      @actionClicked="postAction"
+      @actionClicked="onActionClicked"
     />
 
     <gl-loading-icon v-if="isLoading" :size="2" class="prepend-top-default append-bottom-default" />
 
     <gl-modal
-      modal-id="pipeline-delete-modal"
+      :modal-id="$options.DELETE_MODAL_ID"
       :title="__('Delete pipeline')"
       :ok-title="__('Delete pipeline')"
       ok-variant="danger"
       @ok="deletePipeline()"
     >
       <p>
-        {{
-          __(
-            'Are you sure you want to delete this pipeline? Doing so will expire all pipeline caches and delete all related objects, such as builds, logs, artifacts, and triggers. This action cannot be undone.',
-          )
-        }}
+        {{ deleteModalConfirmationText }}
       </p>
     </gl-modal>
   </div>

app/assets/javascripts/pipelines/mixins/pipelines.js

-import Vue from 'vue';
 import Visibility from 'visibilityjs';
-import { GlLoadingIcon, GlToast } from '@gitlab/ui';
-import { doesHashExistInUrl } from '~/lib/utils/url_utility';
-import { historyPushState, buildUrlWithCurrentLocation } from '~/lib/utils/common_utils';
+import { GlLoadingIcon } from '@gitlab/ui';
 import { __ } from '../../locale';
 import createFlash from '../../flash';
 import Poll from '../../lib/utils/poll';
@@ -12,8 +9,6 @@ import PipelinesTableComponent from '../components/pipelines_table.vue';
 import eventHub from '../event_hub';
 import { CANCEL_REQUEST } from '../constants';
 
-Vue.use(GlToast);
-
 export default {
   components: {
     PipelinesTableComponent,
@@ -62,11 +57,6 @@ export default {
       }
     });
 
-    if (doesHashExistInUrl('delete_success')) {
-      this.$toast.show(__('The pipeline has been deleted'));
-      historyPushState(buildUrlWithCurrentLocation());
-    }
-
     eventHub.$on('postAction', this.postAction);
     eventHub.$on('retryPipeline', this.postAction);
     eventHub.$on('clickedDropdown', this.updateTable);

app/assets/javascripts/pipelines/pipeline_details_bundle.js

@@ -80,9 +80,7 @@ export default () => {
         this.mediator.stopPipelinePoll();
         this.mediator.service
           .deleteAction(action.path)
-          .then(response =>
-            redirectTo(setUrlFragment(response.request.responseURL, 'delete_success')),
-          )
+          .then(({ request }) => redirectTo(setUrlFragment(request.responseURL, 'delete_success')))
           .catch(() => Flash(__('An error occurred while deleting the pipeline.')));
       },
     },

app/assets/javascripts/pipelines/pipeline_details_mediator.js

@@ -35,7 +35,7 @@ export default class pipelinesMediator {
       if (!Visibility.hidden()) {
         this.poll.restart();
       } else {
-        this.poll.stop();
+        this.stopPipelinePoll();
       }
     });
   }
@@ -51,7 +51,7 @@ export default class pipelinesMediator {
   }
 
   refreshPipeline() {
-    this.poll.stop();
+    this.stopPipelinePoll();
 
     return this.service
       .getPipeline()

app/assets/javascripts/vue_shared/components/header_ci_component.vue

 <script>
-import { GlTooltipDirective, GlLink, GlButton, GlModalDirective } from '@gitlab/ui';
+import { GlTooltipDirective, GlLink, GlButton } from '@gitlab/ui';
 import { __, sprintf } from '~/locale';
 import CiIconBadge from './ci_badge_link.vue';
 import TimeagoTooltip from './time_ago_tooltip.vue';
@@ -24,7 +24,6 @@ export default {
   },
   directives: {
     GlTooltip: GlTooltipDirective,
-    GlModal: GlModalDirective,
   },
   props: {
     status: {
@@ -128,17 +127,6 @@ export default {
           :label="action.label"
           @click="onClickAction(action)"
         />
-
-        <loading-button
-          v-else-if="action.type === 'modal-button'"
-          :key="i"
-          v-gl-modal="action.modal"
-          :loading="action.isLoading"
-          :disabled="action.isLoading"
-          :class="action.cssClass"
-          container-class="d-inline"
-          :label="action.label"
-        />
       </template>
     </section>
     <gl-button

spec/controllers/projects/pipelines_controller_spec.rb

@@ -756,8 +756,8 @@ describe Projects::PipelinesController do
 
         expect(response).to have_gitlab_http_status(303)
 
-        expect { build.reload }.to raise_error(ActiveRecord::RecordNotFound)
-        expect { pipeline.reload }.to raise_error(ActiveRecord::RecordNotFound)
+        expect(Ci::Build.exists?(build.id)).to be_falsy
+        expect(Ci::Pipeline.exists?(pipeline.id)).to be_falsy
       end
 
       context 'and builds are disabled' do

spec/javascripts/pipelines/header_component_spec.js

@@ -56,17 +56,19 @@ describe('Pipeline details header', () => {
   });
 
   describe('action buttons', () => {
-    it('should call postAction when retry button action is clicked', () => {
+    it('should call postAction when retry button action is clicked', done => {
       eventHub.$on('headerPostAction', action => {
         expect(action.path).toEqual('path');
+        done();
       });
 
       vm.$el.querySelector('.js-retry-button').click();
     });
 
-    it('should fire modal event when delete button action is clicked', () => {
+    it('should fire modal event when delete button action is clicked', done => {
       vm.$root.$on('bv::modal::show', action => {
         expect(action.componentId).toEqual('pipeline-delete-modal');
+        done();
       });
 
       vm.$el.querySelector('.js-btn-delete-pipeline').click();

spec/javascripts/vue_shared/components/header_ci_component_spec.js

@@ -35,13 +35,6 @@ describe('Header CI Component', () => {
           cssClass: 'btn',
           isLoading: false,
         },
-        {
-          label: 'Delete',
-          path: 'path',
-          type: 'modal-button',
-          cssClass: 'btn-modal',
-          isLoading: false,
-        },
       ],
       hasSidebarButton: true,
     };
@@ -78,12 +71,9 @@ describe('Header CI Component', () => {
 
     it('should render provided actions', () => {
       const btn = vm.$el.querySelector('.btn');
-      const btnModal = vm.$el.querySelector('.btn-modal');
 
       expect(btn.tagName).toEqual('BUTTON');
       expect(btn.textContent.trim()).toEqual(props.actions[0].label);
-      expect(btnModal.tagName).toEqual('BUTTON');
-      expect(btnModal.textContent.trim()).toEqual(props.actions[1].label);
     });
 
     it('should show loading icon', done => {

spec/serializers/pipeline_entity_spec.rb

@@ -128,7 +128,7 @@ describe PipelineEntity do
         let(:project) { create(:project, namespace: user.namespace) }
         let(:pipeline) { create(:ci_pipeline, project: project) }
 
-        it 'contains cancel path' do
+        it 'contains delete path' do
           expect(subject[:delete_path]).to be_present
         end
       end
@@ -137,7 +137,7 @@ describe PipelineEntity do
         let(:project) { create(:project) }
         let(:pipeline) { create(:ci_pipeline, project: project) }
 
-        it 'does not contain cancel path' do
+        it 'does not contain delete path' do
           expect(subject).not_to have_key(:delete_path)
         end
       end