From be9a08807e5ab72cccc03793362d43cb9a7fa767 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Fri, 20 Sep 2019 11:43:07 +0100 Subject: [PATCH 1/3] Add nav link for Instance Security Dashboard This adds a nav link for the [Instance Security Dashboard MVC][1], which is behind the `security_dashboard` feature flag. This also refactors slightly how the visibility of the Operations and Environments dashboard links is handled. One result of this is that they are now consistently visible/hidden between the dashboards dropdown on larger screens, and the "More" dropdown on smaller screens. Finally, the partials have been renamed, since they're no longer specific to operations. [1]: https://gitlab.com/gitlab-org/gitlab/issues/6953 --- app/views/layouts/nav/_dashboard.html.haml | 4 ++-- ee/app/helpers/ee/dashboard_helper.rb | 12 +++++++++++ .../{operations => }/_nav_link.html.haml | 6 +++--- .../views/dashboard/_nav_link_list.html.haml | 9 +++++++++ .../operations/_nav_link_list.html.haml | 5 ----- ee/spec/helpers/ee/dashboard_helper_spec.rb | 20 +++++++++++++++++++ 6 files changed, 46 insertions(+), 10 deletions(-) rename ee/app/views/dashboard/{operations => }/_nav_link.html.haml (55%) create mode 100644 ee/app/views/dashboard/_nav_link_list.html.haml delete mode 100644 ee/app/views/dashboard/operations/_nav_link_list.html.haml diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index 7b0824ae2af426..4b83239dfbde71 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -59,7 +59,7 @@ = render_if_exists 'layouts/nav/sidebar/analytics_more_link' %li.dropdown.d-lg-none - = render_if_exists 'dashboard/operations/nav_link_list' + = render_if_exists 'dashboard/nav_link_list' - if can?(current_user, :read_instance_statistics) = nav_link(controller: [:conversational_development_index, :cohorts], html_options: { class: 'd-lg-none' }) do = link_to instance_statistics_root_path do @@ -86,7 +86,7 @@ = _('Web IDE') %li.dropdown{ class: 'd-none d-lg-block' } - = render_if_exists 'dashboard/operations/nav_link' + = render_if_exists 'dashboard/nav_link' - if can?(current_user, :read_instance_statistics) = nav_link(controller: [:conversational_development_index, :cohorts], html_options: { class: "d-none d-lg-block d-xl-block"}) do = link_to instance_statistics_root_path, title: _('Instance Statistics'), aria: { label: _('Instance Statistics') }, data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do diff --git a/ee/app/helpers/ee/dashboard_helper.rb b/ee/app/helpers/ee/dashboard_helper.rb index 0474888585347d..19c4efc891a4c0 100644 --- a/ee/app/helpers/ee/dashboard_helper.rb +++ b/ee/app/helpers/ee/dashboard_helper.rb @@ -37,6 +37,18 @@ def has_start_trial? def get_dashboard_nav_links super.tap do |links| links << :analytics if ::Gitlab::Analytics.any_features_enabled? + + if can?(current_user, :read_operations_dashboard) + if ::Feature.enabled?(:environments_dashboard) + links << :environments + end + + links << :operations + end + + if ::Feature.enabled?(:security_dashboard) && can?(current_user, :read_security_dashboard) + links << :security + end end end end diff --git a/ee/app/views/dashboard/operations/_nav_link.html.haml b/ee/app/views/dashboard/_nav_link.html.haml similarity index 55% rename from ee/app/views/dashboard/operations/_nav_link.html.haml rename to ee/app/views/dashboard/_nav_link.html.haml index 2e2249a0a0d90d..f75462b418ee25 100644 --- a/ee/app/views/dashboard/operations/_nav_link.html.haml +++ b/ee/app/views/dashboard/_nav_link.html.haml @@ -1,8 +1,8 @@ -- if can?(current_user, :read_operations_dashboard) - %button#js-dashboards-menu.btn-link{ type: 'button', data: { toggle: 'dropdown' }, 'aria-label': _('Operations Dashboard'), 'aria-haspopup': true, 'aria-expanded': false } +- if any_dashboard_nav_link?([:environments, :operations, :security]) + %button#js-dashboards-menu.btn-link{ type: 'button', data: { toggle: 'dropdown' }, 'aria-label': _('Dashboards'), 'aria-haspopup': true, 'aria-expanded': false } = sprite_icon('dashboard', size: 18) = sprite_icon('angle-down', css_class: 'caret-down') .dropdown-menu{ 'aria-labelledby': "js-dashboards-menu" } .dropdown-bold-header = _('Dashboards') - = render_if_exists 'dashboard/operations/nav_link_list' + = render_if_exists 'dashboard/nav_link_list' diff --git a/ee/app/views/dashboard/_nav_link_list.html.haml b/ee/app/views/dashboard/_nav_link_list.html.haml new file mode 100644 index 00000000000000..f9bf02a9d2470a --- /dev/null +++ b/ee/app/views/dashboard/_nav_link_list.html.haml @@ -0,0 +1,9 @@ +- if dashboard_nav_link?(:environments) + = link_to operations_environments_path, class: 'dropdown-item' do + = _('Environments') +- if dashboard_nav_link?(:operations) + = link_to operations_path, class: 'dropdown-item' do + = _('Operations') +- if dashboard_nav_link?(:security) + = link_to security_path, class: 'dropdown-item' do + = _('Security') diff --git a/ee/app/views/dashboard/operations/_nav_link_list.html.haml b/ee/app/views/dashboard/operations/_nav_link_list.html.haml deleted file mode 100644 index 3d9cd252a957e8..00000000000000 --- a/ee/app/views/dashboard/operations/_nav_link_list.html.haml +++ /dev/null @@ -1,5 +0,0 @@ -- if Feature.enabled?('environments_dashboard') - = link_to operations_environments_path, class: 'dropdown-item' do - = _('Environments') -= link_to operations_path, class: 'dropdown-item' do - = _('Operations') diff --git a/ee/spec/helpers/ee/dashboard_helper_spec.rb b/ee/spec/helpers/ee/dashboard_helper_spec.rb index 5980454d0dfddd..ba9277a1f1dc80 100644 --- a/ee/spec/helpers/ee/dashboard_helper_spec.rb +++ b/ee/spec/helpers/ee/dashboard_helper_spec.rb @@ -32,6 +32,26 @@ expect(helper.dashboard_nav_links).not_to include(:analytics) end end + + context 'when the instance security dashboard feature is disabled' do + before do + stub_feature_flags(security_dashboard: false) + end + + it 'does not include security' do + expect(helper.dashboard_nav_links).not_to include(:security) + end + end + + context 'when the instance security dashboard feature is enabled' do + before do + stub_feature_flags(security_dashboard: true) + end + + it 'includes security' do + expect(helper.dashboard_nav_links).to include(:security) + end + end end describe '.has_start_trial?' do -- GitLab From 920e5ea57806f419eac5a52530620e2c73b43358 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Tue, 24 Sep 2019 11:00:36 +0100 Subject: [PATCH 2/3] Reduce code block nesting --- ee/app/helpers/ee/dashboard_helper.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ee/app/helpers/ee/dashboard_helper.rb b/ee/app/helpers/ee/dashboard_helper.rb index 19c4efc891a4c0..3fa2970abe54bc 100644 --- a/ee/app/helpers/ee/dashboard_helper.rb +++ b/ee/app/helpers/ee/dashboard_helper.rb @@ -39,10 +39,7 @@ def get_dashboard_nav_links links << :analytics if ::Gitlab::Analytics.any_features_enabled? if can?(current_user, :read_operations_dashboard) - if ::Feature.enabled?(:environments_dashboard) - links << :environments - end - + links << :environments if ::Feature.enabled?(:environments_dashboard) links << :operations end -- GitLab From bd9c3e9b5698174c2898c7276d368cc4cf9cdbea Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Tue, 24 Sep 2019 11:04:37 +0100 Subject: [PATCH 3/3] Add tests for operations and environments --- ee/spec/helpers/ee/dashboard_helper_spec.rb | 78 +++++++++++++-------- 1 file changed, 49 insertions(+), 29 deletions(-) diff --git a/ee/spec/helpers/ee/dashboard_helper_spec.rb b/ee/spec/helpers/ee/dashboard_helper_spec.rb index ba9277a1f1dc80..95b9aff5e7f020 100644 --- a/ee/spec/helpers/ee/dashboard_helper_spec.rb +++ b/ee/spec/helpers/ee/dashboard_helper_spec.rb @@ -7,49 +7,69 @@ let(:user) { build(:user) } - before do - allow(helper).to receive(:current_user).and_return(user) - allow(helper).to receive(:can?) { true } - end - describe '#dashboard_nav_links' do - context 'when at least one analytics feature is enabled' do - before do - enable_only_one_analytics_feature_flag - end - - it 'includes analytics' do - expect(helper.dashboard_nav_links).to include(:analytics) - end + before do + allow(helper).to receive(:current_user).and_return(user) end - context 'when all analytics features are disabled' do + describe 'analytics' do before do - disable_all_analytics_feature_flags + allow(helper).to receive(:can?) { true } end - it 'does not include analytics' do - expect(helper.dashboard_nav_links).not_to include(:analytics) - end - end + context 'when at least one analytics feature is enabled' do + before do + enable_only_one_analytics_feature_flag + end - context 'when the instance security dashboard feature is disabled' do - before do - stub_feature_flags(security_dashboard: false) + it 'includes analytics' do + expect(helper.dashboard_nav_links).to include(:analytics) + end end - it 'does not include security' do - expect(helper.dashboard_nav_links).not_to include(:security) + context 'when all analytics features are disabled' do + before do + disable_all_analytics_feature_flags + end + + it 'does not include analytics' do + expect(helper.dashboard_nav_links).not_to include(:analytics) + end end end - context 'when the instance security dashboard feature is enabled' do - before do - stub_feature_flags(security_dashboard: true) + describe 'operations, environments and security' do + using RSpec::Parameterized::TableSyntax + + where(:ability, :feature_flag, :nav_link) do + :read_operations_dashboard | nil | :operations + :read_operations_dashboard | :environments_dashboard | :environments + :read_security_dashboard | :security_dashboard | :security end - it 'includes security' do - expect(helper.dashboard_nav_links).to include(:security) + with_them do + describe 'when the feature is enabled' do + before do + stub_feature_flags(feature_flag => true) unless feature_flag.nil? + allow(helper).to receive(:can?).and_return(false) + allow(helper).to receive(:can?).with(user, ability).and_return(true) + end + + it 'includes the nav link' do + expect(helper.dashboard_nav_links).to include(nav_link) + end + end + + describe 'when the feature is disabled' do + before do + stub_feature_flags(feature_flag => false) unless feature_flag.nil? + allow(helper).to receive(:can?).and_return(false) + end + + it 'does not include the nav link' do + expect(helper.dashboard_nav_links).not_to include(nav_link) + end + end end end end -- GitLab