Merge remote-tracking branch 'origin/master' into ce-to-ee

CHANGELOG-EE

@@ -2,9 +2,14 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.7.0 (unreleased)
 
-
-v 8.6.1 (unreleased)
-
+v 8.6.2 (unreleased)
+  - Mark pending todos as done when approving a merge request
+
+v 8.6.1
+  - Only rename the `light_logo` column in the `appearances` table if its not there yet. !290
+  - Fix diffs in text part of email-on-push messages (Stan Hu). !293
+  - Fix an issue with methods not accessible in some controllers. !295
+  - Ensure Projects::ApproversController inherits from Projects::ApplicationController. !296
 
 v 8.6.0
   - Handle duplicate appearances table creation issue with upgrade from CE to EE

app/controllers/audit_events_controller.rb

-class AuditEventsController < ApplicationController
-  # Authorize
-  before_action :repository, only: :project_log
-  before_action :authorize_admin_project!, only: :project_log
-  before_action :group, only: :group_log
-  before_action :authorize_admin_group!, only: :group_log
-
-  layout :determine_layout
-
-  def project_log
-    @events = AuditEvent.where(entity_type: "Project", entity_id: project.id).page(params[:page]).per(20)
-  end
-
-  def group_log
-    @events = AuditEvent.where(entity_type: "Group", entity_id: group.id).page(params[:page]).per(20)
-  end
-
-  private
-
-  def group
-    @group ||= Group.find_by(path: params[:group_id])
-  end
-
-  def authorize_admin_group!
-    render_404 unless can?(current_user, :admin_group, group)
-  end
-
-  def determine_layout
-    if @project
-      'project_settings'
-    elsif @group
-      'group_settings'
-    end
-  end
-
-  def audit_events_params
-    params.permit(:project_id, :group_id)
-  end
-end

app/controllers/groups/audit_events_controller.rb

+class Groups::AuditEventsController < Groups::ApplicationController
+  before_action :authorize_admin_group!
+
+  layout 'group_settings'
+
+  def index
+    @events = group.audit_events.page(params[:page])
+  end
+end

app/controllers/projects/audit_events_controller.rb

+class Projects::AuditEventsController < Projects::ApplicationController
+  before_action :authorize_admin_project!
+
+  layout 'project_settings'
+
+  def index
+    @events = project.audit_events.page(params[:page])
+  end
+end

app/models/group.rb

@@ -29,6 +29,9 @@ class Group < Namespace
   has_many :shared_projects, through: :project_group_links, source: :project
   has_many :ldap_group_links, foreign_key: 'group_id', dependent: :destroy
   has_many :hooks, dependent: :destroy, class_name: 'GroupHook'
+  # We cannot simply set `has_many :audit_events, as: :entity, dependent: :destroy`
+  # here since Group inherits from Namespace, the entity_type would be set to `Namespace`.
+  has_many :audit_events, -> { where(entity_type: Group) }, dependent: :destroy, foreign_key: 'entity_id'
 
   validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
   validate :visibility_level_allowed_by_projects

app/models/project.rb

@@ -164,6 +164,7 @@ class Project < ActiveRecord::Base
   has_many :invited_groups, through: :project_group_links, source: :group
   has_many :pages_domains, dependent: :destroy
   has_many :todos, dependent: :destroy
+  has_many :audit_events, as: :entity, dependent: :destroy
 
   has_one :import_data, dependent: :destroy, class_name: "ProjectImportData"
 

app/services/merge_requests/approval_service.rb

@@ -5,6 +5,7 @@ module MergeRequests
 
       if approval.save
         create_approval_note(merge_request)
+        mark_pending_todos_as_done(merge_request)
 
         if merge_request.approvals_left.zero?
           execute_hooks(merge_request, 'approved')
@@ -17,5 +18,9 @@ module MergeRequests
     def create_approval_note(merge_request)
       SystemNoteService.approve_mr(merge_request, current_user)
     end
+
+    def mark_pending_todos_as_done(merge_request)
+      todo_service.mark_pending_todos_as_done(merge_request, current_user)
+    end
   end
 end

app/views/audit_events/group_log.html.haml → app/views/groups/audit_events/index.html.haml

