[EE] Fix the default branches sorting to actually be 'Last updated'

Signed-off-by: Rémy Coutable <remy@rymai.me>

app/controllers/projects/branches_controller.rb

@@ -9,7 +9,7 @@ class Projects::BranchesController < Projects::ApplicationController
 
   def index
     @sort = params[:sort].presence || sort_value_recently_updated
-    @branches = BranchesFinder.new(@repository, params).execute
+    @branches = BranchesFinder.new(@repository, params.merge(sort: @sort)).execute
     @branches = Kaminari.paginate_array(@branches).page(params[:page])
 
     respond_to do |format|

changelogs/unreleased/26890-fix-default-branches-sorting.yml

+---
+title: Fix the default branches sorting to actually be 'Last updated'
+merge_request: 14295
+author:
+type: fixed

spec/ee/support/protected_branch_helpers.rb

+module EE
+  module ProtectedBranchHelpers
+    def set_allowed_to(operation, option = 'Masters', form: '.js-new-protected-branch')
+      within form do
+        find(".js-allowed-to-#{operation}").trigger('click')
+        wait_for_requests
+
+        Array(option).each { |opt| click_on(opt) }
+
+        find(".js-allowed-to-#{operation}").trigger('click') # needed to submit form in some cases
+      end
+    end
+  end
+end

spec/features/projects/branches/download_buttons_spec.rb

@@ -29,7 +29,7 @@ feature 'Download buttons in branches page' do
   describe 'when checking branches' do
     context 'with artifacts' do
       before do
-        visit project_branches_path(project)
+        visit project_branches_path(project, search: 'binary-encoding')
       end
 
       scenario 'shows download artifacts button' do

spec/features/projects/branches_spec.rb

 require 'spec_helper'
 
 describe 'Branches' do
-  include ProtectedBranchHelpers
+  include EE::ProtectedBranchHelpers
 
   let(:user) { create(:user) }
   let(:project) { create(:project, :public, :repository) }
@@ -14,12 +14,10 @@ describe 'Branches' do
     end
 
     describe 'Initial branches page' do
-      it 'shows all the branches' do
+      it 'shows all the branches sorted by last updated by default' do
         visit project_branches_path(project)
 
-        repository.branches_sorted_by(:name).first(20).each do |branch|
-          expect(page).to have_content("#{branch.name}")
-        end
+        expect(page).to have_content(sorted_branches(repository, count: 20, sort_by: :updated_desc))
       end
 
       it 'sorts the branches by name' do
@@ -28,22 +26,7 @@ describe 'Branches' do
         click_button "Last updated" # Open sorting dropdown
         click_link "Name"
 
