Merge branch '36670-remove-edit-form' into 'master'

Remove edit action for issues

Closes #36670

See merge request gitlab-org/gitlab-ce!14523

app/controllers/projects/issues_controller.rb

@@ -16,7 +16,7 @@ class Projects::IssuesController < Projects::ApplicationController
   before_action :authorize_create_issue!, only: [:new, :create]
 
   # Allow modify issue
-  before_action :authorize_update_issue!, only: [:edit, :update, :move]
+  before_action :authorize_update_issue!, only: [:update, :move]
 
   # Allow create a new branch and empty WIP merge request from current issue
   before_action :authorize_create_merge_request!, only: [:create_merge_request]
@@ -63,10 +63,6 @@ class Projects::IssuesController < Projects::ApplicationController
     respond_with(@issue)
   end
 
-  def edit
-    respond_with(@issue)
-  end
-
   def show
     @noteable = @issue
     @note     = @project.notes.new(noteable: @issue)
@@ -126,10 +122,6 @@ class Projects::IssuesController < Projects::ApplicationController
     @issue = Issues::UpdateService.new(project, current_user, update_params).execute(issue)
 
     respond_to do |format|
-      format.html do
-        recaptcha_check_with_fallback { render :edit }
-      end
-
       format.json do
         render_issue_json
       end

app/views/projects/issues/edit.html.haml

-- page_title "Edit", "#{@issue.title} (#{@issue.to_reference})", "Issues"
-
-%h3.page-title
-  Edit Issue ##{@issue.iid}
-%hr
-
-= render "form"

changelogs/unreleased/36670-remove-edit-form.yml

+---
+title: Remove the ability to visit the issue edit form directly
+merge_request: 14523
+author:
+type: removed

spec/controllers/projects/issues_controller_spec.rb

@@ -207,162 +207,6 @@ describe Projects::IssuesController do
     end
   end
 
