Merge branch 'jk-epic-changes-ce-port' into 'master'

CE port of code changed for epics

See merge request gitlab-org/gitlab-ce!15144

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

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

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

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
 

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

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)

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

app/models/epic.rb

+# 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

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
 

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

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

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

app/serializers/issuable_entity.rb

 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

app/serializers/issue_entity.rb

 class IssueEntity < IssuableEntity
-  include RequestAwareEntity
+  include TimeTrackableEntity
 
+  expose :state
+  expose :deleted_at
   expose :branch_name
   expose :confidential
   expose :discussion_locked

app/serializers/merge_request_entity.rb

 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

app/serializers/time_trackable_entity.rb

+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

app/services/issuable/common_system_notes_service.rb

+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

app/services/issuable_base_service.rb

 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)
+  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
-
-    if issuable.time_spent?
-      create_time_spent_note(issuable)
   end
 
-    create_labels_note(issuable, old_labels) if issuable.labels != old_labels
+  # override if needed
+  def handle_changes(issuable, options)
   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
+  # override if needed
+  def execute_hooks(issuable, action = 'open', params = {})
   end
 end

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)

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

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"

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
 

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

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)
         %(<a href="#{url}" #{data} class="#{link_class}" title="#{escape_once(title)}">#{link_content}</a>)
       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

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) }

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

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

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' => '<p dir="auto">issue text</p>',
+        'initialDescriptionText' => 'issue text',
+        'initialTaskStatus' => '0 of 0 tasks completed'
+      }
+      expect(JSON.parse(helper.issuable_initial_data(issue))).to eq(expected_data)
+    end
+  end
 end

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 = %{<a href="#{reference}">Reference</a>}
+
+      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 = %{<a href="#{reference}">Reference</a>}
+      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

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

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(/\(<a.+>#{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

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

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(/<a.+>#{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

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("<p>#{user.to_reference}</p>")

spec/serializers/issue_entity_spec.rb

+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

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,

spec/services/issuable/common_system_notes_service_spec.rb

+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