diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-11-30 12:09:21 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-11-30 12:09:21 +0000 |
commit | f66bb12f3879bf86387157af1e614c5e0e93e561 (patch) | |
tree | 48fc2a455fab6ad9cec3f59c91cc0f121a1e108f | |
parent | 06eadec5527fe78b0c383a949a64b8f2a49a6360 (diff) | |
download | gitlab-ce-f66bb12f3879bf86387157af1e614c5e0e93e561.tar.gz |
Add latest changes from gitlab-org/gitlab@master
30 files changed, 535 insertions, 126 deletions
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 7ce38a2ddcf..74091286619 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -d5024702496569b6de051046e5e295c44de94be5 +f085d841e2f5571b260910a454215bc1a7687bf0 diff --git a/app/assets/javascripts/boards/components/board_content.vue b/app/assets/javascripts/boards/components/board_content.vue index 54ce736e26b..a768cd833ad 100644 --- a/app/assets/javascripts/boards/components/board_content.vue +++ b/app/assets/javascripts/boards/components/board_content.vue @@ -1,10 +1,13 @@ <script> +import Draggable from 'vuedraggable'; import { mapState, mapGetters, mapActions } from 'vuex'; import { sortBy } from 'lodash'; import { GlAlert } from '@gitlab/ui'; import BoardColumn from 'ee_else_ce/boards/components/board_column.vue'; import BoardColumnNew from './board_column_new.vue'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; +import defaultSortableConfig from '~/sortable/sortable_config'; +import { sortableEnd, sortableStart } from '~/boards/mixins/sortable_default_options'; export default { components: { @@ -36,6 +39,25 @@ export default { ? sortBy([...Object.values(this.boardLists)], 'position') : this.lists; }, + canDragColumns() { + return this.glFeatures.graphqlBoardLists && this.canAdminList; + }, + boardColumnWrapper() { + return this.canDragColumns ? Draggable : 'div'; + }, + draggableOptions() { + const options = { + ...defaultSortableConfig, + disabled: this.disabled, + draggable: '.is-draggable', + fallbackOnBody: false, + group: 'boards-list', + tag: 'div', + value: this.lists, + }; + + return this.canDragColumns ? options : {}; + }, }, mounted() { if (this.glFeatures.graphqlBoardLists) { @@ -43,7 +65,26 @@ export default { } }, methods: { - ...mapActions(['showPromotionList']), + ...mapActions(['moveList', 'showPromotionList']), + handleDragOnStart() { + sortableStart(); + }, + + handleDragOnEnd(params) { + sortableEnd(); + + const { item, newIndex, oldIndex, to } = params; + + const listId = item.dataset.id; + const replacedListId = to.children[newIndex].dataset.id; + + this.moveList({ + listId, + replacedListId, + newIndex, + adjustmentValue: newIndex < oldIndex ? 1 : -1, + }); + }, }, }; </script> @@ -53,16 +94,28 @@ export default { <gl-alert v-if="error" variant="danger" :dismissible="false"> {{ error }} </gl-alert> - <div v-if="!isSwimlanesOn" class="boards-list gl-w-full gl-py-5 gl-px-3 gl-white-space-nowrap"> + <component + :is="boardColumnWrapper" + v-if="!isSwimlanesOn" + ref="list" + v-bind="draggableOptions" + class="boards-list gl-w-full gl-py-5 gl-px-3 gl-white-space-nowrap" + data-qa-selector="boards_list" + @start="handleDragOnStart" + @end="handleDragOnEnd" + > <board-column v-for="list in boardListsToUse" :key="list.id" ref="board" :can-admin-list="canAdminList" + :class="{ + 'is-draggable': !list.preset, + }" :list="list" :disabled="disabled" /> - </div> + </component> <template v-else> <epics-swimlanes diff --git a/app/assets/javascripts/boards/stores/actions.js b/app/assets/javascripts/boards/stores/actions.js index 00c64bff74e..2b6d606c56e 100644 --- a/app/assets/javascripts/boards/stores/actions.js +++ b/app/assets/javascripts/boards/stores/actions.js @@ -180,6 +180,10 @@ export default { { state, commit, dispatch }, { listId, replacedListId, newIndex, adjustmentValue }, ) => { + if (listId === replacedListId) { + return; + } + const { boardLists } = state; const backupList = { ...boardLists }; const movedList = boardLists[listId]; diff --git a/app/assets/javascripts/diffs/components/settings_dropdown.vue b/app/assets/javascripts/diffs/components/settings_dropdown.vue index 78647065c8e..590b2127e6b 100644 --- a/app/assets/javascripts/diffs/components/settings_dropdown.vue +++ b/app/assets/javascripts/diffs/components/settings_dropdown.vue @@ -1,7 +1,6 @@ <script> import { mapActions, mapGetters, mapState } from 'vuex'; import { GlButtonGroup, GlButton, GlDropdown } from '@gitlab/ui'; -import { __ } from '~/locale'; export default { components: { @@ -13,12 +12,6 @@ export default { ...mapGetters('diffs', ['isInlineView', 'isParallelView']), ...mapState('diffs', ['renderTreeList', 'showWhitespace']), }, - mounted() { - this.patchAriaLabel(); - }, - updated() { - this.patchAriaLabel(); - }, methods: { ...mapActions('diffs', [ 'setInlineDiffViewType', @@ -26,17 +19,18 @@ export default { 'setRenderTreeList', 'setShowWhitespace', ]), - patchAriaLabel() { - this.$el - .querySelector('.js-show-diff-settings') - .setAttribute('aria-label', __('Diff view settings')); - }, }, }; </script> <template> - <gl-dropdown icon="settings" toggle-class="js-show-diff-settings" right> + <gl-dropdown + icon="settings" + :text="__('Diff view settings')" + :text-sr-only="true" + toggle-class="js-show-diff-settings" + right + > <div class="gl-px-3"> <span class="gl-font-weight-bold gl-display-block gl-mb-2">{{ __('File browser') }}</span> <gl-button-group class="gl-display-flex"> diff --git a/app/assets/stylesheets/bootstrap_migration.scss b/app/assets/stylesheets/bootstrap_migration.scss index 3d5076f485c..deeef86c386 100644 --- a/app/assets/stylesheets/bootstrap_migration.scss +++ b/app/assets/stylesheets/bootstrap_migration.scss @@ -235,10 +235,6 @@ h3.popover-header { @extend .border-0; } - &.card-without-margin { - margin: 0; - } - &.bg-light { @extend .border-0; } diff --git a/app/assets/stylesheets/framework/lists.scss b/app/assets/stylesheets/framework/lists.scss index 2464ea3607b..b0334da6943 100644 --- a/app/assets/stylesheets/framework/lists.scss +++ b/app/assets/stylesheets/framework/lists.scss @@ -234,6 +234,8 @@ ul.content-list { } } +ul.content-list.issuable-list > li, +ul.content-list.todos-list > li, .card > .content-list > li { padding: $gl-padding-top $gl-padding; } diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb index cf1efda5d13..28b419717c5 100644 --- a/app/controllers/projects/branches_controller.rb +++ b/app/controllers/projects/branches_controller.rb @@ -42,10 +42,6 @@ class Projects::BranchesController < Projects::ApplicationController end end - def recent - @branches = @repository.recent_branches - end - def diverging_commit_counts respond_to do |format| format.json do diff --git a/app/finders/ci/pipelines_finder.rb b/app/finders/ci/pipelines_finder.rb index 7347a83d294..a77faebb160 100644 --- a/app/finders/ci/pipelines_finder.rb +++ b/app/finders/ci/pipelines_finder.rb @@ -18,7 +18,9 @@ module Ci return Ci::Pipeline.none end - items = pipelines.no_child + items = pipelines + items = items.no_child unless params[:iids].present? + items = by_iids(items) items = by_scope(items) items = by_status(items) items = by_ref(items) @@ -52,6 +54,14 @@ module Ci project.repository.tag_names end + def by_iids(items) + if params[:iids].present? + items.for_iid(params[:iids]) + else + items + end + end + def by_scope(items) case params[:scope] when 'running' diff --git a/app/graphql/resolvers/project_pipeline_resolver.rb b/app/graphql/resolvers/project_pipeline_resolver.rb index 4cf47dbdc60..8bf4e0b08ef 100644 --- a/app/graphql/resolvers/project_pipeline_resolver.rb +++ b/app/graphql/resolvers/project_pipeline_resolver.rb @@ -12,7 +12,9 @@ module Resolvers def resolve(iid:) BatchLoader::GraphQL.for(iid).batch(key: project) do |iids, loader, args| - args[:key].all_pipelines.for_iid(iids).each { |pl| loader.call(pl.iid.to_s, pl) } + finder = ::Ci::PipelinesFinder.new(project, context[:current_user], iids: iids) + + finder.execute.each { |pipeline| loader.call(pipeline.iid.to_s, pipeline) } end end end diff --git a/app/views/dashboard/todos/index.html.haml b/app/views/dashboard/todos/index.html.haml index 221d79a30ce..c34e457dbd9 100644 --- a/app/views/dashboard/todos/index.html.haml +++ b/app/views/dashboard/todos/index.html.haml @@ -85,9 +85,8 @@ - if @todos.any? .js-todos-list-container{ data: { qa_selector: "todos_list_container" } } .js-todos-options{ data: { per_page: @todos.limit_value, current_page: @todos.current_page, total_pages: @todos.total_pages } } - .card.card-without-border.card-without-margin - %ul.content-list.todos-list - = render @todos + %ul.content-list.todos-list + = render @todos = paginate @todos, theme: "gitlab" .js-nothing-here-container.todos-all-done.hidden.svg-content = image_tag 'illustrations/todos_all_done.svg' diff --git a/app/views/shared/_issues.html.haml b/app/views/shared/_issues.html.haml index 0f38d0e3b39..57575f89803 100644 --- a/app/views/shared/_issues.html.haml +++ b/app/views/shared/_issues.html.haml @@ -1,7 +1,6 @@ - if @issues.to_a.any? - .card.card-without-border - %ul.content-list.issues-list.issuable-list{ class: ("manual-ordering" if @sort == 'relative_position'), data: { group_full_path: @group&.full_path } } - = render partial: 'projects/issues/issue', collection: @issues + %ul.content-list.issues-list.issuable-list{ class: ("manual-ordering" if @sort == 'relative_position'), data: { group_full_path: @group&.full_path } } + = render partial: 'projects/issues/issue', collection: @issues = paginate @issues, theme: "gitlab" - else = render 'shared/empty_states/issues' diff --git a/app/views/shared/_merge_requests.html.haml b/app/views/shared/_merge_requests.html.haml index d280df8b370..dc8efa3e734 100644 --- a/app/views/shared/_merge_requests.html.haml +++ b/app/views/shared/_merge_requests.html.haml @@ -1,7 +1,6 @@ - if @merge_requests.to_a.any? - .card.card-without-border - %ul.content-list.mr-list.issuable-list - = render partial: 'projects/merge_requests/merge_request', collection: @merge_requests + %ul.content-list.mr-list.issuable-list + = render partial: 'projects/merge_requests/merge_request', collection: @merge_requests = paginate @merge_requests, theme: "gitlab" diff --git a/app/views/shared/issuable/form/_type_selector.html.haml b/app/views/shared/issuable/form/_type_selector.html.haml index 5d64c15d9f9..67bc4019a82 100644 --- a/app/views/shared/issuable/form/_type_selector.html.haml +++ b/app/views/shared/issuable/form/_type_selector.html.haml @@ -13,7 +13,7 @@ .dropdown-title.gl-display-flex %span.gl-ml-auto = _("Select type") - %button.dropdown-title-button.dropdown-menu-close.gl-ml-auto{ "aria-label" => _('Close') } + %button.dropdown-title-button.dropdown-menu-close.gl-ml-auto{ type: 'button', "aria-label" => _('Close') } = sprite_icon('close', size: 16, css_class: 'dropdown-menu-close-icon') .dropdown-content %ul diff --git a/changelogs/unreleased/273580-when-selecting-issue-type-and-closing-the-dropdown-the-issue-gets-.yml b/changelogs/unreleased/273580-when-selecting-issue-type-and-closing-the-dropdown-the-issue-gets-.yml new file mode 100644 index 00000000000..6cf89bb53f4 --- /dev/null +++ b/changelogs/unreleased/273580-when-selecting-issue-type-and-closing-the-dropdown-the-issue-gets-.yml @@ -0,0 +1,6 @@ +--- +title: Adds type="button" to the close button for the issue type selector to prevent + accidental form submission +merge_request: 48249 +author: +type: fixed diff --git a/changelogs/unreleased/284597-remove-bootstrap-4-s-cards-components-from-dashboard-todos.yml b/changelogs/unreleased/284597-remove-bootstrap-4-s-cards-components-from-dashboard-todos.yml new file mode 100644 index 00000000000..3cd7999df9e --- /dev/null +++ b/changelogs/unreleased/284597-remove-bootstrap-4-s-cards-components-from-dashboard-todos.yml @@ -0,0 +1,5 @@ +--- +title: Remove Bootstrap 4's Cards components from Issuables and Todos +merge_request: 48004 +author: Takuya Noguchi +type: performance diff --git a/danger/pipeline/Dangerfile b/danger/pipeline/Dangerfile new file mode 100644 index 00000000000..0342a39554b --- /dev/null +++ b/danger/pipeline/Dangerfile @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +MESSAGE = <<~MESSAGE +## Pipeline Changes + +This Merge Request contains changes to the pipeline configuration for the GitLab project. + +Please consider the effect of the changes in this Merge Request on the following: +- Effects on different [pipeline types](https://docs.gitlab.com/ee/development/pipelines.html#pipelines-for-merge-requests) +- Effects on non-canonical projects (`gitlab-foss`, `security`, etc) +- Effects on [pipeline performance](https://about.gitlab.com/handbook/engineering/quality/performance-indicators/#average-merge-request-pipeline-duration-for-gitlab) +- Effects on fork pipelines + +Please consider communicating these changes to the broader team following the [communication guideline for pipeline changes](https://about.gitlab.com/handbook/engineering/quality/engineering-productivity-team/#pipeline-changes) +MESSAGE + +if helper.has_ci_changes? + markdown(MESSAGE) +end diff --git a/doc/development/fe_guide/graphql.md b/doc/development/fe_guide/graphql.md index 762281756fe..e51cb8a08cd 100644 --- a/doc/development/fe_guide/graphql.md +++ b/doc/development/fe_guide/graphql.md @@ -439,9 +439,11 @@ parameter, indicating a starting or ending point of our pagination. They should followed with `first` or `last` parameter respectively to indicate _how many_ items we want to fetch after or before a given endpoint. -For example, here we're fetching 10 designs after a cursor: +For example, here we're fetching 10 designs after a cursor (let us call this `projectQuery`): ```javascript +#import "~/graphql_shared/fragments/pageInfo.fragment.graphql" + query { project(fullPath: "root/my-project") { id @@ -453,6 +455,9 @@ query { id } } + pageInfo { + ...PageInfo + } } } } @@ -460,21 +465,31 @@ query { } ``` +Note that we are using the [`pageInfo.fragment.graphql`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/javascripts/graphql_shared/fragments/pageInfo.fragment.graphql) to populate the `pageInfo` information. + #### Using `fetchMore` method in components +This approach makes sense to use with user-handled pagination (e.g. when the scrolls to fetch more data or explicitly clicks a "Next Page"-button). +When we need to fetch all the data initially, it is recommended to use [a (non-smart) query, instead](#using-a-recursive-query-in-components). + When making an initial fetch, we usually want to start a pagination from the beginning. In this case, we can either: - Skip passing a cursor. - Pass `null` explicitly to `after`. -After data is fetched, we should save a `pageInfo` object. Let's assume we're storing -it to Vue component `data`: +After data is fetched, we can use the `update`-hook as an opportunity [to customize +the data that is set in the Vue component property](https://apollo.vuejs.org/api/smart-query.html#options), getting a hold of the `pageInfo` object among other data. + +In the `result`-hook, we can inspect the `pageInfo` object to see if we need to fetch +the next page. Note that we also keep a `requestCount` to ensure that the application +does not keep requesting the next page, indefinitely: ```javascript data() { return { pageInfo: null, + requestCount: 0, } }, apollo: { @@ -482,13 +497,29 @@ apollo: { query: projectQuery, variables() { return { - // rest of design variables - ... + // ... The rest of the design variables first: 10, }; }, - result(res) { - this.pageInfo = res.data?.project?.issue?.designCollection?.designs?.pageInfo; + update(data) { + const { id = null, issue = {} } = data.project || {}; + const { edges = [], pageInfo } = issue.designCollection?.designs || {}; + + return { + id, + edges, + pageInfo, + }; + }, + result() { + const { pageInfo } = this.designs; + + // Increment the request count with each new result + this.requestCount += 1; + // Only fetch next page if we have more requests and there is a next page to fetch + if (this.requestCount < MAX_REQUEST_COUNT && pageInfo?.hasNextPage) { + this.fetchNextPage(pageInfo.endCursor); + } }, }, }, @@ -497,34 +528,102 @@ apollo: { When we want to move to the next page, we use an Apollo `fetchMore` method, passing a new cursor (and, optionally, new variables) there. In the `updateQuery` hook, we have to return a result we want to see in the Apollo cache after fetching the next page. +[`Immer`s `produce`](#immutability-and-cache-updates)-function can help us with the immutability here: ```javascript -fetchNextPage() { - // as a first step, we're checking if we have more pages to move forward - if (this.pageInfo?.hasNextPage) { - this.$apollo.queries.designs.fetchMore({ - variables: { - // rest of design variables - ... - first: 10, - after: this.pageInfo?.endCursor, - }, - updateQuery(previousResult, { fetchMoreResult }) { - // here we can implement the logic of adding new designs to fetched one (for example, if we use infinite scroll) - // or replacing old result with the new one if we use numbered pages +fetchNextPage(endCursor) { + this.$apollo.queries.designs.fetchMore({ + variables: { + // ... The rest of the design variables + first: 10, + after: endCursor, + }, + updateQuery(previousResult, { fetchMoreResult }) { + // Here we can implement the logic of adding new designs to existing ones + // (for example, if we use infinite scroll) or replacing old result + // with the new one if we use numbered pages + + const { designs: previousDesigns } = previousResult.project.issue.designCollection; + const { designs: newDesigns } = fetchMoreResult.project.issue.designCollection + + return produce(previousResult, draftData => { + // `produce` gives us a working copy, `draftData`, that we can modify + // as we please and from it will produce the next immutable result for us + draftData.project.issue.designCollection.designs = [...previousDesigns, ...newDesigns]; + }); + }, + }); +} +``` - const newDesigns = fetchMoreResult.project.issue.designCollection.designs; - previousResult.project.issue.designCollection.designs.push(...newDesigns) +#### Using a recursive query in components - return previousResult; - }, - }); +When it is necessary to fetch all paginated data initially an Apollo query can do the trick for us. +If we need to fetch the next page based on user interactions, it is recommend to use a [`smartQuery`](https://apollo.vuejs.org/api/smart-query.html) along with the [`fetchMore`-hook](#using-fetchmore-method-in-components). + +When the query resolves we can update the component data and inspect the `pageInfo` object +to see if we need to fetch the next page, i.e. call the method recursively. + +Note that we also keep a `requestCount` to ensure that the application does not keep +requesting the next page, indefinitely. + +```javascript +data() { + return { + requestCount: 0, + isLoading: false, + designs: { + edges: [], + pageInfo: null, + }, } -} -``` +}, +created() { + this.fetchDesigns(); +}, +methods: { + handleError(error) { + this.isLoading = false; + // Do something with `error` + }, + fetchDesigns(endCursor) { + this.isLoading = true; + + return this.$apollo + .query({ + query: projectQuery, + variables() { + return { + // ... The rest of the design variables + first: 10, + endCursor, + }; + }, + }) + .then(({ data }) => { + const { id = null, issue = {} } = data.project || {}; + const { edges = [], pageInfo } = issue.designCollection?.designs || {}; + + // Update data + this.designs = { + id, + edges: [...this.designs.edges, ...edges]; + pageInfo: pageInfo; + }; -Please note we don't have to save `pageInfo` one more time; `fetchMore` triggers a query -`result` hook as well. + // Increment the request count with each new result + this.requestCount += 1; + // Only fetch next page if we have more requests and there is a next page to fetch + if (this.requestCount < MAX_REQUEST_COUNT && pageInfo?.hasNextPage) { + this.fetchDesigns(pageInfo.endCursor); + } else { + this.isLoading = false; + } + }) + .catch(this.handleError); + }, +}, +``` ### Managing performance @@ -666,34 +765,51 @@ it('calls mutation on submitting form ', () => { To test the logic of Apollo cache updates, we might want to mock an Apollo Client in our unit tests. We use [`mock-apollo-client`](https://www.npmjs.com/package/mock-apollo-client) library to mock Apollo client and [`createMockApollo` helper](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/frontend/helpers/mock_apollo_helper.js) we created on top of it. -To separate tests with mocked client from 'usual' unit tests, it's recommended to create an additional component factory. This way we only create Apollo Client instance when it's necessary: +To separate tests with mocked client from 'usual' unit tests, it's recommended to create an additional factory and pass the created `mockApollo` as an option to the `createComponent`-factory. This way we only create Apollo Client instance when it's necessary. -```javascript -function createComponent() {...} - -function createComponentWithApollo() {...} -``` - -Then we need to inject `VueApollo` to Vue local instance (`localVue.use()` can also be called within `createComponentWithApollo()`) +We need to inject `VueApollo` to the Vue local instance and, likewise, it is recommended to call `localVue.use()` within `createMockApolloProvider()` to only load it when it is necessary. ```javascript import VueApollo from 'vue-apollo'; import { createLocalVue } from '@vue/test-utils'; const localVue = createLocalVue(); -localVue.use(VueApollo); + +function createMockApolloProvider() { + localVue.use(VueApollo); + + return createMockApollo(requestHandlers); +} + +function createComponent(options = {}) { + const { mockApollo } = options; + ... + return shallowMount(..., { + localVue, + apolloProvider: mockApollo, + ... + }); +} ``` -After this, on the global `describe`, we should create a variable for `fakeApollo`: +After this, you can control whether you need a variable for `mockApollo` and assign it in the appropriate `describe`-scope: ```javascript -describe('Some component with Apollo mock', () => { +describe('Some component', () => { let wrapper; - let fakeApollo -}) + + describe('with Apollo mock', () => { + let mockApollo; + + beforeEach(() => { + mockApollo = createMockApolloProvider(); + wrapper = createComponent({ mockApollo }); + }); + }); +}); ``` -Within component factory, we need to define an array of _handlers_ for every query or mutation: +Within `createMockApolloProvider`-factory, we need to define an array of _handlers_ for every query or mutation: ```javascript import getDesignListQuery from '~/design_management/graphql/queries/get_design_list.query.graphql'; @@ -702,13 +818,16 @@ import moveDesignMutation from '~/design_management/graphql/mutations/move_desig describe('Some component with Apollo mock', () => { let wrapper; - let fakeApollo; + let mockApollo; + + function createMockApolloProvider() { + Vue.use(VueApollo); - function createComponentWithApollo() { const requestHandlers = [ [getDesignListQuery, jest.fn().mockResolvedValue(designListQueryResponse)], [permissionsQuery, jest.fn().mockResolvedValue(permissionsQueryResponse)], ]; + ... } }) ``` @@ -718,23 +837,38 @@ After this, we need to create a mock Apollo Client instance using a helper: ```javascript import createMockApollo from 'jest/helpers/mock_apollo_helper'; -describe('Some component with Apollo mock', () => { +describe('Some component', () => { let wrapper; - let fakeApollo; - function createComponentWithApollo() { + function createMockApolloProvider() { + Vue.use(VueApollo); + const requestHandlers = [ [getDesignListQuery, jest.fn().mockResolvedValue(designListQueryResponse)], [permissionsQuery, jest.fn().mockResolvedValue(permissionsQueryResponse)], ]; - fakeApollo = createMockApollo(requestHandlers); - wrapper = shallowMount(Index, { + return createMockApollo(requestHandlers); + } + + function createComponent(options = {}) { + const { mockApollo } = options; + + return shallowMount(Index, { localVue, - apolloProvider: fakeApollo, + apolloProvider: mockApollo, }); } -}) + + describe('with Apollo mock', () => { + let mockApollo; + + beforeEach(() => { + mockApollo = createMockApolloProvider(); + wrapper = createComponent({ mockApollo }); + }); + }); +}); ``` When mocking resolved values, ensure the structure of the response is the same @@ -744,13 +878,15 @@ When testing queries, please keep in mind they are promises, so they need to be ```javascript it('renders a loading state', () => { - createComponentWithApollo(); + const mockApollo = createMockApolloProvider(); + const wrapper = createComponent({ mockApollo }); expect(wrapper.find(LoadingSpinner).exists()).toBe(true) }); it('renders designs list', async () => { - createComponentWithApollo(); + const mockApollo = createMockApolloProvider(); + const wrapper = createComponent({ mockApollo }); jest.runOnlyPendingTimers(); await wrapper.vm.$nextTick(); @@ -762,7 +898,7 @@ it('renders designs list', async () => { If we need to test a query error, we need to mock a rejected value as request handler: ```javascript -function createComponentWithApollo() { +function createMockApolloProvider() { ... const requestHandlers = [ [getDesignListQuery, jest.fn().mockRejectedValue(new Error('GraphQL error')], @@ -772,7 +908,7 @@ function createComponentWithApollo() { ... it('renders error if query fails', async () => { - createComponent() + const wrapper = createComponent(); jest.runOnlyPendingTimers(); await wrapper.vm.$nextTick(); @@ -786,9 +922,11 @@ Request handlers can also be passed to component factory as a parameter. Mutations could be tested the same way with a few additional `nextTick`s to get the updated result: ```javascript -function createComponentWithApollo({ +function createMockApolloProvider({ moveHandler = jest.fn().mockResolvedValue(moveDesignMutationResponse), }) { + Vue.use(VueApollo); + moveDesignHandler = moveHandler; const requestHandlers = [ @@ -797,15 +935,21 @@ function createComponentWithApollo({ [moveDesignMutation, moveDesignHandler], ]; - fakeApollo = createMockApollo(requestHandlers); - wrapper = shallowMount(Index, { + return createMockApollo(requestHandlers); +} + +function createComponent(options = {}) { + const { mockApollo } = options; + + return shallowMount(Index, { localVue, - apolloProvider: fakeApollo, + apolloProvider: mockApollo, }); } ... it('calls a mutation with correct parameters and reorders designs', async () => { - createComponentWithApollo({}); + const mockApollo = createMockApolloProvider({}); + const wrapper = createComponent({ mockApollo }); wrapper.find(VueDraggable).vm.$emit('change', { moved: { @@ -833,7 +977,7 @@ If your application contains `@client` queries, most probably you will have an A ```javascript import createMockApollo from 'jest/helpers/mock_apollo_helper'; ... -mockApollo = createMockApollo(requestHandlers, resolvers); +const mockApollo = createMockApollo(requestHandlers, resolvers); ``` Sometimes we want to test a `result` hook of the local query. In order to have it triggered, we need to populate a cache with correct data to be fetched with this query: @@ -850,12 +994,14 @@ query fetchLocalUser { import fetchLocalUserQuery from '~/design_management/graphql/queries/fetch_local_user.query.graphql'; function createMockApolloProvider() { + Vue.use(VueApollo); + const requestHandlers = [ [getDesignListQuery, jest.fn().mockResolvedValue(designListQueryResponse)], [permissionsQuery, jest.fn().mockResolvedValue(permissionsQueryResponse)], ]; - mockApollo = createMockApollo(requestHandlers, {}); + const mockApollo = createMockApollo(requestHandlers, {}); mockApollo.clients.defaultClient.cache.writeQuery({ query: fetchLocalUserQuery, data: { @@ -885,9 +1031,10 @@ Sometimes it is necessary to control what the local resolver returns and inspect import fetchLocalUserQuery from '~/design_management/graphql/queries/fetch_local_user.query.graphql'; function createMockApolloProvider(options = {}) { + Vue.use(VueApollo); const { fetchLocalUserSpy } = options; - mockApollo = createMockApollo([], { + const mockApollo = createMockApollo([], { Query: { fetchLocalUser: fetchLocalUserSpy, }, diff --git a/doc/development/pipelines.md b/doc/development/pipelines.md index be4011e27f8..e3666d810e0 100644 --- a/doc/development/pipelines.md +++ b/doc/development/pipelines.md @@ -485,17 +485,19 @@ request, be sure to start the `dont-interrupt-me` job before pushing. 1. All jobs must only pull caches by default. 1. All jobs must be able to pass with an empty cache. In other words, caches are only there to speed up jobs. -1. We currently have 6 different caches defined in +1. We currently have several different caches defined in [`.gitlab/ci/global.gitlab-ci.yml`](https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/ci/global.gitlab-ci.yml), with fixed keys: - `.rails-cache`. - `.static-analysis-cache`. + - `.coverage-cache` - `.qa-cache` - `.yarn-cache`. - `.assets-compile-cache` (the key includes `${NODE_ENV}` so it's actually two different caches). 1. Only 6 specific jobs, running in 2-hourly scheduled pipelines, are pushing (i.e. updating) to the caches: - `update-rails-cache`, defined in [`.gitlab/ci/rails.gitlab-ci.yml`](https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/ci/rails.gitlab-ci.yml). - `update-static-analysis-cache`, defined in [`.gitlab/ci/rails.gitlab-ci.yml`](https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/ci/rails.gitlab-ci.yml). + - `update-coverage-cache`, defined in [`.gitlab/ci/rails.gitlab-ci.yml`](https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/ci/rails.gitlab-ci.yml). - `update-qa-cache`, defined in [`.gitlab/ci/qa.gitlab-ci.yml`](https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/ci/qa.gitlab-ci.yml). - `update-assets-compile-production-cache`, defined in [`.gitlab/ci/frontend.gitlab-ci.yml`](https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/ci/frontend.gitlab-ci.yml). - `update-assets-compile-test-cache`, defined in [`.gitlab/ci/frontend.gitlab-ci.yml`](https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/ci/frontend.gitlab-ci.yml). @@ -519,7 +521,7 @@ variable: ```shell echo "Downloading archived master..." -wget -O /tmp/gitlab.tar.gz https://storage.googleapis.com/gitlab-ci-git-repo-cache/project-278964/gitlab-master.tar.gz +wget -O /tmp/gitlab.tar.gz https://storage.googleapis.com/gitlab-ci-git-repo-cache/project-278964/gitlab-master-shallow.tar.gz if [ ! -f /tmp/gitlab.tar.gz ]; then echo "Repository cache not available, cloning a new directory..." diff --git a/lib/gitlab/danger/helper.rb b/lib/gitlab/danger/helper.rb index 89f21e8bd23..7f68ea74798 100644 --- a/lib/gitlab/danger/helper.rb +++ b/lib/gitlab/danger/helper.rb @@ -268,6 +268,10 @@ module Gitlab def has_database_scoped_labels?(current_mr_labels) current_mr_labels.any? { |label| label.start_with?('database::') } end + + def has_ci_changes? + changed_files(%r{\A(\.gitlab-ci\.yml|\.gitlab/ci/)}).any? + end end end end diff --git a/lib/gitlab_danger.rb b/lib/gitlab_danger.rb index bbb64e0d5da..ec9dd20ccc0 100644 --- a/lib/gitlab_danger.rb +++ b/lib/gitlab_danger.rb @@ -14,6 +14,7 @@ class GitlabDanger product_analytics utility_css pajamas + pipeline ].freeze CI_ONLY_RULES ||= %w[ diff --git a/qa/qa/specs/features/api/3_create/gitaly/changing_repository_storage_spec.rb b/qa/qa/specs/features/api/3_create/gitaly/changing_repository_storage_spec.rb index e96b9ad9258..631056ed52e 100644 --- a/qa/qa/specs/features/api/3_create/gitaly/changing_repository_storage_spec.rb +++ b/qa/qa/specs/features/api/3_create/gitaly/changing_repository_storage_spec.rb @@ -45,7 +45,7 @@ module QA # Note: This test doesn't have the :orchestrated tag because it runs in the Test::Integration::Praefect # scenario with other tests that aren't considered orchestrated. # It also runs on staging using nfs-file07 as non-cluster storage and nfs-file22 as cluster/praefect storage - context 'when moving from Gitaly to Gitaly Cluster', :requires_praefect, testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/974' do + context 'when moving from Gitaly to Gitaly Cluster', :requires_praefect, testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/974', quarantine: { issue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/284645', type: :investigating } do let(:source_storage) { { type: :gitaly, name: QA::Runtime::Env.non_cluster_repository_storage } } let(:destination_storage) { { type: :praefect, name: QA::Runtime::Env.praefect_repository_storage } } let(:project) do diff --git a/spec/finders/ci/pipelines_finder_spec.rb b/spec/finders/ci/pipelines_finder_spec.rb index a2a714689ba..16561aa65b6 100644 --- a/spec/finders/ci/pipelines_finder_spec.rb +++ b/spec/finders/ci/pipelines_finder_spec.rb @@ -72,7 +72,7 @@ RSpec.describe Ci::PipelinesFinder do create(:ci_sources_pipeline, pipeline: child_pipeline, source_pipeline: parent_pipeline) end - it 'filters out child pipelines and show only the parents' do + it 'filters out child pipelines and shows only the parents by default' do is_expected.to eq([parent_pipeline]) end end @@ -195,6 +195,21 @@ RSpec.describe Ci::PipelinesFinder do end end + context 'when iids filter is specified' do + let(:params) { { iids: [pipeline1.iid, pipeline3.iid] } } + let!(:pipeline1) { create(:ci_pipeline, project: project) } + let!(:pipeline2) { create(:ci_pipeline, project: project) } + let!(:pipeline3) { create(:ci_pipeline, project: project, source: :parent_pipeline) } + + it 'returns matches pipelines' do + is_expected.to match_array([pipeline1, pipeline3]) + end + + it 'does not fitler out child pipelines' do + is_expected.to include(pipeline3) + end + end + context 'when sha is specified' do let!(:pipeline) { create(:ci_pipeline, project: project, sha: '97de212e80737a608d939f648d959671fb0a0142') } diff --git a/spec/frontend/boards/components/board_content_spec.js b/spec/frontend/boards/components/board_content_spec.js index 09e38001e2e..1cf05404cc9 100644 --- a/spec/frontend/boards/components/board_content_spec.js +++ b/spec/frontend/boards/components/board_content_spec.js @@ -1,6 +1,7 @@ import Vuex from 'vuex'; import { createLocalVue, shallowMount } from '@vue/test-utils'; import { GlAlert } from '@gitlab/ui'; +import Draggable from 'vuedraggable'; import EpicsSwimlanes from 'ee_component/boards/components/epics_swimlanes.vue'; import BoardColumn from 'ee_else_ce/boards/components/board_column.vue'; import getters from 'ee_else_ce/boards/stores/getters'; @@ -10,6 +11,11 @@ import BoardContent from '~/boards/components/board_content.vue'; const localVue = createLocalVue(); localVue.use(Vuex); +const actions = { + moveList: jest.fn(), + showPromotionList: jest.fn(), +}; + describe('BoardContent', () => { let wrapper; @@ -21,12 +27,13 @@ describe('BoardContent', () => { const createStore = (state = defaultState) => { return new Vuex.Store({ + actions, getters, state, }); }; - const createComponent = state => { + const createComponent = ({ state, props = {}, graphqlBoardListsEnabled = false } = {}) => { const store = createStore({ ...defaultState, ...state, @@ -37,25 +44,61 @@ describe('BoardContent', () => { lists: mockListsWithModel, canAdminList: true, disabled: false, + ...props, + }, + provide: { + glFeatures: { graphqlBoardLists: graphqlBoardListsEnabled }, }, store, }); }; - beforeEach(() => { - createComponent(); - }); - afterEach(() => { wrapper.destroy(); }); it('renders a BoardColumn component per list', () => { + createComponent(); + expect(wrapper.findAll(BoardColumn)).toHaveLength(mockListsWithModel.length); }); it('does not display EpicsSwimlanes component', () => { + createComponent(); + expect(wrapper.find(EpicsSwimlanes).exists()).toBe(false); expect(wrapper.find(GlAlert).exists()).toBe(false); }); + + describe('graphqlBoardLists feature flag enabled', () => { + describe('can admin list', () => { + beforeEach(() => { + createComponent({ graphqlBoardListsEnabled: true, props: { canAdminList: true } }); + }); + + it('renders draggable component', () => { + expect(wrapper.find(Draggable).exists()).toBe(true); + }); + }); + + describe('can not admin list', () => { + beforeEach(() => { + createComponent({ graphqlBoardListsEnabled: true, props: { canAdminList: false } }); + }); + + it('renders draggable component', () => { + expect(wrapper.find(Draggable).exists()).toBe(false); + }); + }); + }); + + describe('graphqlBoardLists feature flag disabled', () => { + beforeEach(() => { + createComponent({ graphqlBoardListsEnabled: false }); + }); + + it('does not render draggable component', () => { + expect(wrapper.find(Draggable).exists()).toBe(false); + }); + }); }); diff --git a/spec/frontend/boards/stores/actions_spec.js b/spec/frontend/boards/stores/actions_spec.js index f168e427431..478f5dc59e0 100644 --- a/spec/frontend/boards/stores/actions_spec.js +++ b/spec/frontend/boards/stores/actions_spec.js @@ -290,6 +290,33 @@ describe('moveList', () => { done, ); }); + + it('should not commit MOVE_LIST or dispatch updateList if listId and replacedListId are the same', () => { + const initialBoardListsState = { + 'gid://gitlab/List/1': mockListsWithModel[0], + 'gid://gitlab/List/2': mockListsWithModel[1], + }; + + const state = { + endpoints: { fullPath: 'gitlab-org', boardId: '1' }, + boardType: 'group', + disabled: false, + boardLists: initialBoardListsState, + }; + + testAction( + actions.moveList, + { + listId: 'gid://gitlab/List/1', + replacedListId: 'gid://gitlab/List/1', + newIndex: 1, + adjustmentValue: 1, + }, + state, + [], + [], + ); + }); }); describe('updateList', () => { diff --git a/spec/graphql/resolvers/project_pipeline_resolver_spec.rb b/spec/graphql/resolvers/project_pipeline_resolver_spec.rb index 1950c2ca067..b852b349d4f 100644 --- a/spec/graphql/resolvers/project_pipeline_resolver_spec.rb +++ b/spec/graphql/resolvers/project_pipeline_resolver_spec.rb @@ -18,6 +18,10 @@ RSpec.describe Resolvers::ProjectPipelineResolver do resolve(described_class, obj: project, args: args, ctx: { current_user: current_user }) end + before do + project.add_developer(current_user) + end + it 'resolves pipeline for the passed iid' do result = batch_sync do resolve_pipeline(project, { iid: '1234' }) @@ -26,6 +30,21 @@ RSpec.describe Resolvers::ProjectPipelineResolver do expect(result).to eq(pipeline) end + it 'keeps the queries under the threshold' do + create(:ci_pipeline, project: project, iid: '1235') + + control = ActiveRecord::QueryRecorder.new do + batch_sync { resolve_pipeline(project, { iid: '1234' }) } + end + + expect do + batch_sync do + resolve_pipeline(project, { iid: '1234' }) + resolve_pipeline(project, { iid: '1235' }) + end + end.not_to exceed_query_limit(control) + end + it 'does not resolve a pipeline outside the project' do result = batch_sync do resolve_pipeline(other_pipeline.project, { iid: '1234' }) diff --git a/spec/lib/gitlab/danger/helper_spec.rb b/spec/lib/gitlab/danger/helper_spec.rb index f400641706d..faf49632e49 100644 --- a/spec/lib/gitlab/danger/helper_spec.rb +++ b/spec/lib/gitlab/danger/helper_spec.rb @@ -591,4 +591,30 @@ RSpec.describe Gitlab::Danger::Helper do expect(helper.prepare_labels_for_mr([])).to eq('') end end + + describe '#has_ci_changes?' do + context 'when .gitlab/ci is changed' do + it 'returns true' do + expect(helper).to receive(:all_changed_files).and_return(%w[migration.rb .gitlab/ci/test.yml]) + + expect(helper.has_ci_changes?).to be_truthy + end + end + + context 'when .gitlab-ci.yml is changed' do + it 'returns true' do + expect(helper).to receive(:all_changed_files).and_return(%w[migration.rb .gitlab-ci.yml]) + + expect(helper.has_ci_changes?).to be_truthy + end + end + + context 'when neither .gitlab/ci/ or .gitlab-ci.yml is changed' do + it 'returns false' do + expect(helper).to receive(:all_changed_files).and_return(%w[migration.rb nested/.gitlab-ci.yml]) + + expect(helper.has_ci_changes?).to be_falsey + end + end + end end diff --git a/spec/lib/gitlab_danger_spec.rb b/spec/lib/gitlab_danger_spec.rb index e332647cf8a..ed668c52a0e 100644 --- a/spec/lib/gitlab_danger_spec.rb +++ b/spec/lib/gitlab_danger_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'spec_helper' +require 'fast_spec_helper' RSpec.describe GitlabDanger do let(:gitlab_danger_helper) { nil } @@ -9,7 +9,7 @@ RSpec.describe GitlabDanger do describe '.local_warning_message' do it 'returns an informational message with rules that can run' do - expect(described_class.local_warning_message).to eq('==> Only the following Danger rules can be run locally: changes_size, documentation, frozen_string, duplicate_yarn_dependencies, prettier, eslint, karma, database, commit_messages, product_analytics, utility_css, pajamas') + expect(described_class.local_warning_message).to eq("==> Only the following Danger rules can be run locally: #{described_class::LOCAL_RULES.join(', ')}") end end diff --git a/spec/requests/api/graphql/project/pipeline_spec.rb b/spec/requests/api/graphql/project/pipeline_spec.rb index fef0e7e160c..6179b43629b 100644 --- a/spec/requests/api/graphql/project/pipeline_spec.rb +++ b/spec/requests/api/graphql/project/pipeline_spec.rb @@ -5,12 +5,12 @@ require 'spec_helper' RSpec.describe 'getting pipeline information nested in a project' do include GraphqlHelpers - let(:project) { create(:project, :repository, :public) } - let(:pipeline) { create(:ci_pipeline, project: project) } - let(:current_user) { create(:user) } + let!(:project) { create(:project, :repository, :public) } + let!(:pipeline) { create(:ci_pipeline, project: project) } + let!(:current_user) { create(:user) } let(:pipeline_graphql_data) { graphql_data['project']['pipeline'] } - let(:query) do + let!(:query) do graphql_query_for( 'project', { 'fullPath' => project.full_path }, @@ -35,4 +35,45 @@ RSpec.describe 'getting pipeline information nested in a project' do expect(pipeline_graphql_data.dig('configSource')).to eq('UNKNOWN_SOURCE') end + + context 'batching' do + let!(:pipeline2) { create(:ci_pipeline, project: project, user: current_user, builds: [create(:ci_build, :success)]) } + let!(:pipeline3) { create(:ci_pipeline, project: project, user: current_user, builds: [create(:ci_build, :success)]) } + let!(:query) { build_query_to_find_pipeline_shas(pipeline, pipeline2, pipeline3) } + + it 'executes the finder once' do + mock = double(Ci::PipelinesFinder) + opts = { iids: [pipeline.iid, pipeline2.iid, pipeline3.iid].map(&:to_s) } + + expect(Ci::PipelinesFinder).to receive(:new).once.with(project, current_user, opts).and_return(mock) + expect(mock).to receive(:execute).once.and_return(Ci::Pipeline.none) + + post_graphql(query, current_user: current_user) + end + + it 'keeps the queries under the threshold' do + control = ActiveRecord::QueryRecorder.new do + single_pipeline_query = build_query_to_find_pipeline_shas(pipeline) + + post_graphql(single_pipeline_query, current_user: current_user) + end + + aggregate_failures do + expect(response).to have_gitlab_http_status(:success) + expect do + post_graphql(query, current_user: current_user) + end.not_to exceed_query_limit(control) + end + end + end + + private + + def build_query_to_find_pipeline_shas(*pipelines) + pipeline_fields = pipelines.map.each_with_index do |pipeline, idx| + "pipeline#{idx}: pipeline(iid: \"#{pipeline.iid}\") { sha }" + end.join(' ') + + graphql_query_for('project', { 'fullPath' => project.full_path }, pipeline_fields) + end end diff --git a/spec/tooling/lib/tooling/test_map_generator_spec.rb b/spec/tooling/lib/tooling/test_map_generator_spec.rb index 4c720ba62a2..eb49b1db20e 100644 --- a/spec/tooling/lib/tooling/test_map_generator_spec.rb +++ b/spec/tooling/lib/tooling/test_map_generator_spec.rb @@ -50,13 +50,13 @@ RSpec.describe Tooling::TestMapGenerator do let(:expected_mapping) do { 'lib/gitlab/current_settings.rb' => [ - './spec/factories_spec.rb' + 'spec/factories_spec.rb' ], 'lib/feature.rb' => [ - './spec/factories_spec.rb' + 'spec/factories_spec.rb' ], 'lib/gitlab/marginalia.rb' => [ - './spec/factories_spec.rb' + 'spec/factories_spec.rb' ] } end @@ -80,16 +80,16 @@ RSpec.describe Tooling::TestMapGenerator do let(:expected_mapping) do { 'lib/gitlab/current_settings.rb' => [ - './spec/factories_spec.rb', - './spec/models/project_spec.rb' + 'spec/factories_spec.rb', + 'spec/models/project_spec.rb' ], 'lib/feature.rb' => [ - './spec/factories_spec.rb', - './spec/models/project_spec.rb' + 'spec/factories_spec.rb', + 'spec/models/project_spec.rb' ], 'lib/gitlab/marginalia.rb' => [ - './spec/factories_spec.rb', - './spec/models/project_spec.rb' + 'spec/factories_spec.rb', + 'spec/models/project_spec.rb' ] } end diff --git a/tooling/lib/tooling/test_map_generator.rb b/tooling/lib/tooling/test_map_generator.rb index bd0415f6e67..20e4ed8e405 100644 --- a/tooling/lib/tooling/test_map_generator.rb +++ b/tooling/lib/tooling/test_map_generator.rb @@ -17,7 +17,7 @@ module Tooling example_groups.each do |example_id, files| files.each do |file| spec_file = strip_example_uid(example_id) - @mapping[file] << spec_file + @mapping[file] << spec_file.delete_prefix('./') end end end |