From 868cb4307f93b2f8fa0d25d9e47e632d0855881e Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 23 Feb 2018 12:10:57 +0000 Subject: Fix subgroup issue and MR pages empty states and counts Previously, these wouldn't count issues or MRs in subgroups - meaning that if _this_ group had no issues or MRs, we'd show the empty state, which was wrong. --- app/controllers/groups/application_controller.rb | 4 - app/controllers/groups_controller.rb | 1 - app/helpers/groups_helper.rb | 18 ++- app/views/groups/issues.html.haml | 7 +- app/views/groups/merge_requests.html.haml | 2 +- app/views/layouts/nav/sidebar/_group.html.haml | 5 +- ...sts-and-issues-don-t-show-for-all-subgroups.yml | 6 + spec/features/groups/empty_states_spec.rb | 124 ++++++++++++++------- 8 files changed, 107 insertions(+), 60 deletions(-) create mode 100644 changelogs/unreleased/43510-merge-requests-and-issues-don-t-show-for-all-subgroups.yml diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb index 4a2bfc1f887..9f3bb60b4cc 100644 --- a/app/controllers/groups/application_controller.rb +++ b/app/controllers/groups/application_controller.rb @@ -18,10 +18,6 @@ class Groups::ApplicationController < ApplicationController @projects ||= GroupProjectsFinder.new(group: group, current_user: current_user).execute end - def group_merge_requests - @group_merge_requests = MergeRequestsFinder.new(current_user, group_id: @group.id).execute - end - def authorize_admin_group! unless can?(current_user, :admin_group, group) return render_404 diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 14b9d6c22bd..283c3e5f1e0 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -14,7 +14,6 @@ class GroupsController < Groups::ApplicationController before_action :authorize_create_group!, only: [:new] before_action :group_projects, only: [:projects, :activity, :issues, :merge_requests] - before_action :group_merge_requests, only: [:merge_requests] before_action :event_filter, only: [:activity] before_action :user_actions, only: [:show, :subgroups] diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index 5fbaa17c40e..7910de73c52 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -19,6 +19,20 @@ module GroupsHelper can?(current_user, :change_share_with_group_lock, group) end + def group_issues_count(state:) + IssuesFinder + .new(current_user, group_id: @group.id, state: state, non_archived: true, include_subgroups: true) + .execute + .count + end + + def group_merge_requests_count(state:) + MergeRequestsFinder + .new(current_user, group_id: @group.id, state: state, non_archived: true, include_subgroups: true) + .execute + .count + end + def group_icon(group, options = {}) img_path = group_icon_url(group, options) image_tag img_path, options @@ -77,10 +91,6 @@ module GroupsHelper end end - def group_issues(group) - IssuesFinder.new(current_user, group_id: group.id).execute - end - def remove_group_message(group) _("You are going to remove %{group_name}. Removed groups CANNOT be restored! Are you ABSOLUTELY sure?") % { group_name: group.name } diff --git a/app/views/groups/issues.html.haml b/app/views/groups/issues.html.haml index f2ae7c52031..ca3f018c5e6 100644 --- a/app/views/groups/issues.html.haml +++ b/app/views/groups/issues.html.haml @@ -1,12 +1,13 @@ - page_title "Issues" -- group_issues_exists = group_issues(@group).exists? = content_for :meta_tags do = auto_discovery_link_tag(:atom, params.merge(rss_url_options), title: "#{@group.name} issues") - content_for :page_specific_javascripts do = webpack_bundle_tag 'common_vue' -- if group_issues_exists +- if group_issues_count(state: 'all').zero? + = render 'shared/empty_states/issues', project_select_button: true +- else .top-area = render 'shared/issuable/nav', type: :issues .nav-controls @@ -19,5 +20,3 @@ = render 'shared/issuable/search_bar', type: :issues = render 'shared/issues' -- else - = render 'shared/empty_states/issues', project_select_button: true diff --git a/app/views/groups/merge_requests.html.haml b/app/views/groups/merge_requests.html.haml index 046b92bd9fb..520fe217ae1 100644 --- a/app/views/groups/merge_requests.html.haml +++ b/app/views/groups/merge_requests.html.haml @@ -3,7 +3,7 @@ - content_for :page_specific_javascripts do = webpack_bundle_tag 'common_vue' -- if @group_merge_requests.empty? +- if group_merge_requests_count(state: 'all').zero? = render 'shared/empty_states/merge_requests', project_select_button: true - else .top-area diff --git a/app/views/layouts/nav/sidebar/_group.html.haml b/app/views/layouts/nav/sidebar/_group.html.haml index 47ae79b7a69..b520f28123f 100644 --- a/app/views/layouts/nav/sidebar/_group.html.haml +++ b/app/views/layouts/nav/sidebar/_group.html.haml @@ -1,6 +1,5 @@ -- issues_count = IssuesFinder.new(current_user, group_id: @group.id, state: 'opened').execute.count -- merge_requests_count = MergeRequestsFinder.new(current_user, group_id: @group.id, state: 'opened', non_archived: true).execute.count - +- issues_count = group_issues_count(state: 'opened') +- merge_requests_count = group_merge_requests_count(state: 'opened') - issues_sub_menu_items = ['groups#issues', 'labels#index', 'milestones#index'] .nav-sidebar{ class: ("sidebar-collapsed-desktop" if collapsed_sidebar?) } diff --git a/changelogs/unreleased/43510-merge-requests-and-issues-don-t-show-for-all-subgroups.yml b/changelogs/unreleased/43510-merge-requests-and-issues-don-t-show-for-all-subgroups.yml new file mode 100644 index 00000000000..e163c04f430 --- /dev/null +++ b/changelogs/unreleased/43510-merge-requests-and-issues-don-t-show-for-all-subgroups.yml @@ -0,0 +1,6 @@ +--- +title: Ensure group issues and merge requests pages show results from subgroups when + there are no results from the current group +merge_request: 17312 +author: +type: fixed diff --git a/spec/features/groups/empty_states_spec.rb b/spec/features/groups/empty_states_spec.rb index 243e8536168..04217fec06c 100644 --- a/spec/features/groups/empty_states_spec.rb +++ b/spec/features/groups/empty_states_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -feature 'Groups Merge Requests Empty States' do +feature 'Group empty states' do let(:group) { create(:group) } let(:user) { create(:group_member, :developer, user: create(:user), group: group ).user } @@ -8,62 +8,100 @@ feature 'Groups Merge Requests Empty States' do sign_in(user) end - context 'group has a project' do - let(:project) { create(:project, namespace: group) } + [:issue, :merge_request].each do |issuable| + issuable_name = issuable.to_s.humanize.downcase + project_relation = issuable == :issue ? :project : :source_project - before do - project.add_master(user) - end + context "for #{issuable_name}s" do + let(:path) { public_send(:"#{issuable}s_group_path", group) } - context 'the project has a merge request' do - before do - create(:merge_request, source_project: project) + context 'group has a project' do + let(:project) { create(:project, namespace: group) } - visit merge_requests_group_path(group) - end + before do + project.add_master(user) + end - it 'should not display an empty state' do - expect(page).not_to have_selector('.empty-state') - end - end + context "the project has #{issuable_name}s" do + before do + create(issuable, project_relation => project) - context 'the project has no merge requests', :js do - before do - visit merge_requests_group_path(group) - end + visit path + end - it 'should display an empty state' do - expect(page).to have_selector('.empty-state') - end + it 'does not display an empty state' do + expect(page).not_to have_selector('.empty-state') + end + end + + context "the project has no #{issuable_name}s", :js do + before do + visit path + end + + it 'displays an empty state' do + expect(page).to have_selector('.empty-state') + end + + it "shows a new #{issuable_name} button" do + within '.empty-state' do + expect(page).to have_content("create #{issuable_name}") + end + end + + it "the new #{issuable_name} button opens a project dropdown" do + within '.empty-state' do + find('.new-project-item-select-button').click + end - it 'should show a new merge request button' do - within '.empty-state' do - expect(page).to have_content('create merge request') + expect(page).to have_selector('.ajax-project-dropdown') + end end end - it 'the new merge request button opens a project dropdown' do - within '.empty-state' do - find('.new-project-item-select-button').click - end + context 'group without a project' do + context 'group has a subgroup', :nested_groups do + let(:subgroup) { create(:group, parent: group) } + let(:subgroup_project) { create(:project, namespace: subgroup) } - expect(page).to have_selector('.ajax-project-dropdown') - end - end - end + context "the project has #{issuable_name}s" do + before do + create(issuable, project_relation => subgroup_project) - context 'group without a project' do - before do - visit merge_requests_group_path(group) - end + visit path + end - it 'should display an empty state' do - expect(page).to have_selector('.empty-state') - end + it 'does not display an empty state' do + expect(page).not_to have_selector('.empty-state') + end + end - it 'should not show a new merge request button' do - within '.empty-state' do - expect(page).not_to have_link('create merge request') + context "the project has no #{issuable_name}s" do + before do + visit path + end + + it 'displays an empty state' do + expect(page).to have_selector('.empty-state') + end + end + end + + context 'group has no subgroups' do + before do + visit path + end + + it 'displays an empty state' do + expect(page).to have_selector('.empty-state') + end + + it "shows a new #{issuable_name} button" do + within '.empty-state' do + expect(page).not_to have_link("create #{issuable_name}") + end + end + end end end end -- cgit v1.2.1