Merge remote-tracking branch 'upstream/master' into preserve-note_type-and-position

* upstream/master:
  Restrict ProjectCacheWorker jobs to one per 15 min
  Removed code from project members controller
  Make label API spec independent of order
  Refactoring find_commits functionality
  Differentiate the expire from leave event
  Remove pagination description from individual doc
  Fix a broken table in Project API doc
  Create project feature when project is created
  Simpler arguments passed to named_route on toggle_award_url helper method
  Fixed height of issue board blank state

CHANGELOG.md

@@ -1,6 +1,8 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 ## 8.14.0 (2016-11-22)
+  - Adds user project membership expired event to clarify why user was removed (Callum Dryden)
+  - Simpler arguments passed to named_route on toggle_award_url helper method
 
 ## 8.13.0 (2016-10-22)
 
@@ -25,6 +27,7 @@ Please view this file on the master branch, on stable branches it's out of date.
   - Add an example for testing a phoenix application with Gitlab CI in the docs (Manthan Mallikarjun)
   - Cancelled pipelines could be retried. !6927
   - Updating verbiage on git basics to be more intuitive
+  - Fix project_feature record not generated on project creation
   - Clarify documentation for Runners API (Gennady Trafimenkov)
   - The instrumentation for Banzai::Renderer has been restored
   - Change user & group landing page routing from /u/:username to /:username
@@ -33,6 +36,7 @@ Please view this file on the master branch, on stable branches it's out of date.
   - AbstractReferenceFilter caches project_refs on RequestStore when active
   - Replaced the check sign to arrow in the show build view. !6501
   - Add a /wip slash command to toggle the Work In Progress status of a merge request. !6259 (tbalthazar)
+  - ProjectCacheWorker updates caches at most once per 15 minutes per project
   - Fix Error 500 when viewing old merge requests with bad diff data
   - Create a new /templates namespace for the /licenses, /gitignores and /gitlab_ci_ymls API endpoints. !5717 (tbalthazar)
   - Speed-up group milestones show page
@@ -134,6 +138,7 @@ Please view this file on the master branch, on stable branches it's out of date.
   - Delete dynamic environments
   - Fix buggy iOS tooltip layering behavior.
   - Make guests unable to view MRs on private projects
+  - Fix broken Project API docs (Takuya Noguchi)
 
 ## 8.12.7
 

app/assets/stylesheets/pages/boards.scss

@@ -1,4 +1,3 @@
-lex
 [v-cloak] {
   display: none;
 }
@@ -132,7 +131,7 @@ lex
 }
 
 .board-blank-state {
-  height: 100%;
+  height: calc(100% - 49px);
   padding: $gl-padding;
   background-color: #fff;
 }

app/controllers/projects/commits_controller.rb

@@ -13,7 +13,7 @@ class Projects::CommitsController < Projects::ApplicationController
 
     @commits =
       if search.present?
-        @repository.find_commits_by_message(search, @ref, @path, @limit, @offset).compact
+        @repository.find_commits_by_message(search, @ref, @path, @limit, @offset)
       else
         @repository.commits(@ref, path: @path, limit: @limit, offset: @offset)
       end

app/controllers/projects/project_members_controller.rb

@@ -32,21 +32,6 @@ class Projects::ProjectMembersController < Projects::ApplicationController
       current_user: current_user
     )
 
-    if params[:group_ids].present?
-      group_ids = params[:group_ids].split(',')
-      groups = Group.where(id: group_ids)
-
-      groups.each do |group|
-        next unless can?(current_user, :read_group, group)
-
-        project.project_group_links.create(
-          group: group,
-          group_access: params[:access_level],
-          expires_at: params[:expires_at]
-        )
-      end
-    end
-
     redirect_to namespace_project_project_members_path(@project.namespace, @project)
   end
 

app/helpers/award_emoji_helper.rb

@@ -3,8 +3,8 @@ module AwardEmojiHelper
     return url_for([:toggle_award_emoji, awardable]) unless @project
 
     if awardable.is_a?(Note)
-      # We render a list of notes very frequently and calling the specific method is a lot faster than the generic one (6.5x)
-      toggle_award_emoji_namespace_project_note_url(namespace_id: @project.namespace, project_id: @project, id: awardable.id)
+      # We render a list of notes very frequently and calling the specific method is a lot faster than the generic one (4.5x)
+      toggle_award_emoji_namespace_project_note_url(@project.namespace, @project, awardable.id)
     else
       url_for([:toggle_award_emoji, @project.namespace.becomes(Namespace), @project, awardable])
     end

app/models/concerns/expirable.rb

