diff options
-rw-r--r-- | app/controllers/projects/branches_controller.rb | 41 | ||||
-rw-r--r-- | app/finders/branches_finder.rb | 2 | ||||
-rw-r--r-- | app/helpers/branches_helper.rb | 11 | ||||
-rw-r--r-- | app/views/projects/branches/_panel.html.haml | 19 | ||||
-rw-r--r-- | app/views/projects/branches/index.html.haml | 53 | ||||
-rw-r--r-- | changelogs/unreleased/40187-project-branch-dashboard-with-active-stale-branches.yml | 5 | ||||
-rw-r--r-- | config/routes/repository.rb | 1 | ||||
-rw-r--r-- | lib/gitlab/git/branch.rb | 14 | ||||
-rw-r--r-- | spec/controllers/projects/branches_controller_spec.rb | 33 | ||||
-rw-r--r-- | spec/features/projects/branches/download_buttons_spec.rb | 2 | ||||
-rw-r--r-- | spec/features/projects/branches_spec.rb | 137 | ||||
-rw-r--r-- | spec/features/projects/environments/environment_spec.rb | 2 | ||||
-rw-r--r-- | spec/features/projects/merge_request_button_spec.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/git/branch_spec.rb | 64 |
14 files changed, 338 insertions, 50 deletions
diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb index cabafe26357..965cece600e 100644 --- a/app/controllers/projects/branches_controller.rb +++ b/app/controllers/projects/branches_controller.rb @@ -7,13 +7,19 @@ class Projects::BranchesController < Projects::ApplicationController before_action :authorize_download_code! before_action :authorize_push_code!, only: [:new, :create, :destroy, :destroy_all_merged] - def index - @sort = params[:sort].presence || sort_value_recently_updated - @branches = BranchesFinder.new(@repository, params.merge(sort: @sort)).execute - @branches = Kaminari.paginate_array(@branches).page(params[:page]) + # Support legacy URLs + before_action :redirect_for_legacy_index_sort_or_search, only: [:index] + def index respond_to do |format| format.html do + @sort = params[:sort].presence || sort_value_recently_updated + @mode = params[:state].presence || 'overview' + @overview_max_branches = 5 + + # Fetch branches for the specified mode + fetch_branches_by_mode + @refs_pipelines = @project.pipelines.latest_successful_for_refs(@branches.map(&:name)) @merged_branch_names = repository.merged_branch_names(@branches.map(&:name)) @@ -28,7 +34,9 @@ class Projects::BranchesController < Projects::ApplicationController end end format.json do - render json: @branches.map(&:name) + branches = BranchesFinder.new(@repository, params).execute + branches = Kaminari.paginate_array(branches).page(params[:page]) + render json: branches.map(&:name) end end end @@ -123,4 +131,27 @@ class Projects::BranchesController < Projects::ApplicationController context: 'autodeploy' ) end + + def redirect_for_legacy_index_sort_or_search + # Normalize a legacy URL with redirect + if request.format != :json && !params[:state].presence && [:sort, :search, :page].any? { |key| params[key].presence } + redirect_to project_branches_filtered_path(@project, state: 'all'), notice: 'Update your bookmarked URLs as filtered/sorted branches URL has been changed.' + end + end + + def fetch_branches_by_mode + if @mode == 'overview' + # overview mode + @active_branches, @stale_branches = BranchesFinder.new(@repository, sort: sort_value_recently_updated).execute.partition(&:active?) + # Here we get one more branch to indicate if there are more data we're not showing + @active_branches = @active_branches.first(@overview_max_branches + 1) + @stale_branches = @stale_branches.first(@overview_max_branches + 1) + @branches = @active_branches + @stale_branches + else + # active/stale/all view mode + @branches = BranchesFinder.new(@repository, params.merge(sort: @sort)).execute + @branches = @branches.select { |b| b.state.to_s == @mode } if %w[active stale].include?(@mode) + @branches = Kaminari.paginate_array(@branches).page(params[:page]) + end + end end diff --git a/app/finders/branches_finder.rb b/app/finders/branches_finder.rb index 852eac3647d..8bb1366867c 100644 --- a/app/finders/branches_finder.rb +++ b/app/finders/branches_finder.rb @@ -1,5 +1,5 @@ class BranchesFinder - def initialize(repository, params) + def initialize(repository, params = {}) @repository = repository @params = params end diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb index 00b9a0e00eb..07b1fc3d7cf 100644 --- a/app/helpers/branches_helper.rb +++ b/app/helpers/branches_helper.rb @@ -1,15 +1,4 @@ module BranchesHelper - def filter_branches_path(options = {}) - exist_opts = { - search: params[:search], - sort: params[:sort] - } - - options = exist_opts.merge(options) - - project_branches_path(@project, @id, options) - end - def project_branches options_for_select(@project.repository.branch_names, @project.default_branch) end diff --git a/app/views/projects/branches/_panel.html.haml b/app/views/projects/branches/_panel.html.haml new file mode 100644 index 00000000000..12e5a8e8d69 --- /dev/null +++ b/app/views/projects/branches/_panel.html.haml @@ -0,0 +1,19 @@ +- branches = local_assigns.fetch(:branches) +- state = local_assigns.fetch(:state) +- panel_title = local_assigns.fetch(:panel_title) +- show_more_text = local_assigns.fetch(:show_more_text) +- project = local_assigns.fetch(:project) +- overview_max_branches = local_assigns.fetch(:overview_max_branches) + +- return unless branches.any? + +.panel.panel-default.prepend-top-10 + .panel-heading + %h4.panel-title + = panel_title + %ul.content-list.all-branches + - branches.first(overview_max_branches).each do |branch| + = render "projects/branches/branch", branch: branch, merged: project.repository.merged_to_root_ref?(branch) + - if branches.size > overview_max_branches + .panel-footer.text-center + = link_to show_more_text, project_branches_filtered_path(project, state: state), id: "state-#{state}", data: { state: state } diff --git a/app/views/projects/branches/index.html.haml b/app/views/projects/branches/index.html.haml index fb770764364..5dcc72d8263 100644 --- a/app/views/projects/branches/index.html.haml +++ b/app/views/projects/branches/index.html.haml @@ -3,26 +3,35 @@ %div{ class: container_class } .top-area.adjust - - if can?(current_user, :admin_project, @project) - .nav-text - - project_settings_link = link_to s_('Branches|project settings'), project_protected_branches_path(@project) - = s_('Branches|Protected branches can be managed in %{project_settings_link}').html_safe % { project_settings_link: project_settings_link } + %ul.nav-links.issues-state-filters + %li{ class: active_when(@mode == 'overview') }> + = link_to s_('Branches|Overview'), project_branches_path(@project), title: s_('Branches|Show overview of the branches') + + %li{ class: active_when(@mode == 'active') }> + = link_to s_('Branches|Active'), project_branches_filtered_path(@project, state: 'active'), title: s_('Branches|Show active branches') + + %li{ class: active_when(@mode == 'stale') }> + = link_to s_('Branches|Stale'), project_branches_filtered_path(@project, state: 'stale'), title: s_('Branches|Show stale branches') + + %li{ class: active_when(!%w[overview active stale].include?(@mode)) }> + = link_to s_('Branches|All'), project_branches_filtered_path(@project, state: 'all'), title: s_('Branches|Show all branches') .nav-controls - = form_tag(filter_branches_path, method: :get) do + = form_tag(project_branches_filtered_path(@project, state: 'all'), method: :get) do = search_field_tag :search, params[:search], { placeholder: s_('Branches|Filter by branch name'), id: 'branch-search', class: 'form-control search-text-input input-short', spellcheck: false } - .dropdown.inline> - %button.dropdown-menu-toggle{ type: 'button', 'data-toggle' => 'dropdown' } - %span.light - = branches_sort_options_hash[@sort] - = icon('chevron-down') - %ul.dropdown-menu.dropdown-menu-align-right.dropdown-menu-selectable - %li.dropdown-header - = s_('Branches|Sort by') - - branches_sort_options_hash.each do |value, title| - %li - = link_to title, filter_branches_path(sort: value), class: ("is-active" if @sort == value) + - unless @mode == 'overview' + .dropdown.inline> + %button.dropdown-menu-toggle{ type: 'button', 'data-toggle' => 'dropdown' } + %span.light + = branches_sort_options_hash[@sort] + = icon('chevron-down') + %ul.dropdown-menu.dropdown-menu-align-right.dropdown-menu-selectable + %li.dropdown-header + = s_('Branches|Sort by') + - branches_sort_options_hash.each do |value, title| + %li + = link_to title, project_branches_filtered_path(@project, state: 'all', search: params[:search], sort: value), class: ("is-active" if @sort == value) - if can? current_user, :push_code, @project = link_to project_merged_branches_path(@project), @@ -35,7 +44,17 @@ = link_to new_project_branch_path(@project), class: 'btn btn-create' do = s_('Branches|New branch') - - if @branches.any? + - if can?(current_user, :admin_project, @project) + - project_settings_link = link_to s_('Branches|project settings'), project_protected_branches_path(@project) + .row-content-block + %h5 + = s_('Branches|Protected branches can be managed in %{project_settings_link}.').html_safe % { project_settings_link: project_settings_link } + + - if @mode == 'overview' && (@active_branches.any? || @stale_branches.any?) + = render "projects/branches/panel", branches: @active_branches, state: 'active', panel_title: s_('Branches|Active branches'), show_more_text: s_('Branches|Show more active branches'), project: @project, overview_max_branches: @overview_max_branches + = render "projects/branches/panel", branches: @stale_branches, state: 'stale', panel_title: s_('Branches|Stale branches'), show_more_text: s_('Branches|Show more stale branches'), project: @project, overview_max_branches: @overview_max_branches + + - elsif @branches.any? %ul.content-list.all-branches - @branches.each do |branch| = render "projects/branches/branch", branch: branch, merged: @merged_branch_names.include?(branch.name) diff --git a/changelogs/unreleased/40187-project-branch-dashboard-with-active-stale-branches.yml b/changelogs/unreleased/40187-project-branch-dashboard-with-active-stale-branches.yml new file mode 100644 index 00000000000..3833aab42dd --- /dev/null +++ b/changelogs/unreleased/40187-project-branch-dashboard-with-active-stale-branches.yml @@ -0,0 +1,5 @@ +--- +title: Add overview of branches and a filter for active/stale branches +merge_request: 15402 +author: Takuya Noguchi +type: added diff --git a/config/routes/repository.rb b/config/routes/repository.rb index 9ffdebbcff1..eace3a615b4 100644 --- a/config/routes/repository.rb +++ b/config/routes/repository.rb @@ -49,6 +49,7 @@ scope format: false do end end + get '/branches/:state', to: 'branches#index', as: :branches_filtered, constraints: { state: /active|stale|all/ } resources :branches, only: [:index, :new, :create, :destroy] delete :merged_branches, controller: 'branches', action: :destroy_all_merged resources :tags, only: [:index, :show, :new, :create, :destroy] do diff --git a/lib/gitlab/git/branch.rb b/lib/gitlab/git/branch.rb index ae7e88f0503..6351cfb83e3 100644 --- a/lib/gitlab/git/branch.rb +++ b/lib/gitlab/git/branch.rb @@ -1,6 +1,8 @@ module Gitlab module Git class Branch < Ref + STALE_BRANCH_THRESHOLD = 3.months + def self.find(repo, branch_name) if branch_name.is_a?(Gitlab::Git::Branch) branch_name @@ -12,6 +14,18 @@ module Gitlab def initialize(repository, name, target, target_commit) super(repository, name, target, target_commit) end + + def active? + self.dereferenced_target.committed_date >= STALE_BRANCH_THRESHOLD.ago + end + + def stale? + !active? + end + + def state + active? ? :active : :stale + end end end end diff --git a/spec/controllers/projects/branches_controller_spec.rb b/spec/controllers/projects/branches_controller_spec.rb index 734396ddf7b..3b9e06cb5ad 100644 --- a/spec/controllers/projects/branches_controller_spec.rb +++ b/spec/controllers/projects/branches_controller_spec.rb @@ -407,10 +407,43 @@ describe Projects::BranchesController do get :index, namespace_id: project.namespace, project_id: project, + state: 'all', format: :html expect(response).to have_gitlab_http_status(200) end end + + context 'when depreated sort/search/page parameters are specified' do + it 'returns with a status 301 when sort specified' do + get :index, + namespace_id: project.namespace, + project_id: project, + sort: 'updated_asc', + format: :html + + expect(response).to redirect_to project_branches_filtered_path(project, state: 'all') + end + + it 'returns with a status 301 when search specified' do + get :index, + namespace_id: project.namespace, + project_id: project, + search: 'feature', + format: :html + + expect(response).to redirect_to project_branches_filtered_path(project, state: 'all') + end + + it 'returns with a status 301 when page specified' do + get :index, + namespace_id: project.namespace, + project_id: project, + page: 2, + format: :html + + expect(response).to redirect_to project_branches_filtered_path(project, state: 'all') + end + end end end diff --git a/spec/features/projects/branches/download_buttons_spec.rb b/spec/features/projects/branches/download_buttons_spec.rb index 39bcea013e7..605298ba8ab 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, search: 'binary-encoding') + visit project_branches_filtered_path(project, state: 'all', 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 2fddd274078..2a9d9e6416c 100644 --- a/spec/features/projects/branches_spec.rb +++ b/spec/features/projects/branches_spec.rb @@ -11,15 +11,109 @@ describe 'Branches' do project.add_developer(user) end - describe 'Initial branches page' do - it 'shows all the branches sorted by last updated by default' do + context 'on the projects with 6 active branches and 4 stale branches' do + let(:project) { create(:project, :public, :empty_repo) } + let(:repository) { project.repository } + let(:threshold) { Gitlab::Git::Branch::STALE_BRANCH_THRESHOLD } + + before do + # Add 4 stale branches + (1..4).reverse_each do |i| + Timecop.freeze((threshold + i).ago) { create_file(message: "a commit in stale-#{i}", branch_name: "stale-#{i}") } + end + # Add 6 active branches + (1..6).each do |i| + Timecop.freeze((threshold - i).ago) { create_file(message: "a commit in active-#{i}", branch_name: "active-#{i}") } + end + end + + describe 'Overview page of the branches' do + it 'shows the first 5 active branches and the first 4 stale branches sorted by last updated' do + visit project_branches_path(project) + + expect(page).to have_content(sorted_branches(repository, count: 5, sort_by: :updated_desc, state: 'active')) + expect(page).to have_content(sorted_branches(repository, count: 4, sort_by: :updated_desc, state: 'stale')) + + expect(page).to have_link('Show more active branches', href: project_branches_filtered_path(project, state: 'active')) + expect(page).not_to have_content('Show more stale branches') + end + end + + describe 'Active branches page' do + it 'shows 6 active branches sorted by last updated' do + visit project_branches_filtered_path(project, state: 'active') + + expect(page).to have_content(sorted_branches(repository, count: 6, sort_by: :updated_desc, state: 'active')) + end + end + + describe 'Stale branches page' do + it 'shows 4 active branches sorted by last updated' do + visit project_branches_filtered_path(project, state: 'stale') + + expect(page).to have_content(sorted_branches(repository, count: 4, sort_by: :updated_desc, state: 'stale')) + end + end + + describe 'All branches page' do + it 'shows 10 branches sorted by last updated' do + visit project_branches_filtered_path(project, state: 'all') + + expect(page).to have_content(sorted_branches(repository, count: 10, sort_by: :updated_desc)) + end + end + + context 'with branches over more than one page' do + before do + allow(Kaminari.config).to receive(:default_per_page).and_return(5) + end + + it 'shows only default_per_page active branches sorted by last updated' do + visit project_branches_filtered_path(project, state: 'active') + + expect(page).to have_content(sorted_branches(repository, count: Kaminari.config.default_per_page, sort_by: :updated_desc, state: 'active')) + end + + it 'shows only default_per_page branches sorted by last updated on All branches' do + visit project_branches_filtered_path(project, state: 'all') + + expect(page).to have_content(sorted_branches(repository, count: Kaminari.config.default_per_page, sort_by: :updated_desc)) + end + end + end + + describe 'Find branches' do + it 'shows filtered branches', :js 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) + end + end + + describe 'Delete unprotected branch on Overview' do + it 'removes branch after confirmation', :js do + visit project_branches_filtered_path(project, state: 'all') + + expect(all('.all-branches').last).to have_selector('li', count: 20) + accept_confirm { find('.js-branch-add-pdf-text-binary .btn-remove').click } + + expect(all('.all-branches').last).to have_selector('li', count: 19) + end + end + + describe 'All branches page' do + it 'shows all the branches sorted by last updated by default' do + visit project_branches_filtered_path(project, state: 'all') + expect(page).to have_content(sorted_branches(repository, count: 20, sort_by: :updated_desc)) end it 'sorts the branches by name' do - visit project_branches_path(project) + visit project_branches_filtered_path(project, state: 'all') click_button "Last updated" # Open sorting dropdown click_link "Name" @@ -28,7 +122,7 @@ describe 'Branches' do end it 'sorts the branches by oldest updated' do - visit project_branches_path(project) + visit project_branches_filtered_path(project, state: 'all') click_button "Last updated" # Open sorting dropdown click_link "Oldest updated" @@ -41,13 +135,13 @@ describe 'Branches' do %w(one two three four five).each { |ref| repository.add_branch(user, ref, 'master') } - expect { visit project_branches_path(project) }.not_to exceed_query_limit(control_count) + expect { visit project_branches_filtered_path(project, state: 'all') }.not_to exceed_query_limit(control_count) end end - describe 'Find branches' do + describe 'Find branches on All branches' do it 'shows filtered branches', :js do - visit project_branches_path(project) + visit project_branches_filtered_path(project, state: 'all') fill_in 'branch-search', with: 'fix' find('#branch-search').native.send_keys(:enter) @@ -57,9 +151,9 @@ describe 'Branches' do end end - describe 'Delete unprotected branch' do + describe 'Delete unprotected branch on All branches' do it 'removes branch after confirmation', :js do - visit project_branches_path(project) + visit project_branches_filtered_path(project, state: 'all') fill_in 'branch-search', with: 'fix' @@ -73,6 +167,19 @@ describe 'Branches' do expect(find('.all-branches')).to have_selector('li', count: 0) end end + + context 'on project with 0 branch' do + let(:project) { create(:project, :public, :empty_repo) } + let(:repository) { project.repository } + + describe '0 branches on Overview' do + it 'shows warning' do + visit project_branches_path(project) + + expect(page).not_to have_selector('.all-branches') + end + end + end end context 'logged in as master' do @@ -83,7 +190,7 @@ describe 'Branches' do describe 'Initial branches page' do it 'shows description for admin' do - visit project_branches_path(project) + visit project_branches_filtered_path(project, state: 'all') expect(page).to have_content("Protected branches can be managed in project settings") end @@ -102,12 +209,18 @@ describe 'Branches' do end end - def sorted_branches(repository, count:, sort_by:) + def sorted_branches(repository, count:, sort_by:, state: nil) + branches = repository.branches_sorted_by(sort_by) + branches = branches.select { |b| state == 'active' ? b.active? : b.stale? } if state sorted_branches = - repository.branches_sorted_by(sort_by).first(count).map do |branch| + branches.first(count).map do |branch| Regexp.escape(branch.name) end Regexp.new(sorted_branches.join('.*')) end + + def create_file(message: 'message', branch_name:) + repository.create_file(user, generate(:branch), 'content', message: message, branch_name: branch_name) + end end diff --git a/spec/features/projects/environments/environment_spec.rb b/spec/features/projects/environments/environment_spec.rb index 64e600144e0..b233af83eec 100644 --- a/spec/features/projects/environments/environment_spec.rb +++ b/spec/features/projects/environments/environment_spec.rb @@ -234,7 +234,7 @@ feature 'Environment' do end scenario 'user deletes the branch with running environment' do - visit project_branches_path(project, search: 'feature') + visit project_branches_filtered_path(project, state: 'all', search: 'feature') remove_branch_with_hooks(project, user, 'feature') do page.within('.js-branch-feature') { find('a.btn-remove').click } diff --git a/spec/features/projects/merge_request_button_spec.rb b/spec/features/projects/merge_request_button_spec.rb index 85d518c0cc3..40689964b91 100644 --- a/spec/features/projects/merge_request_button_spec.rb +++ b/spec/features/projects/merge_request_button_spec.rb @@ -81,8 +81,8 @@ feature 'Merge Request button' do context 'on branches page' do it_behaves_like 'Merge request button only shown when allowed' do let(:label) { 'Merge request' } - let(:url) { project_branches_path(project, search: 'feature') } - let(:fork_url) { project_branches_path(forked_project, search: 'feature') } + let(:url) { project_branches_filtered_path(project, state: 'all', search: 'feature') } + let(:fork_url) { project_branches_filtered_path(forked_project, state: 'all', search: 'feature') } end end diff --git a/spec/lib/gitlab/git/branch_spec.rb b/spec/lib/gitlab/git/branch_spec.rb index 708870060e7..a19155ed5b0 100644 --- a/spec/lib/gitlab/git/branch_spec.rb +++ b/spec/lib/gitlab/git/branch_spec.rb @@ -59,5 +59,69 @@ describe Gitlab::Git::Branch, seed_helper: true do it { expect(branch.dereferenced_target.sha).to eq(SeedRepo::LastCommit::ID) } end + context 'with active, stale and future branches' do + let(:repository) do + Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') + end + + let(:user) { create(:user) } + let(:committer) do + Gitlab::Git.committer_hash(email: user.email, name: user.name) + end + let(:params) do + parents = [repository.rugged.head.target] + tree = parents.first.tree + + { + message: 'commit message', + author: committer, + committer: committer, + tree: tree, + parents: parents + } + end + let(:stale_sha) { Timecop.freeze(Gitlab::Git::Branch::STALE_BRANCH_THRESHOLD.ago - 5.days) { create_commit } } + let(:active_sha) { Timecop.freeze(Gitlab::Git::Branch::STALE_BRANCH_THRESHOLD.ago + 5.days) { create_commit } } + let(:future_sha) { Timecop.freeze(100.days.since) { create_commit } } + + before do + repository.create_branch('stale-1', stale_sha) + repository.create_branch('active-1', active_sha) + repository.create_branch('future-1', future_sha) + end + + after do + ensure_seeds + end + + describe 'examine if the branch is active or stale' do + let(:stale_branch) { repository.find_branch('stale-1') } + let(:active_branch) { repository.find_branch('active-1') } + let(:future_branch) { repository.find_branch('future-1') } + + describe '#active?' do + it { expect(stale_branch.active?).to be_falsey } + it { expect(active_branch.active?).to be_truthy } + it { expect(future_branch.active?).to be_truthy } + end + + describe '#stale?' do + it { expect(stale_branch.stale?).to be_truthy } + it { expect(active_branch.stale?).to be_falsey } + it { expect(future_branch.stale?).to be_falsey } + end + + describe '#state' do + it { expect(stale_branch.state).to eq(:stale) } + it { expect(active_branch.state).to eq(:active) } + it { expect(future_branch.state).to eq(:active) } + end + end + end + it { expect(repository.branches.size).to eq(SeedRepo::Repo::BRANCHES.size) } + + def create_commit + repository.create_commit(params.merge(committer: committer.merge(time: Time.now))) + end end |