@@ -2,4 +2,4 @@
 %h3.page-title Group Audit Events
 %p.light Events in #{@group.name}
 
-= render 'event_table', events: @events
+= render 'shared/audit_events/event_table', events: @events

app/views/audit_events/project_log.html.haml → app/views/projects/audit_events/index.html.haml

@@ -2,4 +2,4 @@
 %h3.page-title Project Audit Events
 %p.light Events in #{@project.path_with_namespace}
 
-= render 'event_table', events: @events
+= render 'shared/audit_events/event_table', events: @events

config/routes.rb

@@ -424,10 +424,9 @@ Rails.application.routes.draw do
 
       resource :avatar, only: [:destroy]
       resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :update, :new, :create]
+      resources :audit_events, only: [:index]
     end
 
-    get "/audit_events" => "audit_events#group_log"
-
     resources :hooks, only: [:index, :create, :destroy], constraints: { id: /\d+/ }, module: :groups do
       member do
         get :test
@@ -797,10 +796,8 @@ Rails.application.routes.draw do
             get :build, constraints: { format: /svg/ }
           end
         end
+        resources :audit_events, only: [:index]
       end
-
-      get "/audit_events" => "audit_events#project_log"
-
     end
   end
 

db/migrate/20160302141317_rename_header_field_on_appearrance.rb

 class RenameHeaderFieldOnAppearrance < ActiveRecord::Migration
-  def change
-    rename_column :appearances, :light_logo, :header_logo
+  def up
+    unless column_exists?(:appearances, :header_logo)
+      rename_column :appearances, :light_logo, :header_logo
+    end
 
-    remove_column :appearances, :dark_logo
+    if column_exists?(:appearances, :dark_logo)
+      remove_column :appearances, :dark_logo
+    end
+  end
+
+  def down
+    rename_column(:appearances, :header_logo, :light_logo)
+    add_column(:appearances, :dark_logo, :string)
   end
 end

doc/pages/README.md

@@ -307,8 +307,8 @@ See an example that has different files in the [`master` branch][jekyll-master]
 and the source files for Jekyll are in a [`pages` branch][jekyll-pages] which
 also includes `.gitlab-ci.yml`.
 
-[jekyll-master]: https://gitlab.com/gitlab-examples/pages-jekyll-branched/tree/master
-[jekyll-pages]: https://gitlab.com/gitlab-examples/pages-jekyll-branched/tree/pages
+[jekyll-master]: https://gitlab.com/pages/jekyll-branched/tree/master
+[jekyll-pages]: https://gitlab.com/pages/jekyll-branched/tree/pages
 
 ## Next steps
 
@@ -320,17 +320,17 @@ what more you can do with GitLab Pages.
 Below is a list of example projects for GitLab Pages with a plain HTML website
 or various static site generators. Contributions are very welcome.
 
