Suggest forking when unable to edit directly (edit url)28899-linking-to-edit-file

See https://gitlab.com/gitlab-org/gitlab-ce/issues/28899

14 files changed, 311 insertions, 58 deletions

diff --git a/app/assets/javascripts/blob/blob_fork_suggestion.js b/app/assets/javascripts/blob/blob_fork_suggestion.js
new file mode 100644
index 00000000000..aa9a4e1c99a
--- /dev/null
+++ b/app/assets/javascripts/blob/blob_fork_suggestion.js
@@ -0,0 +1,15 @@
+function BlobForkSuggestion(openButton, cancelButton, suggestionSection) {
+  if (openButton) {
+    openButton.addEventListener('click', () => {
+      suggestionSection.classList.remove('hidden');
+    });
+  }
+
+  if (cancelButton) {
+    cancelButton.addEventListener('click', () => {
+      suggestionSection.classList.add('hidden');
+    });
+  }
+}
+
+export default BlobForkSuggestion;
diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js
index 9c7acc903d1..307c6a306d1 100644
--- a/app/assets/javascripts/dispatcher.js
+++ b/app/assets/javascripts/dispatcher.js
@@ -43,6 +43,7 @@ import GroupsList from './groups_list';
 import ProjectsList from './projects_list';
 import MiniPipelineGraph from './mini_pipeline_graph_dropdown';
 import BlobLinePermalinkUpdater from './blob/blob_line_permalink_updater';
+import BlobForkSuggestion from './blob/blob_fork_suggestion';
 import UserCallout from './user_callout';
 
 const ShortcutsBlob = require('./shortcuts_blob');
@@ -86,6 +87,12 @@ const ShortcutsBlob = require('./shortcuts_blob');
           skipResetBindings: true,
           fileBlobPermalinkUrl,
         });
+
+        new BlobForkSuggestion(
+          document.querySelector('.js-edit-blob-link-fork-toggler'),
+          document.querySelector('.js-cancel-fork-suggestion'),
+          document.querySelector('.js-file-fork-suggestion-section'),
+        );
       }
 
       switch (page) {
diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss
index ddea1cf540b..a5a8522739e 100644
--- a/app/assets/stylesheets/framework/files.scss
+++ b/app/assets/stylesheets/framework/files.scss
@@ -281,3 +281,16 @@ span.idiff {
     display: none;
   }
 }
+
+.file-fork-suggestion {
+  display: flex;
+  align-items: center;
+  justify-content: flex-end;
+  background-color: $gray-light;
+  border-bottom: 1px solid $border-color;
+  padding: 5px $gl-padding;
+}
+
+.file-fork-suggestion-note {
+  margin-right: 1.5em;
+}
diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb
index 7927b785601..73706bf8dae 100644
--- a/app/controllers/projects/blob_controller.rb
+++ b/app/controllers/projects/blob_controller.rb
@@ -11,7 +11,7 @@ class Projects::BlobController < Projects::ApplicationController
 
   before_action :require_non_empty_project, except: [:new, :create]
   before_action :authorize_download_code!
-  before_action :authorize_edit_tree!, only: [:new, :create, :edit, :update, :destroy]
+  before_action :authorize_edit_tree!, only: [:new, :create, :update, :destroy]
   before_action :assign_blob_vars
   before_action :commit, except: [:new, :create]
   before_action :blob, except: [:new, :create]
@@ -39,7 +39,11 @@ class Projects::BlobController < Projects::ApplicationController
   end
 
   def edit
-    blob.load_all_data!(@repository)
+    if can_collaborate_with_project?
+      blob.load_all_data!(@repository)
+    else
+      redirect_to action: 'show'
+    end
   end
 
   def update
diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb
index 8631bc54509..91d6d1852cf 100644
--- a/app/helpers/blob_helper.rb
+++ b/app/helpers/blob_helper.rb
@@ -8,31 +8,36 @@ module BlobHelper
     %w(credits changelog news copying copyright license authors)
   end
 