-        sorted = repository.branches_sorted_by(:name).first(20).map do |branch|
-          Regexp.escape(branch.name)
-        end
-        expect(page).to have_content(/#{sorted.join(".*")}/)
-      end
-
-      it 'sorts the branches by last updated' do
-        visit project_branches_path(project)
-
-        click_button "Last updated" # Open sorting dropdown
-        click_link "Last updated"
-
-        sorted = repository.branches_sorted_by(:updated_desc).first(20).map do |branch|
-          Regexp.escape(branch.name)
-        end
-        expect(page).to have_content(/#{sorted.join(".*")}/)
+        expect(page).to have_content(sorted_branches(repository, count: 20, sort_by: :name))
       end
 
       it 'sorts the branches by oldest updated' do
@@ -52,10 +35,7 @@ describe 'Branches' do
         click_button "Last updated" # Open sorting dropdown
         click_link "Oldest updated"
 
-        sorted = repository.branches_sorted_by(:updated_asc).first(20).map do |branch|
-          Regexp.escape(branch.name)
-        end
-        expect(page).to have_content(/#{sorted.join(".*")}/)
+        expect(page).to have_content(sorted_branches(repository, count: 20, sort_by: :updated_asc))
       end
 
       it 'avoids a N+1 query in branches index' do
@@ -95,30 +75,6 @@ describe 'Branches' do
         expect(find('.all-branches')).to have_selector('li', count: 0)
       end
     end
-
-    describe 'Delete protected branch' do
-      before do
-        project.add_user(user, :master)
-        visit project_protected_branches_path(project)
-        set_protected_branch_name('fix')
-        set_allowed_to('merge')
-        set_allowed_to('push')
-        click_on "Protect"
-
-        within(".protected-branches-list") { expect(page).to have_content('fix') }
-        expect(ProtectedBranch.count).to eq(1)
-        project.add_user(user, :developer)
-      end
-
-      it 'does not allow devleoper to remove protected branch', js: true do
-        visit project_branches_path(project)
-
-        fill_in 'branch-search', with: 'fix'
-        find('#branch-search').native.send_keys(:enter)
-
-        expect(page).to have_css('.btn-remove.disabled')
-      end
-    end
   end
 
   context 'logged in as master' do
@@ -134,39 +90,6 @@ describe 'Branches' do
         expect(page).to have_content("Protected branches can be managed in project settings")
       end
     end
-
-    describe 'Delete protected branch' do
-      before do
-        visit project_protected_branches_path(project)
-        set_protected_branch_name('fix')
-        set_allowed_to('merge')
-        set_allowed_to('push')
-        click_on "Protect"
-
-        within(".protected-branches-list") { expect(page).to have_content('fix') }
-        expect(ProtectedBranch.count).to eq(1)
-      end
-
-      it 'removes branch after modal confirmation', js: true do
-        visit project_branches_path(project)
-
-        fill_in 'branch-search', with: 'fix'
-        find('#branch-search').native.send_keys(:enter)
-
-        expect(page).to have_content('fix')
-        expect(find('.all-branches')).to have_selector('li', count: 1)
-        page.find('[data-target="#modal-delete-branch"]').trigger(:click)
-
-        expect(page).to have_css('.js-delete-branch[disabled]')
-        fill_in 'delete_branch_input', with: 'fix'
-        click_link 'Delete protected branch'
-
-        fill_in 'branch-search', with: 'fix'
-        find('#branch-search').native.send_keys(:enter)
-
-        expect(page).to have_content('No branches to show')
-      end
-    end
   end
 
   context 'logged out' do
@@ -180,4 +103,13 @@ describe 'Branches' do
       end
     end
   end
+
+  def sorted_branches(repository, count:, sort_by:)
+    sorted_branches =
+      repository.branches_sorted_by(sort_by).first(count).map do |branch|
+        Regexp.escape(branch.name)
+      end
+
+    Regexp.new(sorted_branches.join('.*'))
+  end
 end

spec/features/protected_branches_spec.rb

 require 'spec_helper'
 
 feature 'Protected Branches', :js do
-  include ProtectedBranchHelpers
+  include EE::ProtectedBranchHelpers
 
-  let(:user) { create(:user, :admin) }
+  let(:user) { create(:user) }
+  let(:admin) { create(:admin) }
   let(:project) { create(:project, :repository) }
 
-  before do
-    sign_in(user)
-  end
-
-  def set_protected_branch_name(branch_name)
-    find(".js-protected-branch-select").trigger('click')
-    find(".dropdown-input-field").set(branch_name)
-    click_on("Create wildcard #{branch_name}")
-  end
-
-  describe "explicit protected branches" do
-    it "allows creating explicit protected branches" do
-      visit project_protected_branches_path(project)
-      set_protected_branch_name('some-branch')
-      set_allowed_to('merge')
-      set_allowed_to('push')
-      click_on "Protect"
-
-      within(".protected-branches-list") { expect(page).to have_content('some-branch') }
-      expect(ProtectedBranch.count).to eq(1)
-      expect(ProtectedBranch.last.name).to eq('some-branch')
+  context 'logged in as developer' do
+    before do
+      project.add_developer(user)
+      sign_in(user)
     end
 
-    it "displays the last commit on the matching branch if it exists" do
-      commit = create(:commit, project: project)
-      project.repository.add_branch(user, 'some-branch', commit.id)
-
-      visit project_protected_branches_path(project)
-      set_protected_branch_name('some-branch')
-      set_allowed_to('merge')
-      set_allowed_to('push')
-      click_on "Protect"
+    describe 'Delete protected branch' do
+      before do
+        create(:protected_branch, project: project, name: 'fix')
+        expect(ProtectedBranch.count).to eq(1)
+      end
 
-      within(".protected-branches-list") { expect(page).to have_content(commit.id[0..7]) }
-    end
+      it 'does not allow developer to removes protected branch' do
+        visit project_branches_path(project)
 
-    it "displays an error message if the named branch does not exist" do
-      visit project_protected_branches_path(project)
-      set_protected_branch_name('some-branch')
-      set_allowed_to('merge')
-      set_allowed_to('push')
-      click_on "Protect"
+        fill_in 'branch-search', with: 'fix'
+        find('#branch-search').native.send_keys(:enter)
 
-      within(".protected-branches-list") { expect(page).to have_content('branch was removed') }
+        expect(page).to have_css('.btn-remove.disabled')
+      end
     end
   end
 
-  describe "wildcard protected branches" do
-    it "allows creating protected branches with a wildcard" do
-      visit project_protected_branches_path(project)
-      set_protected_branch_name('*-stable')
-      set_allowed_to('merge')
-      set_allowed_to('push')
-      click_on "Protect"
-
-      within(".protected-branches-list") { expect(page).to have_content('*-stable') }
-      expect(ProtectedBranch.count).to eq(1)
-      expect(ProtectedBranch.last.name).to eq('*-stable')
+  context 'logged in as master' do
+    before do
+      project.add_master(user)
+      sign_in(user)
     end
 
-    it "displays the number of matching branches" do
-      project.repository.add_branch(user, 'production-stable', 'master')
-      project.repository.add_branch(user, 'staging-stable', 'master')
+    describe 'Delete protected branch' do
+      before do
+        create(:protected_branch, project: project, name: 'fix')
+        expect(ProtectedBranch.count).to eq(1)
+      end
 
-      visit project_protected_branches_path(project)
-      set_protected_branch_name('*-stable')
-      set_allowed_to('merge')
-      set_allowed_to('push')
-      click_on "Protect"
+      it 'removes branch after modal confirmation' do
+        visit project_branches_path(project)
 
-      within(".protected-branches-list") { expect(page).to have_content("2 matching branches") }
-    end
+        fill_in 'branch-search', with: 'fix'
+        find('#branch-search').native.send_keys(:enter)
 
-    it "displays all the branches matching the wildcard" do
-      project.repository.add_branch(user, 'production-stable', 'master')
-      project.repository.add_branch(user, 'staging-stable', 'master')
-      project.repository.add_branch(user, 'development', 'master')
+        expect(page).to have_content('fix')
+        expect(find('.all-branches')).to have_selector('li', count: 1)
+        page.find('[data-target="#modal-delete-branch"]').trigger(:click)
 
-      visit project_protected_branches_path(project)
-      set_protected_branch_name('*-stable')
-      set_allowed_to('merge')
-      set_allowed_to('push')
-      click_on "Protect"
+        expect(page).to have_css('.js-delete-branch[disabled]')
+        fill_in 'delete_branch_input', with: 'fix'
+        click_link 'Delete protected branch'
 
-      visit project_protected_branches_path(project)
-      click_on "2 matching branches"
+        fill_in 'branch-search', with: 'fix'
+        find('#branch-search').native.send_keys(:enter)
 
-      within(".protected-branches-list") do
-        expect(page).to have_content("production-stable")
-        expect(page).to have_content("staging-stable")
-        expect(page).not_to have_content("development")
+        expect(page).to have_content('No branches to show')
       end
     end
   end
 
-  describe "access control" do
-    describe 'with ref permissions for users enabled' do
-      before do
-        stub_licensed_features(protected_refs_for_users: true)
+  context 'logged in as admin' do
+    before do
+      sign_in(admin)
+    end
+
+    describe "explicit protected branches" do
+      it "allows creating explicit protected branches" do
+        visit project_protected_branches_path(project)
+        set_protected_branch_name('some-branch')
+        set_allowed_to('merge')
+        set_allowed_to('push')
+        click_on "Protect"
+
+        within(".protected-branches-list") { expect(page).to have_content('some-branch') }
+        expect(ProtectedBranch.count).to eq(1)
+        expect(ProtectedBranch.last.name).to eq('some-branch')
+      end
+
+      it "displays the last commit on the matching branch if it exists" do
+        commit = create(:commit, project: project)
+        project.repository.add_branch(user, 'some-branch', commit.id)
+
+        visit project_protected_branches_path(project)
+        set_protected_branch_name('some-branch')
+        set_allowed_to('merge')
+        set_allowed_to('push')
+        click_on "Protect"
+
+        within(".protected-branches-list") { expect(page).to have_content(commit.id[0..7]) }
       end
 
-      include_examples "protected branches > access control > EE"
+      it "displays an error message if the named branch does not exist" do
+        visit project_protected_branches_path(project)
+        set_protected_branch_name('some-branch')
+        set_allowed_to('merge')
+        set_allowed_to('push')
+        click_on "Protect"
+
+        within(".protected-branches-list") { expect(page).to have_content('branch was removed') }
+      end
     end
 
-    describe 'with ref permissions for users disabled' do
-      before do
-        stub_licensed_features(protected_refs_for_users: false)
+    describe "wildcard protected branches" do
+      it "allows creating protected branches with a wildcard" do
+        visit project_protected_branches_path(project)
+        set_protected_branch_name('*-stable')
+        set_allowed_to('merge')
+        set_allowed_to('push')
+        click_on "Protect"
+
+        within(".protected-branches-list") { expect(page).to have_content('*-stable') }
+        expect(ProtectedBranch.count).to eq(1)
+        expect(ProtectedBranch.last.name).to eq('*-stable')
       end
 
-      include_examples "protected branches > access control > CE"
+      it "displays the number of matching branches" do
+        project.repository.add_branch(user, 'production-stable', 'master')
+        project.repository.add_branch(user, 'staging-stable', 'master')
+
+        visit project_protected_branches_path(project)
+        set_protected_branch_name('*-stable')
+        set_allowed_to('merge')
+        set_allowed_to('push')
+        click_on "Protect"
 
-      context 'with existing access levels' do
-        let(:protected_branch) { create(:protected_branch, project: project) }
+        within(".protected-branches-list") { expect(page).to have_content("2 matching branches") }
+      end
+
+      it "displays all the branches matching the wildcard" do
+        project.repository.add_branch(user, 'production-stable', 'master')
+        project.repository.add_branch(user, 'staging-stable', 'master')
+        project.repository.add_branch(user, 'development', 'master')
 
-        it 'shows users that can push to the branch' do
-          protected_branch.push_access_levels.new(user: create(:user, name: 'Jane'))
-            .save!(validate: false)
+        visit project_protected_branches_path(project)
+        set_protected_branch_name('*-stable')
+        set_allowed_to('merge')
+        set_allowed_to('push')
+        click_on "Protect"
 
-          visit project_settings_repository_path(project)
+        visit project_protected_branches_path(project)
+        click_on "2 matching branches"
 
-          expect(page).to have_content("The following user can also push to this branch: "\
-                                       "Jane")
+        within(".protected-branches-list") do
+          expect(page).to have_content("production-stable")
+          expect(page).to have_content("staging-stable")
+          expect(page).not_to have_content("development")
         end
+      end
+    end
 
-        it 'shows groups that can push to the branch' do
-          protected_branch.push_access_levels.new(group: create(:group, name: 'Team Awesome'))
-            .save!(validate: false)
+    describe "access control" do
+      describe 'with ref permissions for users enabled' do
+        before do
+          stub_licensed_features(protected_refs_for_users: true)
+        end
 
-          visit project_settings_repository_path(project)
+        include_examples "protected branches > access control > EE"
+      end
 
-          expect(page).to have_content("Members of this group can also push to "\
-                                       "this branch: Team Awesome")
+      describe 'with ref permissions for users disabled' do
+        before do
+          stub_licensed_features(protected_refs_for_users: false)
         end
 
-        it 'shows users that can merge into the branch' do
-          protected_branch.merge_access_levels.new(user: create(:user, name: 'Jane'))
-            .save!(validate: false)
+        include_examples "protected branches > access control > CE"
 
-          visit project_settings_repository_path(project)
+        context 'with existing access levels' do
+          let(:protected_branch) { create(:protected_branch, project: project) }
 
-          expect(page).to have_content("The following user can also merge into "\
-                                       "this branch: Jane")
-        end
+          it 'shows users that can push to the branch' do
+            protected_branch.push_access_levels.new(user: create(:user, name: 'Jane'))
+              .save!(validate: false)
+
+            visit project_settings_repository_path(project)
+
+            expect(page).to have_content("The following user can also push to this branch: "\
+                                         "Jane")
+          end
 
-        it 'shows groups that have can push to the branch' do
-          protected_branch.merge_access_levels.new(group: create(:group, name: 'Team Awesome'))
-            .save!(validate: false)
-          protected_branch.merge_access_levels.new(group: create(:group, name: 'Team B'))
-            .save!(validate: false)
+          it 'shows groups that can push to the branch' do
+            protected_branch.push_access_levels.new(group: create(:group, name: 'Team Awesome'))
+              .save!(validate: false)
 
-          visit project_settings_repository_path(project)
+            visit project_settings_repository_path(project)
 
-          expect(page).to have_content("Members of these groups can also merge into "\
-                                       "this branch:")
-          expect(page).to have_content(/(Team Awesome|Team B) and (Team Awesome|Team B)/)
+            expect(page).to have_content("Members of this group can also push to "\
+                                         "this branch: Team Awesome")
+          end
+
+          it 'shows users that can merge into the branch' do
+            protected_branch.merge_access_levels.new(user: create(:user, name: 'Jane'))
+              .save!(validate: false)
+
+            visit project_settings_repository_path(project)
+
+            expect(page).to have_content("The following user can also merge into "\
+                                         "this branch: Jane")
+          end
+
+          it 'shows groups that have can push to the branch' do
+            protected_branch.merge_access_levels.new(group: create(:group, name: 'Team Awesome'))
+              .save!(validate: false)
+            protected_branch.merge_access_levels.new(group: create(:group, name: 'Team B'))
+              .save!(validate: false)
+
+            visit project_settings_repository_path(project)
+
+            expect(page).to have_content("Members of these groups can also merge into "\
+                                         "this branch:")
+            expect(page).to have_content(/(Team Awesome|Team B) and (Team Awesome|Team B)/)
+          end
         end
       end
     end
   end
+
+  def set_protected_branch_name(branch_name)
+    find(".js-protected-branch-select").trigger('click')
+    find(".dropdown-input-field").set(branch_name)
+    click_on("Create wildcard #{branch_name}")
+  end
 end

spec/support/protected_branch_helpers.rb

-module ProtectedBranchHelpers
-  def set_allowed_to(operation, option = 'Masters', form: '.js-new-protected-branch')
-    within form do
-      find(".js-allowed-to-#{operation}").trigger('click')
-      wait_for_requests
-
-      Array(option).each { |opt| click_on(opt) }
-
-      find(".js-allowed-to-#{operation}").trigger('click') # needed to submit form in some cases
-    end
-  end
-
-  def set_protected_branch_name(branch_name)
-    find(".js-protected-branch-select").trigger('click')
-    find(".dropdown-input-field").set(branch_name)
-    click_on("Create wildcard #{branch_name}")
-  end
-end