diff options
72 files changed, 1281 insertions, 129 deletions
diff --git a/app/assets/javascripts/diff.js b/app/assets/javascripts/diff.js index ae8338f5fd2..9ddfdb6ae21 100644 --- a/app/assets/javascripts/diff.js +++ b/app/assets/javascripts/diff.js @@ -17,7 +17,8 @@ class Diff { } }); - FilesCommentButton.init($diffFile); + const tab = document.getElementById('diffs'); + if (!tab || (tab && tab.dataset && tab.dataset.isLocked !== '')) FilesCommentButton.init($diffFile); $diffFile.each((index, file) => new gl.ImageFile(file)); diff --git a/app/assets/javascripts/notes/components/issue_comment_form.vue b/app/assets/javascripts/notes/components/issue_comment_form.vue index 1a7da84a424..ab8516296a8 100644 --- a/app/assets/javascripts/notes/components/issue_comment_form.vue +++ b/app/assets/javascripts/notes/components/issue_comment_form.vue @@ -7,10 +7,12 @@ import TaskList from '../../task_list'; import * as constants from '../constants'; import eventHub from '../event_hub'; - import confidentialIssue from '../../vue_shared/components/issue/confidential_issue_warning.vue'; + import issueWarning from '../../vue_shared/components/issue/issue_warning.vue'; import issueNoteSignedOutWidget from './issue_note_signed_out_widget.vue'; + import issueDiscussionLockedWidget from './issue_discussion_locked_widget.vue'; import markdownField from '../../vue_shared/components/markdown/field.vue'; import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue'; + import issuableStateMixin from '../mixins/issuable_state'; export default { name: 'issueCommentForm', @@ -26,8 +28,9 @@ }; }, components: { - confidentialIssue, + issueWarning, issueNoteSignedOutWidget, + issueDiscussionLockedWidget, markdownField, userAvatarLink, }, @@ -55,6 +58,9 @@ isIssueOpen() { return this.issueState === constants.OPENED || this.issueState === constants.REOPENED; }, + canCreateNote() { + return this.getIssueData.current_user.can_create_note; + }, issueActionButtonTitle() { if (this.note.length) { const actionText = this.isIssueOpen ? 'close' : 'reopen'; @@ -90,9 +96,6 @@ endpoint() { return this.getIssueData.create_note_path; }, - isConfidentialIssue() { - return this.getIssueData.confidential; - }, }, methods: { ...mapActions([ @@ -220,6 +223,9 @@ }); }, }, + mixins: [ + issuableStateMixin, + ], mounted() { // jQuery is needed here because it is a custom event being dispatched with jQuery. $(document).on('issuable:change', (e, isClosed) => { @@ -235,6 +241,7 @@ <template> <div> <issue-note-signed-out-widget v-if="!isLoggedIn" /> + <issue-discussion-locked-widget v-else-if="!canCreateNote" /> <ul v-else class="notes notes-form timeline"> @@ -253,15 +260,22 @@ <div class="timeline-content timeline-content-form"> <form ref="commentForm" - class="new-note js-quick-submit common-note-form gfm-form js-main-target-form"> - <confidentialIssue v-if="isConfidentialIssue" /> + class="new-note js-quick-submit common-note-form gfm-form js-main-target-form" + > + <div class="error-alert"></div> + + <issue-warning + v-if="hasWarning(getIssueData)" + :is-locked="isLocked(getIssueData)" + :is-confidential="isConfidential(getIssueData)" + /> + <markdown-field :markdown-preview-path="markdownPreviewPath" :markdown-docs-path="markdownDocsPath" :quick-actions-docs-path="quickActionsDocsPath" :add-spacing-classes="false" - :is-confidential-issue="isConfidentialIssue" ref="markdownField"> <textarea id="note-body" diff --git a/app/assets/javascripts/notes/components/issue_discussion_locked_widget.vue b/app/assets/javascripts/notes/components/issue_discussion_locked_widget.vue new file mode 100644 index 00000000000..e73ec2aaf71 --- /dev/null +++ b/app/assets/javascripts/notes/components/issue_discussion_locked_widget.vue @@ -0,0 +1,19 @@ +<script> + export default { + computed: { + lockIcon() { + return gl.utils.spriteIcon('lock'); + }, + }, + }; + +</script> + +<template> + <div class="disabled-comment text-center"> + <span class="issuable-note-warning"> + <span class="icon" v-html="lockIcon"></span> + <span>This issue is locked. Only <b>project members</b> can comment.</span> + </span> + </div> +</template> diff --git a/app/assets/javascripts/notes/components/issue_note_form.vue b/app/assets/javascripts/notes/components/issue_note_form.vue index 626c0f2ce18..e2539d6b89d 100644 --- a/app/assets/javascripts/notes/components/issue_note_form.vue +++ b/app/assets/javascripts/notes/components/issue_note_form.vue @@ -1,8 +1,9 @@ <script> import { mapGetters } from 'vuex'; import eventHub from '../event_hub'; - import confidentialIssue from '../../vue_shared/components/issue/confidential_issue_warning.vue'; + import issueWarning from '../../vue_shared/components/issue/issue_warning.vue'; import markdownField from '../../vue_shared/components/markdown/field.vue'; + import issuableStateMixin from '../mixins/issuable_state'; export default { name: 'issueNoteForm', @@ -39,12 +40,13 @@ }; }, components: { - confidentialIssue, + issueWarning, markdownField, }, computed: { ...mapGetters([ 'getDiscussionLastNote', + 'getIssueData', 'getIssueDataByProp', 'getNotesDataByProp', 'getUserDataByProp', @@ -67,9 +69,6 @@ isDisabled() { return !this.note.length || this.isSubmitting; }, - isConfidentialIssue() { - return this.getIssueDataByProp('confidential'); - }, }, methods: { handleUpdate() { @@ -95,6 +94,9 @@ this.$emit('cancelFormEdition', shouldConfirm, this.noteBody !== this.note); }, }, + mixins: [ + issuableStateMixin, + ], mounted() { this.$refs.textarea.focus(); }, @@ -125,7 +127,13 @@ <div class="flash-container timeline-content"></div> <form class="edit-note common-note-form js-quick-submit gfm-form"> - <confidentialIssue v-if="isConfidentialIssue" /> + + <issue-warning + v-if="hasWarning(getIssueData)" + :is-locked="isLocked(getIssueData)" + :is-confidential="isConfidential(getIssueData)" + /> + <markdown-field :markdown-preview-path="markdownPreviewPath" :markdown-docs-path="markdownDocsPath" diff --git a/app/assets/javascripts/notes/mixins/issuable_state.js b/app/assets/javascripts/notes/mixins/issuable_state.js new file mode 100644 index 00000000000..97f3ea0d5de --- /dev/null +++ b/app/assets/javascripts/notes/mixins/issuable_state.js @@ -0,0 +1,15 @@ +export default { + methods: { + isConfidential(issue) { + return !!issue.confidential; + }, + + isLocked(issue) { + return !!issue.discussion_locked; + }, + + hasWarning(issue) { + return this.isConfidential(issue) || this.isLocked(issue); + }, + }, +}; diff --git a/app/assets/javascripts/sidebar/components/confidential/confidential_issue_sidebar.vue b/app/assets/javascripts/sidebar/components/confidential/confidential_issue_sidebar.vue index 8e7abdbffef..f2b1099a678 100644 --- a/app/assets/javascripts/sidebar/components/confidential/confidential_issue_sidebar.vue +++ b/app/assets/javascripts/sidebar/components/confidential/confidential_issue_sidebar.vue @@ -47,9 +47,9 @@ export default { </script> <template> - <div class="block confidentiality"> + <div class="block issuable-sidebar-item confidentiality"> <div class="sidebar-collapsed-icon"> - <i class="fa" :class="faEye" aria-hidden="true" data-hidden="true"></i> + <i class="fa" :class="faEye" aria-hidden="true"></i> </div> <div class="title hide-collapsed"> Confidentiality @@ -62,19 +62,19 @@ export default { Edit </a> </div> - <div class="value confidential-value hide-collapsed"> + <div class="value sidebar-item-value hide-collapsed"> <editForm v-if="edit" :toggle-form="toggleForm" :is-confidential="isConfidential" :update-confidential-attribute="updateConfidentialAttribute" /> - <div v-if="!isConfidential" class="no-value confidential-value"> - <i class="fa fa-eye is-not-confidential"></i> + <div v-if="!isConfidential" class="no-value sidebar-item-value"> + <i class="fa fa-eye sidebar-item-icon"></i> Not confidential </div> - <div v-else class="value confidential-value hide-collapsed"> - <i aria-hidden="true" data-hidden="true" class="fa fa-eye-slash is-confidential"></i> + <div v-else class="value sidebar-item-value hide-collapsed"> + <i aria-hidden="true" class="fa fa-eye-slash sidebar-item-icon is-active"></i> This issue is confidential </div> </div> diff --git a/app/assets/javascripts/sidebar/components/confidential/edit_form.vue b/app/assets/javascripts/sidebar/components/confidential/edit_form.vue index d578b663a54..dd17b5abd46 100644 --- a/app/assets/javascripts/sidebar/components/confidential/edit_form.vue +++ b/app/assets/javascripts/sidebar/components/confidential/edit_form.vue @@ -2,9 +2,6 @@ import editFormButtons from './edit_form_buttons.vue'; export default { - components: { - editFormButtons, - }, props: { isConfidential: { required: true, @@ -19,12 +16,16 @@ export default { type: Function, }, }, + + components: { + editFormButtons, + }, }; </script> <template> <div class="dropdown open"> - <div class="dropdown-menu confidential-warning-message"> + <div class="dropdown-menu sidebar-item-warning-message"> <div> <p v-if="!isConfidential"> You are going to turn on the confidentiality. This means that only team members with diff --git a/app/assets/javascripts/sidebar/components/confidential/edit_form_buttons.vue b/app/assets/javascripts/sidebar/components/confidential/edit_form_buttons.vue index 97af4a3f505..7ed0619ee6b 100644 --- a/app/assets/javascripts/sidebar/components/confidential/edit_form_buttons.vue +++ b/app/assets/javascripts/sidebar/components/confidential/edit_form_buttons.vue @@ -15,7 +15,7 @@ export default { }, }, computed: { - onOrOff() { + toggleButtonText() { return this.isConfidential ? 'Turn Off' : 'Turn On'; }, updateConfidentialBool() { @@ -26,7 +26,7 @@ export default { </script> <template> - <div class="confidential-warning-message-actions"> + <div class="sidebar-item-warning-message-actions"> <button type="button" class="btn btn-default append-right-10" @@ -39,7 +39,7 @@ export default { class="btn btn-close" @click.prevent="updateConfidentialAttribute(updateConfidentialBool)" > - {{ onOrOff }} + {{ toggleButtonText }} </button> </div> </template> diff --git a/app/assets/javascripts/sidebar/components/lock/edit_form.vue b/app/assets/javascripts/sidebar/components/lock/edit_form.vue new file mode 100644 index 00000000000..c7a6edc7c70 --- /dev/null +++ b/app/assets/javascripts/sidebar/components/lock/edit_form.vue @@ -0,0 +1,61 @@ +<script> +import editFormButtons from './edit_form_buttons.vue'; +import issuableMixin from '../../../vue_shared/mixins/issuable'; + +export default { + props: { + isLocked: { + required: true, + type: Boolean, + }, + + toggleForm: { + required: true, + type: Function, + }, + + updateLockedAttribute: { + required: true, + type: Function, + }, + + issuableType: { + required: true, + type: String, + }, + }, + + mixins: [ + issuableMixin, + ], + + components: { + editFormButtons, + }, +}; +</script> + +<template> + <div class="dropdown open"> + <div class="dropdown-menu sidebar-item-warning-message"> + <p class="text" v-if="isLocked"> + Unlock this {{ issuableDisplayName(issuableType) }}? + <strong>Everyone</strong> + will be able to comment. + </p> + + <p class="text" v-else> + Lock this {{ issuableDisplayName(issuableType) }}? + Only + <strong>project members</strong> + will be able to comment. + </p> + + <edit-form-buttons + :is-locked="isLocked" + :toggle-form="toggleForm" + :update-locked-attribute="updateLockedAttribute" + /> + </div> + </div> +</template> diff --git a/app/assets/javascripts/sidebar/components/lock/edit_form_buttons.vue b/app/assets/javascripts/sidebar/components/lock/edit_form_buttons.vue new file mode 100644 index 00000000000..c3a553a7605 --- /dev/null +++ b/app/assets/javascripts/sidebar/components/lock/edit_form_buttons.vue @@ -0,0 +1,50 @@ +<script> +export default { + props: { + isLocked: { + required: true, + type: Boolean, + }, + + toggleForm: { + required: true, + type: Function, + }, + + updateLockedAttribute: { + required: true, + type: Function, + }, + }, + + computed: { + buttonText() { + return this.isLocked ? this.__('Unlock') : this.__('Lock'); + }, + + toggleLock() { + return !this.isLocked; + }, + }, +}; +</script> + +<template> + <div class="sidebar-item-warning-message-actions"> + <button + type="button" + class="btn btn-default append-right-10" + @click="toggleForm" + > + {{ __('Cancel') }} + </button> + + <button + type="button" + class="btn btn-close" + @click.prevent="updateLockedAttribute(toggleLock)" + > + {{ buttonText }} + </button> + </div> +</template> diff --git a/app/assets/javascripts/sidebar/components/lock/lock_issue_sidebar.vue b/app/assets/javascripts/sidebar/components/lock/lock_issue_sidebar.vue new file mode 100644 index 00000000000..c4b2900e020 --- /dev/null +++ b/app/assets/javascripts/sidebar/components/lock/lock_issue_sidebar.vue @@ -0,0 +1,120 @@ +<script> +/* global Flash */ +import editForm from './edit_form.vue'; +import issuableMixin from '../../../vue_shared/mixins/issuable'; + +export default { + props: { + isLocked: { + required: true, + type: Boolean, + }, + + isEditable: { + required: true, + type: Boolean, + }, + + mediator: { + required: true, + type: Object, + validator(mediatorObject) { + return mediatorObject.service && mediatorObject.service.update && mediatorObject.store; + }, + }, + + issuableType: { + required: true, + type: String, + }, + }, + + mixins: [ + issuableMixin, + ], + + components: { + editForm, + }, + + computed: { + lockIconClass() { + return this.isLocked ? 'fa-lock' : 'fa-unlock'; + }, + + isLockDialogOpen() { + return this.mediator.store.isLockDialogOpen; + }, + }, + + methods: { + toggleForm() { + this.mediator.store.isLockDialogOpen = !this.mediator.store.isLockDialogOpen; + }, + + updateLockedAttribute(locked) { + this.mediator.service.update(this.issuableType, { + discussion_locked: locked, + }) + .then(() => location.reload()) + .catch(() => Flash(this.__(`Something went wrong trying to change the locked state of this ${this.issuableDisplayName(this.issuableType)}`))); + }, + }, +}; +</script> + +<template> + <div class="block issuable-sidebar-item lock"> + <div class="sidebar-collapsed-icon"> + <i + class="fa" + :class="lockIconClass" + aria-hidden="true" + ></i> + </div> + + <div class="title hide-collapsed"> + Lock {{issuableDisplayName(issuableType) }} + <button + v-if="isEditable" + class="pull-right lock-edit btn btn-blank" + type="button" + @click.prevent="toggleForm" + > + {{ __('Edit') }} + </button> + </div> + + <div class="value sidebar-item-value hide-collapsed"> + <edit-form + v-if="isLockDialogOpen" + :toggle-form="toggleForm" + :is-locked="isLocked" + :update-locked-attribute="updateLockedAttribute" + :issuable-type="issuableType" + /> + + <div + v-if="isLocked" + class="value sidebar-item-value" + > + <i + aria-hidden="true" + class="fa fa-lock sidebar-item-icon is-active" + ></i> + {{ __('Locked') }} + </div> + + <div + v-else + class="no-value sidebar-item-value hide-collapsed" + > + <i + aria-hidden="true" + class="fa fa-unlock sidebar-item-icon" + ></i> + {{ __('Unlocked') }} + </div> + </div> + </div> +</template> diff --git a/app/assets/javascripts/sidebar/sidebar_bundle.js b/app/assets/javascripts/sidebar/sidebar_bundle.js index 3d8972050a9..09b9d75c02d 100644 --- a/app/assets/javascripts/sidebar/sidebar_bundle.js +++ b/app/assets/javascripts/sidebar/sidebar_bundle.js @@ -1,46 +1,76 @@ import Vue from 'vue'; -import sidebarTimeTracking from './components/time_tracking/sidebar_time_tracking'; -import sidebarAssignees from './components/assignees/sidebar_assignees'; -import confidential from './components/confidential/confidential_issue_sidebar.vue'; +import SidebarTimeTracking from './components/time_tracking/sidebar_time_tracking'; +import SidebarAssignees from './components/assignees/sidebar_assignees'; +import ConfidentialIssueSidebar from './components/confidential/confidential_issue_sidebar.vue'; import SidebarMoveIssue from './lib/sidebar_move_issue'; +import LockIssueSidebar from './components/lock/lock_issue_sidebar.vue'; +import Translate from '../vue_shared/translate'; import Mediator from './sidebar_mediator'; +Vue.use(Translate); + +function mountConfidentialComponent(mediator) { + const el = document.getElementById('js-confidential-entry-point'); + + if (!el) return; + + const dataNode = document.getElementById('js-confidential-issue-data'); + const initialData = JSON.parse(dataNode.innerHTML); + + const ConfidentialComp = Vue.extend(ConfidentialIssueSidebar); + + new ConfidentialComp({ + propsData: { + isConfidential: initialData.is_confidential, + isEditable: initialData.is_editable, + service: mediator.service, + }, + }).$mount(el); +} + +function mountLockComponent(mediator) { + const el = document.getElementById('js-lock-entry-point'); + + if (!el) return; + + const dataNode = document.getElementById('js-lock-issue-data'); + const initialData = JSON.parse(dataNode.innerHTML); + + const LockComp = Vue.extend(LockIssueSidebar); + + new LockComp({ + propsData: { + isLocked: initialData.is_locked, + isEditable: initialData.is_editable, + mediator, + issuableType: gl.utils.isInIssuePage() ? 'issue' : 'merge_request', + }, + }).$mount(el); +} + function domContentLoaded() { const sidebarOptions = JSON.parse(document.querySelector('.js-sidebar-options').innerHTML); const mediator = new Mediator(sidebarOptions); mediator.fetch(); - const sidebarAssigneesEl = document.querySelector('#js-vue-sidebar-assignees'); - const confidentialEl = document.querySelector('#js-confidential-entry-point'); + const sidebarAssigneesEl = document.getElementById('js-vue-sidebar-assignees'); // Only create the sidebarAssignees vue app if it is found in the DOM // We currently do not use sidebarAssignees for the MR page if (sidebarAssigneesEl) { - new Vue(sidebarAssignees).$mount(sidebarAssigneesEl); + new Vue(SidebarAssignees).$mount(sidebarAssigneesEl); } - if (confidentialEl) { - const dataNode = document.getElementById('js-confidential-issue-data'); - const initialData = JSON.parse(dataNode.innerHTML); + mountConfidentialComponent(mediator); + mountLockComponent(mediator); - const ConfidentialComp = Vue.extend(confidential); - - new ConfidentialComp({ - propsData: { - isConfidential: initialData.is_confidential, - isEditable: initialData.is_editable, - service: mediator.service, - }, - }).$mount(confidentialEl); - - new SidebarMoveIssue( - mediator, - $('.js-move-issue'), - $('.js-move-issue-confirmation-button'), - ).init(); - } + new SidebarMoveIssue( + mediator, + $('.js-move-issue'), + $('.js-move-issue-confirmation-button'), + ).init(); - new Vue(sidebarTimeTracking).$mount('#issuable-time-tracker'); + new Vue(SidebarTimeTracking).$mount('#issuable-time-tracker'); } document.addEventListener('DOMContentLoaded', domContentLoaded); diff --git a/app/assets/javascripts/sidebar/stores/sidebar_store.js b/app/assets/javascripts/sidebar/stores/sidebar_store.js index cc04a2a3fcf..d5d04103f3f 100644 --- a/app/assets/javascripts/sidebar/stores/sidebar_store.js +++ b/app/assets/javascripts/sidebar/stores/sidebar_store.js @@ -15,6 +15,7 @@ export default class SidebarStore { }; this.autocompleteProjects = []; this.moveToProjectId = 0; + this.isLockDialogOpen = false; SidebarStore.singleton = this; } diff --git a/app/assets/javascripts/vue_shared/components/issue/confidential_issue_warning.vue b/app/assets/javascripts/vue_shared/components/issue/confidential_issue_warning.vue deleted file mode 100644 index 397d16331d5..00000000000 --- a/app/assets/javascripts/vue_shared/components/issue/confidential_issue_warning.vue +++ /dev/null @@ -1,16 +0,0 @@ -<script> - export default { - name: 'confidentialIssueWarning', - }; -</script> -<template> - <div class="confidential-issue-warning"> - <i - aria-hidden="true" - class="fa fa-eye-slash"> - </i> - <span> - This is a confidential issue. Your comment will not be visible to the public. - </span> - </div> -</template> diff --git a/app/assets/javascripts/vue_shared/components/issue/issue_warning.vue b/app/assets/javascripts/vue_shared/components/issue/issue_warning.vue new file mode 100644 index 00000000000..16c0a8efcd2 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/issue/issue_warning.vue @@ -0,0 +1,55 @@ +<script> + export default { + props: { + isLocked: { + type: Boolean, + default: false, + required: false, + }, + + isConfidential: { + type: Boolean, + default: false, + required: false, + }, + }, + + computed: { + iconClass() { + return { + 'fa-eye-slash': this.isConfidential, + 'fa-lock': this.isLocked, + }; + }, + + isLockedAndConfidential() { + return this.isConfidential && this.isLocked; + }, + }, + }; +</script> +<template> + <div class="issuable-note-warning"> + <i + aria-hidden="true" + class="fa icon" + :class="iconClass" + v-if="!isLockedAndConfidential" + ></i> + + <span v-if="isLockedAndConfidential"> + {{ __('This issue is confidential and locked.') }} + {{ __('People without permission will never get a notification and won\'t be able to comment.') }} + </span> + + <span v-else-if="isConfidential"> + {{ __('This is a confidential issue.') }} + {{ __('Your comment will not be visible to the public.') }} + </span> + + <span v-else-if="isLocked"> + {{ __('This issue is locked.') }} + {{ __('Only project members can comment.') }} + </span> + </div> +</template> diff --git a/app/assets/javascripts/vue_shared/mixins/issuable.js b/app/assets/javascripts/vue_shared/mixins/issuable.js new file mode 100644 index 00000000000..263361587e0 --- /dev/null +++ b/app/assets/javascripts/vue_shared/mixins/issuable.js @@ -0,0 +1,9 @@ +export default { + methods: { + issuableDisplayName(issuableType) { + const displayName = issuableType.replace(/_/, ' '); + + return this.__ ? this.__(displayName) : displayName; + }, + }, +}; diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index d178bc17462..c77160a678b 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -381,7 +381,11 @@ background: transparent; border: 0; + &:hover, + &:active, &:focus { outline: 0; + background: transparent; + box-shadow: none; } } diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index e04790ab952..5ab40947da9 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -705,3 +705,9 @@ Project Templates Icons $rails: #c00; $node: #353535; $java: #70ad51; + +/* +Issuable warning +*/ +$issuable-warning-size: 24px; +$issuable-warning-icon-margin: 4px; diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 7eb28354e6d..db3b7e89d7b 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -5,27 +5,29 @@ margin-right: auto; } -.is-confidential { +.issuable-warning-icon { color: $orange-600; background-color: $orange-100; border-radius: $border-radius-default; padding: 5px; - margin: 0 3px 0 -4px; + margin: 0 $btn-side-margin 0 0; + width: $issuable-warning-size; + height: $issuable-warning-size; + text-align: center; + + &:first-of-type { + margin-right: $issuable-warning-icon-margin; + } } -.is-not-confidential { +.sidebar-item-icon { border-radius: $border-radius-default; padding: 5px; margin: 0 3px 0 -4px; -} - -.confidentiality { - .is-not-confidential { - margin: auto; - } - .is-confidential { - margin: auto; + &.is-active { + color: $orange-600; + background-color: $orange-50; } } diff --git a/app/assets/stylesheets/pages/note_form.scss b/app/assets/stylesheets/pages/note_form.scss index 74d9acb5490..420bca9ece5 100644 --- a/app/assets/stylesheets/pages/note_form.scss +++ b/app/assets/stylesheets/pages/note_form.scss @@ -101,7 +101,7 @@ } } -.confidential-issue-warning { +.issuable-note-warning { color: $orange-600; background-color: $orange-100; border-radius: $border-radius-default $border-radius-default 0 0; @@ -110,28 +110,52 @@ padding: 3px 12px; margin: auto; align-items: center; + + .icon { + margin-right: $issuable-warning-icon-margin; + } +} + +.disabled-comment .issuable-note-warning { + border: none; + border-radius: $label-border-radius; + padding-top: $gl-vert-padding; + padding-bottom: $gl-vert-padding; + + .icon svg { + position: relative; + top: 2px; + margin-right: $btn-xs-side-margin; + width: $gl-font-size; + height: $gl-font-size; + fill: $orange-600; + } } -.confidential-value { +.sidebar-item-value { .fa { background-color: inherit; } } -.confidential-warning-message { +.sidebar-item-warning-message { line-height: 1.5; padding: 16px; - .confidential-warning-message-actions { + .text { + color: $text-color; + } + + .sidebar-item-warning-message-actions { display: flex; - button { + .btn { flex-grow: 1; } } } -.confidential-issue-warning + .md-area { +.issuable-note-warning + .md-area { border-top-left-radius: 0; border-top-right-radius: 0; } diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 46d31e41ada..925fe4513ee 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -703,6 +703,12 @@ ul.notes { color: $note-disabled-comment-color; padding: 90px 0; + &.discussion-locked { + border: none; + background-color: $white-light; + } + + a { color: $gl-link-color; } diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index ee6e6f80cdd..b7a108a0ebd 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -278,6 +278,7 @@ class Projects::IssuesController < Projects::ApplicationController state_event task_num lock_version + discussion_locked ] + [{ label_ids: [], assignee_ids: [] }] end diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index 6602b204fcb..eb7d7bf374c 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -34,6 +34,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont :target_project_id, :task_num, :title, + :discussion_locked, label_ids: [] ] diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 41a13f6f577..ef7d047b1ad 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -66,7 +66,16 @@ class Projects::NotesController < Projects::ApplicationController params.merge(last_fetched_at: last_fetched_at) end + def authorize_admin_note! + return access_denied! unless can?(current_user, :admin_note, note) + end + def authorize_resolve_note! return access_denied! unless can?(current_user, :resolve_note, note) end + + def authorize_create_note! + return unless noteable.lockable? + access_denied! unless can?(current_user, :create_note, noteable) + end end diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index ce028195e51..c219aa3d6a9 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -130,8 +130,12 @@ module NotesHelper end def can_create_note? + issuable = @issue || @merge_request + if @snippet.is_a?(PersonalSnippet) can?(current_user, :comment_personal_snippet, @snippet) + elsif issuable + can?(current_user, :create_note, issuable) else can?(current_user, :create_note, @project) end diff --git a/app/helpers/system_note_helper.rb b/app/helpers/system_note_helper.rb index d7eaf6ce24d..00fe67d6ffb 100644 --- a/app/helpers/system_note_helper.rb +++ b/app/helpers/system_note_helper.rb @@ -19,7 +19,9 @@ module SystemNoteHelper 'discussion' => 'comment', 'moved' => 'arrow-right', 'outdated' => 'pencil', - 'duplicate' => 'issue-duplicate' + 'duplicate' => 'issue-duplicate', + 'locked' => 'lock', + 'unlocked' => 'lock-open' }.freeze def system_note_icon_name(note) diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb index 1c4ddabcad5..5d75b2aa6a3 100644 --- a/app/models/concerns/noteable.rb +++ b/app/models/concerns/noteable.rb @@ -74,4 +74,8 @@ module Noteable def discussions_can_be_resolved_by?(user) discussions_to_be_resolved.all? { |discussion| discussion.can_resolve?(user) } end + + def lockable? + [MergeRequest, Issue].include?(self.class) + end end diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 0b33e45473b..1f9f8d7286b 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -2,7 +2,7 @@ class SystemNoteMetadata < ActiveRecord::Base ICON_TYPES = %w[ commit description merge confidential visible label assignee cross_reference title time_tracking branch milestone discussion task moved - opened closed merged duplicate + opened closed merged duplicate locked unlocked outdated ].freeze diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index daf6fa9e18a..f0aa16d2ecf 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -1,6 +1,10 @@ class IssuablePolicy < BasePolicy delegate { @subject.project } + condition(:locked, scope: :subject, score: 0) { @subject.discussion_locked? } + + condition(:is_project_member) { @user && @subject.project && @subject.project.team.member?(@user) } + desc "User is the assignee or author" condition(:assignee_or_author) do @user && @subject.assignee_or_author?(@user) @@ -12,4 +16,12 @@ class IssuablePolicy < BasePolicy enable :read_merge_request enable :update_merge_request end + + rule { locked & ~is_project_member }.policy do + prevent :create_note + prevent :update_note + prevent :admin_note + prevent :resolve_note + prevent :edit_note + end end diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb index 20cd51cfb99..d4cb5a77e63 100644 --- a/app/policies/note_policy.rb +++ b/app/policies/note_policy.rb @@ -1,5 +1,6 @@ class NotePolicy < BasePolicy delegate { @subject.project } + delegate { @subject.noteable if @subject.noteable.lockable? } condition(:is_author) { @user && @subject.author == @user } condition(:for_merge_request, scope: :subject) { @subject.for_merge_request? } @@ -8,6 +9,7 @@ class NotePolicy < BasePolicy condition(:editable, scope: :subject) { @subject.editable? } rule { ~editable | anonymous }.prevent :edit_note + rule { is_author | admin }.enable :edit_note rule { can?(:master_access) }.enable :edit_note diff --git a/app/serializers/issue_entity.rb b/app/serializers/issue_entity.rb index 0d6feb78173..10d3ad0214b 100644 --- a/app/serializers/issue_entity.rb +++ b/app/serializers/issue_entity.rb @@ -3,6 +3,7 @@ class IssueEntity < IssuableEntity expose :branch_name expose :confidential + expose :discussion_locked expose :assignees, using: API::Entities::UserBasic expose :due_date expose :moved_to_id @@ -14,7 +15,7 @@ class IssueEntity < IssuableEntity expose :current_user do expose :can_create_note do |issue| - can?(request.current_user, :create_note, issue.project) + can?(request.current_user, :create_note, issue) end expose :can_update do |issue| diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 12604e7eb5d..f83ece7098f 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -43,6 +43,10 @@ class IssuableBaseService < BaseService SystemNoteService.change_time_spent(issuable, issuable.project, current_user) end + def create_discussion_lock_note(issuable) + SystemNoteService.discussion_lock(issuable, current_user) + end + def filter_params(issuable) ability_name = :"admin_#{issuable.to_ability_name}" @@ -57,6 +61,7 @@ class IssuableBaseService < BaseService params.delete(:due_date) params.delete(:canonical_issue_id) params.delete(:project) + params.delete(:discussion_locked) end filter_assignee(issuable) @@ -236,6 +241,7 @@ class IssuableBaseService < BaseService handle_common_system_notes(issuable, old_labels: old_labels) end + change_discussion_lock(issuable) handle_changes( issuable, old_labels: old_labels, @@ -294,6 +300,12 @@ class IssuableBaseService < BaseService end end + def change_discussion_lock(issuable) + if issuable.previous_changes.include?('discussion_locked') + create_discussion_lock_note(issuable) + end + end + def toggle_award(issuable) award = params.delete(:emoji_award) if award diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 1f66a2668f9..7b32e215c7f 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -591,6 +591,13 @@ module SystemNoteService create_note(NoteSummary.new(noteable, project, author, body, action: 'duplicate')) end + def discussion_lock(issuable, author) + action = issuable.discussion_locked? ? 'locked' : 'unlocked' + body = "#{action} this issue" + + create_note(NoteSummary.new(issuable, issuable.project, author, body, action: action)) + end + private def notes_for_mentioner(mentioner, noteable, notes) diff --git a/app/views/projects/_md_preview.html.haml b/app/views/projects/_md_preview.html.haml index 71424593f2e..770608eddff 100644 --- a/app/views/projects/_md_preview.html.haml +++ b/app/views/projects/_md_preview.html.haml @@ -1,5 +1,12 @@ - referenced_users = local_assigns.fetch(:referenced_users, nil) +- if defined?(@merge_request) && @merge_request.discussion_locked? + .issuable-note-warning + = icon('lock', class: 'icon') + %span + = _('This merge request is locked.') + = _('Only project members can comment.') + .md-area .md-header %ul.nav-links.clearfix diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index fbaf88356bf..b9fec8af4d7 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -27,7 +27,9 @@ .issuable-meta - if @issue.confidential - = icon('eye-slash', class: 'is-confidential') + = icon('eye-slash', class: 'issuable-warning-icon') + - if @issue.discussion_locked? + = icon('lock', class: 'issuable-warning-icon') = issuable_meta(@issue, @project, "Issue") .issuable-actions.js-issuable-actions diff --git a/app/views/projects/merge_requests/_mr_title.html.haml b/app/views/projects/merge_requests/_mr_title.html.haml index 9ff85c2ee4c..cb723fe6a18 100644 --- a/app/views/projects/merge_requests/_mr_title.html.haml +++ b/app/views/projects/merge_requests/_mr_title.html.haml @@ -15,6 +15,8 @@ = icon('angle-double-left') .issuable-meta + - if @merge_request.discussion_locked? + = icon('lock', class: 'issuable-warning-icon') = issuable_meta(@merge_request, @project, "Merge request") .issuable-actions.js-issuable-actions diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index d3742f3e4be..d88e3d794d3 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/app/views/projects/merge_requests/show.html.haml @@ -83,7 +83,7 @@ #pipelines.pipelines.tab-pane - if @pipelines.any? = render 'projects/commit/pipelines_list', disable_initialization: true, endpoint: pipelines_project_merge_request_path(@project, @merge_request) - #diffs.diffs.tab-pane + #diffs.diffs.tab-pane{ data: { "is-locked" => @merge_request.discussion_locked? } } -# This tab is always loaded via AJAX .mr-loading-status diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index 674f13ddb23..7b7411b1e23 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -119,6 +119,10 @@ %script#js-confidential-issue-data{ type: "application/json" }= { is_confidential: @issue.confidential, is_editable: can_edit_issuable }.to_json.html_safe #js-confidential-entry-point + - if issuable.has_attribute?(:discussion_locked) + %script#js-lock-issue-data{ type: "application/json" }= { is_locked: issuable.discussion_locked?, is_editable: can_edit_issuable }.to_json.html_safe + #js-lock-entry-point + = render "shared/issuable/participants", participants: issuable.participants(current_user) - if current_user - subscribed = issuable.subscribed?(current_user, @project) diff --git a/app/views/shared/notes/_notes_with_form.html.haml b/app/views/shared/notes/_notes_with_form.html.haml index e3e86709b8f..c6e18108c7a 100644 --- a/app/views/shared/notes/_notes_with_form.html.haml +++ b/app/views/shared/notes/_notes_with_form.html.haml @@ -1,3 +1,6 @@ +- issuable = @issue || @merge_request +- discussion_locked = issuable&.discussion_locked? + %ul#notes-list.notes.main-notes-list.timeline = render "shared/notes/notes" @@ -21,5 +24,14 @@ or = link_to "sign in", new_session_path(:user, redirect_to_referer: 'yes'), class: 'js-sign-in-link' to comment - +- elsif discussion_locked + .disabled-comment.text-center.prepend-top-default + %span.issuable-note-warning + %span.icon= sprite_icon('lock', size: 14) + %span + This + = issuable.class.to_s.titleize.downcase + is locked. Only + %b project members + can comment. %script.js-notes-data{ type: "application/json" }= initial_notes_data(autocomplete).to_json.html_safe diff --git a/changelogs/unreleased/18608-lock-issues.yml b/changelogs/unreleased/18608-lock-issues.yml new file mode 100644 index 00000000000..7d907f744f6 --- /dev/null +++ b/changelogs/unreleased/18608-lock-issues.yml @@ -0,0 +1,4 @@ +title: Discussion lock for issues and merge requests +merge_request: +author: +type: added diff --git a/db/migrate/20170815221154_add_discussion_locked_to_issuable.rb b/db/migrate/20170815221154_add_discussion_locked_to_issuable.rb new file mode 100644 index 00000000000..5bd777c53a0 --- /dev/null +++ b/db/migrate/20170815221154_add_discussion_locked_to_issuable.rb @@ -0,0 +1,13 @@ +class AddDiscussionLockedToIssuable < ActiveRecord::Migration + DOWNTIME = false + + def up + add_column(:merge_requests, :discussion_locked, :boolean) + add_column(:issues, :discussion_locked, :boolean) + end + + def down + remove_column(:merge_requests, :discussion_locked) + remove_column(:issues, :discussion_locked) + end +end diff --git a/db/schema.rb b/db/schema.rb index 46b0ac03418..76985c416af 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -677,6 +677,7 @@ ActiveRecord::Schema.define(version: 20171005130944) do t.integer "cached_markdown_version" t.datetime "last_edited_at" t.integer "last_edited_by_id" + t.boolean "discussion_locked" end add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree @@ -900,6 +901,7 @@ ActiveRecord::Schema.define(version: 20171005130944) do t.integer "head_pipeline_id" t.boolean "ref_fetched" t.string "merge_jid" + t.boolean "discussion_locked" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree diff --git a/doc/api/issues.md b/doc/api/issues.md index cd2cfe8e430..ec8ff3cd3f3 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -110,7 +110,8 @@ Example response: "human_time_estimate": null, "human_total_time_spent": null }, - "confidential": false + "confidential": false, + "discussion_locked": false } ] ``` @@ -216,7 +217,8 @@ Example response: "human_time_estimate": null, "human_total_time_spent": null }, - "confidential": false + "confidential": false, + "discussion_locked": false } ] ``` @@ -323,7 +325,8 @@ Example response: "human_time_estimate": null, "human_total_time_spent": null }, - "confidential": false + "confidential": false, + "discussion_locked": false } ] ``` @@ -407,6 +410,7 @@ Example response: "human_total_time_spent": null }, "confidential": false, + "discussion_locked": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", "notes": "http://example.com/api/v4/projects/1/issues/2/notes", @@ -482,6 +486,7 @@ Example response: "human_total_time_spent": null }, "confidential": false, + "discussion_locked": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", "notes": "http://example.com/api/v4/projects/1/issues/2/notes", @@ -515,6 +520,8 @@ PUT /projects/:id/issues/:issue_iid | `state_event` | string | no | The state event of an issue. Set `close` to close the issue and `reopen` to reopen it | | `updated_at` | string | no | Date time string, ISO 8601 formatted, e.g. `2016-03-11T03:45:40Z` (requires admin or project owner rights) | | `due_date` | string | no | Date time string in the format YEAR-MONTH-DAY, e.g. `2016-03-11` | +| `discussion_locked` | boolean | no | Flag indicating if the issue's discussion is locked. If the discussion is locked only project members can add or edit comments. | + ```bash curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/4/issues/85?state_event=close @@ -558,6 +565,7 @@ Example response: "human_total_time_spent": null }, "confidential": false, + "discussion_locked": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", "notes": "http://example.com/api/v4/projects/1/issues/2/notes", @@ -657,6 +665,7 @@ Example response: "human_total_time_spent": null }, "confidential": false, + "discussion_locked": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", "notes": "http://example.com/api/v4/projects/1/issues/2/notes", @@ -735,6 +744,7 @@ Example response: "human_total_time_spent": null }, "confidential": false, + "discussion_locked": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", "notes": "http://example.com/api/v4/projects/1/issues/2/notes", @@ -765,6 +775,44 @@ POST /projects/:id/issues/:issue_iid/unsubscribe curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/issues/93/unsubscribe ``` +Example response: + +```json +{ + "id": 93, + "iid": 12, + "project_id": 5, + "title": "Incidunt et rerum ea expedita iure quibusdam.", + "description": "Et cumque architecto sed aut ipsam.", + "state": "opened", + "created_at": "2016-04-05T21:41:45.217Z", + "updated_at": "2016-04-07T13:02:37.905Z", + "labels": [], + "milestone": null, + "assignee": { + "name": "Edwardo Grady", + "username": "keyon", + "id": 21, + "state": "active", + "avatar_url": "http://www.gravatar.com/avatar/3e6f06a86cf27fa8b56f3f74f7615987?s=80&d=identicon", + "web_url": "https://gitlab.example.com/keyon" + }, + "author": { + "name": "Vivian Hermann", + "username": "orville", + "id": 11, + "state": "active", + "avatar_url": "http://www.gravatar.com/avatar/5224fd70153710e92fb8bcf79ac29d67?s=80&d=identicon", + "web_url": "https://gitlab.example.com/orville" + }, + "subscribed": false, + "due_date": null, + "web_url": "http://example.com/example/example/issues/12", + "confidential": false, + "discussion_locked": false +} +``` + ## Create a todo Manually creates a todo for the current user on an issue. If @@ -857,7 +905,8 @@ Example response: "downvotes": 0, "due_date": null, "web_url": "http://example.com/example/example/issues/110", - "confidential": false + "confidential": false, + "discussion_locked": false }, "target_url": "https://gitlab.example.com/gitlab-org/gitlab-ci/issues/10", "body": "Vel voluptas atque dicta mollitia adipisci qui at.", diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 63a5dd1445c..50a971102fb 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -201,6 +201,7 @@ Parameters: "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -276,6 +277,7 @@ Parameters: "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -387,6 +389,7 @@ Parameters: "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -480,6 +483,7 @@ POST /projects/:id/merge_requests "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -509,6 +513,7 @@ PUT /projects/:id/merge_requests/:merge_request_iid | `labels` | string | no | Labels for MR as a comma-separated list | | `milestone_id` | integer | no | The ID of a milestone | | `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging | +| `discussion_locked` | boolean | no | Flag indicating if the merge request's discussion is locked. If the discussion is locked only project members can add, edit or resolve comments. | Must include at least one non-required attribute from above. @@ -563,6 +568,7 @@ Must include at least one non-required attribute from above. "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -667,6 +673,7 @@ Parameters: "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -743,6 +750,7 @@ Parameters: "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -1037,7 +1045,8 @@ Example response: "id": 14, "state": "active", "avatar_url": "http://www.gravatar.com/avatar/a7fa515d53450023c83d62986d0658a8?s=80&d=identicon", - "web_url": "https://gitlab.example.com/francisca" + "web_url": "https://gitlab.example.com/francisca", + "discussion_locked": false }, "assignee": { "name": "Dr. Gabrielle Strosin", diff --git a/doc/user/discussions/img/discussion_lock_system_notes.png b/doc/user/discussions/img/discussion_lock_system_notes.png Binary files differnew file mode 100644 index 00000000000..8e8e8e0bc3d --- /dev/null +++ b/doc/user/discussions/img/discussion_lock_system_notes.png diff --git a/doc/user/discussions/img/lock_form_member.png b/doc/user/discussions/img/lock_form_member.png Binary files differnew file mode 100644 index 00000000000..01c6308d24c --- /dev/null +++ b/doc/user/discussions/img/lock_form_member.png diff --git a/doc/user/discussions/img/lock_form_non_member.png b/doc/user/discussions/img/lock_form_non_member.png Binary files differnew file mode 100644 index 00000000000..3bb70b69580 --- /dev/null +++ b/doc/user/discussions/img/lock_form_non_member.png diff --git a/doc/user/discussions/img/turn_off_lock.png b/doc/user/discussions/img/turn_off_lock.png Binary files differnew file mode 100644 index 00000000000..dd05b398a8b --- /dev/null +++ b/doc/user/discussions/img/turn_off_lock.png diff --git a/doc/user/discussions/img/turn_on_lock.png b/doc/user/discussions/img/turn_on_lock.png Binary files differnew file mode 100644 index 00000000000..9597da4e14d --- /dev/null +++ b/doc/user/discussions/img/turn_on_lock.png diff --git a/doc/user/discussions/index.md b/doc/user/discussions/index.md index efea99eb120..ab46befb91c 100644 --- a/doc/user/discussions/index.md +++ b/doc/user/discussions/index.md @@ -153,12 +153,52 @@ comments in greater detail.  +## Locking discussions + +> [Introduced][ce-14531] in GitLab 10.1. + +There might be some cases where a discussion is better off if it's locked down. +For example: + +- Discussions that are several years old and the issue/merge request is closed, + but people continue to try to resurrect the discussion. +- Discussions where someone or a group of people are trolling, are abusive, or + in-general are causing the discussion to be unproductive. + +In locked discussions, only team members can write new comments and edit the old +ones. + +To lock or unlock a discussion, you need to have at least Master [permissions]: + +1. Find the "Lock" section in the sidebar and click **Edit** +1. In the dialog that will appear, you can choose to turn on or turn off the + discussion lock +1. Optionally, leave a comment to explain your reasoning behind that action + +| Turn off discussion lock | Turn on discussion lock | +| :-----------: | :----------: | +|  |  | + +Every change is indicated by a system note in the issue's or merge request's +comments. + + + +Once an issue or merge request is locked, project members can see the indicator +in the comment area, whereas non project members can only see the information +that the discussion is locked. + +| Team member | Not a member | +| :-----------: | :----------: | +|  |  | + [ce-5022]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5022 [ce-7125]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7125 [ce-7527]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7527 [ce-7180]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7180 [ce-8266]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8266 [ce-14053]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14053 +[ce-14531]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14531 [resolve-discussion-button]: img/resolve_discussion_button.png [resolve-comment-button]: img/resolve_comment_button.png [discussion-view]: img/discussion_view.png diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 8ff7b483c2a..be72d42625a 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -25,6 +25,7 @@ The following table depicts the various user permission levels in a project. | Create confidential issue | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | | View confidential issues | (✓) [^2] | ✓ | ✓ | ✓ | ✓ | | Leave comments | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | +| Lock comments | | | | ✓ | ✓ | | See a list of jobs | ✓ [^3] | ✓ | ✓ | ✓ | ✓ | | See a job log | ✓ [^3] | ✓ | ✓ | ✓ | ✓ | | Download and browse job artifacts | ✓ [^3] | ✓ | ✓ | ✓ | ✓ | diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 86dec3ca9f1..5f0bad14839 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -368,6 +368,7 @@ module API end expose :due_date expose :confidential + expose :discussion_locked expose :web_url do |issue, options| Gitlab::UrlBuilder.build(issue) @@ -464,6 +465,7 @@ module API expose :diff_head_sha, as: :sha expose :merge_commit_sha expose :user_notes_count + expose :discussion_locked expose :should_remove_source_branch?, as: :should_remove_source_branch expose :force_remove_source_branch?, as: :force_remove_source_branch diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 1729df2aad0..0df41dcc903 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -48,6 +48,7 @@ module API optional :labels, type: String, desc: 'Comma-separated list of label names' optional :due_date, type: String, desc: 'Date string in the format YEAR-MONTH-DAY' optional :confidential, type: Boolean, desc: 'Boolean parameter if the issue should be confidential' + optional :discussion_locked, type: Boolean, desc: " Boolean parameter indicating if the issue's discussion is locked" end params :issue_params do @@ -193,7 +194,7 @@ module API desc: 'Date time when the issue was updated. Available only for admins and project owners.' optional :state_event, type: String, values: %w[reopen close], desc: 'State of the issue' use :issue_params - at_least_one_of :title, :description, :assignee_ids, :assignee_id, :milestone_id, + at_least_one_of :title, :description, :assignee_ids, :assignee_id, :milestone_id, :discussion_locked, :labels, :created_at, :due_date, :confidential, :state_event end put ':id/issues/:issue_iid' do diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 828bc48383e..be843ec8251 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -214,12 +214,14 @@ module API :remove_source_branch, :state_event, :target_branch, - :title + :title, + :discussion_locked ] optional :title, type: String, allow_blank: false, desc: 'The title of the merge request' optional :target_branch, type: String, allow_blank: false, desc: 'The target branch' optional :state_event, type: String, values: %w[close reopen], desc: 'Status of the merge request' + optional :discussion_locked, type: Boolean, desc: 'Whether the MR discussion is locked' use :optional_params at_least_one_of(*at_least_one_of_ce) diff --git a/lib/api/notes.rb b/lib/api/notes.rb index d6e7203adaf..0b9ab4eeb05 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -78,6 +78,8 @@ module API } if can?(current_user, noteable_read_ability_name(noteable), noteable) + authorize! :create_note, noteable + if params[:created_at] && (current_user.admin? || user_project.owner == current_user) opts[:created_at] = params[:created_at] end diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index c0337f96fc6..e3114e5c06e 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -266,6 +266,56 @@ describe Projects::NotesController do end end end + + context 'when the merge request discussion is locked' do + before do + project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) + merge_request.update_attribute(:discussion_locked, true) + end + + context 'when a noteable is not found' do + it 'returns 404 status' do + request_params[:note][:noteable_id] = 9999 + post :create, request_params.merge(format: :json) + + expect(response).to have_http_status(404) + end + end + + context 'when a user is a team member' do + it 'returns 302 status for html' do + post :create, request_params + + expect(response).to have_http_status(302) + end + + it 'returns 200 status for json' do + post :create, request_params.merge(format: :json) + + expect(response).to have_http_status(200) + end + + it 'creates a new note' do + expect { post :create, request_params }.to change { Note.count }.by(1) + end + end + + context 'when a user is not a team member' do + before do + project.project_member(user).destroy + end + + it 'returns 404 status' do + post :create, request_params + + expect(response).to have_http_status(404) + end + + it 'does not create a new note' do + expect { post :create, request_params }.not_to change { Note.count } + end + end + end end describe 'DELETE destroy' do diff --git a/spec/features/issuables/discussion_lock_spec.rb b/spec/features/issuables/discussion_lock_spec.rb new file mode 100644 index 00000000000..7ea29ff252b --- /dev/null +++ b/spec/features/issuables/discussion_lock_spec.rb @@ -0,0 +1,106 @@ +require 'spec_helper' + +describe 'Discussion Lock', :js do + let(:user) { create(:user) } + let(:issue) { create(:issue, project: project, author: user) } + let(:project) { create(:project, :public) } + + before do + sign_in(user) + end + + context 'when a user is a team member' do + before do + project.add_developer(user) + end + + context 'when the discussion is unlocked' do + it 'the user can lock the issue' do + visit project_issue_path(project, issue) + + expect(find('.issuable-sidebar')).to have_content('Unlocked') + + page.within('.issuable-sidebar') do + find('.lock-edit').click + click_button('Lock') + end + + expect(find('#notes')).to have_content('locked this issue') + end + end + + context 'when the discussion is locked' do + before do + issue.update_attribute(:discussion_locked, true) + visit project_issue_path(project, issue) + end + + it 'the user can unlock the issue' do + expect(find('.issuable-sidebar')).to have_content('Locked') + + page.within('.issuable-sidebar') do + find('.lock-edit').click + click_button('Unlock') + end + + expect(find('#notes')).to have_content('unlocked this issue') + expect(find('.issuable-sidebar')).to have_content('Unlocked') + end + + it 'the user can create a comment' do + page.within('#notes .js-main-target-form') do + fill_in 'note[note]', with: 'Some new comment' + click_button 'Comment' + end + + wait_for_requests + + expect(find('div#notes')).to have_content('Some new comment') + end + end + end + + context 'when a user is not a team member' do + context 'when the discussion is unlocked' do + before do + visit project_issue_path(project, issue) + end + + it 'the user can not lock the issue' do + expect(find('.issuable-sidebar')).to have_content('Unlocked') + expect(find('.issuable-sidebar')).not_to have_selector('.lock-edit') + end + + it 'the user can create a comment' do + page.within('#notes .js-main-target-form') do + fill_in 'note[note]', with: 'Some new comment' + click_button 'Comment' + end + + wait_for_requests + + expect(find('div#notes')).to have_content('Some new comment') + end + end + + context 'when the discussion is locked' do + before do + issue.update_attribute(:discussion_locked, true) + visit project_issue_path(project, issue) + end + + it 'the user can not unlock the issue' do + expect(find('.issuable-sidebar')).to have_content('Locked') + expect(find('.issuable-sidebar')).not_to have_selector('.lock-edit') + end + + it 'the user can not create a comment' do + page.within('#notes') do + expect(page).not_to have_selector('js-main-target-form') + expect(page.find('.disabled-comment')) + .to have_content('This issue is locked. Only project members can comment.') + end + end + end + end +end diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb index aa8cf3b013c..e2db1442d90 100644 --- a/spec/features/issues_spec.rb +++ b/spec/features/issues_spec.rb @@ -609,14 +609,14 @@ describe 'Issues', :js do visit project_issue_path(project, issue) - expect(page).to have_css('.confidential-issue-warning') - expect(page).to have_css('.is-confidential') - expect(page).not_to have_css('.is-not-confidential') + expect(page).to have_css('.issuable-note-warning') + expect(find('.issuable-sidebar-item.confidentiality')).to have_css('.is-active') + expect(find('.issuable-sidebar-item.confidentiality')).not_to have_css('.not-active') find('.confidential-edit').click - expect(page).to have_css('.confidential-warning-message') + expect(page).to have_css('.sidebar-item-warning-message') - within('.confidential-warning-message') do + within('.sidebar-item-warning-message') do find('.btn-close').click end @@ -624,7 +624,7 @@ describe 'Issues', :js do visit project_issue_path(project, issue) - expect(page).not_to have_css('.is-confidential') + expect(page).not_to have_css('.is-active') end end end diff --git a/spec/features/merge_requests/discussion_lock_spec.rb b/spec/features/merge_requests/discussion_lock_spec.rb new file mode 100644 index 00000000000..7bbd3b1e69e --- /dev/null +++ b/spec/features/merge_requests/discussion_lock_spec.rb @@ -0,0 +1,49 @@ +require 'spec_helper' + +describe 'Discussion Lock', :js do + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request, source_project: project, author: user) } + let(:project) { create(:project, :public, :repository) } + + before do + sign_in(user) + end + + context 'when the discussion is locked' do + before do + merge_request.update_attribute(:discussion_locked, true) + end + + context 'when a user is a team member' do + before do + project.add_developer(user) + visit project_merge_request_path(project, merge_request) + end + + it 'the user can create a comment' do + page.within('.issuable-discussion #notes .js-main-target-form') do + fill_in 'note[note]', with: 'Some new comment' + click_button 'Comment' + end + + wait_for_requests + + expect(find('.issuable-discussion #notes')).to have_content('Some new comment') + end + end + + context 'when a user is not a team member' do + before do + visit project_merge_request_path(project, merge_request) + end + + it 'the user can not create a comment' do + page.within('.issuable-discussion #notes') do + expect(page).not_to have_selector('js-main-target-form') + expect(page.find('.disabled-comment')) + .to have_content('This merge request is locked. Only project members can comment.') + end + end + end + end +end diff --git a/spec/fixtures/api/schemas/public_api/v4/issues.json b/spec/fixtures/api/schemas/public_api/v4/issues.json index 03c422ab023..5c08dbc3b96 100644 --- a/spec/fixtures/api/schemas/public_api/v4/issues.json +++ b/spec/fixtures/api/schemas/public_api/v4/issues.json @@ -9,6 +9,7 @@ "title": { "type": "string" }, "description": { "type": ["string", "null"] }, "state": { "type": "string" }, + "discussion_locked": { "type": ["boolean", "null"] }, "closed_at": { "type": "date" }, "created_at": { "type": "date" }, "updated_at": { "type": "date" }, diff --git a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json index 31b3f4ba946..5828be5255b 100644 --- a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json +++ b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json @@ -72,6 +72,7 @@ "user_notes_count": { "type": "integer" }, "should_remove_source_branch": { "type": ["boolean", "null"] }, "force_remove_source_branch": { "type": ["boolean", "null"] }, + "discussion_locked": { "type": ["boolean", "null"] }, "web_url": { "type": "uri" }, "time_stats": { "time_estimate": { "type": "integer" }, diff --git a/spec/javascripts/sidebar/lock/edit_form_buttons_spec.js b/spec/javascripts/sidebar/lock/edit_form_buttons_spec.js new file mode 100644 index 00000000000..b0ea8ae0206 --- /dev/null +++ b/spec/javascripts/sidebar/lock/edit_form_buttons_spec.js @@ -0,0 +1,36 @@ +import Vue from 'vue'; +import editFormButtons from '~/sidebar/components/lock/edit_form_buttons.vue'; +import mountComponent from '../../helpers/vue_mount_component_helper'; + +describe('EditFormButtons', () => { + let vm1; + let vm2; + + beforeEach(() => { + const Component = Vue.extend(editFormButtons); + const toggleForm = () => { }; + const updateLockedAttribute = () => { }; + + vm1 = mountComponent(Component, { + isLocked: true, + toggleForm, + updateLockedAttribute, + }); + + vm2 = mountComponent(Component, { + isLocked: false, + toggleForm, + updateLockedAttribute, + }); + }); + + it('renders unlock or lock text based on locked state', () => { + expect( + vm1.$el.innerHTML.includes('Unlock'), + ).toBe(true); + + expect( + vm2.$el.innerHTML.includes('Lock'), + ).toBe(true); + }); +}); diff --git a/spec/javascripts/sidebar/lock/edit_form_spec.js b/spec/javascripts/sidebar/lock/edit_form_spec.js new file mode 100644 index 00000000000..7abd6997a18 --- /dev/null +++ b/spec/javascripts/sidebar/lock/edit_form_spec.js @@ -0,0 +1,41 @@ +import Vue from 'vue'; +import editForm from '~/sidebar/components/lock/edit_form.vue'; + +describe('EditForm', () => { + let vm1; + let vm2; + + beforeEach(() => { + const Component = Vue.extend(editForm); + const toggleForm = () => { }; + const updateLockedAttribute = () => { }; + + vm1 = new Component({ + propsData: { + isLocked: true, + toggleForm, + updateLockedAttribute, + issuableType: 'issue', + }, + }).$mount(); + + vm2 = new Component({ + propsData: { + isLocked: false, + toggleForm, + updateLockedAttribute, + issuableType: 'merge_request', + }, + }).$mount(); + }); + + it('renders on the appropriate warning text', () => { + expect( + vm1.$el.innerHTML.includes('Unlock this issue?'), + ).toBe(true); + + expect( + vm2.$el.innerHTML.includes('Lock this merge request?'), + ).toBe(true); + }); +}); diff --git a/spec/javascripts/sidebar/lock/lock_issue_sidebar_spec.js b/spec/javascripts/sidebar/lock/lock_issue_sidebar_spec.js new file mode 100644 index 00000000000..696fca516bc --- /dev/null +++ b/spec/javascripts/sidebar/lock/lock_issue_sidebar_spec.js @@ -0,0 +1,71 @@ +import Vue from 'vue'; +import lockIssueSidebar from '~/sidebar/components/lock/lock_issue_sidebar.vue'; + +describe('LockIssueSidebar', () => { + let vm1; + let vm2; + + beforeEach(() => { + const Component = Vue.extend(lockIssueSidebar); + + const mediator = { + service: { + update: Promise.resolve(true), + }, + + store: { + isLockDialogOpen: false, + }, + }; + + vm1 = new Component({ + propsData: { + isLocked: true, + isEditable: true, + mediator, + issuableType: 'issue', + }, + }).$mount(); + + vm2 = new Component({ + propsData: { + isLocked: false, + isEditable: false, + mediator, + issuableType: 'merge_request', + }, + }).$mount(); + }); + + it('shows if locked and/or editable', () => { + expect( + vm1.$el.innerHTML.includes('Edit'), + ).toBe(true); + + expect( + vm1.$el.innerHTML.includes('Locked'), + ).toBe(true); + + expect( + vm2.$el.innerHTML.includes('Unlocked'), + ).toBe(true); + }); + + it('displays the edit form when editable', (done) => { + expect(vm1.isLockDialogOpen).toBe(false); + + vm1.$el.querySelector('.lock-edit').click(); + + expect(vm1.isLockDialogOpen).toBe(true); + + vm1.$nextTick(() => { + expect( + vm1.$el + .innerHTML + .includes('Unlock this issue?'), + ).toBe(true); + + done(); + }); + }); +}); diff --git a/spec/javascripts/vue_shared/components/issue/confidential_issue_warning_spec.js b/spec/javascripts/vue_shared/components/issue/confidential_issue_warning_spec.js deleted file mode 100644 index 6df08f3ebe7..00000000000 --- a/spec/javascripts/vue_shared/components/issue/confidential_issue_warning_spec.js +++ /dev/null @@ -1,20 +0,0 @@ -import Vue from 'vue'; -import confidentialIssue from '~/vue_shared/components/issue/confidential_issue_warning.vue'; - -describe('Confidential Issue Warning Component', () => { - let vm; - - beforeEach(() => { - const Component = Vue.extend(confidentialIssue); - vm = new Component().$mount(); - }); - - afterEach(() => { - vm.$destroy(); - }); - - it('should render confidential issue warning information', () => { - expect(vm.$el.querySelector('i').className).toEqual('fa fa-eye-slash'); - expect(vm.$el.querySelector('span').textContent.trim()).toEqual('This is a confidential issue. Your comment will not be visible to the public.'); - }); -}); diff --git a/spec/javascripts/vue_shared/components/issue/issue_warning_spec.js b/spec/javascripts/vue_shared/components/issue/issue_warning_spec.js new file mode 100644 index 00000000000..2cf4d8e00ed --- /dev/null +++ b/spec/javascripts/vue_shared/components/issue/issue_warning_spec.js @@ -0,0 +1,46 @@ +import Vue from 'vue'; +import issueWarning from '~/vue_shared/components/issue/issue_warning.vue'; +import mountComponent from '../../../helpers/vue_mount_component_helper'; + +const IssueWarning = Vue.extend(issueWarning); + +function formatWarning(string) { + // Replace newlines with a space then replace multiple spaces with one space + return string.trim().replace(/\n/g, ' ').replace(/\s\s+/g, ' '); +} + +describe('Issue Warning Component', () => { + describe('isLocked', () => { + it('should render locked issue warning information', () => { + const vm = mountComponent(IssueWarning, { + isLocked: true, + }); + + expect(vm.$el.querySelector('i').className).toEqual('fa icon fa-lock'); + expect(formatWarning(vm.$el.querySelector('span').textContent)).toEqual('This issue is locked. Only project members can comment.'); + }); + }); + + describe('isConfidential', () => { + it('should render confidential issue warning information', () => { + const vm = mountComponent(IssueWarning, { + isConfidential: true, + }); + + expect(vm.$el.querySelector('i').className).toEqual('fa icon fa-eye-slash'); + expect(formatWarning(vm.$el.querySelector('span').textContent)).toEqual('This is a confidential issue. Your comment will not be visible to the public.'); + }); + }); + + describe('isLocked and isConfidential', () => { + it('should render locked and confidential issue warning information', () => { + const vm = mountComponent(IssueWarning, { + isLocked: true, + isConfidential: true, + }); + + expect(vm.$el.querySelector('i')).toBeFalsy(); + expect(formatWarning(vm.$el.querySelector('span').textContent)).toEqual('This issue is confidential and locked. People without permission will never get a notification and won\'t be able to comment.'); + }); + }); +}); diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 48938346577..0ab493a4246 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -25,6 +25,7 @@ Issue: - relative_position - last_edited_at - last_edited_by_id +- discussion_locked Event: - id - target_type @@ -168,6 +169,7 @@ MergeRequest: - last_edited_at - last_edited_by_id - head_pipeline_id +- discussion_locked MergeRequestDiff: - id - state diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb new file mode 100644 index 00000000000..2cf669e8191 --- /dev/null +++ b/spec/policies/issuable_policy_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' + +describe IssuablePolicy, models: true do + describe '#rules' do + context 'when discussion is locked for the issuable' do + let(:user) { create(:user) } + let(:project) { create(:project, :public) } + let(:issue) { create(:issue, project: project, discussion_locked: true) } + let(:policies) { described_class.new(user, issue) } + + context 'when the user is not a project member' do + it 'can not create a note' do + expect(policies).to be_disallowed(:create_note) + end + end + + context 'when the user is a project member' do + before do + project.add_guest(user) + end + + it 'can create a note' do + expect(policies).to be_allowed(:create_note) + end + end + end + end +end diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb new file mode 100644 index 00000000000..58d36a2c84e --- /dev/null +++ b/spec/policies/note_policy_spec.rb @@ -0,0 +1,71 @@ +require 'spec_helper' + +describe NotePolicy, mdoels: true do + describe '#rules' do + let(:user) { create(:user) } + let(:project) { create(:project, :public) } + let(:issue) { create(:issue, project: project) } + + def policies(noteable = nil) + return @policies if @policies + + noteable ||= issue + note = create(:note, noteable: noteable, author: user, project: project) + + @policies = described_class.new(user, note) + end + + context 'when the project is public' do + context 'when the note author is not a project member' do + it 'can edit a note' do + expect(policies).to be_allowed(:update_note) + expect(policies).to be_allowed(:admin_note) + expect(policies).to be_allowed(:resolve_note) + expect(policies).to be_allowed(:read_note) + end + end + + context 'when the noteable is a snippet' do + it 'can edit note' do + policies = policies(create(:project_snippet, project: project)) + + expect(policies).to be_allowed(:update_note) + expect(policies).to be_allowed(:admin_note) + expect(policies).to be_allowed(:resolve_note) + expect(policies).to be_allowed(:read_note) + end + end + + context 'when a discussion is locked' do + before do + issue.update_attribute(:discussion_locked, true) + end + + context 'when the note author is a project member' do + before do + project.add_developer(user) + end + + it 'can edit a note' do + expect(policies).to be_allowed(:update_note) + expect(policies).to be_allowed(:admin_note) + expect(policies).to be_allowed(:resolve_note) + expect(policies).to be_allowed(:read_note) + end + end + + context 'when the note author is not a project member' do + it 'can not edit a note' do + expect(policies).to be_disallowed(:update_note) + expect(policies).to be_disallowed(:admin_note) + expect(policies).to be_disallowed(:resolve_note) + end + + it 'can read a note' do + expect(policies).to be_allowed(:read_note) + end + end + end + end + end +end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index f5882c0c74a..fb440fa551c 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -302,6 +302,40 @@ describe API::Notes do expect(private_issue.notes.reload).to be_empty end end + + context 'when the merge request discussion is locked' do + before do + merge_request.update_attribute(:discussion_locked, true) + end + + context 'when a user is a team member' do + subject { post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes", user), body: 'Hi!' } + + it 'returns 200 status' do + subject + + expect(response).to have_http_status(201) + end + + it 'creates a new note' do + expect { subject }.to change { Note.count }.by(1) + end + end + + context 'when a user is not a team member' do + subject { post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes", private_user), body: 'Hi!' } + + it 'returns 403 status' do + subject + + expect(response).to have_http_status(403) + end + + it 'does not create a new note' do + expect { subject }.not_to change { Note.count } + end + end + end end describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index a8a8aeed1bd..bdbe0a353fb 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -48,7 +48,8 @@ describe Issues::UpdateService, :mailer do assignee_ids: [user2.id], state_event: 'close', label_ids: [label.id], - due_date: Date.tomorrow + due_date: Date.tomorrow, + discussion_locked: true } end @@ -62,6 +63,7 @@ describe Issues::UpdateService, :mailer do expect(issue).to be_closed expect(issue.labels).to match_array [label] expect(issue.due_date).to eq Date.tomorrow + expect(issue.discussion_locked).to be_truthy end it 'refreshes the number of open issues when the issue is made confidential', :use_clean_rails_memory_store_caching do @@ -110,6 +112,7 @@ describe Issues::UpdateService, :mailer do expect(issue.labels).to be_empty expect(issue.milestone).to be_nil expect(issue.due_date).to be_nil + expect(issue.discussion_locked).to be_falsey end end @@ -148,6 +151,13 @@ describe Issues::UpdateService, :mailer do expect(note).not_to be_nil expect(note.note).to eq 'changed title from **{-Old-} title** to **{+New+} title**' end + + it 'creates system note about discussion lock' do + note = find_note('locked this issue') + + expect(note).not_to be_nil + expect(note.note).to eq 'locked this issue' + end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 681feee61d1..b11a1b31f32 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -49,7 +49,8 @@ describe MergeRequests::UpdateService, :mailer do state_event: 'close', label_ids: [label.id], target_branch: 'target', - force_remove_source_branch: '1' + force_remove_source_branch: '1', + discussion_locked: true } end @@ -73,6 +74,7 @@ describe MergeRequests::UpdateService, :mailer do expect(@merge_request.labels.first.title).to eq(label.name) expect(@merge_request.target_branch).to eq('target') expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') + expect(@merge_request.discussion_locked).to be_truthy end it 'executes hooks with update action' do @@ -123,6 +125,13 @@ describe MergeRequests::UpdateService, :mailer do expect(note.note).to eq 'changed target branch from `master` to `target`' end + it 'creates system note about discussion lock' do + note = find_note('locked this issue') + + expect(note).not_to be_nil + expect(note.note).to eq 'locked this issue' + end + context 'when not including source branch removal options' do before do opts.delete(:force_remove_source_branch) |