-  def edit_blob_link(project = @project, ref = @ref, path = @path, options = {})
-    return unless current_user
+  def edit_path(project = @project, ref = @ref, path = @path, options = {})
+    namespace_project_edit_blob_path(project.namespace, project,
+                                     tree_join(ref, path),
+                                     options[:link_opts])
+  end
 
+  def fork_path(project = @project, ref = @ref, path = @path, options = {})
+    continue_params = {
+      to: edit_path,
+      notice: edit_in_new_fork_notice,
+      notice_now: edit_in_new_fork_notice_now
+    }
+    namespace_project_forks_path(project.namespace, project, namespace_key: current_user.namespace.id, continue: continue_params)
+  end
+
+  def edit_blob_link(project = @project, ref = @ref, path = @path, options = {})
     blob = options.delete(:blob)
     blob ||= project.repository.blob_at(ref, path) rescue nil
 
     return unless blob
 
-    edit_path = namespace_project_edit_blob_path(project.namespace, project,
-                                     tree_join(ref, path),
-                                     options[:link_opts])
+    common_classes = "btn js-edit-blob #{options[:extra_class]}"
 
     if !on_top_of_branch?(project, ref)
-      button_tag "Edit", class: "btn disabled has-tooltip", title: "You can only edit files when you are on a branch", data: { container: 'body' }
-    elsif can_edit_blob?(blob, project, ref)
-      link_to "Edit", edit_path, class: 'btn btn-sm'
-    elsif can?(current_user, :fork_project, project)
-      continue_params = {
-        to:     edit_path,
-        notice: edit_in_new_fork_notice,
-        notice_now: edit_in_new_fork_notice_now
-      }
-      fork_path = namespace_project_forks_path(project.namespace, project, namespace_key: current_user.namespace.id, continue: continue_params)
-
-      link_to "Edit", fork_path, class: 'btn', method: :post
+      button_tag 'Edit', class: "#{common_classes} disabled has-tooltip", title: "You can only edit files when you are on a branch", data: { container: 'body' }
+    # This condition applies to anonymous or users who can edit directly
+    elsif !current_user || (current_user && can_edit_blob?(blob, project, ref))
+      link_to 'Edit', edit_path(project, ref, path, options), class: "#{common_classes} btn-sm"
+    elsif current_user && can?(current_user, :fork_project, project)
+      button_tag 'Edit', class: "#{common_classes} js-edit-blob-link-fork-toggler"
     end
   end
 
diff --git a/app/views/projects/blob/_blob.html.haml b/app/views/projects/blob/_blob.html.haml
index 2b2ee6ed987..aa9b852035e 100644
--- a/app/views/projects/blob/_blob.html.haml
+++ b/app/views/projects/blob/_blob.html.haml
@@ -25,4 +25,11 @@
 #blob-content-holder.blob-content-holder
   %article.file-holder
     = render "projects/blob/header", blob: blob
+    - if current_user
+      .js-file-fork-suggestion-section.file-fork-suggestion.hidden
+        %span.file-fork-suggestion-note
+          You don't have permission to edit this file. Try forking this project to edit the file.
+        = link_to 'Fork', fork_path, method: :post, class: 'btn btn-grouped btn-inverted btn-new'
+        %button.js-cancel-fork-suggestion.btn.btn-grouped{ type: 'button' }
+          Cancel
     = render blob.to_partial_path(@project), blob: blob
diff --git a/app/views/projects/blob/_header.html.haml b/app/views/projects/blob/_header.html.haml
index deeeae3d64a..6000e057ec4 100644
--- a/app/views/projects/blob/_header.html.haml
+++ b/app/views/projects/blob/_header.html.haml
@@ -32,8 +32,8 @@
       = link_to 'Permalink', namespace_project_blob_path(@project.namespace, @project,
           tree_join(@commit.sha, @path)), class: 'btn btn-sm js-data-file-blob-permalink-url'
 
-    - if current_user
-      .btn-group{ role: "group" }<
-        = edit_blob_link if blob_text_viewable?(blob)
+    .btn-group{ role: "group" }<
+      = edit_blob_link if blob_text_viewable?(blob)
+      - if current_user
         = replace_blob_link
         = delete_blob_link