@@ -5,11 +5,15 @@ module Expirable
     scope :expired, -> { where('expires_at <= ?', Time.current) }
   end
 
+  def expired?
+    expires? && expires_at <= Time.current
+  end
+
   def expires?
     expires_at.present?
   end
 
   def expires_soon?
-    expires_at < 7.days.from_now
+    expires? && expires_at < 7.days.from_now
   end
 end

app/models/event.rb

@@ -12,6 +12,7 @@ class Event < ActiveRecord::Base
   JOINED    = 8 # User joined project
   LEFT      = 9 # User left project
   DESTROYED = 10
+  EXPIRED   = 11 # User left project due to expiry
 
   RESET_PROJECT_ACTIVITY_INTERVAL = 1.hour
 
@@ -115,6 +116,10 @@ class Event < ActiveRecord::Base
     action == LEFT
   end
 
+  def expired?
+    action == EXPIRED
+  end
+
   def destroyed?
     action == DESTROYED
   end
@@ -124,7 +129,7 @@ class Event < ActiveRecord::Base
   end
 
   def membership_changed?
-    joined? || left?
+    joined? || left? || expired?
   end
 
   def created_project?
@@ -184,6 +189,8 @@ class Event < ActiveRecord::Base
       'joined'
     elsif left?
       'left'
+    elsif expired?
+      'removed due to membership expiration from'
     elsif destroyed?
       'destroyed'
     elsif commented?

app/models/members/project_member.rb

@@ -121,7 +121,11 @@ class ProjectMember < Member
   end
 
   def post_destroy_hook
-    event_service.leave_project(self.project, self.user)
+    if expired?
+      event_service.expired_leave_project(self.project, self.user)
+    else
+      event_service.leave_project(self.project, self.user)
+    end
 
     super
   end

app/models/project.rb

@@ -32,8 +32,8 @@ class Project < ActiveRecord::Base
   default_value_for(:shared_runners_enabled) { current_application_settings.shared_runners_enabled }
 
   after_create :ensure_dir_exist
+  after_create :create_project_feature, unless: :project_feature
   after_save :ensure_dir_exist, if: :namespace_id_changed?
-  after_initialize :setup_project_feature
 
   # set last_activity_at to the same as created_at
   after_create :set_last_activity_at
@@ -1310,11 +1310,6 @@ class Project < ActiveRecord::Base
     "projects/#{id}/pushes_since_gc"
   end
 
-  # Prevents the creation of project_feature record for every project
-  def setup_project_feature
-    build_project_feature unless project_feature
-  end
-
   def default_branch_protected?
     current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_FULL ||
       current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_MERGE

app/models/repository.rb

@@ -109,6 +109,10 @@ class Repository
   end
 
   def find_commits_by_message(query, ref = nil, path = nil, limit = 1000, offset = 0)
