diff options
78 files changed, 1389 insertions, 381 deletions
diff --git a/.gitignore b/.gitignore index 93fb0b1144b..388718898e4 100644 --- a/.gitignore +++ b/.gitignore @@ -19,8 +19,8 @@ eslint-report.html /.ruby-version /.tool-versions /.rvmrc -.sass-cache/ /.secret +.sass-cache/ /.vagrant /.yarn-cache /.byebug_history @@ -104,3 +104,4 @@ ee/changelogs/unreleased-ee /sitespeed-result tags.lock tags.temp +.stylelintcache diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index 22629c1167f..e76b0f2d07f 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -149,6 +149,7 @@ - "{,ee/}{app/channels,app/controllers,app/finders,app/graphql,app/helpers,app/mailers,app/models,app/policies,app/presenters,app/serializers,app/services,app/uploaders,app/validators,app/views,app/workers}/**/*" - "{,ee/}{bin,cable,config,db,lib}/**/*" - "{,ee/}spec/**/*.rb" + # CI changes - ".gitlab-ci.yml" - ".gitlab/ci/**/*" - "*_VERSION" @@ -162,6 +163,9 @@ - "{,ee/}spec/support/helpers/database/**/*" - "config/prometheus/common_metrics.yml" # Used by Gitlab::DatabaseImporters::CommonMetrics::Importer - "{,ee/}app/models/project_statistics.rb" # Used to calculate sizes in migration specs + # CI changes + - ".gitlab-ci.yml" + - ".gitlab/ci/**/*" .db-library-patterns: &db-library-patterns - "{,ee/}{,spec/}lib/{,ee/}gitlab/database/**/*" @@ -183,6 +187,8 @@ - ".csscomb.json" - "Dockerfile.assets" - "vendor/assets/**/*" + # CI changes + - ".gitlab-ci.yml" - ".gitlab/ci/**/*" - ".{eslintignore,gitattributes,nvmrc,prettierrc,stylelintrc,yamllint}" - ".{codeclimate,eslintrc,gitlab-ci,haml-lint,haml-lint_todo,rubocop,rubocop_todo,rubocop_manual_todo}.yml" @@ -206,6 +212,8 @@ - ".csscomb.json" - "Dockerfile.assets" - "vendor/assets/**/*" + # CI changes + - ".gitlab-ci.yml" - ".gitlab/ci/**/*" - ".{eslintignore,gitattributes,nvmrc,prettierrc,stylelintrc,yamllint}" - ".{codeclimate,eslintrc,gitlab-ci,haml-lint,haml-lint_todo,rubocop,rubocop_todo,rubocop_manual_todo}.yml" @@ -232,6 +240,8 @@ - ".csscomb.json" - "Dockerfile.assets" - "vendor/assets/**/*" + # CI changes + - ".gitlab-ci.yml" - ".gitlab/ci/**/*" - ".{eslintignore,gitattributes,nvmrc,prettierrc,stylelintrc,yamllint}" - ".{codeclimate,eslintrc,gitlab-ci,haml-lint,haml-lint_todo,rubocop,rubocop_todo,rubocop_manual_todo}.yml" @@ -254,6 +264,8 @@ - ".csscomb.json" - "Dockerfile.assets" - "vendor/assets/**/*" + # CI changes + - ".gitlab-ci.yml" - ".gitlab/ci/**/*" - ".{eslintignore,gitattributes,nvmrc,prettierrc,stylelintrc,yamllint}" - ".{codeclimate,eslintrc,gitlab-ci,haml-lint,haml-lint_todo,rubocop,rubocop_todo,rubocop_manual_todo}.yml" diff --git a/app/assets/javascripts/awards_handler.js b/app/assets/javascripts/awards_handler.js index b0652a9e2b0..dbdc7e43d2d 100644 --- a/app/assets/javascripts/awards_handler.js +++ b/app/assets/javascripts/awards_handler.js @@ -12,6 +12,7 @@ import axios from './lib/utils/axios_utils'; import { isInVueNoteablePage } from './lib/utils/dom_utils'; import { __ } from './locale'; +window.axios = axios; const animationEndEventString = 'animationend webkitAnimationEnd MSAnimationEnd oAnimationEnd'; const transitionEndEventString = 'transitionend webkitTransitionEnd oTransitionEnd MSTransitionEnd'; diff --git a/app/assets/javascripts/commons/vue.js b/app/assets/javascripts/commons/vue.js index 5b5a1507d38..23647d99656 100644 --- a/app/assets/javascripts/commons/vue.js +++ b/app/assets/javascripts/commons/vue.js @@ -6,3 +6,5 @@ if (process.env.NODE_ENV !== 'production') { } Vue.use(GlFeatureFlagsPlugin); + +Vue.config.ignoredElements = ['gl-emoji']; diff --git a/app/assets/javascripts/emoji/components/category.vue b/app/assets/javascripts/emoji/components/category.vue new file mode 100644 index 00000000000..a11122d5403 --- /dev/null +++ b/app/assets/javascripts/emoji/components/category.vue @@ -0,0 +1,61 @@ +<script> +import { GlIntersectionObserver } from '@gitlab/ui'; +import { capitalizeFirstCharacter } from '~/lib/utils/text_utility'; +import EmojiGroup from './emoji_group.vue'; + +export default { + components: { + GlIntersectionObserver, + EmojiGroup, + }, + props: { + category: { + type: String, + required: true, + }, + emojis: { + type: Array, + required: true, + }, + }, + data() { + return { + renderGroup: false, + }; + }, + computed: { + categoryTitle() { + return capitalizeFirstCharacter(this.category); + }, + }, + methods: { + categoryAppeared() { + this.renderGroup = true; + this.$emit('appear', this.category); + }, + categoryDissappeared() { + this.renderGroup = false; + }, + }, +}; +</script> + +<template> + <gl-intersection-observer class="gl-px-5 gl-h-full" @appear="categoryAppeared"> + <div class="gl-top-0 gl-py-3 gl-w-full emoji-picker-category-header"> + <b>{{ categoryTitle }}</b> + </div> + <template v-if="emojis.length"> + <emoji-group + v-for="(emojiGroup, index) in emojis" + :key="index" + :emojis="emojiGroup" + :render-group="renderGroup" + :click-emoji="(emoji) => $emit('click', emoji)" + /> + </template> + <p v-else> + {{ s__('AwardEmoji|No emojis found.') }} + </p> + </gl-intersection-observer> +</template> diff --git a/app/assets/javascripts/emoji/components/emoji_group.vue b/app/assets/javascripts/emoji/components/emoji_group.vue new file mode 100644 index 00000000000..539cd6963b1 --- /dev/null +++ b/app/assets/javascripts/emoji/components/emoji_group.vue @@ -0,0 +1,35 @@ +<script> +export default { + props: { + emojis: { + type: Array, + required: true, + }, + renderGroup: { + type: Boolean, + required: true, + }, + clickEmoji: { + type: Function, + required: true, + }, + }, +}; +</script> + +<template functional> + <div class="gl-display-flex gl-flex-wrap gl-mb-2"> + <template v-if="props.renderGroup"> + <button + v-for="emoji in props.emojis" + :key="emoji" + type="button" + class="gl-border-0 gl-bg-transparent gl-px-0 gl-py-2 gl-text-center emoji-picker-emoji" + data-testid="emoji-button" + @click="props.clickEmoji(emoji)" + > + <gl-emoji :data-name="emoji" /> + </button> + </template> + </div> +</template> diff --git a/app/assets/javascripts/emoji/components/emoji_list.vue b/app/assets/javascripts/emoji/components/emoji_list.vue new file mode 100644 index 00000000000..0d73d751c6d --- /dev/null +++ b/app/assets/javascripts/emoji/components/emoji_list.vue @@ -0,0 +1,44 @@ +<script> +import { chunk } from 'lodash'; +import { searchEmoji } from '~/emoji'; +import { EMOJIS_PER_ROW } from '../constants'; +import { getEmojiCategories, generateCategoryHeight } from './utils'; + +export default { + props: { + searchValue: { + type: String, + required: true, + }, + }, + data() { + return { render: false }; + }, + computed: { + filteredCategories() { + if (this.searchValue !== '') { + const emojis = chunk( + searchEmoji(this.searchValue).map(({ emoji }) => emoji.name), + EMOJIS_PER_ROW, + ); + + return { + search: { emojis, height: generateCategoryHeight(emojis.length) }, + }; + } + + return this.categories; + }, + }, + async mounted() { + this.categories = await getEmojiCategories(); + this.render = true; + }, +}; +</script> + +<template> + <div v-if="render"> + <slot :filtered-categories="filteredCategories"></slot> + </div> +</template> diff --git a/app/assets/javascripts/emoji/components/picker.vue b/app/assets/javascripts/emoji/components/picker.vue new file mode 100644 index 00000000000..7cd20d82329 --- /dev/null +++ b/app/assets/javascripts/emoji/components/picker.vue @@ -0,0 +1,121 @@ +<script> +import { GlIcon, GlDropdown, GlSearchBoxByType } from '@gitlab/ui'; +import VirtualList from 'vue-virtual-scroll-list'; +import { CATEGORY_NAMES } from '~/emoji'; +import { CATEGORY_ICON_MAP } from '../constants'; +import Category from './category.vue'; +import EmojiList from './emoji_list.vue'; +import { getEmojiCategories } from './utils'; + +export default { + components: { + GlIcon, + GlDropdown, + GlSearchBoxByType, + VirtualList, + Category, + EmojiList, + }, + props: { + toggleClass: { + type: [Array, String, Object], + required: false, + default: () => [], + }, + }, + data() { + return { + currentCategory: null, + searchValue: '', + }; + }, + computed: { + categoryNames() { + return CATEGORY_NAMES.map((category) => ({ + name: category, + icon: CATEGORY_ICON_MAP[category], + })); + }, + }, + methods: { + categoryAppeared(category) { + this.currentCategory = category; + }, + async scrollToCategory(categoryName) { + const categories = await getEmojiCategories(); + const { top } = categories[categoryName]; + + this.$refs.virtualScoller.setScrollTop(top); + }, + selectEmoji(name) { + this.$emit('click', name); + this.$refs.dropdown.hide(); + }, + getBoundaryElement() { + return document.querySelector('.content-wrapper') || 'scrollParent'; + }, + onSearchInput() { + this.$refs.virtualScoller.setScrollTop(0); + this.$refs.virtualScoller.forceRender(); + }, + }, +}; +</script> + +<template> + <div class="emoji-picker"> + <gl-dropdown + ref="dropdown" + :toggle-class="toggleClass" + :boundary="getBoundaryElement()" + menu-class="dropdown-extended-height" + no-flip + right + lazy + > + <template #button-content><slot name="button-content"></slot></template> + <gl-search-box-by-type + v-model="searchValue" + class="gl-mx-5! gl-mb-2!" + autofocus + debounce="500" + @input="onSearchInput" + /> + <div + v-show="!searchValue" + class="gl-display-flex gl-mx-5 gl-border-b-solid gl-border-gray-100 gl-border-b-1" + > + <button + v-for="category in categoryNames" + :key="category.name" + :class="{ + 'gl-text-black-normal! emoji-picker-category-active': category.name === currentCategory, + }" + type="button" + class="gl-border-0 gl-border-b-2 gl-border-b-solid gl-flex-fill-1 gl-text-gray-300 gl-pt-3 gl-pb-3 gl-bg-transparent emoji-picker-category-tab" + @click="scrollToCategory(category.name)" + > + <gl-icon :name="category.icon" :size="12" /> + </button> + </div> + <emoji-list :search-value="searchValue"> + <template #default="{ filteredCategories }"> + <virtual-list ref="virtualScoller" :size="258" :remain="1" :bench="2" variable> + <div + v-for="(category, categoryKey) in filteredCategories" + :key="categoryKey" + :style="{ height: category.height + 'px' }" + > + <category + :category="categoryKey" + :emojis="category.emojis" + @appear="categoryAppeared" + @click="selectEmoji" + /> + </div> + </virtual-list> + </template> + </emoji-list> + </gl-dropdown> + </div> +</template> diff --git a/app/assets/javascripts/emoji/components/utils.js b/app/assets/javascripts/emoji/components/utils.js new file mode 100644 index 00000000000..b95b56a1d6f --- /dev/null +++ b/app/assets/javascripts/emoji/components/utils.js @@ -0,0 +1,27 @@ +import { chunk, memoize } from 'lodash'; +import { initEmojiMap, getEmojiCategoryMap } from '~/emoji'; +import { EMOJIS_PER_ROW, EMOJI_ROW_HEIGHT, CATEGORY_ROW_HEIGHT } from '../constants'; + +export const generateCategoryHeight = (emojisLength) => + emojisLength * EMOJI_ROW_HEIGHT + CATEGORY_ROW_HEIGHT; + +export const getEmojiCategories = memoize(async () => { + await initEmojiMap(); + + const categories = await getEmojiCategoryMap(); + let top = 0; + + return Object.freeze( + Object.keys(categories).reduce((acc, category) => { + const emojis = chunk(categories[category], EMOJIS_PER_ROW); + const height = generateCategoryHeight(emojis.length); + const newAcc = { + ...acc, + [category]: { emojis, height, top }, + }; + top += height; + + return newAcc; + }, {}), + ); +}); diff --git a/app/assets/javascripts/emoji/constants.js b/app/assets/javascripts/emoji/constants.js new file mode 100644 index 00000000000..bf73d1ca5a9 --- /dev/null +++ b/app/assets/javascripts/emoji/constants.js @@ -0,0 +1,14 @@ +export const CATEGORY_ICON_MAP = { + activity: 'dumbbell', + people: 'smiley', + nature: 'nature', + food: 'food', + travel: 'car', + objects: 'object', + symbols: 'heart', + flags: 'flag', +}; + +export const EMOJIS_PER_ROW = 9; +export const EMOJI_ROW_HEIGHT = 34; +export const CATEGORY_ROW_HEIGHT = 37; diff --git a/app/assets/javascripts/emoji/index.js b/app/assets/javascripts/emoji/index.js index d022fcbeabe..d3b658a4020 100644 --- a/app/assets/javascripts/emoji/index.js +++ b/app/assets/javascripts/emoji/index.js @@ -2,6 +2,7 @@ import { escape, minBy } from 'lodash'; import emojiAliases from 'emojis/aliases.json'; import AccessorUtilities from '../lib/utils/accessor'; import axios from '../lib/utils/axios_utils'; +import { CATEGORY_ICON_MAP } from './constants'; let emojiMap = null; let validEmojiNames = null; @@ -155,19 +156,14 @@ export function sortEmoji(items) { return [...items].sort((a, b) => a.score - b.score || a.fieldValue.localeCompare(b.fieldValue)); } +export const CATEGORY_NAMES = Object.keys(CATEGORY_ICON_MAP); + let emojiCategoryMap; export function getEmojiCategoryMap() { if (!emojiCategoryMap) { - emojiCategoryMap = { - activity: [], - people: [], - nature: [], - food: [], - travel: [], - objects: [], - symbols: [], - flags: [], - }; + emojiCategoryMap = CATEGORY_NAMES.reduce((acc, category) => { + return { ...acc, [category]: [] }; + }, {}); Object.keys(emojiMap).forEach((name) => { const emoji = emojiMap[name]; if (emojiCategoryMap[emoji.c]) { diff --git a/app/assets/javascripts/notes/components/note_actions.vue b/app/assets/javascripts/notes/components/note_actions.vue index 0b2c7611f8e..ed6701b34e8 100644 --- a/app/assets/javascripts/notes/components/note_actions.vue +++ b/app/assets/javascripts/notes/components/note_actions.vue @@ -1,6 +1,6 @@ <script> import { GlTooltipDirective, GlIcon, GlButton, GlDropdownItem } from '@gitlab/ui'; -import { mapGetters } from 'vuex'; +import { mapActions, mapGetters } from 'vuex'; import Api from '~/api'; import resolvedStatusMixin from '~/batch_comments/mixins/resolved_status'; import { deprecatedCreateFlash as flash } from '~/flash'; @@ -8,6 +8,7 @@ import { BV_HIDE_TOOLTIP } from '~/lib/utils/constants'; import { __, sprintf } from '~/locale'; import eventHub from '~/sidebar/event_hub'; import UserAccessRoleBadge from '~/vue_shared/components/user_access_role_badge.vue'; +import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import { splitCamelCase } from '../../lib/utils/text_utility'; import ReplyButton from './note_actions/reply_button.vue'; @@ -19,11 +20,12 @@ export default { GlButton, GlDropdownItem, UserAccessRoleBadge, + EmojiPicker: () => import('~/emoji/components/picker.vue'), }, directives: { GlTooltip: GlTooltipDirective, }, - mixins: [resolvedStatusMixin], + mixins: [resolvedStatusMixin, glFeatureFlagsMixin()], props: { author: { type: Object, @@ -117,6 +119,10 @@ export default { type: Boolean, required: true, }, + awardPath: { + type: String, + required: true, + }, }, computed: { ...mapGetters(['getUserDataByProp', 'getNoteableData']), @@ -185,6 +191,7 @@ export default { }, }, methods: { + ...mapActions(['toggleAwardRequest']), onEdit() { this.$emit('handleEdit'); }, @@ -222,6 +229,13 @@ export default { .catch(() => flash(__('Something went wrong while updating assignees'))); } }, + setAwardEmoji(awardName) { + this.toggleAwardRequest({ + endpoint: this.awardPath, + noteId: this.noteId, + awardName, + }); + }, }, }; </script> @@ -267,28 +281,41 @@ export default { class="line-resolve-btn note-action-button" @click="onResolve" /> - <gl-button - v-if="canAwardEmoji" - v-gl-tooltip - :class="{ 'js-user-authored': isAuthoredByCurrentUser }" - class="note-action-button note-emoji-button add-reaction-button js-add-award js-note-emoji" - category="tertiary" - variant="default" - size="small" - title="Add reaction" - data-position="right" - :aria-label="__('Add reaction')" - > - <span class="reaction-control-icon reaction-control-icon-neutral"> - <gl-icon name="slight-smile" /> - </span> - <span class="reaction-control-icon reaction-control-icon-positive"> - <gl-icon name="smiley" /> - </span> - <span class="reaction-control-icon reaction-control-icon-super-positive"> - <gl-icon name="smile" /> - </span> - </gl-button> + <template v-if="canAwardEmoji"> + <emoji-picker + v-if="glFeatures.improvedEmojiPicker" + toggle-class="note-action-button note-emoji-button gl-text-gray-600 gl-m-2 gl-p-0! gl-shadow-none! gl-bg-transparent!" + @click="setAwardEmoji" + > + <template #button-content> + <gl-icon class="link-highlight award-control-icon-neutral gl-m-0!" name="slight-smile" /> + <gl-icon class="link-highlight award-control-icon-positive gl-m-0!" name="smiley" /> + <gl-icon class="link-highlight award-control-icon-super-positive gl-m-0!" name="smile" /> + </template> + </emoji-picker> + <gl-button + v-else + v-gl-tooltip + :class="{ 'js-user-authored': isAuthoredByCurrentUser }" + class="note-action-button note-emoji-button add-reaction-button js-add-award js-note-emoji" + category="tertiary" + variant="default" + size="small" + title="Add reaction" + data-position="right" + :aria-label="__('Add reaction')" + > + <span class="reaction-control-icon reaction-control-icon-neutral"> + <gl-icon name="slight-smile" /> + </span> + <span class="reaction-control-icon reaction-control-icon-positive"> + <gl-icon name="smiley" /> + </span> + <span class="reaction-control-icon reaction-control-icon-super-positive"> + <gl-icon name="smile" /> + </span> + </gl-button> + </template> <reply-button v-if="showReply" ref="replyButton" diff --git a/app/assets/javascripts/notes/components/note_form.vue b/app/assets/javascripts/notes/components/note_form.vue index a28c467117a..d74ade15de1 100644 --- a/app/assets/javascripts/notes/components/note_form.vue +++ b/app/assets/javascripts/notes/components/note_form.vue @@ -201,7 +201,7 @@ export default { changedCommentText() { return sprintf( __( - 'This comment has changed since you started editing, please review the %{startTag}updated comment%{endTag} to ensure information is not lost.', + 'This comment changed after you started editing it. Review the %{startTag}updated comment%{endTag} to ensure information is not lost.', ), { startTag: `<a href="${this.noteHash}" target="_blank" rel="noopener noreferrer">`, diff --git a/app/assets/javascripts/notes/components/noteable_note.vue b/app/assets/javascripts/notes/components/noteable_note.vue index 4343fac3cfa..9bf1496f479 100644 --- a/app/assets/javascripts/notes/components/noteable_note.vue +++ b/app/assets/javascripts/notes/components/noteable_note.vue @@ -416,6 +416,7 @@ export default { :is-draft="note.isDraft" :resolve-discussion="note.isDraft && note.resolve_discussion" :discussion-id="discussionId" + :award-path="note.toggle_award_path" @handleEdit="editHandler" @handleDelete="deleteHandler" @handleResolve="resolveHandler" diff --git a/app/assets/javascripts/vue_shared/components/awards_list.vue b/app/assets/javascripts/vue_shared/components/awards_list.vue index ce67d33d4a1..82b3545117f 100644 --- a/app/assets/javascripts/vue_shared/components/awards_list.vue +++ b/app/assets/javascripts/vue_shared/components/awards_list.vue @@ -2,7 +2,9 @@ /* eslint-disable vue/no-v-html */ import { GlIcon, GlButton, GlTooltipDirective } from '@gitlab/ui'; import { groupBy } from 'lodash'; +import EmojiPicker from '~/emoji/components/picker.vue'; import { __, sprintf } from '~/locale'; +import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import { glEmojiTag } from '../../emoji'; // Internal constant, specific to this component, used when no `currentUserId` is given @@ -12,10 +14,12 @@ export default { components: { GlButton, GlIcon, + EmojiPicker, }, directives: { GlTooltip: GlTooltipDirective, }, + mixins: [glFeatureFlagsMixin()], props: { awards: { type: Array, @@ -166,7 +170,25 @@ export default { <span class="js-counter">{{ awardList.list.length }}</span> </gl-button> <div v-if="canAwardEmoji" class="award-menu-holder"> + <emoji-picker + v-if="glFeatures.improvedEmojiPicker" + toggle-class="add-reaction-button gl-relative!" + @click="handleAward" + > + <template #button-content> + <span class="reaction-control-icon reaction-control-icon-neutral"> + <gl-icon name="slight-smile" /> + </span> + <span class="reaction-control-icon reaction-control-icon-positive"> + <gl-icon name="smiley" /> + </span> + <span class="reaction-control-icon reaction-control-icon-super-positive"> + <gl-icon name="smile" /> + </span> + </template> + </emoji-picker> <gl-button + v-else v-gl-tooltip.viewport :class="addButtonClass" class="add-reaction-button js-add-award" diff --git a/app/assets/javascripts/vue_shared/components/notes/timeline_icon.vue b/app/assets/javascripts/vue_shared/components/notes/timeline_icon.vue new file mode 100644 index 00000000000..35f9ac14681 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/notes/timeline_icon.vue @@ -0,0 +1,3 @@ +<template> + <div class="timeline-icon"><slot></slot></div> +</template> diff --git a/app/assets/stylesheets/_page_specific_files.scss b/app/assets/stylesheets/_page_specific_files.scss index e55654e84d8..4a15e0eb458 100644 --- a/app/assets/stylesheets/_page_specific_files.scss +++ b/app/assets/stylesheets/_page_specific_files.scss @@ -14,7 +14,6 @@ @import './pages/issues'; @import './pages/labels'; @import './pages/login'; -@import './pages/members'; @import './pages/merge_requests'; @import './pages/monitor'; @import './pages/note_form'; diff --git a/app/assets/stylesheets/framework/awards.scss b/app/assets/stylesheets/framework/awards.scss index a7623b65539..662f7f52d61 100644 --- a/app/assets/stylesheets/framework/awards.scss +++ b/app/assets/stylesheets/framework/awards.scss @@ -274,7 +274,9 @@ // `position:absolute` &::after { content: '\a0'; + display: block !important; width: 1em; + color: transparent; } .reaction-control-icon { diff --git a/app/assets/stylesheets/framework/emojis.scss b/app/assets/stylesheets/framework/emojis.scss index 13c5541da92..c5c660c1014 100644 --- a/app/assets/stylesheets/framework/emojis.scss +++ b/app/assets/stylesheets/framework/emojis.scss @@ -16,3 +16,26 @@ gl-emoji { vertical-align: baseline; } } + +.emoji-picker-category-header { + @include gl-sticky; + background-color: $white-transparent; +} + +.emoji-picker-emoji { + height: 30px; + // Create a width that fits 9 emojis per row + width: 100 / 9 * 1%; +} + +.emoji-picker .gl-new-dropdown .dropdown-menu { + width: 350px; +} + +.emoji-picker-category-tab { + border-bottom-color: transparent; +} + +.emoji-picker .gl-new-dropdown-inner > :last-child { + padding-bottom: 0; +} diff --git a/app/assets/stylesheets/pages/members.scss b/app/assets/stylesheets/page_bundles/members.scss index 0ccde57746a..7b4c74b8253 100644 --- a/app/assets/stylesheets/pages/members.scss +++ b/app/assets/stylesheets/page_bundles/members.scss @@ -1,3 +1,5 @@ +@import 'mixins_and_variables_and_functions'; + .project-members-title { padding-bottom: 10px; border-bottom: 1px solid $border-color; diff --git a/app/assets/stylesheets/themes/theme_helper.scss b/app/assets/stylesheets/themes/theme_helper.scss index aae9f3ded4f..5b3e2ab4cd0 100644 --- a/app/assets/stylesheets/themes/theme_helper.scss +++ b/app/assets/stylesheets/themes/theme_helper.scss @@ -205,6 +205,10 @@ } } + .emoji-picker-category-active { + border-bottom-color: $active-tab-border; + } + .branch-header-title { color: $border-and-box-shadow; } diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 17138da6e2b..c454ae6eaf4 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -44,6 +44,7 @@ class Projects::IssuesController < Projects::ApplicationController push_frontend_feature_flag(:tribute_autocomplete, @project) push_frontend_feature_flag(:vue_issuables_list, project) push_frontend_feature_flag(:usage_data_design_action, project, default_enabled: true) + push_frontend_feature_flag(:improved_emoji_picker, project, default_enabled: :yaml) end before_action only: :show do diff --git a/app/finders/repositories/changelog_commits_finder.rb b/app/finders/repositories/changelog_commits_finder.rb index 08f1144701a..b80b8e94e59 100644 --- a/app/finders/repositories/changelog_commits_finder.rb +++ b/app/finders/repositories/changelog_commits_finder.rb @@ -93,7 +93,7 @@ module Repositories end def revert_commit_sha(commit) - matches = commit.description.match(REVERT_REGEX) + matches = commit.description&.match(REVERT_REGEX) matches[:sha] if matches end diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index d66a2333d11..7ab5dc36e4a 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -13,8 +13,6 @@ class GitlabSchema < GraphQL::Schema use GraphQL::Pagination::Connections use BatchLoader::GraphQL use Gitlab::Graphql::Authorize - use Gitlab::Graphql::Present - use Gitlab::Graphql::CallsGitaly use Gitlab::Graphql::Pagination::Connections use Gitlab::Graphql::GenericTracing use Gitlab::Graphql::Timeout, max_seconds: Gitlab.config.gitlab.graphql_timeout diff --git a/app/graphql/resolvers/base_resolver.rb b/app/graphql/resolvers/base_resolver.rb index 5db618254cb..67bba079512 100644 --- a/app/graphql/resolvers/base_resolver.rb +++ b/app/graphql/resolvers/base_resolver.rb @@ -12,8 +12,17 @@ module Resolvers @requires_argument = true end + def self.calls_gitaly! + @calls_gitaly = true + end + def self.field_options - super.merge(requires_argument: @requires_argument) + extra_options = { + requires_argument: @requires_argument, + calls_gitaly: @calls_gitaly + }.compact + + super.merge(extra_options) end def self.singular_type diff --git a/app/graphql/resolvers/last_commit_resolver.rb b/app/graphql/resolvers/last_commit_resolver.rb index dd89c322617..00c43bdfee6 100644 --- a/app/graphql/resolvers/last_commit_resolver.rb +++ b/app/graphql/resolvers/last_commit_resolver.rb @@ -4,6 +4,8 @@ module Resolvers class LastCommitResolver < BaseResolver type Types::CommitType, null: true + calls_gitaly! + alias_method :tree, :object def resolve(**args) diff --git a/app/graphql/resolvers/metrics/dashboard_resolver.rb b/app/graphql/resolvers/metrics/dashboard_resolver.rb index f569cb0b2c3..a82a4a95254 100644 --- a/app/graphql/resolvers/metrics/dashboard_resolver.rb +++ b/app/graphql/resolvers/metrics/dashboard_resolver.rb @@ -3,19 +3,30 @@ module Resolvers module Metrics class DashboardResolver < Resolvers::BaseResolver + type Types::Metrics::DashboardType, null: true + calls_gitaly! + argument :path, GraphQL::STRING_TYPE, required: true, - description: "Path to a file which defines metrics dashboard eg: 'config/prometheus/common_metrics.yml'." - - type Types::Metrics::DashboardType, null: true + description: "Path to a file which defines metrics dashboard " \ + "eg: 'config/prometheus/common_metrics.yml'." alias_method :environment, :object def resolve(**args) return unless environment - ::PerformanceMonitoring::PrometheusDashboard - .find_for(project: environment.project, user: context[:current_user], path: args[:path], options: { environment: environment }) + ::PerformanceMonitoring::PrometheusDashboard.find_for(**args, **service_params) + end + + private + + def service_params + { + project: environment.project, + user: current_user, + options: { environment: environment } + } end end end diff --git a/app/graphql/resolvers/snippets/blobs_resolver.rb b/app/graphql/resolvers/snippets/blobs_resolver.rb index 672214df7d5..569b82149d3 100644 --- a/app/graphql/resolvers/snippets/blobs_resolver.rb +++ b/app/graphql/resolvers/snippets/blobs_resolver.rb @@ -8,6 +8,7 @@ module Resolvers type Types::Snippets::BlobType.connection_type, null: true authorize :read_snippet + calls_gitaly! alias_method :snippet, :object diff --git a/app/graphql/resolvers/tree_resolver.rb b/app/graphql/resolvers/tree_resolver.rb index 7a70c35897d..c07d9187d4d 100644 --- a/app/graphql/resolvers/tree_resolver.rb +++ b/app/graphql/resolvers/tree_resolver.rb @@ -4,6 +4,8 @@ module Resolvers class TreeResolver < BaseResolver type Types::Tree::TreeType, null: true + calls_gitaly! + argument :path, GraphQL::STRING_TYPE, required: false, default_value: '', diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index f85675f72f2..78ab6890923 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -9,16 +9,25 @@ module Types DEFAULT_COMPLEXITY = 1 - def initialize(*args, **kwargs, &block) + def initialize(**kwargs, &block) @calls_gitaly = !!kwargs.delete(:calls_gitaly) - @constant_complexity = !!kwargs[:complexity] + @constant_complexity = kwargs[:complexity].is_a?(Integer) && kwargs[:complexity] > 0 @requires_argument = !!kwargs.delete(:requires_argument) kwargs[:complexity] = field_complexity(kwargs[:resolver_class], kwargs[:complexity]) @feature_flag = kwargs[:feature_flag] kwargs = check_feature_flag(kwargs) kwargs = gitlab_deprecation(kwargs) - super(*args, **kwargs, &block) + super(**kwargs, &block) + + # We want to avoid the overhead of this in prod + extension ::Gitlab::Graphql::CallsGitaly::FieldExtension if Gitlab.dev_or_test_env? + + extension ::Gitlab::Graphql::Present::FieldExtension + end + + def may_call_gitaly? + @constant_complexity || @calls_gitaly end def requires_argument? @@ -54,8 +63,10 @@ module Types end def check_feature_flag(args) - args[:description] = feature_documentation_message(args[:feature_flag], args[:description]) if args[:feature_flag].present? - args.delete(:feature_flag) + ff = args.delete(:feature_flag) + return args unless ff.present? + + args[:description] = feature_documentation_message(ff, args[:description]) args end @@ -78,7 +89,9 @@ module Types # items which can be loaded. proc do |ctx, args, child_complexity| # Resolvers may add extra complexity depending on used arguments - complexity = child_complexity + self.resolver&.try(:resolver_complexity, args, child_complexity: child_complexity).to_i + complexity = child_complexity + resolver&.try( + :resolver_complexity, args, child_complexity: child_complexity + ).to_i complexity += 1 if calls_gitaly? complexity += complexity * connection_complexity_multiplier(ctx, args) @@ -93,7 +106,7 @@ module Types page_size = field_defn.connection_max_page_size || ctx.schema.default_max_page_size limit_value = [args[:first], args[:last], page_size].compact.min - multiplier = self.resolver&.try(:complexity_multiplier, args).to_f + multiplier = resolver&.try(:complexity_multiplier, args).to_f limit_value * multiplier end end diff --git a/app/graphql/types/current_user_todos.rb b/app/graphql/types/current_user_todos.rb index 79a430af1d7..2551db875b0 100644 --- a/app/graphql/types/current_user_todos.rb +++ b/app/graphql/types/current_user_todos.rb @@ -16,9 +16,10 @@ module Types end def current_user_todos(state: nil) - state ||= %i(done pending) # TodosFinder treats a `nil` state param as `pending` + state ||= %i[done pending] # TodosFinder treats a `nil` state param as `pending` + klass = unpresented.class - TodosFinder.new(current_user, state: state, type: object.class.name, target_id: object.id).execute + TodosFinder.new(current_user, state: state, type: klass.name, target_id: object.id).execute end end end diff --git a/app/graphql/types/snippets/blob_type.rb b/app/graphql/types/snippets/blob_type.rb index fb0c1d9409b..fb9ee380705 100644 --- a/app/graphql/types/snippets/blob_type.rb +++ b/app/graphql/types/snippets/blob_type.rb @@ -14,7 +14,6 @@ module Types field :plain_data, GraphQL::STRING_TYPE, description: 'Blob plain highlighted data.', - calls_gitaly: true, null: true field :raw_path, GraphQL::STRING_TYPE, diff --git a/app/graphql/types/tree/blob_type.rb b/app/graphql/types/tree/blob_type.rb index 3823bd94083..d192c8d3c57 100644 --- a/app/graphql/types/tree/blob_type.rb +++ b/app/graphql/types/tree/blob_type.rb @@ -15,6 +15,7 @@ module Types field :web_path, GraphQL::STRING_TYPE, null: true, description: 'Web path of the blob.' field :lfs_oid, GraphQL::STRING_TYPE, null: true, + calls_gitaly: true, description: 'LFS ID of the blob.' field :mode, GraphQL::STRING_TYPE, null: true, description: 'Blob mode in numeric format.' diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb index dee91789f50..36c6f2f6c79 100644 --- a/app/mailers/previews/notify_preview.rb +++ b/app/mailers/previews/notify_preview.rb @@ -60,6 +60,10 @@ class NotifyPreview < ActionMailer::Preview end end + def new_mention_in_merge_request_email + Notify.new_mention_in_merge_request_email(user.id, issue.id, user.id).message + end + def closed_issue_email Notify.closed_issue_email(user.id, issue.id, user.id).message end diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index aafba9bfcef..4d7d632ee14 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -20,7 +20,6 @@ module MergeRequests merge_request_activity_counter.track_merge_mr_action(user: current_user) notification_service.merge_mr(merge_request, current_user) execute_hooks(merge_request, 'merge') - retarget_chain_merge_requests(merge_request) invalidate_cache_counts(merge_request, users: merge_request.assignees | merge_request.reviewers) merge_request.update_project_counter_caches delete_non_latest_diffs(merge_request) @@ -31,34 +30,6 @@ module MergeRequests private - def retarget_chain_merge_requests(merge_request) - return unless Feature.enabled?(:retarget_merge_requests, merge_request.target_project) - - # we can only retarget MRs that are targeting the same project - # and have a remove source branch set - return unless merge_request.for_same_project? && merge_request.remove_source_branch? - - # find another merge requests that - # - as a target have a current source project and branch - other_merge_requests = merge_request.source_project - .merge_requests - .opened - .by_target_branch(merge_request.source_branch) - .preload_source_project - .at_most(MAX_RETARGET_MERGE_REQUESTS) - - other_merge_requests.find_each do |other_merge_request| - # Update only MRs on projects that we have access to - next unless can?(current_user, :update_merge_request, other_merge_request.source_project) - - ::MergeRequests::UpdateService - .new(other_merge_request.source_project, current_user, - target_branch: merge_request.target_branch, - target_branch_was_deleted: true) - .execute(other_merge_request) - end - end - def close_issues(merge_request) return unless merge_request.target_branch == project.default_branch diff --git a/app/services/merge_requests/retarget_chain_service.rb b/app/services/merge_requests/retarget_chain_service.rb new file mode 100644 index 00000000000..f24d67243c9 --- /dev/null +++ b/app/services/merge_requests/retarget_chain_service.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module MergeRequests + class RetargetChainService < MergeRequests::BaseService + MAX_RETARGET_MERGE_REQUESTS = 4 + + def execute(merge_request) + return unless Feature.enabled?(:retarget_merge_requests, merge_request.target_project, default_enabled: :yaml) + + # we can only retarget MRs that are targeting the same project + return unless merge_request.for_same_project? && merge_request.merged? + + # find another merge requests that + # - as a target have a current source project and branch + other_merge_requests = merge_request.source_project + .merge_requests + .opened + .by_target_branch(merge_request.source_branch) + .preload_source_project + .at_most(MAX_RETARGET_MERGE_REQUESTS) + + other_merge_requests.find_each do |other_merge_request| + # Update only MRs on projects that we have access to + next unless can?(current_user, :update_merge_request, other_merge_request.source_project) + + ::MergeRequests::UpdateService + .new(other_merge_request.source_project, current_user, + target_branch: merge_request.target_branch, + target_branch_was_deleted: true) + .execute(other_merge_request) + end + end + end +end diff --git a/app/views/admin/groups/show.html.haml b/app/views/admin/groups/show.html.haml index d8128d2fa7e..f8c490dd948 100644 --- a/app/views/admin/groups/show.html.haml +++ b/app/views/admin/groups/show.html.haml @@ -1,3 +1,4 @@ +- add_page_specific_style 'page_bundles/members' - add_to_breadcrumbs _("Groups"), admin_groups_path - breadcrumb_title @group.name - page_title @group.name, _("Groups") diff --git a/app/views/admin/projects/show.html.haml b/app/views/admin/projects/show.html.haml index 2085515e349..40443fb3406 100644 --- a/app/views/admin/projects/show.html.haml +++ b/app/views/admin/projects/show.html.haml @@ -1,3 +1,4 @@ +- add_page_specific_style 'page_bundles/members' - add_to_breadcrumbs _("Projects"), admin_projects_path - breadcrumb_title @project.full_name - page_title @project.full_name, _("Projects") diff --git a/app/views/groups/group_members/index.html.haml b/app/views/groups/group_members/index.html.haml index df39e6297f6..da00879ecf9 100644 --- a/app/views/groups/group_members/index.html.haml +++ b/app/views/groups/group_members/index.html.haml @@ -1,3 +1,4 @@ +- add_page_specific_style 'page_bundles/members' - page_title _('Group members') - can_manage_members = can?(current_user, :admin_group_member, @group) - show_invited_members = can_manage_members && @invited_members.exists? diff --git a/app/views/notify/new_mention_in_merge_request_email.text.erb b/app/views/notify/new_mention_in_merge_request_email.text.erb index 3c78e257a88..0121006852c 100644 --- a/app/views/notify/new_mention_in_merge_request_email.text.erb +++ b/app/views/notify/new_mention_in_merge_request_email.text.erb @@ -4,6 +4,7 @@ You have been mentioned in Merge Request <%= @merge_request.to_reference %> <%= merge_path_description(@merge_request, 'to') %> Author: <%= sanitize_name(@merge_request.author_name) %> -= assignees_label(@merge_request) +<%= assignees_label(@merge_request) %> +<%= reviewers_label(@merge_request) %> <%= @merge_request.description %> diff --git a/app/views/projects/project_members/index.html.haml b/app/views/projects/project_members/index.html.haml index 6bc9bcf6b90..c88dae079ae 100644 --- a/app/views/projects/project_members/index.html.haml +++ b/app/views/projects/project_members/index.html.haml @@ -1,3 +1,4 @@ +- add_page_specific_style 'page_bundles/members' - page_title _("Members") .js-remove-member-modal diff --git a/app/workers/merge_requests/delete_source_branch_worker.rb b/app/workers/merge_requests/delete_source_branch_worker.rb index 99816d5ed0b..eb83d10af33 100644 --- a/app/workers/merge_requests/delete_source_branch_worker.rb +++ b/app/workers/merge_requests/delete_source_branch_worker.rb @@ -16,6 +16,9 @@ class MergeRequests::DeleteSourceBranchWorker ::Branches::DeleteService.new(merge_request.source_project, user) .execute(merge_request.source_branch) + + ::MergeRequests::RetargetChainService.new(merge_request.source_project, user) + .execute(merge_request) rescue ActiveRecord::RecordNotFound end end diff --git a/changelogs/unreleased/297428-code-owner-optional-section.yml b/changelogs/unreleased/297428-code-owner-optional-section.yml new file mode 100644 index 00000000000..7de07ee38e6 --- /dev/null +++ b/changelogs/unreleased/297428-code-owner-optional-section.yml @@ -0,0 +1,5 @@ +--- +title: Update code owner approval tooltip message +merge_request: 55842 +author: +type: changed diff --git a/changelogs/unreleased/300750-add-missing-reviewers-information-to-new_mention_in_merge_request_.yml b/changelogs/unreleased/300750-add-missing-reviewers-information-to-new_mention_in_merge_request_.yml new file mode 100644 index 00000000000..899eb1f6784 --- /dev/null +++ b/changelogs/unreleased/300750-add-missing-reviewers-information-to-new_mention_in_merge_request_.yml @@ -0,0 +1,5 @@ +--- +title: Add reviewers detail to new mention in merge request email +merge_request: 56184 +author: +type: added diff --git a/changelogs/unreleased/changelog-commits-without-description.yml b/changelogs/unreleased/changelog-commits-without-description.yml new file mode 100644 index 00000000000..41f757e5e2b --- /dev/null +++ b/changelogs/unreleased/changelog-commits-without-description.yml @@ -0,0 +1,5 @@ +--- +title: Handle commits without descriptions for changelogs +merge_request: 56224 +author: +type: fixed diff --git a/changelogs/unreleased/cwoolley-gitlab-master-patch-20932.yml b/changelogs/unreleased/cwoolley-gitlab-master-patch-20932.yml new file mode 100644 index 00000000000..86679560713 --- /dev/null +++ b/changelogs/unreleased/cwoolley-gitlab-master-patch-20932.yml @@ -0,0 +1,5 @@ +--- +title: Fix bug in wiki page destroy API endpoint when an error is raised +merge_request: 56285 +author: +type: fixed diff --git a/changelogs/unreleased/improve-retarget-merge-requests.yml b/changelogs/unreleased/improve-retarget-merge-requests.yml new file mode 100644 index 00000000000..f194ef7a151 --- /dev/null +++ b/changelogs/unreleased/improve-retarget-merge-requests.yml @@ -0,0 +1,5 @@ +--- +title: Automatically retarget merge requests upon merge (default on) +merge_request: 56233 +author: +type: added diff --git a/config/application.rb b/config/application.rb index f3dfa95ae1a..e5710edc811 100644 --- a/config/application.rb +++ b/config/application.rb @@ -196,6 +196,7 @@ module Gitlab config.assets.precompile << "page_bundles/jira_connect.css" config.assets.precompile << "page_bundles/jira_connect_users.css" config.assets.precompile << "page_bundles/learn_gitlab.css" + config.assets.precompile << "page_bundles/members.css" config.assets.precompile << "page_bundles/merge_conflicts.css" config.assets.precompile << "page_bundles/merge_requests.css" config.assets.precompile << "page_bundles/milestone.css" diff --git a/config/feature_flags/development/improved_emoji_picker.yml b/config/feature_flags/development/improved_emoji_picker.yml new file mode 100644 index 00000000000..2d72c38630a --- /dev/null +++ b/config/feature_flags/development/improved_emoji_picker.yml @@ -0,0 +1,8 @@ +--- +name: improved_emoji_picker +introduced_by_url: +rollout_issue_url: +milestone: '13.9' +type: development +group: group::code review +default_enabled: false diff --git a/config/feature_flags/development/retarget_merge_requests.yml b/config/feature_flags/development/retarget_merge_requests.yml index 39ac4be0219..cbad472a56e 100644 --- a/config/feature_flags/development/retarget_merge_requests.yml +++ b/config/feature_flags/development/retarget_merge_requests.yml @@ -5,4 +5,4 @@ rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/320895 milestone: '13.9' type: development group: group::memory -default_enabled: false +default_enabled: true diff --git a/doc/user/group/import/index.md b/doc/user/group/import/index.md index 7a7523d7c14..302f12273cb 100644 --- a/doc/user/group/import/index.md +++ b/doc/user/group/import/index.md @@ -57,6 +57,15 @@ The following resources are migrated to the target instance: - due date - created at - updated at +- Iterations ([Introduced in 13.10](https://gitlab.com/gitlab-org/gitlab/-/issues/292428)) + - iid + - title + - description + - state (upcoming / started / closed) + - start date + - due date + - created at + - updated at Any other items are **not** migrated. diff --git a/lib/api/wikis.rb b/lib/api/wikis.rb index 3fa42be47a9..8441aeb10ab 100644 --- a/lib/api/wikis.rb +++ b/lib/api/wikis.rb @@ -111,7 +111,7 @@ module API if response.success? no_content! else - render_api_error!(reponse.message) + unprocessable_entity!(response.message) end end diff --git a/lib/gitlab/graphql/calls_gitaly.rb b/lib/gitlab/graphql/calls_gitaly.rb deleted file mode 100644 index 40cd74a34f2..00000000000 --- a/lib/gitlab/graphql/calls_gitaly.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - # Wraps the field resolution to count Gitaly calls before and after. - # Raises an error if the field calls Gitaly but hadn't declared such. - module CallsGitaly - extend ActiveSupport::Concern - - def self.use(schema_definition) - schema_definition.instrument(:field, Gitlab::Graphql::CallsGitaly::Instrumentation.new, after_built_ins: true) - end - end - end -end diff --git a/lib/gitlab/graphql/calls_gitaly/field_extension.rb b/lib/gitlab/graphql/calls_gitaly/field_extension.rb new file mode 100644 index 00000000000..32530b47ce3 --- /dev/null +++ b/lib/gitlab/graphql/calls_gitaly/field_extension.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module CallsGitaly + # Check if any `calls_gitaly: true` declarations need to be added + # + # See BaseField: this extension is not applied if the field does not + # need it (i.e. it has a constant complexity or knows that it calls + # gitaly) + class FieldExtension < ::GraphQL::Schema::FieldExtension + include Laziness + + def resolve(object:, arguments:, **rest) + yield(object, arguments, [current_gitaly_call_count, accounted_for]) + end + + def after_resolve(value:, memo:, **rest) + (value, count) = value_with_count(value, memo) + calls_gitaly_check(count) + accounted_for(count) + + value + end + + private + + # Resolutions are not nested nicely (due to laziness), so we have to + # know not just how many calls were made before resolution started, but + # also how many were accounted for by fields with the correct settings + # in between. + # + # e.g. the following is not just plausible, but common: + # + # enter A.user (lazy) + # enter A.x + # leave A.x + # enter A.calls_gitaly + # leave A.calls_gitaly (accounts for 1 call) + # leave A.user + # + # In this circumstance we need to mark the calls made by A.calls_gitaly + # as accounted for, even though they were made after we yielded + # in A.user + def value_with_count(value, (previous_count, previous_accounted_for)) + newly_accounted_for = accounted_for - previous_accounted_for + value = force(value) + count = [current_gitaly_call_count - (previous_count + newly_accounted_for), 0].max + + [value, count] + end + + def current_gitaly_call_count + Gitlab::GitalyClient.get_request_count || 0 + end + + def calls_gitaly_check(calls) + return if calls < 1 || field.may_call_gitaly? + + error = RuntimeError.new(<<~ERROR) + #{field_name} unexpectedly calls Gitaly! + + Please either specify a constant complexity or add `calls_gitaly: true` + to the field declaration + ERROR + Gitlab::ErrorTracking.track_and_raise_for_dev_exception(error) + end + + def accounted_for(count = nil) + return 0 unless Gitlab::SafeRequestStore.active? + + Gitlab::SafeRequestStore["graphql_gitaly_accounted_for"] ||= 0 + + if count.nil? + Gitlab::SafeRequestStore["graphql_gitaly_accounted_for"] + else + Gitlab::SafeRequestStore["graphql_gitaly_accounted_for"] += count + end + end + + def field_name + "#{field.owner.graphql_name}.#{field.graphql_name}" + end + end + end + end +end diff --git a/lib/gitlab/graphql/calls_gitaly/instrumentation.rb b/lib/gitlab/graphql/calls_gitaly/instrumentation.rb deleted file mode 100644 index 11d3c50e093..00000000000 --- a/lib/gitlab/graphql/calls_gitaly/instrumentation.rb +++ /dev/null @@ -1,40 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - module CallsGitaly - class Instrumentation - # Check if any `calls_gitaly: true` declarations need to be added - # Do nothing if a constant complexity was provided - def instrument(_type, field) - type_object = field.metadata[:type_class] - return field unless type_object.respond_to?(:calls_gitaly?) - return field if type_object.constant_complexity? || type_object.calls_gitaly? - - old_resolver_proc = field.resolve_proc - - gitaly_wrapped_resolve = -> (typed_object, args, ctx) do - previous_gitaly_call_count = Gitlab::GitalyClient.get_request_count - result = old_resolver_proc.call(typed_object, args, ctx) - current_gitaly_call_count = Gitlab::GitalyClient.get_request_count - calls_gitaly_check(type_object, current_gitaly_call_count - previous_gitaly_call_count) - result - end - - field.redefine do - resolve(gitaly_wrapped_resolve) - end - end - - def calls_gitaly_check(type_object, calls) - return if calls < 1 - - # Will inform you if there needs to be `calls_gitaly: true` as a kwarg in the field declaration - # if there is at least 1 Gitaly call involved with the field resolution. - error = RuntimeError.new("Gitaly is called for field '#{type_object.name}' on #{type_object.owner.try(:name)} - please either specify a constant complexity or add `calls_gitaly: true` to the field declaration") - Gitlab::ErrorTracking.track_and_raise_for_dev_exception(error) - end - end - end - end -end diff --git a/lib/gitlab/graphql/present.rb b/lib/gitlab/graphql/present.rb index 6d86d632ab4..fdaf075eb25 100644 --- a/lib/gitlab/graphql/present.rb +++ b/lib/gitlab/graphql/present.rb @@ -12,11 +12,30 @@ module Gitlab def self.presenter_class @presenter_class end + + def self.present(object, attrs) + klass = @presenter_class + return object if !klass || object.is_a?(klass) + + @presenter_class.new(object, **attrs) + end + end + + def unpresented + unwrapped || object end - def self.use(schema_definition) - schema_definition.instrument(:field, ::Gitlab::Graphql::Present::Instrumentation.new) + def present(object_type, attrs) + return unless object_type.respond_to?(:present) + + self.unwrapped ||= object + # @object belongs to Schema::Object, which does not expose a writer. + @object = object_type.present(unwrapped, attrs) # rubocop: disable Gitlab/ModuleWithInstanceVariables end + + private + + attr_accessor :unwrapped end end end diff --git a/lib/gitlab/graphql/present/field_extension.rb b/lib/gitlab/graphql/present/field_extension.rb new file mode 100644 index 00000000000..2e211b70d35 --- /dev/null +++ b/lib/gitlab/graphql/present/field_extension.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Present + class FieldExtension < ::GraphQL::Schema::FieldExtension + SAFE_CONTEXT_KEYS = %i[current_user].freeze + + def resolve(object:, arguments:, context:) + attrs = safe_context_values(context) + + # We need to handle the object being either a Schema::Object or an + # inner Schema::Object#object. This depends on whether the field + # has a @resolver_proc or not. + if object.is_a?(::Types::BaseObject) + object.present(field.owner, attrs) + yield(object, arguments) + else + # This is the legacy code-path, hit if the field has a @resolver_proc + # TODO: remove this when resolve procs are removed from the + # graphql-ruby library, and all field instrumentation is removed. + # See: https://github.com/rmosolgo/graphql-ruby/issues/3385 + presented = field.owner.try(:present, object, attrs) || object + yield(presented, arguments) + end + end + + private + + def safe_context_values(context) + context.to_h.slice(*SAFE_CONTEXT_KEYS) + end + end + end + end +end diff --git a/lib/gitlab/graphql/present/instrumentation.rb b/lib/gitlab/graphql/present/instrumentation.rb deleted file mode 100644 index b8535575da5..00000000000 --- a/lib/gitlab/graphql/present/instrumentation.rb +++ /dev/null @@ -1,49 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - module Present - class Instrumentation - SAFE_CONTEXT_KEYS = %i[current_user].freeze - - def instrument(type, field) - return field unless field.metadata[:type_class] - - presented_in = field.metadata[:type_class].owner - return field unless presented_in.respond_to?(:presenter_class) - return field unless presented_in.presenter_class - - old_resolver = field.resolve_proc - - resolve_with_presenter = -> (presented_type, args, context) do - # We need to wrap the original presentation type into a type that - # uses the presenter as an object. - object = presented_type.object - - if object.is_a?(presented_in.presenter_class) - next old_resolver.call(presented_type, args, context) - end - - attrs = safe_context_values(context) - presenter = presented_in.presenter_class.new(object, **attrs) - - # we have to use the new `authorized_new` method, as `new` is protected - wrapped = presented_type.class.authorized_new(presenter, context) - - old_resolver.call(wrapped, args, context) - end - - field.redefine do - resolve(resolve_with_presenter) - end - end - - private - - def safe_context_values(context) - context.to_h.slice(*SAFE_CONTEXT_KEYS) - end - end - end - end -end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 07ac698cf83..83113ca541e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4534,6 +4534,9 @@ msgstr "" msgid "Average per day: %{average}" msgstr "" +msgid "AwardEmoji|No emojis found." +msgstr "" + msgid "Back to page %{number}" msgstr "" @@ -24557,7 +24560,7 @@ msgstr "" msgid "ProtectedBranch|Code owner approval" msgstr "" -msgid "ProtectedBranch|Does not apply to users allowed to push." +msgid "ProtectedBranch|Does not apply to users allowed to push. Optional sections are not enforced." msgstr "" msgid "ProtectedBranch|Protect" @@ -30553,7 +30556,7 @@ msgstr "" msgid "This chart could not be displayed" msgstr "" -msgid "This comment has changed since you started editing, please review the %{startTag}updated comment%{endTag} to ensure information is not lost." +msgid "This comment changed after you started editing it. Review the %{startTag}updated comment%{endTag} to ensure information is not lost." msgstr "" msgid "This commit is part of merge request %{link_to_merge_request}. Comments created here will be created in the context of that merge request." diff --git a/spec/features/issues/user_interacts_with_awards_spec.rb b/spec/features/issues/user_interacts_with_awards_spec.rb index 181d6f65b78..1c7bc5f239f 100644 --- a/spec/features/issues/user_interacts_with_awards_spec.rb +++ b/spec/features/issues/user_interacts_with_awards_spec.rb @@ -135,11 +135,9 @@ RSpec.describe 'User interacts with awards' do it 'allows adding a new emoji' do page.within('.note-actions') do - find('.btn.js-add-award').click - end - page.within('.emoji-menu-content') do - find('gl-emoji[data-name="8ball"]').click + find('.note-emoji-button').click end + find('gl-emoji[data-name="8ball"]').click wait_for_requests page.within('.note-awards') do diff --git a/spec/finders/repositories/changelog_commits_finder_spec.rb b/spec/finders/repositories/changelog_commits_finder_spec.rb index fe40666a955..8665d36144a 100644 --- a/spec/finders/repositories/changelog_commits_finder_spec.rb +++ b/spec/finders/repositories/changelog_commits_finder_spec.rb @@ -64,4 +64,30 @@ RSpec.describe Repositories::ChangelogCommitsFinder do expect(commits.count).to eq(4) end end + + describe '#revert_commit_sha' do + let(:finder) { described_class.new(project: project, from: 'a', to: 'b') } + + it 'returns the SHA of a reverted commit' do + commit = double( + :commit, + description: 'This reverts commit 152c03af1b09f50fa4b567501032b106a3a81ff3.' + ) + + expect(finder.send(:revert_commit_sha, commit)) + .to eq('152c03af1b09f50fa4b567501032b106a3a81ff3') + end + + it 'returns nil when the commit is not a revert commit' do + commit = double(:commit, description: 'foo') + + expect(finder.send(:revert_commit_sha, commit)).to be_nil + end + + it 'returns nil when the commit has no description' do + commit = double(:commit, description: nil) + + expect(finder.send(:revert_commit_sha, commit)).to be_nil + end + end end diff --git a/spec/frontend/emoji/components/category_spec.js b/spec/frontend/emoji/components/category_spec.js new file mode 100644 index 00000000000..afd36a1eb88 --- /dev/null +++ b/spec/frontend/emoji/components/category_spec.js @@ -0,0 +1,49 @@ +import { GlIntersectionObserver } from '@gitlab/ui'; +import { shallowMount } from '@vue/test-utils'; +import { nextTick } from 'vue'; +import Category from '~/emoji/components/category.vue'; +import EmojiGroup from '~/emoji/components/emoji_group.vue'; + +let wrapper; +function factory(propsData = {}) { + wrapper = shallowMount(Category, { propsData }); +} + +describe('Emoji category component', () => { + afterEach(() => { + wrapper.destroy(); + }); + + beforeEach(() => { + factory({ + category: 'Activity', + emojis: [['thumbsup'], ['thumbsdown']], + }); + }); + + it('renders emoji groups', () => { + expect(wrapper.findAll(EmojiGroup).length).toBe(2); + }); + + it('renders group', async () => { + await wrapper.setData({ renderGroup: true }); + + expect(wrapper.find(EmojiGroup).attributes('rendergroup')).toBe('true'); + }); + + it('renders group on appear', async () => { + wrapper.find(GlIntersectionObserver).vm.$emit('appear'); + + await nextTick(); + + expect(wrapper.find(EmojiGroup).attributes('rendergroup')).toBe('true'); + }); + + it('emits appear event on appear', async () => { + wrapper.find(GlIntersectionObserver).vm.$emit('appear'); + + await nextTick(); + + expect(wrapper.emitted().appear[0]).toEqual(['Activity']); + }); +}); diff --git a/spec/frontend/emoji/components/emoji_group_spec.js b/spec/frontend/emoji/components/emoji_group_spec.js new file mode 100644 index 00000000000..1aca2fbb8fc --- /dev/null +++ b/spec/frontend/emoji/components/emoji_group_spec.js @@ -0,0 +1,56 @@ +import { shallowMount } from '@vue/test-utils'; +import Vue from 'vue'; +import { extendedWrapper } from 'helpers/vue_test_utils_helper'; +import EmojiGroup from '~/emoji/components/emoji_group.vue'; + +Vue.config.ignoredElements = ['gl-emoji']; + +let wrapper; +function factory(propsData = {}) { + wrapper = extendedWrapper( + shallowMount(EmojiGroup, { + propsData, + }), + ); +} + +describe('Emoji group component', () => { + afterEach(() => { + wrapper.destroy(); + }); + + it('does not render any buttons', () => { + factory({ + emojis: [], + renderGroup: false, + clickEmoji: jest.fn(), + }); + + expect(wrapper.findByTestId('emoji-button').exists()).toBe(false); + }); + + it('renders emojis', () => { + factory({ + emojis: ['thumbsup', 'thumbsdown'], + renderGroup: true, + clickEmoji: jest.fn(), + }); + + expect(wrapper.findAllByTestId('emoji-button').exists()).toBe(true); + expect(wrapper.findAllByTestId('emoji-button').length).toBe(2); + }); + + it('calls clickEmoji', () => { + const clickEmoji = jest.fn(); + + factory({ + emojis: ['thumbsup', 'thumbsdown'], + renderGroup: true, + clickEmoji, + }); + + wrapper.findByTestId('emoji-button').trigger('click'); + + expect(clickEmoji).toHaveBeenCalledWith('thumbsup'); + }); +}); diff --git a/spec/frontend/emoji/components/emoji_list_spec.js b/spec/frontend/emoji/components/emoji_list_spec.js new file mode 100644 index 00000000000..9dc73ef191e --- /dev/null +++ b/spec/frontend/emoji/components/emoji_list_spec.js @@ -0,0 +1,73 @@ +import { shallowMount } from '@vue/test-utils'; +import { nextTick } from 'vue'; +import { extendedWrapper } from 'helpers/vue_test_utils_helper'; +import EmojiList from '~/emoji/components/emoji_list.vue'; + +jest.mock('~/emoji', () => ({ + initEmojiMap: jest.fn(() => Promise.resolve()), + searchEmoji: jest.fn((search) => [{ emoji: { name: search } }]), + getEmojiCategoryMap: jest.fn(() => + Promise.resolve({ + activity: ['thumbsup', 'thumbsdown'], + }), + ), +})); + +let wrapper; +async function factory(render, propsData = { searchValue: '' }) { + wrapper = extendedWrapper( + shallowMount(EmojiList, { + propsData, + scopedSlots: { + default: '<div data-testid="default-slot">{{props.filteredCategories}}</div>', + }, + }), + ); + + // Wait for categories to be set + await nextTick(); + + if (render) { + wrapper.setData({ render: true }); + + // Wait for component to render + await nextTick(); + } +} + +const findDefaultSlot = () => wrapper.findByTestId('default-slot'); + +describe('Emoji list component', () => { + afterEach(() => { + wrapper.destroy(); + }); + + it('does not render until render is set', async () => { + await factory(false); + + expect(findDefaultSlot().exists()).toBe(false); + }); + + it('renders with none filtered list', async () => { + await factory(true); + + expect(JSON.parse(findDefaultSlot().text())).toEqual({ + activity: { + emojis: [['thumbsup', 'thumbsdown']], + height: expect.any(Number), + top: expect.any(Number), + }, + }); + }); + + it('renders filtered list of emojis', async () => { + await factory(true, { searchValue: 'smile' }); + + expect(JSON.parse(findDefaultSlot().text())).toEqual({ + search: { + emojis: [['smile']], + height: expect.any(Number), + }, + }); + }); +}); diff --git a/spec/frontend/notes/components/note_actions_spec.js b/spec/frontend/notes/components/note_actions_spec.js index f56be1a9af7..cc41088e21e 100644 --- a/spec/frontend/notes/components/note_actions_spec.js +++ b/spec/frontend/notes/components/note_actions_spec.js @@ -48,6 +48,7 @@ describe('noteActions', () => { projectName: 'project', reportAbusePath: `${TEST_HOST}/abuse_reports/new?ref_url=http%3A%2F%2Flocalhost%3A3000%2Fgitlab-org%2Fgitlab-ce%2Fissues%2F7%23note_539&user_id=26`, showReply: false, + awardPath: `${TEST_HOST}/award_emoji`, }; actions = { diff --git a/spec/frontend/notes/components/note_form_spec.js b/spec/frontend/notes/components/note_form_spec.js index 7615f3b70f1..92137d3190f 100644 --- a/spec/frontend/notes/components/note_form_spec.js +++ b/spec/frontend/notes/components/note_form_spec.js @@ -83,7 +83,7 @@ describe('issue_note_form component', () => { }); const message = - 'This comment has changed since you started editing, please review the updated comment to ensure information is not lost.'; + 'This comment changed after you started editing it. Review the updated comment to ensure information is not lost.'; await nextTick(); diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb index 4db12643069..cb2bb25b098 100644 --- a/spec/graphql/gitlab_schema_spec.rb +++ b/spec/graphql/gitlab_schema_spec.rb @@ -18,14 +18,6 @@ RSpec.describe GitlabSchema do expect(field_instrumenters).to include(instance_of(::Gitlab::Graphql::Authorize::Instrumentation)) end - it 'enables using presenters' do - expect(field_instrumenters).to include(instance_of(::Gitlab::Graphql::Present::Instrumentation)) - end - - it 'enables using gitaly call checker' do - expect(field_instrumenters).to include(instance_of(::Gitlab::Graphql::CallsGitaly::Instrumentation)) - end - it 'has the base mutation' do expect(described_class.mutation).to eq(::Types::MutationType) end @@ -47,7 +39,7 @@ RSpec.describe GitlabSchema do end describe '.execute' do - context 'for different types of users' do + context 'with different types of users' do context 'when no context' do it 'returns DEFAULT_MAX_COMPLEXITY' do expect(GraphQL::Schema) @@ -78,13 +70,15 @@ RSpec.describe GitlabSchema do context 'when a logged in user' do it 'returns AUTHENTICATED_COMPLEXITY' do - expect(GraphQL::Schema).to receive(:execute).with('query', hash_including(max_complexity: GitlabSchema::AUTHENTICATED_COMPLEXITY)) + expect(GraphQL::Schema).to receive(:execute) + .with('query', hash_including(max_complexity: GitlabSchema::AUTHENTICATED_COMPLEXITY)) described_class.execute('query', context: { current_user: user }) end it 'returns AUTHENTICATED_MAX_DEPTH' do - expect(GraphQL::Schema).to receive(:execute).with('query', hash_including(max_depth: GitlabSchema::AUTHENTICATED_MAX_DEPTH)) + expect(GraphQL::Schema).to receive(:execute) + .with('query', hash_including(max_depth: GitlabSchema::AUTHENTICATED_MAX_DEPTH)) described_class.execute('query', context: { current_user: user }) end @@ -94,7 +88,8 @@ RSpec.describe GitlabSchema do it 'returns ADMIN_COMPLEXITY' do user = build :user, :admin - expect(GraphQL::Schema).to receive(:execute).with('query', hash_including(max_complexity: GitlabSchema::ADMIN_COMPLEXITY)) + expect(GraphQL::Schema).to receive(:execute) + .with('query', hash_including(max_complexity: GitlabSchema::ADMIN_COMPLEXITY)) described_class.execute('query', context: { current_user: user }) end @@ -130,7 +125,7 @@ RSpec.describe GitlabSchema do end describe '.object_from_id' do - context 'for subclasses of `ApplicationRecord`' do + context 'with subclasses of `ApplicationRecord`' do let_it_be(:user) { create(:user) } it 'returns the correct record' do @@ -162,7 +157,7 @@ RSpec.describe GitlabSchema do end end - context 'for classes that are not ActiveRecord subclasses and have implemented .lazy_find' do + context 'with classes that are not ActiveRecord subclasses and have implemented .lazy_find' do it 'returns the correct record' do note = create(:discussion_note_on_merge_request) @@ -182,7 +177,7 @@ RSpec.describe GitlabSchema do end end - context 'for other classes' do + context 'with other classes' do # We cannot use an anonymous class here as `GlobalID` expects `.name` not # to return `nil` before do diff --git a/spec/lib/bulk_imports/importers/group_importer_spec.rb b/spec/lib/bulk_imports/importers/group_importer_spec.rb index 820a0e03c3d..5d501b49e41 100644 --- a/spec/lib/bulk_imports/importers/group_importer_spec.rb +++ b/spec/lib/bulk_imports/importers/group_importer_spec.rb @@ -27,6 +27,8 @@ RSpec.describe BulkImports::Importers::GroupImporter do if Gitlab.ee? expect_to_run_pipeline('EE::BulkImports::Groups::Pipelines::EpicsPipeline'.constantize, context: context) expect_to_run_pipeline('EE::BulkImports::Groups::Pipelines::EpicAwardEmojiPipeline'.constantize, context: context) + expect_to_run_pipeline('EE::BulkImports::Groups::Pipelines::EpicEventsPipeline'.constantize, context: context) + expect_to_run_pipeline('EE::BulkImports::Groups::Pipelines::IterationsPipeline'.constantize, context: context) end subject.execute diff --git a/spec/lib/gitlab/graphql/calls_gitaly/field_extension_spec.rb b/spec/lib/gitlab/graphql/calls_gitaly/field_extension_spec.rb new file mode 100644 index 00000000000..1d8849f7e38 --- /dev/null +++ b/spec/lib/gitlab/graphql/calls_gitaly/field_extension_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Gitlab::Graphql::CallsGitaly::FieldExtension, :request_store do + include GraphqlHelpers + + let(:field_args) { {} } + let(:owner) { fresh_object_type } + let(:field) do + ::Types::BaseField.new(name: 'value', type: GraphQL::STRING_TYPE, null: true, owner: owner, **field_args) + end + + def resolve_value + resolve_field(field, { value: 'foo' }, object_type: owner) + end + + context 'when the field calls gitaly' do + before do + owner.define_method :value do + Gitlab::SafeRequestStore['gitaly_call_actual'] = 1 + 'fresh-from-the-gitaly-mines!' + end + end + + context 'when the field has a constant complexity' do + let(:field_args) { { complexity: 100 } } + + it 'allows the call' do + expect { resolve_value }.not_to raise_error + end + end + + context 'when the field declares that it calls gitaly' do + let(:field_args) { { calls_gitaly: true } } + + it 'allows the call' do + expect { resolve_value }.not_to raise_error + end + end + + context 'when the field does not have these arguments' do + let(:field_args) { {} } + + it 'notices, and raises, mentioning the field' do + expect { resolve_value }.to raise_error(include('Object.value')) + end + end + end + + context 'when it does not call gitaly' do + let(:field_args) { {} } + + it 'does not raise' do + value = resolve_value + + expect(value).to eq 'foo' + end + end + + context 'when some field calls gitaly while we were waiting' do + let(:extension) { described_class.new(field: field, options: {}) } + + it 'is acceptable if all are accounted for' do + object = :anything + arguments = :any_args + + ::Gitlab::SafeRequestStore['gitaly_call_actual'] = 3 + ::Gitlab::SafeRequestStore['graphql_gitaly_accounted_for'] = 0 + + expect do |b| + extension.resolve(object: object, arguments: arguments, &b) + end.to yield_with_args(object, arguments, [3, 0]) + + ::Gitlab::SafeRequestStore['gitaly_call_actual'] = 13 + ::Gitlab::SafeRequestStore['graphql_gitaly_accounted_for'] = 10 + + expect { extension.after_resolve(value: 'foo', memo: [3, 0]) }.not_to raise_error + end + + it 'is unacceptable if some of the calls are unaccounted for' do + ::Gitlab::SafeRequestStore['gitaly_call_actual'] = 10 + ::Gitlab::SafeRequestStore['graphql_gitaly_accounted_for'] = 9 + + expect { extension.after_resolve(value: 'foo', memo: [0, 0]) }.to raise_error(include('Object.value')) + end + end +end diff --git a/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb b/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb deleted file mode 100644 index f16767f7d14..00000000000 --- a/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -# frozen_string_literal: true -require 'spec_helper' - -RSpec.describe Gitlab::Graphql::CallsGitaly::Instrumentation do - subject { described_class.new } - - describe '#calls_gitaly_check' do - let(:gitaly_field) { Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, calls_gitaly: true) } - let(:no_gitaly_field) { Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, calls_gitaly: false) } - - context 'if there are no Gitaly calls' do - it 'does not raise an error if calls_gitaly is false' do - expect { subject.send(:calls_gitaly_check, no_gitaly_field, 0) }.not_to raise_error - end - end - - context 'if there is at least 1 Gitaly call' do - it 'raises an error if calls_gitaly: is false or not defined' do - expect { subject.send(:calls_gitaly_check, no_gitaly_field, 1) }.to raise_error(/specify a constant complexity or add `calls_gitaly: true`/) - end - end - end -end diff --git a/spec/lib/gitlab/graphql/present/field_extension_spec.rb b/spec/lib/gitlab/graphql/present/field_extension_spec.rb new file mode 100644 index 00000000000..5e66e16d655 --- /dev/null +++ b/spec/lib/gitlab/graphql/present/field_extension_spec.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Gitlab::Graphql::Present::FieldExtension do + include GraphqlHelpers + + let_it_be(:user) { create(:user) } + + let(:object) { double(value: 'foo') } + let(:owner) { fresh_object_type } + let(:field_name) { 'value' } + let(:field) do + ::Types::BaseField.new(name: field_name, type: GraphQL::STRING_TYPE, null: true, owner: owner) + end + + let(:base_presenter) do + Class.new(SimpleDelegator) do + def initialize(object, **options) + super(object) + @object = object + @options = options + end + end + end + + def resolve_value + resolve_field(field, object, current_user: user, object_type: owner) + end + + context 'when the object does not declare a presenter' do + it 'does not affect normal resolution' do + expect(resolve_value).to eq 'foo' + end + end + + describe 'interactions with inheritance' do + def parent + type = fresh_object_type('Parent') + type.present_using(provide_foo) + type.field :foo, ::GraphQL::INT_TYPE, null: true + type.field :value, ::GraphQL::STRING_TYPE, null: true + type + end + + def child + type = Class.new(parent) + type.graphql_name 'Child' + type.present_using(provide_bar) + type.field :bar, ::GraphQL::INT_TYPE, null: true + type + end + + def provide_foo + Class.new(base_presenter) do + def foo + 100 + end + end + end + + def provide_bar + Class.new(base_presenter) do + def bar + 101 + end + end + end + + it 'can resolve value, foo and bar' do + type = child + value = resolve_field(:value, object, object_type: type) + foo = resolve_field(:foo, object, object_type: type) + bar = resolve_field(:bar, object, object_type: type) + + expect([value, foo, bar]).to eq ['foo', 100, 101] + end + end + + shared_examples 'calling the presenter method' do + it 'calls the presenter method' do + expect(resolve_value).to eq presenter.new(object, current_user: user).send(field_name) + end + end + + context 'when the object declares a presenter' do + before do + owner.present_using(presenter) + end + + context 'when the presenter overrides the original method' do + def twice + Class.new(base_presenter) do + def value + @object.value * 2 + end + end + end + + let(:presenter) { twice } + + it_behaves_like 'calling the presenter method' + end + + # This is exercised here using an explicit `resolve:` proc, but + # @resolver_proc values are used in field instrumentation as well. + context 'when the field uses a resolve proc' do + let(:presenter) { base_presenter } + let(:field) do + ::Types::BaseField.new( + name: field_name, + type: GraphQL::STRING_TYPE, + null: true, + owner: owner, + resolve: ->(obj, args, ctx) { 'Hello from a proc' } + ) + end + + specify { expect(resolve_value).to eq 'Hello from a proc' } + end + + context 'when the presenter provides a new method' do + def presenter + Class.new(base_presenter) do + def current_username + "Hello #{@options[:current_user]&.username} from the presenter!" + end + end + end + + context 'when we select the original field' do + it 'is unaffected' do + expect(resolve_value).to eq 'foo' + end + end + + context 'when we select the new field' do + let(:field_name) { 'current_username' } + + it_behaves_like 'calling the presenter method' + end + end + end +end diff --git a/spec/mailers/emails/merge_requests_spec.rb b/spec/mailers/emails/merge_requests_spec.rb index cf191df5378..0c0dae6d7e6 100644 --- a/spec/mailers/emails/merge_requests_spec.rb +++ b/spec/mailers/emails/merge_requests_spec.rb @@ -24,6 +24,23 @@ RSpec.describe Emails::MergeRequests do let(:recipient) { assignee } let(:current_user_sanitized) { 'www_example_com' } + describe '#new_mention_in_merge_request_email' do + subject { Notify.new_mention_in_merge_request_email(recipient.id, merge_request.id, current_user.id) } + + it 'has the correct subject and body' do + aggregate_failures do + is_expected.to have_referable_subject(merge_request, reply: true) + is_expected.to have_body_text(project_merge_request_path(project, merge_request)) + is_expected.to have_body_text('You have been mentioned in Merge Request') + is_expected.to have_link(merge_request.to_reference, href: project_merge_request_url(merge_request.target_project, merge_request)) + is_expected.to have_text_part_content(assignee.name) + is_expected.to have_text_part_content(reviewer.name) + is_expected.to have_html_part_content(assignee.name) + is_expected.to have_html_part_content(reviewer.name) + end + end + end + describe '#merge_request_unmergeable_email' do subject { Notify.merge_request_unmergeable_email(recipient.id, merge_request.id) } diff --git a/spec/requests/api/graphql/issue/issue_spec.rb b/spec/requests/api/graphql/issue/issue_spec.rb index 09e89f65882..e8b8caf6c2d 100644 --- a/spec/requests/api/graphql/issue/issue_spec.rb +++ b/spec/requests/api/graphql/issue/issue_spec.rb @@ -8,10 +8,9 @@ RSpec.describe 'Query.issue(id)' do let_it_be(:project) { create(:project) } let_it_be(:issue) { create(:issue, project: project) } let_it_be(:current_user) { create(:user) } + let_it_be(:issue_params) { { 'id' => issue.to_global_id.to_s } } let(:issue_data) { graphql_data['issue'] } - - let_it_be(:issue_params) { { 'id' => issue.to_global_id.to_s } } let(:issue_fields) { all_graphql_fields_for('Issue'.classify) } let(:query) do @@ -62,7 +61,7 @@ RSpec.describe 'Query.issue(id)' do ) end - context 'selecting any single field' do + context 'when selecting any single field' do where(:field) do scalar_fields_of('Issue').map { |name| [name] } end @@ -84,13 +83,13 @@ RSpec.describe 'Query.issue(id)' do end end - context 'selecting multiple fields' do + context 'when selecting multiple fields' do let(:issue_fields) { ['title', 'description', 'updatedBy { username }'] } it 'returns the Issue with the specified fields' do post_graphql(query, current_user: current_user) - expect(issue_data.keys).to eq( %w(title description updatedBy) ) + expect(issue_data.keys).to eq %w[title description updatedBy] expect(issue_data['title']).to eq(issue.title) expect(issue_data['description']).to eq(issue.description) expect(issue_data['updatedBy']['username']).to eq(issue.author.username) @@ -110,14 +109,14 @@ RSpec.describe 'Query.issue(id)' do it 'returns correct attributes' do post_graphql(query, current_user: current_user) - expect(issue_data.keys).to eq( %w(moved movedTo) ) + expect(issue_data.keys).to eq %w[moved movedTo] expect(issue_data['moved']).to eq(true) expect(issue_data['movedTo']['title']).to eq(new_issue.title) end end context 'when passed a non-Issue gid' do - let(:mr) {create(:merge_request)} + let(:mr) { create(:merge_request) } it 'returns an error' do gid = mr.to_global_id.to_s diff --git a/spec/requests/api/wikis_spec.rb b/spec/requests/api/wikis_spec.rb index f271f8aa853..d35aab40ca9 100644 --- a/spec/requests/api/wikis_spec.rb +++ b/spec/requests/api/wikis_spec.rb @@ -14,6 +14,7 @@ require 'spec_helper' RSpec.describe API::Wikis do include WorkhorseHelpers + include AfterNextHelpers let(:user) { create(:user) } let(:group) { create(:group).tap { |g| g.add_owner(user) } } @@ -578,6 +579,20 @@ RSpec.describe API::Wikis do include_examples 'wiki API 404 Wiki Page Not Found' end end + + context 'when there is an error deleting the page' do + it 'returns 422' do + project.add_maintainer(user) + + allow_next(WikiPages::DestroyService, current_user: user, container: project) + .to receive(:execute).and_return(ServiceResponse.error(message: 'foo')) + + delete(api(url, user)) + + expect(response).to have_gitlab_http_status(:unprocessable_entity) + expect(json_response['message']).to eq 'foo' + end + end end context 'when wiki belongs to a group project' do diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 71329905558..247b053e729 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -130,139 +130,5 @@ RSpec.describe MergeRequests::PostMergeService do expect(deploy_job.reload.canceled?).to be false end end - - context 'for a merge request chain' do - before do - ::MergeRequests::UpdateService - .new(project, user, force_remove_source_branch: '1') - .execute(merge_request) - end - - context 'when there is another MR' do - let!(:another_merge_request) do - create(:merge_request, - source_project: source_project, - source_branch: 'my-awesome-feature', - target_project: merge_request.source_project, - target_branch: merge_request.source_branch - ) - end - - shared_examples 'does not retarget merge request' do - it 'another merge request is unchanged' do - expect { subject }.not_to change { another_merge_request.reload.target_branch } - .from(merge_request.source_branch) - end - end - - shared_examples 'retargets merge request' do - it 'another merge request is retargeted' do - expect(SystemNoteService) - .to receive(:change_branch).once - .with(another_merge_request, another_merge_request.project, user, - 'target', 'delete', - merge_request.source_branch, merge_request.target_branch) - - expect { subject }.to change { another_merge_request.reload.target_branch } - .from(merge_request.source_branch) - .to(merge_request.target_branch) - end - - context 'when FF retarget_merge_requests is disabled' do - before do - stub_feature_flags(retarget_merge_requests: false) - end - - include_examples 'does not retarget merge request' - end - - context 'when source branch is to be kept' do - before do - ::MergeRequests::UpdateService - .new(project, user, force_remove_source_branch: false) - .execute(merge_request) - end - - include_examples 'does not retarget merge request' - end - end - - context 'in the same project' do - let(:source_project) { project } - - it_behaves_like 'retargets merge request' - - context 'and is closed' do - before do - another_merge_request.close - end - - it_behaves_like 'does not retarget merge request' - end - - context 'and is merged' do - before do - another_merge_request.mark_as_merged - end - - it_behaves_like 'does not retarget merge request' - end - end - - context 'in forked project' do - let!(:source_project) { fork_project(project) } - - context 'when user has access to source project' do - before do - source_project.add_developer(user) - end - - it_behaves_like 'retargets merge request' - end - - context 'when user does not have access to source project' do - it_behaves_like 'does not retarget merge request' - end - end - - context 'and current and another MR is from a fork' do - let(:project) { create(:project) } - let(:source_project) { fork_project(project) } - - let(:merge_request) do - create(:merge_request, - source_project: source_project, - target_project: project - ) - end - - before do - source_project.add_developer(user) - end - - it_behaves_like 'does not retarget merge request' - end - end - - context 'when many merge requests are to be retargeted' do - let!(:many_merge_requests) do - create_list(:merge_request, 10, :unique_branches, - source_project: merge_request.source_project, - target_project: merge_request.source_project, - target_branch: merge_request.source_branch - ) - end - - it 'retargets only 4 of them' do - subject - - expect(many_merge_requests.each(&:reload).pluck(:target_branch).tally) - .to eq( - merge_request.source_branch => 6, - merge_request.target_branch => 4 - ) - end - end - end end end diff --git a/spec/services/merge_requests/retarget_chain_service_spec.rb b/spec/services/merge_requests/retarget_chain_service_spec.rb new file mode 100644 index 00000000000..3937fbe58c3 --- /dev/null +++ b/spec/services/merge_requests/retarget_chain_service_spec.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::RetargetChainService do + include ProjectForksHelper + + let_it_be(:user) { create(:user) } + let_it_be(:merge_request, reload: true) { create(:merge_request, assignees: [user]) } + let_it_be(:project) { merge_request.project } + + subject { described_class.new(project, user).execute(merge_request) } + + before do + project.add_maintainer(user) + end + + describe '#execute' do + context 'when there is another MR' do + let!(:another_merge_request) do + create(:merge_request, + source_project: source_project, + source_branch: 'my-awesome-feature', + target_project: merge_request.source_project, + target_branch: merge_request.source_branch + ) + end + + shared_examples 'does not retarget merge request' do + it 'another merge request is unchanged' do + expect { subject }.not_to change { another_merge_request.reload.target_branch } + .from(merge_request.source_branch) + end + end + + shared_examples 'retargets merge request' do + it 'another merge request is retargeted' do + expect(SystemNoteService) + .to receive(:change_branch).once + .with(another_merge_request, another_merge_request.project, user, + 'target', 'delete', + merge_request.source_branch, merge_request.target_branch) + + expect { subject }.to change { another_merge_request.reload.target_branch } + .from(merge_request.source_branch) + .to(merge_request.target_branch) + end + + context 'when FF retarget_merge_requests is disabled' do + before do + stub_feature_flags(retarget_merge_requests: false) + end + + include_examples 'does not retarget merge request' + end + end + + context 'in the same project' do + let(:source_project) { project } + + context 'and current is merged' do + before do + merge_request.mark_as_merged + end + + it_behaves_like 'retargets merge request' + end + + context 'and current is closed' do + before do + merge_request.close + end + + it_behaves_like 'does not retarget merge request' + end + + context 'and another is closed' do + before do + another_merge_request.close + end + + it_behaves_like 'does not retarget merge request' + end + + context 'and another is merged' do + before do + another_merge_request.mark_as_merged + end + + it_behaves_like 'does not retarget merge request' + end + end + + context 'in forked project' do + let!(:source_project) { fork_project(project) } + + context 'when user has access to source project' do + before do + source_project.add_developer(user) + merge_request.mark_as_merged + end + + it_behaves_like 'retargets merge request' + end + + context 'when user does not have access to source project' do + it_behaves_like 'does not retarget merge request' + end + end + + context 'and current and another MR is from a fork' do + let(:project) { create(:project) } + let(:source_project) { fork_project(project) } + + let(:merge_request) do + create(:merge_request, + source_project: source_project, + target_project: project + ) + end + + before do + source_project.add_developer(user) + end + + it_behaves_like 'does not retarget merge request' + end + end + + context 'when many merge requests are to be retargeted' do + let!(:many_merge_requests) do + create_list(:merge_request, 10, :unique_branches, + source_project: merge_request.source_project, + target_project: merge_request.source_project, + target_branch: merge_request.source_branch + ) + end + + before do + merge_request.mark_as_merged + end + + it 'retargets only 4 of them' do + subject + + expect(many_merge_requests.each(&:reload).pluck(:target_branch).tally) + .to eq( + merge_request.source_branch => 6, + merge_request.target_branch => 4 + ) + end + end + end +end diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index 0e05ca320ed..75d9508f470 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/spec/support/helpers/graphql_helpers.rb @@ -38,7 +38,10 @@ module GraphqlHelpers # All resolution goes through fields, so we need to create one here that # uses our resolver. Thankfully, apart from the field name, resolvers # contain all the configuration needed to define one. - field_options = resolver_class.field_options.merge(name: 'field_value') + field_options = resolver_class.field_options.merge( + owner: resolver_parent, + name: 'field_value' + ) field = ::Types::BaseField.new(**field_options) # All mutations accept a single `:input` argument. Wrap arguments here. diff --git a/spec/workers/merge_requests/delete_source_branch_worker_spec.rb b/spec/workers/merge_requests/delete_source_branch_worker_spec.rb index df5773d55c1..957adbbbd6e 100644 --- a/spec/workers/merge_requests/delete_source_branch_worker_spec.rb +++ b/spec/workers/merge_requests/delete_source_branch_worker_spec.rb @@ -13,6 +13,7 @@ RSpec.describe MergeRequests::DeleteSourceBranchWorker do context 'with a non-existing merge request' do it 'does nothing' do expect(::Branches::DeleteService).not_to receive(:new) + expect(::MergeRequests::RetargetChainService).not_to receive(:new) worker.perform(non_existing_record_id, sha, user.id) end @@ -21,6 +22,7 @@ RSpec.describe MergeRequests::DeleteSourceBranchWorker do context 'with a non-existing user' do it 'does nothing' do expect(::Branches::DeleteService).not_to receive(:new) + expect(::MergeRequests::RetargetChainService).not_to receive(:new) worker.perform(merge_request.id, sha, non_existing_record_id) end @@ -35,9 +37,18 @@ RSpec.describe MergeRequests::DeleteSourceBranchWorker do worker.perform(merge_request.id, sha, user.id) end + it 'calls service to try retarget merge requests' do + expect_next_instance_of(::MergeRequests::RetargetChainService) do |instance| + expect(instance).to receive(:execute).with(merge_request) + end + + worker.perform(merge_request.id, sha, user.id) + end + context 'source branch sha does not match' do it 'does nothing' do expect(::Branches::DeleteService).not_to receive(:new) + expect(::MergeRequests::RetargetChainService).not_to receive(:new) worker.perform(merge_request.id, 'new-source-branch-sha', user.id) end |