Merge branch 'master' into dirceu/gitlab-ce-new-merge-request-from-file-edit

.gitlab-ci.yml

@@ -87,4 +87,3 @@ flay:
   tags:
     - ruby
     - mysql
-  allow_failure: true

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
-v 8.2.0 (unreleased)
+v 8.3.0 (unreleased)
+
+v 8.2.0
+  - Fix grouping of contributors by email in graph.
   - Remove CSS property preventing hard tabs from rendering in Chromium 45 (Stan Hu)
   - Fix Drone CI service template not saving properly (Stan Hu)
   - Fix avatars not showing in Atom feeds and project issues when Gravatar disabled (Stan Hu)
@@ -32,6 +35,7 @@ v 8.2.0 (unreleased)
   - Add "added", "modified" and "removed" properties to commit object in webhook
   - Rename "Back to" links to "Go to" because its not always a case it point to place user come from
   - Allow groups to appear in the search results if the group owner allows it
+  - Add email notification to former assignee upon unassignment (Adam Lieskovský)
   - New design for project graphs page
   - Remove deprecated dumped yaml file generated from previous job definitions
   - Fix incoming email config defaults
@@ -46,6 +50,7 @@ v 8.2.0 (unreleased)
   - Fix bug when milestone/label filter was empty for dashboard issues page
   - Add ability to create milestone in group projects from single form
   - Add option to create merge request when editing/creating a file (Dirceu Tiegs)
+  - Prevent the last owner of a group from being able to delete themselves by 'adding' themselves as a master (James Lopez)
 
 v 8.1.4
   - Fix bug where manually merged branches in a MR would end up with an empty diff (Stan Hu)

PROCESS.md

@@ -40,7 +40,12 @@ Workflow labels are purposely not very detailed since that would be hard to keep
 -   *Awaiting confirmation of fix*: The issue should already be solved in **master** (generally you can avoid this workflow item and just close the issue right away)
 -   *Attached MR*: There is a MR attached and the discussion should happen there
     - We need to let issues stay in sync with the MR's. We can do this with a "Closing #XXXX" or "Fixes #XXXX" comment in the MR. We can't close the issue when there is a merge request because sometimes a MR is not good and we just close the MR, then the issue must stay.
--   *Awaiting developer action/feedback*: Issue needs to be fixed or clarified by a developer
+- *Developer*: needs help from a developer
+- *UX* needs needs help from a UX designer
+- *Frontend* needs help from a Front-end engineer
+- *Graphics* needs help from a Graphics designer
+
+Example workflow: when a UX designer provided a design but it needs frontend work they remove the UX label and add the frontend label.
 
 ## Functional labels
 

VERSION

-8.1.0.pre
+8.3.0.pre

app/assets/javascripts/stat_graph_contributors_util.js.coffee

@@ -6,7 +6,7 @@ window.ContributorsStatGraphUtil =
     for entry in log
       @add_date(entry.date, total) unless total[entry.date]?
 
-      data = by_author[entry.author_name] #|| by_email[entry.author_email]      
+      data = by_author[entry.author_name] || by_email[entry.author_email]      
       data ?= @add_author(entry, by_author, by_email)
 
       @add_date(entry.date, data) unless data[entry.date]
@@ -96,4 +96,3 @@ window.ContributorsStatGraphUtil =
       true
     else
       false
\ No newline at end of file
-

app/controllers/concerns/issues_action.rb

+module IssuesAction
+  extend ActiveSupport::Concern
+
+  def issues
+    @issues = get_issues_collection
+    @issues = @issues.page(params[:page]).per(ApplicationController::PER_PAGE)
+    @issues = @issues.preload(:author, :project)
+
+    respond_to do |format|
+      format.html
+      format.atom { render layout: false }
+    end
+  end
+end

app/controllers/concerns/merge_requests_action.rb

+module MergeRequestsAction
+  extend ActiveSupport::Concern
+
+  def merge_requests
+    @merge_requests = get_merge_requests_collection
+    @merge_requests = @merge_requests.page(params[:page]).per(ApplicationController::PER_PAGE)
+    @merge_requests = @merge_requests.preload(:author, :target_project)
+  end
+end

app/controllers/dashboard_controller.rb

 class DashboardController < Dashboard::ApplicationController
+  include IssuesAction
+  include MergeRequestsAction
+
   before_action :event_filter, only: :activity
   before_action :projects, only: [:issues, :merge_requests]
 
   respond_to :html
 
-  def merge_requests
-    @merge_requests = get_merge_requests_collection
-    @merge_requests = @merge_requests.page(params[:page]).per(PER_PAGE)
-    @merge_requests = @merge_requests.preload(:author, :target_project)
-  end
-
-  def issues
-    @issues = get_issues_collection
-    @issues = @issues.page(params[:page]).per(PER_PAGE)
-    @issues = @issues.preload(:author, :project)
-
-    respond_to do |format|
-      format.html
-      format.atom { render layout: false }
-    end
-  end
-
   def activity
     @last_push = current_user.recent_push
 

app/controllers/groups/group_members_controller.rb

@@ -3,8 +3,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
 
   # Authorize
   before_action :authorize_read_group!
-  before_action :authorize_admin_group!, except: [:index, :leave]
-  before_action :authorize_admin_group_member!, only: [:create, :resend_invite]
+  before_action :authorize_admin_group_member!, except: [:index, :leave]
 
   def index
     @project = @group.projects.find(params[:project_id]) if params[:project_id]
@@ -17,7 +16,8 @@ class Groups::GroupMembersController < Groups::ApplicationController
     end
 
     @members = @members.order('access_level DESC').page(params[:page]).per(50)
-    @group_member = GroupMember.new
+
+    @group_member = @group.group_members.new
   end
 
   def create
@@ -27,25 +27,24 @@ class Groups::GroupMembersController < Groups::ApplicationController
   end
 
   def update
-    @member = @group.group_members.find(params[:id])
+    @group_member = @group.group_members.find(params[:id])
 
-    return render_403 unless can?(current_user, :update_group_member, @member)
+    return render_403 unless can?(current_user, :update_group_member, @group_member)
 
-    @member.update_attributes(member_params)
+    @group_member.update_attributes(member_params)
   end
 
   def destroy
     @group_member = @group.group_members.find(params[:id])
 
-    if can?(current_user, :destroy_group_member, @group_member)  # May fail if last owner.
+    return render_403 unless can?(current_user, :destroy_group_member, @group_member)
+
     @group_member.destroy
+
     respond_to do |format|
       format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' }
       format.js { render nothing: true }
     end
-    else
-      return render_403
-    end
   end
 
   def resend_invite
@@ -63,10 +62,11 @@ class Groups::GroupMembersController < Groups::ApplicationController
   end
 
   def leave
-    @group_member = @group.group_members.where(user_id: current_user.id).first
+    @group_member = @group.group_members.find_by(user_id: current_user)
 
     if can?(current_user, :destroy_group_member, @group_member)
       @group_member.destroy
+
       redirect_to(dashboard_groups_path, notice: "You left #{group.name} group.")
     else
       if @group.last_owner?(current_user)

app/controllers/groups_controller.rb

 class GroupsController < Groups::ApplicationController
+  include IssuesAction
+  include MergeRequestsAction
+
   skip_before_action :authenticate_user!, only: [:show, :issues, :merge_requests]
   respond_to :html
   before_action :group, except: [:new, :create]
@@ -53,23 +56,6 @@ class GroupsController < Groups::ApplicationController
     end
   end
 
-  def merge_requests
-    @merge_requests = get_merge_requests_collection
-    @merge_requests = @merge_requests.page(params[:page]).per(PER_PAGE)
-    @merge_requests = @merge_requests.preload(:author, :target_project)
-  end
-
-  def issues
-    @issues = get_issues_collection
-    @issues = @issues.page(params[:page]).per(PER_PAGE)
-    @issues = @issues.preload(:author, :project)
-
-    respond_to do |format|
-      format.html
-      format.atom { render layout: false }
-    end
-  end
-
   def edit
   end
 

app/controllers/projects/compare_controller.rb

@@ -20,8 +20,8 @@ class Projects::CompareController < Projects::ApplicationController
     if compare_result
       @commits = Commit.decorate(compare_result.commits, @project)
       @diffs = compare_result.diffs