-- [Plain HTML](https://gitlab.com/gitlab-examples/pages-plain-html)
-- [Jekyll](https://gitlab.com/gitlab-examples/pages-jekyll)
-- [Hugo](https://gitlab.com/gitlab-examples/pages-hugo)
-- [Middleman](https://gitlab.com/gitlab-examples/pages-middleman)
-- [Hexo](https://gitlab.com/gitlab-examples/pages-hexo)
-- [Brunch](https://gitlab.com/gitlab-examples/pages-brunch)
-- [Metalsmith](https://gitlab.com/gitlab-examples/pages-metalsmith)
-- [Harp](https://gitlab.com/gitlab-examples/pages-harp)
+- [Plain HTML](https://gitlab.com/pages/plain-html)
+- [Jekyll](https://gitlab.com/pages/jekyll)
+- [Hugo](https://gitlab.com/pages/hugo)
+- [Middleman](https://gitlab.com/pages/middleman)
+- [Hexo](https://gitlab.com/pages/hexo)
+- [Brunch](https://gitlab.com/pages/brunch)
+- [Metalsmith](https://gitlab.com/pages/metalsmith)
+- [Harp](https://gitlab.com/pages/harp)
 
-Visit the gitlab-examples group for a full list of projects:
-<https://gitlab.com/groups/gitlab-examples>.
+Visit the GitLab Pages group for a full list of example projects:
+<https://gitlab.com/groups/pages>.
 
 ### Add a custom domain to your Pages website
 
@@ -458,6 +458,6 @@ For a list of known issues, visit GitLab's [public issue tracker].
 [gitlab runner]: https://gitlab.com/gitlab-org/gitlab-ci-multi-runner
 [pages]: ../ci/yaml/README.md#pages
 [staticgen]: https://www.staticgen.com/
-[pages-jekyll]: https://gitlab.com/gitlab-examples/pages-jekyll
+[pages-jekyll]: https://gitlab.com/pages/jekyll
 [metarefresh]: https://en.wikipedia.org/wiki/Meta_refresh
 [public issue tracker]: https://gitlab.com/gitlab-org/gitlab-ee/issues?label_name=Pages

features/project/audit_event.feature

-Feature: Audit Event
-  Background:
-    Given I sign in as a user
-    And I own project "Shop"
-
-  Scenario: I add new deploy key
-    Given I created new depoloy key
-    When I visit audit event page
-    Then I see deploy key event
-    When I remove deploy key
-    And I visit audit event page
-    Then I see remove deploy key event
-
-  @javascript
-  Scenario: I should see audit events
-    And gitlab user "Pete"
-    And "Pete" is "Shop" developer
-    When I visit project "Shop" page
-    And I go to "Members"
-    And I change "Pete" access level to master
-    And I visit project "Shop" settings page
-    And I go to "Audit Events"
-    Then I should see the audit event listed

features/steps/groups.rb

@@ -66,37 +66,6 @@ class Spinach::Features::Groups < Spinach::FeatureSteps
     end
   end
 
-  step 'I change the role to "Developer"' do
-    user = User.find_by(name: "Mary Jane")
-    member = Group.find_by(name: "Owned").members.where(user_id: user.id).first
-
-    page.within "#group_member_#{member.id}" do
-      find(".js-toggle-button").click
-      page.within "#edit_group_member_#{member.id}" do
-        select 'Developer', from: 'group_member_access_level'
-        click_on 'Save'
-      end
-    end
-
-    page.within "#group_member_#{member.id}" do
-      expect(page).to have_content "Developer"
-    end
-  end
-
-  step 'I go to "Audit Events"' do
-    find(:link, 'Audit Events').trigger('click')
-  end
-
-  step 'I should see the audit event listed' do
-    page.within('table#audits') do
-      expect(page).to have_content 'Add user access as reporter'
-      expect(page).to have_content 'Change access level from reporter to developer'
-      expect(page).to have_content 'Remove user access'
-      expect(page).to have_content('John Doe', count: 3)
-      expect(page).to have_content('Mary Jane', count: 3)
-    end
-  end
-
   step 'project from group "Owned" has issues assigned to me' do
     create :issue,
       project: project,

features/steps/project/audit_event.rb

-class Spinach::Features::AuditEvent < Spinach::FeatureSteps
-  include SharedAuthentication
-  include SharedProject
-  include SharedPaths
-
-  step 'I created new depoloy key' do
-    visit new_namespace_project_deploy_key_path(@project.namespace, @project)
-
-    fill_in "deploy_key_title", with: "laptop"
-    fill_in "deploy_key_key", with: "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAzrEJUIR6Y03TCE9rIJ+GqTBvgb8t1jI9h5UBzCLuK4VawOmkLornPqLDrGbm6tcwM/wBrrLvVOqi2HwmkKEIecVO0a64A4rIYScVsXIniHRS6w5twyn1MD3sIbN+socBDcaldECQa2u1dI3tnNVcs8wi77fiRe7RSxePsJceGoheRQgC8AZ510UdIlO+9rjIHUdVN7LLyz512auAfYsgx1OfablkQ/XJcdEwDNgi9imI6nAXhmoKUm1IPLT2yKajTIC64AjLOnE0YyCh6+7RFMpiMyu1qiOCpdjYwTgBRiciNRZCH8xIedyCoAmiUgkUT40XYHwLuwiPJICpkAzp7Q== user@laptop"
-
-    click_button "Create"
-  end
-
-  step 'I remove deploy key' do
-    visit namespace_project_deploy_keys_path(@project.namespace, @project)
-    click_link "Remove"
-  end
-
-  step 'I see remove deploy key event' do
-    expect(page).to have_content("Remove deploy key")
-  end
-
-  step 'I see deploy key event' do
-    expect(page).to have_content("Add deploy key")
-  end
-
-  step 'I should see the audit event listed' do
-    page.within('table#audits') do
-      expect(page).to have_content "Change access level from developer to master"
-      expect(page).to have_content(project.owner.name)
-      expect(page).to have_content('Pete')
-    end
-  end
-
-  step 'gitlab user "Pete"' do
-    create(:user, name: "Pete")
-  end
-
-  step '"Pete" is "Shop" developer' do
-    user = User.find_by(name: "Pete")
-    project = Project.find_by(name: "Shop")
-    project.team << [user, :developer]
-  end
-
-  step 'I go to "Members"' do
-    find(:link, 'Members').trigger('click')
-  end
-
-  step 'I visit project "Shop" settings page' do
-    find(:link, 'Settings').trigger('click')
-  end
-
-  step 'I change "Pete" access level to master' do
-    user = User.find_by(name: "Pete")
-    project_member = @project.project_members.find_by(user_id: user)
-
-    page.within "#project_member_#{project_member.id}" do
-      click_button "Edit access level"
-      select "Master", from: "project_member_access_level"
-      click_button "Save"
-    end
-
-    sleep 0.05
-  end
-
-  step 'I go to "Audit Events"' do
-    find(:link, 'Audit Events').trigger('click')
-  end
-end

features/steps/project/project.rb

@@ -122,14 +122,6 @@ class Spinach::Features::Project < Spinach::FeatureSteps
     end
   end
 
-  step 'I visit project "Shop" settings page' do
-    click_link 'Settings'
-  end
-
-  step 'I go to "Members"' do
-    click_link 'Members'
-  end
-
   step 'I add project tags' do
     fill_in 'Tags', with: 'tag1, tag2'
   end

features/steps/shared/paths.rb

@@ -79,10 +79,6 @@ module SharedPaths
     visit edit_group_path(Group.find_by(name: "Guest"))
   end
 
-  step 'I visit audit event page' do
-    visit namespace_project_audit_events_path(@project.namespace, @project)
-  end
-
   # ----------------------------------------
   # Dashboard
   # ----------------------------------------

spec/features/groups/audit_events.rb

+require 'spec_helper'
+
+feature 'Groups > Audit Events', js: true, feature: true do
+  let(:user) { create(:user) }
+  let(:pete) { create(:user, name: 'Pete') }
+  let(:group) { create(:group) }
+
+  before do
+    group.add_owner(user)
+    group.add_developer(pete)
+    login_with(user)
+  end
+
+  describe 'changing a user access level' do
+    it "appears in the group's audit events" do
+      visit group_path(group)
+
+      click_link 'Members'
+
+      group_member = group.members.find_by(user_id: pete)
+      page.within "#group_member_#{group_member.id}" do
+        click_button 'Edit access level'
+        select 'Master', from: 'group_member_access_level'
+        click_button 'Save'
+      end
+
+      # This is to avoid a Capybara::Poltergeist::MouseEventFailed error
+      find('a[title=Settings]').trigger('click')
+
+      click_link 'Audit Events'
+
+      page.within('table#audits') do
+        expect(page).to have_content 'Change access level from developer to master'
+        expect(page).to have_content(user.name)
+        expect(page).to have_content('Pete')
+      end
+    end
+  end
+end

spec/features/projects/audit_events.rb

+require 'spec_helper'
+
+feature 'Projects > Audit Events', js: true, feature: true do
+  let(:user) { create(:user) }
+  let(:pete) { create(:user, name: 'Pete') }
+  let(:project) { create(:project, namespace: user.namespace) }
+
+  before do
+    project.team << [user, :master]
+    login_with(user)
+  end
+
+  describe 'adding an SSH key' do
+    it "appears in the project's audit events" do
+      visit new_namespace_project_deploy_key_path(project.namespace, project)
+
+      fill_in 'deploy_key_title', with: 'laptop'
+      fill_in 'deploy_key_key', with: 'ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAzrEJUIR6Y03TCE9rIJ+GqTBvgb8t1jI9h5UBzCLuK4VawOmkLornPqLDrGbm6tcwM/wBrrLvVOqi2HwmkKEIecVO0a64A4rIYScVsXIniHRS6w5twyn1MD3sIbN+socBDcaldECQa2u1dI3tnNVcs8wi77fiRe7RSxePsJceGoheRQgC8AZ510UdIlO+9rjIHUdVN7LLyz512auAfYsgx1OfablkQ/XJcdEwDNgi9imI6nAXhmoKUm1IPLT2yKajTIC64AjLOnE0YyCh6+7RFMpiMyu1qiOCpdjYwTgBRiciNRZCH8xIedyCoAmiUgkUT40XYHwLuwiPJICpkAzp7Q== user@laptop'
+
+      click_button 'Create'
+
+      visit namespace_project_audit_events_path(project.namespace, project)
+
+      expect(page).to have_content('Add deploy key')
+
+      visit namespace_project_deploy_keys_path(project.namespace, project)
+      click_link 'Remove'
+
+      visit namespace_project_audit_events_path(project.namespace, project)
+
+      expect(page).to have_content('Remove deploy key')
+    end
+  end
+
+  describe 'changing a user access level' do
+    before do
+      project.team << [pete, :developer]
+    end
+
+    it "appears in the project's audit events" do
+      visit namespace_project_path(project.namespace, project)
+
+      click_link 'Members'
+
+      project_member = project.project_member(pete)
+      page.within "#project_member_#{project_member.id}" do
+        click_button 'Edit access level'
+        select 'Master', from: 'project_member_access_level'
+        click_button 'Save'
+      end
+
+      # This is to avoid a Capybara::Poltergeist::MouseEventFailed error
+      find('a[title=Settings]').trigger('click')
+
+      click_link 'Audit Events'
+
+      page.within('table#audits') do
+        expect(page).to have_content 'Change access level from developer to master'
+        expect(page).to have_content(project.owner.name)
+        expect(page).to have_content('Pete')
+      end
+    end
+  end
+end

spec/services/merge_requests/approval_service_spec.rb

@@ -3,35 +3,46 @@ require 'rails_helper'
 describe MergeRequests::ApprovalService, services: true do
   describe '#execute' do
     let(:user)          { build_stubbed(:user) }
-    let(:project)       { build_stubbed(:empty_project) }
     let(:merge_request) { build_stubbed(:merge_request) }
+    let(:project)       { merge_request.project }
+    let!(:todo)         { create(:todo, user: user, project: project, target: merge_request) }
+
+    subject(:service) { described_class.new(project, user) }
 
     context 'with invalid approval' do
-      it 'does not create an approval note' do
-        allow(merge_request.approvals).
-          to receive(:new).and_return(double(save: false))
-        service = described_class.new(double, double)
+      before do
+        allow(merge_request.approvals).to receive(:new).and_return(double(save: false))
+      end
 
+      it 'does not create an approval note' do
         expect(SystemNoteService).not_to receive(:approve_mr)
 
         service.execute(merge_request)
       end
+
+      it 'does not mark pending todos as done' do
+        service.execute(merge_request)
+
+        expect(todo.reload).to be_pending
+      end
     end
 
     context 'with valid approval' do
       it 'creates an approval note' do
-        service = described_class.new(project, user)
-
         expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user)
 
         service.execute(merge_request)
       end
 
+      it 'marks pending todos as done' do
+        service.execute(merge_request)
+
+        expect(todo.reload).to be_done
+      end
+
       context 'with remaining approvals' do
         it 'does not fire a webhook' do
           expect(merge_request).to receive(:approvals_left).and_return(5)
-
-          service = described_class.new(project, user)
           expect(service).not_to receive(:execute_hooks)
 
           service.execute(merge_request)
@@ -41,8 +52,6 @@ describe MergeRequests::ApprovalService, services: true do
       context 'with required approvals' do
         it 'fires a webhook' do
           expect(merge_request).to receive(:approvals_left).and_return(0)
-
-          service = described_class.new(project, user)
           expect(service).to receive(:execute_hooks).with(merge_request, 'approved')
 
           service.execute(merge_request)