+    unless exists? && has_visible_content? && query.present?
+      return []
+    end
+
     ref ||= root_ref
 
     args = %W(
@@ -117,9 +121,8 @@ class Repository
     )
     args = args.concat(%W(-- #{path})) if path.present?
 
-    git_log_results = Gitlab::Popen.popen(args, path_to_repo).first.lines.map(&:chomp)
-    commits = git_log_results.map { |c| commit(c) }
-    commits
+    git_log_results = Gitlab::Popen.popen(args, path_to_repo).first.lines
+    git_log_results.map { |c| commit(c.chomp) }.compact
   end
 
   def find_branch(name, fresh_repo: true)

app/services/event_create_service.rb

@@ -62,6 +62,10 @@ class EventCreateService
     create_event(project, current_user, Event::LEFT)
   end
 
+  def expired_leave_project(project, current_user)
+    create_event(project, current_user, Event::EXPIRED)
+  end
+
   def create_project(project, current_user)
     create_event(project, current_user, Event::CREATED)
   end

app/workers/project_cache_worker.rb

@@ -1,9 +1,30 @@
+# Worker for updating any project specific caches.
+#
+# This worker runs at most once every 15 minutes per project. This is to ensure
+# that multiple instances of jobs for this worker don't hammer the underlying
+# storage engine as much.
 class ProjectCacheWorker
   include Sidekiq::Worker
 
   sidekiq_options queue: :default
 
+  LEASE_TIMEOUT = 15.minutes.to_i
+
   def perform(project_id)
+    if try_obtain_lease_for(project_id)
+      Rails.logger.
+        info("Obtained ProjectCacheWorker lease for project #{project_id}")
+    else
+      Rails.logger.
+        info("Could not obtain ProjectCacheWorker lease for project #{project_id}")
+
+      return
+    end
+
+    update_caches(project_id)
+  end
+
+  def update_caches(project_id)
     project = Project.find(project_id)
 
     return unless project.repository.exists?
@@ -15,4 +36,10 @@ class ProjectCacheWorker
       project.repository.build_cache
     end
   end
+
+  def try_obtain_lease_for(project_id)
+    Gitlab::ExclusiveLease.
+      new("project_cache_worker:#{project_id}", timeout: LEASE_TIMEOUT).
+      try_obtain
+  end
 end

db/migrate/20161019213545_generate_project_feature_for_projects.rb

@@ -0,0 +1,28 @@
+class GenerateProjectFeatureForProjects < ActiveRecord::Migration
+  DOWNTIME = true
+
+  DOWNTIME_REASON = <<-HEREDOC
+    Application was eager loading project_feature for all projects generating an extra query
+    everytime a project was fetched. We removed that behavior to avoid the extra query, this migration
+    makes sure all projects have a project_feature record associated.
+  HEREDOC
+
+  def up
+    # Generate enabled values for each project feature 20, 20, 20, 20, 20
+    # All features are enabled by default
+    enabled_values = [ProjectFeature::ENABLED] * 5
+
+    execute <<-EOF.strip_heredoc
+      INSERT INTO project_features
+      (project_id, merge_requests_access_level, builds_access_level,
+      issues_access_level, snippets_access_level, wiki_access_level)
+      (SELECT projects.id, #{enabled_values.join(',')} FROM projects LEFT OUTER JOIN project_features
+      ON project_features.project_id = projects.id
+      WHERE project_features.id IS NULL)
+    EOF
+  end
+
+  def down
+    "Not needed"
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20161018024550) do
+ActiveRecord::Schema.define(version: 20161019213545) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"

doc/api/projects.md

@@ -1333,8 +1333,6 @@ Parameters:
 
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
-| `query` (required) - A string contained in the project name
-| `per_page` (optional) - number of projects to return per page
-| `page` (optional) - the page to retrieve
-| `order_by` (optional) - Return requests ordered by `id`, `name`, `created_at` or `last_activity_at` fields
+| `query` | string | yes | A string contained in the project name |
+| `order_by` | string | no | Return requests ordered by `id`, `name`, `created_at` or `last_activity_at` fields |
 | `sort` | string | no | Return requests sorted in `asc` or `desc` order |

features/dashboard/dashboard.feature

@@ -31,19 +31,6 @@ Feature: Dashboard
     And I click "Create Merge Request" link
     Then I see prefilled new Merge Request page
 
-  @javascript
-  Scenario: I should see User joined Project event
-    Given user with name "John Doe" joined project "Shop"
-    When I visit dashboard activity page
-    Then I should see "John Doe joined project Shop" event
-
-  @javascript
-  Scenario: I should see User left Project event
-    Given user with name "John Doe" joined project "Shop"
-    And user with name "John Doe" left project "Shop"
-    When I visit dashboard activity page
-    Then I should see "John Doe left project Shop" event
-
   @javascript
   Scenario: Sorting Issues
     Given I visit dashboard issues page

features/steps/dashboard/dashboard.rb

@@ -33,33 +33,6 @@ class Spinach::Features::Dashboard < Spinach::FeatureSteps
     expect(find("input#merge_request_target_branch").value).to eq "master"
   end
 
-  step 'user with name "John Doe" joined project "Shop"' do
-    user = create(:user, { name: "John Doe" })
-    project.team << [user, :master]
-    Event.create(
-      project: project,
-      author_id: user.id,
-      action: Event::JOINED
-    )
-  end
-
-  step 'I should see "John Doe joined project Shop" event' do
-    expect(page).to have_content "John Doe joined project #{project.name_with_namespace}"
-  end
-
-  step 'user with name "John Doe" left project "Shop"' do
-    user = User.find_by(name: "John Doe")
-    Event.create(
-      project: project,
-      author_id: user.id,
-      action: Event::LEFT
-    )
-  end
-
-  step 'I should see "John Doe left project Shop" event' do
-    expect(page).to have_content "John Doe left project #{project.name_with_namespace}"
-  end
-
   step 'I have group with projects' do
     @group   = create(:group)
     @project = create(:project, namespace: @group)

lib/event_filter.rb

@@ -45,9 +45,16 @@ class EventFilter
     when EventFilter.comments
       actions = [Event::COMMENTED]
     when EventFilter.team
-      actions = [Event::JOINED, Event::LEFT]
+      actions = [Event::JOINED, Event::LEFT, Event::EXPIRED]
     when EventFilter.all
-      actions = [Event::PUSHED, Event::MERGED, Event::COMMENTED, Event::JOINED, Event::LEFT]
+      actions = [
+        Event::PUSHED,
+        Event::MERGED,
+        Event::COMMENTED,
+        Event::JOINED,
+        Event::LEFT,
+        Event::EXPIRED
+      ]
     end
 
     events.where(action: actions)

lib/gitlab/project_search_results.rb

@@ -73,11 +73,7 @@ module Gitlab
     end
 
     def commits
-      if project.empty_repo? || query.blank?
-        []
-      else
-        project.repository.find_commits_by_message(query).compact
-      end
+      project.repository.find_commits_by_message(query)
     end
 
     def project_ids_relation

spec/features/dashboard/project_member_activity_index_spec.rb

@@ -0,0 +1,41 @@
+require 'spec_helper'
+
+feature 'Project member activity', feature: true, js: true do
+  include WaitForAjax
+
+  let(:user)            { create(:user) }
+  let(:project)         { create(:empty_project, :public, name: 'x', namespace: user.namespace) }
+
+  before do
+    project.team << [user, :master]
+  end
+
+  def visit_activities_and_wait_with_event(event_type)
+    Event.create(project: project, author_id: user.id, action: event_type)
+    visit activity_namespace_project_path(project.namespace.path, project.path)
+    wait_for_ajax
+  end
+
+  subject { page.find(".event-title").text }
+
+  context 'when a user joins the project' do
+    before { visit_activities_and_wait_with_event(Event::JOINED) }
+
+    it { is_expected.to eq("#{user.name} joined project") }
+  end
+
+  context 'when a user leaves the project' do
+    before { visit_activities_and_wait_with_event(Event::LEFT) }
+
+    it { is_expected.to eq("#{user.name} left project") }
+  end
+
+  context 'when a users membership expires for the project' do
+    before { visit_activities_and_wait_with_event(Event::EXPIRED) }
+
+    it "presents the correct message" do
+      message = "#{user.name} removed due to membership expiration from project"
+      is_expected.to eq(message)
+    end
+  end
+end

spec/models/concerns/expirable_spec.rb

@@ -0,0 +1,31 @@
+require 'spec_helper'
+
+describe Expirable do
+  describe 'ProjectMember' do
+    let(:no_expire) { create(:project_member) }
+    let(:expire_later) { create(:project_member, expires_at: Time.current + 6.days) }
+    let(:expired) { create(:project_member, expires_at: Time.current - 6.days) }
+
+    describe '.expired' do
+      it { expect(ProjectMember.expired).to match_array([expired]) }
+    end
+
+    describe '#expired?' do
+      it { expect(no_expire.expired?).to eq(false) }
+      it { expect(expire_later.expired?).to eq(false) }
+      it { expect(expired.expired?).to eq(true) }
+    end
+
+    describe '#expires?' do
+      it { expect(no_expire.expires?).to eq(false) }
+      it { expect(expire_later.expires?).to eq(true) }
+      it { expect(expired.expires?).to eq(true) }
+    end
+
+    describe '#expires_soon?' do
+      it { expect(no_expire.expires_soon?).to eq(false) }
+      it { expect(expire_later.expires_soon?).to eq(true) }
+      it { expect(expired.expires_soon?).to eq(true) }
+    end
+  end
+end

spec/models/event_spec.rb

@@ -40,6 +40,33 @@ describe Event, models: true do
     end
   end
 
+  describe '#membership_changed?' do
+    context "created" do
+      subject { build(:event, action: Event::CREATED).membership_changed? }
+      it { is_expected.to be_falsey }
+    end
+
+    context "updated" do
+      subject { build(:event, action: Event::UPDATED).membership_changed? }
+      it { is_expected.to be_falsey }
+    end
+
+    context "expired" do
+      subject { build(:event, action: Event::EXPIRED).membership_changed? }
+      it { is_expected.to be_truthy }
+    end
+
+    context "left" do
+      subject { build(:event, action: Event::LEFT).membership_changed? }
+      it { is_expected.to be_truthy }
+    end
+
+    context "joined" do
+      subject { build(:event, action: Event::JOINED).membership_changed? }
+      it { is_expected.to be_truthy }
+    end
+  end
+
   describe '#note?' do
     subject { Event.new(project: target.project, target: target) }
 

spec/models/members/project_member_spec.rb

@@ -54,6 +54,17 @@ describe ProjectMember, models: true do
       master_todos
     end
 
+    it "creates an expired event when left due to expiry" do
+      expired = create(:project_member, project: project, expires_at: Time.now - 6.days)
+      expired.destroy
+      expect(Event.first.action).to eq(Event::EXPIRED)
+    end
+
+    it "creates a left event when left due to leave" do
+      master.destroy
+      expect(Event.first.action).to eq(Event::LEFT)
+    end
+
     it "destroys itself and delete associated todos" do
       expect(owner.user.todos.size).to eq(2)
       expect(master.user.todos.size).to eq(3)

spec/models/project_spec.rb

@@ -67,6 +67,14 @@ describe Project, models: true do
     it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
     it { is_expected.to have_many(:forks).through(:forked_project_links) }
 
+    context 'after create' do
+      it "creates project feature" do
+        project = FactoryGirl.build(:project)
+
+        expect { project.save }.to change{ project.project_feature.present? }.from(false).to(true)
+      end
+    end
+
     describe '#members & #requesters' do
       let(:project) { create(:project, :public) }
       let(:requester) { create(:user) }
@@ -531,9 +539,9 @@ describe Project, models: true do
   end
 
   describe '#has_wiki?' do
-    let(:no_wiki_project) { build(:project, wiki_enabled: false, has_external_wiki: false) }
-    let(:wiki_enabled_project) { build(:project) }
-    let(:external_wiki_project) { build(:project, has_external_wiki: true) }
+    let(:no_wiki_project)       { create(:project, wiki_access_level: ProjectFeature::DISABLED, has_external_wiki: false) }
+    let(:wiki_enabled_project)  { create(:project) }
+    let(:external_wiki_project) { create(:project, has_external_wiki: true) }
 
     it 'returns true if project is wiki enabled or has external wiki' do
       expect(wiki_enabled_project).to have_wiki

spec/requests/api/labels_spec.rb

@@ -22,8 +22,7 @@ describe API::API, api: true  do
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
       expect(json_response.size).to eq(2)
-      expect(json_response.first['name']).to eq(group_label.name)
-      expect(json_response.second['name']).to eq(label1.name)
+      expect(json_response.map { |l| l['name'] }).to match_array([group_label.name, label1.name])
     end
   end
 

spec/services/event_create_service_spec.rb

@@ -110,4 +110,23 @@ describe EventCreateService, services: true do
       end
     end
   end
+
+  describe 'Project' do
+    let(:user) { create :user }
+    let(:project) { create(:empty_project) }
+
+    describe '#join_project' do
+      subject { service.join_project(project, user) }
+
+      it { is_expected.to be_truthy }
+      it { expect { subject }.to change { Event.count }.from(0).to(1) }
+    end
+
+    describe '#expired_leave_project' do
+      subject { service.expired_leave_project(project, user) }
+
+      it { is_expected.to be_truthy }
+      it { expect { subject }.to change { Event.count }.from(0).to(1) }
+    end
+  end
 end

spec/workers/project_cache_worker_spec.rb

@@ -6,21 +6,39 @@ describe ProjectCacheWorker do
   subject { described_class.new }
 
   describe '#perform' do
-    it 'updates project cache data' do
-      expect_any_instance_of(Repository).to receive(:size)
-      expect_any_instance_of(Repository).to receive(:commit_count)
+    context 'when an exclusive lease can be obtained' do
+      before do
+        allow(subject).to receive(:try_obtain_lease_for).with(project.id).
+          and_return(true)
+      end
 
-      expect_any_instance_of(Project).to receive(:update_repository_size)
-      expect_any_instance_of(Project).to receive(:update_commit_count)
+      it 'updates project cache data' do
+        expect_any_instance_of(Repository).to receive(:size)
+        expect_any_instance_of(Repository).to receive(:commit_count)
 
-      subject.perform(project.id)
+        expect_any_instance_of(Project).to receive(:update_repository_size)
+        expect_any_instance_of(Project).to receive(:update_commit_count)
+
+        subject.perform(project.id)
+      end
+
+      it 'handles missing repository data' do
+        expect_any_instance_of(Repository).to receive(:exists?).and_return(false)
+        expect_any_instance_of(Repository).not_to receive(:size)
+
+        subject.perform(project.id)
+      end
     end
 
-    it 'handles missing repository data' do
-      expect_any_instance_of(Repository).to receive(:exists?).and_return(false)
-      expect_any_instance_of(Repository).not_to receive(:size)
+    context 'when an exclusive lease can not be obtained' do
+      it 'does nothing' do
+        allow(subject).to receive(:try_obtain_lease_for).with(project.id).
+          and_return(false)
 
-      subject.perform(project.id)
+        expect(subject).not_to receive(:update_caches)
+
+        subject.perform(project.id)
+      end
     end
   end
 end