-      @commit = @commits.last
-      @first_commit = @commits.first
+      @commit = @project.commit(head_ref)
+      @first_commit = @project.commit(base_ref)
       @line_notes = []
     end
   end

app/controllers/projects/project_members_controller.rb

 class Projects::ProjectMembersController < Projects::ApplicationController
   # Authorize
-  before_action :authorize_admin_project!, except: :leave
+  before_action :authorize_admin_project_member!, except: :leave
 
   def index
     @project_members = @project.project_members
@@ -29,10 +29,6 @@ class Projects::ProjectMembersController < Projects::ApplicationController
     @project_member = @project.project_members.new
   end
 
-  def new
-    @project_member = @project.project_members.new
-  end
-
   def create
     @project.team.add_users(params[:user_ids].split(','), params[:access_level], current_user)
 
@@ -41,11 +37,17 @@ class Projects::ProjectMembersController < Projects::ApplicationController
 
   def update
     @project_member = @project.project_members.find(params[:id])
+
+    return render_403 unless can?(current_user, :update_project_member, @project_member)
+
     @project_member.update_attributes(member_params)
   end
 
   def destroy
     @project_member = @project.project_members.find(params[:id])
+
+    return render_403 unless can?(current_user, :destroy_project_member, @project_member)
+
     @project_member.destroy
 
     respond_to do |format|
@@ -71,17 +73,23 @@ class Projects::ProjectMembersController < Projects::ApplicationController
   end
 
   def leave
-    if @project.namespace == current_user.namespace
-      message = 'You can not leave your own project. Transfer or delete the project.'
-      return redirect_back_or_default(default: { action: 'index' }, options: { alert: message })
-    end
+    @project_member = @project.project_members.find_by(user_id: current_user)
 
-    @project.project_members.find_by(user_id: current_user).destroy
+    if can?(current_user, :destroy_project_member, @project_member)
+      @project_member.destroy
 
       respond_to do |format|
-      format.html { redirect_to dashboard_projects_path }
+        format.html { redirect_to dashboard_projects_path, notice: "You left the project." }
         format.js { render nothing: true }
       end
+    else
+      if current_user == @project.owner
+        message = 'You can not leave your own project. Transfer or delete the project.'
+        redirect_back_or_default(default: { action: 'index' }, options: { alert: message })
+      else
+        render_403
+      end
+    end
   end
 
   def apply_import

app/helpers/diff_helper.rb

 module DiffHelper
+  def diff_view
+    params[:view] == 'parallel' ? 'parallel' : 'inline'
+  end
+
   def allowed_diff_size
     if diff_hard_limit_enabled?
       Commit::DIFF_HARD_LIMIT_FILES
@@ -132,25 +136,11 @@ module DiffHelper
   end
 
   def inline_diff_btn
-    params_copy = params.dup
-    params_copy[:view] = 'inline'
-    # Always use HTML to handle case where JSON diff rendered this button
-    params_copy.delete(:format)
-
-    link_to url_for(params_copy), id: "inline-diff-btn", class: (params[:view] != 'parallel' ? 'btn active' : 'btn') do
-      'Inline'
-    end
+    diff_btn('Inline', 'inline', diff_view == 'inline')
   end
 
   def parallel_diff_btn
-    params_copy = params.dup
-    params_copy[:view] = 'parallel'
-    # Always use HTML to handle case where JSON diff rendered this button
-    params_copy.delete(:format)
-
-    link_to url_for(params_copy), id: "parallel-diff-btn", class: (params[:view] == 'parallel' ? 'btn active' : 'btn') do
-      'Side-by-side'
-    end
+    diff_btn('Side-by-side', 'parallel', diff_view == 'parallel')
   end
 
   def submodule_link(blob, ref, repository = @repository)
@@ -171,7 +161,7 @@ module DiffHelper
   def commit_for_diff(diff)
     if diff.deleted_file
       first_commit = @first_commit || @commit
-      first_commit.parent
+      first_commit.parent || @first_commit
     else
       @commit
     end
@@ -187,4 +177,18 @@ module DiffHelper
   def editable_diff?(diff)
     !diff.deleted_file && @merge_request && @merge_request.source_project
   end
+
+  private
+
+  def diff_btn(title, name, selected)
+    params_copy = params.dup
+    params_copy[:view] = name
+
+    # Always use HTML to handle case where JSON diff rendered this button
+    params_copy.delete(:format)
+
+    link_to url_for(params_copy), id: "#{name}-diff-btn", class: (selected ? 'btn active' : 'btn') do
+      title
+    end
+  end
 end

app/helpers/gitlab_markdown_helper.rb

@@ -46,39 +46,13 @@ module GitlabMarkdownHelper
   end
 
   def markdown(text, context = {})
-    return "" unless text.present?
-
-    context.reverse_merge!(
-      path:         @path,
-      pipeline:     :default,
-      project:      @project,
-      project_wiki: @project_wiki,
-      ref:          @ref
-    )
-
-    user = current_user if defined?(current_user)
-
-    html = Gitlab::Markdown.render(text, context)
-    Gitlab::Markdown.post_process(html, pipeline: context[:pipeline], project: @project, user: user)
+    process_markdown(text, context)
   end
 
   # TODO (rspeicher): Remove all usages of this helper and just call `markdown`
   # with a custom pipeline depending on the content being rendered
   def gfm(text, options = {})
-    return "" unless text.present?
-
-    options.reverse_merge!(
-      path:         @path,
-      pipeline:     :default,
-      project:      @project,
-      project_wiki: @project_wiki,
-      ref:          @ref
-    )
-
-    user = current_user if defined?(current_user)
-
-    html = Gitlab::Markdown.gfm(text, options)
-    Gitlab::Markdown.post_process(html, pipeline: options[:pipeline], project: @project, user: user)
+    process_markdown(text, options, :gfm)
   end
 
   def asciidoc(text)
@@ -204,4 +178,26 @@ module GitlabMarkdownHelper
       ''
     end
   end
+
+  def process_markdown(text, options, method = :markdown)
+    return "" unless text.present?
+
+    options.reverse_merge!(
+      path:         @path,
+      pipeline:     :default,
+      project:      @project,
+      project_wiki: @project_wiki,
+      ref:          @ref
+    )
+
+    user = current_user if defined?(current_user)
+
+    html = if method == :gfm
+             Gitlab::Markdown.gfm(text, options)
+           else
+             Gitlab::Markdown.render(text, options)
+           end
+
+    Gitlab::Markdown.post_process(html, pipeline: options[:pipeline], project: @project, user: user)
+  end
 end

app/helpers/namespaces_helper.rb

@@ -17,15 +17,6 @@ module NamespacesHelper
     grouped_options_for_select(options, selected)
   end
 
-  def namespace_select_tag(id, opts = {})
-    css_class = "ajax-namespace-select "
-    css_class << "multiselect " if opts[:multiple]
-    css_class << (opts[:class] || '')
-    value = opts[:selected] || ''
-
-    hidden_field_tag(id, value, class: css_class)
-  end
-
   def namespace_icon(namespace, size = 40)
     if namespace.kind_of?(Group)
       group_icon(namespace)

app/helpers/selects_helper.rb

@@ -35,8 +35,20 @@ module SelectsHelper
   end
 
   def groups_select_tag(id, opts = {})
-    css_class = "ajax-groups-select "
-    css_class << "multiselect " if opts[:multiple]
+    opts[:class] ||= ''
+    opts[:class] << ' ajax-groups-select'
+    select2_tag(id, opts)
+  end
+
+  def namespace_select_tag(id, opts = {})
+    opts[:class] ||= ''
+    opts[:class] << ' ajax-namespace-select'
+    select2_tag(id, opts)
+  end
+
+  def select2_tag(id, opts = {})
+    css_class = ''
+    css_class << 'multiselect ' if opts[:multiple]
     css_class << (opts[:class] || '')
     value = opts[:selected] || ''
 

app/mailers/emails/issues.rb

 module Emails
   module Issues
     def new_issue_email(recipient_id, issue_id)
