Merge branch 'file_lock_nested_locks' into 'master'

Improve how File Lock feature works with nested items

fixes https://gitlab.com/gitlab-org/gitlab-ee/issues/705

Recent screenshot:
![Screen_Shot_2016-06-23_at_12.12.20](/uploads/14278f4a8c1057f7aee43c240e9bab81/Screen_Shot_2016-06-23_at_12.12.20.png)

Previous one 
![joxi_screenshot_1466623505450](/uploads/82fef39f919c33e7a968240382c8cb8c/joxi_screenshot_1466623505450.png)

See merge request !497

CHANGELOG-EE

@@ -2,6 +2,9 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.10.0 (unreleased)
 
+v 8.9.4 (unreleased)
+  - Improve how File Lock feature works with nested items
+
 v 8.9.3
   - Fix encrypted data backwards compatibility after upgrading attr_encrypted gem. !502
   - Fix creating MRs on forks of deleted projects. !503

app/assets/javascripts/path_locks.js.coffee

 class @PathLocks
   @init: (url, path) ->
-    $('.path-lock').on 'click',  ->
+    $('a.path-lock').on 'click',  ->
       $lockBtn = $(this)
       currentState = $lockBtn.data('state')
       toggleAction = if currentState is 'lock' then 'unlock' else 'lock'
       $.post url, {
         path: path
       }, ->
-        $lockBtn.text(gl.utils.capitalize(toggleAction))
-        $lockBtn.data('state', toggleAction)
+        Turbolinks.visit(location.href)

app/controllers/projects/refs_controller.rb

@@ -63,12 +63,12 @@ class Projects::RefsController < Projects::ApplicationController
     @logs = contents[@offset, @limit].to_a.map do |content|
       file = @path ? File.join(@path, content.name) : content.name
       last_commit = @repo.last_commit_for_path(@commit.id, file)
-      path_lock_info = show_path_locks && @project.path_lock_info(file, exact_match: true)
+      path_lock = show_path_locks && @project.find_path_lock(file)
 
       {
         file_name: content.name,
         commit: last_commit,
-        path_lock_info: path_lock_info
+        lock_label: path_lock && text_label_for_lock(path_lock, file)
       }
     end
 

app/helpers/path_locks_helper.rb

@@ -6,4 +6,13 @@ module PathLocksHelper
   def license_allows_file_locks?
     @license_allows_file_locks ||= (::License.current && ::License.current.add_on?('GitLab_FileLocks'))
   end
+
+  def text_label_for_lock(file_lock, path)
+    if file_lock.path == path
+      "Locked by #{file_lock.user.name}"
+    else
+      # Nested lock
+      "#{file_lock.user.name} has a lock on \"#{file_lock.path}\""
+    end
+  end
 end

app/helpers/tree_helper.rb

@@ -104,7 +104,7 @@ module TreeHelper
         part_path = part if part_path.empty?
 
         next if parts.count > max_links && !parts.last(2).include?(part)
-        yield(part, tree_join(@ref, part_path))
+        yield(part, tree_join(@ref, part_path), part_path)
       end
     end
   end
@@ -128,15 +128,70 @@ module TreeHelper
     return unless license_allows_file_locks?
     return if path.blank?
 
-    path_lock = project.path_lock_info(path, exact_match: true)
+    path_lock = project.find_path_lock(path, downstream: true)
 