-  describe 'PUT #update' do
-    before do
-      sign_in(user)
-      project.team << [user, :developer]
-    end
-
-    it_behaves_like 'update invalid issuable', Issue
-
-    context 'changing the assignee' do
-      it 'limits the attributes exposed on the assignee' do
-        assignee = create(:user)
-        project.add_developer(assignee)
-
-        put :update,
-          namespace_id: project.namespace.to_param,
-          project_id: project,
-          id: issue.iid,
-          issue: { assignee_ids: [assignee.id] },
-          format: :json
-        body = JSON.parse(response.body)
-
-        expect(body['assignees'].first.keys)
-          .to match_array(%w(id name username avatar_url state web_url))
-      end
-    end
-
-    context 'Akismet is enabled' do
-      let(:project) { create(:project_empty_repo, :public) }
-
-      before do
-        stub_application_setting(recaptcha_enabled: true)
-        allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true)
-      end
-
-      context 'when an issue is not identified as spam' do
-        before do
-          allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false)
-          allow_any_instance_of(AkismetService).to receive(:spam?).and_return(false)
-        end
-
-        it 'normally updates the issue' do
-          expect { update_issue(title: 'Foo') }.to change { issue.reload.title }.to('Foo')
-        end
-      end
-
-      context 'when an issue is identified as spam' do
-        before do
-          allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true)
-        end
-
-        context 'when captcha is not verified' do
-          def update_spam_issue
-            update_issue(title: 'Spam Title', description: 'Spam lives here')
-          end
-
-          before do
-            allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false)
-          end
-
-          it 'rejects an issue recognized as a spam' do
-            expect(Gitlab::Recaptcha).to receive(:load_configurations!).and_return(true)
-            expect { update_spam_issue }.not_to change { issue.reload.title }
-          end
-
-          it 'rejects an issue recognized as a spam when recaptcha disabled' do
-            stub_application_setting(recaptcha_enabled: false)
-
-            expect { update_spam_issue }.not_to change { issue.reload.title }
-          end
-
-          it 'creates a spam log' do
-            update_spam_issue
-
-            spam_logs = SpamLog.all
-
-            expect(spam_logs.count).to eq(1)
-            expect(spam_logs.first.title).to eq('Spam Title')
-            expect(spam_logs.first.recaptcha_verified).to be_falsey
-          end
-
-          context 'as HTML' do
-            it 'renders verify template' do
-              update_spam_issue
-
-              expect(response).to render_template(:verify)
-            end
-          end
-
-          context 'as JSON' do
-            before do
-              update_issue({ title: 'Spam Title', description: 'Spam lives here' }, format: :json)
-            end
-
-            it 'renders json errors' do
-              expect(json_response)
-                .to eql("errors" => ["Your issue has been recognized as spam. Please, change the content or solve the reCAPTCHA to proceed."])
-            end
-
-            it 'returns 422 status' do
-              expect(response).to have_http_status(422)
-            end
-          end
-        end
-
-        context 'when captcha is verified' do
-          let(:spammy_title) { 'Whatever' }
-          let!(:spam_logs) { create_list(:spam_log, 2, user: user, title: spammy_title) }
-
-          def update_verified_issue
-            update_issue({ title: spammy_title },
-                         { spam_log_id: spam_logs.last.id,
-                           recaptcha_verification: true })
-          end
-
-          before do
-            allow_any_instance_of(described_class).to receive(:verify_recaptcha)
-              .and_return(true)
-          end
-
-          it 'redirect to issue page' do
-            update_verified_issue
-
-            expect(response)
-              .to redirect_to(project_issue_path(project, issue))
-          end
-
-          it 'accepts an issue after recaptcha is verified' do
-            expect { update_verified_issue }.to change { issue.reload.title }.to(spammy_title)
-          end
-
-          it 'marks spam log as recaptcha_verified' do
-            expect { update_verified_issue }.to change { SpamLog.last.recaptcha_verified }.from(false).to(true)
-          end
-
-          it 'does not mark spam log as recaptcha_verified when it does not belong to current_user' do
-            spam_log = create(:spam_log)
-
-            expect { update_issue(spam_log_id: spam_log.id, recaptcha_verification: true) }
-              .not_to change { SpamLog.last.recaptcha_verified }
-          end
-        end
-      end
-
-      def update_issue(issue_params = {}, additional_params = {})
-        params = {
-          namespace_id: project.namespace.to_param,
-          project_id: project,
-          id: issue.iid,
-          issue: issue_params
-        }.merge(additional_params)
-
-        put :update, params
-      end
-    end
-  end
-
   describe 'POST #move' do
     before do
       sign_in(user)
@@ -533,6 +377,146 @@ describe Projects::IssuesController do
       end
     end
 
