diff options
162 files changed, 2966 insertions, 522 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index fd02d72b4c2..797a20ef16e 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -739,7 +739,7 @@ karma: - chrome_debug.log - coverage-javascript/ -codequality: +code_quality: <<: *dedicated-no-docs-no-db-pull-cache-job image: docker:stable allow_failure: true @@ -757,9 +757,13 @@ codequality: script: # Extract "MAJOR.MINOR" from CI_SERVER_VERSION and generate "MAJOR-MINOR-stable" for Security Products - export SP_VERSION=$(echo "$CI_SERVER_VERSION" | sed 's/^\([0-9]*\)\.\([0-9]*\).*/\1-\2-stable/') - - docker run --env SOURCE_CODE="$PWD" --volume "$PWD":/code --volume /var/run/docker.sock:/var/run/docker.sock "registry.gitlab.com/gitlab-org/security-products/codequality:$SP_VERSION" /code + - docker run + --env SOURCE_CODE="$PWD" + --volume "$PWD":/code + --volume /var/run/docker.sock:/var/run/docker.sock + "registry.gitlab.com/gitlab-org/security-products/codequality:$SP_VERSION" /code artifacts: - paths: [codeclimate.json] + paths: [gl-code-quality-report.json] expire_in: 1 week sast: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e68e3b9cab0..fb7c0c88629 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -64,11 +64,11 @@ As of July 2018, all the documentation for contributing to the GitLab project ha ## Contribute to GitLab -For a first-time step-by-step guide to the contribution process, see -["Contributing to GitLab"](https://about.gitlab.com/contributing/). - Thank you for your interest in contributing to GitLab. This guide details how -to contribute to GitLab in a way that is efficient for everyone. +to contribute to GitLab in a way that is easy for everyone. + +For a first-time step-by-step guide to the contribution process, please see +["Contributing to GitLab"](https://about.gitlab.com/contributing/). Looking for something to work on? Look for issues with the label [Accepting Merge Requests](#i-want-to-contribute). @@ -77,10 +77,10 @@ source edition, and GitLab Enterprise Edition (EE) which is our commercial edition. Throughout this guide you will see references to CE and EE for abbreviation. -If you have read this guide and want to know how the GitLab [core team] +If you want to know how the GitLab [core team] operates please see [the GitLab contributing process](PROCESS.md). -- [GitLab Inc engineers should refer to the engineering workflow document](https://about.gitlab.com/handbook/engineering/workflow/) +[GitLab Inc engineers should refer to the engineering workflow document](https://about.gitlab.com/handbook/engineering/workflow/) ## Security vulnerability disclosure diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index a38b3bd31b1..377d8aca07e 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.117.0 +0.117.2 @@ -423,7 +423,7 @@ group :ed25519 do end # Gitaly GRPC client -gem 'gitaly-proto', '~> 0.112.0', require: 'gitaly' +gem 'gitaly-proto', '~> 0.113.0', require: 'gitaly' gem 'grpc', '~> 1.11.0' # Locked until https://github.com/google/protobuf/issues/4210 is closed diff --git a/Gemfile.lock b/Gemfile.lock index 1aadc3fd0b6..333e586949f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -284,7 +284,7 @@ GEM gettext_i18n_rails (>= 0.7.1) po_to_json (>= 1.0.0) rails (>= 3.2.0) - gitaly-proto (0.112.0) + gitaly-proto (0.113.0) google-protobuf (~> 3.1) grpc (~> 1.10) github-linguist (5.3.3) @@ -1045,7 +1045,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.3) - gitaly-proto (~> 0.112.0) + gitaly-proto (~> 0.113.0) github-linguist (~> 5.3.3) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-gollum-lib (~> 4.2) diff --git a/Gemfile.rails5.lock b/Gemfile.rails5.lock index af70e2c1939..c919f1a08ae 100644 --- a/Gemfile.rails5.lock +++ b/Gemfile.rails5.lock @@ -287,7 +287,7 @@ GEM gettext_i18n_rails (>= 0.7.1) po_to_json (>= 1.0.0) rails (>= 3.2.0) - gitaly-proto (0.112.0) + gitaly-proto (0.113.0) google-protobuf (~> 3.1) grpc (~> 1.10) github-linguist (5.3.3) @@ -1057,7 +1057,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.3) - gitaly-proto (~> 0.112.0) + gitaly-proto (~> 0.113.0) github-linguist (~> 5.3.3) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-gollum-lib (~> 4.2) @@ -1216,4 +1216,4 @@ DEPENDENCIES wikicloth (= 0.8.1) BUNDLED WITH - 1.16.2 + 1.16.3 diff --git a/README.md b/README.md index b6e1cc9a432..335736e53f5 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,7 @@ You can access a new installation with the login **`root`** and password **`5ive ## Contributing -GitLab is an open source project and we are very happy to accept community contributions. Please refer to [CONTRIBUTING.md](/CONTRIBUTING.md) for details. +GitLab is an open source project and we are very happy to accept community contributions. Please refer to [Contributing to GitLab page](https://about.gitlab.com/contributing/) for more details. ## Licensing @@ -66,7 +66,7 @@ GitLab Community Edition (CE) is available freely under the MIT Expat license. All third party components incorporated into the GitLab Software are licensed under the original license provided by the owner of the applicable component. -All Documentation content that resides under the doc/ directory of this repository is licensed under Creative Commons: CC BY-SA 4.0. +All Documentation content that resides under the `doc/` directory of this repository is licensed under Creative Commons: CC BY-SA 4.0. ## Install a development environment diff --git a/app/assets/javascripts/ide/components/repo_editor.vue b/app/assets/javascripts/ide/components/repo_editor.vue index f9badb01535..f55aa843444 100644 --- a/app/assets/javascripts/ide/components/repo_editor.vue +++ b/app/assets/javascripts/ide/components/repo_editor.vue @@ -133,7 +133,6 @@ export default { .then(() => this.getRawFileData({ path: this.file.path, - baseSha: this.currentMergeRequest ? this.currentMergeRequest.baseCommitSha : '', }), ) .then(() => { diff --git a/app/assets/javascripts/ide/stores/actions/file.js b/app/assets/javascripts/ide/stores/actions/file.js index c9795750d65..28b9d0df201 100644 --- a/app/assets/javascripts/ide/stores/actions/file.js +++ b/app/assets/javascripts/ide/stores/actions/file.js @@ -92,7 +92,7 @@ export const setFileMrChange = ({ commit }, { file, mrChange }) => { commit(types.SET_FILE_MERGE_REQUEST_CHANGE, { file, mrChange }); }; -export const getRawFileData = ({ state, commit, dispatch }, { path, baseSha }) => { +export const getRawFileData = ({ state, commit, dispatch, getters }, { path }) => { const file = state.entries[path]; return new Promise((resolve, reject) => { service @@ -100,6 +100,9 @@ export const getRawFileData = ({ state, commit, dispatch }, { path, baseSha }) = .then(raw => { if (!(file.tempFile && !file.prevPath)) commit(types.SET_FILE_RAW_DATA, { file, raw }); if (file.mrChange && file.mrChange.new_file === false) { + const baseSha = + (getters.currentMergeRequest && getters.currentMergeRequest.baseCommitSha) || ''; + service .getBaseRawFileData(file, baseSha) .then(baseRaw => { @@ -122,7 +125,7 @@ export const getRawFileData = ({ state, commit, dispatch }, { path, baseSha }) = action: payload => dispatch('getRawFileData', payload).then(() => dispatch('setErrorMessage', null)), actionText: __('Please try again'), - actionPayload: { path, baseSha }, + actionPayload: { path }, }); reject(); }); diff --git a/app/assets/javascripts/jobs/components/commit_block.vue b/app/assets/javascripts/jobs/components/commit_block.vue new file mode 100644 index 00000000000..7f485295513 --- /dev/null +++ b/app/assets/javascripts/jobs/components/commit_block.vue @@ -0,0 +1,64 @@ +<script> +import ClipboardButton from '~/vue_shared/components/clipboard_button.vue'; + +export default { + components: { + ClipboardButton, + }, + props: { + pipelineShortSha: { + type: String, + required: true, + }, + pipelineShaPath: { + type: String, + required: true, + }, + mergeRequestReference: { + type: String, + required: false, + default: null, + }, + mergeRequestPath: { + type: String, + required: false, + default: null, + }, + gitCommitTitlte: { + type: String, + required: true, + }, + }, +}; +</script> +<template> + <div class="block"> + <p> + {{ __('Commit') }} + + <a + :href="pipelineShaPath" + class="js-commit-sha commit-sha link-commit" + > + {{ pipelineShortSha }} + </a> + + <clipboard-button + :text="pipelineShortSha" + :title="__('Copy commit SHA to clipboard')" + /> + + <a + v-if="mergeRequestPath && mergeRequestReference" + :href="mergeRequestPath" + class="js-link-commit link-commit" + > + {{ mergeRequestReference }} + </a> + </p> + + <p class="build-light-text append-bottom-0"> + {{ gitCommitTitlte }} + </p> + </div> +</template> diff --git a/app/assets/javascripts/jobs/components/jobs_container.vue b/app/assets/javascripts/jobs/components/jobs_container.vue new file mode 100644 index 00000000000..b81109bdd06 --- /dev/null +++ b/app/assets/javascripts/jobs/components/jobs_container.vue @@ -0,0 +1,60 @@ +<script> + import CiIcon from '~/vue_shared/components/ci_icon.vue'; + import Icon from '~/vue_shared/components/icon.vue'; + import tooltip from '~/vue_shared/directives/tooltip'; + + export default { + components: { + CiIcon, + Icon, + }, + directives: { + tooltip, + }, + props: { + jobs: { + type: Array, + required: true, + }, + }, + }; +</script> +<template> + <div class="builds-container"> + <div + class="build-job" + > + <a + v-tooltip + v-for="job in jobs" + :key="job.id" + :href="job.path" + :title="job.tooltip" + :class="{ active: job.active, retried: job.retried }" + > + <icon + v-if="job.active" + name="arrow-right" + class="js-arrow-right" + /> + + <ci-icon :status="job.status" /> + + <span> + <template v-if="job.name"> + {{ job.name }} + </template> + <template v-else> + {{ job.id }} + </template> + </span> + + <icon + v-if="job.retried" + name="retry" + class="js-retry-icon" + /> + </a> + </div> + </div> +</template> diff --git a/app/assets/javascripts/jobs/components/stages_dropdown.vue b/app/assets/javascripts/jobs/components/stages_dropdown.vue new file mode 100644 index 00000000000..d6d64fa32f7 --- /dev/null +++ b/app/assets/javascripts/jobs/components/stages_dropdown.vue @@ -0,0 +1,97 @@ +<script> + import CiIcon from '~/vue_shared/components/ci_icon.vue'; + import Icon from '~/vue_shared/components/icon.vue'; + + import { sprintf, __ } from '~/locale'; + + export default { + components: { + CiIcon, + Icon, + }, + props: { + pipelineId: { + type: Number, + required: true, + }, + pipelinePath: { + type: String, + required: true, + }, + pipelineRef: { + type: String, + required: true, + }, + pipelineRefPath: { + type: String, + required: true, + }, + stages: { + type: Array, + required: true, + }, + pipelineStatus: { + type: Object, + required: true, + }, + }, + data() { + return { + selectedStage: this.stages.length > 0 ? this.stages[0].name : __('More'), + }; + }, + computed: { + pipelineLink() { + return sprintf(__('Pipeline %{pipelineLinkStart} #%{pipelineId} %{pipelineLinkEnd} from %{pipelineLinkRefStart} %{pipelineRef} %{pipelineLinkRefEnd}'), { + pipelineLinkStart: `<a href=${this.pipelinePath} class="js-pipeline-path link-commit">`, + pipelineId: this.pipelineId, + pipelineLinkEnd: '</a>', + pipelineLinkRefStart: `<a href=${this.pipelineRefPath} class="link-commit ref-name">`, + pipelineRef: this.pipelineRef, + pipelineLinkRefEnd: '</a>', + }, false); + }, + }, + methods: { + onStageClick(stage) { + // todo: consider moving into store + this.selectedStage = stage.name; + + // update dropdown with jobs + // jobs container is a new component. + this.$emit('requestSidebarStageDropdown', stage); + }, + }, + }; +</script> +<template> + <div class="block-last"> + <ci-icon :status="pipelineStatus" /> + + <p v-html="pipelineLink"></p> + + <div class="dropdown"> + <button + type="button" + data-toggle="dropdown" + > + {{ selectedStage }} + <icon name="chevron-down" /> + </button> + <ul class="dropdown-menu"> + <li + v-for="(stage, index) in stages" + :key="index" + > + <button + type="button" + class="stage-item" + @click="onStageClick(stage)" + > + {{ stage.name }} + </button> + </li> + </ul> + </div> + </div> +</template> diff --git a/app/assets/javascripts/jobs/components/trigger_block.vue b/app/assets/javascripts/jobs/components/trigger_block.vue new file mode 100644 index 00000000000..8a88e5da6aa --- /dev/null +++ b/app/assets/javascripts/jobs/components/trigger_block.vue @@ -0,0 +1,84 @@ +<script> + export default { + props: { + shortToken: { + type: String, + required: false, + default: null, + }, + + variables: { + type: Object, + required: false, + default: () => ({}), + }, + }, + data() { + return { + areVariablesVisible: false, + }; + }, + computed: { + hasVariables() { + return Object.keys(this.variables).length > 0; + }, + }, + methods: { + revealVariables() { + this.areVariablesVisible = true; + }, + }, + }; +</script> + +<template> + <div class="build-widget block"> + <h4 class="title"> + {{ __('Trigger') }} + </h4> + + <p + v-if="shortToken" + class="js-short-token" + > + <span class="build-light-text"> + {{ __('Token') }} + </span> + {{ shortToken }} + </p> + + <p v-if="hasVariables"> + <button + type="button" + class="btn btn-default group js-reveal-variables" + @click="revealVariables" + > + {{ __('Reveal Variables') }} + </button> + + </p> + + <dl + v-if="areVariablesVisible" + class="js-build-variables trigger-build-variables" + > + <template + v-for="(value, key) in variables" + > + <dt + :key="`${key}-variable`" + class="js-build-variable trigger-build-variable" + > + {{ key }} + </dt> + + <dd + :key="`${key}-value`" + class="js-build-value trigger-build-value" + > + {{ value }} + </dd> + </template> + </dl> + </div> +</template> diff --git a/app/assets/stylesheets/framework/awards.scss b/app/assets/stylesheets/framework/awards.scss index 8d11b92cf88..a265e4206f1 100644 --- a/app/assets/stylesheets/framework/awards.scss +++ b/app/assets/stylesheets/framework/awards.scss @@ -141,8 +141,8 @@ &:hover, &:active, &.is-active { - background-color: $row-hover; - border-color: $row-hover-border; + background-color: $blue-50; + border-color: $blue-200; box-shadow: none; outline: 0; diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index 48a87ea8616..79ca6e61e9a 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -71,7 +71,7 @@ pre { } &.card.card-body-pre { - border: 1px solid $well-pre-bg; + border: 1px solid $gray-darker; background: $gray-light; border-radius: 0; color: $well-pre-color; @@ -233,7 +233,7 @@ li.note { .error-message { padding: 10px; - background: $error-bg; + background: $red-400; margin: 0; color: $white-light; @@ -244,11 +244,11 @@ li.note { } .warning_message { - border-left: 4px solid $warning-message-border; - color: $warning-message-color; + border-left: 4px solid $orange-200; + color: $orange-700; padding: 10px; margin-bottom: 10px; - background: $warning-message-bg; + background: $orange-100; padding-left: 20px; &.centered { @@ -348,15 +348,6 @@ img.emoji { } } -.profiler-results { - top: 73px !important; - - .profiler-button, - .profiler-controls { - border-color: $profiler-border !important; - } -} - .dropzone .dz-preview .dz-progress { border-color: $border-color !important; diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index 54882633fea..3bdf5bfc93a 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/app/assets/stylesheets/framework/files.scss @@ -112,7 +112,7 @@ &.image_file, &.video { - background: $file-image-bg; + background: $gray-darker; text-align: center; padding: 30px; @@ -131,7 +131,7 @@ } &.blob-no-preview { - background: $blob-bg; + background: $gray-darker; text-shadow: 0 1px 2px $white-light; padding: 100px 0; } @@ -146,7 +146,7 @@ } tr { - border-bottom: 1px solid $blame-border; + border-bottom: 1px solid $gray-darker; &:last-child { border-bottom: 0; @@ -211,7 +211,7 @@ } &.logs { - background: $logs-bg; + background: $gray-darker; max-height: 700px; overflow-y: auto; @@ -233,7 +233,7 @@ } &:hover { - background: $row-hover; + background: $blue-50; } } } diff --git a/app/assets/stylesheets/framework/issue_box.scss b/app/assets/stylesheets/framework/issue_box.scss index 86de88729ee..da5f80d9d37 100644 --- a/app/assets/stylesheets/framework/issue_box.scss +++ b/app/assets/stylesheets/framework/issue_box.scss @@ -39,7 +39,7 @@ } &.status-box-expired { - background-color: $issue-status-expired; + background-color: $orange-500; } &.status-box-upcoming { diff --git a/app/assets/stylesheets/framework/lists.scss b/app/assets/stylesheets/framework/lists.scss index 4b67eab05b3..fdc0454d837 100644 --- a/app/assets/stylesheets/framework/lists.scss +++ b/app/assets/stylesheets/framework/lists.scss @@ -21,11 +21,11 @@ } &.disabled { - color: $list-text-disabled-color; + color: $gl-text-color-tertiary; } &:not(.ui-sort-disabled):hover { - background: $row-hover; + background: $blue-50; } &.unstyled { @@ -35,12 +35,12 @@ } &.warning-row { - background-color: $list-warning-row-bg; - border-color: $list-warning-row-border; - color: $list-warning-row-color; + background-color: $orange-100; + border-color: $orange-200; + color: $orange-700; &:hover { - background: $list-warning-row-bg; + background: $orange-100; } } @@ -73,7 +73,7 @@ } .card.card-body-title { - font-size: $list-font-size; + font-size: $gl-font-size; line-height: 18px; } } @@ -109,8 +109,8 @@ ul.content-list { li { border-color: $white-normal; - font-size: $list-font-size; - color: $list-text-color; + font-size: $gl-font-size; + color: $gl-text-color; &.no-description { .title { diff --git a/app/assets/stylesheets/framework/mixins.scss b/app/assets/stylesheets/framework/mixins.scss index 98bf26a5222..7edb89ce6f3 100644 --- a/app/assets/stylesheets/framework/mixins.scss +++ b/app/assets/stylesheets/framework/mixins.scss @@ -50,7 +50,7 @@ @include clearfix; padding: 10px 0; - border-bottom: 1px solid $list-border-light; + border-bottom: 1px solid $gray-darker; display: block; margin: 0; diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index eccc814b747..5c6110737a4 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -7,7 +7,7 @@ } a { - color: $md-link-color; + color: $blue-600; } img:not(.emoji) { diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 866cb88ba5b..96508a71bd8 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -203,21 +203,12 @@ $gl-header-color: #4c4e54; /* * Lists */ -$list-font-size: $gl-font-size; -$list-title-color: $gl-text-color; -$list-text-color: $gl-text-color; -$list-text-disabled-color: $gl-text-color-tertiary; -$list-border-light: #eee; $list-border: rgba(0, 0, 0, 0.05); $list-text-height: 42px; -$list-warning-row-bg: $orange-100; -$list-warning-row-border: $orange-200; -$list-warning-row-color: $orange-700; /* * Markdown */ -$md-link-color: $blue-600; $md-area-border: #ddd; /* @@ -249,8 +240,6 @@ $gl-bar-padding: 3px; /* * Misc */ -$row-hover: $blue-50; -$row-hover-border: $blue-200; $progress-color: #c0392b; $header-height: 40px; $ide-statusbar-height: 25px; @@ -258,19 +247,13 @@ $fixed-layout-width: 1280px; $limited-layout-width: 990px; $container-text-max-width: 540px; $gl-avatar-size: 40px; -$error-exclamation-point: $red-500; $border-radius-default: 4px; $border-radius-small: 2px; $settings-icon-size: 18px; -$provider-btn-not-active-color: $blue-500; -$link-underline-blue: $blue-500; -$active-item-blue: $blue-500; $layout-link-gray: #7e7c7c; $btn-side-margin: 10px; $btn-sm-side-margin: 7px; $btn-margin-5: 5px; -$issue-status-expired: $orange-500; -$issuable-sidebar-color: $gl-text-color-secondary; $sidebar-block-hover-color: #ebebeb; $group-path-color: #999; $namespace-kind-color: #aaa; @@ -292,7 +275,6 @@ $breadcrumb-min-height: 48px; * Common component specific colors */ $hint-color: #999; -$well-pre-bg: #eee; $well-pre-color: #555; $loading-color: #555; $update-author-color: #999; @@ -301,10 +283,6 @@ $user-mention-bg-hover: rgba($blue-500, 0.15); $time-color: #999; $project-member-show-color: #aaa; $gl-promo-color: #aaa; -$error-bg: $red-400; -$warning-message-bg: $orange-100; -$warning-message-border: $orange-200; -$warning-message-color: $orange-700; $control-group-descr-color: #666; $table-permission-x-bg: #d9edf7; $username-color: #666; @@ -374,7 +352,7 @@ $dropdown-min-height: 40px; $dropdown-max-height: 312px; $dropdown-vertical-offset: 4px; $dropdown-link-color: #555; -$dropdown-link-hover-bg: $row-hover; +$dropdown-link-hover-bg: $blue-50; $dropdown-empty-row-bg: rgba(#000, 0.04); $dropdown-border-color: $border-color; $dropdown-shadow-color: rgba(#000, 0.1); @@ -592,11 +570,7 @@ $events-body-border: #ddd; /* * Files */ -$file-image-bg: #eee; -$blob-bg: #eee; -$blame-border: #eee; $blame-line-numbers-border: #ddd; -$logs-bg: #eee; $logs-li-color: #888; $logs-p-color: #333; @@ -667,7 +641,6 @@ $login-devise-error-color: $red-700; * Nav */ $nav-link-gray: #959494; -$nav-badge-bg: #eee; $nav-toggle-gray: #666; /* diff --git a/app/assets/stylesheets/pages/boards.scss b/app/assets/stylesheets/pages/boards.scss index 91f470ca709..69d7de886b4 100644 --- a/app/assets/stylesheets/pages/boards.scss +++ b/app/assets/stylesheets/pages/boards.scss @@ -288,7 +288,7 @@ &.is-active, &.is-active .board-card-assignee:hover a { - background-color: $row-hover; + background-color: $blue-50; } .badge { diff --git a/app/assets/stylesheets/pages/cycle_analytics.scss b/app/assets/stylesheets/pages/cycle_analytics.scss index bba9f38d3dd..bc4c90711d7 100644 --- a/app/assets/stylesheets/pages/cycle_analytics.scss +++ b/app/assets/stylesheets/pages/cycle_analytics.scss @@ -165,7 +165,7 @@ border-right-color: transparent; border-top-color: $border-color; border-bottom-color: $border-color; - box-shadow: inset 2px 0 0 0 $active-item-blue; + box-shadow: inset 2px 0 0 0 $blue-500; .stage-name { font-weight: $gl-font-weight-bold; diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 47778110bae..a999a70693e 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -186,7 +186,7 @@ } .image { - background: $file-image-bg; + background: $gray-darker; text-align: center; padding: 30px; diff --git a/app/assets/stylesheets/pages/events.scss b/app/assets/stylesheets/pages/events.scss index f79586b68b9..da0c9b44498 100644 --- a/app/assets/stylesheets/pages/events.scss +++ b/app/assets/stylesheets/pages/events.scss @@ -6,7 +6,7 @@ font-size: $gl-font-size; padding: $gl-padding-top 0 $gl-padding-top 40px; border-bottom: 1px solid $white-normal; - color: $list-text-color; + color: $gl-text-color; position: relative; &.event-inline { @@ -58,7 +58,7 @@ .event-title { @include str-truncated(calc(100% - 174px)); font-weight: $gl-font-weight-bold; - color: $list-text-color; + color: $gl-text-color; } .event-body { diff --git a/app/assets/stylesheets/pages/groups.scss b/app/assets/stylesheets/pages/groups.scss index fa8a0f26b5d..60b4d39bb1a 100644 --- a/app/assets/stylesheets/pages/groups.scss +++ b/app/assets/stylesheets/pages/groups.scss @@ -385,8 +385,8 @@ padding: $gl-padding-top; &:hover { - border-color: $row-hover-border; - background-color: $row-hover; + border-color: $blue-200; + background-color: $blue-50; cursor: pointer; } diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index d16a63d009a..6f0f82964c8 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -423,10 +423,10 @@ width: 100%; text-align: center; margin-bottom: 10px; - color: $issuable-sidebar-color; + color: $gl-text-color-secondary; svg { - fill: $issuable-sidebar-color; + fill: $gl-text-color-secondary; } &:hover:not(.disabled), @@ -457,14 +457,14 @@ } .avatar-counter:hover { - color: $issuable-sidebar-color; - border-color: $issuable-sidebar-color; + color: $gl-text-color-secondary; + border-color: $gl-text-color-secondary; } .btn-clipboard { border: 0; background: transparent; - color: $issuable-sidebar-color; + color: $gl-text-color-secondary; &:hover { color: $gl-text-color; @@ -821,7 +821,7 @@ svg { width: 16px; height: 16px; - fill: $issuable-sidebar-color; + fill: $gl-text-color-secondary; } &:hover svg { diff --git a/app/assets/stylesheets/pages/profile.scss b/app/assets/stylesheets/pages/profile.scss index b45e305897c..17f34319050 100644 --- a/app/assets/stylesheets/pages/profile.scss +++ b/app/assets/stylesheets/pages/profile.scss @@ -20,7 +20,7 @@ .account-btn-link, .profile-settings-sidebar a, .settings-sidebar a { - color: $md-link-color; + color: $blue-600; } .private-tokens-reset div.reset-action:not(:first-child) { @@ -137,7 +137,7 @@ .profile-settings-content { a { - color: $md-link-color; + color: $blue-600; } } @@ -170,7 +170,7 @@ background-color: $gray-light; &.not-active { - color: $provider-btn-not-active-color; + color: $blue-500; } } diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index fffb440027c..c11916454c8 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -449,8 +449,8 @@ &:hover:not(.disabled), &.forked { - background-color: $row-hover; - border-color: $row-hover-border; + background-color: $blue-50; + border-color: $blue-200; } .avatar-container, @@ -894,7 +894,7 @@ pre.light-well { .cannot-be-merged, .cannot-be-merged:hover { - color: $error-exclamation-point; + color: $red-500; margin-top: 2px; } diff --git a/app/assets/stylesheets/pages/settings.scss b/app/assets/stylesheets/pages/settings.scss index fb03970f64f..5aa4cdec9c3 100644 --- a/app/assets/stylesheets/pages/settings.scss +++ b/app/assets/stylesheets/pages/settings.scss @@ -113,9 +113,9 @@ .settings-message { padding: 5px; line-height: 1.3; - color: $warning-message-color; - background-color: $warning-message-bg; - border: 1px solid $warning-message-border; + color: $orange-700; + background-color: $orange-100; + border: 1px solid $orange-200; border-radius: $border-radius-base; } @@ -303,7 +303,7 @@ } .mirror-error-badge { - background-color: $error-bg; + background-color: $red-400; border-radius: $border-radius-default; color: $white-light; } diff --git a/app/assets/stylesheets/pages/todos.scss b/app/assets/stylesheets/pages/todos.scss index 010a2c05a1c..5d3b7b21ce4 100644 --- a/app/assets/stylesheets/pages/todos.scss +++ b/app/assets/stylesheets/pages/todos.scss @@ -12,8 +12,8 @@ flex-direction: row; &:hover { - background-color: $row-hover; - border-color: $row-hover-border; + background-color: $blue-50; + border-color: $blue-200; cursor: pointer; } @@ -22,7 +22,7 @@ border-bottom: 1px solid transparent; &:hover { - border-color: $row-hover-border; + border-color: $blue-200; } } diff --git a/app/assets/stylesheets/pages/tree.scss b/app/assets/stylesheets/pages/tree.scss index 1cc26d40ba9..dc5ca78ff58 100644 --- a/app/assets/stylesheets/pages/tree.scss +++ b/app/assets/stylesheets/pages/tree.scss @@ -111,9 +111,9 @@ &:hover:not(.tree-truncated-warning) { td { - background-color: $row-hover; - border-top: 1px solid $row-hover-border; - border-bottom: 1px solid $row-hover-border; + background-color: $blue-50; + border-top: 1px solid $blue-200; + border-bottom: 1px solid $blue-200; cursor: pointer; } } @@ -229,7 +229,7 @@ .upload-link { font-weight: $gl-font-weight-normal; - color: $md-link-color; + color: $blue-600; } .repo-charts { diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index 86bade49ec9..9e30b982b06 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -1,67 +1,39 @@ class AutocompleteController < ApplicationController - AWARD_EMOJI_MAX = 100 - skip_before_action :authenticate_user!, only: [:users, :award_emojis] - before_action :load_project, only: [:users] - before_action :load_group, only: [:users] def users - @users = AutocompleteUsersFinder.new(params: params, current_user: current_user, project: @project, group: @group).execute - - render json: UserSerializer.new.represent(@users) - end - - def user - @user = User.find(params[:id]) - render json: UserSerializer.new.represent(@user) - end - - def projects - project = Project.find_by_id(params[:project_id]) - projects = projects_finder.execute(project, search: params[:search], offset_id: params[:offset_id]) + project = Autocomplete::ProjectFinder + .new(current_user, params) + .execute - render json: projects.to_json(only: [:id, :name_with_namespace], methods: :name_with_namespace) - end + group = Autocomplete::GroupFinder + .new(current_user, project, params) + .execute - def award_emojis - emoji_with_count = AwardEmoji - .limit(AWARD_EMOJI_MAX) - .where(user: current_user) - .group(:name) - .order('count_all DESC, name ASC') - .count + users = Autocomplete::UsersFinder + .new(params: params, current_user: current_user, project: project, group: group) + .execute - # Transform from hash to array to guarantee json order - # e.g. { 'thumbsup' => 2, 'thumbsdown' = 1 } - # => [{ name: 'thumbsup' }, { name: 'thumbsdown' }] - render json: emoji_with_count.map { |k, v| { name: k } } + render json: UserSerializer.new.represent(users) end - private - - def load_group - @group ||= begin - if @project.blank? && params[:group_id].present? - group = Group.find(params[:group_id]) - return render_404 unless can?(current_user, :read_group, group) + def user + user = UserFinder.new(params).execute! - group - end - end + render json: UserSerializer.new.represent(user) end - def load_project - @project ||= begin - if params[:project_id].present? - project = Project.find(params[:project_id]) - return render_404 unless can?(current_user, :read_project, project) + # Displays projects to use for the dropdown when moving a resource from one + # project to another. + def projects + projects = Autocomplete::MoveToProjectFinder + .new(current_user, params) + .execute - project - end - end + render json: MoveToProjectSerializer.new.represent(projects) end - def projects_finder - MoveToProjectFinder.new(current_user) + def award_emojis + render json: AwardedEmojiFinder.new(current_user).execute end end diff --git a/app/controllers/projects/pages_controller.rb b/app/controllers/projects/pages_controller.rb index cae6e2c40b8..ff49911d892 100644 --- a/app/controllers/projects/pages_controller.rb +++ b/app/controllers/projects/pages_controller.rb @@ -11,7 +11,7 @@ class Projects::PagesController < Projects::ApplicationController def destroy project.remove_pages - project.pages_domains.destroy_all + project.pages_domains.destroy_all # rubocop: disable DestroyAll respond_to do |format| format.html do diff --git a/app/finders/autocomplete/group_finder.rb b/app/finders/autocomplete/group_finder.rb new file mode 100644 index 00000000000..dd97ac4c817 --- /dev/null +++ b/app/finders/autocomplete/group_finder.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Autocomplete + # Finder for retrieving a group to use for autocomplete data sources. + class GroupFinder + attr_reader :current_user, :project, :group_id + + # current_user - The currently logged in user, if any. + # project - The Project (if any) to use for the autocomplete data sources. + # params - A Hash containing parameters to use for finding the project. + # + # The following parameters are supported: + # + # * group_id: The ID of the group to find. + def initialize(current_user = nil, project = nil, params = {}) + @current_user = current_user + @project = project + @group_id = params[:group_id] + end + + # Attempts to find a Group based on the current group ID. + def execute + return unless project.blank? && group_id.present? + + group = Group.find(group_id) + + # This removes the need for using `return render_404` and similar patterns + # in controllers that use this finder. + unless Ability.allowed?(current_user, :read_group, group) + raise ActiveRecord::RecordNotFound + .new("Could not find a Group with ID #{group_id}") + end + + group + end + end +end diff --git a/app/finders/autocomplete/move_to_project_finder.rb b/app/finders/autocomplete/move_to_project_finder.rb new file mode 100644 index 00000000000..edaf74c5f92 --- /dev/null +++ b/app/finders/autocomplete/move_to_project_finder.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Autocomplete + # Finder that retrieves a list of projects that an issue can be moved to. + class MoveToProjectFinder + attr_reader :current_user, :search, :project_id, :offset_id + + # current_user - The User object of the user that wants to view the list of + # projects. + # + # params - A Hash containing additional parameters to set. + # + # The following parameters can be set (as Symbols): + # + # * search: An optional search query to apply to the list of projects. + # * project_id: The ID of a project to exclude from the returned relation. + # * offset_id: The ID of a project to use for pagination. When given, only + # projects with a lower ID are included in the list. + def initialize(current_user, params = {}) + @current_user = current_user + @search = params[:search] + @project_id = params[:project_id] + @offset_id = params[:offset_id] + end + + def execute + current_user + .projects_where_can_admin_issues + .optionally_search(search) + .excluding_project(project_id) + .paginate_in_descending_order_using_id(before: offset_id) + .eager_load_namespace_and_owner + end + end +end diff --git a/app/finders/autocomplete/project_finder.rb b/app/finders/autocomplete/project_finder.rb new file mode 100644 index 00000000000..3a4696f4c2e --- /dev/null +++ b/app/finders/autocomplete/project_finder.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Autocomplete + # Finder for retrieving a project to use for autocomplete data sources. + class ProjectFinder + attr_reader :current_user, :project_id + + # current_user - The currently logged in user, if any. + # params - A Hash containing parameters to use for finding the project. + # + # The following parameters are supported: + # + # * project_id: The ID of the project to find. + def initialize(current_user = nil, params = {}) + @current_user = current_user + @project_id = params[:project_id] + end + + # Attempts to find a Project based on the current project ID. + def execute + return if project_id.blank? + + project = Project.find(project_id) + + # This removes the need for using `return render_404` and similar patterns + # in controllers that use this finder. + unless Ability.allowed?(current_user, :read_project, project) + raise ActiveRecord::RecordNotFound + .new("Could not find a Project with ID #{project_id}") + end + + project + end + end +end diff --git a/app/finders/autocomplete/users_finder.rb b/app/finders/autocomplete/users_finder.rb new file mode 100644 index 00000000000..b2557469079 --- /dev/null +++ b/app/finders/autocomplete/users_finder.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +module Autocomplete + class UsersFinder + # The number of users to display in the results is hardcoded to 20, and + # pagination is not supported. This ensures that performance remains + # consistent and removes the need for implementing keyset pagination to + # ensure good performance. + LIMIT = 20 + + attr_reader :current_user, :project, :group, :search, :skip_users, + :author_id, :todo_filter, :todo_state_filter, + :filter_by_current_user + + def initialize(params:, current_user:, project:, group:) + @current_user = current_user + @project = project + @group = group + @search = params[:search] + @skip_users = params[:skip_users] + @author_id = params[:author_id] + @todo_filter = params[:todo_filter] + @todo_state_filter = params[:todo_state_filter] + @filter_by_current_user = params[:current_user] + end + + def execute + items = limited_users + + if search.blank? + # Include current user if available to filter by "Me" + items.unshift(current_user) if prepend_current_user? + + if prepend_author? && (author = User.find_by_id(author_id)) + items.unshift(author) + end + end + + items.uniq + end + + private + + # Returns the users based on the input parameters, as an Array. + # + # This method is separate so it is easier to extend in EE. + def limited_users + # When changing the order of these method calls, make sure that + # reorder_by_name() is called _before_ optionally_search(), otherwise + # reorder_by_name will break the ORDER BY applied in optionally_search(). + find_users + .active + .reorder_by_name + .optionally_search(search) + .where_not_in(skip_users) + .limit_to_todo_authors( + user: current_user, + with_todos: todo_filter, + todo_state: todo_state_filter + ) + .limit(LIMIT) + .to_a + end + + def prepend_current_user? + filter_by_current_user.present? && current_user + end + + def prepend_author? + author_id.present? && current_user + end + + def find_users + if project + project.authorized_users.union_with_user(author_id) + elsif group + group.users_with_parents + elsif current_user + User.all + else + User.none + end + end + end +end diff --git a/app/finders/autocomplete_users_finder.rb b/app/finders/autocomplete_users_finder.rb deleted file mode 100644 index e8a03947f59..00000000000 --- a/app/finders/autocomplete_users_finder.rb +++ /dev/null @@ -1,68 +0,0 @@ -class AutocompleteUsersFinder - # The number of users to display in the results is hardcoded to 20, and - # pagination is not supported. This ensures that performance remains - # consistent and removes the need for implementing keyset pagination to ensure - # good performance. - LIMIT = 20 - - attr_reader :current_user, :project, :group, :search, :skip_users, - :author_id, :params - - def initialize(params:, current_user:, project:, group:) - @current_user = current_user - @project = project - @group = group - @search = params[:search] - @skip_users = params[:skip_users] - @author_id = params[:author_id] - @params = params - end - - def execute - items = find_users - items = items.active - items = items.reorder(:name) - items = items.search(search) if search.present? - items = items.where.not(id: skip_users) if skip_users.present? - items = items.limit(LIMIT) - - if params[:todo_filter].present? && current_user - items = items.todo_authors(current_user.id, params[:todo_state_filter]) - end - - if search.blank? - # Include current user if available to filter by "Me" - if params[:current_user].present? && current_user - items = [current_user, *items].uniq - end - - if author_id.present? && current_user - author = User.find_by_id(author_id) - items = [author, *items].uniq if author - end - end - - items - end - - private - - def find_users - return users_from_project if project - return group.users_with_parents if group - return User.all if current_user - - User.none - end - - def users_from_project - if author_id.present? - union = Gitlab::SQL::Union - .new([project.authorized_users, User.where(id: author_id)]) - - User.from("(#{union.to_sql}) #{User.table_name}") - else - project.authorized_users - end - end -end diff --git a/app/finders/awarded_emoji_finder.rb b/app/finders/awarded_emoji_finder.rb new file mode 100644 index 00000000000..f0cc17f3b26 --- /dev/null +++ b/app/finders/awarded_emoji_finder.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# Class for retrieving information about emoji awarded _by_ a particular user. +class AwardedEmojiFinder + attr_reader :current_user + + # current_user - The User to generate the data for. + def initialize(current_user = nil) + @current_user = current_user + end + + def execute + return [] unless current_user + + # We want the resulting data set to be an Array containing the emoji names + # in descending order, based on how often they were awarded. + AwardEmoji + .award_counts_for_user(current_user) + .map { |name, _| { name: name } } + end +end diff --git a/app/finders/move_to_project_finder.rb b/app/finders/move_to_project_finder.rb deleted file mode 100644 index 038d5565a1e..00000000000 --- a/app/finders/move_to_project_finder.rb +++ /dev/null @@ -1,21 +0,0 @@ -class MoveToProjectFinder - PAGE_SIZE = 50 - - def initialize(user) - @user = user - end - - def execute(from_project, search: nil, offset_id: nil) - projects = @user.projects_where_can_admin_issues - projects = projects.search(search) if search.present? - projects = projects.excluding_project(from_project) - projects = projects.order_id_desc - - # infinite scroll using offset - projects = projects.where('projects.id < ?', offset_id) if offset_id.present? - projects = projects.limit(PAGE_SIZE) - - # to ask for Project#name_with_namespace - projects.includes(namespace: :owner) - end -end diff --git a/app/finders/user_finder.rb b/app/finders/user_finder.rb new file mode 100644 index 00000000000..484a93c9873 --- /dev/null +++ b/app/finders/user_finder.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +# A simple finding for obtaining a single User. +# +# While using `User.find_by` directly is straightforward, it can lead to a lot +# of code duplication. Sometimes we just want to find a user by an ID, other +# times we may want to exclude blocked user. By using this finder (and extending +# it whenever necessary) we can keep this logic in one place. +class UserFinder + attr_reader :params + + def initialize(params) + @params = params + end + + # Tries to find a User, returning nil if none could be found. + def execute + User.find_by(id: params[:id]) + end + + # Tries to find a User, raising a `ActiveRecord::RecordNotFound` if it could + # not be found. + def execute! + User.find(params[:id]) + end +end diff --git a/app/models/award_emoji.rb b/app/models/award_emoji.rb index 99c7866d636..ddc516ccb60 100644 --- a/app/models/award_emoji.rb +++ b/app/models/award_emoji.rb @@ -28,6 +28,23 @@ class AwardEmoji < ActiveRecord::Base .where('name IN (?) AND awardable_type = ? AND awardable_id IN (?)', [DOWNVOTE_NAME, UPVOTE_NAME], type, ids) .group('name', 'awardable_id') end + + # Returns the top 100 emoji awarded by the given user. + # + # The returned value is a Hash mapping emoji names to the number of times + # they were awarded: + # + # { 'thumbsup' => 2, 'thumbsdown' => 1 } + # + # user - The User to get the awards for. + # limt - The maximum number of emoji to return. + def award_counts_for_user(user, limit = 100) + limit(limit) + .where(user: user) + .group(:name) + .order('count_all DESC, name ASC') + .count + end end def downvote? diff --git a/app/models/concerns/awardable.rb b/app/models/concerns/awardable.rb index dd07f389fa5..49981db0d80 100644 --- a/app/models/concerns/awardable.rb +++ b/app/models/concerns/awardable.rb @@ -101,7 +101,7 @@ module Awardable end def remove_award_emoji(name, current_user) - award_emoji.where(name: name, user: current_user).destroy_all + award_emoji.where(name: name, user: current_user).destroy_all # rubocop: disable DestroyAll end def toggle_award_emoji(emoji_name, current_user) diff --git a/app/models/concerns/fast_destroy_all.rb b/app/models/concerns/fast_destroy_all.rb index 65ed46ea202..c342d01243e 100644 --- a/app/models/concerns/fast_destroy_all.rb +++ b/app/models/concerns/fast_destroy_all.rb @@ -34,7 +34,7 @@ module FastDestroyAll included do before_destroy do - raise ForbiddenActionError, '`destroy` and `destroy_all` are forbbiden. Please use `fast_destroy_all`' + raise ForbiddenActionError, '`destroy` and `destroy_all` are forbidden. Please use `fast_destroy_all`' end end diff --git a/app/models/concerns/optionally_search.rb b/app/models/concerns/optionally_search.rb new file mode 100644 index 00000000000..dec97b7dee8 --- /dev/null +++ b/app/models/concerns/optionally_search.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module OptionallySearch + extend ActiveSupport::Concern + + module ClassMethods + def search(*) + raise( + NotImplementedError, + 'Your model must implement the "search" class method' + ) + end + + # Optionally limits a result set to those matching the given search query. + def optionally_search(query = nil) + query.present? ? search(query) : all + end + end +end diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb index 4eb211eff61..e7168d49db9 100644 --- a/app/models/internal_id.rb +++ b/app/models/internal_id.rb @@ -111,7 +111,7 @@ class InternalId < ActiveRecord::Base # Generates next internal id and returns it def generate - subject.transaction do + InternalId.transaction do # Create a record in internal_ids if one does not yet exist # and increment its last value # @@ -125,7 +125,7 @@ class InternalId < ActiveRecord::Base # # Note this will acquire a ROW SHARE lock on the InternalId record def track_greatest(new_value) - subject.transaction do + InternalId.transaction do (lookup || create_record).track_greatest_and_save!(new_value) end end @@ -148,7 +148,7 @@ class InternalId < ActiveRecord::Base # violation. We can safely roll-back the nested transaction and perform # a lookup instead to retrieve the record. def create_record - subject.transaction(requires_new: true) do + InternalId.transaction(requires_new: true) do InternalId.create!( **scope, usage: usage_value, diff --git a/app/models/lfs_object.rb b/app/models/lfs_object.rb index 2a1a4ef48b7..97bf5d611c2 100644 --- a/app/models/lfs_object.rb +++ b/app/models/lfs_object.rb @@ -29,11 +29,13 @@ class LfsObject < ActiveRecord::Base [nil, LfsObjectUploader::Store::LOCAL].include?(self.file_store) end + # rubocop: disable DestroyAll def self.destroy_unreferenced joins("LEFT JOIN lfs_objects_projects ON lfs_objects_projects.lfs_object_id = #{table_name}.id") .where(lfs_objects_projects: { id: nil }) .destroy_all end + # rubocop: enable DestroyAll def self.calculate_oid(path) Digest::SHA256.file(path).hexdigest diff --git a/app/models/project.rb b/app/models/project.rb index 94c1d60f071..15336ec2ea2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -28,6 +28,7 @@ class Project < ActiveRecord::Base include WithUploads include BatchDestroyDependentAssociations include FeatureGate + include OptionallySearch extend Gitlab::Cache::RequestCache extend Gitlab::ConfigHelper @@ -384,6 +385,26 @@ class Project < ActiveRecord::Base only_integer: true, message: 'needs to be beetween 10 minutes and 1 month' } + # Paginates a collection using a `WHERE id < ?` condition. + # + # before - A project ID to use for filtering out projects with an equal or + # greater ID. If no ID is given, all projects are included. + # + # limit - The maximum number of rows to include. + def self.paginate_in_descending_order_using_id( + before: nil, + limit: Kaminari.config.default_per_page + ) + relation = order_id_desc.limit(limit) + relation = relation.where('projects.id < ?', before) if before + + relation + end + + def self.eager_load_namespace_and_owner + includes(namespace: :owner) + end + # Returns a collection of projects that is either public or visible to the # logged in user. def self.public_or_visible_to_user(user = nil) diff --git a/app/models/site_statistic.rb b/app/models/site_statistic.rb index daac1c57db9..48324570f0b 100644 --- a/app/models/site_statistic.rb +++ b/app/models/site_statistic.rb @@ -49,7 +49,7 @@ class SiteStatistic < ActiveRecord::Base # # @return [SiteStatistic] record with tracked information def self.fetch - SiteStatistic.transaction(requires_new: true) do + transaction(requires_new: true) do SiteStatistic.first_or_create! end rescue ActiveRecord::RecordNotUnique diff --git a/app/models/user.rb b/app/models/user.rb index fb19de4b980..a6ba90794d6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -19,6 +19,7 @@ class User < ActiveRecord::Base include BulkMemberAccessLoad include BlocksJsonSerialization include WithUploads + include OptionallySearch DEFAULT_NOTIFICATION_LEVEL = :participating @@ -253,11 +254,41 @@ class User < ActiveRecord::Base scope :external, -> { where(external: true) } scope :active, -> { with_state(:active).non_internal } scope :without_projects, -> { joins('LEFT JOIN project_authorizations ON users.id = project_authorizations.user_id').where(project_authorizations: { user_id: nil }) } - scope :todo_authors, ->(user_id, state) { where(id: Todo.where(user_id: user_id, state: state).select(:author_id)) } scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'DESC')) } scope :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'ASC')) } scope :confirmed, -> { where.not(confirmed_at: nil) } + # Limits the users to those that have TODOs, optionally in the given state. + # + # user - The user to get the todos for. + # + # with_todos - If we should limit the result set to users that are the + # authors of todos. + # + # todo_state - An optional state to require the todos to be in. + def self.limit_to_todo_authors(user: nil, with_todos: false, todo_state: nil) + if user && with_todos + where(id: Todo.where(user: user, state: todo_state).select(:author_id)) + else + all + end + end + + # Returns a relation that optionally includes the given user. + # + # user_id - The ID of the user to include. + def self.union_with_user(user_id = nil) + if user_id.present? + union = Gitlab::SQL::Union.new([all, User.unscoped.where(id: user_id)]) + + # We use "unscoped" here so that any inner conditions are not repeated for + # the outer query, which would be redundant. + User.unscoped.from("(#{union.to_sql}) #{User.table_name}") + else + all + end + end + def self.with_two_factor_indistinct joins("LEFT OUTER JOIN u2f_registrations AS u2f ON u2f.user_id = users.id") .where("u2f.id IS NOT NULL OR users.otp_required_for_login = ?", true) @@ -365,6 +396,18 @@ class User < ActiveRecord::Base ).reorder(order % { query: ActiveRecord::Base.connection.quote(query) }, :name) end + # Limits the result set to users _not_ in the given query/list of IDs. + # + # users - The list of users to ignore. This can be an + # `ActiveRecord::Relation`, or an Array. + def where_not_in(users = nil) + users ? where.not(id: users) : all + end + + def reorder_by_name + reorder(:name) + end + # searches user by given pattern # it compares name, email, username fields and user's secondary emails with given pattern # This method uses ILIKE on PostgreSQL and LIKE on MySQL. @@ -516,7 +559,7 @@ class User < ActiveRecord::Base otp_grace_period_started_at: nil, otp_backup_codes: nil ) - self.u2f_registrations.destroy_all + self.u2f_registrations.destroy_all # rubocop: disable DestroyAll end end diff --git a/app/serializers/move_to_project_entity.rb b/app/serializers/move_to_project_entity.rb new file mode 100644 index 00000000000..dac1124b0b3 --- /dev/null +++ b/app/serializers/move_to_project_entity.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +class MoveToProjectEntity < Grape::Entity + expose :id + expose :name_with_namespace +end diff --git a/app/serializers/move_to_project_serializer.rb b/app/serializers/move_to_project_serializer.rb new file mode 100644 index 00000000000..6a59317505c --- /dev/null +++ b/app/serializers/move_to_project_serializer.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class MoveToProjectSerializer < BaseSerializer + entity MoveToProjectEntity +end diff --git a/app/services/labels/promote_service.rb b/app/services/labels/promote_service.rb index c0463052821..623a5f0950e 100644 --- a/app/services/labels/promote_service.rb +++ b/app/services/labels/promote_service.rb @@ -65,7 +65,7 @@ module Labels end def update_project_labels(label_ids) - Label.where(id: label_ids).destroy_all + Label.where(id: label_ids).destroy_all # rubocop: disable DestroyAll end def clone_label_to_group_label(label) diff --git a/app/services/milestones/promote_service.rb b/app/services/milestones/promote_service.rb index 37aa6d3a9bc..660b4faaec0 100644 --- a/app/services/milestones/promote_service.rb +++ b/app/services/milestones/promote_service.rb @@ -73,7 +73,7 @@ module Milestones end def destroy_old_milestones(milestone) - Milestone.where(id: milestone_ids_for_merge(milestone)).destroy_all + Milestone.where(id: milestone_ids_for_merge(milestone)).destroy_all # rubocop: disable DestroyAll end def group_project_ids diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 46a8a5e4d98..76e22507698 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -83,9 +83,6 @@ module Projects end def remove_repository(path) - # Skip repository removal. We use this flag when remove user or group - return true if params[:skip_repo] == true - # There is a possibility project does not have repository or wiki return true unless repo_exists?(path) diff --git a/app/services/projects/move_deploy_keys_projects_service.rb b/app/services/projects/move_deploy_keys_projects_service.rb index 40a22837eaf..9f3f44f30ea 100644 --- a/app/services/projects/move_deploy_keys_projects_service.rb +++ b/app/services/projects/move_deploy_keys_projects_service.rb @@ -27,7 +27,7 @@ module Projects end def remove_remaining_deploy_keys_projects - source_project.deploy_keys_projects.destroy_all + source_project.deploy_keys_projects.destroy_all # rubocop: disable DestroyAll end end end diff --git a/app/services/projects/move_lfs_objects_projects_service.rb b/app/services/projects/move_lfs_objects_projects_service.rb index a5099519594..f78546a1e9c 100644 --- a/app/services/projects/move_lfs_objects_projects_service.rb +++ b/app/services/projects/move_lfs_objects_projects_service.rb @@ -21,7 +21,7 @@ module Projects end def remove_remaining_lfs_objects_project - source_project.lfs_objects_projects.destroy_all + source_project.lfs_objects_projects.destroy_all # rubocop: disable DestroyAll end def non_existent_lfs_objects_projects diff --git a/app/services/projects/move_notification_settings_service.rb b/app/services/projects/move_notification_settings_service.rb index 746605d56f1..109a00dd6d9 100644 --- a/app/services/projects/move_notification_settings_service.rb +++ b/app/services/projects/move_notification_settings_service.rb @@ -22,7 +22,7 @@ module Projects # Remove remaining notification settings from source_project def remove_remaining_notification_settings - source_project.notification_settings.destroy_all + source_project.notification_settings.destroy_all # rubocop: disable DestroyAll end # Get users of current notification_settings diff --git a/app/services/projects/move_project_group_links_service.rb b/app/services/projects/move_project_group_links_service.rb index d9038030f7e..1efafdce36d 100644 --- a/app/services/projects/move_project_group_links_service.rb +++ b/app/services/projects/move_project_group_links_service.rb @@ -26,7 +26,7 @@ module Projects # Remove remaining project group links from source_project def remove_remaining_project_group_links - source_project.reload.project_group_links.destroy_all + source_project.reload.project_group_links.destroy_all # rubocop: disable DestroyAll end def group_links_in_target_project diff --git a/app/services/projects/move_project_members_service.rb b/app/services/projects/move_project_members_service.rb index bb0c0d10242..ec983582d94 100644 --- a/app/services/projects/move_project_members_service.rb +++ b/app/services/projects/move_project_members_service.rb @@ -25,7 +25,7 @@ module Projects def remove_remaining_members # Remove remaining members and authorizations from source_project - source_project.project_members.destroy_all + source_project.project_members.destroy_all # rubocop: disable DestroyAll end def project_members_in_target_project diff --git a/app/services/protected_branches/legacy_api_update_service.rb b/app/services/protected_branches/legacy_api_update_service.rb index 1f6bbe72f85..da8bf2ce02a 100644 --- a/app/services/protected_branches/legacy_api_update_service.rb +++ b/app/services/protected_branches/legacy_api_update_service.rb @@ -38,11 +38,11 @@ module ProtectedBranches def delete_redundant_access_levels unless @developers_can_merge.nil? - @protected_branch.merge_access_levels.destroy_all + @protected_branch.merge_access_levels.destroy_all # rubocop: disable DestroyAll end unless @developers_can_push.nil? - @protected_branch.push_access_levels.destroy_all + @protected_branch.push_access_levels.destroy_all # rubocop: disable DestroyAll end end end diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index 4bc78b5b64e..73fa6089945 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -2,6 +2,8 @@ module Users class DestroyService + DestroyError = Class.new(StandardError) + attr_accessor :current_user def initialize(current_user) @@ -46,9 +48,8 @@ module Users namespace.prepare_for_destroy user.personal_projects.each do |project| - # Skip repository removal because we remove directory with namespace - # that contain all this repositories - ::Projects::DestroyService.new(project, current_user, skip_repo: project.legacy_storage?).execute + success = ::Projects::DestroyService.new(project, current_user).execute + raise DestroyError, "Project #{project.id} can't be deleted" unless success end yield(user) if block_given? diff --git a/app/views/admin/runners/index.html.haml b/app/views/admin/runners/index.html.haml index 8dfd176f1b7..9280ff4d478 100644 --- a/app/views/admin/runners/index.html.haml +++ b/app/views/admin/runners/index.html.haml @@ -49,7 +49,7 @@ = submit_tag 'Search', class: 'btn' .float-right.light - Runners with last contact more than a minute ago: #{@active_runners_cnt} + Runners currently online: #{@active_runners_cnt} %br diff --git a/app/views/projects/mirrors/_mirror_repos.html.haml b/app/views/projects/mirrors/_mirror_repos.html.haml index 53387b3a50c..c6764c7607a 100644 --- a/app/views/projects/mirrors/_mirror_repos.html.haml +++ b/app/views/projects/mirrors/_mirror_repos.html.haml @@ -1,7 +1,7 @@ - expanded = Rails.env.test? - protocols = Gitlab::UrlSanitizer::ALLOWED_SCHEMES.join('|') -%section.settings.project-mirror-settings.js-mirror-settings.no-animate{ class: ('expanded' if expanded) } +%section.settings.project-mirror-settings.js-mirror-settings.no-animate#js-push-remote-settings{ class: ('expanded' if expanded) } .settings-header %h4= _('Mirroring repositories') %button.btn.js-settings-toggle diff --git a/app/workers/remove_expired_group_links_worker.rb b/app/workers/remove_expired_group_links_worker.rb index 6b8b972a440..25128caf72f 100644 --- a/app/workers/remove_expired_group_links_worker.rb +++ b/app/workers/remove_expired_group_links_worker.rb @@ -5,6 +5,6 @@ class RemoveExpiredGroupLinksWorker include CronjobQueue def perform - ProjectGroupLink.expired.destroy_all + ProjectGroupLink.expired.destroy_all # rubocop: disable DestroyAll end end diff --git a/app/workers/remove_old_web_hook_logs_worker.rb b/app/workers/remove_old_web_hook_logs_worker.rb index 17140ac4450..0f486f8991d 100644 --- a/app/workers/remove_old_web_hook_logs_worker.rb +++ b/app/workers/remove_old_web_hook_logs_worker.rb @@ -6,7 +6,9 @@ class RemoveOldWebHookLogsWorker WEB_HOOK_LOG_LIFETIME = 2.days + # rubocop: disable DestroyAll def perform WebHookLog.destroy_all(['created_at < ?', Time.now - WEB_HOOK_LOG_LIFETIME]) end + # rubocop: enable DestroyAll end diff --git a/changelogs/unreleased/47845-propagate_failure_reason-to-job-webhook.yml b/changelogs/unreleased/47845-propagate_failure_reason-to-job-webhook.yml new file mode 100644 index 00000000000..3f85f75bef4 --- /dev/null +++ b/changelogs/unreleased/47845-propagate_failure_reason-to-job-webhook.yml @@ -0,0 +1,5 @@ +--- +title: "#47845 Add failure_reason to job webhook" +merge_request: 21143 +author: matemaciek +type: added diff --git a/changelogs/unreleased/49796-project-deletion-may-not-log-audit-events-during-user-deletion.yml b/changelogs/unreleased/49796-project-deletion-may-not-log-audit-events-during-user-deletion.yml new file mode 100644 index 00000000000..a8e3d590a4a --- /dev/null +++ b/changelogs/unreleased/49796-project-deletion-may-not-log-audit-events-during-user-deletion.yml @@ -0,0 +1,5 @@ +--- +title: 'Fix: Project deletion may not log audit events during user deletion' +merge_request: +author: +type: fixed diff --git a/changelogs/unreleased/49907-commits-and-merge-requests-does-not-list-all-files-when-one-file-exceeds-size-limits.yml b/changelogs/unreleased/49907-commits-and-merge-requests-does-not-list-all-files-when-one-file-exceeds-size-limits.yml new file mode 100644 index 00000000000..2fce00a662f --- /dev/null +++ b/changelogs/unreleased/49907-commits-and-merge-requests-does-not-list-all-files-when-one-file-exceeds-size-limits.yml @@ -0,0 +1,5 @@ +--- +title: Fix merge requests not showing any diff files for big patches +merge_request: 21125 +author: +type: fixed diff --git a/changelogs/unreleased/50101-builds-dropdown.yml b/changelogs/unreleased/50101-builds-dropdown.yml new file mode 100644 index 00000000000..9194b0e0d31 --- /dev/null +++ b/changelogs/unreleased/50101-builds-dropdown.yml @@ -0,0 +1,6 @@ +--- +title: Creates vue components for stage dropdowns and job list container for job log + view +merge_request: +author: +type: other diff --git a/changelogs/unreleased/50101-commit-block.yml b/changelogs/unreleased/50101-commit-block.yml new file mode 100644 index 00000000000..f6bad4c8154 --- /dev/null +++ b/changelogs/unreleased/50101-commit-block.yml @@ -0,0 +1,5 @@ +--- +title: Creates vue component for commit block in job log page +merge_request: +author: +type: other diff --git a/changelogs/unreleased/50101-trigger.yml b/changelogs/unreleased/50101-trigger.yml new file mode 100644 index 00000000000..df4243afa63 --- /dev/null +++ b/changelogs/unreleased/50101-trigger.yml @@ -0,0 +1,5 @@ +--- +title: Creates Vue component for trigger variables block in job log page +merge_request: +author: +type: other diff --git a/changelogs/unreleased/ab-49446-internal-ids-inconsistency.yml b/changelogs/unreleased/ab-49446-internal-ids-inconsistency.yml new file mode 100644 index 00000000000..bfea57d79e0 --- /dev/null +++ b/changelogs/unreleased/ab-49446-internal-ids-inconsistency.yml @@ -0,0 +1,5 @@ +--- +title: Add migration to cleanup internal_ids inconsistency. +merge_request: 20926 +author: +type: fixed diff --git a/changelogs/unreleased/repopulate_site_statistics.yml b/changelogs/unreleased/repopulate_site_statistics.yml new file mode 100644 index 00000000000..1961088061d --- /dev/null +++ b/changelogs/unreleased/repopulate_site_statistics.yml @@ -0,0 +1,5 @@ +--- +title: Migrate NULL wiki_access_level to correct number so we count active wikis correctly +merge_request: 21030 +author: +type: changed diff --git a/changelogs/unreleased/runners-online.yml b/changelogs/unreleased/runners-online.yml new file mode 100644 index 00000000000..a732d9cb723 --- /dev/null +++ b/changelogs/unreleased/runners-online.yml @@ -0,0 +1,5 @@ +--- +title: Clarify current runners online text +merge_request: 21151 +author: Ben Bodenmiller +type: other diff --git a/changelogs/unreleased/sh-bump-gitaly-for-11-2.yml b/changelogs/unreleased/sh-bump-gitaly-for-11-2.yml new file mode 100644 index 00000000000..0e748c3a346 --- /dev/null +++ b/changelogs/unreleased/sh-bump-gitaly-for-11-2.yml @@ -0,0 +1,5 @@ +--- +title: Bump Gitaly to 0.117.1 for Rouge update +merge_request: 21277 +author: +type: security diff --git a/db/migrate/20160712171823_remove_award_emojis_with_no_user.rb b/db/migrate/20160712171823_remove_award_emojis_with_no_user.rb index 668c22bb51c..8ebf1a5234d 100644 --- a/db/migrate/20160712171823_remove_award_emojis_with_no_user.rb +++ b/db/migrate/20160712171823_remove_award_emojis_with_no_user.rb @@ -16,6 +16,6 @@ class RemoveAwardEmojisWithNoUser < ActiveRecord::Migration # disable_ddl_transaction! def up - AwardEmoji.joins('LEFT JOIN users ON users.id = user_id').where('users.id IS NULL').destroy_all + AwardEmoji.joins('LEFT JOIN users ON users.id = user_id').where('users.id IS NULL').destroy_all # rubocop: disable DestroyAll end end diff --git a/db/post_migrate/20180723130817_delete_inconsistent_internal_id_records.rb b/db/post_migrate/20180723130817_delete_inconsistent_internal_id_records.rb new file mode 100644 index 00000000000..3b9b95ec9ca --- /dev/null +++ b/db/post_migrate/20180723130817_delete_inconsistent_internal_id_records.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true +class DeleteInconsistentInternalIdRecords < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + # This migration cleans up any inconsistent records in internal_ids. + # + # That is, it deletes records that track a `last_value` that is + # smaller than the maximum internal id (usually `iid`) found in + # the corresponding model records. + + def up + disable_statement_timeout do + delete_internal_id_records('issues', 'project_id') + delete_internal_id_records('merge_requests', 'project_id', 'target_project_id') + delete_internal_id_records('deployments', 'project_id') + delete_internal_id_records('milestones', 'project_id') + delete_internal_id_records('milestones', 'namespace_id', 'group_id') + delete_internal_id_records('ci_pipelines', 'project_id') + end + end + + class InternalId < ActiveRecord::Base + self.table_name = 'internal_ids' + enum usage: { issues: 0, merge_requests: 1, deployments: 2, milestones: 3, epics: 4, ci_pipelines: 5 } + end + + private + + def delete_internal_id_records(base_table, scope_column_name, base_scope_column_name = scope_column_name) + sql = <<~SQL + SELECT id FROM ( -- workaround for MySQL + SELECT internal_ids.id FROM ( + SELECT #{base_scope_column_name} AS #{scope_column_name}, max(iid) as maximum_iid from #{base_table} GROUP BY #{scope_column_name} + ) maxima JOIN internal_ids USING (#{scope_column_name}) + WHERE internal_ids.usage=#{InternalId.usages.fetch(base_table)} AND maxima.maximum_iid > internal_ids.last_value + ) internal_ids + SQL + + InternalId.where("id IN (#{sql})").tap do |ids| # rubocop:disable GitlabSecurity/SqlInjection + say "Deleting internal_id records for #{base_table}: #{ids.pluck(:project_id, :last_value)}" unless ids.empty? + end.delete_all + end +end diff --git a/db/post_migrate/20180809195358_migrate_null_wiki_access_levels.rb b/db/post_migrate/20180809195358_migrate_null_wiki_access_levels.rb new file mode 100644 index 00000000000..0a0a33299e4 --- /dev/null +++ b/db/post_migrate/20180809195358_migrate_null_wiki_access_levels.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class MigrateNullWikiAccessLevels < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + class ProjectFeature < ActiveRecord::Base + include EachBatch + + self.table_name = 'project_features' + end + + def up + ProjectFeature.where(wiki_access_level: nil).each_batch do |relation| + relation.update_all(wiki_access_level: 20) + end + + # We need to re-count wikis as previous attempt was not considering the NULLs. + transaction do + execute('SET LOCAL statement_timeout TO 0') if Gitlab::Database.postgresql? # see https://gitlab.com/gitlab-org/gitlab-ce/issues/48967 + + execute("UPDATE site_statistics SET wikis_count = (SELECT COUNT(*) FROM project_features WHERE wiki_access_level != 0)") + end + end + + def down + # there is no way to rollback this change, there are no downsides in keeping migrated data. + end +end diff --git a/db/schema.rb b/db/schema.rb index 1288a98745c..9dc122b54b3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180808162000) do +ActiveRecord::Schema.define(version: 20180809195358) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/doc/administration/high_availability/nfs.md b/doc/administration/high_availability/nfs.md index 387c3fb6a5b..cd2284f5f2a 100644 --- a/doc/administration/high_availability/nfs.md +++ b/doc/administration/high_availability/nfs.md @@ -55,14 +55,14 @@ Below is an example of an NFS mount point defined in `/etc/fstab` we use on GitLab.com: ``` -10.1.1.1:/var/opt/gitlab/git-data /var/opt/gitlab/git-data nfs4 defaults,soft,rsize=1048576,wsize=1048576,noatime,nobootwait,lookupcache=positive 0 2 +10.1.1.1:/var/opt/gitlab/git-data /var/opt/gitlab/git-data nfs4 defaults,soft,rsize=1048576,wsize=1048576,noatime,nofail,lookupcache=positive 0 2 ``` Notice several options that you should consider using: | Setting | Description | | ------- | ----------- | -| `nobootwait` | Don't halt boot process waiting for this mount to become available +| `nofail` | Don't halt boot process waiting for this mount to become available | `lookupcache=positive` | Tells the NFS client to honor `positive` cache results but invalidates any `negative` cache results. Negative cache results cause problems with Git. Specifically, a `git push` can fail to register uniformly across all NFS clients. The negative cache causes the clients to 'remember' that the files did not exist previously. ## A single NFS mount diff --git a/doc/administration/repository_storage_types.md b/doc/administration/repository_storage_types.md index 88221db78f1..bd758c49eba 100644 --- a/doc/administration/repository_storage_types.md +++ b/doc/administration/repository_storage_types.md @@ -42,7 +42,7 @@ Registry, etc. ## Hashed Storage > **Warning:** Hashed storage is in **Beta**. For the latest updates, check the -> associated [issue](https://gitlab.com/gitlab-com/infrastructure/issues/2821) +> associated [issue](https://gitlab.com/gitlab-com/infrastructure/issues/3542) > and please report any problems you encounter. Hashed Storage is the new storage behavior we are rolling out with 10.0. Instead diff --git a/doc/ci/README.md b/doc/ci/README.md index 7666219acb0..d782d64e971 100644 --- a/doc/ci/README.md +++ b/doc/ci/README.md @@ -76,6 +76,8 @@ learn how to leverage its potential even more. - [Trigger pipelines on a schedule](../user/project/pipelines/schedules.md) - [Kubernetes clusters](../user/project/clusters/index.md) - Integrate one or more Kubernetes clusters to your project +- [Interactive web terminal](interactive_web_terminal/index.md) - Open an interactive + web terminal to debug the running jobs ## GitLab CI/CD for Docker diff --git a/doc/ci/interactive_web_terminal/img/finished_job_with_terminal_open.png b/doc/ci/interactive_web_terminal/img/finished_job_with_terminal_open.png Binary files differnew file mode 100644 index 00000000000..199268a1486 --- /dev/null +++ b/doc/ci/interactive_web_terminal/img/finished_job_with_terminal_open.png diff --git a/doc/ci/interactive_web_terminal/img/interactive_web_terminal_page.png b/doc/ci/interactive_web_terminal/img/interactive_web_terminal_page.png Binary files differnew file mode 100644 index 00000000000..b59c1b6bc43 --- /dev/null +++ b/doc/ci/interactive_web_terminal/img/interactive_web_terminal_page.png diff --git a/doc/ci/interactive_web_terminal/img/interactive_web_terminal_running_job.png b/doc/ci/interactive_web_terminal/img/interactive_web_terminal_running_job.png Binary files differnew file mode 100644 index 00000000000..f92c6df07a1 --- /dev/null +++ b/doc/ci/interactive_web_terminal/img/interactive_web_terminal_running_job.png diff --git a/doc/ci/interactive_web_terminal/index.md b/doc/ci/interactive_web_terminal/index.md new file mode 100644 index 00000000000..507aceb27fa --- /dev/null +++ b/doc/ci/interactive_web_terminal/index.md @@ -0,0 +1,52 @@ +# Getting started with interactive web terminals + +> Introduced in GitLab 11.3. + +CAUTION: **Warning:** +Interactive web terminals are in beta, so they might not work properly and +lack features. For more information [follow issue #25990](https://gitlab.com/gitlab-org/gitlab-ce/issues/25990). + +Interactive web terminals give the user access to a terminal in GitLab for +running one-of commands for their CI pipeline. + +NOTE: **Note:** +This is not available for the shared Runners on GitLab.com. +To make use of this feature, you need to provide your +[own Runner](https://docs.gitlab.com/runner/install/) and properly +[configure it](#configuration). + +## Configuration + +Two things need to be configured for the interactive web terminal to work: + +- The Runner needs to have [`[session_server]` configured + properly][session-server] +- Web terminals need to be + [enabled](../../administration/integration/terminal.md#enabling-and-disabling-terminal-support) + +## Debugging a running job + +NOTE: **Note:** Not all executors are +[supported](https://docs.gitlab.com/runner/executors/#compatibility-chart). + +Sometimes, when a job is running, things don't go as you would expect, and it +would be helpful if one can have a shell to aid debugging. When a job is +running, on the right panel you can see a button `debug` that will open the terminal +for the current job. + + + +When clicked, a new tab will open to the terminal page where you can access +the terminal and type commands like a normal shell. + + + +If you have the terminal open and the job has finished with its tasks, the +terminal will block the job from finishing for the duration configured in +[`[session_server].terminal_max_retention_time`][session-server] until you +close the terminal window. + + + +[session-server]: https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-session_server-section diff --git a/doc/development/README.md b/doc/development/README.md index fed3903c771..ee9a9852205 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -55,7 +55,13 @@ description: 'Learn how to contribute to GitLab.' - [Merge request performance guidelines](merge_request_performance_guidelines.md) for ensuring merge requests do not negatively impact GitLab performance -## Databases guides +## Database guides + +### Tooling + +- [Understanding EXPLAIN plans](understanding_explain_plans.md) +- [explain.depesz.com](https://explain.depesz.com/) for visualising the output + of `EXPLAIN` ### Migrations diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index 32de741c9fe..1cd873b6fe3 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -258,6 +258,31 @@ end [`extend ::Gitlab::Utils::Override`]: utilities.md#override +##### Overriding CE class methods + +The same applies to class methods, except we want to use +`ActiveSupport::Concern` and put `extend ::Gitlab::Utils::Override` +within the block of `class_methods`. Here's an example: + +```ruby +module EE + module Groups + module GroupMembersController + extend ActiveSupport::Concern + + class_methods do + extend ::Gitlab::Utils::Override + + override :admin_not_required_endpoints + def admin_not_required_endpoints + super.concat(%i[update override]) + end + end + end + end +end +``` + #### Use self-descriptive wrapper methods When it's not possible/logical to modify the implementation of a @@ -665,6 +690,9 @@ module EE extend ActiveSupport::Concern class_methods do + extend ::Gitlab::Utils::Override + + override :update_params_at_least_one_of def update_params_at_least_one_of super.push(*%i[ squash diff --git a/doc/development/understanding_explain_plans.md b/doc/development/understanding_explain_plans.md new file mode 100644 index 00000000000..adf8795a5e3 --- /dev/null +++ b/doc/development/understanding_explain_plans.md @@ -0,0 +1,676 @@ +# Understanding EXPLAIN plans + +PostgreSQL allows you to obtain query plans using the `EXPLAIN` command. This +command can be invaluable when trying to determine how a query will perform. +You can use this command directly in your SQL query, as long as the query starts +with it: + +```sql +EXPLAIN +SELECT COUNT(*) +FROM projects +WHERE visibility_level IN (0, 20); +``` + +When running this on GitLab.com, we are presented with the following output: + +``` +Aggregate (cost=922411.76..922411.77 rows=1 width=8) + -> Seq Scan on projects (cost=0.00..908044.47 rows=5746914 width=0) + Filter: (visibility_level = ANY ('{0,20}'::integer[])) +``` + +When using _just_ `EXPLAIN`, PostgreSQL won't actually execute our query, +instead it produces an _estimated_ execution plan based on the available +statistics. This means the actual plan can differ quite a bit. Fortunately, +PostgreSQL provides us with the option to execute the query as well. To do so, +we need to use `EXPLAIN ANALYZE` instead of just `EXPLAIN`: + +```sql +EXPLAIN ANALYZE +SELECT COUNT(*) +FROM projects +WHERE visibility_level IN (0, 20); +``` + +This will produce: + +``` +Aggregate (cost=922420.60..922420.61 rows=1 width=8) (actual time=3428.535..3428.535 rows=1 loops=1) + -> Seq Scan on projects (cost=0.00..908053.18 rows=5746969 width=0) (actual time=0.041..2987.606 rows=5746940 loops=1) + Filter: (visibility_level = ANY ('{0,20}'::integer[])) + Rows Removed by Filter: 65677 +Planning time: 2.861 ms +Execution time: 3428.596 ms +``` + +As we can see this plan is quite different, and includes a lot more data. Let's +discuss this step by step. + +Because `EXPLAIN ANALYZE` executes the query, care should be taken when using a +query that will write data or might time out. If the query modifies data, +consider wrapping it in a transaction that rolls back automatically like so: + +```sql +BEGIN; +EXPLAIN ANALYZE +DELETE FROM users WHERE id = 1; +ROLLBACK; +``` + +The `EXPLAIN` command also takes additional options, such as `BUFFERS`: + +```sql +EXPLAIN (ANALYZE, BUFFERS) +SELECT COUNT(*) +FROM projects +WHERE visibility_level IN (0, 20); +``` + +This will then produce: + +``` +Aggregate (cost=922420.60..922420.61 rows=1 width=8) (actual time=3428.535..3428.535 rows=1 loops=1) + Buffers: shared hit=208846 + -> Seq Scan on projects (cost=0.00..908053.18 rows=5746969 width=0) (actual time=0.041..2987.606 rows=5746940 loops=1) + Filter: (visibility_level = ANY ('{0,20}'::integer[])) + Rows Removed by Filter: 65677 + Buffers: shared hit=208846 +Planning time: 2.861 ms +Execution time: 3428.596 ms +``` + +For more information, refer to the official [EXPLAIN +documentation](https://www.postgresql.org/docs/current/static/sql-explain.html). + +## Nodes + +Every query plan consists of nodes. Nodes can be nested, and are executed from +the inside out. This means that the innermost node is executed before an outer +node. This can be best thought of as nested function calls, returning their +results as they unwind. For example, a plan starting with an `Aggregate` +followed by a `Nested Loop`, followed by an `Index Only scan` can be thought of +as the following Ruby code: + +```ruby +aggregate( + nested_loop( + index_only_scan() + index_only_scan() + ) +) +``` + +Nodes are indicated using a `->` followed by the type of node taken. For +example: + +``` +Aggregate (cost=922411.76..922411.77 rows=1 width=8) + -> Seq Scan on projects (cost=0.00..908044.47 rows=5746914 width=0) + Filter: (visibility_level = ANY ('{0,20}'::integer[])) +``` + +Here the first node executed is `Seq scan on projects`. The `Filter:` is an +additional filter applied to the results of the node. A filter is very similar +to Ruby's `Array#select`: it takes the input rows, applies the filter, and +produces a new list of rows. Once the node is done, we perform the `Aggregate` +above it. + +Nested nodes will look like this: + +``` +Aggregate (cost=176.97..176.98 rows=1 width=8) (actual time=0.252..0.252 rows=1 loops=1) + Buffers: shared hit=155 + -> Nested Loop (cost=0.86..176.75 rows=87 width=0) (actual time=0.035..0.249 rows=36 loops=1) + Buffers: shared hit=155 + -> Index Only Scan using users_pkey on users users_1 (cost=0.43..4.95 rows=87 width=4) (actual time=0.029..0.123 rows=36 loops=1) + Index Cond: (id < 100) + Heap Fetches: 0 + -> Index Only Scan using users_pkey on users (cost=0.43..1.96 rows=1 width=4) (actual time=0.003..0.003 rows=1 loops=36) + Index Cond: (id = users_1.id) + Heap Fetches: 0 +Planning time: 2.585 ms +Execution time: 0.310 ms +``` + +Here we first perform two separate "Index Only" scans, followed by performing a +"Nested Loop" on the result of these two scans. + +## Node statistics + +Each node in a plan has a set of associated statistics, such as the cost, the +number of rows produced, the number of loops performed, and more. For example: + +``` +Seq Scan on projects (cost=0.00..908044.47 rows=5746914 width=0) +``` + +Here we can see that our cost ranges from `0.00..908044.47` (we'll cover this in +a moment), and we estimate (since we're using `EXPLAIN` and not `EXPLAIN +ANALYZE`) a total of 5,746,914 rows to be produced by this node. The `width` +statistics describes the estimated width of each row, in bytes. + +The `costs` field specifies how expensive a node was. The cost is measured in +arbitrary units determined by the query planner's cost parameters. What +influences the costs depends on a variety of settings, such as `seq_page_cost`, +`cpu_tuple_cost`, and various others. +The format of the costs field is as follows: + +``` +STARTUP COST..TOTAL COST +``` + +The startup cost states how expensive it was to start the node, with the total +cost describing how expensive the entire node was. In general: the greater the +values, the more expensive the node. + +When using `EXPLAIN ANALYZE`, these statistics will also include the actual time +(in milliseconds) spent, and other runtime statistics (e.g. the actual number of +produced rows): + +``` +Seq Scan on projects (cost=0.00..908053.18 rows=5746969 width=0) (actual time=0.041..2987.606 rows=5746940 loops=1) +``` + +Here we can see we estimated 5,746,969 rows to be returned, but in reality we +returned 5,746,940 rows. We can also see that _just_ this sequential scan took +2.98 seconds to run. + +Using `EXPLAIN (ANALYZE, BUFFERS)` will also give us information about the +number of rows removed by a filter, the number of buffers used, and more. For +example: + +``` +Seq Scan on projects (cost=0.00..908053.18 rows=5746969 width=0) (actual time=0.041..2987.606 rows=5746940 loops=1) + Filter: (visibility_level = ANY ('{0,20}'::integer[])) + Rows Removed by Filter: 65677 + Buffers: shared hit=208846 +``` + +Here we can see that our filter has to remove 65,677 rows, and that we use +208,846 buffers. Each buffer in PostgreSQL is 8 KB (8192 bytes), meaning our +above node uses *1.6 GB of buffers*. That's a lot! + +## Node types + +There are quite a few different types of nodes, so we only cover some of the +more common ones here. + +A full list of all the available nodes and their descriptions can be found in +the [PostgreSQL source file +"plannodes.h"](https://github.com/postgres/postgres/blob/master/src/include/nodes/plannodes.h) + +### Seq Scan + +A sequential scan over (a chunk of) a database table. This is like using +`Array#each`, but on a database table. Sequential scans can be quite slow when +retrieving lots of rows, so it's best to avoid these for large tables. + +### Index Only Scan + +A scan on an index that did not require fetching anything from the table. In +certain cases an index only scan may still fetch data from the table, in this +case the node will include a `Heap Fetches:` statistic. + +### Index Scan + +A scan on an index that required retrieving some data from the table. + +### Bitmap Index Scan and Bitmap Heap scan + +Bitmap scans fall between sequential scans and index scans. These are typically +used when we would read too much data from an index scan, but too little to +perform a sequential scan. A bitmap scan uses what is known as a [bitmap +index](https://en.wikipedia.org/wiki/Bitmap_index) to perform its work. + +The [source code of PostgreSQL](https://github.com/postgres/postgres/blob/1c2cb2744bf3d8ad751cd5cf3b347f10f48492b3/src/include/nodes/plannodes.h#L446-L457) +states the following on bitmap scans: + +> Bitmap Index Scan delivers a bitmap of potential tuple locations; it does not +> access the heap itself. The bitmap is used by an ancestor Bitmap Heap Scan +> node, possibly after passing through intermediate Bitmap And and/or Bitmap Or +> nodes to combine it with the results of other Bitmap Index Scans. + +### Limit + +Applies a `LIMIT` on the input rows. + +### Sort + +Sorts the input rows as specified using an `ORDER BY` statement. + +### Nested Loop + +A nested loop will execute its child nodes for every row produced by a node that +precedes it. For example: + +``` +-> Nested Loop (cost=0.86..176.75 rows=87 width=0) (actual time=0.035..0.249 rows=36 loops=1) + Buffers: shared hit=155 + -> Index Only Scan using users_pkey on users users_1 (cost=0.43..4.95 rows=87 width=4) (actual time=0.029..0.123 rows=36 loops=1) + Index Cond: (id < 100) + Heap Fetches: 0 + -> Index Only Scan using users_pkey on users (cost=0.43..1.96 rows=1 width=4) (actual time=0.003..0.003 rows=1 loops=36) + Index Cond: (id = users_1.id) + Heap Fetches: 0 +``` + +Here the first child node (`Index Only Scan using users_pkey on users users_1`) +produces 36 rows, and is executed once (`rows=36 loops=1`). The next node +produces 1 row (`rows=1`), but is repeated 36 times (`loops=36`). This is +because the previous node produced 36 rows. + +This means that nested loops can quickly slow the query down if the various +child nodes keep producing many rows. + +## Optimising queries + +With that out of the way, let's see how we can optimise a query. Let's use the +following query as an example: + +```sql +SELECT COUNT(*) +FROM users +WHERE twitter != ''; +``` + +This query simply counts the number of users that have a Twitter profile set. +Let's run this using `EXPLAIN (ANALYZE, BUFFERS)`: + +```sql +EXPLAIN (ANALYZE, BUFFERS) +SELECT COUNT(*) +FROM users +WHERE twitter != ''; +``` + +This will produce the following plan: + +``` +Aggregate (cost=845110.21..845110.22 rows=1 width=8) (actual time=1271.157..1271.158 rows=1 loops=1) + Buffers: shared hit=202662 + -> Seq Scan on users (cost=0.00..844969.99 rows=56087 width=0) (actual time=0.019..1265.883 rows=51833 loops=1) + Filter: ((twitter)::text <> ''::text) + Rows Removed by Filter: 2487813 + Buffers: shared hit=202662 +Planning time: 0.390 ms +Execution time: 1271.180 ms +``` + +From this query plan we can see the following: + +1. We need to perform a sequential scan on the `users` table. +1. This sequential scan filters out 2,487,813 rows using a `Filter`. +1. We use 202,622 buffers, which equals 1.58 GB of memory. +1. It takes us 1.2 seconds to do all of this. + +Considering we are just counting users, that's quite expensive! + +Before we start making any changes, let's see if there are any existing indexes +on the `users` table that we might be able to use. We can obtain this +information by running `\d users` in a `psql` console, then scrolling down to +the `Indexes:` section: + +``` +Indexes: + "users_pkey" PRIMARY KEY, btree (id) + "users_confirmation_token_key" UNIQUE CONSTRAINT, btree (confirmation_token) + "users_email_key" UNIQUE CONSTRAINT, btree (email) + "users_reset_password_token_key" UNIQUE CONSTRAINT, btree (reset_password_token) + "index_on_users_lower_email" btree (lower(email::text)) + "index_on_users_lower_username" btree (lower(username::text)) + "index_on_users_name_lower" btree (lower(name::text)) + "index_users_on_admin" btree (admin) + "index_users_on_created_at" btree (created_at) + "index_users_on_email_trigram" gin (email gin_trgm_ops) + "index_users_on_feed_token" btree (feed_token) + "index_users_on_ghost" btree (ghost) + "index_users_on_incoming_email_token" btree (incoming_email_token) + "index_users_on_name" btree (name) + "index_users_on_name_trigram" gin (name gin_trgm_ops) + "index_users_on_state" btree (state) + "index_users_on_state_and_internal_attrs" btree (state) WHERE ghost <> true AND support_bot <> true + "index_users_on_support_bot" btree (support_bot) + "index_users_on_username" btree (username) + "index_users_on_username_trigram" gin (username gin_trgm_ops) +``` + +Here we can see there is no index on the `twitter` column, which means +PostgreSQL has to perform a sequential scan in this case. Let's try to fix this +by adding the following index: + +```sql +CREATE INDEX CONCURRENTLY twitter_test ON users (twitter); +``` + +If we now re-run our query using `EXPLAIN (ANALYZE, BUFFERS)` we get the +following plan: + +``` +Aggregate (cost=61002.82..61002.83 rows=1 width=8) (actual time=297.311..297.312 rows=1 loops=1) + Buffers: shared hit=51854 dirtied=19 + -> Index Only Scan using twitter_test on users (cost=0.43..60873.13 rows=51877 width=0) (actual time=279.184..293.532 rows=51833 loops=1) + Filter: ((twitter)::text <> ''::text) + Rows Removed by Filter: 2487830 + Heap Fetches: 26037 + Buffers: shared hit=51854 dirtied=19 +Planning time: 0.191 ms +Execution time: 297.334 ms +``` + +Now it takes just under 300 milliseconds to get our data, instead of 1.2 +seconds. However, we still use 51,854 buffers, which is about 400 MB of memory. +300 milliseconds is also quite slow for such a simple query. To understand why +this query is still expensive, let's take a look at the following: + +``` +Index Only Scan using twitter_test on users (cost=0.43..60873.13 rows=51877 width=0) (actual time=279.184..293.532 rows=51833 loops=1) + Filter: ((twitter)::text <> ''::text) + Rows Removed by Filter: 2487830 +``` + +We start with an index only scan on our index, but we somehow still apply a +`Filter` that filters out 2,487,830 rows. Why is that? Well, let's look at how +we created the index: + +```sql +CREATE INDEX CONCURRENTLY twitter_test ON users (twitter); +``` + +We simply told PostgreSQL to index all possible values of the `twitter` column, +even empty strings. Our query in turn uses `WHERE twitter != ''`. This means +that the index does improve things, as we don't need to do a sequential scan, +but we may still encounter empty strings. This means PostgreSQL _has_ to apply a +Filter on the index results to get rid of those values. + +Fortunately, we can improve this even further using "partial indexes". Partial +indexes are indexes with a `WHERE` condition that is applied when indexing data. +For example: + +```sql +CREATE INDEX CONCURRENTLY some_index ON users (email) WHERE id < 100 +``` + +This index would only index the `email` value of rows that match `WHERE id < +100`. We can use partial indexes to change our Twitter index to the following: + +```sql +CREATE INDEX CONCURRENTLY twitter_test ON users (twitter) WHERE twitter != ''; +``` + +Once created, if we run our query again we will be given the following plan: + +``` +Aggregate (cost=1608.26..1608.27 rows=1 width=8) (actual time=19.821..19.821 rows=1 loops=1) + Buffers: shared hit=44036 + -> Index Only Scan using twitter_test on users (cost=0.41..1479.71 rows=51420 width=0) (actual time=0.023..15.514 rows=51833 loops=1) + Heap Fetches: 1208 + Buffers: shared hit=44036 +Planning time: 0.123 ms +Execution time: 19.848 ms +``` + +That's _a lot_ better! Now it only takes 20 milliseconds to get the data, and we +only use about 344 MB of buffers (instead of the original 1.58 GB). The reason +this works is that now PostgreSQL no longer needs to apply a `Filter`, as the +index only contains `twitter` values that are not empty. + +Keep in mind that you shouldn't just add partial indexes every time you want to +optimise a query. Every index has to be updated for every write, and they may +require quite a bit of space, depending on the amount of indexed data. As a +result, first check if there are any existing indexes you may be able to reuse. +If there aren't any, check if you can perhaps slightly change an existing one to +fit both the existing and new queries. Only add a new index if none of the +existing indexes can be used in any way. + +## Queries that can't be optimised + +Now that we have seen how to optimise a query, let's look at another query that +we might not be able to optimise: + +```sql +EXPLAIN (ANALYZE, BUFFERS) +SELECT COUNT(*) +FROM projects +WHERE visibility_level IN (0, 20); +``` + +The output of `EXPLAIN (ANALYZE, BUFFERS)` is as follows: + +``` +Aggregate (cost=922420.60..922420.61 rows=1 width=8) (actual time=3428.535..3428.535 rows=1 loops=1) + Buffers: shared hit=208846 + -> Seq Scan on projects (cost=0.00..908053.18 rows=5746969 width=0) (actual time=0.041..2987.606 rows=5746940 loops=1) + Filter: (visibility_level = ANY ('{0,20}'::integer[])) + Rows Removed by Filter: 65677 + Buffers: shared hit=208846 +Planning time: 2.861 ms +Execution time: 3428.596 ms +``` + +Looking at the output we see the following Filter: + +``` +Filter: (visibility_level = ANY ('{0,20}'::integer[])) +Rows Removed by Filter: 65677 +``` + +Looking at the number of rows removed by the filter, we may be tempted to add an +index on `projects.visibility_level` to somehow turn this Sequential scan + +filter into an index-only scan. + +Unfortunately, doing so is unlikely to improve anything. Contrary to what some +might believe, an index being present _does not guarantee_ that PostgreSQL will +actually use it. For example, when doing a `SELECT * FROM projects` it is much +cheaper to just scan the entire table, instead of using an index and then +fetching data from the table. In such cases PostgreSQL may decide to not use an +index. + +Second, let's think for a moment what our query does: it gets all projects with +visibility level 0 or 20. In the above plan we can see this produces quite a lot +of rows (5,745,940), but how much is that relative to the total? Let's find out +by running the following query: + +```sql +SELECT visibility_level, count(*) AS amount +FROM projects +GROUP BY visibility_level +ORDER BY visibility_level ASC; +``` + +For GitLab.com this produces: + +``` + visibility_level | amount +------------------+--------- + 0 | 5071325 + 10 | 65678 + 20 | 674801 +``` + +Here the total number of projects is 5,811,804, and 5,746,126 of those are of +level 0 or 20. That's 98% of the entire table! + +So no matter what we do, this query will retrieve 98% of the entire table. Since +most time is spent doing exactly that, there isn't really much we can do to +improve this query, other than _not_ running it at all. + +What is important here is that while some may recommend to straight up add an +index the moment you see a sequential scan, it is _much more important_ to first +understand what your query does, how much data it retrieves, and so on. After +all, you can not optimise something you do not understand. + +### Cardinality and selectivity + +Earlier we saw that our query had to retrieve 98% of the rows in the table. +There are two terms commonly used for databases: cardinality, and selectivity. +Cardinality refers to the number of unique values in a particular column in a +table. + +Selectivity is the number of unique values produced by an operation (e.g. an +index scan or filter), relative to the total number of rows. The higher the +selectivity, the more likely PostgreSQL is able to use an index. + +In the above example, there are only 3 unique values: 0, 10, and 20. This means +the cardinality is 3. The selectivity in turn is also very low: 0.0000003% (2 / +5,811,804), because our `Filter` only filters using two values (`0` and `20`). +With such a low selectivity value it's not surprising that PostgreSQL decides +using an index is not worth it, because it would produce almost no unique rows. + +## Rewriting queries + +So the above query can't really be optimised as-is, or at least not much. But +what if we slightly change the purpose of it? What if instead of retrieving all +projects with `visibility_level` 0 or 20, we retrieve those that a user +interacted with somehow? + +Fortunately, GitLab has an answer for this, and it's a table called +`user_interacted_projects`. This table has the following schema: + +``` +Table "public.user_interacted_projects" + Column | Type | Modifiers +------------+---------+----------- + user_id | integer | not null + project_id | integer | not null +Indexes: + "index_user_interacted_projects_on_project_id_and_user_id" UNIQUE, btree (project_id, user_id) + "index_user_interacted_projects_on_user_id" btree (user_id) +Foreign-key constraints: + "fk_rails_0894651f08" FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE + "fk_rails_722ceba4f7" FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE +``` + +Let's rewrite our query to JOIN this table onto our projects, and get the +projects for a specific user: + +```sql +EXPLAIN ANALYZE +SELECT COUNT(*) +FROM projects +INNER JOIN user_interacted_projects ON user_interacted_projects.project_id = projects.id +WHERE projects.visibility_level IN (0, 20) +AND user_interacted_projects.user_id = 1; +``` + +What we do here is the following: + +1. Get our projects. +1. INNER JOIN `user_interacted_projects`, meaning we're only left with rows in + `projects` that have a corresponding row in `user_interacted_projects`. +1. Limit this to the projects with `visibility_level` of 0 or 20, and to + projects that the user with ID 1 interacted with. + +If we run this query we get the following plan: + +``` + Aggregate (cost=871.03..871.04 rows=1 width=8) (actual time=9.763..9.763 rows=1 loops=1) + -> Nested Loop (cost=0.86..870.52 rows=203 width=0) (actual time=1.072..9.748 rows=143 loops=1) + -> Index Scan using index_user_interacted_projects_on_user_id on user_interacted_projects (cost=0.43..160.71 rows=205 width=4) (actual time=0.939..2.508 rows=145 loops=1) + Index Cond: (user_id = 1) + -> Index Scan using projects_pkey on projects (cost=0.43..3.45 rows=1 width=4) (actual time=0.049..0.050 rows=1 loops=145) + Index Cond: (id = user_interacted_projects.project_id) + Filter: (visibility_level = ANY ('{0,20}'::integer[])) + Rows Removed by Filter: 0 + Planning time: 2.614 ms + Execution time: 9.809 ms +``` + +Here it only took us just under 10 milliseconds to get the data. We can also see +we're retrieving far fewer projects: + +``` +Index Scan using projects_pkey on projects (cost=0.43..3.45 rows=1 width=4) (actual time=0.049..0.050 rows=1 loops=145) + Index Cond: (id = user_interacted_projects.project_id) + Filter: (visibility_level = ANY ('{0,20}'::integer[])) + Rows Removed by Filter: 0 +``` + +Here we see we perform 145 loops (`loops=145`), with every loop producing 1 row +(`rows=1`). This is much less than before, and our query performs much better! + +If we look at the plan we also see our costs are very low: + +``` +Index Scan using projects_pkey on projects (cost=0.43..3.45 rows=1 width=4) (actual time=0.049..0.050 rows=1 loops=145) +``` + +Here our cost is only 3.45, and it only takes us 0.050 milliseconds to do so. +The next index scan is a bit more expensive: + +``` +Index Scan using index_user_interacted_projects_on_user_id on user_interacted_projects (cost=0.43..160.71 rows=205 width=4) (actual time=0.939..2.508 rows=145 loops=1) +``` + +Here the cost is 160.71 (`cost=0.43..160.71`), taking about 2.5 milliseconds +(based on the output of `actual time=....`). + +The most expensive part here is the "Nested Loop" that acts upon the result of +these two index scans: + +``` +Nested Loop (cost=0.86..870.52 rows=203 width=0) (actual time=1.072..9.748 rows=143 loops=1) +``` + +Here we had to perform 870.52 disk page fetches for 203 rows, 9.748 +milliseconds, producing 143 rows in a single loop. + +The key takeaway here is that sometimes you have to rewrite (parts of) a query +to make it better. Sometimes that means having to slightly change your feature +to accommodate for better performance. + +## What makes a bad plan + +This is a bit of a difficult question to answer, because the definition of "bad" +is relative to the problem you are trying to solve. However, some patterns are +best avoided in most cases, such as: + +* Sequential scans on large tables +* Filters that remove a lot of rows +* Performing a certain step (e.g. an index scan) that requires _a lot_ of + buffers (e.g. more than 512 MB for GitLab.com). + +As a general guideline, aim for a query that: + +1. Takes no more than 10 milliseconds. Our target time spent in SQL per request + is around 100 milliseconds, so every query should be as fast as possible. +1. Does not use an excessive number of buffers, relative to the workload. For + example, retrieving ten rows shouldn't require 1 GB of buffers. +1. Does not spend a long amount of time performing disk IO operations. The + setting `track_io_timing` must be enabled for this data to be included in the + output of `EXPLAIN ANALYZE`. +1. Applies a `LIMIT` when retrieving rows without aggregating them, such as + `SELECT * FROM users`. +1. Doesn't use a `Filter` to filter out too many rows, especially if the query + does not use a `LIMIT` to limit the number of returned rows. Filters can + usually be removed by adding a (partial) index. + +These are _guidelines_ and not hard requirements, as different needs may require +different queries. The only _rule_ is that you _must always measure_ your query +(preferably using a production-like database) using `EXPLAIN (ANALYZE, BUFFERS)` +and related tools such as: + +* <https://explain.depesz.com/> +* <http://tatiyants.com/postgres-query-plan-visualization/> + +GitLab employees can also use our chatops solution, available in Slack using the +`/chatops` slash command. You can use chatops to get a query plan by running the +following: + +``` +/chatops run explain SELECT COUNT(*) FROM projects WHERE visibility_level IN (0, 20) +``` + +Visualising the plan using <https://explain.depesz.com/> is also supported: + +``` +/chatops run explain --visual SELECT COUNT(*) FROM projects WHERE visibility_level IN (0, 20) +``` + +Quoting the query is not necessary. + +For more information about the available options, run: + +``` +/chatops run explain --help +``` diff --git a/doc/install/kubernetes/gitlab_omnibus.md b/doc/install/kubernetes/gitlab_omnibus.md index 9aee6b9dc74..c2c8a7a92fd 100644 --- a/doc/install/kubernetes/gitlab_omnibus.md +++ b/doc/install/kubernetes/gitlab_omnibus.md @@ -120,7 +120,7 @@ gitlabConfigStorageSize: 1Gi Ingress routing and SSL are automatically configured within this Chart. An NGINX ingress is provisioned and configured, and will route traffic to any service. SSL certificates are automatically created and configured by [kube-lego](https://github.com/kubernetes/charts/tree/master/stable/kube-lego). > **Note:** -Let's Encrypt limits a single TLD to five certificate requests within a single week. This means that common DNS wildcard services like [nip.io](http://nip.io) and [nip.io](http://nip.io) are unlikely to work. +Let's Encrypt limits a single TLD to five certificate requests within a single week. This means that common DNS wildcard services like [nip.io](http://nip.io) are unlikely to work. ## Installing GitLab using the Helm Chart > **Note:** diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md index 77fa517b5b1..770b1810da1 100644 --- a/doc/user/project/integrations/webhooks.md +++ b/doc/user/project/integrations/webhooks.md @@ -1102,6 +1102,7 @@ X-Gitlab-Event: Build Hook "build_finished_at": null, "build_duration": null, "build_allow_failure": false, + "build_failure_reason": "script_failure", "project_id": 380, "project_name": "gitlab-org/gitlab-test", "user": { diff --git a/lib/gitlab/data_builder/build.rb b/lib/gitlab/data_builder/build.rb index 2f1445a050a..0b71b31a476 100644 --- a/lib/gitlab/data_builder/build.rb +++ b/lib/gitlab/data_builder/build.rb @@ -28,6 +28,7 @@ module Gitlab build_finished_at: build.finished_at, build_duration: build.duration, build_allow_failure: build.allow_failure, + build_failure_reason: build.failure_reason, # TODO: do we still need it? project_id: project.id, diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index b58296375ef..61ce10ca131 100644 --- a/lib/gitlab/git/diff.rb +++ b/lib/gitlab/git/diff.rb @@ -226,6 +226,7 @@ module Gitlab @new_file = diff.from_id == BLANK_SHA @renamed_file = diff.from_path != diff.to_path @deleted_file = diff.to_id == BLANK_SHA + @too_large = diff.too_large if diff.respond_to?(:too_large) collapse! if diff.respond_to?(:collapsed) && diff.collapsed end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index e9c901f8592..9521a2d63a0 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -543,14 +543,8 @@ module Gitlab end def update_branch(branch_name, user:, newrev:, oldrev:) - gitaly_migrate(:operation_user_update_branch) do |is_enabled| - if is_enabled - gitaly_operation_client.user_update_branch(branch_name, user, newrev, oldrev) - else - Gitlab::GitalyClient::StorageSettings.allow_disk_access do - OperationService.new(user, self).update_branch(branch_name, newrev, oldrev) - end - end + wrapped_gitaly_errors do + gitaly_operation_client.user_update_branch(branch_name, user, newrev, oldrev) end end diff --git a/lib/gitlab/gitaly_client/diff.rb b/lib/gitlab/gitaly_client/diff.rb index d98a0ce988f..af9d674535b 100644 --- a/lib/gitlab/gitaly_client/diff.rb +++ b/lib/gitlab/gitaly_client/diff.rb @@ -1,7 +1,7 @@ module Gitlab module GitalyClient class Diff - ATTRS = %i(from_path to_path old_mode new_mode from_id to_id patch overflow_marker collapsed).freeze + ATTRS = %i(from_path to_path old_mode new_mode from_id to_id patch overflow_marker collapsed too_large).freeze include AttributesBag end diff --git a/lib/gitlab/github_import/bulk_importing.rb b/lib/gitlab/github_import/bulk_importing.rb index 147597289cf..da2f96b5c4b 100644 --- a/lib/gitlab/github_import/bulk_importing.rb +++ b/lib/gitlab/github_import/bulk_importing.rb @@ -15,10 +15,12 @@ module Gitlab end # Bulk inserts the given rows into the database. - def bulk_insert(model, rows, batch_size: 100) + def bulk_insert(model, rows, batch_size: 100, pre_hook: nil) rows.each_slice(batch_size) do |slice| + pre_hook.call(slice) if pre_hook Gitlab::Database.bulk_insert(model.table_name, slice) end + rows end end end diff --git a/lib/gitlab/github_import/importer/diff_note_importer.rb b/lib/gitlab/github_import/importer/diff_note_importer.rb index 8274f37d358..d562958e955 100644 --- a/lib/gitlab/github_import/importer/diff_note_importer.rb +++ b/lib/gitlab/github_import/importer/diff_note_importer.rb @@ -13,7 +13,7 @@ module Gitlab @note = note @project = project @client = client - @user_finder = UserFinder.new(project, client) + @user_finder = GithubImport::UserFinder.new(project, client) end def execute diff --git a/lib/gitlab/github_import/importer/issue_importer.rb b/lib/gitlab/github_import/importer/issue_importer.rb index 31fefebf787..cb4d7a6a0b6 100644 --- a/lib/gitlab/github_import/importer/issue_importer.rb +++ b/lib/gitlab/github_import/importer/issue_importer.rb @@ -19,7 +19,7 @@ module Gitlab @issue = issue @project = project @client = client - @user_finder = UserFinder.new(project, client) + @user_finder = GithubImport::UserFinder.new(project, client) @milestone_finder = MilestoneFinder.new(project) @issuable_finder = GithubImport::IssuableFinder.new(project, issue) end @@ -55,7 +55,11 @@ module Gitlab updated_at: issue.updated_at } - GithubImport.insert_and_return_id(attributes, project.issues) + GithubImport.insert_and_return_id(attributes, project.issues).tap do |id| + # We use .insert_and_return_id which effectively disables all callbacks. + # Trigger iid logic here to make sure we track internal id values consistently. + project.issues.find(id).ensure_project_iid! + end rescue ActiveRecord::InvalidForeignKey # It's possible the project has been deleted since scheduling this # job. In this case we'll just skip creating the issue. diff --git a/lib/gitlab/github_import/importer/milestones_importer.rb b/lib/gitlab/github_import/importer/milestones_importer.rb index c53480e828a..94eb9136b9a 100644 --- a/lib/gitlab/github_import/importer/milestones_importer.rb +++ b/lib/gitlab/github_import/importer/milestones_importer.rb @@ -17,10 +17,20 @@ module Gitlab end def execute - bulk_insert(Milestone, build_milestones) + # We insert records in bulk, by-passing any standard model callbacks. + # The pre_hook here makes sure we track internal ids consistently. + # Note this has to be called before performing an insert of a batch + # because we're outside a transaction scope here. + bulk_insert(Milestone, build_milestones, pre_hook: method(:track_greatest_iid)) build_milestones_cache end + def track_greatest_iid(slice) + greatest_iid = slice.max { |e| e[:iid] }[:iid] + + InternalId.track_greatest(nil, { project: project }, :milestones, greatest_iid, ->(_) { project.milestones.maximum(:iid) }) + end + def build_milestones build_database_rows(each_milestone) end diff --git a/lib/gitlab/github_import/importer/note_importer.rb b/lib/gitlab/github_import/importer/note_importer.rb index c890f2df360..2b06d1b3baf 100644 --- a/lib/gitlab/github_import/importer/note_importer.rb +++ b/lib/gitlab/github_import/importer/note_importer.rb @@ -13,7 +13,7 @@ module Gitlab @note = note @project = project @client = client - @user_finder = UserFinder.new(project, client) + @user_finder = GithubImport::UserFinder.new(project, client) end def execute diff --git a/lib/gitlab/github_import/importer/pull_request_importer.rb b/lib/gitlab/github_import/importer/pull_request_importer.rb index 6b3688c4381..ed17aa54373 100644 --- a/lib/gitlab/github_import/importer/pull_request_importer.rb +++ b/lib/gitlab/github_import/importer/pull_request_importer.rb @@ -15,7 +15,7 @@ module Gitlab @pull_request = pull_request @project = project @client = client - @user_finder = UserFinder.new(project, client) + @user_finder = GithubImport::UserFinder.new(project, client) @milestone_finder = MilestoneFinder.new(project) @issuable_finder = GithubImport::IssuableFinder.new(project, pull_request) @@ -76,7 +76,13 @@ module Gitlab merge_request_id = GithubImport .insert_and_return_id(attributes, project.merge_requests) - [project.merge_requests.find(merge_request_id), false] + merge_request = project.merge_requests.find(merge_request_id) + + # We use .insert_and_return_id which effectively disables all callbacks. + # Trigger iid logic here to make sure we track internal id values consistently. + merge_request.ensure_target_project_iid! + + [merge_request, false] end rescue ActiveRecord::InvalidForeignKey # It's possible the project has been deleted since scheduling this diff --git a/lib/gitlab/import_export/members_mapper.rb b/lib/gitlab/import_export/members_mapper.rb index ac827cbe1ca..bcbaf00e11b 100644 --- a/lib/gitlab/import_export/members_mapper.rb +++ b/lib/gitlab/import_export/members_mapper.rb @@ -45,7 +45,7 @@ module Gitlab end def ensure_default_member! - @project.project_members.destroy_all + @project.project_members.destroy_all # rubocop: disable DestroyAll ProjectMember.create!(user: @user, access_level: ProjectMember::MAINTAINER, source_id: @project.id, importing: true) end diff --git a/lib/tasks/gitlab/site_statistics.rake b/lib/tasks/gitlab/site_statistics.rake new file mode 100644 index 00000000000..7d24ec72a9d --- /dev/null +++ b/lib/tasks/gitlab/site_statistics.rake @@ -0,0 +1,23 @@ +namespace :gitlab do + desc "GitLab | Refresh Site Statistics counters" + task refresh_site_statistics: :environment do + puts 'Updating Site Statistics counters: ' + + print '* Repositories... ' + SiteStatistic.transaction do + # see https://gitlab.com/gitlab-org/gitlab-ce/issues/48967 + ActiveRecord::Base.connection.execute('SET LOCAL statement_timeout TO 0') if Gitlab::Database.postgresql? + SiteStatistic.update_all('repositories_count = (SELECT COUNT(*) FROM projects)') + end + puts 'OK!'.color(:green) + + print '* Wikis... ' + SiteStatistic.transaction do + # see https://gitlab.com/gitlab-org/gitlab-ce/issues/48967 + ActiveRecord::Base.connection.execute('SET LOCAL statement_timeout TO 0') if Gitlab::Database.postgresql? + SiteStatistic.update_all('wikis_count = (SELECT COUNT(*) FROM project_features WHERE wiki_access_level != 0)') + end + puts 'OK!'.color(:green) + puts + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index a4dde120c57..73bff79aabe 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4110,6 +4110,9 @@ msgstr "" msgid "Pipeline" msgstr "" +msgid "Pipeline %{pipelineLinkStart} #%{pipelineId} %{pipelineLinkEnd} from %{pipelineLinkRefStart} %{pipelineRef} %{pipelineLinkRefEnd}" +msgstr "" + msgid "Pipeline Health" msgstr "" @@ -4796,6 +4799,9 @@ msgstr "" msgid "Retry verification" msgstr "" +msgid "Reveal Variables" +msgstr "" + msgid "Reveal value" msgid_plural "Reveal values" msgstr[0] "" @@ -5958,6 +5964,9 @@ msgstr "" msgid "Trending" msgstr "" +msgid "Trigger" +msgstr "" + msgid "Trigger this manual action" msgstr "" diff --git a/qa/qa/specs/features/project/auto_devops_spec.rb b/qa/qa/specs/features/project/auto_devops_spec.rb index c2c3bef98e4..b3e4889abc0 100644 --- a/qa/qa/specs/features/project/auto_devops_spec.rb +++ b/qa/qa/specs/features/project/auto_devops_spec.rb @@ -15,6 +15,13 @@ module QA p.description = 'Project with Auto Devops' end + # Disable code_quality check in Auto DevOps pipeline as it takes + # too long and times out the test + Factory::Resource::SecretVariable.fabricate! do |resource| + resource.key = 'CODE_QUALITY_DISABLED' + resource.value = '1' + end + # Create Auto Devops compatible repo Factory::Repository::ProjectPush.fabricate! do |push| push.project = project diff --git a/rubocop/cop/destroy_all.rb b/rubocop/cop/destroy_all.rb new file mode 100644 index 00000000000..38b6cb40f91 --- /dev/null +++ b/rubocop/cop/destroy_all.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Cop that blacklists the use of `destroy_all`. + class DestroyAll < RuboCop::Cop::Cop + MSG = 'Use `delete_all` instead of `destroy_all`. ' \ + '`destroy_all` will load the rows into memory, then execute a ' \ + '`DELETE` for every individual row.' + + def_node_matcher :destroy_all?, <<~PATTERN + (send {send ivar lvar const} :destroy_all ...) + PATTERN + + def on_send(node) + return unless destroy_all?(node) + + add_offense(node, location: :expression) + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index a427208cdab..88c9bbf24f4 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -27,3 +27,4 @@ require_relative 'cop/project_path_helper' require_relative 'cop/rspec/env_assignment' require_relative 'cop/rspec/factories_in_migration_specs' require_relative 'cop/sidekiq_options_queue' +require_relative 'cop/destroy_all' diff --git a/spec/controllers/autocomplete_controller_spec.rb b/spec/controllers/autocomplete_controller_spec.rb index 2c59d1929a1..883bb35f396 100644 --- a/spec/controllers/autocomplete_controller_spec.rb +++ b/spec/controllers/autocomplete_controller_spec.rb @@ -274,14 +274,11 @@ describe AutocompleteController do context 'authorized projects apply limit' do before do - authorized_project2 = create(:project) - authorized_project3 = create(:project) - - authorized_project.add_maintainer(user) - authorized_project2.add_maintainer(user) - authorized_project3.add_maintainer(user) + allow(Kaminari.config).to receive(:default_per_page).and_return(2) - stub_const 'MoveToProjectFinder::PAGE_SIZE', 2 + create_list(:project, 2) do |project| + project.add_maintainer(user) + end end describe 'GET #projects with project ID' do @@ -291,7 +288,7 @@ describe AutocompleteController do it 'returns projects' do expect(json_response).to be_kind_of(Array) - expect(json_response.size).to eq 2 # Of a total of 3 + expect(json_response.size).to eq(Kaminari.config.default_per_page) end end end diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index b23f183fec8..d377d69457f 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -95,7 +95,7 @@ describe OmniauthCallbacksController, type: :controller do end it 'allows linking the disabled provider' do - user.identities.destroy_all + user.identities.destroy_all # rubocop: disable DestroyAll sign_in(user) expect { post provider }.to change { user.reload.identities.count }.by(1) diff --git a/spec/controllers/projects/releases_controller_spec.rb b/spec/controllers/projects/releases_controller_spec.rb index fc1619acec6..20a6beb3df8 100644 --- a/spec/controllers/projects/releases_controller_spec.rb +++ b/spec/controllers/projects/releases_controller_spec.rb @@ -14,7 +14,7 @@ describe Projects::ReleasesController do describe 'GET #edit' do it 'initializes a new release' do tag_id = release.tag - project.releases.destroy_all + project.releases.destroy_all # rubocop: disable DestroyAll get :edit, namespace_id: project.namespace, project_id: project, tag_id: tag_id diff --git a/spec/features/admin/admin_runners_spec.rb b/spec/features/admin/admin_runners_spec.rb index be8754a5315..5623e47eadf 100644 --- a/spec/features/admin/admin_runners_spec.rb +++ b/spec/features/admin/admin_runners_spec.rb @@ -20,7 +20,7 @@ describe "Admin Runners" do it 'has all necessary texts' do expect(page).to have_text "Setup a shared Runner manually" - expect(page).to have_text "Runners with last contact more than a minute ago: 1" + expect(page).to have_text "Runners currently online: 1" end describe 'search' do @@ -55,7 +55,7 @@ describe "Admin Runners" do it 'has all necessary texts including no runner message' do expect(page).to have_text "Setup a shared Runner manually" - expect(page).to have_text "Runners with last contact more than a minute ago: 0" + expect(page).to have_text "Runners currently online: 0" expect(page).to have_text 'No runners found' end end diff --git a/spec/finders/autocomplete/group_finder_spec.rb b/spec/finders/autocomplete/group_finder_spec.rb new file mode 100644 index 00000000000..d7cb2c3bbe2 --- /dev/null +++ b/spec/finders/autocomplete/group_finder_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Autocomplete::GroupFinder do + let(:user) { create(:user) } + + describe '#execute' do + context 'with a project' do + it 'returns nil' do + project = create(:project) + + expect(described_class.new(user, project).execute).to be_nil + end + end + + context 'without a group ID' do + it 'returns nil' do + expect(described_class.new(user).execute).to be_nil + end + end + + context 'with an empty String as the group ID' do + it 'returns nil' do + expect(described_class.new(user, nil, group_id: '').execute).to be_nil + end + end + + context 'without a project and with a group ID' do + it 'raises ActiveRecord::RecordNotFound if the group does not exist' do + finder = described_class.new(user, nil, group_id: 1) + + expect { finder.execute }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'raises ActiveRecord::RecordNotFound if the user can not read the group' do + group = create(:group, :private) + finder = described_class.new(user, nil, group_id: group.id) + + expect { finder.execute }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'raises ActiveRecord::RecordNotFound if an anonymous user can not read the group' do + group = create(:group, :private) + finder = described_class.new(nil, nil, group_id: group.id) + + expect { finder.execute }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'returns the group if it exists and is readable' do + group = create(:group) + finder = described_class.new(user, nil, group_id: group.id) + + expect(finder.execute).to eq(group) + end + end + end +end diff --git a/spec/finders/move_to_project_finder_spec.rb b/spec/finders/autocomplete/move_to_project_finder_spec.rb index 1b3f44cced1..c3bc410a7f6 100644 --- a/spec/finders/move_to_project_finder_spec.rb +++ b/spec/finders/autocomplete/move_to_project_finder_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe MoveToProjectFinder do +describe Autocomplete::MoveToProjectFinder do let(:user) { create(:user) } let(:project) { create(:project) } @@ -10,14 +10,14 @@ describe MoveToProjectFinder do let(:developer_project) { create(:project) } let(:maintainer_project) { create(:project) } - subject { described_class.new(user) } - describe '#execute' do context 'filter' do it 'does not return projects under Gitlab::Access::REPORTER' do guest_project.add_guest(user) - expect(subject.execute(project)).to be_empty + finder = described_class.new(user, project_id: project.id) + + expect(finder.execute).to be_empty end it 'returns projects equal or above Gitlab::Access::REPORTER ordered by id in descending order' do @@ -25,13 +25,17 @@ describe MoveToProjectFinder do developer_project.add_developer(user) maintainer_project.add_maintainer(user) - expect(subject.execute(project).to_a).to eq([maintainer_project, developer_project, reporter_project]) + finder = described_class.new(user, project_id: project.id) + + expect(finder.execute.to_a).to eq([maintainer_project, developer_project, reporter_project]) end it 'does not include the source project' do project.add_reporter(user) - expect(subject.execute(project).to_a).to be_empty + finder = described_class.new(user, project_id: project.id) + + expect(finder.execute.to_a).to be_empty end it 'does not return archived projects' do @@ -40,7 +44,9 @@ describe MoveToProjectFinder do other_reporter_project = create(:project) other_reporter_project.add_reporter(user) - expect(subject.execute(project).to_a).to eq([other_reporter_project]) + finder = described_class.new(user, project_id: project.id) + + expect(finder.execute.to_a).to eq([other_reporter_project]) end it 'does not return projects for which issues are disabled' do @@ -49,39 +55,42 @@ describe MoveToProjectFinder do other_reporter_project = create(:project) other_reporter_project.add_reporter(user) - expect(subject.execute(project).to_a).to eq([other_reporter_project]) + finder = described_class.new(user, project_id: project.id) + + expect(finder.execute.to_a).to eq([other_reporter_project]) end it 'returns a page of projects ordered by id in descending order' do - stub_const 'MoveToProjectFinder::PAGE_SIZE', 2 + allow(Kaminari.config).to receive(:default_per_page).and_return(2) - reporter_project.add_reporter(user) - developer_project.add_developer(user) - maintainer_project.add_maintainer(user) + projects = create_list(:project, 2) do |project| + project.add_developer(user) + end - expect(subject.execute(project).to_a).to eq([maintainer_project, developer_project]) + finder = described_class.new(user, project_id: project.id) + page = finder.execute.to_a + + expect(page.length).to eq(Kaminari.config.default_per_page) + expect(page[0]).to eq(projects.last) end it 'returns projects after the given offset id' do - stub_const 'MoveToProjectFinder::PAGE_SIZE', 2 - reporter_project.add_reporter(user) developer_project.add_developer(user) maintainer_project.add_maintainer(user) - expect(subject.execute(project, search: nil, offset_id: maintainer_project.id).to_a).to eq([developer_project, reporter_project]) - expect(subject.execute(project, search: nil, offset_id: developer_project.id).to_a).to eq([reporter_project]) - expect(subject.execute(project, search: nil, offset_id: reporter_project.id).to_a).to be_empty - end - end + expect(described_class.new(user, project_id: project.id, offset_id: maintainer_project.id).execute.to_a) + .to eq([developer_project, reporter_project]) - context 'search' do - it 'uses Project#search' do - expect(user).to receive_message_chain(:projects_where_can_admin_issues, :search) { Project.all } + expect(described_class.new(user, project_id: project.id, offset_id: developer_project.id).execute.to_a) + .to eq([reporter_project]) - subject.execute(project, search: 'wadus') + expect(described_class.new(user, project_id: project.id, offset_id: reporter_project.id).execute.to_a) + .to be_empty end + end + context 'search' do it 'returns projects matching a search query' do foo_project = create(:project) foo_project.add_maintainer(user) @@ -89,8 +98,11 @@ describe MoveToProjectFinder do wadus_project = create(:project, name: 'wadus') wadus_project.add_maintainer(user) - expect(subject.execute(project).to_a).to eq([wadus_project, foo_project]) - expect(subject.execute(project, search: 'wadus').to_a).to eq([wadus_project]) + expect(described_class.new(user, project_id: project.id).execute.to_a) + .to eq([wadus_project, foo_project]) + + expect(described_class.new(user, project_id: project.id, search: 'wadus').execute.to_a) + .to eq([wadus_project]) end end end diff --git a/spec/finders/autocomplete/project_finder_spec.rb b/spec/finders/autocomplete/project_finder_spec.rb new file mode 100644 index 00000000000..207d0598c28 --- /dev/null +++ b/spec/finders/autocomplete/project_finder_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Autocomplete::ProjectFinder do + let(:user) { create(:user) } + + describe '#execute' do + context 'without a project ID' do + it 'returns nil' do + expect(described_class.new(user).execute).to be_nil + end + end + + context 'with an empty String as the project ID' do + it 'returns nil' do + expect(described_class.new(user, project_id: '').execute).to be_nil + end + end + + context 'with a project ID' do + it 'raises ActiveRecord::RecordNotFound if the project does not exist' do + finder = described_class.new(user, project_id: 1) + + expect { finder.execute }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'raises ActiveRecord::RecordNotFound if the user can not read the project' do + project = create(:project, :private) + + finder = described_class.new(user, project_id: project.id) + + expect { finder.execute }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'raises ActiveRecord::RecordNotFound if an anonymous user can not read the project' do + project = create(:project, :private) + + finder = described_class.new(nil, project_id: project.id) + + expect { finder.execute }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'returns the project if it exists and is readable' do + project = create(:project, :private) + + project.add_maintainer(user) + + finder = described_class.new(user, project_id: project.id) + + expect(finder.execute).to eq(project) + end + end + end +end diff --git a/spec/finders/autocomplete_users_finder_spec.rb b/spec/finders/autocomplete/users_finder_spec.rb index dcf9111776e..abd0d6b5185 100644 --- a/spec/finders/autocomplete_users_finder_spec.rb +++ b/spec/finders/autocomplete/users_finder_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe AutocompleteUsersFinder do +describe Autocomplete::UsersFinder do describe '#execute' do let!(:user1) { create(:user, username: 'johndoe') } let!(:user2) { create(:user, :blocked, username: 'notsorandom') } diff --git a/spec/finders/awarded_emoji_finder_spec.rb b/spec/finders/awarded_emoji_finder_spec.rb new file mode 100644 index 00000000000..d4479df7418 --- /dev/null +++ b/spec/finders/awarded_emoji_finder_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AwardedEmojiFinder do + describe '#execute' do + it 'returns an Array containing the awarded emoji names' do + user = create(:user) + + create(:award_emoji, user: user, name: 'thumbsup') + create(:award_emoji, user: user, name: 'thumbsup') + create(:award_emoji, user: user, name: 'thumbsdown') + + awarded = described_class.new(user).execute + + expect(awarded).to eq([{ name: 'thumbsup' }, { name: 'thumbsdown' }]) + end + + it 'returns an empty Array when no user is given' do + awarded = described_class.new.execute + + expect(awarded).to be_empty + end + end +end diff --git a/spec/finders/user_finder_spec.rb b/spec/finders/user_finder_spec.rb new file mode 100644 index 00000000000..e53aa50dd33 --- /dev/null +++ b/spec/finders/user_finder_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe UserFinder do + describe '#execute' do + context 'when the user exists' do + it 'returns the user' do + user = create(:user) + found = described_class.new(id: user.id).execute + + expect(found).to eq(user) + end + end + + context 'when the user does not exist' do + it 'returns nil' do + found = described_class.new(id: 1).execute + + expect(found).to be_nil + end + end + end + + describe '#execute!' do + context 'when the user exists' do + it 'returns the user' do + user = create(:user) + found = described_class.new(id: user.id).execute! + + expect(found).to eq(user) + end + end + + context 'when the user does not exist' do + it 'raises ActiveRecord::RecordNotFound' do + finder = described_class.new(id: 1) + + expect { finder.execute! }.to raise_error(ActiveRecord::RecordNotFound) + end + end + end +end diff --git a/spec/javascripts/ide/stores/actions/file_spec.js b/spec/javascripts/ide/stores/actions/file_spec.js index 72eb20bdc87..bca2033ff97 100644 --- a/spec/javascripts/ide/stores/actions/file_spec.js +++ b/spec/javascripts/ide/stores/actions/file_spec.js @@ -352,10 +352,22 @@ describe('IDE store file actions', () => { it('calls also getBaseRawFileData service method', done => { spyOn(service, 'getBaseRawFileData').and.returnValue(Promise.resolve('baseraw')); + store.state.currentProjectId = 'gitlab-org/gitlab-ce'; + store.state.currentMergeRequestId = '1'; + store.state.projects = { + 'gitlab-org/gitlab-ce': { + mergeRequests: { + 1: { + baseCommitSha: 'SHA', + }, + }, + }, + }; + tmpFile.mrChange = { new_file: false }; store - .dispatch('getRawFileData', { path: tmpFile.path, baseSha: 'SHA' }) + .dispatch('getRawFileData', { path: tmpFile.path }) .then(() => { expect(service.getBaseRawFileData).toHaveBeenCalledWith(tmpFile, 'SHA'); expect(tmpFile.baseRaw).toBe('baseraw'); @@ -392,10 +404,7 @@ describe('IDE store file actions', () => { const dispatch = jasmine.createSpy('dispatch'); actions - .getRawFileData( - { state: store.state, commit() {}, dispatch }, - { path: tmpFile.path, baseSha: tmpFile.baseSha }, - ) + .getRawFileData({ state: store.state, commit() {}, dispatch }, { path: tmpFile.path }) .then(done.fail) .catch(() => { expect(dispatch).toHaveBeenCalledWith('setErrorMessage', { @@ -404,7 +413,6 @@ describe('IDE store file actions', () => { actionText: 'Please try again', actionPayload: { path: tmpFile.path, - baseSha: tmpFile.baseSha, }, }); diff --git a/spec/javascripts/jobs/commit_block_spec.js b/spec/javascripts/jobs/commit_block_spec.js new file mode 100644 index 00000000000..f755a5042b5 --- /dev/null +++ b/spec/javascripts/jobs/commit_block_spec.js @@ -0,0 +1,73 @@ +import Vue from 'vue'; +import component from '~/jobs/components/commit_block.vue'; +import mountComponent from '../helpers/vue_mount_component_helper'; + +describe('Commit block', () => { + const Component = Vue.extend(component); + let vm; + + const props = { + pipelineShortSha: '1f0fb84f', + pipelineShaPath: 'commit/1f0fb84fb6770d74d97eee58118fd3909cd4f48c', + mergeRequestReference: '!21244', + mergeRequestPath: 'merge_requests/21244', + gitCommitTitlte: 'Regenerate pot files', + }; + + afterEach(() => { + vm.$destroy(); + }); + + describe('pipeline short sha', () => { + beforeEach(() => { + vm = mountComponent(Component, { + ...props, + }); + }); + + it('renders pipeline short sha link', () => { + expect(vm.$el.querySelector('.js-commit-sha').getAttribute('href')).toEqual(props.pipelineShaPath); + expect(vm.$el.querySelector('.js-commit-sha').textContent.trim()).toEqual(props.pipelineShortSha); + }); + + it('renders clipboard button', () => { + expect(vm.$el.querySelector('button').getAttribute('data-clipboard-text')).toEqual(props.pipelineShortSha); + }); + }); + + describe('with merge request', () => { + it('renders merge request link and reference', () => { + vm = mountComponent(Component, { + ...props, + }); + + expect(vm.$el.querySelector('.js-link-commit').getAttribute('href')).toEqual(props.mergeRequestPath); + expect(vm.$el.querySelector('.js-link-commit').textContent.trim()).toEqual(props.mergeRequestReference); + + }); + }); + + describe('without merge request', () => { + it('does not render merge request', () => { + const copyProps = Object.assign({}, props); + delete copyProps.mergeRequestPath; + delete copyProps.mergeRequestReference; + + vm = mountComponent(Component, { + ...copyProps, + }); + + expect(vm.$el.querySelector('.js-link-commit')).toBeNull(); + }); + }); + + describe('git commit title', () => { + it('renders git commit title', () => { + vm = mountComponent(Component, { + ...props, + }); + + expect(vm.$el.textContent).toContain(props.gitCommitTitlte); + }); + }); +}); diff --git a/spec/javascripts/jobs/jobs_container_spec.js b/spec/javascripts/jobs/jobs_container_spec.js new file mode 100644 index 00000000000..bf52e65cbc8 --- /dev/null +++ b/spec/javascripts/jobs/jobs_container_spec.js @@ -0,0 +1,126 @@ +import Vue from 'vue'; +import component from '~/jobs/components/jobs_container.vue'; +import mountComponent from '../helpers/vue_mount_component_helper'; + +describe('Artifacts block', () => { + const Component = Vue.extend(component); + let vm; + + const retried = { + status: { + details_path: '/gitlab-org/gitlab-ce/pipelines/28029444', + group: 'success', + has_details: true, + icon: 'status_success', + label: 'passed', + text: 'passed', + tooltip: 'passed', + }, + path: 'job/233432756', + id: '233432756', + tooltip: 'build - passed', + retried: true, + }; + + const active = { + name: 'test', + status: { + details_path: '/gitlab-org/gitlab-ce/pipelines/28029444', + group: 'success', + has_details: true, + icon: 'status_success', + label: 'passed', + text: 'passed', + tooltip: 'passed', + }, + path: 'job/2322756', + id: '2322756', + tooltip: 'build - passed', + active: true, + }; + + const job = { + name: 'build', + status: { + details_path: '/gitlab-org/gitlab-ce/pipelines/28029444', + group: 'success', + has_details: true, + icon: 'status_success', + label: 'passed', + text: 'passed', + tooltip: 'passed', + }, + path: 'job/232153', + id: '232153', + tooltip: 'build - passed', + }; + + afterEach(() => { + vm.$destroy(); + }); + + it('renders list of jobs', () => { + vm = mountComponent(Component, { + jobs: [job, retried, active], + }); + + expect(vm.$el.querySelectorAll('a').length).toEqual(3); + }); + + it('renders arrow right when job is active', () => { + vm = mountComponent(Component, { + jobs: [active], + }); + + expect(vm.$el.querySelector('a .js-arrow-right')).not.toBeNull(); + }); + + it('does not render arrow right when job is not active', () => { + vm = mountComponent(Component, { + jobs: [job], + }); + + expect(vm.$el.querySelector('a .js-arrow-right')).toBeNull(); + }); + + it('renders job name when present', () => { + vm = mountComponent(Component, { + jobs: [job], + }); + + expect(vm.$el.querySelector('a').textContent.trim()).toContain(job.name); + expect(vm.$el.querySelector('a').textContent.trim()).not.toContain(job.id); + }); + + it('renders job id when job name is not available', () => { + vm = mountComponent(Component, { + jobs: [retried], + }); + + expect(vm.$el.querySelector('a').textContent.trim()).toContain(retried.id); + }); + + it('links to the job page', () => { + vm = mountComponent(Component, { + jobs: [job], + }); + + expect(vm.$el.querySelector('a').getAttribute('href')).toEqual(job.path); + }); + + it('renders retry icon when job was retried', () => { + vm = mountComponent(Component, { + jobs: [retried], + }); + + expect(vm.$el.querySelector('.js-retry-icon')).not.toBeNull(); + }); + + it('does not render retry icon when job was not retried', () => { + vm = mountComponent(Component, { + jobs: [job], + }); + + expect(vm.$el.querySelector('.js-retry-icon')).toBeNull(); + }); +}); diff --git a/spec/javascripts/jobs/stages_dropdown_spec.js b/spec/javascripts/jobs/stages_dropdown_spec.js new file mode 100644 index 00000000000..d3a5d48f56c --- /dev/null +++ b/spec/javascripts/jobs/stages_dropdown_spec.js @@ -0,0 +1,63 @@ +import Vue from 'vue'; +import component from '~/jobs/components/stages_dropdown.vue'; +import mountComponent from '../helpers/vue_mount_component_helper'; + +describe('Artifacts block', () => { + const Component = Vue.extend(component); + let vm; + + beforeEach(() => { + vm = mountComponent(Component, { + pipelineId: 28029444, + pipelinePath: 'pipeline/28029444', + pipelineRef: '50101-truncated-job-information', + pipelineRefPath: 'commits/50101-truncated-job-information', + stages: [ + { + name: 'build', + }, + { + name: 'test', + }, + ], + pipelineStatus: { + details_path: '/gitlab-org/gitlab-ce/pipelines/28029444', + group: 'success', + has_details: true, + icon: 'status_success', + label: 'passed', + text: 'passed', + tooltip: 'passed', + }, + }); + }); + + afterEach(() => { + vm.$destroy(); + }); + + it('renders pipeline status', () => { + expect(vm.$el.querySelector('.js-ci-status-icon-success')).not.toBeNull(); + }); + + it('renders pipeline link', () => { + expect(vm.$el.querySelector('.js-pipeline-path').getAttribute('href')).toEqual( + 'pipeline/28029444', + ); + }); + + it('renders dropdown with stages', () => { + expect(vm.$el.querySelector('.dropdown button').textContent).toContain('build'); + }); + + it('updates selected stage on click', done => { + vm.$el.querySelectorAll('.stage-item')[1].click(); + vm + .$nextTick() + .then(() => { + expect(vm.$el.querySelector('.dropdown button').textContent).toContain('test'); + }) + .then(done) + .catch(done.fail); + }); +}); diff --git a/spec/javascripts/jobs/trigger_value_spec.js b/spec/javascripts/jobs/trigger_value_spec.js new file mode 100644 index 00000000000..acf91510ed2 --- /dev/null +++ b/spec/javascripts/jobs/trigger_value_spec.js @@ -0,0 +1,66 @@ +import Vue from 'vue'; +import component from '~/jobs/components/trigger_block.vue'; +import mountComponent from '../helpers/vue_mount_component_helper'; + +describe('Trigger block', () => { + const Component = Vue.extend(component); + let vm; + + afterEach(() => { + vm.$destroy(); + }); + + describe('with short token', () => { + it('renders short token', () => { + vm = mountComponent(Component, { + shortToken: '0a666b2', + }); + + expect(vm.$el.querySelector('.js-short-token').textContent).toContain('0a666b2'); + }); + }); + + describe('without short token', () => { + it('does not render short token', () => { + vm = mountComponent(Component, {}); + + expect(vm.$el.querySelector('.js-short-token')).toBeNull(); + }); + }); + + describe('with variables', () => { + describe('reveal variables', () => { + it('reveals variables on click', done => { + vm = mountComponent(Component, { + variables: { + key: 'value', + variable: 'foo', + }, + }); + + vm.$el.querySelector('.js-reveal-variables').click(); + + vm + .$nextTick() + .then(() => { + expect(vm.$el.querySelector('.js-build-variables')).not.toBeNull(); + expect(vm.$el.querySelector('.js-build-variables').textContent).toContain('key'); + expect(vm.$el.querySelector('.js-build-variables').textContent).toContain('value'); + expect(vm.$el.querySelector('.js-build-variables').textContent).toContain('variable'); + expect(vm.$el.querySelector('.js-build-variables').textContent).toContain('foo'); + }) + .then(done) + .catch(done.fail); + }); + }); + }); + + describe('without variables', () => { + it('does not render variables', () => { + vm = mountComponent(Component); + + expect(vm.$el.querySelector('.js-reveal-variables')).toBeNull(); + expect(vm.$el.querySelector('.js-build-variables')).toBeNull(); + }); + }); +}); diff --git a/spec/lib/gitlab/background_migration/create_gpg_key_subkeys_from_gpg_keys_spec.rb b/spec/lib/gitlab/background_migration/create_gpg_key_subkeys_from_gpg_keys_spec.rb index 26d48cc8201..f92acf61682 100644 --- a/spec/lib/gitlab/background_migration/create_gpg_key_subkeys_from_gpg_keys_spec.rb +++ b/spec/lib/gitlab/background_migration/create_gpg_key_subkeys_from_gpg_keys_spec.rb @@ -5,7 +5,7 @@ describe Gitlab::BackgroundMigration::CreateGpgKeySubkeysFromGpgKeys, :migration let!(:gpg_key) { create(:gpg_key, key: GpgHelpers::User3.public_key) } before do - GpgKeySubkey.destroy_all + GpgKeySubkey.destroy_all # rubocop: disable DestroyAll end it 'generate the subkeys' do diff --git a/spec/lib/gitlab/bare_repository_import/importer_spec.rb b/spec/lib/gitlab/bare_repository_import/importer_spec.rb index 6e21c846c0a..3c63e601abc 100644 --- a/spec/lib/gitlab/bare_repository_import/importer_spec.rb +++ b/spec/lib/gitlab/bare_repository_import/importer_spec.rb @@ -10,9 +10,6 @@ describe Gitlab::BareRepositoryImport::Importer, :seed_helper do subject(:importer) { described_class.new(admin, bare_repository) } before do - @rainbow = Rainbow.enabled - Rainbow.enabled = false - allow(described_class).to receive(:log) end @@ -20,7 +17,6 @@ describe Gitlab::BareRepositoryImport::Importer, :seed_helper do FileUtils.rm_rf(base_dir) TestEnv.clean_test_path ensure_seeds - Rainbow.enabled = @rainbow end shared_examples 'importing a repository' do diff --git a/spec/lib/gitlab/data_builder/build_spec.rb b/spec/lib/gitlab/data_builder/build_spec.rb index ee91decafad..14fe196a986 100644 --- a/spec/lib/gitlab/data_builder/build_spec.rb +++ b/spec/lib/gitlab/data_builder/build_spec.rb @@ -15,6 +15,7 @@ describe Gitlab::DataBuilder::Build do it { expect(data[:build_id]).to eq(build.id) } it { expect(data[:build_status]).to eq(build.status) } it { expect(data[:build_allow_failure]).to eq(false) } + it { expect(data[:build_failure_reason]).to eq(build.failure_reason) } it { expect(data[:project_id]).to eq(build.project.id) } it { expect(data[:project_name]).to eq(build.project.full_name) } diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 35a6fc94753..17348b01006 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -542,7 +542,7 @@ describe Gitlab::Git::Repository, :seed_helper do Gitlab::Shell.new.remove_repository('default', 'my_project') end - shared_examples 'repository mirror fecthing' do + shared_examples 'repository mirror fetching' do it 'fetches a repository as a mirror remote' do subject @@ -569,11 +569,11 @@ describe Gitlab::Git::Repository, :seed_helper do end context 'with gitaly enabled' do - it_behaves_like 'repository mirror fecthing' + it_behaves_like 'repository mirror fetching' end context 'with gitaly enabled', :skip_gitaly_mock do - it_behaves_like 'repository mirror fecthing' + it_behaves_like 'repository mirror fetching' end def new_repository_path diff --git a/spec/lib/gitlab/gitaly_client/diff_spec.rb b/spec/lib/gitlab/gitaly_client/diff_spec.rb index 00a31ac0b96..ec7ab2fdedb 100644 --- a/spec/lib/gitlab/gitaly_client/diff_spec.rb +++ b/spec/lib/gitlab/gitaly_client/diff_spec.rb @@ -9,7 +9,9 @@ describe Gitlab::GitalyClient::Diff do new_mode: 0100644, from_id: '357406f3075a57708d0163752905cc1576fceacc', to_id: '8e5177d718c561d36efde08bad36b43687ee6bf0', - patch: 'a' * 100 + patch: 'a' * 100, + collapsed: false, + too_large: false } end @@ -22,6 +24,8 @@ describe Gitlab::GitalyClient::Diff do it { is_expected.to respond_to(:from_id) } it { is_expected.to respond_to(:to_id) } it { is_expected.to respond_to(:patch) } + it { is_expected.to respond_to(:collapsed) } + it { is_expected.to respond_to(:too_large) } describe '#==' do it { expect(subject).to eq(described_class.new(diff_fields)) } diff --git a/spec/lib/gitlab/github_import/bulk_importing_spec.rb b/spec/lib/gitlab/github_import/bulk_importing_spec.rb index 91229d9c7d4..861710f7e9b 100644 --- a/spec/lib/gitlab/github_import/bulk_importing_spec.rb +++ b/spec/lib/gitlab/github_import/bulk_importing_spec.rb @@ -58,5 +58,17 @@ describe Gitlab::GithubImport::BulkImporting do importer.bulk_insert(model, rows, batch_size: 5) end + + it 'calls pre_hook for each slice if given' do + rows = [{ title: 'Foo' }] * 10 + model = double(:model, table_name: 'kittens') + pre_hook = double('pre_hook', call: nil) + allow(Gitlab::Database).to receive(:bulk_insert) + + expect(pre_hook).to receive(:call).with(rows[0..4]) + expect(pre_hook).to receive(:call).with(rows[5..9]) + + importer.bulk_insert(model, rows, batch_size: 5, pre_hook: pre_hook) + end end end diff --git a/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb b/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb index 81fe97c1e49..3f7a12144d5 100644 --- a/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb @@ -78,6 +78,11 @@ describe Gitlab::GithubImport::Importer::IssueImporter, :clean_gitlab_redis_cach .to receive(:id_for) .with(issue) .and_return(milestone.id) + + allow(importer.user_finder) + .to receive(:author_id_for) + .with(issue) + .and_return([user.id, true]) end context 'when the issue author could be found' do @@ -172,6 +177,23 @@ describe Gitlab::GithubImport::Importer::IssueImporter, :clean_gitlab_redis_cach expect(importer.create_issue).to be_a_kind_of(Numeric) end + + it 'triggers internal_id functionality to track greatest iids' do + allow(importer.user_finder) + .to receive(:author_id_for) + .with(issue) + .and_return([user.id, true]) + + issue = build_stubbed(:issue, project: project) + allow(Gitlab::GithubImport) + .to receive(:insert_and_return_id) + .and_return(issue.id) + allow(project.issues).to receive(:find).with(issue.id).and_return(issue) + + expect(issue).to receive(:ensure_project_iid!) + + importer.create_issue + end end describe '#create_assignees' do diff --git a/spec/lib/gitlab/github_import/importer/milestones_importer_spec.rb b/spec/lib/gitlab/github_import/importer/milestones_importer_spec.rb index b1cac3b6e46..db0be760c7b 100644 --- a/spec/lib/gitlab/github_import/importer/milestones_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/milestones_importer_spec.rb @@ -29,13 +29,25 @@ describe Gitlab::GithubImport::Importer::MilestonesImporter, :clean_gitlab_redis expect(importer) .to receive(:bulk_insert) - .with(Milestone, [milestone_hash]) + .with(Milestone, [milestone_hash], any_args) expect(importer) .to receive(:build_milestones_cache) importer.execute end + + it 'tracks internal ids' do + milestone_hash = { iid: 1, title: '1.0', project_id: project.id } + allow(importer) + .to receive(:build_milestones) + .and_return([milestone_hash]) + + expect(InternalId).to receive(:track_greatest) + .with(nil, { project: project }, :milestones, 1, any_args) + + importer.execute + end end describe '#build_milestones' do diff --git a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb index 3422a1e82fc..44c920043b4 100644 --- a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb @@ -111,6 +111,16 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi expect(mr).to be_instance_of(MergeRequest) expect(exists).to eq(false) end + + it 'triggers internal_id functionality to track greatest iids' do + mr = build_stubbed(:merge_request, source_project: project, target_project: project) + allow(Gitlab::GithubImport).to receive(:insert_and_return_id).and_return(mr.id) + allow(project.merge_requests).to receive(:find).with(mr.id).and_return(mr) + + expect(mr).to receive(:ensure_target_project_iid!) + + importer.create_merge_request + end end context 'when the author could not be found' do diff --git a/spec/lib/system_check/simple_executor_spec.rb b/spec/lib/system_check/simple_executor_spec.rb index 9da3648400e..e71e9da369d 100644 --- a/spec/lib/system_check/simple_executor_spec.rb +++ b/spec/lib/system_check/simple_executor_spec.rb @@ -98,15 +98,6 @@ describe SystemCheck::SimpleExecutor do end end - before do - @rainbow = Rainbow.enabled - Rainbow.enabled = false - end - - after do - Rainbow.enabled = @rainbow - end - describe '#component' do it 'returns stored component name' do expect(subject.component).to eq('Test') diff --git a/spec/migrations/delete_inconsistent_internal_id_records_spec.rb b/spec/migrations/delete_inconsistent_internal_id_records_spec.rb new file mode 100644 index 00000000000..becb71cf427 --- /dev/null +++ b/spec/migrations/delete_inconsistent_internal_id_records_spec.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true +# rubocop:disable RSpec/FactoriesInMigrationSpecs +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20180723130817_delete_inconsistent_internal_id_records.rb') + +describe DeleteInconsistentInternalIdRecords, :migration do + let!(:project1) { create(:project) } + let!(:project2) { create(:project) } + let!(:project3) { create(:project) } + + let(:internal_id_query) { ->(project) { InternalId.where(usage: InternalId.usages[scope.to_s.tableize], project: project) } } + + let(:create_models) do + 3.times { create(scope, project: project1) } + 3.times { create(scope, project: project2) } + 3.times { create(scope, project: project3) } + end + + shared_examples_for 'deleting inconsistent internal_id records' do + before do + create_models + + internal_id_query.call(project1).first.tap do |iid| + iid.last_value = iid.last_value - 2 + # This is an inconsistent record + iid.save! + end + + internal_id_query.call(project3).first.tap do |iid| + iid.last_value = iid.last_value + 2 + # This is a consistent record + iid.save! + end + end + + it "deletes inconsistent issues" do + expect { migrate! }.to change { internal_id_query.call(project1).size }.from(1).to(0) + end + + it "retains consistent issues" do + expect { migrate! }.not_to change { internal_id_query.call(project2).size } + end + + it "retains consistent records, especially those with a greater last_value" do + expect { migrate! }.not_to change { internal_id_query.call(project3).size } + end + end + + context 'for issues' do + let(:scope) { :issue } + it_behaves_like 'deleting inconsistent internal_id records' + end + + context 'for merge_requests' do + let(:scope) { :merge_request } + + let(:create_models) do + 3.times { |i| create(scope, target_project: project1, source_project: project1, source_branch: i.to_s) } + 3.times { |i| create(scope, target_project: project2, source_project: project2, source_branch: i.to_s) } + 3.times { |i| create(scope, target_project: project3, source_project: project3, source_branch: i.to_s) } + end + + it_behaves_like 'deleting inconsistent internal_id records' + end + + context 'for deployments' do + let(:scope) { :deployment } + it_behaves_like 'deleting inconsistent internal_id records' + end + + context 'for milestones (by project)' do + let(:scope) { :milestone } + it_behaves_like 'deleting inconsistent internal_id records' + end + + context 'for ci_pipelines' do + let(:scope) { :ci_pipeline } + it_behaves_like 'deleting inconsistent internal_id records' + end + + context 'for milestones (by group)' do + # milestones (by group) is a little different than all of the other models + let!(:group1) { create(:group) } + let!(:group2) { create(:group) } + let!(:group3) { create(:group) } + + let(:internal_id_query) { ->(group) { InternalId.where(usage: InternalId.usages['milestones'], namespace: group) } } + + before do + 3.times { create(:milestone, group: group1) } + 3.times { create(:milestone, group: group2) } + 3.times { create(:milestone, group: group3) } + + internal_id_query.call(group1).first.tap do |iid| + iid.last_value = iid.last_value - 2 + # This is an inconsistent record + iid.save! + end + + internal_id_query.call(group3).first.tap do |iid| + iid.last_value = iid.last_value + 2 + # This is a consistent record + iid.save! + end + end + + it "deletes inconsistent issues" do + expect { migrate! }.to change { internal_id_query.call(group1).size }.from(1).to(0) + end + + it "retains consistent issues" do + expect { migrate! }.not_to change { internal_id_query.call(group2).size } + end + + it "retains consistent records, especially those with a greater last_value" do + expect { migrate! }.not_to change { internal_id_query.call(group3).size } + end + end +end diff --git a/spec/migrations/migrate_null_wiki_access_levels_spec.rb b/spec/migrations/migrate_null_wiki_access_levels_spec.rb new file mode 100644 index 00000000000..f99273072a2 --- /dev/null +++ b/spec/migrations/migrate_null_wiki_access_levels_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20180809195358_migrate_null_wiki_access_levels.rb') + +describe MigrateNullWikiAccessLevels, :migration do + let(:namespaces) { table('namespaces') } + let(:projects) { table(:projects) } + let(:project_features) { table(:project_features) } + let(:migration) { described_class.new } + + before do + namespace = namespaces.create(name: 'foo', path: 'foo') + + projects.create!(id: 1, name: 'gitlab1', path: 'gitlab1', namespace_id: namespace.id) + projects.create!(id: 2, name: 'gitlab2', path: 'gitlab2', namespace_id: namespace.id) + projects.create!(id: 3, name: 'gitlab3', path: 'gitlab3', namespace_id: namespace.id) + + project_features.create!(id: 1, project_id: 1, wiki_access_level: nil) + project_features.create!(id: 2, project_id: 2, wiki_access_level: 10) + project_features.create!(id: 3, project_id: 3, wiki_access_level: 20) + end + + describe '#up' do + it 'migrates existing project_features with wiki_access_level NULL to 20' do + expect { migration.up }.to change { project_features.where(wiki_access_level: 20).count }.by(1) + end + end +end diff --git a/spec/migrations/schedule_create_gpg_key_subkeys_from_gpg_keys_spec.rb b/spec/migrations/schedule_create_gpg_key_subkeys_from_gpg_keys_spec.rb index 96bef107599..c4427910518 100644 --- a/spec/migrations/schedule_create_gpg_key_subkeys_from_gpg_keys_spec.rb +++ b/spec/migrations/schedule_create_gpg_key_subkeys_from_gpg_keys_spec.rb @@ -6,7 +6,7 @@ describe ScheduleCreateGpgKeySubkeysFromGpgKeys, :migration, :sidekiq do create(:gpg_key, id: 1, key: GpgHelpers::User1.public_key) # rubocop:disable RSpec/FactoriesInMigrationSpecs create(:gpg_key, id: 2, key: GpgHelpers::User3.public_key) # rubocop:disable RSpec/FactoriesInMigrationSpecs # Delete all subkeys so they can be recreated - GpgKeySubkey.destroy_all + GpgKeySubkey.destroy_all # rubocop: disable DestroyAll end it 'correctly schedules background migrations' do diff --git a/spec/models/award_emoji_spec.rb b/spec/models/award_emoji_spec.rb index b909e04dfc3..3f52091698c 100644 --- a/spec/models/award_emoji_spec.rb +++ b/spec/models/award_emoji_spec.rb @@ -77,4 +77,27 @@ describe AwardEmoji do end end end + + describe '.award_counts_for_user' do + let(:user) { create(:user) } + + before do + create(:award_emoji, user: user, name: 'thumbsup') + create(:award_emoji, user: user, name: 'thumbsup') + create(:award_emoji, user: user, name: 'thumbsdown') + create(:award_emoji, user: user, name: '+1') + end + + it 'returns the awarded emoji in descending order' do + awards = described_class.award_counts_for_user(user) + + expect(awards).to eq('thumbsup' => 2, 'thumbsdown' => 1, '+1' => 1) + end + + it 'limits the returned number of rows' do + awards = described_class.award_counts_for_user(user, 1) + + expect(awards).to eq('thumbsup' => 2) + end + end end diff --git a/spec/models/concerns/optionally_search_spec.rb b/spec/models/concerns/optionally_search_spec.rb new file mode 100644 index 00000000000..ff4212ddf18 --- /dev/null +++ b/spec/models/concerns/optionally_search_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe OptionallySearch do + let(:model) do + Class.new(ActiveRecord::Base) do + self.table_name = 'users' + + include OptionallySearch + end + end + + describe '.search' do + it 'raises NotImplementedError' do + expect { model.search('foo') }.to raise_error(NotImplementedError) + end + end + + describe '.optionally_search' do + context 'when a query is given' do + it 'delegates to the search method' do + expect(model) + .to receive(:search) + .with('foo') + + model.optionally_search('foo') + end + end + + context 'when no query is given' do + it 'returns the current relation' do + expect(model.optionally_search).to be_a_kind_of(ActiveRecord::Relation) + end + end + + context 'when an empty query is given' do + it 'returns the current relation' do + expect(model.optionally_search('')) + .to be_a_kind_of(ActiveRecord::Relation) + end + end + end +end diff --git a/spec/models/fork_network_member_spec.rb b/spec/models/fork_network_member_spec.rb index 25bf596fddc..60d04562e6c 100644 --- a/spec/models/fork_network_member_spec.rb +++ b/spec/models/fork_network_member_spec.rb @@ -11,7 +11,7 @@ describe ForkNetworkMember do let(:fork_network) { fork_network_member.fork_network } it 'removes the fork network if it was the last member' do - fork_network.fork_network_members.destroy_all + fork_network.fork_network_members.destroy_all # rubocop: disable DestroyAll expect(ForkNetwork.count).to eq(0) end diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb index 01129df1107..edd1cb455af 100644 --- a/spec/models/hooks/system_hook_spec.rb +++ b/spec/models/hooks/system_hook_spec.rb @@ -73,7 +73,7 @@ describe SystemHook do it "project_destroy hook" do project.add_maintainer(user) - project.project_members.destroy_all + project.project_members.destroy_all # rubocop: disable DestroyAll expect(WebMock).to have_requested(:post, system_hook.url).with( body: /user_remove_from_team/, @@ -110,7 +110,7 @@ describe SystemHook do it 'group member destroy hook' do group.add_maintainer(user) - group.group_members.destroy_all + group.group_members.destroy_all # rubocop: disable DestroyAll expect(WebMock).to have_requested(:post, system_hook.url).with( body: /user_remove_from_group/, diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 6258bfa232f..48f4e53b93e 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1764,7 +1764,7 @@ describe MergeRequest do context 'with no discussions' do before do - merge_request.notes.destroy_all + merge_request.notes.destroy_all # rubocop: disable DestroyAll end it 'returns true' do diff --git a/spec/models/project_group_link_spec.rb b/spec/models/project_group_link_spec.rb index 1fccf92627a..5bea21427d4 100644 --- a/spec/models/project_group_link_spec.rb +++ b/spec/models/project_group_link_spec.rb @@ -41,7 +41,7 @@ describe ProjectGroupLink do project.project_group_links.create(group: group) group_users.each { |user| expect(user.authorized_projects).to include(project) } - project.project_group_links.destroy_all + project.project_group_links.destroy_all # rubocop: disable DestroyAll group_users.each { |user| expect(user.authorized_projects).not_to include(project) } end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index d8a5e5f6869..56c07f5793b 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1478,6 +1478,53 @@ describe Project do end end + describe '.optionally_search' do + let(:project) { create(:project) } + + it 'searches for projects matching the query if one is given' do + relation = described_class.optionally_search(project.name) + + expect(relation).to eq([project]) + end + + it 'returns the current relation if no search query is given' do + relation = described_class.where(id: project.id) + + expect(relation.optionally_search).to eq(relation) + end + end + + describe '.paginate_in_descending_order_using_id' do + let!(:project1) { create(:project) } + let!(:project2) { create(:project) } + + it 'orders the relation in descending order' do + expect(described_class.paginate_in_descending_order_using_id) + .to eq([project2, project1]) + end + + it 'applies a limit to the relation' do + expect(described_class.paginate_in_descending_order_using_id(limit: 1)) + .to eq([project2]) + end + + it 'limits projects by and ID when given' do + expect(described_class.paginate_in_descending_order_using_id(before: project2.id)) + .to eq([project1]) + end + end + + describe '.including_namespace_and_owner' do + it 'eager loads the namespace and namespace owner' do + create(:project) + + row = described_class.eager_load_namespace_and_owner.to_a.first + recorder = ActiveRecord::QueryRecorder.new { row.namespace.owner } + + expect(recorder.count).to be_zero + end + end + describe '#expire_caches_before_rename' do let(:project) { create(:project, :repository) } let(:repo) { double(:repo, exists?: true) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f5e2c977104..9763477a711 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -346,17 +346,55 @@ describe User do end end - describe '.todo_authors' do - it 'filters users' do - create :user - user_2 = create :user - user_3 = create :user - current_user = create :user - create(:todo, user: current_user, author: user_2, state: :done) - create(:todo, user: current_user, author: user_3, state: :pending) + describe '.limit_to_todo_authors' do + context 'when filtering by todo authors' do + let(:user1) { create(:user) } + let(:user2) { create(:user) } - expect(described_class.todo_authors(current_user.id, 'pending')).to eq [user_3] - expect(described_class.todo_authors(current_user.id, 'done')).to eq [user_2] + before do + create(:todo, user: user1, author: user1, state: :done) + create(:todo, user: user2, author: user2, state: :pending) + end + + it 'only returns users that have authored todos' do + users = described_class.limit_to_todo_authors( + user: user2, + with_todos: true, + todo_state: :pending + ) + + expect(users).to eq([user2]) + end + + it 'ignores users that do not have a todo in the matching state' do + users = described_class.limit_to_todo_authors( + user: user1, + with_todos: true, + todo_state: :pending + ) + + expect(users).to be_empty + end + end + + context 'when not filtering by todo authors' do + it 'returns the input relation' do + user1 = create(:user) + user2 = create(:user) + rel = described_class.limit_to_todo_authors(user: user1) + + expect(rel).to include(user1, user2) + end + end + + context 'when no user is provided' do + it 'returns the input relation' do + user1 = create(:user) + user2 = create(:user) + rel = described_class.limit_to_todo_authors + + expect(rel).to include(user1, user2) + end end end end @@ -2901,4 +2939,86 @@ describe User do let(:uploader_class) { AttachmentUploader } end end + + describe '.union_with_user' do + context 'when no user ID is provided' do + it 'returns the input relation' do + user = create(:user) + + expect(described_class.union_with_user).to eq([user]) + end + end + + context 'when a user ID is provided' do + it 'includes the user object in the returned relation' do + user1 = create(:user) + user2 = create(:user) + users = described_class.where(id: user1.id).union_with_user(user2.id) + + expect(users).to include(user1) + expect(users).to include(user2) + end + + it 'does not re-apply any WHERE conditions on the outer query' do + relation = described_class.where(id: 1).union_with_user(2) + + expect(relation.arel.where_sql).to be_nil + end + end + end + + describe '.optionally_search' do + context 'using nil as the argument' do + it 'returns the current relation' do + user = create(:user) + + expect(described_class.optionally_search).to eq([user]) + end + end + + context 'using an empty String as the argument' do + it 'returns the current relation' do + user = create(:user) + + expect(described_class.optionally_search('')).to eq([user]) + end + end + + context 'using a non-empty String' do + it 'returns users matching the search query' do + user1 = create(:user) + create(:user) + + expect(described_class.optionally_search(user1.name)).to eq([user1]) + end + end + end + + describe '.where_not_in' do + context 'without an argument' do + it 'returns the current relation' do + user = create(:user) + + expect(described_class.where_not_in).to eq([user]) + end + end + + context 'using a list of user IDs' do + it 'excludes the users from the returned relation' do + user1 = create(:user) + user2 = create(:user) + + expect(described_class.where_not_in([user2.id])).to eq([user1]) + end + end + end + + describe '.reorder_by_name' do + it 'reorders the input relation' do + user1 = create(:user, name: 'A') + user2 = create(:user, name: 'B') + + expect(described_class.reorder_by_name).to eq([user1, user2]) + end + end end diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 35951251bc5..615fea11f26 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -205,7 +205,7 @@ describe GroupPolicy do nested_group.add_guest(developer) nested_group.add_guest(maintainer) - group.owners.destroy_all + group.owners.destroy_all # rubocop: disable DestroyAll group.add_guest(owner) nested_group.add_owner(owner) diff --git a/spec/requests/api/project_hooks_spec.rb b/spec/requests/api/project_hooks_spec.rb index bc45a63d9f1..d3f81cc038d 100644 --- a/spec/requests/api/project_hooks_spec.rb +++ b/spec/requests/api/project_hooks_spec.rb @@ -83,11 +83,6 @@ describe API::ProjectHooks, 'ProjectHooks' do expect(response).to have_gitlab_http_status(403) end end - - it "returns a 404 error if hook id is not available" do - get api("/projects/#{project.id}/hooks/1234", user) - expect(response).to have_gitlab_http_status(404) - end end describe "POST /projects/:id/hooks" do diff --git a/spec/requests/api/project_import_spec.rb b/spec/requests/api/project_import_spec.rb index e3fb6cecce9..bc06f3c3732 100644 --- a/spec/requests/api/project_import_spec.rb +++ b/spec/requests/api/project_import_spec.rb @@ -42,7 +42,7 @@ describe API::ProjectImport do expect(response).to have_gitlab_http_status(201) end - it 'does not shedule an import for a nampespace that does not exist' do + it 'does not schedule an import for a namespace that does not exist' do expect_any_instance_of(Project).not_to receive(:import_schedule) expect(::Projects::CreateService).not_to receive(:new) diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index eb41750bf47..c249c881db5 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -20,7 +20,6 @@ describe API::Projects do let(:admin) { create(:admin) } let(:project) { create(:project, :repository, namespace: user.namespace) } let(:project2) { create(:project, namespace: user.namespace) } - let(:snippet) { create(:project_snippet, :public, author: user, project: project, title: 'example') } let(:project_member) { create(:project_member, :developer, user: user3, project: project) } let(:user4) { create(:user) } let(:project3) do @@ -575,7 +574,7 @@ describe API::Projects do expect(json_response['avatar_url']).to eq("http://localhost/uploads/-/system/project/avatar/#{project_id}/banana_sample.gif") end - it 'sets a project as allowing outdated diff discussions to automatically resolve' do + it 'sets a project as not allowing outdated diff discussions to automatically resolve' do project = attributes_for(:project, resolve_outdated_diff_discussions: false) post api('/projects', user), project @@ -583,7 +582,7 @@ describe API::Projects do expect(json_response['resolve_outdated_diff_discussions']).to be_falsey end - it 'sets a project as allowing outdated diff discussions to automatically resolve if resolve_outdated_diff_discussions' do + it 'sets a project as allowing outdated diff discussions to automatically resolve' do project = attributes_for(:project, resolve_outdated_diff_discussions: true) post api('/projects', user), project @@ -698,7 +697,7 @@ describe API::Projects do expect(json_response.map { |project| project['id'] }).to contain_exactly(public_project.id) end - it 'returns projects filetered by minimal access level' do + it 'returns projects filtered by minimal access level' do private_project1 = create(:project, :private, name: 'private_project1', creator_id: user4.id, namespace: user4.namespace) private_project2 = create(:project, :private, name: 'private_project2', creator_id: user4.id, namespace: user4.namespace) private_project1.add_developer(user2) @@ -789,7 +788,7 @@ describe API::Projects do expect(json_response['visibility']).to eq('private') end - it 'sets a project as allowing outdated diff discussions to automatically resolve' do + it 'sets a project as not allowing outdated diff discussions to automatically resolve' do project = attributes_for(:project, resolve_outdated_diff_discussions: false) post api("/projects/user/#{user.id}", admin), project @@ -1119,100 +1118,6 @@ describe API::Projects do end end - describe 'GET /projects/:id/snippets' do - before do - snippet - end - - it 'returns an array of project snippets' do - get api("/projects/#{project.id}/snippets", user) - - expect(response).to have_gitlab_http_status(200) - expect(response).to include_pagination_headers - expect(json_response).to be_an Array - expect(json_response.first['title']).to eq(snippet.title) - end - end - - describe 'GET /projects/:id/snippets/:snippet_id' do - it 'returns a project snippet' do - get api("/projects/#{project.id}/snippets/#{snippet.id}", user) - expect(response).to have_gitlab_http_status(200) - expect(json_response['title']).to eq(snippet.title) - end - - it 'returns a 404 error if snippet id not found' do - get api("/projects/#{project.id}/snippets/1234", user) - expect(response).to have_gitlab_http_status(404) - end - end - - describe 'POST /projects/:id/snippets' do - it 'creates a new project snippet' do - post api("/projects/#{project.id}/snippets", user), - title: 'api test', file_name: 'sample.rb', code: 'test', visibility: 'private' - expect(response).to have_gitlab_http_status(201) - expect(json_response['title']).to eq('api test') - end - - it 'returns a 400 error if invalid snippet is given' do - post api("/projects/#{project.id}/snippets", user) - expect(status).to eq(400) - end - end - - describe 'PUT /projects/:id/snippets/:snippet_id' do - it 'updates an existing project snippet' do - put api("/projects/#{project.id}/snippets/#{snippet.id}", user), - code: 'updated code' - expect(response).to have_gitlab_http_status(200) - expect(json_response['title']).to eq('example') - expect(snippet.reload.content).to eq('updated code') - end - - it 'updates an existing project snippet with new title' do - put api("/projects/#{project.id}/snippets/#{snippet.id}", user), - title: 'other api test' - expect(response).to have_gitlab_http_status(200) - expect(json_response['title']).to eq('other api test') - end - end - - describe 'DELETE /projects/:id/snippets/:snippet_id' do - before do - snippet - end - - it 'deletes existing project snippet' do - expect do - delete api("/projects/#{project.id}/snippets/#{snippet.id}", user) - - expect(response).to have_gitlab_http_status(204) - end.to change { Snippet.count }.by(-1) - end - - it 'returns 404 when deleting unknown snippet id' do - delete api("/projects/#{project.id}/snippets/1234", user) - expect(response).to have_gitlab_http_status(404) - end - - it_behaves_like '412 response' do - let(:request) { api("/projects/#{project.id}/snippets/#{snippet.id}", user) } - end - end - - describe 'GET /projects/:id/snippets/:snippet_id/raw' do - it 'gets a raw project snippet' do - get api("/projects/#{project.id}/snippets/#{snippet.id}/raw", user) - expect(response).to have_gitlab_http_status(200) - end - - it 'returns a 404 error if raw project snippet not found' do - get api("/projects/#{project.id}/snippets/5555/raw", user) - expect(response).to have_gitlab_http_status(404) - end - end - describe 'fork management' do let(:project_fork_target) { create(:project) } let(:project_fork_source) { create(:project, :public) } @@ -1235,7 +1140,7 @@ describe API::Projects do expect(project_fork_target.forked?).to be_truthy end - it 'refreshes the forks count cachce' do + it 'refreshes the forks count cache' do expect(project_fork_source.forks_count).to be_zero post api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin) diff --git a/spec/rubocop/cop/destroy_all_spec.rb b/spec/rubocop/cop/destroy_all_spec.rb new file mode 100644 index 00000000000..b0bc40552b3 --- /dev/null +++ b/spec/rubocop/cop/destroy_all_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../rubocop/cop/destroy_all' + +describe RuboCop::Cop::DestroyAll do + include CopHelper + + subject(:cop) { described_class.new } + + it 'flags the use of destroy_all with a send receiver' do + inspect_source('foo.destroy_all # rubocop: disable DestroyAll') + + expect(cop.offenses.size).to eq(1) + end + + it 'flags the use of destroy_all with a constant receiver' do + inspect_source('User.destroy_all # rubocop: disable DestroyAll') + + expect(cop.offenses.size).to eq(1) + end + + it 'flags the use of destroy_all when passing arguments' do + inspect_source('User.destroy_all([])') + + expect(cop.offenses.size).to eq(1) + end + + it 'flags the use of destroy_all with a local variable receiver' do + inspect_source(<<~RUBY) + users = User.all + users.destroy_all # rubocop: disable DestroyAll + RUBY + + expect(cop.offenses.size).to eq(1) + end + + it 'does not flag the use of delete_all' do + inspect_source('foo.delete_all') + + expect(cop.offenses).to be_empty + end +end diff --git a/spec/serializers/move_to_project_entity_spec.rb b/spec/serializers/move_to_project_entity_spec.rb new file mode 100644 index 00000000000..ac495eadb68 --- /dev/null +++ b/spec/serializers/move_to_project_entity_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MoveToProjectEntity do + describe '#as_json' do + let(:project) { build(:project, id: 1) } + + subject { described_class.new(project).as_json } + + it 'includes the project ID' do + expect(subject[:id]).to eq(project.id) + end + + it 'includes the full path' do + expect(subject[:name_with_namespace]).to eq(project.name_with_namespace) + end + end +end diff --git a/spec/serializers/move_to_project_serializer_spec.rb b/spec/serializers/move_to_project_serializer_spec.rb new file mode 100644 index 00000000000..841ac969eeb --- /dev/null +++ b/spec/serializers/move_to_project_serializer_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MoveToProjectSerializer do + describe '#represent' do + it 'includes the name and name with namespace' do + project = build(:project, id: 1) + output = described_class.new.represent(project) + + expect(output).to include(:id, :name_with_namespace) + end + end +end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 06fb61baf33..74bcc15f912 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -134,9 +134,11 @@ describe MergeRequests::CreateService do let!(:pipeline_3) { create(:ci_pipeline, project: project, ref: "other_branch", project_id: project.id) } before do + # rubocop: disable DestroyAll project.merge_requests .where(source_branch: opts[:source_branch], target_branch: opts[:target_branch]) .destroy_all + # rubocop: enable DestroyAll end it 'sets head pipeline' do diff --git a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb index 1c632847940..6268c149fc6 100644 --- a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb +++ b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb @@ -46,10 +46,12 @@ describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_shared_ end it 'schedules no removal if there is no non-latest diffs' do + # rubocop: disable DestroyAll merge_request .merge_request_diffs .where.not(id: merge_request.latest_merge_request_diff_id) .destroy_all + # rubocop: enable DestroyAll expect(DeleteDiffFilesWorker).not_to receive(:bulk_perform_in) diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 9a51c873b30..1746721b0d0 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -280,7 +280,7 @@ describe TodoService do end it 'does not create a todo if unassigned' do - issue.assignees.destroy_all + issue.assignees.destroy_all # rubocop: disable DestroyAll should_not_create_any_todo { service.reassigned_issue(issue, author) } end diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index 3bae8bfbd42..83f1495a1c6 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -20,7 +20,7 @@ describe Users::DestroyService do it 'will delete the project' do expect_next_instance_of(Projects::DestroyService) do |destroy_service| - expect(destroy_service).to receive(:execute).once + expect(destroy_service).to receive(:execute).once.and_return(true) end service.execute(user) @@ -35,7 +35,7 @@ describe Users::DestroyService do it 'destroys a project in pending_delete' do expect_next_instance_of(Projects::DestroyService) do |destroy_service| - expect(destroy_service).to receive(:execute).once + expect(destroy_service).to receive(:execute).once.and_return(true) end service.execute(user) @@ -172,23 +172,36 @@ describe Users::DestroyService do end describe "user personal's repository removal" do - before do - perform_enqueued_jobs { service.execute(user) } - end + context 'storages' do + before do + perform_enqueued_jobs { service.execute(user) } + end + + context 'legacy storage' do + let!(:project) { create(:project, :empty_repo, :legacy_storage, namespace: user.namespace) } + + it 'removes repository' do + expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey + end + end - context 'legacy storage' do - let!(:project) { create(:project, :empty_repo, :legacy_storage, namespace: user.namespace) } + context 'hashed storage' do + let!(:project) { create(:project, :empty_repo, namespace: user.namespace) } - it 'removes repository' do - expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey + it 'removes repository' do + expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey + end end end - context 'hashed storage' do - let!(:project) { create(:project, :empty_repo, namespace: user.namespace) } + context 'repository removal status is taken into account' do + it 'raises exception' do + expect_next_instance_of(::Projects::DestroyService) do |destroy_service| + expect(destroy_service).to receive(:execute).and_return(false) + end - it 'removes repository' do - expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey + expect { service.execute(user) } + .to raise_error(Users::DestroyService::DestroyError, "Project #{project.id} can't be deleted" ) end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index f4441a6b700..a15a46a9534 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -29,6 +29,7 @@ end # require rainbow gem String monkeypatch, so we can test SystemChecks require 'rainbow/ext/string' +Rainbow.enabled = false # Requires supporting ruby files with custom matchers and macros, etc, # in spec/support/ and its subdirectories. diff --git a/spec/support/api/milestones_shared_examples.rb b/spec/support/api/milestones_shared_examples.rb index a15189db35f..afd6448aa26 100644 --- a/spec/support/api/milestones_shared_examples.rb +++ b/spec/support/api/milestones_shared_examples.rb @@ -102,14 +102,6 @@ shared_examples_for 'group and project milestones' do |route_definition| expect(json_response['iid']).to eq(milestone.iid) end - it 'returns a milestone by id' do - get api(resource_route, user) - - expect(response).to have_gitlab_http_status(200) - expect(json_response['title']).to eq(milestone.title) - expect(json_response['iid']).to eq(milestone.iid) - end - it 'returns 401 error if user not authenticated' do get api(resource_route) diff --git a/spec/support/shared_examples/fast_destroy_all.rb b/spec/support/shared_examples/fast_destroy_all.rb index 5448ddcfe33..a8079b6d864 100644 --- a/spec/support/shared_examples/fast_destroy_all.rb +++ b/spec/support/shared_examples/fast_destroy_all.rb @@ -4,8 +4,8 @@ shared_examples_for 'fast destroyable' do expect(external_data_counter).to be > 0 expect(subjects.count).to be > 0 - expect { subjects.first.destroy }.to raise_error('`destroy` and `destroy_all` are forbbiden. Please use `fast_destroy_all`') - expect { subjects.destroy_all }.to raise_error('`destroy` and `destroy_all` are forbbiden. Please use `fast_destroy_all`') + expect { subjects.first.destroy }.to raise_error('`destroy` and `destroy_all` are forbidden. Please use `fast_destroy_all`') + expect { subjects.destroy_all }.to raise_error('`destroy` and `destroy_all` are forbidden. Please use `fast_destroy_all`') # rubocop: disable DestroyAll expect(subjects.count).to be > 0 expect(external_data_counter).to be > 0 diff --git a/spec/tasks/gitlab/site_statistics_rake_spec.rb b/spec/tasks/gitlab/site_statistics_rake_spec.rb new file mode 100644 index 00000000000..20f0df65e63 --- /dev/null +++ b/spec/tasks/gitlab/site_statistics_rake_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true +require 'rake_helper' + +describe 'rake gitlab:refresh_site_statistics' do + before do + Rake.application.rake_require 'tasks/gitlab/site_statistics' + + create(:project) + SiteStatistic.fetch.update(repositories_count: 0, wikis_count: 0) + end + + let(:task) { 'gitlab:refresh_site_statistics' } + + it 'recalculates existing counters' do + run_rake_task(task) + + expect(SiteStatistic.fetch.repositories_count).to eq(1) + expect(SiteStatistic.fetch.wikis_count).to eq(1) + end + + it 'displays message listing counters' do + expect { run_rake_task(task) }.to output(/Updating Site Statistics counters:.* Repositories\.\.\. OK!.* Wikis\.\.\. OK!/m).to_stdout + end +end diff --git a/spec/workers/project_destroy_worker_spec.rb b/spec/workers/project_destroy_worker_spec.rb index 42e1d86e3bb..6132f145f8d 100644 --- a/spec/workers/project_destroy_worker_spec.rb +++ b/spec/workers/project_destroy_worker_spec.rb @@ -18,13 +18,6 @@ describe ProjectDestroyWorker do expect(Dir.exist?(path)).to be_falsey end - it 'deletes the project but skips repo deletion' do - subject.perform(project.id, project.owner.id, { "skip_repo" => true }) - - expect(Project.all).not_to include(project) - expect(Dir.exist?(path)).to be_truthy - end - it 'does not raise error when project could not be found' do expect do subject.perform(-1, project.owner.id, {}) diff --git a/spec/workers/repository_check/single_repository_worker_spec.rb b/spec/workers/repository_check/single_repository_worker_spec.rb index 22fc64c1536..f11875cffd1 100644 --- a/spec/workers/repository_check/single_repository_worker_spec.rb +++ b/spec/workers/repository_check/single_repository_worker_spec.rb @@ -6,7 +6,7 @@ describe RepositoryCheck::SingleRepositoryWorker do it 'skips when the project has no push events' do project = create(:project, :repository, :wiki_disabled) - project.events.destroy_all + project.events.destroy_all # rubocop: disable DestroyAll break_project(project) expect(worker).not_to receive(:git_fsck) |
