diff options
37 files changed, 1396 insertions, 224 deletions
diff --git a/app/assets/javascripts/notes/components/toggle_replies_widget.vue b/app/assets/javascripts/notes/components/toggle_replies_widget.vue index 734e08dd586..4437d461308 100644 --- a/app/assets/javascripts/notes/components/toggle_replies_widget.vue +++ b/app/assets/javascripts/notes/components/toggle_replies_widget.vue @@ -79,7 +79,7 @@ export default { :link-href="author.path" :img-alt="author.name" img-css-classes="gl-mr-0!" - :img-src="author.avatar_url" + :img-src="author.avatar_url || author.avatarUrl" :img-size="24" :tooltip-text="author.name" tooltip-placement="bottom" @@ -102,7 +102,10 @@ export default { </gl-link> </template> </gl-sprintf> - <time-ago-tooltip :time="lastReply.created_at" tooltip-placement="bottom" /> + <time-ago-tooltip + :time="lastReply.created_at || lastReply.createdAt" + tooltip-placement="bottom" + /> </template> <gl-button v-else diff --git a/app/assets/javascripts/work_items/components/notes/system_note.vue b/app/assets/javascripts/work_items/components/notes/system_note.vue index 92a2fcaf1df..bca061f5e01 100644 --- a/app/assets/javascripts/work_items/components/notes/system_note.vue +++ b/app/assets/javascripts/work_items/components/notes/system_note.vue @@ -22,6 +22,7 @@ import SafeHtml from '~/vue_shared/directives/safe_html'; import descriptionVersionHistoryMixin from 'ee_else_ce/notes/mixins/description_version_history'; import axios from '~/lib/utils/axios_utils'; import { getLocationHash } from '~/lib/utils/url_utility'; +import { getIdFromGraphQLId } from '~/graphql_shared/utils'; import { __ } from '~/locale'; import NoteHeader from '~/notes/components/note_header.vue'; import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; @@ -87,6 +88,9 @@ export default { descriptionVersion() { return this.descriptionVersions[this.note.description_version_id]; }, + noteId() { + return getIdFromGraphQLId(this.note.id); + }, }, mounted() { renderGFM(this.$refs['gfm-content']); @@ -129,7 +133,7 @@ export default { <note-header :author="note.author" :created-at="note.createdAt" - :note-id="note.id" + :note-id="noteId" :is-system-note="true" > <span ref="gfm-content" v-safe-html="actionTextHtml"></span> diff --git a/app/assets/javascripts/work_items/components/notes/work_item_discussion.vue b/app/assets/javascripts/work_items/components/notes/work_item_discussion.vue new file mode 100644 index 00000000000..70253f88795 --- /dev/null +++ b/app/assets/javascripts/work_items/components/notes/work_item_discussion.vue @@ -0,0 +1,184 @@ +<script> +import { GlAvatarLink, GlAvatar } from '@gitlab/ui'; +import { ASC } from '~/notes/constants'; +import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue'; +import DiscussionNotesRepliesWrapper from '~/notes/components/discussion_notes_replies_wrapper.vue'; +import ToggleRepliesWidget from '~/notes/components/toggle_replies_widget.vue'; +import WorkItemNote from '~/work_items/components/notes/work_item_note.vue'; +import WorkItemNoteReplying from '~/work_items/components/notes/work_item_note_replying.vue'; +import WorkItemCommentForm from '../work_item_comment_form.vue'; + +export default { + components: { + TimelineEntryItem, + GlAvatarLink, + GlAvatar, + WorkItemNote, + WorkItemCommentForm, + ToggleRepliesWidget, + DiscussionNotesRepliesWrapper, + WorkItemNoteReplying, + }, + props: { + workItemId: { + type: String, + required: true, + }, + queryVariables: { + type: Object, + required: true, + }, + fullPath: { + type: String, + required: true, + }, + workItemType: { + type: String, + required: true, + }, + fetchByIid: { + type: Boolean, + required: false, + default: false, + }, + discussion: { + type: Array, + required: true, + }, + sortOrder: { + type: String, + default: ASC, + required: false, + }, + }, + data() { + return { + isExpanded: false, + autofocus: false, + isReplying: false, + replyingText: '', + }; + }, + computed: { + note() { + return this.discussion[0]; + }, + author() { + return this.note.author; + }, + noteAnchorId() { + return `note_${this.note.id}`; + }, + hasReplies() { + return this.replies?.length; + }, + replies() { + if (this.discussion?.length > 1) { + return this.discussion.slice(1); + } + return null; + }, + discussionId() { + return this.discussion[0]?.discussion?.id || ''; + }, + }, + methods: { + showReplyForm() { + this.isExpanded = true; + this.autofocus = true; + }, + hideReplyForm() { + this.isExpanded = this.hasReplies; + this.autofocus = false; + }, + toggleDiscussion() { + this.isExpanded = !this.isExpanded; + this.autofocus = this.isExpanded; + }, + threadKey(note) { + /* eslint-disable @gitlab/require-i18n-strings */ + return `${note.id}-thread`; + }, + onReplied() { + this.isExpanded = true; + this.isReplying = false; + this.replyingText = ''; + }, + onReplying(commentText) { + this.isReplying = true; + this.replyingText = commentText; + }, + }, +}; +</script> + +<template> + <timeline-entry-item + :id="noteAnchorId" + :class="{ 'internal-note': note.internal }" + :data-note-id="note.id" + class="note note-wrapper note-comment gl-px-0" + > + <div class="timeline-avatar gl-float-left"> + <gl-avatar-link :href="author.webUrl"> + <gl-avatar + :src="author.avatarUrl" + :entity-name="author.username" + :alt="author.name" + :size="32" + /> + </gl-avatar-link> + </div> + + <div class="timeline-content"> + <div class="discussion-body"> + <div class="discussion-wrapper"> + <div class="discussion-notes"> + <ul class="notes"> + <work-item-note + :is-first-note="true" + :note="note" + :discussion-id="discussionId" + @startReplying="showReplyForm" + /> + <discussion-notes-replies-wrapper> + <toggle-replies-widget + v-if="hasReplies" + :collapsed="!isExpanded" + :replies="replies" + @toggle="toggleDiscussion({ discussionId })" + /> + <template v-if="isExpanded"> + <template v-for="reply in replies"> + <work-item-note + :key="threadKey(reply)" + discussion-id="discussionId" + :note="reply" + @startReplying="showReplyForm" + /> + </template> + <work-item-note-replying v-if="isReplying" :body="replyingText" /> + <work-item-comment-form + :autofocus="autofocus" + :query-variables="queryVariables" + :full-path="fullPath" + :work-item-id="workItemId" + :fetch-by-iid="fetchByIid" + :discussion-id="discussionId" + :work-item-type="workItemType" + :sort-order="sortOrder" + :add-padding="true" + @cancelEditing="hideReplyForm" + @replied="onReplied" + @replying="onReplying" + @error="$emit('error', $event)" + /> + </template> + </discussion-notes-replies-wrapper> + </ul> + </div> + </div> + </div> + </div> + </timeline-entry-item> +</template> diff --git a/app/assets/javascripts/work_items/components/notes/work_item_note.vue b/app/assets/javascripts/work_items/components/notes/work_item_note.vue index c8e6e79145b..6985e8b3695 100644 --- a/app/assets/javascripts/work_items/components/notes/work_item_note.vue +++ b/app/assets/javascripts/work_items/components/notes/work_item_note.vue @@ -1,42 +1,60 @@ <script> import { GlAvatarLink, GlAvatar } from '@gitlab/ui'; import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue'; +import NoteBody from '~/work_items/components/notes/work_item_note_body.vue'; import NoteHeader from '~/notes/components/note_header.vue'; -import NoteBody from './work_item_note_body.vue'; +import NoteActions from '~/work_items/components/notes/work_item_note_actions.vue'; +import { renderGFM } from '~/behaviors/markdown/render_gfm'; export default { + name: 'WorkItemNoteThread', components: { - NoteHeader, - NoteBody, TimelineEntryItem, - GlAvatarLink, + NoteBody, + NoteHeader, + NoteActions, GlAvatar, + GlAvatarLink, }, props: { note: { type: Object, required: true, }, + isFirstNote: { + type: Boolean, + required: false, + default: false, + }, }, computed: { author() { return this.note.author; }, - noteAnchorId() { - return `note_${this.note.id}`; + entryClass() { + return { + 'note note-wrapper note-comment': true, + 'gl-p-4': !this.isFirstNote, + }; + }, + showReply() { + return this.note.userPermissions.createNote && this.isFirstNote; + }, + }, + methods: { + renderGFM() { + renderGFM(this.$refs['note-body']); + }, + showReplyForm() { + this.$emit('startReplying'); }, }, }; </script> <template> - <timeline-entry-item - :id="noteAnchorId" - :class="{ 'internal-note': note.internal }" - :data-note-id="note.id" - class="note note-wrapper note-comment" - > - <div class="timeline-avatar gl-float-left"> + <timeline-entry-item :class="entryClass"> + <div v-if="!isFirstNote" :key="note.id" class="timeline-avatar gl-float-left"> <gl-avatar-link :href="author.webUrl"> <gl-avatar :src="author.avatarUrl" @@ -46,13 +64,13 @@ export default { /> </gl-avatar-link> </div> - - <div class="timeline-content"> + <div class="timeline-content-inner"> <div class="note-header"> <note-header :author="author" :created-at="note.createdAt" :note-id="note.id" /> + <note-actions :show-reply="showReply" @startReplying="showReplyForm" /> </div> <div class="timeline-discussion-body"> - <note-body :note="note" /> + <note-body ref="noteBody" :note="note" /> </div> </div> </timeline-entry-item> diff --git a/app/assets/javascripts/work_items/components/notes/work_item_note_actions.vue b/app/assets/javascripts/work_items/components/notes/work_item_note_actions.vue new file mode 100644 index 00000000000..584dfb68ae3 --- /dev/null +++ b/app/assets/javascripts/work_items/components/notes/work_item_note_actions.vue @@ -0,0 +1,22 @@ +<script> +import ReplyButton from '~/notes/components/note_actions/reply_button.vue'; + +export default { + name: 'WorkItemNoteActions', + components: { + ReplyButton, + }, + props: { + showReply: { + type: Boolean, + required: true, + }, + }, +}; +</script> + +<template> + <div class="note-actions"> + <reply-button v-if="showReply" ref="replyButton" @startReplying="$emit('startReplying')" /> + </div> +</template> diff --git a/app/assets/javascripts/work_items/components/notes/work_item_note_body.vue b/app/assets/javascripts/work_items/components/notes/work_item_note_body.vue index 9aa01784c9f..d353aa29f4a 100644 --- a/app/assets/javascripts/work_items/components/notes/work_item_note_body.vue +++ b/app/assets/javascripts/work_items/components/notes/work_item_note_body.vue @@ -3,6 +3,7 @@ import SafeHtml from '~/vue_shared/directives/safe_html'; import { renderGFM } from '~/behaviors/markdown/render_gfm'; export default { + name: 'WorkItemNoteBody', directives: { SafeHtml, }, diff --git a/app/assets/javascripts/work_items/components/notes/work_item_note_replying.vue b/app/assets/javascripts/work_items/components/notes/work_item_note_replying.vue new file mode 100644 index 00000000000..46f61ccd204 --- /dev/null +++ b/app/assets/javascripts/work_items/components/notes/work_item_note_replying.vue @@ -0,0 +1,53 @@ +<script> +import { GlAvatar } from '@gitlab/ui'; +import SafeHtml from '~/vue_shared/directives/safe_html'; +import NoteHeader from '~/notes/components/note_header.vue'; +import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue'; + +export default { + name: 'WorkItemNoteReplying', + components: { + TimelineEntryItem, + GlAvatar, + NoteHeader, + }, + directives: { + SafeHtml, + }, + safeHtmlConfig: { + ADD_TAGS: ['use', 'gl-emoji', 'copy-code'], + }, + constantOptions: { + avatarUrl: window.gon.current_user_avatar_url, + }, + props: { + body: { + type: String, + required: false, + default: '', + }, + }, + computed: { + author() { + return { + avatarUrl: window.gon.current_user_avatar_url, + id: window.gon.current_user_id, + name: window.gon.current_user_fullname, + username: window.gon.current_username, + }; + }, + }, +}; +</script> + +<template> + <timeline-entry-item class="note note-wrapper note-comment gl-p-4 being-posted"> + <div class="timeline-avatar gl-float-left"> + <gl-avatar :src="$options.constantOptions.avatarUrl" :size="32" class="gl-mr-3" /> + </div> + <div class="note-header"> + <note-header :author="author" /> + </div> + <div ref="note-body" v-safe-html:[$options.safeHtmlConfig]="body" class="note-body"></div> + </timeline-entry-item> +</template> diff --git a/app/assets/javascripts/work_items/components/work_item_comment_form.vue b/app/assets/javascripts/work_items/components/work_item_comment_form.vue index 2c9b2c967dd..9da4473cd46 100644 --- a/app/assets/javascripts/work_items/components/work_item_comment_form.vue +++ b/app/assets/javascripts/work_items/components/work_item_comment_form.vue @@ -6,11 +6,13 @@ import { getDraft, clearDraft, updateDraft } from '~/lib/utils/autosave'; import { confirmAction } from '~/lib/utils/confirm_via_gl_modal/confirm_via_gl_modal'; import { __, s__ } from '~/locale'; import Tracking from '~/tracking'; +import { ASC } from '~/notes/constants'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; +import { updateCommentState } from '~/work_items/graphql/cache_utils'; import MarkdownEditor from '~/vue_shared/components/markdown/markdown_editor.vue'; -import { getWorkItemQuery, getWorkItemNotesQuery } from '../utils'; +import { getWorkItemQuery } from '../utils'; import createNoteMutation from '../graphql/create_work_item_note.mutation.graphql'; -import { i18n, TRACKING_CATEGORY_SHOW } from '../constants'; +import { TRACKING_CATEGORY_SHOW, i18n } from '../constants'; import WorkItemNoteSignedOut from './work_item_note_signed_out.vue'; import WorkItemCommentLocked from './work_item_comment_locked.vue'; @@ -45,6 +47,30 @@ export default { type: Object, required: true, }, + discussionId: { + type: String, + required: false, + default: '', + }, + autofocus: { + type: Boolean, + required: false, + default: false, + }, + addPadding: { + type: Boolean, + required: false, + default: false, + }, + workItemType: { + type: String, + required: true, + }, + sortOrder: { + type: String, + required: false, + default: ASC, + }, }, data() { return { @@ -82,11 +108,6 @@ export default { // eslint-disable-next-line @gitlab/require-i18n-strings return `${this.workItemId}-comment`; }, - canEdit() { - // maybe this should use `NotePermissions.updateNote`, but if - // we don't have any notes yet, that permission isn't on WorkItem - return Boolean(this.workItem?.userPermissions?.updateWorkItem); - }, tracking() { return { category: TRACKING_CATEGORY_SHOW, @@ -94,17 +115,33 @@ export default { property: `type_${this.workItemType}`, }; }, - workItemType() { - return this.workItem?.workItemType?.name; - }, markdownPreviewPath() { return `${gon.relative_url_root || ''}/${this.fullPath}/preview_markdown?target_type=${ this.workItemType }`; }, + timelineEntryClass() { + return { + 'timeline-entry gl-mb-3': true, + 'gl-p-4': this.addPadding, + }; + }, isProjectArchived() { return this.workItem?.project?.archived; }, + canUpdate() { + return this.workItem?.userPermissions?.updateWorkItem; + }, + }, + watch: { + autofocus: { + immediate: true, + handler(focus) { + if (focus) { + this.isEditing = true; + } + }, + }, }, methods: { startEditing() { @@ -125,6 +162,7 @@ export default { } } + this.$emit('cancelEditing'); this.isEditing = false; clearDraft(this.autosaveKey); }, @@ -136,33 +174,31 @@ export default { } this.isSubmitting = true; + this.$emit('replying', this.commentText); + const { queryVariables, fetchByIid, sortOrder } = this; try { this.track('add_work_item_comment'); - const { - data: { createNote }, - } = await this.$apollo.mutate({ + await this.$apollo.mutate({ mutation: createNoteMutation, variables: { input: { - noteableId: this.workItem.id, + noteableId: this.workItemId, body: this.commentText, + discussionId: this.discussionId || null, }, }, + update(store, createNoteData) { + if (createNoteData.data?.createNote?.errors?.length) { + throw new Error(createNoteData.data?.createNote?.errors[0]); + } + updateCommentState(store, createNoteData, fetchByIid, queryVariables, sortOrder); + }, }); - - if (createNote.errors?.length) { - throw new Error(createNote.errors[0]); - } - - const client = this.$apollo.provider.defaultClient; - client.refetchQueries({ - include: [getWorkItemNotesQuery(this.fetchByIid)], - }); - - this.isEditing = false; clearDraft(this.autosaveKey); + this.$emit('replied'); + this.isEditing = false; } catch (error) { this.$emit('error', error.message); Sentry.captureException(error); @@ -179,10 +215,10 @@ export default { </script> <template> - <li class="timeline-entry"> + <li :class="timelineEntryClass"> <work-item-note-signed-out v-if="!signedIn" /> <work-item-comment-locked - v-else-if="!canEdit" + v-else-if="!canUpdate" :work-item-type="workItemType" :is-project-archived="isProjectArchived" /> diff --git a/app/assets/javascripts/work_items/components/work_item_notes.vue b/app/assets/javascripts/work_items/components/work_item_notes.vue index affa6be6e1b..474fea5cec6 100644 --- a/app/assets/javascripts/work_items/components/work_item_notes.vue +++ b/app/assets/javascripts/work_items/components/work_item_notes.vue @@ -6,7 +6,7 @@ import ActivityFilter from '~/work_items/components/notes/activity_filter.vue'; import { i18n, DEFAULT_PAGE_SIZE_NOTES } from '~/work_items/constants'; import { ASC, DESC } from '~/notes/constants'; import { getWorkItemNotesQuery } from '~/work_items/utils'; -import WorkItemNote from '~/work_items/components/notes/work_item_note.vue'; +import WorkItemDiscussion from '~/work_items/components/notes/work_item_discussion.vue'; import WorkItemCommentForm from './work_item_comment_form.vue'; export default { @@ -23,7 +23,7 @@ export default { ActivityFilter, SystemNote, WorkItemCommentForm, - WorkItemNote, + WorkItemDiscussion, }, props: { workItemId: { @@ -91,6 +91,8 @@ export default { fullPath: this.fullPath, workItemId: this.workItemId, fetchByIid: this.fetchByIid, + workItemType: this.workItemType, + sortOrder: this.sortOrder, }; }, }, @@ -133,6 +135,11 @@ export default { }, }, methods: { + getDiscussionKey(discussion) { + // discussion key is important like this since after first comment changes + const discussionId = discussion.notes.nodes[0].id; + return discussionId.split('/')[discussionId.split('/').length - 1]; + }, isSystemNote(note) { return note.notes.nodes[0].system; }, @@ -183,7 +190,7 @@ export default { </script> <template> - <div class="gl-border-t gl-mt-5"> + <div class="gl-border-t gl-mt-5 work-item-notes"> <div class="gl-display-flex gl-justify-content-space-between gl-flex-wrap"> <label class="gl-mb-0">{{ $options.i18n.ACTIVITY_LABEL }}</label> <activity-filter @@ -216,13 +223,23 @@ export default { @error="$emit('error', $event)" /> - <template v-for="note in notesArray"> + <template v-for="discussion in notesArray"> <system-note - v-if="isSystemNote(note)" - :key="note.notes.nodes[0].id" - :note="note.notes.nodes[0]" + v-if="isSystemNote(discussion)" + :key="discussion.notes.nodes[0].id" + :note="discussion.notes.nodes[0]" /> - <work-item-note v-else :key="note.notes.nodes[0].id" :note="note.notes.nodes[0]" /> + <template v-else> + <work-item-discussion + :key="getDiscussionKey(discussion)" + :discussion="discussion.notes.nodes" + :query-variables="queryVariables" + :full-path="fullPath" + :work-item-id="workItemId" + :fetch-by-iid="fetchByIid" + :work-item-type="workItemType" + /> + </template> </template> <work-item-comment-form diff --git a/app/assets/javascripts/work_items/graphql/cache_utils.js b/app/assets/javascripts/work_items/graphql/cache_utils.js new file mode 100644 index 00000000000..c1de9f673f2 --- /dev/null +++ b/app/assets/javascripts/work_items/graphql/cache_utils.js @@ -0,0 +1,66 @@ +import { produce } from 'immer'; +import { WIDGET_TYPE_NOTES } from '~/work_items/constants'; +import { ASC } from '~/notes/constants'; +import { getWorkItemNotesQuery } from '~/work_items/utils'; + +/** + * Updates the cache manually when adding a main comment + * + * @param store + * @param createNoteData + * @param fetchByIid + * @param queryVariables + * @param sortOrder + */ +export const updateCommentState = ( + store, + { data: { createNote } }, + fetchByIid, + queryVariables, + sortOrder, +) => { + const notesQuery = getWorkItemNotesQuery(fetchByIid); + const variables = { + ...queryVariables, + pageSize: 100, + }; + const sourceData = store.readQuery({ + query: notesQuery, + variables, + }); + + const finalData = produce(sourceData, (draftData) => { + const notesWidget = fetchByIid + ? draftData.workspace.workItems.nodes[0].widgets.find( + (widget) => widget.type === WIDGET_TYPE_NOTES, + ) + : draftData.workItem.widgets.find((widget) => widget.type === WIDGET_TYPE_NOTES); + + const arrayPushMethod = sortOrder === ASC ? 'push' : 'unshift'; + + // manual update of cache with a completely new discussion + if (createNote.note.discussion.notes.nodes.length === 1) { + notesWidget.discussions.nodes[arrayPushMethod]({ + id: createNote.note.discussion.id, + notes: { + nodes: createNote.note.discussion.notes.nodes, + __typename: 'NoteConnection', + }, + // eslint-disable-next-line @gitlab/require-i18n-strings + __typename: 'Discussion', + }); + } + + if (fetchByIid) { + draftData.workspace.workItems.nodes[0].widgets[6] = notesWidget; + } else { + draftData.workItem.widgets[6] = notesWidget; + } + }); + + store.writeQuery({ + query: notesQuery, + variables, + data: finalData, + }); +}; diff --git a/app/assets/javascripts/work_items/graphql/create_work_item_note.mutation.graphql b/app/assets/javascripts/work_items/graphql/create_work_item_note.mutation.graphql index 6a7afd7bd5b..0d7e1aeb089 100644 --- a/app/assets/javascripts/work_items/graphql/create_work_item_note.mutation.graphql +++ b/app/assets/javascripts/work_items/graphql/create_work_item_note.mutation.graphql @@ -1,5 +1,21 @@ +#import "~/work_items/graphql/work_item_note.fragment.graphql" + mutation createWorkItemNote($input: CreateNoteInput!) { createNote(input: $input) { + note { + id + discussion { + id + notes { + nodes { + ...WorkItemNote + } + } + __typename + } + __typename + } errors + __typename } } diff --git a/app/assets/javascripts/work_items/graphql/work_item_note.fragment.graphql b/app/assets/javascripts/work_items/graphql/work_item_note.fragment.graphql index 5215ea10918..d1ab4642869 100644 --- a/app/assets/javascripts/work_items/graphql/work_item_note.fragment.graphql +++ b/app/assets/javascripts/work_items/graphql/work_item_note.fragment.graphql @@ -7,10 +7,18 @@ fragment WorkItemNote on Note { internal systemNoteIconName createdAt + discussion { + id + } author { ...User } userPermissions { adminNote + awardEmoji + readNote + createNote + resolveNote + repositionNote } } diff --git a/app/assets/stylesheets/page_bundles/ci_status.scss b/app/assets/stylesheets/page_bundles/ci_status.scss index 7adbf10b83a..17886ab954a 100644 --- a/app/assets/stylesheets/page_bundles/ci_status.scss +++ b/app/assets/stylesheets/page_bundles/ci_status.scss @@ -27,7 +27,6 @@ } &.ci-canceled, - &.ci-skipped, &.ci-disabled, &.ci-scheduled, &.ci-manual { diff --git a/app/assets/stylesheets/page_bundles/work_items.scss b/app/assets/stylesheets/page_bundles/work_items.scss index 4766f124e5b..07a0cf3f367 100644 --- a/app/assets/stylesheets/page_bundles/work_items.scss +++ b/app/assets/stylesheets/page_bundles/work_items.scss @@ -86,3 +86,10 @@ min-width: 0; } } + +.work-item-notes { + .discussion-notes ul.notes li.toggle-replies-widget { + // offset for .timeline-content padding + an extra 1px for border width + margin: -5px -9px; + } +} diff --git a/doc/ci/ssh_keys/index.md b/doc/ci/ssh_keys/index.md index ed16a19c7f5..c1154485018 100644 --- a/doc/ci/ssh_keys/index.md +++ b/doc/ci/ssh_keys/index.md @@ -28,7 +28,7 @@ with any type of [executor](https://docs.gitlab.com/runner/executors/) ## How it works 1. Create a new SSH key pair locally with [`ssh-keygen`](https://linux.die.net/man/1/ssh-keygen) -1. Add the private key as a [variable](../variables/index.md) to +1. Add the private key as a [file type CI/CD variable](../variables/index.md#use-file-type-cicd-variables) to your project 1. Run the [`ssh-agent`](https://linux.die.net/man/1/ssh-agent) during job to load the private key. @@ -52,7 +52,7 @@ to access it. In this case, you can use an SSH key pair. **Do not** add a passphrase to the SSH key, or the `before_script` will prompt for it. -1. Create a new [CI/CD variable](../variables/index.md). +1. Create a new [file type CI/CD variable](../variables/index.md). As **Key** enter the name `SSH_PRIVATE_KEY` and in the **Value** field paste the content of your _private_ key that you created earlier. @@ -73,12 +73,11 @@ to access it. In this case, you can use an SSH key pair. - eval $(ssh-agent -s) ## - ## Add the SSH key stored in SSH_PRIVATE_KEY variable to the agent store - ## We're using tr to fix line endings which makes ed25519 keys work - ## without extra base64 encoding. - ## https://gitlab.com/gitlab-examples/ssh-private-key/issues/1#note_48526556 + ## Give the right permissions, otherwise ssh-add will refuse to add files + ## Add the SSH key stored in SSH_PRIVATE_KEY file type CI/CD variable to the agent store ## - - echo "$SSH_PRIVATE_KEY" | tr -d '\r' | ssh-add - + - chmod 400 "$SSH_PRIVATE_KEY" + - ssh-add "$SSH_PRIVATE_KEY" ## ## Create the SSH directory and give it the right permissions @@ -100,7 +99,7 @@ to access it. In this case, you can use an SSH key pair. 1. Make sure the private server's [SSH host keys are verified](#verifying-the-ssh-host-keys). 1. As a final step, add the _public_ key from the one you created in the first - step to the services that you want to have an access to from within the build + step to the services that you want to have an access to from inside the build environment. If you are accessing a private GitLab repository you must add it as a [deploy key](../../user/project/deploy_keys/index.md). @@ -129,7 +128,7 @@ on, and use that key for all projects that are run on this machine. prompt for it. 1. As a final step, add the _public_ key from the one you created earlier to the - services that you want to have an access to from within the build environment. + services that you want to have an access to from inside the build environment. If you are accessing a private GitLab repository you must add it as a [deploy key](../../user/project/deploy_keys/index.md). @@ -160,14 +159,14 @@ ssh-keyscan example.com ssh-keyscan 1.2.3.4 ``` -Create a new [CI/CD variable](../variables/index.md) with -`SSH_KNOWN_HOSTS` as "Key", and as a "Value" add the output of `ssh-keyscan`. +Create a new [file type CI/CD variable](../variables/index.md#use-file-type-cicd-variables) +with `SSH_KNOWN_HOSTS` as "Key", and as a "Value" add the output of `ssh-keyscan`. If you must connect to multiple servers, all the server host keys must be collected in the **Value** of the variable, one key per line. NOTE: -By using a variable instead of `ssh-keyscan` directly inside +By using a file type CI/CD variable instead of `ssh-keyscan` directly inside `.gitlab-ci.yml`, it has the benefit that you don't have to change `.gitlab-ci.yml` if the host domain name changes for some reason. Also, the values are predefined by you, meaning that if the host keys suddenly change, the CI/CD job doesn't fail, @@ -180,10 +179,10 @@ above, you must add: ```yaml before_script: ## - ## Assuming you created the SSH_KNOWN_HOSTS variable, uncomment the + ## Assuming you created the SSH_KNOWN_HOSTS file type CI/CD variable, uncomment the ## following two lines. ## - - echo "$SSH_KNOWN_HOSTS" >> ~/.ssh/known_hosts + - cp "$SSH_KNOWN_HOSTS" ~/.ssh/known_hosts - chmod 644 ~/.ssh/known_hosts ## diff --git a/doc/development/i18n/translation.md b/doc/development/i18n/translation.md index 2c8d7eac2a6..bfc4d817c73 100644 --- a/doc/development/i18n/translation.md +++ b/doc/development/i18n/translation.md @@ -75,6 +75,7 @@ The level of formality used in software varies by language: | -------- | --------- | ------- | | French | formal | `vous` for `you` | | German | informal | `du` for `you` | +| Spanish | informal | `tú` for `you` | Refer to other translated strings and notes in the glossary to assist you in determining a suitable level of formality. diff --git a/doc/development/redis.md b/doc/development/redis.md index 45712b14c0a..68cab9ac38d 100644 --- a/doc/development/redis.md +++ b/doc/development/redis.md @@ -242,9 +242,8 @@ The Redis [`PFCOUNT`](https://redis.io/commands/pfcount/), [`PFADD`](https://redis.io/commands/pfadd/), and [`PFMERGE`](https://redis.io/commands/pfmerge/) commands operate on HyperLogLogs, a data structure that allows estimating the number of unique -elements with low memory usage. (In addition to the `PFCOUNT` documentation, -Thoughtbot's article on [HyperLogLogs in Redis](https://thoughtbot.com/blog/hyperloglogs-in-redis) -provides a good background here.) +elements with low memory usage. For more information, +see [HyperLogLogs in Redis](https://thoughtbot.com/blog/hyperloglogs-in-redis). [`Gitlab::Redis::HLL`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/redis/hll.rb) provides a convenient interface for adding and counting values in HyperLogLogs. diff --git a/lib/gitlab/memory/watchdog.rb b/lib/gitlab/memory/watchdog.rb index c94dbed1d46..cc335c00e26 100644 --- a/lib/gitlab/memory/watchdog.rb +++ b/lib/gitlab/memory/watchdog.rb @@ -6,47 +6,6 @@ module Gitlab # into a handler when the Ruby process violates defined limits # for an extended period of time. class Watchdog - # This handler does nothing. It returns `false` to indicate to the - # caller that the situation has not been dealt with so it will - # receive calls repeatedly if fragmentation remains high. - # - # This is useful for "dress rehearsals" in production since it allows - # us to observe how frequently the handler is invoked before taking action. - class NullHandler - include Singleton - - def call - # NOP - false - end - end - - # This handler sends SIGTERM and considers the situation handled. - class TermProcessHandler - def initialize(pid = $$) - @pid = pid - end - - def call - Process.kill(:TERM, @pid) - true - end - end - - # This handler invokes Puma's graceful termination handler, which takes - # into account a configurable grace period during which a process may - # remain unresponsive to a SIGTERM. - class PumaHandler - def initialize(puma_options = ::Puma.cli_config.options) - @worker = ::Puma::Cluster::WorkerHandle.new(0, $$, 0, puma_options) - end - - def call - @worker.term - true - end - end - def initialize @configuration = Configuration.new @alive = true @@ -73,6 +32,7 @@ module Gitlab def stop stop_working(reason: 'background task stopped') + handler.stop if handler.respond_to?(:stop) end private @@ -111,7 +71,7 @@ module Gitlab def handler # This allows us to keep the watchdog running but turn it into "friendly mode" where # all that happens is we collect logs and Prometheus events for fragmentation violations. - return NullHandler.instance unless Feature.enabled?(:enforce_memory_watchdog, type: :ops) + return Handlers::NullHandler.instance unless Feature.enabled?(:enforce_memory_watchdog, type: :ops) configuration.handler end diff --git a/lib/gitlab/memory/watchdog/configuration.rb b/lib/gitlab/memory/watchdog/configuration.rb index 5c459220be8..6ab199bf816 100644 --- a/lib/gitlab/memory/watchdog/configuration.rb +++ b/lib/gitlab/memory/watchdog/configuration.rb @@ -48,7 +48,7 @@ module Gitlab end def handler - @handler ||= NullHandler.instance + @handler ||= Handlers::NullHandler.instance end def event_reporter diff --git a/lib/gitlab/memory/watchdog/configurator.rb b/lib/gitlab/memory/watchdog/configurator.rb index 04c04cbde02..8e6e3ef134b 100644 --- a/lib/gitlab/memory/watchdog/configurator.rb +++ b/lib/gitlab/memory/watchdog/configurator.rb @@ -17,7 +17,7 @@ module Gitlab class << self def configure_for_puma ->(config) do - config.handler = Gitlab::Memory::Watchdog::PumaHandler.new + config.handler = Gitlab::Memory::Watchdog::Handlers::PumaHandler.new config.sleep_time_seconds = ENV.fetch('GITLAB_MEMWD_SLEEP_TIME_SEC', DEFAULT_SLEEP_INTERVAL_S).to_i config.monitors(&configure_monitors_for_puma) end @@ -25,7 +25,13 @@ module Gitlab def configure_for_sidekiq ->(config) do - config.handler = Gitlab::Memory::Watchdog::TermProcessHandler.new + # Give Sidekiq up to 30 seconds to allow existing jobs to finish after exceeding the limit + shutdown_timeout_seconds = ENV.fetch('SIDEKIQ_MEMORY_KILLER_SHUTDOWN_WAIT', 30).to_i + + config.handler = Gitlab::Memory::Watchdog::Handlers::SidekiqHandler.new( + shutdown_timeout_seconds, + sidekiq_sleep_time + ) config.sleep_time_seconds = sidekiq_sleep_time config.monitors(&configure_monitors_for_sidekiq) config.event_reporter = SidekiqEventReporter.new diff --git a/lib/gitlab/memory/watchdog/handlers/null_handler.rb b/lib/gitlab/memory/watchdog/handlers/null_handler.rb new file mode 100644 index 00000000000..127001003ce --- /dev/null +++ b/lib/gitlab/memory/watchdog/handlers/null_handler.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Gitlab + module Memory + class Watchdog + module Handlers + # This handler does nothing. It returns `false` to indicate to the + # caller that the situation has not been dealt with so it will + # receive calls repeatedly if fragmentation remains high. + # + # This is useful for "dress rehearsals" in production since it allows + # us to observe how frequently the handler is invoked before taking action. + class NullHandler + include Singleton + + def call + # NOP + false + end + end + end + end + end +end diff --git a/lib/gitlab/memory/watchdog/handlers/puma_handler.rb b/lib/gitlab/memory/watchdog/handlers/puma_handler.rb new file mode 100644 index 00000000000..fffd91733c8 --- /dev/null +++ b/lib/gitlab/memory/watchdog/handlers/puma_handler.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Memory + class Watchdog + module Handlers + # This handler invokes Puma's graceful termination handler, which takes + # into account a configurable grace period during which a process may + # remain unresponsive to a SIGTERM. + class PumaHandler + def initialize(puma_options = ::Puma.cli_config.options) + @worker = ::Puma::Cluster::WorkerHandle.new(0, $$, 0, puma_options) + end + + def call + @worker.term + true + end + end + end + end + end +end diff --git a/lib/gitlab/memory/watchdog/handlers/sidekiq_handler.rb b/lib/gitlab/memory/watchdog/handlers/sidekiq_handler.rb new file mode 100644 index 00000000000..47ed608c576 --- /dev/null +++ b/lib/gitlab/memory/watchdog/handlers/sidekiq_handler.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +module Gitlab + module Memory + class Watchdog + module Handlers + class SidekiqHandler + def initialize(shutdown_timeout_seconds, sleep_time_seconds) + @shutdown_timeout_seconds = shutdown_timeout_seconds + @sleep_time_seconds = sleep_time_seconds + @alive = true + end + + def call + # Tell Sidekiq to stop fetching new jobs + # We first SIGNAL and then wait given time + send_signal(:TSTP, $$, 'stop fetching new jobs', @shutdown_timeout_seconds) + return true unless @alive + + # Tell sidekiq to restart itself + # Keep extra safe to wait `Sidekiq[:timeout] + 2` seconds before SIGKILL + send_signal(:TERM, $$, 'gracefully shut down', Sidekiq[:timeout] + 2) + return true unless @alive + + # Ideally we should never reach this condition + # Wait for Sidekiq to shutdown gracefully, and kill it if it didn't + # If process is group leader, kill the whole pgroup, so we can be sure no children are left behind + send_signal(:KILL, Process.getpgrp == $$ ? 0 : $$, 'hard shut down') + + true + end + + def stop + @alive = false + end + + private + + def send_signal(signal, pid, explanation, wait_time = nil) + Sidekiq.logger.warn( + pid: pid, + worker_id: ::Prometheus::PidProvider.worker_id, + memwd_handler_class: self.class.to_s, + memwd_signal: signal, + memwd_explanation: explanation, + memwd_wait_time: wait_time, + message: "Sending signal and waiting" + ) + + ProcessManagement.signal(pid, signal) + + return unless wait_time + + deadline = Gitlab::Metrics::System.monotonic_time + wait_time + + # Sleep until timeout reached + sleep(@sleep_time_seconds) while @alive && Gitlab::Metrics::System.monotonic_time < deadline + end + end + end + end + end +end diff --git a/spec/frontend/work_items/components/notes/__snapshots__/work_item_note_replying_spec.js.snap b/spec/frontend/work_items/components/notes/__snapshots__/work_item_note_replying_spec.js.snap new file mode 100644 index 00000000000..5901642b8a1 --- /dev/null +++ b/spec/frontend/work_items/components/notes/__snapshots__/work_item_note_replying_spec.js.snap @@ -0,0 +1,3 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`Work Item Note Replying should have the note body and header 1`] = `"<note-header-stub author=\\"[object Object]\\" actiontext=\\"\\" noteabletype=\\"\\" expanded=\\"true\\" showspinner=\\"true\\"></note-header-stub>"`; diff --git a/spec/frontend/work_items/components/notes/work_item_discussion_spec.js b/spec/frontend/work_items/components/notes/work_item_discussion_spec.js new file mode 100644 index 00000000000..fe9b984f028 --- /dev/null +++ b/spec/frontend/work_items/components/notes/work_item_discussion_spec.js @@ -0,0 +1,133 @@ +import { GlAvatarLink, GlAvatar } from '@gitlab/ui'; +import { shallowMount } from '@vue/test-utils'; +import { nextTick } from 'vue'; +import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue'; +import ToggleRepliesWidget from '~/notes/components/toggle_replies_widget.vue'; +import WorkItemDiscussion from '~/work_items/components/notes/work_item_discussion.vue'; +import WorkItemNote from '~/work_items/components/notes/work_item_note.vue'; +import WorkItemNoteReplying from '~/work_items/components/notes/work_item_note_replying.vue'; +import WorkItemCommentForm from '~/work_items/components/work_item_comment_form.vue'; +import { + mockWorkItemCommentNote, + mockWorkItemNotesResponseWithComments, +} from 'jest/work_items/mock_data'; +import { WIDGET_TYPE_NOTES } from '~/work_items/constants'; + +const mockWorkItemNotesWidgetResponseWithComments = mockWorkItemNotesResponseWithComments.data.workItem.widgets.find( + (widget) => widget.type === WIDGET_TYPE_NOTES, +); + +describe('Work Item Discussion', () => { + let wrapper; + const mockWorkItemId = 'gid://gitlab/WorkItem/625'; + + const findTimelineEntryItem = () => wrapper.findComponent(TimelineEntryItem); + const findAvatarLink = () => wrapper.findComponent(GlAvatarLink); + const findAvatar = () => wrapper.findComponent(GlAvatar); + const findToggleRepliesWidget = () => wrapper.findComponent(ToggleRepliesWidget); + const findAllThreads = () => wrapper.findAllComponents(WorkItemNote); + const findThreadAtIndex = (index) => findAllThreads().at(index); + const findWorkItemCommentForm = () => wrapper.findComponent(WorkItemCommentForm); + const findWorkItemNoteReplying = () => wrapper.findComponent(WorkItemNoteReplying); + + const createComponent = ({ + discussion = [mockWorkItemCommentNote], + workItemId = mockWorkItemId, + queryVariables = { id: workItemId }, + fetchByIid = false, + fullPath = 'gitlab-org', + workItemType = 'Task', + } = {}) => { + wrapper = shallowMount(WorkItemDiscussion, { + propsData: { + discussion, + workItemId, + queryVariables, + fetchByIid, + fullPath, + workItemType, + }, + }); + }; + + describe('Default', () => { + beforeEach(() => { + createComponent(); + }); + + it('Should be wrapped inside the timeline entry item', () => { + expect(findTimelineEntryItem().exists()).toBe(true); + }); + + it('should have the author avatar of the work item note', () => { + expect(findAvatarLink().exists()).toBe(true); + expect(findAvatarLink().attributes('href')).toBe(mockWorkItemCommentNote.author.webUrl); + + expect(findAvatar().exists()).toBe(true); + expect(findAvatar().props('src')).toBe(mockWorkItemCommentNote.author.avatarUrl); + expect(findAvatar().props('entityName')).toBe(mockWorkItemCommentNote.author.username); + }); + + it('should not show the the toggle replies widget wrapper when no replies', () => { + expect(findToggleRepliesWidget().exists()).toBe(false); + }); + + it('should not show the comment form by default', () => { + expect(findWorkItemCommentForm().exists()).toBe(false); + }); + }); + + describe('When the main comments has threads', () => { + beforeEach(() => { + createComponent({ + discussion: mockWorkItemNotesWidgetResponseWithComments.discussions.nodes[0].notes.nodes, + }); + }); + + it('should show the toggle replies widget', () => { + expect(findToggleRepliesWidget().exists()).toBe(true); + }); + + it('the number of threads should be equal to the response length', async () => { + findToggleRepliesWidget().vm.$emit('toggle'); + await nextTick(); + expect(findAllThreads()).toHaveLength( + mockWorkItemNotesWidgetResponseWithComments.discussions.nodes[0].notes.nodes.length, + ); + }); + + it('should autofocus when we click expand replies', async () => { + const mainComment = findThreadAtIndex(0); + + mainComment.vm.$emit('startReplying'); + await nextTick(); + expect(findWorkItemCommentForm().exists()).toBe(true); + expect(findWorkItemCommentForm().props('autofocus')).toBe(true); + }); + }); + + describe('When replying to any comment', () => { + beforeEach(async () => { + createComponent({ + discussion: mockWorkItemNotesWidgetResponseWithComments.discussions.nodes[0].notes.nodes, + }); + const mainComment = findThreadAtIndex(0); + + mainComment.vm.$emit('startReplying'); + await nextTick(); + await findWorkItemCommentForm().vm.$emit('replying', 'reply text'); + }); + + it('should show optimistic behavior when replying', async () => { + expect(findAllThreads()).toHaveLength(2); + expect(findWorkItemNoteReplying().exists()).toBe(true); + }); + + it('should be expanded when the reply is successful', async () => { + findWorkItemCommentForm().vm.$emit('replied'); + await nextTick(); + expect(findToggleRepliesWidget().exists()).toBe(true); + expect(findToggleRepliesWidget().props('collapsed')).toBe(false); + }); + }); +}); diff --git a/spec/frontend/work_items/components/notes/work_item_note_actions_spec.js b/spec/frontend/work_items/components/notes/work_item_note_actions_spec.js new file mode 100644 index 00000000000..f3d0e86ee53 --- /dev/null +++ b/spec/frontend/work_items/components/notes/work_item_note_actions_spec.js @@ -0,0 +1,31 @@ +import { shallowMount } from '@vue/test-utils'; +import ReplyButton from '~/notes/components/note_actions/reply_button.vue'; +import WorkItemNoteActions from '~/work_items/components/notes/work_item_note_actions.vue'; + +describe('Work Item Note Actions', () => { + let wrapper; + + const findReplyButton = () => wrapper.findComponent(ReplyButton); + + const createComponent = ({ showReply = true } = {}) => { + wrapper = shallowMount(WorkItemNoteActions, { + propsData: { + showReply, + }, + }); + }; + + describe('Default', () => { + it('Should show the reply button by default', () => { + createComponent(); + expect(findReplyButton().exists()).toBe(true); + }); + }); + + describe('When the reply button needs to be hidden', () => { + it('Should show the reply button by default', () => { + createComponent({ showReply: false }); + expect(findReplyButton().exists()).toBe(false); + }); + }); +}); diff --git a/spec/frontend/work_items/components/notes/work_item_note_replying_spec.js b/spec/frontend/work_items/components/notes/work_item_note_replying_spec.js new file mode 100644 index 00000000000..225cc3bacaf --- /dev/null +++ b/spec/frontend/work_items/components/notes/work_item_note_replying_spec.js @@ -0,0 +1,34 @@ +import { shallowMount } from '@vue/test-utils'; +import WorkItemNoteReplying from '~/work_items/components/notes/work_item_note_replying.vue'; +import NoteHeader from '~/notes/components/note_header.vue'; +import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue'; + +describe('Work Item Note Replying', () => { + let wrapper; + const mockNoteBody = 'replying body'; + + const findTimelineEntry = () => wrapper.findComponent(TimelineEntryItem); + const findNoteHeader = () => wrapper.findComponent(NoteHeader); + + const createComponent = ({ body = mockNoteBody } = {}) => { + wrapper = shallowMount(WorkItemNoteReplying, { + propsData: { + body, + }, + }); + + window.gon.current_user_id = '1'; + window.gon.current_user_avatar_url = 'avatar.png'; + window.gon.current_user_fullname = 'Administrator'; + window.gon.current_username = 'user'; + }; + + beforeEach(() => { + createComponent(); + }); + + it('should have the note body and header', () => { + expect(findTimelineEntry().exists()).toBe(true); + expect(findNoteHeader().html()).toMatchSnapshot(); + }); +}); diff --git a/spec/frontend/work_items/components/notes/work_item_note_spec.js b/spec/frontend/work_items/components/notes/work_item_note_spec.js index 7257d5c8023..d42d82c4e90 100644 --- a/spec/frontend/work_items/components/notes/work_item_note_spec.js +++ b/spec/frontend/work_items/components/notes/work_item_note_spec.js @@ -1,53 +1,69 @@ -import { GlAvatarLink, GlAvatar } from '@gitlab/ui'; +import { GlAvatarLink } from '@gitlab/ui'; import { shallowMount } from '@vue/test-utils'; -import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue'; import WorkItemNote from '~/work_items/components/notes/work_item_note.vue'; +import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue'; import NoteBody from '~/work_items/components/notes/work_item_note_body.vue'; import NoteHeader from '~/notes/components/note_header.vue'; +import NoteActions from '~/work_items/components/notes/work_item_note_actions.vue'; import { mockWorkItemCommentNote } from 'jest/work_items/mock_data'; describe('Work Item Note', () => { let wrapper; + const findAuthorAvatarLink = () => wrapper.findComponent(GlAvatarLink); const findTimelineEntryItem = () => wrapper.findComponent(TimelineEntryItem); const findNoteHeader = () => wrapper.findComponent(NoteHeader); const findNoteBody = () => wrapper.findComponent(NoteBody); - const findAvatarLink = () => wrapper.findComponent(GlAvatarLink); - const findAvatar = () => wrapper.findComponent(GlAvatar); + const findNoteActions = () => wrapper.findComponent(NoteActions); - const createComponent = ({ note = mockWorkItemCommentNote } = {}) => { + const createComponent = ({ note = mockWorkItemCommentNote, isFirstNote = false } = {}) => { wrapper = shallowMount(WorkItemNote, { propsData: { note, + isFirstNote, }, }); }; - beforeEach(() => { - createComponent(); - }); + describe('Main comment', () => { + beforeEach(() => { + createComponent({ isFirstNote: true }); + }); - it('Should be wrapped inside the timeline entry item', () => { - expect(findTimelineEntryItem().exists()).toBe(true); - }); + it('Should have the note header, actions and body', () => { + expect(findTimelineEntryItem().exists()).toBe(true); + expect(findNoteHeader().exists()).toBe(true); + expect(findNoteBody().exists()).toBe(true); + expect(findNoteActions().exists()).toBe(true); + }); - it('should have the author avatar of the work item note', () => { - expect(findAvatarLink().exists()).toBe(true); - expect(findAvatarLink().attributes('href')).toBe(mockWorkItemCommentNote.author.webUrl); + it('Should not have the Avatar link for main thread inside the timeline-entry', () => { + expect(findAuthorAvatarLink().exists()).toBe(false); + }); - expect(findAvatar().exists()).toBe(true); - expect(findAvatar().props('src')).toBe(mockWorkItemCommentNote.author.avatarUrl); - expect(findAvatar().props('entityName')).toBe(mockWorkItemCommentNote.author.username); + it('Should have the reply button props', () => { + expect(findNoteActions().props('showReply')).toBe(true); + }); }); - it('has note header', () => { - expect(findNoteHeader().exists()).toBe(true); - expect(findNoteHeader().props('author')).toEqual(mockWorkItemCommentNote.author); - expect(findNoteHeader().props('createdAt')).toBe(mockWorkItemCommentNote.createdAt); - }); + describe('Comment threads', () => { + beforeEach(() => { + createComponent(); + }); + + it('Should have the note header, actions and body', () => { + expect(findTimelineEntryItem().exists()).toBe(true); + expect(findNoteHeader().exists()).toBe(true); + expect(findNoteBody().exists()).toBe(true); + expect(findNoteActions().exists()).toBe(true); + }); - it('has note body', () => { - expect(findNoteBody().exists()).toBe(true); - expect(findNoteBody().props('note')).toEqual(mockWorkItemCommentNote); + it('Should have the Avatar link for comment threads', () => { + expect(findAuthorAvatarLink().exists()).toBe(true); + }); + + it('Should not have the reply button props', () => { + expect(findNoteActions().props('showReply')).toBe(false); + }); }); }); diff --git a/spec/frontend/work_items/components/work_item_comment_form_spec.js b/spec/frontend/work_items/components/work_item_comment_form_spec.js index 07c00119398..e62eb32fad0 100644 --- a/spec/frontend/work_items/components/work_item_comment_form_spec.js +++ b/spec/frontend/work_items/components/work_item_comment_form_spec.js @@ -56,6 +56,7 @@ describe('WorkItemCommentForm', () => { fetchByIid = false, signedIn = true, isEditing = true, + workItemType = 'Task', } = {}) => { workItemResponseHandler = jest.fn().mockResolvedValue(workItemResponse); @@ -76,6 +77,7 @@ describe('WorkItemCommentForm', () => { fullPath: 'test-project-path', queryVariables, fetchByIid, + workItemType, }, stubs: { MarkdownField, @@ -109,6 +111,7 @@ describe('WorkItemCommentForm', () => { input: { noteableId: workItemId, body: noteText, + discussionId: null, }, }); }); @@ -138,7 +141,19 @@ describe('WorkItemCommentForm', () => { mutationHandler: jest.fn().mockResolvedValue({ data: { createNote: { - note: null, + note: { + id: 'gid://gitlab/Discussion/c872ba2d7d3eb780d2255138d67ca8b04f65b122', + discussion: { + id: 'gid://gitlab/Discussion/c872ba2d7d3eb780d2255138d67ca8b04f65b122', + notes: { + nodes: [], + __typename: 'NoteConnection', + }, + __typename: 'Discussion', + }, + __typename: 'Note', + }, + __typename: 'CreateNotePayload', errors: [error], }, }, diff --git a/spec/frontend/work_items/components/work_item_notes_spec.js b/spec/frontend/work_items/components/work_item_notes_spec.js index 3a8e48dd4da..df9a141d330 100644 --- a/spec/frontend/work_items/components/work_item_notes_spec.js +++ b/spec/frontend/work_items/components/work_item_notes_spec.js @@ -6,6 +6,7 @@ import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; import SystemNote from '~/work_items/components/notes/system_note.vue'; import WorkItemNotes from '~/work_items/components/work_item_notes.vue'; +import WorkItemDiscussion from '~/work_items/components/notes/work_item_discussion.vue'; import WorkItemCommentForm from '~/work_items/components/work_item_comment_form.vue'; import ActivityFilter from '~/work_items/components/notes/activity_filter.vue'; import workItemNotesQuery from '~/work_items/graphql/work_item_notes.query.graphql'; @@ -17,6 +18,7 @@ import { workItemQueryResponse, mockWorkItemNotesByIidResponse, mockMoreWorkItemNotesResponse, + mockWorkItemNotesResponseWithComments, } from '../mock_data'; const mockWorkItemId = workItemQueryResponse.data.workItem.id; @@ -32,8 +34,14 @@ const mockMoreNotesWidgetResponse = mockMoreWorkItemNotesResponse.data.workItem. (widget) => widget.type === WIDGET_TYPE_NOTES, ); +const mockWorkItemNotesWidgetResponseWithComments = mockWorkItemNotesResponseWithComments.data.workItem.widgets.find( + (widget) => widget.type === WIDGET_TYPE_NOTES, +); + const firstSystemNodeId = mockNotesWidgetResponse.discussions.nodes[0].notes.nodes[0].id; +const mockDiscussions = mockWorkItemNotesWidgetResponseWithComments.discussions.nodes; + describe('WorkItemNotes component', () => { let wrapper; @@ -46,11 +54,16 @@ describe('WorkItemNotes component', () => { const findSkeletonLoader = () => wrapper.findComponent(GlSkeletonLoader); const findSortingFilter = () => wrapper.findComponent(ActivityFilter); const findSystemNoteAtIndex = (index) => findAllSystemNotes().at(index); + const findAllWorkItemCommentNotes = () => wrapper.findAllComponents(WorkItemDiscussion); + const findWorkItemCommentNoteAtIndex = (index) => findAllWorkItemCommentNotes().at(index); const workItemNotesQueryHandler = jest.fn().mockResolvedValue(mockWorkItemNotesResponse); const workItemNotesByIidQueryHandler = jest .fn() .mockResolvedValue(mockWorkItemNotesByIidResponse); const workItemMoreNotesQueryHandler = jest.fn().mockResolvedValue(mockMoreWorkItemNotesResponse); + const workItemNotesWithCommentsQueryHandler = jest + .fn() + .mockResolvedValue(mockWorkItemNotesResponseWithComments); const createComponent = ({ workItemId = mockWorkItemId, @@ -88,7 +101,11 @@ describe('WorkItemNotes component', () => { }); it('passes correct props to comment form component', async () => { - createComponent({ workItemId: mockWorkItemId, fetchByIid: false }); + createComponent({ + workItemId: mockWorkItemId, + fetchByIid: false, + defaultWorkItemNotesQueryHandler: workItemNotesByIidQueryHandler, + }); await waitForPromises(); expect(findWorkItemCommentForm().props('fetchByIid')).toEqual(false); @@ -122,6 +139,7 @@ describe('WorkItemNotes component', () => { }); it('renders the notes list to the length of the response', () => { + expect(workItemNotesByIidQueryHandler).toHaveBeenCalled(); expect(findAllSystemNotes()).toHaveLength( mockNotesByIidWidgetResponse.discussions.nodes.length, ); @@ -194,4 +212,32 @@ describe('WorkItemNotes component', () => { expect(findAllListItems().at(-1).is(WorkItemCommentForm)).toEqual(true); }); }); + + describe('Activity comments', () => { + beforeEach(async () => { + createComponent({ + defaultWorkItemNotesQueryHandler: workItemNotesWithCommentsQueryHandler, + }); + await waitForPromises(); + }); + + it('should not have any system notes', () => { + expect(workItemNotesWithCommentsQueryHandler).toHaveBeenCalled(); + expect(findAllSystemNotes()).toHaveLength(0); + }); + + it('should have work item notes', () => { + expect(workItemNotesWithCommentsQueryHandler).toHaveBeenCalled(); + expect(findAllWorkItemCommentNotes()).toHaveLength(mockDiscussions.length); + }); + + it('should pass all the correct props to work item comment note', () => { + const commentIndex = 0; + const firstCommentNote = findWorkItemCommentNoteAtIndex(commentIndex); + + expect(firstCommentNote.props('discussion')).toEqual( + mockDiscussions[commentIndex].notes.nodes, + ); + }); + }); }); diff --git a/spec/frontend/work_items/mock_data.js b/spec/frontend/work_items/mock_data.js index 5d8692dc04f..bd5d0302ad9 100644 --- a/spec/frontend/work_items/mock_data.js +++ b/spec/frontend/work_items/mock_data.js @@ -1599,8 +1599,7 @@ export const mockWorkItemNotesResponse = { }, nodes: [ { - id: - 'gid://gitlab/IndividualNoteDiscussion/8bbc4890b6ff0f2cde93a5a0947cd2b8a13d3b6e', + id: 'gid://gitlab/Discussion/8bbc4890b6ff0f2cde93a5a0947cd2b8a13d3b6e', notes: { nodes: [ { @@ -1611,8 +1610,16 @@ export const mockWorkItemNotesResponse = { createdAt: '2022-11-14T04:18:59Z', system: true, internal: false, + discussion: { + id: 'gid://gitlab/Discussion/9c17769ca29798eddaed539d010da12723561234', + }, userPermissions: { adminNote: false, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, __typename: 'NotePermissions', }, author: { @@ -1632,8 +1639,7 @@ export const mockWorkItemNotesResponse = { __typename: 'Discussion', }, { - id: - 'gid://gitlab/IndividualNoteDiscussion/7b08b89a728a5ceb7de8334246837ba1d07270dc', + id: 'gid://gitlab/Discussion/7b08b89a728a5ceb7de8334246837ba1d07270dc', notes: { nodes: [ { @@ -1644,8 +1650,16 @@ export const mockWorkItemNotesResponse = { createdAt: '2022-11-14T04:18:59Z', system: true, internal: false, + discussion: { + id: 'gid://gitlab/Discussion/9c17769ca29798eddaed539d010da12723565678', + }, userPermissions: { adminNote: false, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, __typename: 'NotePermissions', }, author: { @@ -1665,8 +1679,7 @@ export const mockWorkItemNotesResponse = { __typename: 'Discussion', }, { - id: - 'gid://gitlab/IndividualNoteDiscussion/0f2f195ec0d1ef95ee9d5b10446b8e96a7d83864', + id: 'gid://gitlab/Discussion/0f2f195ec0d1ef95ee9d5b10446b8e96a7d83864', notes: { nodes: [ { @@ -1676,8 +1689,16 @@ export const mockWorkItemNotesResponse = { createdAt: '2022-11-25T07:16:20Z', system: true, internal: false, + discussion: { + id: 'gid://gitlab/Discussion/9c17769ca29798eddaed539d010da12723560987', + }, userPermissions: { adminNote: false, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, __typename: 'NotePermissions', }, author: { @@ -1756,8 +1777,7 @@ export const mockWorkItemNotesByIidResponse = { }, nodes: [ { - id: - 'gid://gitlab/IndividualNoteDiscussion/8bbc4890b6ff0f2cde93a5a0947cd2b8a13d3b6e', + id: 'gid://gitlab/Discussion/8bbc4890b6ff0f2cde93a5a0947cd2b8a13d3b6e', notes: { nodes: [ { @@ -1768,8 +1788,17 @@ export const mockWorkItemNotesByIidResponse = { createdAt: '2022-11-14T04:18:59Z', system: true, internal: false, + discussion: { + id: + 'gid://gitlab/Discussion/9c17769ca29798eddaed539d010da12723561234', + }, userPermissions: { adminNote: false, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, __typename: 'NotePermissions', }, author: { @@ -1789,8 +1818,7 @@ export const mockWorkItemNotesByIidResponse = { __typename: 'Discussion', }, { - id: - 'gid://gitlab/IndividualNoteDiscussion/7b08b89a728a5ceb7de8334246837ba1d07270dc', + id: 'gid://gitlab/Discussion/7b08b89a728a5ceb7de8334246837ba1d07270dc', notes: { nodes: [ { @@ -1802,8 +1830,17 @@ export const mockWorkItemNotesByIidResponse = { createdAt: '2022-11-14T04:18:59Z', system: true, internal: false, + discussion: { + id: + 'gid://gitlab/Discussion/9c17769ca29798eddaed539d010da12723568765', + }, userPermissions: { adminNote: false, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, __typename: 'NotePermissions', }, author: { @@ -1823,8 +1860,7 @@ export const mockWorkItemNotesByIidResponse = { __typename: 'Discussion', }, { - id: - 'gid://gitlab/IndividualNoteDiscussion/addbc177f7664699a135130ab05ffb78c57e4db3', + id: 'gid://gitlab/Discussion/addbc177f7664699a135130ab05ffb78c57e4db3', notes: { nodes: [ { @@ -1836,8 +1872,17 @@ export const mockWorkItemNotesByIidResponse = { createdAt: '2022-11-14T04:19:00Z', system: true, internal: false, + discussion: { + id: + 'gid://gitlab/Discussion/9c17769ca29798eddaed539d010da12723569876', + }, userPermissions: { adminNote: false, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, __typename: 'NotePermissions', }, author: { @@ -1913,8 +1958,7 @@ export const mockMoreWorkItemNotesResponse = { }, nodes: [ { - id: - 'gid://gitlab/IndividualNoteDiscussion/8bbc4890b6ff0f2cde93a5a0947cd2b8a13d3b6e', + id: 'gid://gitlab/Discussion/8bbc4890b6ff0f2cde93a5a0947cd2b8a13d3b6e', notes: { nodes: [ { @@ -1925,8 +1969,16 @@ export const mockMoreWorkItemNotesResponse = { createdAt: '2022-11-14T04:18:59Z', system: true, internal: false, + discussion: { + id: 'gid://gitlab/Discussion/9c17769ca29798eddaed539d010da1112356a59e', + }, userPermissions: { adminNote: false, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, __typename: 'NotePermissions', }, author: { @@ -1946,8 +1998,7 @@ export const mockMoreWorkItemNotesResponse = { __typename: 'Discussion', }, { - id: - 'gid://gitlab/IndividualNoteDiscussion/7b08b89a728a5ceb7de8334246837ba1d07270dc', + id: 'gid://gitlab/Discussion/7b08b89a728a5ceb7de8334246837ba1d07270dc', notes: { nodes: [ { @@ -1958,8 +2009,16 @@ export const mockMoreWorkItemNotesResponse = { createdAt: '2022-11-14T04:18:59Z', system: true, internal: false, + discussion: { + id: 'gid://gitlab/Discussion/9c17769ca29798eddaed539d010da1272356a59e', + }, userPermissions: { adminNote: false, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, __typename: 'NotePermissions', }, author: { @@ -1979,8 +2038,7 @@ export const mockMoreWorkItemNotesResponse = { __typename: 'Discussion', }, { - id: - 'gid://gitlab/IndividualNoteDiscussion/0f2f195ec0d1ef95ee9d5b10446b8e96a7d83864', + id: 'gid://gitlab/Discussion/0f2f195ec0d1ef95ee9d5b10446b8e96a7d83864', notes: { nodes: [ { @@ -1990,8 +2048,16 @@ export const mockMoreWorkItemNotesResponse = { createdAt: '2022-11-25T07:16:20Z', system: true, internal: false, + discussion: { + id: 'gid://gitlab/Discussion/9c17769ca29798eddaed539d010da12723569876', + }, userPermissions: { adminNote: false, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, __typename: 'NotePermissions', }, author: { @@ -2025,6 +2091,51 @@ export const createWorkItemNoteResponse = { data: { createNote: { errors: [], + note: { + discussion: { + id: 'gid://gitlab/Discussion/c872ba2d7d3eb780d2255138d67ca8b04f65b122', + notes: { + nodes: [ + { + id: 'gid://gitlab/Note/569', + bodyHtml: '<p data-sourcepos="1:1-1:9" dir="auto">Main comment</p>', + system: false, + internal: false, + systemNoteIconName: null, + createdAt: '2023-01-25T04:49:46Z', + discussion: { + id: 'gid://gitlab/Discussion/c872ba2d7d3eb780d2255138d67ca8b04f65b122', + __typename: 'Discussion', + }, + author: { + id: 'gid://gitlab/User/1', + avatarUrl: + 'https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon', + name: 'Administrator', + username: 'root', + webUrl: 'http://127.0.0.1:3000/root', + __typename: 'UserCore', + }, + userPermissions: { + adminNote: true, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, + __typename: 'NotePermissions', + }, + __typename: 'Note', + }, + ], + __typename: 'NoteConnection', + }, + __typename: 'Discussion', + }, + body: 'Latest 22', + bodyHtml: '<p data-sourcepos="1:1-1:9" dir="auto">Latest 22</p>', + __typename: 'Note', + }, __typename: 'CreateNotePayload', }, }, @@ -2038,8 +2149,16 @@ export const mockWorkItemCommentNote = { createdAt: '2022-11-25T07:16:20Z', system: false, internal: false, + discussion: { + id: 'gid://gitlab/Discussion/9c17769ca29798eddaed539d010da12723569876', + }, userPermissions: { adminNote: false, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, __typename: 'NotePermissions', }, author: { @@ -2051,3 +2170,165 @@ export const mockWorkItemCommentNote = { __typename: 'UserCore', }, }; + +export const mockWorkItemNotesResponseWithComments = { + data: { + workItem: { + id: 'gid://gitlab/WorkItem/600', + iid: '60', + widgets: [ + { + __typename: 'WorkItemWidgetIteration', + }, + { + __typename: 'WorkItemWidgetWeight', + }, + { + __typename: 'WorkItemWidgetAssignees', + }, + { + __typename: 'WorkItemWidgetLabels', + }, + { + __typename: 'WorkItemWidgetDescription', + }, + { + __typename: 'WorkItemWidgetHierarchy', + }, + { + __typename: 'WorkItemWidgetStartAndDueDate', + }, + { + __typename: 'WorkItemWidgetMilestone', + }, + { + type: 'NOTES', + discussions: { + pageInfo: { + hasNextPage: false, + hasPreviousPage: false, + startCursor: null, + endCursor: null, + __typename: 'PageInfo', + }, + nodes: [ + { + id: 'gid://gitlab/Discussion/8bbc4890b6ff0f2cde93a5a0947cd2b8a13d3b6e', + notes: { + nodes: [ + { + id: 'gid://gitlab/DiscussionNote/174', + bodyHtml: '<p data-sourcepos="1:1-1:15" dir="auto">Separate thread</p>', + system: false, + internal: false, + systemNoteIconName: null, + createdAt: '2023-01-12T07:47:40Z', + discussion: { + id: 'gid://gitlab/Discussion/2bb1162fd0d39297d1a68fdd7d4083d3780af0f3', + __typename: 'Discussion', + }, + author: { + id: 'gid://gitlab/User/1', + avatarUrl: + 'https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon', + name: 'Administrator', + username: 'root', + webUrl: 'http://127.0.0.1:3000/root', + __typename: 'UserCore', + }, + userPermissions: { + adminNote: true, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, + __typename: 'NotePermissions', + }, + __typename: 'Note', + }, + { + id: 'gid://gitlab/DiscussionNote/235', + bodyHtml: '<p data-sourcepos="1:1-1:15" dir="auto">Thread comment</p>', + system: false, + internal: false, + systemNoteIconName: null, + createdAt: '2023-01-18T09:09:54Z', + discussion: { + id: 'gid://gitlab/Discussion/2bb1162fd0d39297d1a68fdd7d4083d3780af0f3', + __typename: 'Discussion', + }, + author: { + id: 'gid://gitlab/User/1', + avatarUrl: + 'https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon', + name: 'Administrator', + username: 'root', + webUrl: 'http://127.0.0.1:3000/root', + __typename: 'UserCore', + }, + userPermissions: { + adminNote: true, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, + __typename: 'NotePermissions', + }, + __typename: 'Note', + }, + ], + __typename: 'NoteConnection', + }, + __typename: 'Discussion', + }, + { + id: 'gid://gitlab/Discussion/0f2f195ec0d1ef95ee9d5b10446b8e96a7d83864', + notes: { + nodes: [ + { + id: 'gid://gitlab/WeightNote/0f2f195ec0d1ef95ee9d5b10446b8e96a9883864', + bodyHtml: '<p data-sourcepos="1:1-1:15" dir="auto">Main thread 2</p>', + systemNoteIconName: 'weight', + createdAt: '2022-11-25T07:16:20Z', + system: false, + internal: false, + discussion: { + id: 'gid://gitlab/Discussion/9c17769ca29798eddaed539d010da12723560987', + }, + userPermissions: { + adminNote: false, + awardEmoji: true, + readNote: true, + createNote: true, + resolveNote: true, + repositionNote: true, + __typename: 'NotePermissions', + }, + author: { + avatarUrl: + 'https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon', + id: 'gid://gitlab/User/1', + name: 'Administrator', + username: 'root', + webUrl: 'http://127.0.0.1:3000/root', + __typename: 'UserCore', + }, + __typename: 'Note', + }, + ], + __typename: 'NoteConnection', + }, + __typename: 'Discussion', + }, + ], + __typename: 'DiscussionConnection', + }, + __typename: 'WorkItemWidgetNotes', + }, + ], + __typename: 'WorkItem', + }, + }, +}; diff --git a/spec/lib/gitlab/memory/watchdog/configuration_spec.rb b/spec/lib/gitlab/memory/watchdog/configuration_spec.rb index 9242344ead2..4c0f6baef8f 100644 --- a/spec/lib/gitlab/memory/watchdog/configuration_spec.rb +++ b/spec/lib/gitlab/memory/watchdog/configuration_spec.rb @@ -15,7 +15,7 @@ RSpec.describe Gitlab::Memory::Watchdog::Configuration do describe '#handler' do context 'when handler is not set' do it 'defaults to NullHandler' do - expect(configuration.handler).to be(Gitlab::Memory::Watchdog::NullHandler.instance) + expect(configuration.handler).to be(Gitlab::Memory::Watchdog::Handlers::NullHandler.instance) end end end diff --git a/spec/lib/gitlab/memory/watchdog/configurator_spec.rb b/spec/lib/gitlab/memory/watchdog/configurator_spec.rb index a901be84a21..035652abfe6 100644 --- a/spec/lib/gitlab/memory/watchdog/configurator_spec.rb +++ b/spec/lib/gitlab/memory/watchdog/configurator_spec.rb @@ -1,11 +1,8 @@ # frozen_string_literal: true -require 'fast_spec_helper' -require 'prometheus/client' -require 'sidekiq' -require_dependency 'gitlab/cluster/lifecycle_events' +require 'spec_helper' -RSpec.describe Gitlab::Memory::Watchdog::Configurator do +RSpec.describe Gitlab::Memory::Watchdog::Configurator, feature_category: :application_performance do shared_examples 'as configurator' do |handler_class, event_reporter_class, sleep_time_env, sleep_time| it 'configures the correct handler' do configurator.call(configuration) @@ -92,7 +89,7 @@ RSpec.describe Gitlab::Memory::Watchdog::Configurator do end it_behaves_like 'as configurator', - Gitlab::Memory::Watchdog::PumaHandler, + Gitlab::Memory::Watchdog::Handlers::PumaHandler, Gitlab::Memory::Watchdog::EventReporter, 'GITLAB_MEMWD_SLEEP_TIME_SEC', described_class::DEFAULT_SLEEP_INTERVAL_S @@ -200,7 +197,7 @@ RSpec.describe Gitlab::Memory::Watchdog::Configurator do subject(:configurator) { described_class.configure_for_sidekiq } it_behaves_like 'as configurator', - Gitlab::Memory::Watchdog::TermProcessHandler, + Gitlab::Memory::Watchdog::Handlers::SidekiqHandler, Gitlab::Memory::Watchdog::SidekiqEventReporter, 'SIDEKIQ_MEMORY_KILLER_CHECK_INTERVAL', described_class::DEFAULT_SIDEKIQ_SLEEP_INTERVAL_S diff --git a/spec/lib/gitlab/memory/watchdog/handlers/null_handler_spec.rb b/spec/lib/gitlab/memory/watchdog/handlers/null_handler_spec.rb new file mode 100644 index 00000000000..09c76de9611 --- /dev/null +++ b/spec/lib/gitlab/memory/watchdog/handlers/null_handler_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::Memory::Watchdog::Handlers::NullHandler, feature_category: :application_performance do + subject(:handler) { described_class.instance } + + describe '#call' do + it 'does nothing' do + expect(handler.call).to be(false) + end + end +end diff --git a/spec/lib/gitlab/memory/watchdog/handlers/puma_handler_spec.rb b/spec/lib/gitlab/memory/watchdog/handlers/puma_handler_spec.rb new file mode 100644 index 00000000000..7df95c1722e --- /dev/null +++ b/spec/lib/gitlab/memory/watchdog/handlers/puma_handler_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::Memory::Watchdog::Handlers::PumaHandler, feature_category: :application_performance do + # rubocop: disable RSpec/VerifiedDoubles + # In tests, the Puma constant is not loaded so we cannot make this an instance_double. + let(:puma_worker_handle_class) { double('Puma::Cluster::WorkerHandle') } + let(:puma_worker_handle) { double('worker') } + # rubocop: enable RSpec/VerifiedDoubles + + subject(:handler) { described_class.new({}) } + + before do + stub_const('::Puma::Cluster::WorkerHandle', puma_worker_handle_class) + allow(puma_worker_handle_class).to receive(:new).and_return(puma_worker_handle) + allow(puma_worker_handle).to receive(:term) + end + + describe '#call' do + it 'invokes orderly termination via Puma API' do + expect(puma_worker_handle).to receive(:term) + + expect(handler.call).to be(true) + end + end +end diff --git a/spec/lib/gitlab/memory/watchdog/handlers/sidekiq_handler_spec.rb b/spec/lib/gitlab/memory/watchdog/handlers/sidekiq_handler_spec.rb new file mode 100644 index 00000000000..d1f303e7731 --- /dev/null +++ b/spec/lib/gitlab/memory/watchdog/handlers/sidekiq_handler_spec.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'sidekiq' + +RSpec.describe Gitlab::Memory::Watchdog::Handlers::SidekiqHandler, feature_category: :application_performance do + let(:sleep_time) { 3 } + let(:shutdown_timeout_seconds) { 30 } + let(:handler_iterations) { 0 } + let(:logger) { instance_double(::Logger) } + let(:pid) { $$ } + + before do + allow(Gitlab::Metrics::System).to receive(:monotonic_time) + .and_return(0, 1, shutdown_timeout_seconds, 0, 1, Sidekiq[:timeout] + 2) + allow(Process).to receive(:kill) + allow(::Sidekiq).to receive(:logger).and_return(logger) + allow(logger).to receive(:warn) + allow(Process).to receive(:getpgrp).and_return(pid) + end + + subject(:handler) do + described_class.new(shutdown_timeout_seconds, sleep_time).tap do |instance| + # We need to defuse `sleep` and stop the handler after n iteration + iterations = 0 + allow(instance).to receive(:sleep) do + if (iterations += 1) > handler_iterations + instance.stop + end + end + end + end + + describe '#call' do + shared_examples_for 'handler issues kill command' do + it 'logs sending signal' do + logs.each do |log| + expect(::Sidekiq.logger).to receive(:warn).once.ordered.with(log) + end + + handler.call + end + + it 'sends TERM to the current process' do + signal_params.each do |args| + expect(Process).to receive(:kill).once.ordered.with(*args.first(2)) + end + + expect(handler.call).to be(true) + end + end + + def log(signal, pid, explanation, wait_time = nil) + { + pid: pid, + worker_id: ::Prometheus::PidProvider.worker_id, + memwd_handler_class: described_class.to_s, + memwd_signal: signal, + memwd_explanation: explanation, + memwd_wait_time: wait_time, + message: "Sending signal and waiting" + } + end + + let(:logs) do + signal_params.map { |args| log(*args) } + end + + context "when stop is received after TSTP" do + let(:signal_params) do + [ + [:TSTP, pid, 'stop fetching new jobs', shutdown_timeout_seconds] + ] + end + + it_behaves_like 'handler issues kill command' + end + + context "when stop is received after TERM" do + let(:handler_iterations) { 1 } + let(:signal_params) do + [ + [:TSTP, pid, 'stop fetching new jobs', shutdown_timeout_seconds], + [:TERM, pid, 'gracefully shut down', Sidekiq[:timeout] + 2] + ] + end + + it_behaves_like 'handler issues kill command' + end + + context "when stop is not received" do + let(:handler_iterations) { 2 } + let(:gpid) { pid + 1 } + let(:kill_pid) { pid } + let(:signal_params) do + [ + [:TSTP, pid, 'stop fetching new jobs', shutdown_timeout_seconds], + [:TERM, pid, 'gracefully shut down', Sidekiq[:timeout] + 2], + [:KILL, kill_pid, 'hard shut down', nil] + ] + end + + before do + allow(Process).to receive(:getpgrp).and_return(gpid) + end + + context 'when process is not group leader' do + it_behaves_like 'handler issues kill command' + end + + context 'when process is a group leader' do + let(:gpid) { pid } + let(:kill_pid) { 0 } + + it_behaves_like 'handler issues kill command' + end + end + end +end diff --git a/spec/lib/gitlab/memory/watchdog_spec.rb b/spec/lib/gitlab/memory/watchdog_spec.rb index c47dcdf8445..dd6bfb6da2c 100644 --- a/spec/lib/gitlab/memory/watchdog_spec.rb +++ b/spec/lib/gitlab/memory/watchdog_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Gitlab::Memory::Watchdog, :aggregate_failures, feature_category: :application_performance do context 'watchdog' do let(:configuration) { instance_double(described_class::Configuration) } - let(:handler) { instance_double(described_class::NullHandler) } + let(:handler) { instance_double(described_class::Handlers::NullHandler) } let(:reporter) { instance_double(described_class::EventReporter) } let(:sleep_time_seconds) { 60 } let(:threshold_violated) { false } @@ -185,7 +185,7 @@ RSpec.describe Gitlab::Memory::Watchdog, :aggregate_failures, feature_category: it 'always uses the NullHandler' do expect(handler).not_to receive(:call) - expect(described_class::NullHandler.instance).to receive(:call).and_return(true) + expect(described_class::Handlers::NullHandler.instance).to receive(:call).and_return(true) watchdog.call end @@ -216,56 +216,4 @@ RSpec.describe Gitlab::Memory::Watchdog, :aggregate_failures, feature_category: end end end - - context 'handlers' do - context 'NullHandler' do - subject(:handler) { described_class::NullHandler.instance } - - describe '#call' do - it 'does nothing' do - expect(handler.call).to be(false) - end - end - end - - context 'TermProcessHandler' do - subject(:handler) { described_class::TermProcessHandler.new(42) } - - describe '#call' do - before do - allow(Process).to receive(:kill) - end - - it 'sends SIGTERM to the current process' do - expect(Process).to receive(:kill).with(:TERM, 42) - - expect(handler.call).to be(true) - end - end - end - - context 'PumaHandler' do - # rubocop: disable RSpec/VerifiedDoubles - # In tests, the Puma constant is not loaded so we cannot make this an instance_double. - let(:puma_worker_handle_class) { double('Puma::Cluster::WorkerHandle') } - let(:puma_worker_handle) { double('worker') } - # rubocop: enable RSpec/VerifiedDoubles - - subject(:handler) { described_class::PumaHandler.new({}) } - - before do - stub_const('::Puma::Cluster::WorkerHandle', puma_worker_handle_class) - allow(puma_worker_handle_class).to receive(:new).and_return(puma_worker_handle) - allow(puma_worker_handle).to receive(:term) - end - - describe '#call' do - it 'invokes orderly termination via Puma API' do - expect(puma_worker_handle).to receive(:term) - - expect(handler.call).to be(true) - end - end - end - end end |