-      @issue = Issue.find(issue_id)
-      @project = @issue.project
-      @target_url = namespace_project_issue_url(@project.namespace, @project, @issue)
-      mail_new_thread(@issue,
-                      from: sender(@issue.author_id),
-                      to: recipient(recipient_id),
-                      subject: subject("#{@issue.title} (##{@issue.iid})"))
-
-      SentNotification.record(@issue, recipient_id, reply_key)
+      issue_mail_with_notification(issue_id, recipient_id) do
+        mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id))
+      end
     end
 
     def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id)
-      @issue = Issue.find(issue_id)
+      issue_mail_with_notification(issue_id, recipient_id) do
         @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
-      @project = @issue.project
-      @target_url = namespace_project_issue_url(@project.namespace, @project, @issue)
-      mail_answer_thread(@issue,
-                         from: sender(updated_by_user_id),
-                         to: recipient(recipient_id),
-                         subject: subject("#{@issue.title} (##{@issue.iid})"))
-
-      SentNotification.record(@issue, recipient_id, reply_key)
+        mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
+      end
     end
 
     def closed_issue_email(recipient_id, issue_id, updated_by_user_id)
-      @issue = Issue.find issue_id
-      @project = @issue.project
+      issue_mail_with_notification(issue_id, recipient_id) do
         @updated_by = User.find updated_by_user_id
-      @target_url = namespace_project_issue_url(@project.namespace, @project, @issue)
-      mail_answer_thread(@issue,
-                         from: sender(updated_by_user_id),
-                         to: recipient(recipient_id),
-                         subject: subject("#{@issue.title} (##{@issue.iid})"))
-
-      SentNotification.record(@issue, recipient_id, reply_key)
+        mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
+      end
     end
 
     def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id)
-      @issue = Issue.find issue_id
+      issue_mail_with_notification(issue_id, recipient_id) do
         @issue_status = status
-      @project = @issue.project
         @updated_by = User.find updated_by_user_id
-      @target_url = namespace_project_issue_url(@project.namespace, @project, @issue)
-      mail_answer_thread(@issue,
-                         from: sender(updated_by_user_id),
+        mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
+      end
+    end
+
+    private
+
+    def issue_thread_options(sender_id, recipient_id)
+      {
+        from: sender(sender_id),
         to: recipient(recipient_id),
-                         subject: subject("#{@issue.title} (##{@issue.iid})"))
+        subject: subject("#{@issue.title} (##{@issue.iid})")
+      }
+    end
+
+    def issue_mail_with_notification(issue_id, recipient_id)
+      @issue = Issue.find(issue_id)
+      @project = @issue.project
+      @target_url = namespace_project_issue_url(@project.namespace, @project, @issue)
+
+      yield
 
       SentNotification.record(@issue, recipient_id, reply_key)
     end

app/mailers/emails/notes.rb

 module Emails
   module Notes
     def note_commit_email(recipient_id, note_id)
-      @note = Note.find(note_id)
+      note_mail_with_notification(note_id, recipient_id) do
         @commit = @note.noteable
-      @project = @note.project
-      @target_url = namespace_project_commit_url(@project.namespace, @project,
-                                                 @commit, anchor:
-                                                 "note_#{@note.id}")
+        @target_url = namespace_project_commit_url(*note_target_url_options)
+
         mail_answer_thread(@commit,
                            from: sender(@note.author_id),
                            to: recipient(recipient_id),
                            subject: subject("#{@commit.title} (#{@commit.short_id})"))
-
-      SentNotification.record_note(@note, recipient_id, reply_key)
+      end
     end
 
     def note_issue_email(recipient_id, note_id)
-      @note = Note.find(note_id)
+      note_mail_with_notification(note_id, recipient_id) do
         @issue = @note.noteable
-      @project = @note.project
-      @target_url = namespace_project_issue_url(@project.namespace, @project,
-                                                @issue, anchor:
-                                                "note_#{@note.id}")
-      mail_answer_thread(@issue,
-                         from: sender(@note.author_id),
-                         to: recipient(recipient_id),
-                         subject: subject("#{@issue.title} (##{@issue.iid})"))
-
-      SentNotification.record_note(@note, recipient_id, reply_key)
+        @target_url = namespace_project_issue_url(*note_target_url_options)
+        mail_answer_thread(@issue, note_thread_options(recipient_id))
+      end
     end
 
     def note_merge_request_email(recipient_id, note_id)
-      @note = Note.find(note_id)
+      note_mail_with_notification(note_id, recipient_id) do
         @merge_request = @note.noteable
-      @project = @note.project
-      @target_url = namespace_project_merge_request_url(@project.namespace,
-                                                        @project,
-                                                        @merge_request, anchor:
-                                                        "note_#{@note.id}")
-      mail_answer_thread(@merge_request,
+        @target_url = namespace_project_merge_request_url(*note_target_url_options)
+        mail_answer_thread(@merge_request, note_thread_options(recipient_id))
+      end
+    end
+
+    private
+
+    def note_target_url_options
+      [@project.namespace, @project, @note.noteable, anchor: "note_#{@note.id}"]
+    end
+
+    def note_thread_options(recipient_id)
+      {
         from: sender(@note.author_id),
         to: recipient(recipient_id),
-                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+        subject: subject("#{@note.noteable.title} (##{@note.noteable.iid})")
+      }
+    end
+
+    def note_mail_with_notification(note_id, recipient_id)
+      @note = Note.find(note_id)
+      @project = @note.project
+
+      yield
 
-      SentNotification.record_note(@note, recipient_id, reply_key)
+      SentNotification.record(@note, recipient_id, reply_key)
     end
   end
 end

app/models/ability.rb

@@ -15,6 +15,7 @@ class Ability
       when "Group" then group_abilities(user, subject)
       when "Namespace" then namespace_abilities(user, subject)
       when "GroupMember" then group_member_abilities(user, subject)
+      when "ProjectMember" then project_member_abilities(user, subject)
       else []
       end.concat(global_abilities(user))
     end
@@ -231,19 +232,19 @@ class Ability
 
       # Only group masters and group owners can create new projects in group
       if group.has_master?(user) || group.has_owner?(user) || user.admin?
-        rules.push(*[
+        rules += [
           :create_projects,
           :admin_milestones
-        ])
+        ]
       end
 
       # Only group owner and administrators can admin group
       if group.has_owner?(user) || user.admin?
-        rules.push(*[
+        rules += [
           :admin_group,
           :admin_namespace,
           :admin_group_member
-        ])
+        ]
       end
 
       rules.flatten
@@ -254,16 +255,15 @@ class Ability
 
       # Only namespace owner and administrators can admin it
       if namespace.owner == user || user.admin?
-        rules.push(*[
+        rules += [
           :create_projects,
           :admin_namespace
-        ])
+        ]
       end
 
       rules.flatten
     end
 
-
     [:issue, :merge_request].each do |name|
       define_method "#{name}_abilities" do |user, subject|
         rules = []
@@ -304,16 +304,40 @@ class Ability
       rules = []
       target_user = subject.user
       group = subject.group
+
+      unless group.last_owner?(target_user)
         can_manage = group_abilities(user, group).include?(:admin_group_member)
 
-      if can_manage && (user != target_user)
+        if can_manage && user != target_user
           rules << :update_group_member
           rules << :destroy_group_member
         end
 
-      if !group.last_owner?(user) && (can_manage || (user == target_user))
+        if user == target_user
           rules << :destroy_group_member
         end
+      end
+
+      rules
+    end
+
+    def project_member_abilities(user, subject)
+      rules = []
+      target_user = subject.user
+      project = subject.project
+
+      unless target_user == project.owner
+        can_manage = project_abilities(user, project).include?(:admin_project_member)
+
+        if can_manage && user != target_user
+          rules << :update_project_member
+          rules << :destroy_project_member
+        end
+
+        if user == target_user
+          rules << :destroy_project_member
+        end
+      end
 
       rules
     end

app/models/group.rb

@@ -22,6 +22,7 @@ class Group < Namespace
   include Referable
   
   has_many :group_members, dependent: :destroy, as: :source, class_name: 'GroupMember'
+  alias_method :members, :group_members
   has_many :users, through: :group_members
 
   validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
@@ -110,10 +111,6 @@ class Group < Namespace
     has_owner?(user) && owners.size == 1
   end
 
-  def members
-    group_members
-  end
-
   def avatar_type
     unless self.avatar.image?
       self.errors.add :avatar, "only images allowed"

app/models/member.rb

@@ -34,9 +34,18 @@ class Member < ActiveRecord::Base
                                     message: "already exists in source",
                                     allow_nil: true }
   validates :access_level, inclusion: { in: Gitlab::Access.all_values }, presence: true