-    # Check permissions to unlock
-    return if path_lock && !can_unlock?(path_lock)
-    # Check permissions to lock
-    return if !path_lock && !can?(current_user, :push_code, project)
+    if path_lock
+      locker = path_lock.user.name
+
+      if path_lock.exact?(path)
+        if can_unlock?(path_lock)
+          html_options[:data] = { state: :unlock }
+          tooltip = path_lock.user == current_user ? '' : "Locked by #{locker}"
+          enabled_lock_link("Unlock", tooltip, html_options)
+        else
+          disabled_lock_link("Unlock", "Locked by #{locker}. You do not have permission to unlock this", html_options)
+        end
+      elsif path_lock.upstream?(path)
+        if can_unlock?(path_lock)
+          disabled_lock_link("Unlock", "#{locker} has a lock on \"#{path_lock.path}\". Unlock that directory in order to unlock this", html_options)
+        else
+          disabled_lock_link("Unlock", "#{locker} has a lock on \"#{path_lock.path}\". You do not have permission to unlock it", html_options)
+        end
+      elsif path_lock.downstream?(path)
+        if can_unlock?(path_lock)
+          disabled_lock_link("Lock", "This directory cannot be locked while #{locker} has a lock on \"#{path_lock.path}\". Unlock this in order to proceed", html_options)
+        else
+          disabled_lock_link("Lock", "This directory cannot be locked while #{locker} has a lock on \"#{path_lock.path}\". You do not have permission to unlock it", html_options)
+        end
+      end
+    else
+      if can?(current_user, :push_code, project)
+        html_options[:data] = { state: :lock }
+        enabled_lock_link("Lock", '', html_options)
+      else
+        disabled_lock_link("Lock", "You do not have permission to lock this", html_options)
+      end
+    end
+  end
+
+  def disabled_lock_link(label, title, html_options)
+    html_options['data-toggle'] = 'tooltip'
+    html_options[:title] = title
+    html_options[:class] = "#{html_options[:class].to_s} disabled has-tooltip"
+
+    content_tag :span, label, html_options
+  end
+
+  def enabled_lock_link(label, title, html_options)
+    html_options['data-toggle'] = 'tooltip'
+    html_options[:title] = title
+    html_options[:class] = "#{html_options[:class].to_s} has-tooltip"
 
-    label = path_lock ? 'Unlock' : 'Lock'
-    html_options = html_options.merge(data: { state: label.downcase })
     link_to label, '#', html_options
   end
+
+  def render_lock_icon(path)
+    return unless license_allows_file_locks?
+    return unless @project.root_ref?(@ref)
+
+    if file_lock = @project.find_path_lock(path, exact_match: true)
+      content_tag(
+        :i,
+        nil,
+        class: "fa fa-lock prepend-left-5 append-right-5",
+        title: text_label_for_lock(file_lock, path),
+        'data-toggle' => 'tooltip'
+      )
+    end
+  end
 end

app/models/path_lock.rb

@@ -4,5 +4,38 @@ class PathLock < ActiveRecord::Base
 
   validates :project, presence: true
   validates :user, presence: true
-  validates :path, presence: true, uniqueness: { scope: [:user, :project] }
+  validates :path, presence: true, uniqueness: { scope: :project }
+  validate :path_unique_validation
+
+  def downstream?(path)
+    self.path.start_with?(path) && !exact?(path)
+  end
+
+  def upstream?(path)
+    path.start_with?(self.path) && !exact?(path)
+  end
+
+  def exact?(path)
+    self.path == path
+  end
+
+  private
+
+  # This takes into account upstream and downstream locks
+  def path_unique_validation
+    return unless path
+    return unless project
+
+    # We don't use `project.find_path_lock` as we want to avoid memoizing the finder
+    # in project instance
+    existed_lock = Gitlab::PathLocksFinder.new(project).find(path, downstream: true)
+
+    return unless existed_lock
+
+    if existed_lock.downstream?(path)
+      errors.add(:path, 'is invalid because there is downstream lock')
+    elsif existed_lock.upstream?(path)
+      errors.add(:path, 'is invalid because there is upstream lock')
+    end
+  end
 end

app/models/project.rb

@@ -1230,9 +1230,9 @@ class Project < ActiveRecord::Base
     Dir.exist?(public_pages_path)
   end
 
-  def path_lock_info(path, exact_match: false)
+  def find_path_lock(path, exact_match: false, downstream: false)
     @path_lock_finder ||= Gitlab::PathLocksFinder.new(self)
-    @path_lock_finder.get_lock_info(path, exact_match: exact_match)
+    @path_lock_finder.find(path, exact_match: exact_match, downstream: downstream)
   end
 
   def schedule_delete!(user_id, params)

app/views/projects/blob/_blob.html.haml