+    describe 'PUT #update' do
+      def update_issue(issue_params: {}, additional_params: {}, id: nil)
+        id ||= issue.iid
+        params = {
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          id: id,
+          issue: { title: 'New title' }.merge(issue_params),
+          format: :json
+        }.merge(additional_params)
+
+        put :update, params
+      end
+
+      def go(id:)
+        update_issue(id: id)
+      end
+
+      before do
+        sign_in(user)
+        project.team << [user, :developer]
+      end
+
+      it_behaves_like 'restricted action', success: 200
+      it_behaves_like 'update invalid issuable', Issue
+
+      context 'changing the assignee' do
+        it 'limits the attributes exposed on the assignee' do
+          assignee = create(:user)
+          project.add_developer(assignee)
+
+          update_issue(issue_params: { assignee_ids: [assignee.id] })
+
+          body = JSON.parse(response.body)
+
+          expect(body['assignees'].first.keys)
+            .to match_array(%w(id name username avatar_url state web_url))
+        end
+      end
+
+      context 'Akismet is enabled' do
+        before do
+          project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+          stub_application_setting(recaptcha_enabled: true)
+          allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true)
+        end
+
+        context 'when an issue is not identified as spam' do
+          before do
+            allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false)
+            allow_any_instance_of(AkismetService).to receive(:spam?).and_return(false)
+          end
+
+          it 'normally updates the issue' do
+            expect { update_issue(issue_params: { title: 'Foo' }) }.to change { issue.reload.title }.to('Foo')
+          end
+        end
+
+        context 'when an issue is identified as spam' do
+          before do
+            allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true)
+          end
+
+          context 'when captcha is not verified' do
+            before do
+              allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false)
+            end
+
+            it 'rejects an issue recognized as a spam' do
+              expect { update_issue }.not_to change { issue.reload.title }
+            end
+
+            it 'rejects an issue recognized as a spam when recaptcha disabled' do
+              stub_application_setting(recaptcha_enabled: false)
+
+              expect { update_issue }.not_to change { issue.reload.title }
+            end
+
+            it 'creates a spam log' do
+              update_issue(issue_params: { title: 'Spam title' })
+
+              spam_logs = SpamLog.all
+
+              expect(spam_logs.count).to eq(1)
+              expect(spam_logs.first.title).to eq('Spam title')
+              expect(spam_logs.first.recaptcha_verified).to be_falsey
+            end
+
+            it 'renders json errors' do
+              update_issue
+
+              expect(json_response)
+                .to eql("errors" => ["Your issue has been recognized as spam. Please, change the content or solve the reCAPTCHA to proceed."])
+            end
+
+            it 'returns 422 status' do
+              update_issue
+
+              expect(response).to have_http_status(422)
+            end
+          end
+
+          context 'when captcha is verified' do
+            let(:spammy_title) { 'Whatever' }
+            let!(:spam_logs) { create_list(:spam_log, 2, user: user, title: spammy_title) }
+
+            def update_verified_issue
+              update_issue(
+                issue_params: { title: spammy_title },
+                additional_params: { spam_log_id: spam_logs.last.id, recaptcha_verification: true })
+            end
+
+            before do
+              allow_any_instance_of(described_class).to receive(:verify_recaptcha)
+                .and_return(true)
+            end
+
+            it 'returns 200 status' do
+              expect(response).to have_http_status(200)
+            end
+
+            it 'accepts an issue after recaptcha is verified' do
+              expect { update_verified_issue }.to change { issue.reload.title }.to(spammy_title)
+            end
+
+            it 'marks spam log as recaptcha_verified' do
+              expect { update_verified_issue }.to change { SpamLog.last.recaptcha_verified }.from(false).to(true)
+            end
+
+            it 'does not mark spam log as recaptcha_verified when it does not belong to current_user' do
+              spam_log = create(:spam_log)
+
+              expect { update_issue(issue_params: { spam_log_id: spam_log.id, recaptcha_verification: true }) }
+                .not_to change { SpamLog.last.recaptcha_verified }
+            end
+          end
+        end
+      end
+    end
+
     describe 'GET #show' do
       it_behaves_like 'restricted action', success: 200
 
@@ -573,29 +557,6 @@ describe Projects::IssuesController do
         end
       end
     end
-
-    describe 'GET #edit' do
-      it_behaves_like 'restricted action', success: 200
-
-      def go(id:)
-        get :edit,
-          namespace_id: project.namespace.to_param,
-          project_id: project,
-          id: id
-      end
-    end
-
-    describe 'PUT #update' do
-      it_behaves_like 'restricted action', success: 302
-
-      def go(id:)
-        put :update,
-          namespace_id: project.namespace.to_param,
-          project_id: project,
-          id: id,
-          issue: { title: 'New title' }
-      end
-    end
   end
 
   describe 'POST #create' do

spec/features/issues/form_spec.rb

@@ -218,54 +218,15 @@ describe 'New/edit issue', :js do
 
   context 'edit issue' do
     before do
