Improve code

app/views/shared/issuable/_form.html.haml

@@ -134,30 +134,29 @@
       title: 'Moving an issue will copy the discussion to a different project and close it here. All participants will be notified of the new location.' }
         = icon('question-circle')
 
-- if issuable.is_a?(MergeRequest)
-  - unless @merge_request.closed_without_fork?
-    %hr
-    - if @merge_request.new_record?
-      .form-group
-        = f.label :source_branch, class: 'control-label'
-        .col-sm-10
-          .issuable-form-select-holder
-            = f.select(:source_branch, [@merge_request.source_branch], { }, { class: 'source_branch select2 span2', disabled: true })
+- if issuable.is_a?(MergeRequest) && !@merge_request.closed_without_fork?
+  %hr
+  - if @merge_request.new_record?
     .form-group
-      = f.label :target_branch, class: 'control-label'
+      = f.label :source_branch, class: 'control-label'
       .col-sm-10
         .issuable-form-select-holder
-          = f.select(:target_branch, @merge_request.target_branches, { include_blank: true }, { class: 'target_branch select2 span2', disabled: @merge_request.new_record?, data: {placeholder: "Select branch"} })
-        - if @merge_request.new_record?
-           
-          = link_to 'Change branches', mr_change_branches_path(@merge_request)
-    - if @merge_request.can_remove_source_branch?(current_user)
-      .form-group
-        .col-sm-10.col-sm-offset-2
-          .checkbox
-            = label_tag 'merge_request[force_remove_source_branch]' do
-              = check_box_tag 'merge_request[force_remove_source_branch]', '1', @merge_request.force_remove_source_branch?
-              Remove source branch when merge request is accepted.
+          = f.select(:source_branch, [@merge_request.source_branch], { }, { class: 'source_branch select2 span2', disabled: true })
+  .form-group
+    = f.label :target_branch, class: 'control-label'
+    .col-sm-10
+      .issuable-form-select-holder
+        = f.select(:target_branch, @merge_request.target_branches, { include_blank: true }, { class: 'target_branch select2 span2', disabled: @merge_request.new_record?, data: {placeholder: "Select branch"} })
+      - if @merge_request.new_record?
+         
+        = link_to 'Change branches', mr_change_branches_path(@merge_request)
+  - if @merge_request.can_remove_source_branch?(current_user)
+    .form-group
+      .col-sm-10.col-sm-offset-2
+        .checkbox
+          = label_tag 'merge_request[force_remove_source_branch]' do
+            = check_box_tag 'merge_request[force_remove_source_branch]', '1', @merge_request.force_remove_source_branch?
+            Remove source branch when merge request is accepted.
 
 - is_footer = !(issuable.is_a?(MergeRequest) && issuable.new_record?)
 .row-content-block{class: (is_footer ? "footer-block" : "middle-block")}

spec/models/merge_request_spec.rb

@@ -992,7 +992,7 @@ describe MergeRequest, models: true do
           target_project: project)
       end
 
-      it do
+      it "returns true" do
         unlink_project.execute
         merge_request.reload
 
@@ -1007,23 +1007,35 @@ describe MergeRequest, models: true do
     let(:user)         { create(:user) }
     let(:unlink_project) { Projects::UnlinkForkService.new(fork_project, user) }
 
-    context "closed MR" do
+    context "when merge request is closed" do
       let(:closed_merge_request) do
         create(:closed_merge_request,
           source_project: fork_project,
           target_project: project)
       end
 
-      it "has a fork" do
+      it "returns false if fork exist" do
         expect(closed_merge_request.closed_without_fork?).to be_falsey
       end
 
-      it "does not have a fork" do
+      it "returns true if fork doesn't exist" do
         unlink_project.execute
         closed_merge_request.reload
 
         expect(closed_merge_request.closed_without_fork?).to be_truthy
       end
     end
+
+    context "when merge request is open" do
+      let(:open_merge_request) do
+        create(:merge_request,
+          source_project: fork_project,
+          target_project: project)
+      end
+
+      it "returns false" do
+        expect(open_merge_request.closed_without_fork?).to be_falsey
+      end
+    end
   end
 end

spec/views/projects/merge_requests/edit.html.haml_spec.rb

@@ -6,36 +6,48 @@ describe 'projects/merge_requests/edit.html.haml' do
   let(:user) { create(:user) }
   let(:project) { create(:project) }
   let(:fork_project) { create(:project, forked_from_project: project) }
+  let(:unlink_project) { Projects::UnlinkForkService.new(fork_project, user) }
+
   let(:closed_merge_request) do
     create(:closed_merge_request,
       source_project: fork_project,
       target_project: project,
       author: user)
   end
-  let(:unlink_project) { Projects::UnlinkForkService.new(fork_project, user) }
 
   before do
     assign(:project, project)
     assign(:merge_request, closed_merge_request)
 
     allow(view).to receive(:can?).and_return(true)
-    allow(view).to receive(:current_user).and_return(User.find(closed_merge_request.author_id))
+    allow(view).to receive(:current_user)
+      .and_return(User.find(closed_merge_request.author_id))
   end
 
-  context 'when closed MR without fork' do
+  context 'when closed merge request without fork' do
     it "shows editable fields" do
       unlink_project.execute
       closed_merge_request.reload
+
       render
 
       expect(rendered).to have_field('merge_request[title]')
-      expect(rendered).to have_css('label', text: "Title")
       expect(rendered).to have_field('merge_request[description]')
-      expect(rendered).to have_css('label', text: "Description")
-      expect(rendered).to have_css('label', text: "Assignee")
-      expect(rendered).to have_css('label', text: "Milestone")
-      expect(rendered).to have_css('label', text: "Labels")
-      expect(rendered).not_to have_css('label', text: "Target branch")
+      expect(rendered).to have_selector('#merge_request_assignee_id', visible: false)
+      expect(rendered).to have_selector('#merge_request_milestone_id', visible: false)
+      expect(rendered).not_to have_selector('#merge_request_target_branch', visible: false)
+    end
+  end
+
+  context 'when closed merge request with fork' do
+    it "shows editable fields" do
+      render
+
+      expect(rendered).to have_field('merge_request[title]')
+      expect(rendered).to have_field('merge_request[description]')
+      expect(rendered).to have_selector('#merge_request_assignee_id', visible: false)
+      expect(rendered).to have_selector('#merge_request_milestone_id', visible: false)
+      expect(rendered).to have_selector('#merge_request_target_branch', visible: false)
     end
   end
 end

spec/views/projects/merge_requests/show.html.haml_spec.rb

@@ -5,23 +5,24 @@ describe 'projects/merge_requests/show.html.haml' do
 
   let(:project) { create(:project) }
   let(:fork_project) { create(:project, forked_from_project: project) }
-  let(:merge_request) do
-    create(:merge_request,
+
+  let(:closed_merge_request) do
+    create(:closed_merge_request,
       source_project: fork_project,
       source_branch: 'add-submodule-version-bump',
-      target_branch: 'master', target_project: project)
+      target_branch: 'master',
+      target_project: project)
   end
 
   before do
     assign(:project, project)
-    assign(:merge_request, merge_request)
+    assign(:merge_request, closed_merge_request)
     assign(:commits_count, 0)
 
-    merge_request.close!
     allow(view).to receive(:can?).and_return(true)
   end
 
-  context 'closed MR' do
+  context 'when merge request is closed' do
     it 'shows Reopen button' do
       render