diff --git a/changelogs/unreleased/28899-linking-to-edit-file.yml b/changelogs/unreleased/28899-linking-to-edit-file.yml
new file mode 100644
index 00000000000..a9f5410693b
--- /dev/null
+++ b/changelogs/unreleased/28899-linking-to-edit-file.yml
@@ -0,0 +1,5 @@
+---
+title: Linking to blob edit page handles anonymous users and users without enough permissions
+  to edit directly
+merge_request:
+author:
diff --git a/features/project/source/browse_files.feature b/features/project/source/browse_files.feature
index d4b91fec6e8..5ede935c562 100644
--- a/features/project/source/browse_files.feature
+++ b/features/project/source/browse_files.feature
@@ -158,6 +158,8 @@ Feature: Project Source Browse Files
     Given I don't have write access
     And I click on ".gitignore" file in repo
     And I click button "Edit"
+    Then I should see a Fork/Cancel combo
+    And I click button "Fork"
     Then I should see a notice about a new fork having been created
     And I can edit code
 
@@ -180,6 +182,8 @@ Feature: Project Source Browse Files
     Given I don't have write access
     And I click on ".gitignore" file in repo
     And I click button "Edit"
+    Then I should see a Fork/Cancel combo
+    And I click button "Fork"
     And I edit code
     And I fill the commit message
     And I click on "Commit Changes"
diff --git a/features/steps/project/source/browse_files.rb b/features/steps/project/source/browse_files.rb
index 5c47eaf0279..29f628763db 100644
--- a/features/steps/project/source/browse_files.rb
+++ b/features/steps/project/source/browse_files.rb
@@ -56,13 +56,17 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
   end
 
   step 'I click button "Edit"' do
-    click_link 'Edit'
+    find('.js-edit-blob').click
   end
 
   step 'I cannot see the edit button' do
     expect(page).not_to have_link 'edit'
   end
 
+  step 'I click button "Fork"' do
+    click_link 'Fork'
+  end
+
   step 'I can edit code' do
     set_new_content
     expect(evaluate_script('ace.edit("editor").getValue()')).to eq new_gitignore_content
@@ -366,6 +370,12 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
     end
   end
 
+  step 'I should see a Fork/Cancel combo' do
+    expect(page).to have_link 'Fork'
+    expect(page).to have_button 'Cancel'
+    expect(page).to have_content 'You don\'t have permission to edit this file. Try forking this project to edit the file.'
+  end
+
   step 'I should see a notice about a new fork having been created' do
     expect(page).to have_content "You're not allowed to make changes to this project directly. A fork of this project has been created that you can make changes in, so you can submit a merge request."
   end
diff --git a/spec/controllers/projects/blob_controller_spec.rb b/spec/controllers/projects/blob_controller_spec.rb
index ec36a64b415..3e9f272a0d8 100644
--- a/spec/controllers/projects/blob_controller_spec.rb
+++ b/spec/controllers/projects/blob_controller_spec.rb
@@ -2,15 +2,10 @@ require 'rails_helper'
 
 describe Projects::BlobController do
   let(:project) { create(:project, :public, :repository) }
-  let(:user)    { create(:user) }
-
-  before do
-    project.team << [user, :master]
-
-    sign_in(user)
-  end
 
   describe 'GET diff' do
+    let(:user) { create(:user) }
+
     render_views
 
     def do_get(opts = {})
@@ -20,6 +15,12 @@ describe Projects::BlobController do
       get :diff, params.merge(opts)
     end
 
+    before do
+      project.team << [user, :master]
+
+      sign_in(user)
+    end
+
     context 'when essential params are missing' do
       it 'renders nothing' do
         do_get
@@ -37,7 +38,69 @@ describe Projects::BlobController do
     end
   end
 