-      visit edit_project_issue_path(project, issue)
-    end
-
-    it 'allows user to update issue' do
-      expect(find('input[name="issue[assignee_ids][]"]', visible: false).value).to match(user.id.to_s)
-      expect(find('input[name="issue[milestone_id]"]', visible: false).value).to match(milestone.id.to_s)
-      expect(find('a', text: 'Assign to me', visible: false)).not_to be_visible
-
-      page.within '.js-user-search' do
-        expect(page).to have_content user.name
-      end
-
-      page.within '.js-milestone-select' do
-        expect(page).to have_content milestone.title
-      end
-
-      click_button 'Labels'
-      page.within '.dropdown-menu-labels' do
-        click_link label.title
-        click_link label2.title
-      end
-      page.within '.js-label-select' do
-        expect(page).to have_content label.title
-      end
-      expect(page.all('input[name="issue[label_ids][]"]', visible: false)[1].value).to match(label.id.to_s)
-      expect(page.all('input[name="issue[label_ids][]"]', visible: false)[2].value).to match(label2.id.to_s)
-
-      click_button 'Save changes'
-
-      page.within '.issuable-sidebar' do
-        page.within '.assignee' do
-          expect(page).to have_content user.name
-        end
-
-        page.within '.milestone' do
-          expect(page).to have_content milestone.title
-        end
-
-        page.within '.labels' do
-          expect(page).to have_content label.title
-          expect(page).to have_content label2.title
-        end
+      visit project_issue_path(project, issue)
+      page.within('.content .issuable-actions') do
+        click_on 'Edit'
       end
     end
 
     it 'description has autocomplete' do
-      find('#issue_description').native.send_keys('')
-      fill_in 'issue_description', with: '@'
+      find_field('issue-description').native.send_keys('')
+      fill_in 'issue-description', with: '@'
 
       expect(page).to have_selector('.atwho-view')
     end

spec/features/issues_spec.rb

 require 'spec_helper'
 
-describe 'Issues' do
+describe 'Issues', :js do
   include DropzoneHelper
   include IssueHelpers
   include SortingHelper
@@ -24,109 +24,15 @@ describe 'Issues' do
     end
 
     before do
-      visit edit_project_issue_path(project, issue)
-      find('.js-zen-enter').click
-    end
-
-    it 'opens new issue popup' do
-      expect(page).to have_content("Issue ##{issue.iid}")
-    end
-  end
-
-  describe 'Editing issue assignee' do
-    let!(:issue) do
-      create(:issue,
-             author: user,
-             assignees: [user],
-             project: project)
-    end
-
-    it 'allows user to select unassigned', js: true do
-      visit edit_project_issue_path(project, issue)
-
-      expect(page).to have_content "Assignee #{user.name}"
-
-      first('.js-user-search').click
-      click_link 'Unassigned'
-
-      click_button 'Save changes'
-
-      page.within('.assignee') do
-        expect(page).to have_content 'No assignee - assign yourself'
-      end
-
-      expect(issue.reload.assignees).to be_empty
-    end
-  end
-
-  describe 'due date', js: true do
-    context 'on new form' do
-      before do
-        visit new_project_issue_path(project)
-      end
-
-      it 'saves with due date' do
-        date = Date.today.at_beginning_of_month
-
-        fill_in 'issue_title', with: 'bug 345'
-        fill_in 'issue_description', with: 'bug description'
-        find('#issuable-due-date').click
-
-        page.within '.pika-single' do
-          click_button date.day
-        end
-
-        expect(find('#issuable-due-date').value).to eq date.to_s
-
-        click_button 'Submit issue'
-
-        page.within '.issuable-sidebar' do
-          expect(page).to have_content date.to_s(:medium)
-        end
+      visit project_issue_path(project, issue)
+      page.within('.content .issuable-actions') do
+        find('.issuable-edit').click
       end
+      find('.issue-details .content-block .js-zen-enter').click
     end
 
-    context 'on edit form' do
-      let(:issue) { create(:issue, author: user, project: project, due_date: Date.today.at_beginning_of_month.to_s) }
-
-      before do
-        visit edit_project_issue_path(project, issue)
-      end
-
-      it 'saves with due date' do
-        date = Date.today.at_beginning_of_month
-
-        expect(find('#issuable-due-date').value).to eq date.to_s
-
-        date = date.tomorrow
-
-        fill_in 'issue_title', with: 'bug 345'
-        fill_in 'issue_description', with: 'bug description'
-        find('#issuable-due-date').click
-
-        page.within '.pika-single' do
-          click_button date.day
-        end
-
-        expect(find('#issuable-due-date').value).to eq date.to_s
-
-        click_button 'Save changes'
-
-        page.within '.issuable-sidebar' do
-          expect(page).to have_content date.to_s(:medium)
-        end
-      end
-
-      it 'warns about version conflict' do
-        issue.update(title: "New title")
-
-        fill_in 'issue_title', with: 'bug 345'
-        fill_in 'issue_description', with: 'bug description'
-
-        click_button 'Save changes'
-
-        expect(page).to have_content 'Someone edited the issue the same time you did'
-      end
+    it 'opens new issue popup' do
+      expect(page).to have_content(issue.description)
     end
   end
 

