From 064c8949bd2cbbd304909b84f4ddac3c80827b94 Mon Sep 17 00:00:00 2001 From: Jarka Kadlecova Date: Wed, 1 Nov 2017 18:35:14 +0100 Subject: CE port of code changed for epics --- app/controllers/concerns/issuable_actions.rb | 72 +++++++++++++++++ app/controllers/projects/issues_controller.rb | 69 ++-------------- .../projects/merge_requests_controller.rb | 10 +-- app/helpers/gitlab_routing_helper.rb | 6 +- app/helpers/issuables_helper.rb | 27 ++++--- app/models/concerns/cache_markdown_field.rb | 3 +- app/models/concerns/issuable.rb | 3 - app/models/epic.rb | 9 +++ app/models/group.rb | 6 ++ app/models/issue.rb | 3 + app/models/merge_request.rb | 3 + app/models/note.rb | 8 +- app/serializers/issuable_entity.rb | 8 +- app/serializers/issue_entity.rb | 4 +- app/serializers/merge_request_entity.rb | 4 +- app/serializers/time_trackable_entity.rb | 11 +++ .../issuable/common_system_notes_service.rb | 81 +++++++++++++++++++ app/services/issuable_base_service.rb | 93 +++------------------- app/services/issues/update_service.rb | 4 - app/services/merge_requests/update_service.rb | 10 ++- .../projects/merge_requests/_mr_title.html.haml | 2 +- lib/banzai/filter/abstract_reference_filter.rb | 6 +- lib/banzai/filter/reference_filter.rb | 4 + lib/banzai/filter/user_reference_filter.rb | 42 +++++++--- .../controllers/projects/issues_controller_spec.rb | 39 +++++++++ .../projects/merge_requests_controller_spec.rb | 56 ++++++------- spec/helpers/gitlab_routing_helper_spec.rb | 26 ++++++ spec/helpers/issuables_helper_spec.rb | 32 ++++++++ .../banzai/filter/issue_reference_filter_spec.rb | 62 +++++++++++++++ .../banzai/filter/label_reference_filter_spec.rb | 12 +++ .../filter/merge_request_reference_filter_spec.rb | 10 +++ .../filter/milestone_reference_filter_spec.rb | 11 +++ .../banzai/filter/snippet_reference_filter_spec.rb | 10 +++ .../banzai/filter/user_reference_filter_spec.rb | 33 ++++++++ spec/serializers/issue_entity_spec.rb | 20 +++++ spec/serializers/merge_request_entity_spec.rb | 11 ++- .../issuable/common_system_notes_service_spec.rb | 49 ++++++++++++ 37 files changed, 631 insertions(+), 228 deletions(-) create mode 100644 app/models/epic.rb create mode 100644 app/serializers/time_trackable_entity.rb create mode 100644 app/services/issuable/common_system_notes_service.rb create mode 100644 spec/serializers/issue_entity_spec.rb create mode 100644 spec/services/issuable/common_system_notes_service_spec.rb diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 4079072a930..b1ed973d178 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -7,6 +7,54 @@ module IssuableActions before_action :authorize_admin_issuable!, only: :bulk_update end + def show + respond_to do |format| + format.html do + render show_view + end + format.json do + render json: serializer.represent(issuable, serializer: params[:serializer]) + end + end + end + + def update + @issuable = update_service.execute(issuable) + + respond_to do |format| + format.html do + recaptcha_check_with_fallback { render :edit } + end + + format.json do + render_entity_json + end + end + + rescue ActiveRecord::StaleObjectError + render_conflict_response + end + + def realtime_changes + Gitlab::PollingInterval.set_header(response, interval: 3_000) + + response = { + title: view_context.markdown_field(issuable, :title), + title_text: issuable.title, + description: view_context.markdown_field(issuable, :description), + description_text: issuable.description, + task_status: issuable.task_status + } + + if issuable.edited? + response[:updated_at] = issuable.updated_at + response[:updated_by_name] = issuable.last_edited_by.name + response[:updated_by_path] = user_path(issuable.last_edited_by) + end + + render json: response + end + def destroy issuable.destroy destroy_method = "destroy_#{issuable.class.name.underscore}".to_sym @@ -68,6 +116,10 @@ module IssuableActions end end + def authorize_update_issuable! + render_404 unless can?(current_user, :"update_#{resource_name}", issuable) + end + def bulk_update_params permitted_keys = [ :issuable_ids, @@ -92,4 +144,24 @@ module IssuableActions def resource_name @resource_name ||= controller_name.singularize end + + def render_entity_json + if @issuable.valid? + render json: serializer.represent(@issuable) + else + render json: { errors: @issuable.errors.full_messages }, status: :unprocessable_entity + end + end + + def show_view + 'show' + end + + def serializer + raise NotImplementedError + end + + def update_service + raise NotImplementedError + end end diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 6a5e4538717..d4e763aa5b8 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -16,7 +16,7 @@ class Projects::IssuesController < Projects::ApplicationController before_action :authorize_create_issue!, only: [:new, :create] # Allow modify issue - before_action :authorize_update_issue!, only: [:edit, :update, :move] + before_action :authorize_update_issuable!, only: [:edit, :update, :move] # Allow create a new branch and empty WIP merge request from current issue before_action :authorize_create_merge_request!, only: [:create_merge_request] @@ -67,18 +67,6 @@ class Projects::IssuesController < Projects::ApplicationController respond_with(@issue) end - def show - @noteable = @issue - @note = @project.notes.new(noteable: @issue) - - respond_to do |format| - format.html - format.json do - render json: serializer.represent(@issue, serializer: params[:serializer]) - end - end - end - def discussions notes = @issue.notes .inc_relations_for_view @@ -120,25 +108,6 @@ class Projects::IssuesController < Projects::ApplicationController end end - def update - update_params = issue_params.merge(spammable_params) - - @issue = Issues::UpdateService.new(project, current_user, update_params).execute(issue) - - respond_to do |format| - format.html do - recaptcha_check_with_fallback { render :edit } - end - - format.json do - render_issue_json - end - end - - rescue ActiveRecord::StaleObjectError - render_conflict_response - end - def move params.require(:move_to_project_id) @@ -196,26 +165,6 @@ class Projects::IssuesController < Projects::ApplicationController end end - def realtime_changes - Gitlab::PollingInterval.set_header(response, interval: 3_000) - - response = { - title: view_context.markdown_field(@issue, :title), - title_text: @issue.title, - description: view_context.markdown_field(@issue, :description), - description_text: @issue.description, - task_status: @issue.task_status - } - - if @issue.edited? - response[:updated_at] = @issue.updated_at - response[:updated_by_name] = @issue.last_edited_by.name - response[:updated_by_path] = user_path(@issue.last_edited_by) - end - - render json: response - end - def create_merge_request result = ::MergeRequests::CreateFromIssueService.new(project, current_user, issue_iid: issue.iid).execute @@ -231,7 +180,8 @@ class Projects::IssuesController < Projects::ApplicationController def issue return @issue if defined?(@issue) # The Sortable default scope causes performance issues when used with find_by - @noteable = @issue ||= @project.issues.where(iid: params[:id]).reorder(nil).take! + @issuable = @noteable = @issue ||= @project.issues.where(iid: params[:id]).reorder(nil).take! + @note = @project.notes.new(noteable: @issuable) return render_404 unless can?(current_user, :read_issue, @issue) @@ -246,14 +196,6 @@ class Projects::IssuesController < Projects::ApplicationController project_issue_path(@project, @issue) end - def authorize_update_issue! - render_404 unless can?(current_user, :update_issue, @issue) - end - - def authorize_admin_issues! - render_404 unless can?(current_user, :admin_issue, @project) - end - def authorize_create_merge_request! render_404 unless can?(current_user, :push_code, @project) && @issue.can_be_worked_on?(current_user) end @@ -305,4 +247,9 @@ class Projects::IssuesController < Projects::ApplicationController def serializer IssueSerializer.new(current_user: current_user, project: issue.project) end + + def update_service + update_params = issue_params.merge(spammable_params) + Issues::UpdateService.new(project, current_user, update_params) + end end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 2b0294c8387..17cac69e588 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -9,7 +9,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo skip_before_action :merge_request, only: [:index, :bulk_update] skip_before_action :ensure_ref_fetched, only: [:index, :bulk_update] - before_action :authorize_update_merge_request!, only: [:close, :edit, :update, :remove_wip, :sort] + before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort] before_action :authenticate_user!, only: [:assign_related_issues] @@ -256,14 +256,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo alias_method :issuable, :merge_request alias_method :awardable, :merge_request - def authorize_update_merge_request! - return render_404 unless can?(current_user, :update_merge_request, @merge_request) - end - - def authorize_admin_merge_request! - return render_404 unless can?(current_user, :admin_merge_request, @merge_request) - end - def validates_merge_request # Show git not found page # if there is no saved commits between source & target branch diff --git a/app/helpers/gitlab_routing_helper.rb b/app/helpers/gitlab_routing_helper.rb index d4a91e533c1..a77aa0ad2cc 100644 --- a/app/helpers/gitlab_routing_helper.rb +++ b/app/helpers/gitlab_routing_helper.rb @@ -71,11 +71,13 @@ module GitlabRoutingHelper project_commit_url(entity.project, entity.sha, *args) end - def preview_markdown_path(project, *args) + def preview_markdown_path(parent, *args) + return group_preview_markdown_path(parent) if parent.is_a?(Group) + if @snippet.is_a?(PersonalSnippet) preview_markdown_snippets_path else - preview_markdown_project_path(project, *args) + preview_markdown_project_path(parent, *args) end end diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index d0069cd48cf..85407e38532 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -211,15 +211,13 @@ module IssuablesHelper def issuable_initial_data(issuable) data = { - endpoint: project_issue_path(@project, issuable), - canUpdate: can?(current_user, :update_issue, issuable), - canDestroy: can?(current_user, :destroy_issue, issuable), + endpoint: issuable_path(issuable), + canUpdate: can?(current_user, :"update_#{issuable.to_ability_name}", issuable), + canDestroy: can?(current_user, :"destroy_#{issuable.to_ability_name}", issuable), issuableRef: issuable.to_reference, - markdownPreviewPath: preview_markdown_path(@project), + markdownPreviewPath: preview_markdown_path(parent), markdownDocsPath: help_page_path('user/markdown'), issuableTemplates: issuable_templates(issuable), - projectPath: ref_project.path, - projectNamespace: ref_project.namespace.full_path, initialTitleHtml: markdown_field(issuable, :title), initialTitleText: issuable.title, initialDescriptionHtml: markdown_field(issuable, :description), @@ -227,6 +225,12 @@ module IssuablesHelper initialTaskStatus: issuable.task_status } + if parent.is_a?(Group) + data[:groupPath] = parent.path + else + data.merge!(projectPath: ref_project.path, projectNamespace: ref_project.namespace.full_path) + end + data.merge!(updated_at_by(issuable)) data.to_json @@ -263,12 +267,7 @@ module IssuablesHelper end def issuable_path(issuable, *options) - case issuable - when Issue - issue_path(issuable, *options) - when MergeRequest - merge_request_path(issuable, *options) - end + polymorphic_path(issuable, *options) end def issuable_url(issuable, *options) @@ -369,4 +368,8 @@ module IssuablesHelper fullPath: @project.full_path } end + + def parent + @project || @group + end end diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 9417033d1f6..98776eab424 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -49,7 +49,8 @@ module CacheMarkdownField # Always include a project key, or Banzai complains project = self.project if self.respond_to?(:project) - context = cached_markdown_fields[field].merge(project: project) + group = self.group if self.respond_to?(:group) + context = cached_markdown_fields[field].merge(project: project, group: group) # Banzai is less strict about authors, so don't always have an author key context[:author] = self.author if self.respond_to?(:author) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 27f4dedffd3..a928b9d6367 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -14,7 +14,6 @@ module Issuable include StripAttribute include Awardable include Taskable - include TimeTrackable include Importable include Editable include AfterCommitQueue @@ -95,8 +94,6 @@ module Issuable strip_attributes :title - acts_as_paranoid - after_save :record_metrics, unless: :imported? # We want to use optimistic lock for cases when only title or description are involved diff --git a/app/models/epic.rb b/app/models/epic.rb new file mode 100644 index 00000000000..0d5f21fb617 --- /dev/null +++ b/app/models/epic.rb @@ -0,0 +1,9 @@ +# Placeholder class for model that is implemented in EE +# It will reserve (ee#3853) '&' as a reference prefix, but the table does not exists in CE +class Epic < ActiveRecord::Base + prepend EE::Epic + + # TODO: this will be implemented as part of #3853 + def to_reference + end +end diff --git a/app/models/group.rb b/app/models/group.rb index 07fb62bb249..4e8023cdb7f 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -180,6 +180,12 @@ class Group < Namespace add_user(user, :owner, current_user: current_user) end + def member?(user, min_access_level = Gitlab::Access::GUEST) + return false unless user + + max_member_access_for_user(user) >= min_access_level + end + def has_owner?(user) return false unless user diff --git a/app/models/issue.rb b/app/models/issue.rb index 36e4108b9d6..fc590f9257e 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -10,6 +10,7 @@ class Issue < ActiveRecord::Base include FasterCacheKeys include RelativePositioning include CreatedAtFilterable + include TimeTrackable DueDateStruct = Struct.new(:title, :name).freeze NoDueDate = DueDateStruct.new('No Due Date', '0').freeze @@ -74,6 +75,8 @@ class Issue < ActiveRecord::Base end end + acts_as_paranoid + def self.reference_prefix '#' end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index d45b9c805a4..3133dc9e7eb 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -6,6 +6,7 @@ class MergeRequest < ActiveRecord::Base include Sortable include IgnorableColumn include CreatedAtFilterable + include TimeTrackable ignore_column :locked_at @@ -119,6 +120,8 @@ class MergeRequest < ActiveRecord::Base after_save :keep_around_commit + acts_as_paranoid + def self.reference_prefix '!' end diff --git a/app/models/note.rb b/app/models/note.rb index 8939e590ef1..f9676361072 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -69,7 +69,7 @@ class Note < ActiveRecord::Base delegate :title, to: :noteable, allow_nil: true validates :note, presence: true - validates :project, presence: true, unless: :for_personal_snippet? + validates :project, presence: true, if: :for_project_noteable? # Attachments are deprecated and are handled by Markdown uploader validates :attachment, file_size: { maximum: :max_attachment_size } @@ -114,7 +114,7 @@ class Note < ActiveRecord::Base after_initialize :ensure_discussion_id before_validation :nullify_blank_type, :nullify_blank_line_code before_validation :set_discussion_id, on: :create - after_save :keep_around_commit, unless: :for_personal_snippet? + after_save :keep_around_commit, if: :for_project_noteable? after_save :expire_etag_cache after_destroy :expire_etag_cache @@ -208,6 +208,10 @@ class Note < ActiveRecord::Base noteable.is_a?(PersonalSnippet) end + def for_project_noteable? + !for_personal_snippet? + end + def skip_project_check? for_personal_snippet? end diff --git a/app/serializers/issuable_entity.rb b/app/serializers/issuable_entity.rb index 61c7a428745..3b5a4fd4f79 100644 --- a/app/serializers/issuable_entity.rb +++ b/app/serializers/issuable_entity.rb @@ -1,20 +1,16 @@ class IssuableEntity < Grape::Entity + include RequestAwareEntity + expose :id expose :iid expose :author_id expose :description expose :lock_version expose :milestone_id - expose :state expose :title expose :updated_by_id expose :created_at expose :updated_at - expose :deleted_at - expose :time_estimate - expose :total_time_spent - expose :human_time_estimate - expose :human_total_time_spent expose :milestone, using: API::Entities::Milestone expose :labels, using: LabelEntity end diff --git a/app/serializers/issue_entity.rb b/app/serializers/issue_entity.rb index 10d3ad0214b..5f47592e4ad 100644 --- a/app/serializers/issue_entity.rb +++ b/app/serializers/issue_entity.rb @@ -1,6 +1,8 @@ class IssueEntity < IssuableEntity - include RequestAwareEntity + include TimeTrackableEntity + expose :state + expose :deleted_at expose :branch_name expose :confidential expose :discussion_locked diff --git a/app/serializers/merge_request_entity.rb b/app/serializers/merge_request_entity.rb index 297a459e394..b53a49fe59e 100644 --- a/app/serializers/merge_request_entity.rb +++ b/app/serializers/merge_request_entity.rb @@ -1,6 +1,8 @@ class MergeRequestEntity < IssuableEntity - include RequestAwareEntity + include TimeTrackableEntity + expose :state + expose :deleted_at expose :in_progress_merge_commit_sha expose :merge_commit_sha expose :merge_error diff --git a/app/serializers/time_trackable_entity.rb b/app/serializers/time_trackable_entity.rb new file mode 100644 index 00000000000..e81cd7bec72 --- /dev/null +++ b/app/serializers/time_trackable_entity.rb @@ -0,0 +1,11 @@ +module TimeTrackableEntity + extend ActiveSupport::Concern + extend Grape + + included do + expose :time_estimate + expose :total_time_spent + expose :human_time_estimate + expose :human_total_time_spent + end +end diff --git a/app/services/issuable/common_system_notes_service.rb b/app/services/issuable/common_system_notes_service.rb new file mode 100644 index 00000000000..92eaa5d5115 --- /dev/null +++ b/app/services/issuable/common_system_notes_service.rb @@ -0,0 +1,81 @@ +module Issuable + class CommonSystemNotesService < ::BaseService + attr_reader :issuable + + def execute(issuable, old_labels) + @issuable = issuable + + if issuable.previous_changes.include?('title') + create_title_change_note(issuable.previous_changes['title'].first) + end + + handle_description_change_note + + handle_time_tracking_note if issuable.is_a?(TimeTrackable) + create_labels_note(old_labels) if issuable.labels != old_labels + create_discussion_lock_note if issuable.previous_changes.include?('discussion_locked') + create_milestone_note if issuable.previous_changes.include?('milestone_id') + end + + private + + def handle_time_tracking_note + if issuable.previous_changes.include?('time_estimate') + create_time_estimate_note + end + + if issuable.time_spent? + create_time_spent_note + end + end + + def handle_description_change_note + if issuable.previous_changes.include?('description') + if issuable.tasks? && issuable.updated_tasks.any? + create_task_status_note + else + # TODO: Show this note if non-task content was modified. + # https://gitlab.com/gitlab-org/gitlab-ce/issues/33577 + create_description_change_note + end + end + end + + def create_labels_note(old_labels) + added_labels = issuable.labels - old_labels + removed_labels = old_labels - issuable.labels + + SystemNoteService.change_label(issuable, issuable.project, current_user, added_labels, removed_labels) + end + + def create_title_change_note(old_title) + SystemNoteService.change_title(issuable, issuable.project, current_user, old_title) + end + + def create_description_change_note + SystemNoteService.change_description(issuable, issuable.project, current_user) + end + + def create_task_status_note + issuable.updated_tasks.each do |task| + SystemNoteService.change_task_status(issuable, issuable.project, current_user, task) + end + end + + def create_time_estimate_note + SystemNoteService.change_time_estimate(issuable, issuable.project, current_user) + end + + def create_time_spent_note + SystemNoteService.change_time_spent(issuable, issuable.project, issuable.time_spent_user) + end + + def create_milestone_note + SystemNoteService.change_milestone(issuable, issuable.project, current_user, issuable.milestone) + end + + def create_discussion_lock_note + SystemNoteService.discussion_lock(issuable, current_user) + end + end +end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index d61a342ebad..68b49d880f7 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -1,56 +1,10 @@ class IssuableBaseService < BaseService private - def create_milestone_note(issuable) - SystemNoteService.change_milestone( - issuable, issuable.project, current_user, issuable.milestone) - end - - def create_labels_note(issuable, old_labels) - added_labels = issuable.labels - old_labels - removed_labels = old_labels - issuable.labels - - SystemNoteService.change_label( - issuable, issuable.project, current_user, added_labels, removed_labels) - end - - def create_title_change_note(issuable, old_title) - SystemNoteService.change_title( - issuable, issuable.project, current_user, old_title) - end - - def create_description_change_note(issuable) - SystemNoteService.change_description(issuable, issuable.project, current_user) - end - - def create_branch_change_note(issuable, branch_type, old_branch, new_branch) - SystemNoteService.change_branch( - issuable, issuable.project, current_user, branch_type, - old_branch, new_branch) - end - - def create_task_status_note(issuable) - issuable.updated_tasks.each do |task| - SystemNoteService.change_task_status(issuable, issuable.project, current_user, task) - end - end - - def create_time_estimate_note(issuable) - SystemNoteService.change_time_estimate(issuable, issuable.project, current_user) - end - - def create_time_spent_note(issuable) - 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}" - unless can?(current_user, ability_name, project) + unless can?(current_user, ability_name, issuable) params.delete(:milestone_id) params.delete(:labels) params.delete(:add_label_ids) @@ -233,15 +187,14 @@ class IssuableBaseService < BaseService # We have to perform this check before saving the issuable as Rails resets # the changed fields upon calling #save. - update_project_counters = issuable.update_project_counter_caches? + update_project_counters = issuable.project && issuable.update_project_counter_caches? if issuable.with_transaction_returning_status { issuable.save } # We do not touch as it will affect a update on updated_at field ActiveRecord::Base.no_touching do - handle_common_system_notes(issuable, old_labels: old_labels) + Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, old_labels) end - change_discussion_lock(issuable) handle_changes( issuable, old_labels: old_labels, @@ -300,12 +253,6 @@ 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 @@ -328,35 +275,17 @@ class IssuableBaseService < BaseService attrs_changed || labels_changed || assignees_changed end - def handle_common_system_notes(issuable, old_labels: []) - if issuable.previous_changes.include?('title') - create_title_change_note(issuable, issuable.previous_changes['title'].first) - end - - if issuable.previous_changes.include?('description') - if issuable.tasks? && issuable.updated_tasks.any? - create_task_status_note(issuable) - else - # TODO: Show this note if non-task content was modified. - # https://gitlab.com/gitlab-org/gitlab-ce/issues/33577 - create_description_change_note(issuable) - end - end - - if issuable.previous_changes.include?('time_estimate') - create_time_estimate_note(issuable) - end - - if issuable.time_spent? - create_time_spent_note(issuable) - end - - create_labels_note(issuable, old_labels) if issuable.labels != old_labels - end - def invalidate_cache_counts(issuable, users: []) users.each do |user| user.public_send("invalidate_#{issuable.model_name.singular}_cache_counts") # rubocop:disable GitlabSecurity/PublicSend end end + + # override if needed + def handle_changes(issuable, options) + end + + # override if needed + def execute_hooks(issuable, action = 'open', params = {}) + end end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index e0339ddf9bb..1b7b5927c5a 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -27,10 +27,6 @@ module Issues todo_service.update_issue(issue, current_user, old_mentioned_users) end - if issue.previous_changes.include?('milestone_id') - create_milestone_note(issue) - end - if issue.assignees != old_assignees create_assignee_note(issue, old_assignees) notification_service.reassigned_issue(issue, current_user, old_assignees) diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 2832d893e95..1f394cacc64 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -40,10 +40,6 @@ module MergeRequests merge_request.target_branch) end - if merge_request.previous_changes.include?('milestone_id') - create_milestone_note(merge_request) - end - if merge_request.previous_changes.include?('assignee_id') create_assignee_note(merge_request) notification_service.reassigned_merge_request(merge_request, current_user) @@ -111,5 +107,11 @@ module MergeRequests end end end + + def create_branch_change_note(issuable, branch_type, old_branch, new_branch) + SystemNoteService.change_branch( + issuable, issuable.project, current_user, branch_type, + old_branch, new_branch) + end end end diff --git a/app/views/projects/merge_requests/_mr_title.html.haml b/app/views/projects/merge_requests/_mr_title.html.haml index cb723fe6a18..72d5c4961ec 100644 --- a/app/views/projects/merge_requests/_mr_title.html.haml +++ b/app/views/projects/merge_requests/_mr_title.html.haml @@ -34,7 +34,7 @@ %li{ class: [merge_request_button_visibility(@merge_request, true), 'js-close-item'] } = link_to 'Close', merge_request_path(@merge_request, merge_request: { state_event: :close }), method: :put, title: 'Close merge request' %li{ class: merge_request_button_visibility(@merge_request, false) } - = link_to 'Reopen', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: 'reopen-mr-link', title: 'Reopen merge request' + = link_to 'Reopen', merge_request_path(@merge_request, merge_request: { state_event: :reopen }), method: :put, class: 'reopen-mr-link', title: 'Reopen merge request' - if can_update_merge_request = link_to 'Edit', edit_project_merge_request_path(@project, @merge_request), class: "hidden-xs hidden-sm btn btn-grouped issuable-edit" diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb index ef4578aabd6..a0f7e4e5ad5 100644 --- a/lib/banzai/filter/abstract_reference_filter.rb +++ b/lib/banzai/filter/abstract_reference_filter.rb @@ -95,7 +95,7 @@ module Banzai end def call - return doc if project.nil? + return doc unless project || group ref_pattern = object_class.reference_pattern link_pattern = object_class.link_reference_pattern @@ -288,10 +288,14 @@ module Banzai end def current_project_path + return unless project + @current_project_path ||= project.full_path end def current_project_namespace_path + return unless project + @current_project_namespace_path ||= project.namespace.full_path end diff --git a/lib/banzai/filter/reference_filter.rb b/lib/banzai/filter/reference_filter.rb index a6f8650ed3d..c6ae28adf87 100644 --- a/lib/banzai/filter/reference_filter.rb +++ b/lib/banzai/filter/reference_filter.rb @@ -55,6 +55,10 @@ module Banzai context[:project] end + def group + context[:group] + end + def skip_project_check? context[:skip_project_check] end diff --git a/lib/banzai/filter/user_reference_filter.rb b/lib/banzai/filter/user_reference_filter.rb index f3356d6c51e..afb6e25963c 100644 --- a/lib/banzai/filter/user_reference_filter.rb +++ b/lib/banzai/filter/user_reference_filter.rb @@ -24,7 +24,7 @@ module Banzai end def call - return doc if project.nil? && !skip_project_check? + return doc if project.nil? && group.nil? && !skip_project_check? ref_pattern = User.reference_pattern ref_pattern_start = /\A#{ref_pattern}\z/ @@ -101,19 +101,12 @@ module Banzai end def link_to_all(link_content: nil) - project = context[:project] author = context[:author] - if author && !project.team.member?(author) + if author && !team_member?(author) link_content else - url = urls.project_url(project, - only_path: context[:only_path]) - - data = data_attribute(project: project.id, author: author.try(:id)) - content = link_content || User.reference_prefix + 'all' - - link_tag(url, data, content, 'All Project and Group Members') + parent_url(link_content, author) end end @@ -144,6 +137,35 @@ module Banzai def link_tag(url, data, link_content, title) %(#{link_content}) end + + def parent + context[:project] || context[:group] + end + + def parent_group? + parent.is_a?(Group) + end + + def team_member?(user) + if parent_group? + parent.member?(user) + else + parent.team.member?(user) + end + end + + def parent_url(link_content, author) + if parent_group? + url = urls.group_url(parent, only_path: context[:only_path]) + data = data_attribute(group: group.id, author: author.try(:id)) + else + url = urls.project_url(parent, only_path: context[:only_path]) + data = data_attribute(project: project.id, author: author.try(:id)) + end + + content = link_content || User.reference_prefix + 'all' + link_tag(url, data, content, 'All Project and Group Members') + end end end end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index aecdfb50759..8016176110e 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -248,6 +248,45 @@ describe Projects::IssuesController do end end + describe 'PUT #update' do + subject do + put :update, + namespace_id: project.namespace, + project_id: project, + id: issue.to_param, + issue: { title: 'New title' }, format: :json + end + + before do + sign_in(user) + end + + context 'when user has access to update issue' do + before do + project.add_developer(user) + end + + it 'updates the issue' do + subject + + expect(response).to have_http_status(:ok) + expect(issue.reload.title).to eq('New title') + end + end + + context 'when user does not have access to update issue' do + before do + project.add_guest(user) + end + + it 'responds with 404' do + subject + + expect(response).to have_http_status(:not_found) + end + end + end + describe 'Confidential Issues' do let(:project) { create(:project_empty_repo, :public) } let(:assignee) { create(:assignee) } diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 52ef8c6a589..14021b8ca50 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -186,17 +186,23 @@ describe Projects::MergeRequestsController do end describe 'PUT update' do + def update_merge_request(mr_params, additional_params = {}) + params = { + namespace_id: project.namespace, + project_id: project, + id: merge_request.iid, + merge_request: mr_params + }.merge(additional_params) + + put :update, params + end + context 'changing the assignee' do it 'limits the attributes exposed on the assignee' do assignee = create(:user) project.add_developer(assignee) - put :update, - namespace_id: project.namespace.to_param, - project_id: project, - id: merge_request.iid, - merge_request: { assignee_id: assignee.id }, - format: :json + update_merge_request({ assignee_id: assignee.id }, format: :json) body = JSON.parse(response.body) expect(body['assignee'].keys) @@ -204,6 +210,20 @@ describe Projects::MergeRequestsController do end end + context 'when user does not have access to update issue' do + before do + reporter = create(:user) + project.add_reporter(reporter) + sign_in(reporter) + end + + it 'responds with 404' do + update_merge_request(title: 'New title') + + expect(response).to have_http_status(:not_found) + end + end + context 'there is no source project' do let(:project) { create(:project, :repository) } let(:forked_project) { fork_project_with_submodules(project) } @@ -214,13 +234,7 @@ describe Projects::MergeRequestsController do end it 'closes MR without errors' do - post :update, - namespace_id: project.namespace, - project_id: project, - id: merge_request.iid, - merge_request: { - state_event: 'close' - } + update_merge_request(state_event: 'close') expect(response).to redirect_to([merge_request.target_project.namespace.becomes(Namespace), merge_request.target_project, merge_request]) expect(merge_request.reload.closed?).to be_truthy @@ -229,13 +243,7 @@ describe Projects::MergeRequestsController do it 'allows editing of a closed merge request' do merge_request.close! - put :update, - namespace_id: project.namespace, - project_id: project, - id: merge_request.iid, - merge_request: { - title: 'New title' - } + update_merge_request(title: 'New title') expect(response).to redirect_to([merge_request.target_project.namespace.becomes(Namespace), merge_request.target_project, merge_request]) expect(merge_request.reload.title).to eq 'New title' @@ -244,13 +252,7 @@ describe Projects::MergeRequestsController do it 'does not allow to update target branch closed merge request' do merge_request.close! - put :update, - namespace_id: project.namespace, - project_id: project, - id: merge_request.iid, - merge_request: { - target_branch: 'new_branch' - } + update_merge_request(target_branch: 'new_branch') expect { merge_request.reload.target_branch }.not_to change { merge_request.target_branch } end diff --git a/spec/helpers/gitlab_routing_helper_spec.rb b/spec/helpers/gitlab_routing_helper_spec.rb index a44b200c5da..6c4f7050ee0 100644 --- a/spec/helpers/gitlab_routing_helper_spec.rb +++ b/spec/helpers/gitlab_routing_helper_spec.rb @@ -63,4 +63,30 @@ describe GitlabRoutingHelper do it { expect(resend_invite_group_member_path(group_member)).to eq resend_invite_group_group_member_path(group_member.source, group_member) } end end + + describe '#preview_markdown_path' do + let(:project) { create(:project) } + + it 'returns group preview markdown path for a group parent' do + group = create(:group) + + expect(preview_markdown_path(group)).to eq("/groups/#{group.path}/preview_markdown") + end + + it 'returns project preview markdown path for a project parent' do + expect(preview_markdown_path(project)).to eq("/#{project.full_path}/preview_markdown") + end + + it 'returns snippet preview markdown path for a personal snippet' do + @snippet = create(:personal_snippet) + + expect(preview_markdown_path(nil)).to eq("/snippets/preview_markdown") + end + + it 'returns project preview markdown path for a project snippet' do + @snippet = create(:project_snippet, project: project) + + expect(preview_markdown_path(project)).to eq("/#{project.full_path}/preview_markdown") + end + end end diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb index ead3e28438e..cb851d828f2 100644 --- a/spec/helpers/issuables_helper_spec.rb +++ b/spec/helpers/issuables_helper_spec.rb @@ -159,4 +159,36 @@ describe IssuablesHelper do end end end + + describe '#issuable_initial_data' do + let(:user) { create(:user) } + + before do + allow(helper).to receive(:current_user).and_return(user) + allow(helper).to receive(:can?).and_return(true) + end + + it 'returns the correct json for an issue' do + issue = create(:issue, author: user, description: 'issue text') + @project = issue.project + + expected_data = { + 'endpoint' => "/#{@project.full_path}/issues/#{issue.iid}", + 'canUpdate' => true, + 'canDestroy' => true, + 'issuableRef' => "##{issue.iid}", + 'markdownPreviewPath' => "/#{@project.full_path}/preview_markdown", + 'markdownDocsPath' => '/help/user/markdown', + 'issuableTemplates' => [], + 'projectPath' => @project.path, + 'projectNamespace' => @project.namespace.path, + 'initialTitleHtml' => issue.title, + 'initialTitleText' => issue.title, + 'initialDescriptionHtml' => '

issue text

', + 'initialDescriptionText' => 'issue text', + 'initialTaskStatus' => '0 of 0 tasks completed' + } + expect(JSON.parse(helper.issuable_initial_data(issue))).to eq(expected_data) + end + end end diff --git a/spec/lib/banzai/filter/issue_reference_filter_spec.rb b/spec/lib/banzai/filter/issue_reference_filter_spec.rb index 9c74c9b8c99..3c98b18f99b 100644 --- a/spec/lib/banzai/filter/issue_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/issue_reference_filter_spec.rb @@ -317,6 +317,68 @@ describe Banzai::Filter::IssueReferenceFilter do end end + context 'group context' do + let(:group) { create(:group) } + let(:context) { { project: nil, group: group } } + + it 'ignores shorthanded issue reference' do + reference = "##{issue.iid}" + text = "Fixed #{reference}" + + expect(reference_filter(text, context).to_html).to eq(text) + end + + it 'ignores valid references when cross-reference project uses external tracker' do + expect_any_instance_of(described_class).to receive(:find_object) + .with(project, issue.iid) + .and_return(nil) + + reference = "#{project.full_path}##{issue.iid}" + text = "Issue #{reference}" + + expect(reference_filter(text, context).to_html).to eq(text) + end + + it 'links to a valid reference for complete cross-reference' do + reference = "#{project.full_path}##{issue.iid}" + doc = reference_filter("See #{reference}", context) + + expect(doc.css('a').first.attr('href')).to eq helper.url_for_issue(issue.iid, project) + end + + it 'ignores reference for shorthand cross-reference' do + reference = "#{project.path}##{issue.iid}" + text = "See #{reference}" + + expect(reference_filter(text, context).to_html).to eq(text) + end + + it 'links to a valid reference for url cross-reference' do + reference = helper.url_for_issue(issue.iid, project) + "#note_123" + + doc = reference_filter("See #{reference}", context) + + expect(doc.css('a').first.attr('href')).to eq(helper.url_for_issue(issue.iid, project) + "#note_123") + end + + it 'links to a valid reference for cross-reference in link href' do + reference = "#{helper.url_for_issue(issue.iid, project) + "#note_123"}" + reference_link = %{Reference} + + doc = reference_filter("See #{reference_link}", context) + + expect(doc.css('a').first.attr('href')).to eq helper.url_for_issue(issue.iid, project) + "#note_123" + end + + it 'links to a valid reference for issue reference in the link href' do + reference = issue.to_reference(group) + reference_link = %{Reference} + doc = reference_filter("See #{reference_link}", context) + + expect(doc.css('a').first.attr('href')).to eq helper.url_for_issue(issue.iid, project) + end + end + describe '#issues_per_project' do context 'using an internal issue tracker' do it 'returns a Hash containing the issues per project' do diff --git a/spec/lib/banzai/filter/label_reference_filter_spec.rb b/spec/lib/banzai/filter/label_reference_filter_spec.rb index 2cd30a5e302..862b1fe3fd3 100644 --- a/spec/lib/banzai/filter/label_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/label_reference_filter_spec.rb @@ -594,4 +594,16 @@ describe Banzai::Filter::LabelReferenceFilter do expect(reference_filter(act).to_html).to eq exp end end + + describe 'group context' do + it 'points to referenced project issues page' do + project = create(:project) + label = create(:label, project: project) + reference = "#{project.full_path}~#{label.name}" + + result = reference_filter("See #{reference}", { project: nil, group: create(:group) } ) + + expect(result.css('a').first.attr('href')).to eq(urls.project_issues_url(project, label_name: label.name)) + end + end end diff --git a/spec/lib/banzai/filter/merge_request_reference_filter_spec.rb b/spec/lib/banzai/filter/merge_request_reference_filter_spec.rb index ed2788f8a33..158844e25ae 100644 --- a/spec/lib/banzai/filter/merge_request_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/merge_request_reference_filter_spec.rb @@ -214,4 +214,14 @@ describe Banzai::Filter::MergeRequestReferenceFilter do expect(doc.to_html).to match(/\(#{Regexp.escape(merge.to_reference(project))} \(diffs, comment 123\)<\/a>\.\)/) end end + + context 'group context' do + it 'links to a valid reference' do + reference = "#{project.full_path}!#{merge.iid}" + + result = reference_filter("See #{reference}", { project: nil, group: create(:group) } ) + + expect(result.css('a').first.attr('href')).to eq(urls.project_merge_request_url(project, merge)) + end + end end diff --git a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb index fe7a8c84c9e..84578668133 100644 --- a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb @@ -343,4 +343,15 @@ describe Banzai::Filter::MilestoneReferenceFilter do expect(doc.css('a')).to be_empty end end + + context 'group context' do + it 'links to a valid reference' do + milestone = create(:milestone, project: project) + reference = "#{project.full_path}%#{milestone.iid}" + + result = reference_filter("See #{reference}", { project: nil, group: create(:group) } ) + + expect(result.css('a').first.attr('href')).to eq(urls.milestone_url(milestone)) + end + end end diff --git a/spec/lib/banzai/filter/snippet_reference_filter_spec.rb b/spec/lib/banzai/filter/snippet_reference_filter_spec.rb index 90ac4c7b238..3a07a6dc179 100644 --- a/spec/lib/banzai/filter/snippet_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/snippet_reference_filter_spec.rb @@ -201,4 +201,14 @@ describe Banzai::Filter::SnippetReferenceFilter do expect(reference_filter(act).to_html).to match(/#{Regexp.escape(invalidate_reference(reference))}<\/a>/) end end + + context 'group context' do + it 'links to a valid reference' do + reference = "#{project.full_path}$#{snippet.id}" + + result = reference_filter("See #{reference}", { project: nil, group: create(:group) } ) + + expect(result.css('a').first.attr('href')).to eq(urls.project_snippet_url(project, snippet)) + end + end end diff --git a/spec/lib/banzai/filter/user_reference_filter_spec.rb b/spec/lib/banzai/filter/user_reference_filter_spec.rb index 34dac1db69a..fc03741976e 100644 --- a/spec/lib/banzai/filter/user_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/user_reference_filter_spec.rb @@ -208,6 +208,39 @@ describe Banzai::Filter::UserReferenceFilter do end end + context 'in group context' do + let(:group) { create(:group) } + let(:group_member) { create(:user) } + + before do + group.add_developer(group_member) + end + + let(:context) { { author: group_member, project: nil, group: group } } + + it 'supports a special @all mention' do + reference = User.reference_prefix + 'all' + doc = reference_filter("Hey #{reference}", context) + + expect(doc.css('a').length).to eq(1) + expect(doc.css('a').first.attr('href')).to eq urls.group_url(group) + end + + it 'supports mentioning a single user' do + reference = group_member.to_reference + doc = reference_filter("Hey #{reference}", context) + + expect(doc.css('a').first.attr('href')).to eq urls.user_url(group_member) + end + + it 'supports mentioning a group' do + reference = group.to_reference + doc = reference_filter("Hey #{reference}", context) + + expect(doc.css('a').first.attr('href')).to eq urls.user_url(group) + end + end + describe '#namespaces' do it 'returns a Hash containing all Namespaces' do document = Nokogiri::HTML.fragment("

#{user.to_reference}

") diff --git a/spec/serializers/issue_entity_spec.rb b/spec/serializers/issue_entity_spec.rb new file mode 100644 index 00000000000..caa3e41402b --- /dev/null +++ b/spec/serializers/issue_entity_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' + +describe IssueEntity do + let(:project) { create(:project) } + let(:resource) { create(:issue, project: project) } + let(:user) { create(:user) } + + let(:request) { double('request', current_user: user) } + + subject { described_class.new(resource, request: request).as_json } + + it 'has Issuable attributes' do + expect(subject).to include(:id, :iid, :author_id, :description, :lock_version, :milestone_id, + :title, :updated_by_id, :created_at, :updated_at, :milestone, :labels) + end + + it 'has time estimation attributes' do + expect(subject).to include(:time_estimate, :total_time_spent, :human_time_estimate, :human_total_time_spent) + end +end diff --git a/spec/serializers/merge_request_entity_spec.rb b/spec/serializers/merge_request_entity_spec.rb index 87832b3dca1..f9285049c0d 100644 --- a/spec/serializers/merge_request_entity_spec.rb +++ b/spec/serializers/merge_request_entity_spec.rb @@ -30,8 +30,17 @@ describe MergeRequestEntity do :assign_to_closing) end + it 'has Issuable attributes' do + expect(subject).to include(:id, :iid, :author_id, :description, :lock_version, :milestone_id, + :title, :updated_by_id, :created_at, :updated_at, :milestone, :labels) + end + + it 'has time estimation attributes' do + expect(subject).to include(:time_estimate, :total_time_spent, :human_time_estimate, :human_total_time_spent) + end + it 'has important MergeRequest attributes' do - expect(subject).to include(:diff_head_sha, :merge_commit_message, + expect(subject).to include(:state, :deleted_at, :diff_head_sha, :merge_commit_message, :has_conflicts, :has_ci, :merge_path, :conflict_resolution_path, :cancel_merge_when_pipeline_succeeds_path, diff --git a/spec/services/issuable/common_system_notes_service_spec.rb b/spec/services/issuable/common_system_notes_service_spec.rb new file mode 100644 index 00000000000..9f92b662be1 --- /dev/null +++ b/spec/services/issuable/common_system_notes_service_spec.rb @@ -0,0 +1,49 @@ +require 'spec_helper' + +describe Issuable::CommonSystemNotesService do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:issuable) { create(:issue) } + + shared_examples 'system note creation' do |update_params, note_text| + subject { described_class.new(project, user).execute(issuable, [])} + + before do + issuable.assign_attributes(update_params) + issuable.save + end + + it 'creates 1 system note with the correct content' do + expect { subject }.to change { Note.count }.from(0).to(1) + + note = Note.last + expect(note.note).to match(note_text) + expect(note.noteable_type).to eq('Issue') + end + end + + describe '#execute' do + it_behaves_like 'system note creation', { title: 'New title' }, 'changed title' + it_behaves_like 'system note creation', { description: 'New description' }, 'changed the description' + it_behaves_like 'system note creation', { discussion_locked: true }, 'locked this issue' + it_behaves_like 'system note creation', { time_estimate: 5 }, 'changed time estimate' + + context 'when new label is added' do + before do + label = create(:label, project: project) + issuable.labels << label + end + + it_behaves_like 'system note creation', {}, /added ~\w+ label/ + end + + context 'when new milestone is assigned' do + before do + milestone = create(:milestone, project: project) + issuable.milestone_id = milestone.id + end + + it_behaves_like 'system note creation', {}, 'changed milestone' + end + end +end -- cgit v1.2.1