@@ -6,7 +6,7 @@
     %li
       = link_to namespace_project_tree_path(@project.namespace, @project, @ref) do
         = @project.path
-    - tree_breadcrumbs(@tree, 6) do |title, path|
+    - tree_breadcrumbs(@tree, 6) do |title, path, part_path|
       %li
         - if path
           - if path.end_with?(@path)
@@ -17,6 +17,7 @@
             = link_to truncate(title, length: 40), namespace_project_tree_path(@project.namespace, @project, path)
         - else
           = link_to title, '#'
+        = render_lock_icon(part_path)
 
 %ul.blob-commit-info.hidden-xs
   - blob_commit = @repository.last_commit_for_path(@commit.id, blob.path)

app/views/projects/refs/logs_tree.js.haml

 - @logs.each do |content_data|
   - file_name = content_data[:file_name]
   - commit = content_data[:commit]
-  - path_lock_info = content_data[:path_lock_info]
+  - lock_label = content_data[:lock_label]
   - next unless commit
 
   :plain
@@ -9,10 +9,10 @@
     row.find("td.tree_time_ago").html('#{escape_javascript time_ago_with_tooltip(commit.committed_date)}');
     row.find("td.tree_commit").html('#{escape_javascript render("projects/tree/tree_commit_column", commit: commit)}');
 
-  - if path_lock_info
+  - if lock_label
     :plain
       var label = $("<span>")
-        .attr('title', 'Locked by #{escape_javascript path_lock_info.user.name}')
+        .attr('title', '#{escape_javascript lock_label}')
         .attr('data-toggle', 'tooltip')
         .addClass('fa fa-lock prepend-left-5');
       row.find('td.tree-item-file-name').append(label);

app/views/projects/tree/_tree_header.html.haml

@@ -5,13 +5,15 @@
   %li
     = link_to namespace_project_tree_path(@project.namespace, @project, @ref) do
       = @project.path
-  - tree_breadcrumbs(tree, 6) do |title, path|
+  - tree_breadcrumbs(tree, 6) do |title, path, part_path|
     %li
+      = render_lock_icon(part_path)
       - if path
         = link_to truncate(title, length: 40), namespace_project_tree_path(@project.namespace, @project, path)
       - else
         = link_to title, '#'
 
+
   - if current_user
     %li
       - if !on_top_of_branch?

lib/gitlab/git_access.rb

@@ -216,7 +216,7 @@ module Gitlab
         commit.diffs.each do |diff|
           path = diff.new_path || diff.old_path
 
-          lock_info = project.path_lock_info(path)
+          lock_info = project.find_path_lock(path)
 
           if lock_info && lock_info.user != user
             return build_status_object(false, "The path '#{lock_info.path}' is locked by #{lock_info.user.name}")

lib/gitlab/path_locks_finder.rb

@@ -4,25 +4,30 @@
 # tokens for every requested paths and check every token whether it exist in path locks table or not.
 # So for 'app/models/user.rb' path we would need to search next paths:
 # 'app', 'app/models' and 'app/models/user.rb'
-# This class also implements a memoization for common paths like 'app' 'app/models', 'vendor', etc.
+#
+# We also need to find downstream locks. As an example for "app" path we would need to
+# return "app/models/user.rb" lock.
 
 class Gitlab::PathLocksFinder
+  include ActiveRecord::Sanitization::ClassMethods
+
   def initialize(project)
     @project = project
     @non_locked_paths = []
   end
 
-  def get_lock_info(path, exact_match: false)
+  def find(path, exact_match: false, downstream: false)
     if exact_match
-      return find_lock(path)
+      find_by_token(path)
     else
+      # Search upstream locks
       tokenize(path).each do |token|
-        if lock = find_lock(token)
+        if lock = find_by_token(token)
           return lock
         end
       end
 
-      false
+      find_downstream(path) if downstream
     end
   end
 
@@ -42,7 +47,7 @@ class Gitlab::PathLocksFinder
     tokens
   end
 
-  def find_lock(token)
+  def find_by_token(token)
     if @non_locked_paths.include?(token)
       return false
     end