spec/features/projects/issuable_templates_spec.rb

@@ -3,6 +3,7 @@ require 'spec_helper'
 feature 'issuable templates', js: true do
   let(:user) { create(:user) }
   let(:project) { create(:project, :public, :repository) }
+  let(:issue_form_location) { '#content-body .issuable-details .detail-page-description' }
 
   before do
     project.team << [user, :master]
@@ -28,14 +29,17 @@ feature 'issuable templates', js: true do
         longtemplate_content,
         message: 'added issue template',
         branch_name: 'master')
-      visit edit_project_issue_path project, issue
-      fill_in :'issue[title]', with: 'test issue title'
+      visit project_issue_path project, issue
+      page.within('.content .issuable-actions') do
+        click_on 'Edit'
+      end
+      fill_in :'issue-title', with: 'test issue title'
     end
 
     scenario 'user selects "bug" template' do
       select_template 'bug'
       wait_for_requests
-      assert_template
+      assert_template(page_part: issue_form_location)
       save_changes
     end
 
@@ -43,30 +47,19 @@ feature 'issuable templates', js: true do
       select_template 'bug'
       wait_for_requests
       select_option 'No template'
-      assert_template('')
+      assert_template(expected_content: '', page_part: issue_form_location)
       save_changes('')
     end
 
     scenario 'user selects "bug" template, edits description and then selects "reset template"' do
       select_template 'bug'
       wait_for_requests
-      find_field('issue_description').send_keys(description_addition)
-      assert_template(template_content + description_addition)
+      find_field('issue-description').send_keys(description_addition)
+      assert_template(expected_content: template_content + description_addition, page_part: issue_form_location)
       select_option 'Reset template'
-      assert_template
+      assert_template(page_part: issue_form_location)
       save_changes
     end
-
-    it 'updates height of markdown textarea' do
-      start_height = page.evaluate_script('$(".markdown-area").outerHeight()')
-
-      select_template 'test'
-      wait_for_requests
-
-      end_height = page.evaluate_script('$(".markdown-area").outerHeight()')
-
-      expect(end_height).not_to eq(start_height)
-    end
   end
 
   context 'user creates an issue using templates, with a prior description' do
@@ -81,15 +74,18 @@ feature 'issuable templates', js: true do
         template_content,
         message: 'added issue template',
         branch_name: 'master')
-      visit edit_project_issue_path project, issue
-      fill_in :'issue[title]', with: 'test issue title'
-      fill_in :'issue[description]', with: prior_description
+      visit project_issue_path project, issue
+      page.within('.content .issuable-actions') do
+        click_on 'Edit'
+      end
+      fill_in :'issue-title', with: 'test issue title'
+      fill_in :'issue-description', with: prior_description
     end
 
     scenario 'user selects "bug" template' do
       select_template 'bug'
       wait_for_requests
-      assert_template("#{template_content}")
+      assert_template(page_part: issue_form_location)
       save_changes
     end
   end
@@ -154,8 +150,10 @@ feature 'issuable templates', js: true do
     end
   end
 
-  def assert_template(expected_content = template_content)
-    expect(find('textarea')['value']).to eq(expected_content)
+  def assert_template(expected_content: template_content, page_part: '#content-body')
+    page.within(page_part) do
+      expect(find('textarea')['value']).to eq(expected_content)
+    end
   end
 
   def save_changes(expected_content = template_content)

spec/features/security/project/internal_access_spec.rb

@@ -181,21 +181,6 @@ describe "Internal Project Access"  do
     it { is_expected.to be_denied_for(:visitor) }
   end
 