-  validates :invite_email,  presence: { if: :invite? }, 
-                            email: { strict_mode: true, allow_nil: true }, 
-                            uniqueness: { scope: [:source_type, :source_id], allow_nil: true }
+  validates :invite_email,
+    presence: {
+      if: :invite?
+    },
+    email: {
+      strict_mode: true,
+      allow_nil: true
+    },
+    uniqueness: {
+      scope: [:source_type, :source_id],
+      allow_nil: true
+    }
 
   scope :invite, -> { where(user_id: nil) }
   scope :non_invite, -> { where("user_id IS NOT NULL") }
@@ -82,6 +91,7 @@ class Member < ActiveRecord::Base
         member.invite_email = user
       end
 
+      if can_update_member?(current_user, member)
         member.created_by ||= current_user
         member.access_level = access_level
 
@@ -89,6 +99,16 @@ class Member < ActiveRecord::Base
       end
     end
 
+    private
+
+    def can_update_member?(current_user, member)
+      # There is no current user for bulk actions, in which case anything is allowed
+      !current_user ||
+        current_user.can?(:update_group_member, member) ||
+        current_user.can?(:update_project_member, member)
+    end
+  end
+
   def invite?
     self.invite_token.present?
   end

app/models/project_services/slack_service/note_message.rb

@@ -45,30 +45,27 @@ class SlackService
     def create_commit_note(commit)
       commit_sha = commit[:id]
       commit_sha = Commit.truncate_sha(commit_sha)
-      commit_link = "[commit #{commit_sha}](#{@note_url})"
-      title = format_title(commit[:message])
-      @message = "#{@user_name} commented on #{commit_link} in #{project_link}: *#{title}*"
+      commented_on_message(
+        "[commit #{commit_sha}](#{@note_url})",
+        format_title(commit[:message]))
     end
 
     def create_issue_note(issue)
-      issue_iid = issue[:iid]
-      note_link = "[issue ##{issue_iid}](#{@note_url})"
-      title = format_title(issue[:title])
-      @message = "#{@user_name} commented on #{note_link} in #{project_link}: *#{title}*"
+      commented_on_message(
+        "[issue ##{issue[:iid]}](#{@note_url})",
+        format_title(issue[:title]))
     end
 
     def create_merge_note(merge_request)
-      merge_request_id = merge_request[:iid]
-      merge_request_link = "[merge request ##{merge_request_id}](#{@note_url})"
-      title = format_title(merge_request[:title])
-      @message = "#{@user_name} commented on #{merge_request_link} in #{project_link}: *#{title}*"
+      commented_on_message(
+        "[merge request ##{merge_request[:iid]}](#{@note_url})",
+        format_title(merge_request[:title]))
     end
 
     def create_snippet_note(snippet)
-      snippet_id = snippet[:id]
-      snippet_link = "[snippet ##{snippet_id}](#{@note_url})"
-      title = format_title(snippet[:title])
-      @message = "#{@user_name} commented on #{snippet_link} in #{project_link}: *#{title}*"
+      commented_on_message(
+        "[snippet ##{snippet[:id]}](#{@note_url})",
+        format_title(snippet[:title]))
     end
 
     def description_message
@@ -78,5 +75,9 @@ class SlackService
     def project_link
       "[#{@project_name}](#{@project_url})"
     end
+
+    def commented_on_message(target_link, title)
+      @message = "#{@user_name} commented on #{target_link} in #{project_link}: *#{title}*"
+    end
   end
 end

app/services/issuable_base_service.rb

@@ -28,6 +28,9 @@ class IssuableBaseService < BaseService
   end
 
   def filter_params(issuable_ability_name = :issue)
+    params[:assignee_id]  = "" if params[:assignee_id] == IssuableFinder::NONE
+    params[:milestone_id] = "" if params[:milestone_id] == IssuableFinder::NONE
+
     ability = :"admin_#{issuable_ability_name}"
 
     unless can?(current_user, ability, project)
@@ -36,4 +39,36 @@ class IssuableBaseService < BaseService
       params.delete(:assignee_id)
     end
   end
+
+  def update(issuable)
+    change_state(issuable)
+    filter_params
+    old_labels = issuable.labels.to_a
+
+    if params.present? && issuable.update_attributes(params.merge(updated_by: current_user))
+      issuable.reset_events_cache
+
+      if issuable.labels != old_labels
+        create_labels_note(
+          issuable,
+          issuable.labels - old_labels,
+          old_labels - issuable.labels)
+      end
+
+      handle_changes(issuable)
+      issuable.create_new_cross_references!(current_user)
+      execute_hooks(issuable, 'update')
+    end
+
+    issuable
+  end
+
+  def change_state(issuable)
+    case params.delete(:state_event)
+    when 'reopen'
+      reopen_service.new(project, current_user, {}).execute(issuable)
+    when 'close'
+      close_service.new(project, current_user, {}).execute(issuable)
+    end
+  end
 end

app/services/issues/update_service.rb

 module Issues
   class UpdateService < Issues::BaseService
     def execute(issue)
-      case params.delete(:state_event)
-      when 'reopen'
-        Issues::ReopenService.new(project, current_user, {}).execute(issue)
-      when 'close'
-        Issues::CloseService.new(project, current_user, {}).execute(issue)
-      end
-
-      params[:assignee_id]  = "" if params[:assignee_id] == IssuableFinder::NONE
-      params[:milestone_id] = "" if params[:milestone_id] == IssuableFinder::NONE
-
-      filter_params
-      old_labels = issue.labels.to_a
-
-      if params.present? && issue.update_attributes(params.merge(updated_by: current_user))
-        issue.reset_events_cache
-
-        if issue.labels != old_labels
-          create_labels_note(
-            issue, issue.labels - old_labels, old_labels - issue.labels)
-        end
-
-        handle_changes(issue)
-        issue.create_new_cross_references!(current_user)
-        execute_hooks(issue, 'update')
-      end
-
-      issue
+      update(issue)
     end
 
     def handle_changes(issue)
@@ -44,5 +18,13 @@ module Issues
         create_title_change_note(issue, issue.previous_changes['title'].first)
       end
     end
+
+    def reopen_service
+      Issues::ReopenService
+    end
+
+    def close_service
+      Issues::CloseService
+    end
   end
 end

app/services/merge_requests/update_service.rb

@@ -11,36 +11,7 @@ module MergeRequests
       params.except!(:target_project_id)
       params.except!(:source_branch)
 
-      case params.delete(:state_event)
-      when 'reopen'
-        MergeRequests::ReopenService.new(project, current_user, {}).execute(merge_request)
-      when 'close'
-        MergeRequests::CloseService.new(project, current_user, {}).execute(merge_request)
-      end
-
-      params[:assignee_id]  = "" if params[:assignee_id] == IssuableFinder::NONE
-      params[:milestone_id] = "" if params[:milestone_id] == IssuableFinder::NONE
-
-      filter_params
-      old_labels = merge_request.labels.to_a
-
-      if params.present? && merge_request.update_attributes(params.merge(updated_by: current_user))
-        merge_request.reset_events_cache
-
-        if merge_request.labels != old_labels
-          create_labels_note(
-            merge_request,
-            merge_request.labels - old_labels,
-            old_labels - merge_request.labels
-          )
-        end
-
-        handle_changes(merge_request)
-        merge_request.create_new_cross_references!(current_user)
-        execute_hooks(merge_request, 'update')
-      end
-
-      merge_request
+      update(merge_request)
     end
 
     def handle_changes(merge_request)
@@ -68,5 +39,13 @@ module MergeRequests
         merge_request.mark_as_unchecked
       end
     end
+
+    def reopen_service
+      MergeRequests::ReopenService
+    end
+
+    def close_service
+      MergeRequests::CloseService
+    end
   end
 end

