From 9b3896fa2e23f9b58b1e7adc77fdc7ba5477b7a5 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Thu, 6 Jul 2017 17:31:49 +0100 Subject: [PATCH 1/3] Prep _merge_request_approvals_settings for use by merge request form --- app/assets/javascripts/dispatcher.js | 1 + ...merge_request_approvals_settings.html.haml | 42 ++++++++++++------- app/views/shared/issuable/_form.html.haml | 2 +- 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index a9eea036918451..3fe652d830bcb5 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -234,6 +234,7 @@ import AuditLogs from './audit_logs'; new MilestoneSelect(); new gl.IssuableTemplateSelectors(); new AutoWidthDropdownSelect($('.js-target-branch-select')).init(); + new ApproversSelect(); break; case 'projects:tags:new': new ZenMode(); diff --git a/app/views/projects/ee/_merge_request_approvals_settings.html.haml b/app/views/projects/ee/_merge_request_approvals_settings.html.haml index 6fe38d55818b8b..bcd76819c9578f 100644 --- a/app/views/projects/ee/_merge_request_approvals_settings.html.haml +++ b/app/views/projects/ee/_merge_request_approvals_settings.html.haml @@ -1,21 +1,25 @@ - return unless project.feature_available?(:merge_request_approvers) +- merge_request = local_assigns.fetch(:merge_request, nil) +- model_name = merge_request ? 'merge_request' : 'project' +- model = merge_request || project - can_override_approvers = project.can_override_approvers? -.form-group.reset-approvals-on-push - .checkbox - = label_tag :require_approvals do - = check_box_tag :require_approvals, nil, project.approvals_before_merge.nonzero?, class: 'js-require-approvals-toggle' - %strong Merge request approvals - = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals"), target: '_blank' +- unless merge_request + .form-group.reset-approvals-on-push + .checkbox + = label_tag :require_approvals do + = check_box_tag :require_approvals, nil, project.approvals_before_merge.nonzero?, class: 'js-require-approvals-toggle' + %strong Merge request approvals + = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals"), target: '_blank' .nested-settings{ class: project.approvals_before_merge.nonzero? ? '' : 'hidden' } .form-group = form.label :approver_ids, class: 'label-light' do Approvers - = hidden_field_tag "project[approver_ids]" - = hidden_field_tag "project[approver_group_ids]" + = hidden_field_tag "#{model_name}[approver_ids]" + = hidden_field_tag "#{model_name}[approver_group_ids]" .input-group.input-btn-group - = hidden_field_tag :approver_user_and_group_ids, '', { class: 'js-select-user-and-group input-large', tabindex: 1, 'data-name': 'project' } + = hidden_field_tag :approver_user_and_group_ids, '', { class: 'js-select-user-and-group input-large', tabindex: 1, 'data-name': model_name } %button.btn.btn-success.js-add-approvers{ type: 'button', title: 'Add approvers(s)' } Add .help-block @@ -26,9 +30,9 @@ Approvers %span.badge - ids = [] - - project.approvers.each do |user| + - model.approvers.each do |user| - ids << user.user_id - - project.approver_groups.each do |group| + - model.approver_groups.each do |group| - group.users.each do |user| - unless ids.include?(user.id) - ids << user.id @@ -36,13 +40,13 @@ %ul.well-list.approver-list .load-wrapper.hidden = icon('spinner spin', class: 'approver-list-loader') - - project.approvers.each do |approver| + - model.approvers.each do |approver| %li.approver.settings-flex-row.js-approver{ data: { id: approver.user_id } } = link_to approver.user.name, approver.user .pull-right - %button{ href: project_approver_path(project, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn btn-remove js-approver-remove", title: 'Remove approver' } + %button{ href: project_approver_path(project, approver, merge_request_id: "#{merge_request.iid if merge_request}"), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn btn-remove js-approver-remove", title: 'Remove approver' } = icon("trash") - - project.approver_groups.each do |approver_group| + - model.approver_groups.each do |approver_group| %li.approver-group.settings-flex-row.js-approver-group{ data: { id: approver_group.group.id } } .span %span.light @@ -51,9 +55,9 @@ %span.badge = approver_group.group.members.count .pull-right - %button{ href: project_approver_group_path(project, approver_group), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}" }, class: "btn btn-remove js-approver-remove", title: 'Remove group' } + %button{ href: project_approver_group_path(project, approver_group, merge_request_id: "#{merge_request.iid if merge_request}"), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}" }, class: "btn btn-remove js-approver-remove", title: 'Remove group' } = icon("trash") - - if project.approvers.empty? && project.approver_groups.empty? + - if model.approvers.empty? && model.approver_groups.empty? %li There are no approvers .form-group @@ -70,9 +74,15 @@ %strong Can override approvers and approvals required per merge request = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals", anchor: 'can-override-approvers-and-approvals-required-per-merge-request'), target: '_blank' +- unless merge_request .form-group.reset-approvals-on-push .checkbox = form.label :reset_approvals_on_push do = form.check_box :reset_approvals_on_push %strong Remove all approvals in a merge request when new commits are pushed to its source branch +- if merge_request + .help-block.suggested-approvers + - if @suggested_approvers.any? + Suggested approvers: + = raw @suggested_approvers.map { |approver| link_to sanitize(approver.name), "#", id: dom_id(approver) }.join(", ") diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index 87e7d1b41a5923..6c8e9746f3f656 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -41,7 +41,7 @@ title: 'Moving an issue will copy the discussion to a different project and close it here. All participants will be notified of the new location.' } = icon('question-circle') -= render 'shared/issuable/approvals', issuable: issuable, form: form += render 'projects/ee/merge_request_approvals_settings', project: project, merge_request: issuable, form: form if issuable.is_a?(MergeRequest) = render 'shared/issuable/form/branch_chooser', issuable: issuable, form: form -- GitLab From cf7af98d53d303c3d048b372db6268149a19ff7e Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Fri, 7 Jul 2017 13:53:02 +0100 Subject: [PATCH 2/3] Add merge_request_approver_path --- app/assets/javascripts/approvers_select.js | 2 ++ app/helpers/ee/merge_requests_helper.rb | 17 +++++++++++++++++ .../_merge_request_approvals_settings.html.haml | 9 +++++---- app/views/shared/issuable/_form.html.haml | 3 ++- 4 files changed, 26 insertions(+), 5 deletions(-) create mode 100644 app/helpers/ee/merge_requests_helper.rb diff --git a/app/assets/javascripts/approvers_select.js b/app/assets/javascripts/approvers_select.js index 656ed95494e5cb..ca9e0133ce5ae7 100644 --- a/app/assets/javascripts/approvers_select.js +++ b/app/assets/javascripts/approvers_select.js @@ -152,6 +152,8 @@ export default class ApproversSelect { const $loadWrapper = $('.load-wrapper'); const $approverSelect = $('.js-select-user-and-group'); + debugger + if (!newValue) { return; } diff --git a/app/helpers/ee/merge_requests_helper.rb b/app/helpers/ee/merge_requests_helper.rb new file mode 100644 index 00000000000000..900b020e398757 --- /dev/null +++ b/app/helpers/ee/merge_requests_helper.rb @@ -0,0 +1,17 @@ +module EE + module MergeRequestsHelper + def merge_request_approver_path(project, merge_request, approver) + params = {} + params[:merge_request_id] = merge_request.iid if merge_request + + case approver + when Approver + project_approver_path(project, approver, params) + when ApproverGroup + project_approver_group_path(project, approver, params) + else + raise TypeError.new('unknown approver type') + end + end + end +end diff --git a/app/views/projects/ee/_merge_request_approvals_settings.html.haml b/app/views/projects/ee/_merge_request_approvals_settings.html.haml index bcd76819c9578f..41b32d2ad77982 100644 --- a/app/views/projects/ee/_merge_request_approvals_settings.html.haml +++ b/app/views/projects/ee/_merge_request_approvals_settings.html.haml @@ -1,5 +1,6 @@ - return unless project.feature_available?(:merge_request_approvers) - merge_request = local_assigns.fetch(:merge_request, nil) +- suggested_approvers = local_assigns.fetch(:suggested_approvers, []) - model_name = merge_request ? 'merge_request' : 'project' - model = merge_request || project - can_override_approvers = project.can_override_approvers? @@ -44,7 +45,7 @@ %li.approver.settings-flex-row.js-approver{ data: { id: approver.user_id } } = link_to approver.user.name, approver.user .pull-right - %button{ href: project_approver_path(project, approver, merge_request_id: "#{merge_request.iid if merge_request}"), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn btn-remove js-approver-remove", title: 'Remove approver' } + %button{ href: merge_request_approver_path(project, merge_request, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn btn-remove js-approver-remove", title: 'Remove approver' } = icon("trash") - model.approver_groups.each do |approver_group| %li.approver-group.settings-flex-row.js-approver-group{ data: { id: approver_group.group.id } } @@ -55,7 +56,7 @@ %span.badge = approver_group.group.members.count .pull-right - %button{ href: project_approver_group_path(project, approver_group, merge_request_id: "#{merge_request.iid if merge_request}"), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}" }, class: "btn btn-remove js-approver-remove", title: 'Remove group' } + %button{ href: merge_request_approver_path(project, merge_request, approver_group), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}" }, class: "btn btn-remove js-approver-remove", title: 'Remove group' } = icon("trash") - if model.approvers.empty? && model.approver_groups.empty? %li There are no approvers @@ -83,6 +84,6 @@ - if merge_request .help-block.suggested-approvers - - if @suggested_approvers.any? + - if suggested_approvers.any? Suggested approvers: - = raw @suggested_approvers.map { |approver| link_to sanitize(approver.name), "#", id: dom_id(approver) }.join(", ") + = raw suggested_approvers.map { |approver| link_to sanitize(approver.name), "#", id: dom_id(approver) }.join(", ") diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index 6c8e9746f3f656..96fe8d65de8b8e 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -41,7 +41,8 @@ title: 'Moving an issue will copy the discussion to a different project and close it here. All participants will be notified of the new location.' } = icon('question-circle') -= render 'projects/ee/merge_request_approvals_settings', project: project, merge_request: issuable, form: form if issuable.is_a?(MergeRequest) +- if issuable.is_a?(MergeRequest) + = render 'projects/ee/merge_request_approvals_settings', project: project, merge_request: issuable, form: form, suggested_approvers: @suggested_approvers = render 'shared/issuable/form/branch_chooser', issuable: issuable, form: form -- GitLab From 14e4a608029511163b3a16a4eb722681f2809f1c Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Fri, 14 Jul 2017 12:22:04 +0100 Subject: [PATCH 3/3] Insert approval items without network request for newMR and editMR --- app/assets/javascripts/approvers_select.js | 127 ++++++++++++++++++--- app/assets/javascripts/dispatcher.js | 4 +- 2 files changed, 110 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/approvers_select.js b/app/assets/javascripts/approvers_select.js index ca9e0133ce5ae7..03028d9fc1fe9d 100644 --- a/app/assets/javascripts/approvers_select.js +++ b/app/assets/javascripts/approvers_select.js @@ -1,12 +1,41 @@ +import { template } from 'underscore'; import Api from './api'; +const approverItemTemplate = template(` +
  • +
    + <% if (isGroup) { %> + Group: + <% } %> + <%- name %> + <% if (isGroup) { %> + <%- count %> + <% } %> +
    +
    + +
    +
    • +`); + export default class ApproversSelect { - constructor() { + constructor(page) { this.$approverSelect = $('.js-select-user-and-group'); + this.$approversListContainer = $('.js-current-approvers'); + this.$approversList = $('.approver-list', this.$approversListContainer); + const name = this.$approverSelect.data('name'); this.fieldNames = [`${name}[approver_ids]`, `${name}[approver_group_ids]`]; + + this.approversField = $(`input[name="${this.fieldNames[0]}"]`); + this.approverGroupsField = $(`input[name="${this.fieldNames[1]}"]`); this.$loadWrapper = $('.load-wrapper'); + this.isMR = name === 'merge_request'; + this.isNewMR = this.isMR && page === 'projects:merge_requests:new'; + this.bindEvents(); this.addEvents(); this.initSelect2(); @@ -20,7 +49,7 @@ export default class ApproversSelect { addEvents() { $(document).on('click', '.js-add-approvers', () => this.addApprover()); - $(document).on('click', '.js-approver-remove', e => ApproversSelect.removeApprover(e)); + $(document).on('click', '.js-approver-remove', e => this.removeApprover(e)); } static getApprovers(fieldName, approverList) { @@ -45,14 +74,16 @@ export default class ApproversSelect { const options = { skip_users: ApproversSelect.getApprovers(this.fieldNames[0], '.js-approver'), project_id: $('#project_id').val(), + merge_request_id: $('#merge_request_id').val(), + approvers: true, }; return Api.approverUsers(term, options); } handleSelectChange(e) { const { added, removed } = e; - const userInput = $(`[name="${this.fieldNames[0]}"]`); - const groupInput = $(`[name="${this.fieldNames[1]}"]`); + const userInput = this.approversField; + const groupInput = this.approverGroupsField; if (added) { if (added.full_name) { @@ -88,6 +119,7 @@ export default class ApproversSelect { }, formatResult: ApproversSelect.formatResult, formatSelection: ApproversSelect.formatSelection, + containerCssClass: 'js-approvers-input-container', dropdownCss() { const $input = $('.js-select-user-and-group .select2-input'); const offset = $input.offset(); @@ -106,10 +138,25 @@ export default class ApproversSelect { }, }) .on('change', this.handleSelectChange); + + this.$approversInputContainer = $('.js-approvers-input-container'); } - static formatSelection(group) { - return group.full_name || group.name; + static formatSelection(approver) { + const type = Object.hasOwnProperty.call(approver, 'username') ? 'user' : 'group'; + + return ` +
    + ${approver.full_name || approver.name} +
    + `; } static formatResult({ @@ -143,31 +190,30 @@ export default class ApproversSelect { } addApprover() { - this.fieldNames.forEach(ApproversSelect.saveApprovers); + this.fieldNames.forEach(this.saveApprovers.bind(this)); } - static saveApprovers(fieldName) { + saveApprovers(fieldName) { const $input = window.$(`[name="${fieldName}"]`); const newValue = $input.val(); const $loadWrapper = $('.load-wrapper'); const $approverSelect = $('.js-select-user-and-group'); - debugger + if (!newValue) return undefined; - if (!newValue) { - return; - } + if (this.isNewMR) return this.addMergeApprover($input); const $form = $('.js-add-approvers').closest('form'); $loadWrapper.removeClass('hidden'); - window.$.ajax({ + + return window.$.ajax({ url: $form.attr('action'), type: 'POST', data: { _method: 'PATCH', [fieldName]: newValue, }, - success: ApproversSelect.updateApproverList, + success: this.isMR ? this.addMergeApprover.bind(this, $input) : this.updateProjectList, complete() { $input.val(''); $approverSelect.select2('val', ''); @@ -179,18 +225,23 @@ export default class ApproversSelect { }); } - static removeApprover(e) { + removeApprover(e) { e.preventDefault(); + const target = e.currentTarget; + + if (this.isNewMR) return this.removeMergeApprover(target); + const $loadWrapper = $('.load-wrapper'); $loadWrapper.removeClass('hidden'); - $.ajax({ + + return $.ajax({ url: target.getAttribute('href'), type: 'POST', data: { _method: 'DELETE', }, - success: ApproversSelect.updateApproverList, + success: this.isMR ? this.removeMergeApprover.bind(this, target) : this.updateProjectList, complete: () => $loadWrapper.addClass('hidden'), error() { window.Flash('Failed to remove Approver', 'alert'); @@ -198,7 +249,45 @@ export default class ApproversSelect { }); } - static updateApproverList(html) { - $('.js-current-approvers').html($(html).find('.js-current-approvers').html()); + updateProjectList(html) { + const newHtml = $('.js-current-approvers', html).html(); + + this.$approversListContainer.html(newHtml); + } + + addMergeApprover($input) { + const ids = $input.val().split(','); + const isGroup = $input.attr('name').contains('approver_group'); + const selector = isGroup ? '.approver-group' : '.approver-user'; + + $(selector, this.$approversInputContainer).forEach((approver) => { + const approverItem = approverItemTemplate({ + id: approver.data('id'), + link: approver.data('link'), + name: approver.data('name'), + count: approver.data('count'), + removeLink: approver.data('remove-link'), + type: isGroup ? 'group' : 'user', + itemClass: isGroup ? 'approver-group' : 'approver', + isGroup, + }); + + this.$approversList.append(approverItem); + }); + + this.$approverSelect.select2('val', ''); + } + + removeMergeApprover(target) { + const approver = $(target).closest('.approver-list li'); + const approverID = approver.data('id'); + const field = approver.hasClass('approver') ? this.approversField : this.approverGroupsField; + + const ids = field.val().split(','); + const approverIndex = ids.indexOf(approverID); + + if (approverIndex !== -1) ids.splice(approverIndex, 1); + field.val(ids.join(',')); + approver.remove(); } } diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 3fe652d830bcb5..813e8ac072c887 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -234,7 +234,7 @@ import AuditLogs from './audit_logs'; new MilestoneSelect(); new gl.IssuableTemplateSelectors(); new AutoWidthDropdownSelect($('.js-target-branch-select')).init(); - new ApproversSelect(); + new ApproversSelect(page); break; case 'projects:tags:new': new ZenMode(); @@ -500,7 +500,7 @@ import AuditLogs from './audit_logs'; case 'edit': shortcut_handler = new ShortcutsNavigation(); new ProjectNew(); - new ApproversSelect(); + new ApproversSelect(page); break; case 'new': new ProjectNew(); -- GitLab