-  describe "GET /:project_path/issues/:id/edit" do
-    let(:issue) { create(:issue, project: project) }
-    subject { edit_project_issue_path(project, issue) }
-
-    it { is_expected.to be_allowed_for(:admin) }
-    it { is_expected.to be_allowed_for(:owner).of(project) }
-    it { is_expected.to be_allowed_for(:master).of(project) }
-    it { is_expected.to be_allowed_for(:developer).of(project) }
-    it { is_expected.to be_allowed_for(:reporter).of(project) }
-    it { is_expected.to be_denied_for(:guest).of(project) }
-    it { is_expected.to be_denied_for(:user) }
-    it { is_expected.to be_denied_for(:external) }
-    it { is_expected.to be_denied_for(:visitor) }
-  end
-
   describe "GET /:project_path/snippets" do
     subject { project_snippets_path(project) }
 

spec/features/security/project/private_access_spec.rb

@@ -181,21 +181,6 @@ describe "Private Project Access"  do
     it { is_expected.to be_denied_for(:visitor) }
   end
 
-  describe "GET /:project_path/issues/:id/edit" do
-    let(:issue) { create(:issue, project: project) }
-    subject { edit_project_issue_path(project, issue) }
-
-    it { is_expected.to be_allowed_for(:admin) }
-    it { is_expected.to be_allowed_for(:owner).of(project) }
-    it { is_expected.to be_allowed_for(:master).of(project) }
-    it { is_expected.to be_allowed_for(:developer).of(project) }
-    it { is_expected.to be_allowed_for(:reporter).of(project) }
-    it { is_expected.to be_denied_for(:guest).of(project) }
-    it { is_expected.to be_denied_for(:user) }
-    it { is_expected.to be_denied_for(:external) }
-    it { is_expected.to be_denied_for(:visitor) }
-  end
-
   describe "GET /:project_path/snippets" do
     subject { project_snippets_path(project) }
 

spec/features/security/project/public_access_spec.rb

@@ -394,21 +394,6 @@ describe "Public Project Access"  do
     it { is_expected.to be_allowed_for(:visitor) }
   end
 
-  describe "GET /:project_path/issues/:id/edit" do
-    let(:issue) { create(:issue, project: project) }
-    subject { edit_project_issue_path(project, issue) }
-
-    it { is_expected.to be_allowed_for(:admin) }
-    it { is_expected.to be_allowed_for(:owner).of(project) }
-    it { is_expected.to be_allowed_for(:master).of(project) }
-    it { is_expected.to be_allowed_for(:developer).of(project) }
-    it { is_expected.to be_allowed_for(:reporter).of(project) }
-    it { is_expected.to be_denied_for(:guest).of(project) }
-    it { is_expected.to be_denied_for(:user) }
-    it { is_expected.to be_denied_for(:external) }
-    it { is_expected.to be_denied_for(:visitor) }
-  end
-
   describe "GET /:project_path/snippets" do
     subject { project_snippets_path(project) }
 

spec/support/update_invalid_issuable.rb

@@ -25,11 +25,13 @@ shared_examples 'update invalid issuable' do |klass|
         .and_raise(ActiveRecord::StaleObjectError.new(issuable, :save))
     end
 
-    it 'renders edit when format is html' do
-      put :update, params
+    if klass == MergeRequest
+      it 'renders edit when format is html' do
+        put :update, params
 
-      expect(response).to render_template(:edit)
-      expect(assigns[:conflict]).to be_truthy
+        expect(response).to render_template(:edit)
+        expect(assigns[:conflict]).to be_truthy
+      end
     end
 
     it 'renders json error message when format is json' do
@@ -42,16 +44,17 @@ shared_examples 'update invalid issuable' do |klass|
     end
   end
 
-  context 'when updating an invalid issuable' do
-    before do
-      key = klass == Issue ? :issue : :merge_request
-      params[key][:title] = ""
-    end
+  if klass == MergeRequest
+    context 'when updating an invalid issuable' do
+      before do
+        params[:merge_request][:title] = ""
+      end
 
-    it 'renders edit when merge request is invalid' do
-      put :update, params
+      it 'renders edit when merge request is invalid' do
+        put :update, params
 
-      expect(response).to render_template(:edit)
+        expect(response).to render_template(:edit)
+      end
     end
   end
 end