app/services/notification_service.rb

@@ -276,35 +276,25 @@ class NotificationService
   # Remove users with disabled notifications from array
   # Also remove duplications and nil recipients
   def reject_muted_users(users, project = nil)
-    users = users.to_a.compact.uniq
-    users = users.reject(&:blocked?)
-
-    users.reject do |user|
-      next user.notification.disabled? unless project
-
-      member = project.project_members.find_by(user_id: user.id)
-
-      if !member && project.group
-        member = project.group.group_members.find_by(user_id: user.id)
-      end
-
-      # reject users who globally disabled notification and has no membership
-      next user.notification.disabled? unless member
-
-      # reject users who disabled notification in project
-      next true if member.notification.disabled?
-
-      # reject users who have N_GLOBAL in project and disabled in global settings
-      member.notification.global? && user.notification.disabled?
-    end
+    reject_users(users, :disabled?, project)
   end
 
   # Remove users with notification level 'Mentioned'
   def reject_mention_users(users, project = nil)
+    reject_users(users, :mention?, project)
+  end
+
+  # Reject users which method_name from notification object returns true.
+  #
+  # Example:
+  #   reject_users(users, :watch?, project)
+  #
+  def reject_users(users, method_name, project = nil)
     users = users.to_a.compact.uniq
+    users = users.reject(&:blocked?)
 
     users.reject do |user|
-      next user.notification.mention? unless project
+      next user.notification.send(method_name) unless project
 
       member = project.project_members.find_by(user_id: user.id)
 
@@ -313,13 +303,13 @@ class NotificationService
       end
 
       # reject users who globally set mention notification and has no membership
-      next user.notification.mention? unless member
+      next user.notification.send(method_name) unless member
 
       # reject users who set mention notification in project
-      next true if member.notification.mention?
+      next true if member.notification.send(method_name)
 
       # reject users who have N_MENTION in project and disabled in global settings
-      member.notification.global? && user.notification.mention?
+      member.notification.global? && user.notification.send(method_name)
     end
   end
 
@@ -361,11 +351,13 @@ class NotificationService
   end
 
   def reassign_resource_email(target, project, current_user, method)
-    assignee_id_was = previous_record(target, "assignee_id")
-    recipients = build_recipients(target, project, current_user)
+    previous_assignee_id = previous_record(target, "assignee_id")
+    previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
+
+    recipients = build_recipients(target, project, current_user, [previous_assignee])
 
     recipients.each do |recipient|
-      mailer.send(method, recipient.id, target.id, assignee_id_was, current_user.id)
+      mailer.send(method, recipient.id, target.id, previous_assignee_id, current_user.id)
     end
   end
 
@@ -377,9 +369,11 @@ class NotificationService
     end
   end
 
-  def build_recipients(target, project, current_user)
+  def build_recipients(target, project, current_user, extra_recipients = nil)
     recipients = target.participants(current_user)
 
+    recipients = recipients.concat(extra_recipients).compact.uniq if extra_recipients
+
     recipients = add_project_watchers(recipients, project)
     recipients = reject_mention_users(recipients, project)
     recipients = reject_muted_users(recipients, project)

app/views/groups/group_members/_group_member.html.haml

 - user = member.user
 - return unless user || member.invite?
-- show_roles = true if show_roles.nil?
 
 %li{class: "#{dom_class(member)} js-toggle-container", id: dom_id(member)}
   %span{class: ("list-item-name" if show_controls)}
@@ -25,11 +24,11 @@
           = link_to member.created_by.name, user_path(member.created_by)
         = time_ago_with_tooltip(member.created_at)
 
-      - if show_controls && can?(current_user, :admin_group_member, member)
+      - if show_controls && can?(current_user, :admin_group_member, @group)
         = link_to resend_invite_group_group_member_path(@group, member), method: :post, class: "btn-xs btn", title: 'Resend invite' do
           Resend invite
 
-  - if show_roles
+  - if should_user_see_group_roles?(current_user, @group)
     %span.pull-right
       %strong= member.human_access
       - if show_controls
@@ -37,6 +36,7 @@
           = button_tag class: "btn-xs btn js-toggle-button",
                        title: 'Edit access level', type: 'button' do
             %i.fa.fa-pencil-square-o
+
         - if can?(current_user, :destroy_group_member, member)
            
           - if current_user == user

app/views/groups/group_members/index.html.haml

 - page_title "Members"
 - header_title group_title(@group, "Members", group_group_members_path(@group))
-- show_roles = should_user_see_group_roles?(current_user, @group)
-
-- if show_roles
+- if should_user_see_group_roles?(current_user, @group)
   %p.light
     Members of group have access to all group projects.
     Read more about permissions
