diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb index a9cce578366..7f03ce07dec 100644 --- a/app/controllers/projects/branches_controller.rb +++ b/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| diff --git a/changelogs/unreleased/26890-fix-default-branches-sorting.yml b/changelogs/unreleased/26890-fix-default-branches-sorting.yml new file mode 100644 index 00000000000..cf7060190b3 --- /dev/null +++ b/changelogs/unreleased/26890-fix-default-branches-sorting.yml @@ -0,0 +1,5 @@ +--- +title: Fix the default branches sorting to actually be 'Last updated' +merge_request: 14295 +author: +type: fixed diff --git a/spec/features/projects/branches/download_buttons_spec.rb b/spec/features/projects/branches/download_buttons_spec.rb index ad06cee4e81..2f407b13c2f 100644 --- a/spec/features/projects/branches/download_buttons_spec.rb +++ b/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 diff --git a/spec/features/projects/branches_spec.rb b/spec/features/projects/branches_spec.rb index ad4527a0b74..d1f5623554d 100644 --- a/spec/features/projects/branches_spec.rb +++ b/spec/features/projects/branches_spec.rb @@ -5,12 +5,6 @@ describe 'Branches' do let(:project) { create(:project, :public, :repository) } let(:repository) { project.repository } - def set_protected_branch_name(branch_name) - find(".js-protected-branch-select").click - find(".dropdown-input-field").set(branch_name) - click_on("Create wildcard #{branch_name}") - end - context 'logged in as developer' do before do sign_in(user) @@ -18,12 +12,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 @@ -32,22 +24,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 @@ -56,10 +33,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 @@ -99,28 +73,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') - 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 removes 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 @@ -136,37 +88,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') - 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 +101,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 diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb index 3677bf38724..bf9885f73bd 100644 --- a/spec/features/protected_branches_spec.rb +++ b/spec/features/protected_branches_spec.rb @@ -1,11 +1,148 @@ require 'spec_helper' -feature 'Protected Branches', js: true do - let(:user) { create(:user, :admin) } +feature 'Protected Branches', :js do + let(:user) { create(:user) } + let(:admin) { create(:admin) } let(:project) { create(:project, :repository) } - before do - sign_in(user) + context 'logged in as developer' do + before do + project.add_developer(user) + sign_in(user) + end + + describe 'Delete protected branch' do + before do + create(:protected_branch, project: project, name: 'fix') + expect(ProtectedBranch.count).to eq(1) + end + + it 'does not allow developer to removes protected branch' 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 + before do + project.add_master(user) + sign_in(user) + end + + describe 'Delete protected branch' do + before do + create(:protected_branch, project: project, name: 'fix') + expect(ProtectedBranch.count).to eq(1) + end + + it 'removes branch after modal confirmation' 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 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') + 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(admin, 'some-branch', commit.id) + + visit project_protected_branches_path(project) + set_protected_branch_name('some-branch') + click_on "Protect" + + within(".protected-branches-list") { expect(page).to have_content(commit.id[0..7]) } + end + + 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') + click_on "Protect" + + within(".protected-branches-list") { expect(page).to have_content('branch was removed') } + 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') + 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 + + it "displays the number of matching branches" do + project.repository.add_branch(admin, 'production-stable', 'master') + project.repository.add_branch(admin, 'staging-stable', 'master') + + visit project_protected_branches_path(project) + set_protected_branch_name('*-stable') + click_on "Protect" + + 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(admin, 'production-stable', 'master') + project.repository.add_branch(admin, 'staging-stable', 'master') + project.repository.add_branch(admin, 'development', 'master') + + visit project_protected_branches_path(project) + set_protected_branch_name('*-stable') + click_on "Protect" + + visit project_protected_branches_path(project) + click_on "2 matching branches" + + 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 + + describe "access control" do + include_examples "protected branches > access control > CE" + end end def set_protected_branch_name(branch_name) @@ -13,81 +150,4 @@ feature 'Protected Branches', js: true do 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') - 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') - click_on "Protect" - - within(".protected-branches-list") { expect(page).to have_content(commit.id[0..7]) } - end - - 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') - click_on "Protect" - - within(".protected-branches-list") { expect(page).to have_content('branch was removed') } - 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') - 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 - - 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') - click_on "Protect" - - 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') - - visit project_protected_branches_path(project) - set_protected_branch_name('*-stable') - click_on "Protect" - - visit project_protected_branches_path(project) - click_on "2 matching branches" - - 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 - - describe "access control" do - include_examples "protected branches > access control > CE" - end end