+  describe 'GET edit' do
+    let(:default_params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        id: 'master/CHANGELOG'
+      }
+    end
+
+    context 'anonymous' do
+      before do
+        get :edit, default_params
+      end
+
+      it 'redirects to sign in and returns' do
+        expect(response).to redirect_to(new_user_session_path)
+      end
+    end
+
+    context 'as guest' do
+      let(:guest) { create(:user) }
+
+      before do
+        sign_in(guest)
+        get :edit, default_params
+      end
+
+      it 'redirects to blob show' do
+        expect(response).to redirect_to(namespace_project_blob_path(project.namespace, project, 'master/CHANGELOG'))
+      end
+    end
+
+    context 'as developer' do
+      let(:developer) { create(:user) }
+
+      before do
+        project.team << [developer, :developer]
+        sign_in(developer)
+        get :edit, default_params
+      end
+
+      it 'redirects to blob show' do
+        expect(response).to have_http_status(200)
+      end
+    end
+
+    context 'as master' do
+      let(:master) { create(:user) }
+
+      before do
+        project.team << [master, :master]
+        sign_in(master)
+        get :edit, default_params
+      end
+
+      it 'redirects to blob show' do
+        expect(response).to have_http_status(200)
+      end
+    end
+  end
+
   describe 'PUT update' do
+    let(:user) { create(:user) }
     let(:default_params) do
       {
         namespace_id: project.namespace,
@@ -53,6 +116,12 @@ describe Projects::BlobController do
       namespace_project_blob_path(project.namespace, project, 'master/CHANGELOG')
     end
 
+    before do
+      project.team << [user, :master]
+
+      sign_in(user)
+    end
+
     it 'redirects to blob' do
       put :update, default_params
 
diff --git a/spec/features/projects/blobs/blob_show_spec.rb b/spec/features/projects/blobs/blob_show_spec.rb
new file mode 100644
index 00000000000..01cd268ffe8
--- /dev/null
+++ b/spec/features/projects/blobs/blob_show_spec.rb
@@ -0,0 +1,23 @@
+require 'spec_helper'
+
+feature 'File blob', feature: true do
+  include WaitForAjax
+  include TreeHelper
+
+  let(:project) { create(:project, :public, :test_repo) }
+  let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'master') }
+  let(:branch) { 'master' }
+  let(:file_path) { project.repository.ls_files(project.repository.root_ref)[1] }
+
+  context 'anonymous' do
+    context 'from blob file path' do
+      before do
+        visit namespace_project_blob_path(project.namespace, project, tree_join(branch, file_path))
+      end
+
+      it 'updates content' do
+        expect(page).to have_link  'Edit'
+      end
+    end
+  end
+end
diff --git a/spec/features/projects/blobs/edit_spec.rb b/spec/features/projects/blobs/edit_spec.rb
index a820d07ab3b..46f2f487e0c 100644
--- a/spec/features/projects/blobs/edit_spec.rb
+++ b/spec/features/projects/blobs/edit_spec.rb
@@ -2,44 +2,135 @@ require 'spec_helper'
 
 feature 'Editing file blob', feature: true, js: true do
   include WaitForAjax
+  include TreeHelper
 
-  given(:user) { create(:user) }
-  given(:role) { :developer }
-  given(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master') }
-  given(:project) { merge_request.target_project }
+  let(:project) { create(:project, :public, :test_repo) }
+  let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'master') }
+  let(:branch) { 'master' }
+  let(:file_path) { project.repository.ls_files(project.repository.root_ref)[1] }
 
-  background do
-    login_as(user)
-    project.team << [user, role]
-  end
-
-  def edit_and_commit
-    wait_for_ajax
-    first('.file-actions').click_link 'Edit'
-    execute_script('ace.edit("editor").setValue("class NextFeature\nend\n")')
-    click_button 'Commit Changes'
-  end
+  context 'as a developer' do
+    let(:user) { create(:user) }
+    let(:role) { :developer }
 
-  context 'from MR diff' do
     before do
-      visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request)
-      edit_and_commit
+      project.team << [user, role]
+      login_as(user)
+    end
+
+    def edit_and_commit
+      wait_for_ajax
+      find('.js-edit-blob').click
+      execute_script('ace.edit("editor").setValue("class NextFeature\nend\n")')
+      click_button 'Commit Changes'
+    end
+
+    context 'from MR diff' do
+      before do
+        visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request)
+        edit_and_commit
+      end
+
+      it 'returns me to the mr' do
+        expect(page).to have_content(merge_request.title)
+      end
     end
 