@@ -32,7 +30,7 @@
       (#{@members.total_count})
   %ul.well-list
     - @members.each do |member|
-      = render 'groups/group_members/group_member', member: member, show_roles: show_roles, show_controls: true
+      = render 'groups/group_members/group_member', member: member, show_controls: true
 
 = paginate @members, theme: 'gitlab'
 

app/views/projects/commit/show.html.haml

@@ -3,4 +3,4 @@
 = render "commit_box"
 = render "ci_menu" if @ci_commit
 = render "projects/diffs/diffs", diffs: @diffs, project: @project
-= render "projects/notes/notes_with_form", view: params[:view]
+= render "projects/notes/notes_with_form"

app/views/projects/diffs/_diffs.html.haml

-- if params[:view] == 'parallel'
+- if diff_view == 'parallel'
   - fluid_layout true
 
 - diff_files = safe_diff_files(diffs)

app/views/projects/diffs/_file.html.haml

@@ -33,7 +33,7 @@
     -# Skipp all non non-supported blobs
     - return unless blob.respond_to?('text?')
     - if blob.text?
-      - if params[:view] == 'parallel'
+      - if diff_view == 'parallel'
         = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob, index: i
       - else
         = render "projects/diffs/text_file", diff_file: diff_file, index: i
@@ -42,4 +42,3 @@
       = render "projects/diffs/image", diff_file: diff_file, old_file: old_file, file: blob, index: i
     - else
       .nothing-here-block No preview for this file type
-

app/views/projects/notes/_form.html.haml

 = form_for [@project.namespace.becomes(Namespace), @project, @note], remote: true, html: { :'data-type' => 'json', multipart: true, id: nil, class: "new_note js-new-note-form common-note-form gfm-form" }, authenticity_token: true do |f|
-  = hidden_field_tag :view, params[:view]
+  = hidden_field_tag :view, diff_view
   = hidden_field_tag :line_type
   = note_target_fields(@note)
   = f.hidden_field :commit_id

app/views/projects/notes/_note.html.haml

@@ -59,8 +59,7 @@
         .note-text
           = preserve do
             = markdown(note.note, {no_header_anchors: true})
-        - unless note.system?
-          -# System notes can't be edited
+        - if note_editable?(note)
           = render 'projects/notes/edit_form', note: note
 
       - if note.attachment.url

app/views/projects/notes/_notes_with_form.html.haml

@@ -4,7 +4,7 @@
 
 .js-main-target-form
 - if can? current_user, :create_note, @project
-  = render "projects/notes/form", view: params[:view]
+  = render "projects/notes/form", view: diff_view
 
 :javascript
-  new Notes("#{namespace_project_notes_path(namespace_id: @project.namespace, target_id: @noteable.id, target_type: @noteable.class.name.underscore)}", #{@notes.map(&:id).to_json}, #{Time.now.to_i}, "#{params[:view]}")
+  new Notes("#{namespace_project_notes_path(namespace_id: @project.namespace, target_id: @noteable.id, target_type: @noteable.class.name.underscore)}", #{@notes.map(&:id).to_json}, #{Time.now.to_i}, "#{diff_view}")

app/views/projects/project_members/_project_member.html.haml

@@ -24,18 +24,19 @@
           = link_to member.created_by.name, user_path(member.created_by)
         = time_ago_with_tooltip(member.created_at)
 
-      - if current_user_can_admin_project
+      - if can?(current_user, :admin_project_member, @project)
         = link_to resend_invite_namespace_project_project_member_path(@project.namespace, @project, member), method: :post, class: "btn-xs btn", title: 'Resend invite' do
           Resend invite
 
-  - if current_user_can_admin_project
-    - unless @project.personal? && user == current_user
+  - if can?(current_user, :admin_project_member, @project)
     .pull-right
       %strong= member.human_access
+      - if can?(current_user, :update_project_member, member)
         = button_tag class: "btn-xs btn js-toggle-button",
                      title: 'Edit access level', type: 'button' do
           %i.fa.fa-pencil-square-o
 
+      - if can?(current_user, :destroy_project_member, member)
          
         - if current_user == user
           = link_to leave_namespace_project_project_members_path(@project.namespace, @project), data: { confirm: leave_project_message(@project) }, method: :delete, class: "btn-xs btn btn-remove", title: 'Leave project' do

app/views/projects/project_members/_team.html.haml

-- can_admin_project = can?(current_user, :admin_project, @project)
-
 .panel.panel-default.prepend-top-20
   .panel-heading
     %strong #{@project.name}
@@ -8,4 +6,4 @@
       (#{members.count})
   %ul.well-list
     - members.each do |project_member|
-      = render 'project_member', member: project_member, current_user_can_admin_project: can_admin_project
+      = render 'project_member', member: project_member

app/views/projects/project_members/update.js.haml

-- can_admin_project = can?(current_user, :admin_project, @project)
 :plain
-  $("##{dom_id(@project_member)}").replaceWith('#{escape_javascript(render("project_member", member: @project_member, current_user_can_admin_project: can_admin_project))}');
+  $("##{dom_id(@project_member)}").replaceWith('#{escape_javascript(render("project_member", member: @project_member))}');

doc/release/monthly.md

@@ -54,21 +54,25 @@ template are explained below:
 - [ ] Update GitLab.com with RC1
 - [ ] Create the regression issue in the CE issue tracker:
 
-    > This is a meta issue to index possible regressions in this monthly release
-    > and any patch versions.
-    >
-    > Please do not raise or discuss issues directly in this issue but link to
-    > issues that might warrant a patch release. If there is a Merge Request
-    > that fixes the issue, please link to that as well.
-    >
-    > Please only post one regression issue and/or merge request per comment.
-    > Comments will be updated by the release manager as they are addressed.
+    ```
+    This is a meta issue to index possible regressions in this monthly release
+    and any patch versions.
+
+    Please do not raise or discuss issues directly in this issue but link to
+    issues that might warrant a patch release. If there is a Merge Request
+    that fixes the issue, please link to that as well.
+
+    Please only post one regression issue and/or merge request per comment.
+    Comments will be updated by the release manager as they are addressed.
+    ```
 
 - [ ] Tweet about RC1 release:
 
-    > GitLab x.y.0.rc1 is available: https://packages.gitlab.com/gitlab/unstable
-    > Use at your own risk. Please link regressions issues from
-    > LINK_TO_REGRESSION_ISSUE
+    ```
+    GitLab x.y.0.rc1 is available: https://packages.gitlab.com/gitlab/unstable
+    Use at your own risk. Please link regressions issues from
+    LINK_TO_REGRESSION_ISSUE
+    ```
 
 ### Xth: (3 working days before the 22nd)
 

doc/workflow/git_lfs.md

+# Git LFS
+
+Managing large files such as audio, video and graphics files has always been one of the shortcomings of Git.
+The general recommendation is to not have Git repositories larger than 1GB to preserve performance.
+
+GitLab already supports [managing large files with git annex](http://doc.gitlab.com/ee/workflow/git_annex.html) (EE only), however in certain
+environments it is not always convenient to use different commands to differentiate between the large files and regular ones.
+
+Git LFS makes this simpler for the end user by removing the requirement to learn new commands.
+<!-- more -->
+
+## How it works
+
+Git LFS client talks with the GitLab server over HTTPS. It uses HTTP Basic Authentication to authorize client requests.
+Once the request is authorized, Git LFS client receives instructions from where to fetch or where to push the large file.
+
+## Requirements
+
+* Git LFS is supported in GitLab starting with version 8.2
+* Git LFS client version 0.6.0 and up
+
+## GitLab and Git LFS
+
+### Configuration
+
+Git LFS objects can be large in size. By default, they are stored on the server GitLab is installed on.
+
+There are two configuration options to help GitLab server administrators:
+
+* Enabling/disabling Git LFS support
+* Changing the location of LFS object storage
+
+#### Omnibus packages
+
+In `/etc/gitlab/gitlab.rb`:
+
+```ruby
+gitlab_rails['lfs_enabled'] = false
+gitlab_rails['lfs_storage_path'] = "/mnt/storage/lfs-objects"
+```
+
+#### Installations from source
+
+In `config/gitlab.yml`:
+
+```yaml
+  lfs:
+    enabled: false
+    storage_path: /mnt/storage/lfs-objects
+```
+
+## Known limitations
+
+* Git LFS v1 original API is not supported since it was deprecated early in LFS development, starting with Git LFS version 0.6.0
+* When SSH is set as a remote, Git LFS objects still go through HTTPS
+* Any Git LFS request will ask for HTTPS credentials to be provided so good Git credentials store is recommended
+* Currently, storing GitLab Git LFS objects on a non-local storage (like S3 buckets) is not supported
+* Git LFS always assumes HTTPS so if you have GitLab server on HTTP you will have to add the url to Git config manually (see #troubleshooting-tips)
+
+## Using Git LFS
+
+Lets take a look at the workflow when you need to check large files into your Git repository with Git LFS:
+For example, if you want to upload a very large file and check it into your Git repository:
+
+```bash
+git clone git@gitlab.example.com:group/project.git
+git lfs init                          # initialize the Git LFS project project
+git lfs track "*.iso"                 # select the file extensions that you want to treat as large files
+```
+
+Once a certain file extension is marked for tracking as a LFS object you can use Git as usual without having to redo the command to track a file with the same extension:
+
+```bash
+cp ~/tmp/debian.iso ./                # copy a large file into the current directory
+git add .                             # add the large file to the project
+git commit -am "Added Debian iso"     # commit the file meta data
+git push origin master                # sync the git repo and large file to the GitLab server
+```
+
+Downloading a single large file is also very simple:
+
+```bash
+git clone git@gitlab.example.com:group/project.git
+git lfs fetch debian.iso              # download the large file
+```
+
+
+## Troubleshooting
+
+### error: Repository or object not found
+
+There are a couple of reasons why this error can occur:
+
+* Wrong version of LFS client used:
+
+Check the version of Git LFS on the client machine with `git lfs version`. Only version 0.6.0 and newer are supported.
+
+* Project is using deprecated LFS API
+
+Check the Git config of the project for traces of deprecated API with `git lfs -l`. If `batch = false` is set in the config, remove the line and try using Git LFS client newer than 0.6.0.
+
+### Invalid status for <url> : 501
+
+When attempting to push a LFS object to a GitLab server that doesn't have Git LFS support enabled, server will return status `error 501`. Check with your GitLab administrator why Git LFS is not enabled on the server. See [Configuration section](#configuration) for instructions on how to enable LFS support.
+
+### getsockopt: connection refused
+
+If you push a LFS object to a project and you receive an error similar to: `Post <URL>/info/lfs/objects/batch: dial tcp IP: getsockopt: connection refused`,
+the LFS client is trying to reach GitLab through HTTPS. However, your GitLab instance is being served on HTTP.
+
+This behaviour is caused by Git LFS using HTTPS connections by default when a `lfsurl` is not set in the Git config.
+
+To prevent this from happening, set the lfs url in project Git config:
+
+```bash
+
+git config --add lfs.url "http://gitlab.example.com/group/project.git/info/lfs/objects/batch"
+```
+
+### Credentials are always required when pushing an object
+
+Given that Git LFS uses HTTP Basic Authentication to authenticate the user pushing the LFS object on every push for every object, user HTTPS credentials are required.
+
+By default, Git has support for remembering the credentials for each repository you use. This is described in [Git credentials man pages](https://git-scm.com/docs/gitcredentials).
+
+For example, you can tell Git to remember the password for a period of time in which you expect to push the objects:
+
+```bash
+git config --global credential.helper 'cache --timeout=3600'
+```
+
+This will remember the credentials for an hour after which Git operations will require re-authentication.
+
+If you are using OS X you can use `osxkeychain` to store and encrypt your credentials. For Windows, `wincred` is available.
+
+More details about various methods of storing the user credentials can be found on [Git Credential Storage documentation](https://git-scm.com/book/en/v2/Git-Tools-Credential-Storage)
\ No newline at end of file

features/groups.feature

@@ -59,6 +59,14 @@ Feature: Groups
     When I select "Mike" as "Reporter"
     Then I should see "Mike" in team list as "Reporter"
 
+  @javascript
+  Scenario: Ignore add user to group when is already Owner
+    Given gitlab user "Mike"
+    When I visit group "Owned" members page
+    And I click link "Add members"
+    When I select "Mike" as "Reporter"
+    Then I should see "Mike" in team list as "Owner"
+
   @javascript
   Scenario: Invite user to group
     When I visit group "Owned" members page

features/steps/groups.rb

@@ -48,6 +48,17 @@ class Spinach::Features::Groups < Spinach::FeatureSteps
     click_button "Add users to group"
   end
 
+  step 'I select "Mike" as "Master"' do
+    user = User.find_by(name: "Mike")
+
+    page.within ".users-group-form" do
+      select2(user.id, from: "#user_ids", multiple: true)
+      select "Master", from: "access_level"
+    end
+
+    click_button "Add users to group"
+  end
+
   step 'I should see "Mike" in team list as "Reporter"' do
     page.within '.well-list' do
       expect(page).to have_content('Mike')
@@ -55,6 +66,13 @@ class Spinach::Features::Groups < Spinach::FeatureSteps
     end
   end
 
+  step 'I should see "Mike" in team list as "Owner"' do
+    page.within '.well-list' do
+      expect(page).to have_content('Mike')
+      expect(page).to have_content('Owner')
+    end
+  end
+
   step 'I select "sjobs@apple.com" as "Reporter"' do
     page.within ".users-group-form" do
       select2("sjobs@apple.com", from: "#user_ids", multiple: true)

lib/gitlab/markdown/abstract_reference_filter.rb

+require 'gitlab/markdown'
+
+module Gitlab
+  module Markdown
+    # Issues, Snippets and Merge Requests shares similar functionality in refernce filtering.
+    # All this functionality moved to this class
+    class AbstractReferenceFilter < ReferenceFilter
+      include CrossProjectReference
+
+      def self.object_class
+        # Implement in child class
+        # Example: MergeRequest
+      end
+
+      def self.object_name
+        object_class.name.underscore
+      end
+
+      def self.object_sym
+        object_name.to_sym
+      end
+
+      def self.data_reference
+        "data-#{object_name.dasherize}"
+      end
+
+      # Public: Find references in text (like `!123` for merge requests)
+      #
+      #   AnyReferenceFilter.references_in(text) do |match, object|
+      #     "<a href=...>PREFIX#{object}</a>"
+      #   end
+      #
+      # PREFIX - symbol that detects reference (like ! for merge requests)
+      # object - reference object (snippet, merget request etc)
+      # text - String text to search.
+      #
+      # Yields the String match, the Integer referenced object ID, and an optional String
+      # of the external project reference.
+      #
+      # Returns a String replaced with the return of the block.
+      def self.references_in(text)
+        text.gsub(object_class.reference_pattern) do |match|
+          yield match, $~[object_sym].to_i, $~[:project]
+        end
+      end
+
+      def self.referenced_by(node)
+        { object_sym => LazyReference.new(object_class, node.attr(data_reference)) }
+      end
+
+      delegate :object_class, :object_sym, :references_in, to: :class
+
+      def find_object(project, id)
+        # Implement in child class
+        # Example: project.merge_requests.find
+      end
+
+      def url_for_object(object, project)
+        # Implement in child class
+        # Example: project_merge_request_url
+      end
+
+      def call
+        replace_text_nodes_matching(object_class.reference_pattern) do |content|
+          object_link_filter(content)
+        end
+      end
+
+      # Replace references (like `!123` for merge requests) in text with links
+      # to the referenced object's details page.
+      #
+      # text - String text to replace references in.
+      #
+      # Returns a String with references replaced with links. All links
+      # have `gfm` and `gfm-OBJECT_NAME` class names attached for styling.
+      def object_link_filter(text)
+        references_in(text) do |match, id, project_ref|
+          project = project_from_ref(project_ref)
+
+          if project && object = find_object(project, id)
+            title = escape_once("#{object_title}: #{object.title}")
+            klass = reference_class(object_sym)
+            data  = data_attribute(project: project.id, object_sym => object.id)
+            url = url_for_object(object, project)
+
+            %(<a href="#{url}" #{data}
+                 title="#{title}"
+                 class="#{klass}">#{match}</a>)
+          else
+            match
+          end
+        end
+      end
+
+      def object_title
+        object_class.name.titleize
+      end
+    end
+  end
+end

lib/gitlab/markdown/issue_reference_filter.rb

@@ -6,66 +6,17 @@ module Gitlab
     # issues that do not exist are ignored.
     #
     # This filter supports cross-project references.
-    class IssueReferenceFilter < ReferenceFilter
-      include CrossProjectReference
-
-      # Public: Find `#123` issue references in text
-      #
-      #   IssueReferenceFilter.references_in(text) do |match, issue, project_ref|
-      #     "<a href=...>##{issue}</a>"
-      #   end
-      #
-      # text - String text to search.
-      #
-      # Yields the String match, the Integer issue ID, and an optional String of
-      # the external project reference.
-      #
-      # Returns a String replaced with the return of the block.
-      def self.references_in(text)
-        text.gsub(Issue.reference_pattern) do |match|
-          yield match, $~[:issue].to_i, $~[:project]
-        end
-      end
-
-      def self.referenced_by(node)
-        { issue: LazyReference.new(Issue, node.attr("data-issue")) }
-      end
-
-      def call
-        replace_text_nodes_matching(Issue.reference_pattern) do |content|
-          issue_link_filter(content)
-        end
+    class IssueReferenceFilter < AbstractReferenceFilter
+      def self.object_class
+        Issue
       end
 
-      # Replace `#123` issue references in text with links to the referenced
-      # issue's details page.
-      #
-      # text - String text to replace references in.
-      #
-      # Returns a String with `#123` references replaced with links. All links
-      # have `gfm` and `gfm-issue` class names attached for styling.
-      def issue_link_filter(text)
-        self.class.references_in(text) do |match, id, project_ref|
-          project = self.project_from_ref(project_ref)
-
-          if project && issue = project.get_issue(id)
-            url = url_for_issue(id, project, only_path: context[:only_path])
-
-            title = escape_once("Issue: #{issue.title}")
-            klass = reference_class(:issue)
-            data  = data_attribute(project: project.id, issue: issue.id)
-
-            %(<a href="#{url}" #{data}
-                 title="#{title}"
-                 class="#{klass}">#{match}</a>)
-          else
-            match
-          end
-        end
+      def find_object(project, id)
+        project.get_issue(id)
       end
 
-      def url_for_issue(*args)
-        IssuesHelper.url_for_issue(*args)
+      def url_for_object(issue, project)
+        IssuesHelper.url_for_issue(issue.iid, project, only_path: context[:only_path])
       end
     end
   end

lib/gitlab/markdown/merge_request_reference_filter.rb

@@ -6,65 +6,16 @@ module Gitlab
     # to merge requests that do not exist are ignored.
     #
     # This filter supports cross-project references.
-    class MergeRequestReferenceFilter < ReferenceFilter
-      include CrossProjectReference
-
-      # Public: Find `!123` merge request references in text
-      #
-      #   MergeRequestReferenceFilter.references_in(text) do |match, merge_request, project_ref|
-      #     "<a href=...>##{merge_request}</a>"
-      #   end
-      #
-      # text - String text to search.
-      #
-      # Yields the String match, the Integer merge request ID, and an optional
-      # String of the external project reference.
-      #
-      # Returns a String replaced with the return of the block.
-      def self.references_in(text)
-        text.gsub(MergeRequest.reference_pattern) do |match|
-          yield match, $~[:merge_request].to_i, $~[:project]
-        end
-      end
-
-      def self.referenced_by(node)
-        { merge_request: LazyReference.new(MergeRequest, node.attr("data-merge-request")) }
-      end
-
-      def call
-        replace_text_nodes_matching(MergeRequest.reference_pattern) do |content|
-          merge_request_link_filter(content)
-        end
+    class MergeRequestReferenceFilter < AbstractReferenceFilter
+      def self.object_class
+        MergeRequest
       end
 
-      # Replace `!123` merge request references in text with links to the
-      # referenced merge request's details page.
-      #
-      # text - String text to replace references in.
-      #
-      # Returns a String with `!123` references replaced with links. All links
-      # have `gfm` and `gfm-merge_request` class names attached for styling.
-      def merge_request_link_filter(text)
-        self.class.references_in(text) do |match, id, project_ref|
-          project = self.project_from_ref(project_ref)
-
-          if project && merge_request = project.merge_requests.find_by(iid: id)
-            title = escape_once("Merge Request: #{merge_request.title}")
-            klass = reference_class(:merge_request)
-            data  = data_attribute(project: project.id, merge_request: merge_request.id)
-
-            url = url_for_merge_request(merge_request, project)
-
-            %(<a href="#{url}" #{data}
-                 title="#{title}"
-                 class="#{klass}">#{match}</a>)
-          else
-            match
-          end
-        end
+      def find_object(project, id)
+        project.merge_requests.find_by(iid: id)
       end
 
-      def url_for_merge_request(mr, project)
+      def url_for_object(mr, project)
         h = Gitlab::Application.routes.url_helpers
         h.namespace_project_merge_request_url(project.namespace, project, mr,
                                             only_path: context[:only_path])

lib/gitlab/markdown/snippet_reference_filter.rb

@@ -6,65 +6,16 @@ module Gitlab
     # snippets that do not exist are ignored.
     #
     # This filter supports cross-project references.
-    class SnippetReferenceFilter < ReferenceFilter
-      include CrossProjectReference
-
-      # Public: Find `$123` snippet references in text
-      #
-      #   SnippetReferenceFilter.references_in(text) do |match, snippet|
-      #     "<a href=...>$#{snippet}</a>"
-      #   end
-      #
-      # text - String text to search.
-      #
-      # Yields the String match, the Integer snippet ID, and an optional String
-      # of the external project reference.
-      #
-      # Returns a String replaced with the return of the block.
-      def self.references_in(text)
-        text.gsub(Snippet.reference_pattern) do |match|
-          yield match, $~[:snippet].to_i, $~[:project]
-        end
-      end
-
-      def self.referenced_by(node)
-        { snippet: LazyReference.new(Snippet, node.attr("data-snippet")) }
-      end
-
-      def call
-        replace_text_nodes_matching(Snippet.reference_pattern) do |content|
-          snippet_link_filter(content)
-        end
+    class SnippetReferenceFilter < AbstractReferenceFilter
+      def self.object_class
+        Snippet
       end
 
-      # Replace `$123` snippet references in text with links to the referenced
-      # snippets's details page.
-      #
-      # text - String text to replace references in.
-      #
-      # Returns a String with `$123` references replaced with links. All links
-      # have `gfm` and `gfm-snippet` class names attached for styling.
-      def snippet_link_filter(text)
-        self.class.references_in(text) do |match, id, project_ref|
-          project = self.project_from_ref(project_ref)
-
-          if project && snippet = project.snippets.find_by(id: id)
-            title = escape_once("Snippet: #{snippet.title}")
-            klass = reference_class(:snippet)
-            data  = data_attribute(project: project.id, snippet: snippet.id)
-
-            url = url_for_snippet(snippet, project)
-
-            %(<a href="#{url}" #{data}
-                 title="#{title}"
-                 class="#{klass}">#{match}</a>)
-          else
-            match
-          end
-        end
+      def find_object(project, id)
+        project.snippets.find_by(id: id)
       end
 
-      def url_for_snippet(snippet, project)
+      def url_for_object(snippet, project)
         h = Gitlab::Application.routes.url_helpers
         h.namespace_project_snippet_url(project.namespace, project, snippet,
                                         only_path: context[:only_path])

lib/gitlab/markdown/user_reference_filter.rb

@@ -85,13 +85,12 @@ module Gitlab
 
       def link_to_all
         project = context[:project]
-
         url = urls.namespace_project_url(project.namespace, project,
                                          only_path: context[:only_path])
         data = data_attribute(project: project.id)
-
         text = User.reference_prefix + 'all'
-        %(<a href="#{url}" #{data} class="#{link_class}">#{text}</a>)
+
+        link_tag(url, data, text)
       end
 
       def link_to_namespace(namespace)
@@ -105,16 +104,20 @@ module Gitlab
       def link_to_group(group, namespace)
         url = urls.group_url(group, only_path: context[:only_path])
         data = data_attribute(group: namespace.id)
-
         text = Group.reference_prefix + group
-        %(<a href="#{url}" #{data} class="#{link_class}">#{text}</a>)
+
+        link_tag(url, data, text)
       end
 
       def link_to_user(user, namespace)
         url = urls.user_url(user, only_path: context[:only_path])
         data = data_attribute(user: namespace.owner_id)
-
         text = User.reference_prefix + user
+
+        link_tag(url, data, text)
+      end
+
+      def link_tag(url, data, text)
         %(<a href="#{url}" #{data} class="#{link_class}">#{text}</a>)
       end
     end

lib/tasks/flay.rake

 desc 'Code duplication analyze via flay'
 task :flay do
-  output = %x(bundle exec flay --mass 30 app/ lib/gitlab/)
+  output = %x(bundle exec flay --mass 35 app/ lib/gitlab/)
 
   if output.include? "Similar code found"
     puts output

spec/services/issues/update_service_spec.rb

@@ -3,13 +3,15 @@ require 'spec_helper'
 describe Issues::UpdateService do
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
-  let(:issue) { create(:issue, title: 'Old title') }
+  let(:user3) { create(:user) }
+  let(:issue) { create(:issue, title: 'Old title', assignee_id: user3.id) }
   let(:label) { create(:label) }
   let(:project) { issue.project }
 
   before do
     project.team << [user, :master]
     project.team << [user2, :developer]
+    project.team << [user3, :developer]
   end
 
   describe 'execute' do
@@ -34,9 +36,11 @@ describe Issues::UpdateService do
       it { expect(@issue.labels.count).to eq(1) }
       it { expect(@issue.labels.first.title).to eq('Bug') }
 
-      it 'should send email to user2 about assign of new issue' do
-        email = ActionMailer::Base.deliveries.last
-        expect(email.to.first).to eq(user2.email)
+      it 'should send email to user2 about assign of new issue and email to user3 about issue unassignment' do
+        deliveries = ActionMailer::Base.deliveries
+        email = deliveries.last
+        recipients = deliveries.last(2).map(&:to).flatten
+        expect(recipients).to include(user2.email, user3.email)
         expect(email.subject).to include(issue.title)
       end
 

spec/services/merge_requests/update_service_spec.rb

@@ -3,7 +3,8 @@ require 'spec_helper'
 describe MergeRequests::UpdateService do
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
-  let(:merge_request) { create(:merge_request, :simple, title: 'Old title') }
+  let(:user3) { create(:user) }
+  let(:merge_request) { create(:merge_request, :simple, title: 'Old title', assignee_id: user3.id) }
   let(:project) { merge_request.project }
   let(:label) { create(:label) }
 
@@ -47,9 +48,11 @@ describe MergeRequests::UpdateService do
                                with(@merge_request, 'update')
       end
 
-      it 'should send email to user2 about assign of new merge_request' do
-        email = ActionMailer::Base.deliveries.last
-        expect(email.to.first).to eq(user2.email)
+      it 'should send email to user2 about assign of new merge request and email to user3 about merge request unassignment' do
+        deliveries = ActionMailer::Base.deliveries
+        email = deliveries.last
+        recipients = deliveries.last(2).map(&:to).flatten
+        expect(recipients).to include(user2.email, user3.email)
         expect(email.subject).to include(merge_request.title)
       end