@@ -55,4 +60,8 @@ class Gitlab::PathLocksFinder
 
     lock
   end
+
+  def find_downstream(path)
+    @project.path_locks.find_by("path LIKE ?", "#{sanitize_sql_like(path)}%")
+  end
 end

spec/helpers/path_locks_helper_spec.rb

+require 'spec_helper'
+
+describe PathLocksHelper do
+  describe '#text_label_for_lock' do
+    it "return correct string for non-nested locks" do
+      user = create :user, name: 'John'
+      path_lock = create :path_lock, path: 'app', user: user
+      expect(text_label_for_lock(path_lock, 'app')).to eq('Locked by John')
+    end
+
+    it "return correct string for nested locks" do
+      user = create :user, name: 'John'
+      path_lock = create :path_lock, path: 'app', user: user
+      expect(text_label_for_lock(path_lock, 'app/models')).to eq('John has a lock on "app"')
+    end
+  end
+end

spec/helpers/tree_helper_spec.rb

@@ -27,22 +27,67 @@ describe TreeHelper do
   end
 
   describe '#lock_file_link' do
-    let(:path_lock) { create :path_lock }
+    let!(:path_lock) { create :path_lock, path: 'app/models' }
     let(:path) { path_lock.path }
     let(:user) { path_lock.user }
     let(:project) { path_lock.project }
 
-    it "renders unlock link" do
+    before do
       allow(helper).to receive(:can?).and_return(true)
+      allow(helper).to receive(:current_user).and_return(user)
       allow(helper).to receive(:license_allows_file_locks?).and_return(true)
+
+      project.reload
+    end
+
+    context "there is no locks" do
+      it "returns Lock with no toltip" do
+        expect(helper.lock_file_link(project, '.gitignore')).to match('Lock')
+      end
+
+      it "returns Lock button with tooltip" do
+        allow(helper).to receive(:can?).and_return(false)
+        expect(helper.lock_file_link(project, '.gitignore')).to match('You do not have permission to lock this')
+      end
+    end
+
+    context "exact lock" do
+      it "returns Unlock with no toltip" do
         expect(helper.lock_file_link(project, path)).to match('Unlock')
       end
 
-    it "renders lock link" do
-      allow(helper).to receive(:can?).and_return(true)
-      allow(helper).to receive(:current_user).and_return(user)
-      allow(helper).to receive(:license_allows_file_locks?).and_return(true)
-      expect(helper.lock_file_link(project, 'app/controller')).to match('Lock')
+      it "returns Lock button with tooltip" do
+        allow(helper).to receive(:can?).and_return(false)
+        expect(helper.lock_file_link(project, path)).to match('Unlock')
+        expect(helper.lock_file_link(project, path)).to match("Locked by #{user.name}. You do not have permission to unlock this.")
+      end
+    end
+
+    context "upstream lock" do
+      let(:requested_path) { 'app/models/user.rb' }
+
+      it "returns Lock with no toltip" do
+        expect(helper.lock_file_link(project, requested_path)).to match('Unlock')
+        expect(helper.lock_file_link(project, requested_path)).to match(html_escape("#{user.name} has a lock on \"app/models\". Unlock that directory in order to unlock this"))
+      end
+
+      it "returns Lock button with tooltip" do
+        allow(helper).to receive(:can?).and_return(false)
+        expect(helper.lock_file_link(project, requested_path)).to match('Unlock')
+        expect(helper.lock_file_link(project, requested_path)).to match(html_escape("#{user.name} has a lock on \"app/models\". You do not have permission to unlock it"))
+      end
+    end
+
+    context "downstream lock" do
+      it "returns Lock with no toltip" do
+        expect(helper.lock_file_link(project, 'app')).to match(html_escape("This directory cannot be locked while #{user.name} has a lock on \"app/models\". Unlock this in order to proceed"))
+      end
+
+      it "returns Lock button with tooltip" do
+        allow(helper).to receive(:can?).and_return(false)
+        expect(helper.lock_file_link(project, 'app')).to match('Lock')
+        expect(helper.lock_file_link(project, 'app')).to match(html_escape("This directory cannot be locked while #{user.name} has a lock on \"app/models\". You do not have permission to unlock it"))
+      end
     end
   end
 end