-    scenario 'returns me to the mr' do
-      expect(page).to have_content(merge_request.title)
+    context 'from blob file path' do
+      before do
+        visit namespace_project_blob_path(project.namespace, project, tree_join(branch, file_path))
+        edit_and_commit
+      end
+
+      it 'updates content' do
+        expect(page).to have_content 'successfully committed'
+        expect(page).to have_content 'NextFeature'
+      end
     end
   end
 
-  context 'from blob file path' do
-    before do
-      visit namespace_project_blob_path(project.namespace, project, '/feature/files/ruby/feature.rb')
-      edit_and_commit
+  context 'visit blob edit' do
+    context 'redirects to sign in and returns' do
+      context 'as developer' do
+        let(:user) { create(:user) }
+
+        before do
+          project.team << [user, :developer]
+          visit namespace_project_edit_blob_path(project.namespace, project, tree_join(branch, file_path))
+        end
+
+        it 'redirects to sign in and returns' do
+          expect(page).to have_current_path(new_user_session_path)
+
+          login_as(user)
+
+          expect(page).to have_current_path(namespace_project_edit_blob_path(project.namespace, project, tree_join(branch, file_path)))
+        end
+      end
+
+      context 'as guest' do
+        let(:user) { create(:user) }
+
+        before do
+          visit namespace_project_edit_blob_path(project.namespace, project, tree_join(branch, file_path))
+        end
+
+        it 'redirects to sign in and returns' do
+          expect(page).to have_current_path(new_user_session_path)
+
+          login_as(user)
+
+          expect(page).to have_current_path(namespace_project_blob_path(project.namespace, project, tree_join(branch, file_path)))
+        end
+      end
+    end
+
+    context 'as developer' do
+      let(:user) { create(:user) }
+      let(:protected_branch) { 'protected-branch' }
+
+      before do
+        project.team << [user, :developer]
+        project.repository.add_branch(user, protected_branch, 'master')
+        create(:protected_branch, project: project, name: protected_branch)
+        login_as(user)
+      end
+
+      context 'on some branch' do
+        before do
+          visit namespace_project_edit_blob_path(project.namespace, project, tree_join(branch, file_path))
+        end
+
+        it 'shows blob editor with same branch' do
+          expect(page).to have_current_path(namespace_project_edit_blob_path(project.namespace, project, tree_join(branch, file_path)))
+          expect(find('.js-target-branch .dropdown-toggle-text').text).to eq(branch)
+        end
+      end
+
+      context 'with protected branch' do
+        before do
+          visit namespace_project_edit_blob_path(project.namespace, project, tree_join(protected_branch, file_path))
+        end
+
+        it 'shows blob editor with patch branch' do
+          expect(find('.js-target-branch .dropdown-toggle-text').text).to eq('patch-1')
+        end
+      end
     end
 
-    scenario 'updates content' do
-      expect(page).to have_content 'successfully committed'
-      expect(page).to have_content 'NextFeature'
+    context 'as master' do
+      let(:user) { create(:user) }
+
+      before do
+        project.team << [user, :master]
+        login_as(user)
+        visit namespace_project_edit_blob_path(project.namespace, project, tree_join(branch, file_path))
+      end
+
+      it 'shows blob editor with same branch' do
+        expect(page).to have_current_path(namespace_project_edit_blob_path(project.namespace, project, tree_join(branch, file_path)))
+        expect(find('.js-target-branch .dropdown-toggle-text').text).to eq(branch)
+      end
     end
   end
 end
diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb
index bead7948486..508aeb7cf67 100644
--- a/spec/helpers/blob_helper_spec.rb
+++ b/spec/helpers/blob_helper_spec.rb
@@ -73,7 +73,7 @@ describe BlobHelper do
     let(:project) { create(:project, :repository, namespace: namespace) }
 
     before do
-      allow(self).to receive(:current_user).and_return(double)
+      allow(self).to receive(:current_user).and_return(nil)
       allow(self).to receive(:can_collaborate_with_project?).and_return(true)
     end