From 15511ed14f595b30de90f32b3751c8599550e4c7 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda <filipa@gitlab.com> Date: Wed, 1 Aug 2018 15:57:05 +0100 Subject: [PATCH] Changes after review: - Cleans up CSS to use common classes - Removes getters to use mapState instead - Makes the first request even when tab is not visible - Show loading state in 204 --- .../components/grouped_test_reports_app.vue | 22 ++++++++++--------- .../javascripts/reports/components/modal.vue | 4 ++-- .../reports/components/test_issue_body.vue | 6 +---- .../javascripts/reports/store/actions.js | 16 +++++++++++--- .../javascripts/reports/store/getters.js | 12 ++-------- .../javascripts/reports/store/mutations.js | 8 ++++++- app/assets/javascripts/reports/store/state.js | 2 -- app/assets/javascripts/reports/store/utils.js | 9 ++++---- .../vue_shared/components/code_block.vue | 4 ++-- app/assets/stylesheets/framework/blocks.scss | 9 -------- app/assets/stylesheets/framework/common.scss | 2 +- .../grouped_test_reports_app_spec.js | 20 +++++++++++++++++ .../components/test_issue_body_spec.js | 1 - .../reports/store/mutations_spec.js | 14 +++--------- 14 files changed, 67 insertions(+), 62 deletions(-) diff --git a/app/assets/javascripts/reports/components/grouped_test_reports_app.vue b/app/assets/javascripts/reports/components/grouped_test_reports_app.vue index d2e72e72421..0fc84b4552a 100644 --- a/app/assets/javascripts/reports/components/grouped_test_reports_app.vue +++ b/app/assets/javascripts/reports/components/grouped_test_reports_app.vue @@ -1,5 +1,5 @@ <script> - import { mapActions, mapGetters } from 'vuex'; + import { mapActions, mapGetters, mapState } from 'vuex'; import { s__ } from '~/locale'; import { componentNames } from '~/vue_shared/components/reports/issue_body'; import ReportSection from '~/vue_shared/components/reports/report_section.vue'; @@ -26,27 +26,29 @@ }, componentNames, computed: { - ...mapGetters([ + ...mapState([ 'reports', - 'summaryStatus', 'isLoading', 'hasError', - 'summaryCounts', - 'modalTitle', - 'modalData', - 'isCreatingNewIssue', + 'summary', + ]), + ...mapState({ + modalTitle: state => state.modal.title || '', + modalData: state => state.modal.data || {}, + }), + ...mapGetters([ + 'summaryStatus', ]), - groupedSummaryText() { if (this.isLoading) { return s__('Reports|Test summary results are being parsed'); } - if (this.hasError || !this.summaryCounts) { + if (this.hasError) { return s__('Reports|Test summary failed loading results'); } - return summaryTextBuilder(s__('Reports|Test summary'), this.summaryCounts); + return summaryTextBuilder(s__('Reports|Test summary'), this.summary); }, }, created() { diff --git a/app/assets/javascripts/reports/components/modal.vue b/app/assets/javascripts/reports/components/modal.vue index 6257ebf71fc..b2133714858 100644 --- a/app/assets/javascripts/reports/components/modal.vue +++ b/app/assets/javascripts/reports/components/modal.vue @@ -36,9 +36,9 @@ :key="index" class="row prepend-top-10 append-bottom-10" > - <label class="col-sm-2 text-right font-weight-bold"> + <strong class="col-sm-2 text-right"> {{ field.text }}: - </label> + </strong> <div class="col-sm-10 text-secondary"> <code-block diff --git a/app/assets/javascripts/reports/components/test_issue_body.vue b/app/assets/javascripts/reports/components/test_issue_body.vue index 13b7fdf58bc..cd443a49b52 100644 --- a/app/assets/javascripts/reports/components/test_issue_body.vue +++ b/app/assets/javascripts/reports/components/test_issue_body.vue @@ -21,10 +21,6 @@ }, methods: { ...mapActions(['openModal']), - handleIssueClick() { - const { issue, status, openModal } = this; - openModal({ issue, status }); - }, }, }; </script> @@ -34,7 +30,7 @@ <button type="button" class="btn-link btn-blank text-left break-link vulnerability-name-button" - @click="handleIssueClick()" + @click="openModal({ issue })" > <div v-if="isNew" diff --git a/app/assets/javascripts/reports/store/actions.js b/app/assets/javascripts/reports/store/actions.js index edbf860ecc6..e8f6b53cea0 100644 --- a/app/assets/javascripts/reports/store/actions.js +++ b/app/assets/javascripts/reports/store/actions.js @@ -3,6 +3,7 @@ import $ from 'jquery'; import axios from '../../lib/utils/axios_utils'; import Poll from '../../lib/utils/poll'; import * as types from './mutation_types'; +import httpStatusCodes from '../../lib/utils/http_status'; export const setEndpoint = ({ commit }, endpoint) => commit(types.SET_ENDPOINT, endpoint); @@ -42,12 +43,17 @@ export const fetchReports = ({ state, dispatch }) => { }, data: state.endpoint, method: 'getReports', - successCallback: ({ data }) => dispatch('receiveReportsSuccess', data), + successCallback: (response) => dispatch('receiveReportsSuccess', response), errorCallback: () => dispatch('receiveReportsError'), }); if (!Visibility.hidden()) { eTagPoll.makeRequest(); + } else { + axios + .get(state.endpoint) + .then((response) => dispatch('receiveReportsSuccess', response)) + .catch(() => dispatch('receiveReportsError')); } Visibility.change(() => { @@ -59,8 +65,12 @@ export const fetchReports = ({ state, dispatch }) => { }); }; -export const receiveReportsSuccess = ({ commit }, response) => - commit(types.RECEIVE_REPORTS_SUCCESS, response); +export const receiveReportsSuccess = ({ commit }, response) => { + // With 204 we keep polling and don't update the state + if (response.status === httpStatusCodes.OK) { + commit(types.RECEIVE_REPORTS_SUCCESS, response.data); + } +}; export const receiveReportsError = ({ commit }) => commit(types.RECEIVE_REPORTS_ERROR); diff --git a/app/assets/javascripts/reports/store/getters.js b/app/assets/javascripts/reports/store/getters.js index 5fbafa201b5..95266194acb 100644 --- a/app/assets/javascripts/reports/store/getters.js +++ b/app/assets/javascripts/reports/store/getters.js @@ -1,19 +1,11 @@ -import { LOADING, ERROR, SUCCESS } from '../constants'; - -export const reports = state => state.reports; -export const summaryCounts = state => state.summary; -export const isLoading = state => state.isLoading; -export const hasError = state => state.hasError; -export const modalTitle = state => state.modal.title || ''; -export const modalData = state => state.modal.data || {}; -export const isCreatingNewIssue = state => state.modal.isLoading; +import { LOADING, ERROR, SUCCESS, STATUS_FAILED } from '../constants'; export const summaryStatus = state => { if (state.isLoading) { return LOADING; } - if (state.hasError) { + if (state.hasError || state.status === STATUS_FAILED) { return ERROR; } diff --git a/app/assets/javascripts/reports/store/mutations.js b/app/assets/javascripts/reports/store/mutations.js index 6b900356bfc..e806d120b51 100644 --- a/app/assets/javascripts/reports/store/mutations.js +++ b/app/assets/javascripts/reports/store/mutations.js @@ -23,11 +23,17 @@ export default { [types.RECEIVE_REPORTS_ERROR](state) { state.isLoading = false; state.hasError = true; + state.reports = []; + state.summary = { + total: 0, + resolved: 0, + failed: 0, + }; + state.status = null; }, [types.SET_ISSUE_MODAL_DATA](state, payload) { state.modal.title = payload.issue.name; - state.modal.status = payload.status; Object.keys(payload.issue).forEach((key) => { if (Object.prototype.hasOwnProperty.call(state.modal.data, key)) { diff --git a/app/assets/javascripts/reports/store/state.js b/app/assets/javascripts/reports/store/state.js index fd1819dbfea..4cab2e27a16 100644 --- a/app/assets/javascripts/reports/store/state.js +++ b/app/assets/javascripts/reports/store/state.js @@ -34,8 +34,6 @@ export default () => ({ modal: { title: null, - status: null, - data: { class: { value: null, diff --git a/app/assets/javascripts/reports/store/utils.js b/app/assets/javascripts/reports/store/utils.js index caa4039af72..35632218269 100644 --- a/app/assets/javascripts/reports/store/utils.js +++ b/app/assets/javascripts/reports/store/utils.js @@ -17,7 +17,8 @@ const textBuilder = results => { ? n__('%d fixed test result', '%d fixed test results', resolved) : null; const totalString = total ? n__('out of %d total test', 'out of %d total tests', total) : null; - let resultsString; + + let resultsString = s__('Reports|no changed test results'); if (failed) { if (resolved) { @@ -30,19 +31,17 @@ const textBuilder = results => { } } else if (resolved) { resultsString = resolvedString; - } else { - resultsString = s__('Reports|no changed test results'); } return `${resultsString} ${totalString}`; }; -export const summaryTextBuilder = (name = '', results) => { +export const summaryTextBuilder = (name = '', results = {}) => { const resultsString = textBuilder(results); return `${name} contained ${resultsString}`; }; -export const reportTextBuilder = (name = '', results) => { +export const reportTextBuilder = (name = '', results = {}) => { const resultsString = textBuilder(results); return `${name} found ${resultsString}`; }; diff --git a/app/assets/javascripts/vue_shared/components/code_block.vue b/app/assets/javascripts/vue_shared/components/code_block.vue index 30920a2a86d..3cca7a86bef 100644 --- a/app/assets/javascripts/vue_shared/components/code_block.vue +++ b/app/assets/javascripts/vue_shared/components/code_block.vue @@ -10,7 +10,7 @@ export default { }; </script> <template> - <pre class="code-block build-trace-rounded"> - <code class="bash">{{ code }}</code> + <pre class="code-block rounded"> + <code class="d-block">{{ code }}</code> </pre> </template> diff --git a/app/assets/stylesheets/framework/blocks.scss b/app/assets/stylesheets/framework/blocks.scss index 12fc307130a..7145a76db6d 100644 --- a/app/assets/stylesheets/framework/blocks.scss +++ b/app/assets/stylesheets/framework/blocks.scss @@ -360,7 +360,6 @@ white-space: pre; overflow-x: auto; font-size: 12px; - border-radius: 0; border: 0; padding: $grid-size; @@ -368,12 +367,4 @@ background-color: inherit; padding: inherit; } - - .bash { - display: block; - } - - &.build-trace-rounded { - border-radius: $border-radius-base; - } } diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index 3825cbf4b96..c9865610b78 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -469,4 +469,4 @@ img.emoji { .inline { display: inline-block; } .center { text-align: center; } .vertical-align-middle { vertical-align: middle; } -.flex-align-self-center { align-self: center; } \ No newline at end of file +.flex-align-self-center { align-self: center; } diff --git a/spec/javascripts/reports/components/grouped_test_reports_app_spec.js b/spec/javascripts/reports/components/grouped_test_reports_app_spec.js index 6ca03620ca3..d86e565036c 100644 --- a/spec/javascripts/reports/components/grouped_test_reports_app_spec.js +++ b/spec/javascripts/reports/components/grouped_test_reports_app_spec.js @@ -49,6 +49,26 @@ describe('Grouped Test Reports App', () => { }); }); + describe('with 204 result', () => { + beforeEach(() => { + mock.onGet('test_results.json').reply(204, {}, {}); + vm = mountComponent(Component, { + endpoint: 'test_results.json', + }); + }); + + it('renders success summary text', done => { + setTimeout(() => { + expect(vm.$el.querySelector('.fa-spinner')).not.toBeNull(); + expect(vm.$el.querySelector('.js-code-text').textContent.trim()).toEqual( + 'Test summary results are being parsed', + ); + + done(); + }, 0); + }); + }); + describe('with new failed result', () => { beforeEach(() => { mock.onGet('test_results.json').reply(200, newFailedTestReports, {}); diff --git a/spec/javascripts/reports/components/test_issue_body_spec.js b/spec/javascripts/reports/components/test_issue_body_spec.js index 242eed114d9..0ea81f714e7 100644 --- a/spec/javascripts/reports/components/test_issue_body_spec.js +++ b/spec/javascripts/reports/components/test_issue_body_spec.js @@ -31,7 +31,6 @@ describe('Test Issue body', () => { vm.$el.querySelector('button').click(); expect(vm.openModal).toHaveBeenCalledWith({ issue: commonProps.issue, - status: commonProps.status, }); }); }); diff --git a/spec/javascripts/reports/store/mutations_spec.js b/spec/javascripts/reports/store/mutations_spec.js index c3e2bb8e00c..8f99d2675a5 100644 --- a/spec/javascripts/reports/store/mutations_spec.js +++ b/spec/javascripts/reports/store/mutations_spec.js @@ -43,24 +43,21 @@ describe('Reports Store Mutations', () => { { name: 'StringHelper#concatenate when a is git and b is lab returns summary', execution_time: 0.0092435, - system_output: - 'Failure/Error: is_expected.to eq(\'gitlab\')', + system_output: "Failure/Error: is_expected.to eq('gitlab')", }, ], resolved_failures: [ { name: 'StringHelper#concatenate when a is git and b is lab returns summary', execution_time: 0.009235, - system_output: - 'Failure/Error: is_expected.to eq(\'gitlab\')', + system_output: "Failure/Error: is_expected.to eq('gitlab')", }, ], existing_failures: [ { name: 'StringHelper#concatenate when a is git and b is lab returns summary', execution_time: 1232.08, - system_output: - 'Failure/Error: is_expected.to eq(\'gitlab\')', + system_output: "Failure/Error: is_expected.to eq('gitlab')", }, ], }, @@ -108,7 +105,6 @@ describe('Reports Store Mutations', () => { beforeEach(() => { mutations[types.SET_ISSUE_MODAL_DATA](stateCopy, { issue, - status: 'failed', }); }); @@ -116,10 +112,6 @@ describe('Reports Store Mutations', () => { expect(stateCopy.modal.title).toEqual(issue.name); }); - it('should set modal status', () => { - expect(stateCopy.modal.status).toEqual('failed'); - }); - it('should set modal data', () => { expect(stateCopy.modal.data.execution_time.value).toEqual(issue.execution_time); expect(stateCopy.modal.data.system_output.value).toEqual(issue.system_output); -- 2.30.9