spec/lib/gitlab/path_locks_finder_spec.rb

@@ -9,9 +9,9 @@ describe Gitlab::PathLocksFinder, lib: true do
     lock1 = create :path_lock, project: project, path: 'app'
     lock2 = create :path_lock, project: project, path: 'lib/gitlab/repo.rb'
 
-    expect(finder.get_lock_info('app')).to eq(lock1)
-    expect(finder.get_lock_info('app/models/project.rb')).to eq(lock1)
-    expect(finder.get_lock_info('lib')).to be_falsey
-    expect(finder.get_lock_info('lib/gitlab/repo.rb')).to eq(lock2)
+    expect(finder.find('app')).to eq(lock1)
+    expect(finder.find('app/models/project.rb')).to eq(lock1)
+    expect(finder.find('lib')).to be_falsey
+    expect(finder.find('lib/gitlab/repo.rb')).to eq(lock2)
   end
 end

spec/models/path_lock_spec.rb

 require 'spec_helper'
 
 describe PathLock, models: true do
-  let(:path_lock) { create(:path_lock) }
+  let!(:path_lock) { create(:path_lock, path: 'app/models') }
+  let(:project) { path_lock.project }
 
+  context 'Relations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:user) }
+  end
 
+  context 'Validations' do
     it { is_expected.to validate_presence_of(:user) }
     it { is_expected.to validate_presence_of(:project) }
     it { is_expected.to validate_presence_of(:path) }
-  it { is_expected.to validate_uniqueness_of(:path).scoped_to(:project_id, :user_id) }
+    it { is_expected.to validate_uniqueness_of(:path).scoped_to(:project_id) }
+
+    describe '#path_unique_validation' do
+      it "is not valid because of upstream lock" do
+        path_lock = build :path_lock, path: 'app/models/user.rb', project: project
+        expect(path_lock.valid?).to be_falsey
+        expect(path_lock.errors[:path].first).to match("upstream lock")
+      end
+
+      it "is not valid because of downstream lock" do
+        path_lock = build :path_lock, path: 'app', project: project
+        expect(path_lock.valid?).to be_falsey
+        expect(path_lock.errors[:path].first).to match("downstream lock")
+      end
+    end
+  end
+
+  describe 'downstream?' do
+    it "returns true" do
+      expect(path_lock.downstream?("app")).to be_truthy
+    end
+
+    it "returns false" do
+      expect(path_lock.downstream?("app/models")).to be_falsey
+    end
+
+    it "returns false" do
+      expect(path_lock.downstream?("app/models/user.rb")).to be_falsey
+    end
+  end
+
+  describe 'upstream?' do
+    it "returns true" do
+      expect(path_lock.upstream?("app/models/user.rb")).to be_truthy
+    end
+
+    it "returns false" do
+      expect(path_lock.upstream?("app/models")).to be_falsey
+    end
+
+    it "returns false" do
+      expect(path_lock.upstream?("app")).to be_falsey
+    end
+  end
+
+  describe 'exact?' do
+    it "returns true" do
+      expect(path_lock.exact?("app/models")).to be_truthy
+    end
+
+    it "returns false" do
+      expect(path_lock.exact?("app")).to be_falsey
+    end
+  end
 end

spec/models/project_spec.rb

@@ -1118,17 +1118,17 @@ describe Project, models: true do
     end
   end
 
-  describe '#path_lock_info' do
+  describe '#find_path_lock' do
     let(:project) { create :empty_project }
     let(:path_lock) { create :path_lock, project: project }
     let(:path) { path_lock.path }
 
     it 'returns path_lock' do
-      expect(project.path_lock_info(path)).to eq(path_lock)
+      expect(project.find_path_lock(path)).to eq(path_lock)
     end
 
     it 'returns nil' do
-      expect(project.path_lock_info('app/controllers')).to be_falsey
+      expect(project.find_path_lock('app/controllers')).to be_falsey
     end
 
   end