Fix bug in sidebar when menu item cannot be rendered

When a user is rendering the project sidebar and any menu item
is not possible to render, we return a `nil` value.

This introduces an edge case when we want to insert another menu item
before or after that menu, because it was returned as `nil` and,
therefore not introduced in the menu item list.

In this MR, we switched to a Null object pattern, in which we return
a nil menu item instead of just a nil value.
parent 3036a86c
......@@ -11,5 +11,5 @@
- if sidebar.render_raw_menus_partial
= render sidebar.render_raw_menus_partial
= render partial: 'shared/nav/sidebar_hidden_menu_item', collection: sidebar.hidden_menu&.items
= render partial: 'shared/nav/sidebar_hidden_menu_item', collection: sidebar.hidden_menu&.renderable_items
= render 'shared/sidebar_toggle_button'
......@@ -13,7 +13,7 @@
%span.badge.badge-pill.count{ **sidebar_menu.pill_html_options }
= number_with_delimiter(sidebar_menu.pill_count)
%ul.sidebar-sub-level-items{ class: ('is-fly-out-only' unless sidebar_menu.has_items?) }
%ul.sidebar-sub-level-items{ class: ('is-fly-out-only' unless sidebar_menu.has_renderable_items?) }
= nav_link(**sidebar_menu.all_active_routes, html_options: { class: 'fly-out-top-item' } ) do
= link_to sidebar_menu.link, **sidebar_menu.collapsed_container_html_options do
%strong.fly-out-top-item-name
......@@ -24,4 +24,4 @@
- if sidebar_menu.has_items?
%li.divider.fly-out-top-item
= render partial: 'shared/nav/sidebar_menu_item', collection: sidebar_menu.items
= render partial: 'shared/nav/sidebar_menu_item', collection: sidebar_menu.renderable_items
......@@ -31,7 +31,7 @@ module EE
return audit_events_menu_item.link if audit_events_menu_item
return dependencies_menu_item.link if dependencies_menu_item
items.first.link
renderable_items.first.link
end
override :render?
......
......@@ -11,7 +11,7 @@ RSpec.describe Sidebars::Projects::Menus::AnalyticsMenu do
subject { described_class.new(context) }
describe 'Menu items' do
subject { described_class.new(context).items.index { |e| e.item_id == item_id } }
subject { described_class.new(context).renderable_items.index { |e| e.item_id == item_id } }
describe 'Code Review' do
let(:item_id) { :code_review }
......
......@@ -8,7 +8,7 @@ RSpec.describe Sidebars::Projects::Menus::CiCdMenu do
let(:context) { Sidebars::Projects::Context.new(current_user: user, container: project, can_view_pipeline_editor: true) }
describe 'Test Cases' do
subject { described_class.new(context).items.index { |e| e.item_id == :test_cases} }
subject { described_class.new(context).renderable_items.index { |e| e.item_id == :test_cases} }
context 'when licensed feature quality_management is not enabled' do
before do
......
......@@ -8,7 +8,7 @@ RSpec.describe Sidebars::Projects::Menus::IssuesMenu do
let(:context) { Sidebars::Projects::Context.new(current_user: user, container: project) }
describe 'Iterations' do
subject { described_class.new(context).items.index { |e| e.item_id == :iterations} }
subject { described_class.new(context).renderable_items.index { |e| e.item_id == :iterations} }
context 'when licensed feature iterations is not enabled' do
it 'does not include iterations menu item' do
......
......@@ -7,8 +7,11 @@ RSpec.describe Sidebars::Projects::Menus::OperationsMenu do
let(:user) { project.owner }
let(:context) { Sidebars::Projects::Context.new(current_user: user, container: project, show_cluster_hint: true) }
describe 'Menu items' do
subject { described_class.new(context).renderable_items.index { |e| e.item_id == item_id } }
describe 'On-call Schedules' do
subject { described_class.new(context).items.index { |e| e.item_id == :on_call_schedules } }
let(:item_id) { :on_call_schedules }
before do
stub_licensed_features(oncall_schedules: true)
......@@ -24,7 +27,7 @@ RSpec.describe Sidebars::Projects::Menus::OperationsMenu do
end
describe 'Escalation policies' do
subject { described_class.new(context).items.index { |e| e.item_id == :escalation_policies } }
let(:item_id) { :escalation_policies }
before do
stub_licensed_features(oncall_schedules: true, escalation_policies: true)
......@@ -38,4 +41,5 @@ RSpec.describe Sidebars::Projects::Menus::OperationsMenu do
specify { is_expected.to be_nil }
end
end
end
end
......@@ -9,7 +9,7 @@ RSpec.describe Sidebars::Projects::Menus::RepositoryMenu do
let(:context) { Sidebars::Projects::Context.new(current_user: user, container: project, current_ref: 'master') }
describe 'File Locks' do
subject { described_class.new(context).items.index { |e| e.item_id == :file_locks} }
subject { described_class.new(context).renderable_items.index { |e| e.item_id == :file_locks} }
context 'when licensed feature file locks is not enabled' do
it 'does not include file locks menu item' do
......
......@@ -62,13 +62,18 @@ RSpec.describe Sidebars::Projects::Menus::SecurityComplianceMenu do
let(:user) { nil }
it 'returns the link to the discover security path', :aggregate_failures do
expect(subject.items).to be_empty
expect(subject.renderable_items).to be_empty
expect(subject.link).to eq("/#{project.full_path}/-/security/discover")
end
end
end
describe 'Menu items' do
subject { described_class.new(context).renderable_items.find { |i| i.item_id == item_id } }
describe 'Configuration' do
let(:item_id) { :configuration }
describe '#sidebar_security_configuration_paths' do
let(:expected_security_configuration_paths) do
%w[
......@@ -83,8 +88,6 @@ RSpec.describe Sidebars::Projects::Menus::SecurityComplianceMenu do
]
end
subject { described_class.new(context).items.find { |i| i.item_id == :configuration } }
it 'includes all the security configuration paths' do
expect(subject.active_routes[:path]).to eq expected_security_configuration_paths
end
......@@ -92,7 +95,7 @@ RSpec.describe Sidebars::Projects::Menus::SecurityComplianceMenu do
end
describe 'Security Dashboard' do
subject { described_class.new(context).items.find { |i| i.item_id == :dashboard } }
let(:item_id) { :dashboard }
before do
stub_licensed_features(security_dashboard: true)
......@@ -110,7 +113,7 @@ RSpec.describe Sidebars::Projects::Menus::SecurityComplianceMenu do
end
describe 'Vulnerability Report' do
subject { described_class.new(context).items.find { |i| i.item_id == :vulnerability_report } }
let(:item_id) { :vulnerability_report }
before do
stub_licensed_features(security_dashboard: true)
......@@ -128,7 +131,7 @@ RSpec.describe Sidebars::Projects::Menus::SecurityComplianceMenu do
end
describe 'On Demand Scans' do
subject { described_class.new(context).items.find { |i| i.item_id == :on_demand_scans } }
let(:item_id) { :on_demand_scans }
before do
stub_licensed_features(security_on_demand_scans: true)
......@@ -146,7 +149,7 @@ RSpec.describe Sidebars::Projects::Menus::SecurityComplianceMenu do
end
describe 'Dependency List' do
subject { described_class.new(context).items.find { |i| i.item_id == :dependency_list } }
let(:item_id) { :dependency_list }
before do
stub_licensed_features(dependency_scanning: true)
......@@ -164,7 +167,7 @@ RSpec.describe Sidebars::Projects::Menus::SecurityComplianceMenu do
end
describe 'License Compliance' do
subject { described_class.new(context).items.find { |i| i.item_id == :license_compliance } }
let(:item_id) { :license_compliance }
before do
stub_licensed_features(license_scanning: true)
......@@ -182,7 +185,7 @@ RSpec.describe Sidebars::Projects::Menus::SecurityComplianceMenu do
end
describe 'Threat monitoring' do
subject { described_class.new(context).items.find { |i| i.item_id == :threat_monitoring } }
let(:item_id) { :threat_monitoring }
before do
stub_licensed_features(threat_monitoring: true)
......@@ -200,7 +203,7 @@ RSpec.describe Sidebars::Projects::Menus::SecurityComplianceMenu do
end
describe 'Scan Policies' do
subject { described_class.new(context).items.find { |i| i.item_id == :scan_policies } }
let(:item_id) { :scan_policies }
context 'when feature flag :security_orchestration_policies_configuration is enabled' do
before do
......@@ -229,7 +232,7 @@ RSpec.describe Sidebars::Projects::Menus::SecurityComplianceMenu do
end
describe 'Audit Events' do
subject { described_class.new(context).items.find { |i| i.item_id == :audit_events } }
let(:item_id) { :audit_events }
context 'when user can access audit events' do
it { is_expected.not_to be_nil }
......@@ -265,4 +268,5 @@ RSpec.describe Sidebars::Projects::Menus::SecurityComplianceMenu do
it { is_expected.to be_nil }
end
end
end
end
......@@ -37,9 +37,9 @@ RSpec.describe Sidebars::Projects::Menus::JiraMenu do
end
it 'contains issue list and open jira menu items' do
expect(subject.items).not_to be_empty
expect(subject.items[0].item_id).to eq :issue_list
expect(subject.items[1].item_id).to eq :open_jira
expect(subject.renderable_items).not_to be_empty
expect(subject.renderable_items[0].item_id).to eq :issue_list
expect(subject.renderable_items[1].item_id).to eq :open_jira
end
end
end
......
......@@ -33,7 +33,7 @@ RSpec.describe Sidebars::Projects::Menus::RequirementsMenu do
end
it 'does not contain any menu item' do
expect(subject.items).to be_empty
expect(subject.renderable_items).to be_empty
end
end
......@@ -47,7 +47,7 @@ RSpec.describe Sidebars::Projects::Menus::RequirementsMenu do
end
it 'contains list menu item' do
expect(subject.items[0].item_id).to eq :requirements_list
expect(subject.renderable_items[0].item_id).to eq :requirements_list
end
end
end
......
......@@ -13,7 +13,7 @@ module Sidebars
include ::Sidebars::Concerns::ContainerWithHtmlOptions
include ::Sidebars::Concerns::HasActiveRoutes
attr_reader :context, :items
attr_reader :context
delegate :current_user, :container, to: :@context
def initialize(context)
......@@ -29,7 +29,7 @@ module Sidebars
override :render?
def render?
has_items?
has_renderable_items?
end
# Menus might have or not a link
......@@ -43,7 +43,7 @@ module Sidebars
# This method filters the information and returns: { path: ['foo', 'bar'], controller: :foo }
def all_active_routes
@all_active_routes ||= begin
([active_routes] + items.map(&:active_routes)).flatten.each_with_object({}) do |pairs, hash|
([active_routes] + renderable_items.map(&:active_routes)).flatten.each_with_object({}) do |pairs, hash|
pairs.each do |k, v|
hash[k] ||= []
hash[k] += Array(v)
......@@ -55,10 +55,22 @@ module Sidebars
end
end
# Returns whether the menu has any menu item, no
# matter whether it is renderable or not
def has_items?
@items.any?
end
# Returns all renderable menu items
def renderable_items
@renderable_items ||= @items.select(&:render?)
end
# Returns whether the menu has any renderable menu item
def has_renderable_items?
renderable_items.any?
end
def add_item(item)
add_element(@items, item)
end
......
......@@ -18,5 +18,9 @@ module Sidebars
def show_hint?
hint_html_options.present?
end
def render?
true
end
end
end
# frozen_string_literal: true
module Sidebars
class NilMenuItem < MenuItem
extend ::Gitlab::Utils::Override
def initialize(item_id:)
super(item_id: item_id, title: nil, link: nil, active_routes: {})
end
override :render?
def render?
false
end
end
end
......@@ -21,7 +21,7 @@ module Sidebars
def link
return cycle_analytics_menu_item.link if cycle_analytics_menu_item
items.first.link
renderable_items.first.link
end
override :extra_container_html_options
......
......@@ -15,7 +15,7 @@ module Sidebars
override :link
def link
items.first.link
renderable_items.first.link
end
override :title
......
......@@ -5,15 +5,18 @@ require 'spec_helper'
RSpec.describe Sidebars::Menu do
let(:menu) { described_class.new(context) }
let(:context) { Sidebars::Context.new(current_user: nil, container: nil) }
let(:nil_menu_item) { Sidebars::NilMenuItem.new(item_id: :foo) }
describe '#all_active_routes' do
it 'gathers all active routes of items and the current menu' do
menu.add_item(Sidebars::MenuItem.new(title: 'foo1', link: 'foo1', active_routes: { path: %w(bar test) }))
menu.add_item(Sidebars::MenuItem.new(title: 'foo2', link: 'foo2', active_routes: { controller: 'fooc' }))
menu.add_item(Sidebars::MenuItem.new(title: 'foo3', link: 'foo3', active_routes: { controller: 'barc' }))
menu.add_item(nil_menu_item)
allow(menu).to receive(:active_routes).and_return({ path: 'foo' })
expect(menu).to receive(:renderable_items).and_call_original
expect(menu.all_active_routes).to eq({ path: %w(foo bar test), controller: %w(fooc barc) })
end
end
......@@ -31,6 +34,60 @@ RSpec.describe Sidebars::Menu do
expect(menu.render?).to be true
end
context 'when menu items are NilMenuItem' do
it 'returns false' do
menu.add_item(nil_menu_item)
expect(menu.render?).to be false
end
end
end
end
describe '#has_items?' do
it 'returns true when there are regular menu items' do
menu.add_item(Sidebars::MenuItem.new(title: 'foo1', link: 'foo1', active_routes: {}))
expect(menu.has_items?).to be true
end
it 'returns true when there are nil menu items' do
menu.add_item(nil_menu_item)
expect(menu.has_items?).to be true
end
end
describe '#has_renderable_items?' do
it 'returns true when there are regular menu items' do
menu.add_item(Sidebars::MenuItem.new(title: 'foo1', link: 'foo1', active_routes: {}))
expect(menu.has_renderable_items?).to be true
end
it 'returns false when there are nil menu items' do
menu.add_item(nil_menu_item)
expect(menu.has_renderable_items?).to be false
end
it 'returns true when there are both regular and nil menu items' do
menu.add_item(Sidebars::MenuItem.new(title: 'foo1', link: 'foo1', active_routes: {}))
menu.add_item(nil_menu_item)
expect(menu.has_renderable_items?).to be true
end
end
describe '#renderable_items' do
it 'returns only regular menu items' do
item = Sidebars::MenuItem.new(title: 'foo1', link: 'foo1', active_routes: {})
menu.add_item(item)
menu.add_item(nil_menu_item)
expect(menu.renderable_items.size).to eq 1
expect(menu.renderable_items.first).to eq item
end
end
......
......@@ -26,7 +26,7 @@ RSpec.describe Sidebars::Projects::Menus::AnalyticsMenu do
context 'when menu does not have any menu items' do
it 'returns false' do
allow(subject).to receive(:has_items?).and_return(false)
allow(subject).to receive(:has_renderable_items?).and_return(false)
expect(subject.render?).to be false
end
......@@ -49,13 +49,13 @@ RSpec.describe Sidebars::Projects::Menus::AnalyticsMenu do
it 'returns link to the the first visible menu item' do
allow(subject).to receive(:cycle_analytics_menu_item).and_return(nil)
expect(subject.link).to eq subject.items.first.link
expect(subject.link).to eq subject.renderable_items.first.link
end
end
end
describe 'Menu items' do
subject { described_class.new(context).items.index { |e| e.item_id == item_id } }
subject { described_class.new(context).renderable_items.index { |e| e.item_id == item_id } }
describe 'CI/CD' do
let(:item_id) { :ci_cd_analytics }
......
......@@ -26,8 +26,11 @@ RSpec.describe Sidebars::Projects::Menus::CiCdMenu do
end
end
describe 'Menu items' do
subject { described_class.new(context).renderable_items.index { |e| e.item_id == item_id } }
describe 'Pipelines Editor' do
subject { described_class.new(context).items.index { |e| e.item_id == :pipelines_editor } }
let(:item_id) { :pipelines_editor }
context 'when user cannot view pipeline editor' do
let(:can_view_pipeline_editor) { false }
......@@ -45,7 +48,7 @@ RSpec.describe Sidebars::Projects::Menus::CiCdMenu do
end
describe 'Artifacts' do
subject { described_class.new(context).items.index { |e| e.item_id == :artifacts } }
let(:item_id) { :artifacts }
context 'when feature flag :artifacts_management_page is disabled' do
it 'does not include artifacts menu item' do
......@@ -63,4 +66,5 @@ RSpec.describe Sidebars::Projects::Menus::CiCdMenu do
end
end
end
end
end
......@@ -36,7 +36,7 @@ RSpec.describe Sidebars::Projects::Menus::ConfluenceMenu do
end
it 'does not contain any sub menu' do
expect(subject.items).to be_empty
expect(subject.has_items?).to be false
end
end
end
......
......@@ -11,7 +11,7 @@ RSpec.describe Sidebars::Projects::Menus::ExternalIssueTrackerMenu do
subject { described_class.new(context) }
it 'does not contain any sub menu' do
expect(subject.items).to be_empty
expect(subject.has_items?).to be false
end
describe '#render?' do
......
......@@ -10,7 +10,7 @@ RSpec.describe Sidebars::Projects::Menus::ExternalWikiMenu do
subject { described_class.new(context) }
it 'does not contain any sub menu' do
expect(subject.items).to be_empty
expect(subject.has_items?).to be false
end
describe '#render?' do
......
......@@ -13,7 +13,7 @@ RSpec.describe Sidebars::Projects::Menus::HiddenMenu do
context 'when menu does not have any menu items' do
it 'returns false' do
allow(subject).to receive(:has_items?).and_return(false)
allow(subject).to receive(:has_renderable_items?).and_return(false)
expect(subject.render?).to be false
end
......@@ -27,7 +27,7 @@ RSpec.describe Sidebars::Projects::Menus::HiddenMenu do
end
describe 'Menu items' do
subject { described_class.new(context).items.index { |e| e.item_id == item_id } }
subject { described_class.new(context).renderable_items.index { |e| e.item_id == item_id } }
shared_examples 'access rights checks' do
specify { is_expected.not_to be_nil }
......
......@@ -10,7 +10,7 @@ RSpec.describe Sidebars::Projects::Menus::LabelsMenu do
subject { described_class.new(context) }
it 'does not contain any sub menu' do
expect(subject.items).to be_empty
expect(subject.has_items?).to eq false
end
describe '#render?' do
......
......@@ -19,7 +19,7 @@ RSpec.describe Sidebars::Projects::Menus::LearnGitlabMenu do
subject { described_class.new(context) }
it 'does not contain any sub menu' do
expect(subject.instance_variable_get(:@items)).to be_empty
expect(subject.has_items?).to be false
end
describe '#nav_link_html_options' do
......
......@@ -21,9 +21,9 @@ RSpec.describe Sidebars::Projects::Menus::OperationsMenu do
end
context 'when operation feature is enabled' do
context 'when menu does not have any menu items' do
context 'when menu does not have any renderable menu items' do
it 'returns false' do
allow(subject).to receive(:has_items?).and_return(false)
allow(subject).to receive(:has_renderable_items?).and_return(false)
expect(subject.render?).to be false
end
......@@ -54,7 +54,7 @@ RSpec.describe Sidebars::Projects::Menus::OperationsMenu do
end
context 'Menu items' do
subject { described_class.new(context).items.index { |e| e.item_id == item_id } }
subject { described_class.new(context).renderable_items.index { |e| e.item_id == item_id } }
describe 'Metrics Dashboard' do
let(:item_id) { :metrics }
......
......@@ -12,7 +12,7 @@ RSpec.describe Sidebars::Projects::Menus::PackagesRegistriesMenu do
describe '#render?' do
context 'when menu does not have any menu item to show' do
it 'returns false' do
allow(subject).to receive(:has_items?).and_return(false)
allow(subject).to receive(:has_renderable_items?).and_return(false)
expect(subject.render?).to eq false
end
......@@ -36,7 +36,7 @@ RSpec.describe Sidebars::Projects::Menus::PackagesRegistriesMenu do
context 'when Packages Registry is visible' do
it 'menu link points to Packages Registry page' do
expect(subject.link).to eq described_class.new(context).items.find { |i| i.item_id == :packages_registry }.link
expect(subject.link).to eq described_class.new(context).renderable_items.find { |i| i.item_id == :packages_registry }.link
end
end
......@@ -44,21 +44,24 @@ RSpec.describe Sidebars::Projects::Menus::PackagesRegistriesMenu do
let(:packages_enabled) { false }
it 'menu link points to Container Registry page' do
expect(subject.link).to eq described_class.new(context).items.find { |i| i.item_id == :container_registry }.link
expect(subject.link).to eq described_class.new(context).renderable_items.find { |i| i.item_id == :container_registry }.link
end
context 'when Container Registry is not visible' do
let(:registry_enabled) { false }
it 'menu link points to Infrastructure Registry page' do
expect(subject.link).to eq described_class.new(context).items.find { |i| i.item_id == :infrastructure_registry }.link
expect(subject.link).to eq described_class.new(context).renderable_items.find { |i| i.item_id == :infrastructure_registry }.link
end
end
end
end
describe 'Menu items' do
subject { described_class.new(context).renderable_items.find { |i| i.item_id == item_id } }
describe 'Packages Registry' do
subject { described_class.new(context).items.find { |i| i.item_id == :packages_registry }}
let(:item_id) { :packages_registry }
context 'when user can read packages' do
context 'when config package setting is disabled' do
......@@ -88,7 +91,7 @@ RSpec.describe Sidebars::Projects::Menus::PackagesRegistriesMenu do
end
describe 'Container Registry' do
subject { described_class.new(context).items.find { |i| i.item_id == :container_registry }}
let(:item_id) { :container_registry }
context 'when user can read container images' do
context 'when config registry setting is disabled' do
......@@ -118,7 +121,7 @@ RSpec.describe Sidebars::Projects::Menus::PackagesRegistriesMenu do
end
describe 'Infrastructure Registry' do
subject { described_class.new(context).items.find { |i| i.item_id == :infrastructure_registry }}
let(:item_id) { :infrastructure_registry }
context 'when feature flag :infrastructure_registry_page is enabled' do
it 'the menu item is added to list of menu items' do
......@@ -136,4 +139,5 @@ RSpec.describe Sidebars::Projects::Menus::PackagesRegistriesMenu do
end
end
end
end
end
......@@ -9,7 +9,7 @@ RSpec.describe Sidebars::Projects::Menus::ProjectInformationMenu do
let(:context) { Sidebars::Projects::Context.new(current_user: user, container: project) }
describe 'Releases' do
subject { described_class.new(context).items.index { |e| e.item_id == :releases } }
subject { described_class.new(context).renderable_items.index { |e| e.item_id == :releases } }
context 'when project repository is empty' do
it 'does not include releases menu item' do
......
......@@ -11,14 +11,14 @@ RSpec.describe Sidebars::Projects::Menus::SettingsMenu do
describe '#render?' do
it 'returns false when menu does not have any menu items' do
allow(subject).to receive(:has_items?).and_return(false)
allow(subject).to receive(:has_renderable_items?).and_return(false)
expect(subject.render?).to be false
end
end
describe 'Menu items' do
subject { described_class.new(context).items.index { |e| e.item_id == item_id } }
subject { described_class.new(context).renderable_items.index { |e| e.item_id == item_id } }
shared_examples 'access rights checks' do
specify { is_expected.not_to be_nil }
......
......@@ -10,7 +10,7 @@ RSpec.describe Sidebars::Projects::Menus::WikiMenu do
subject { described_class.new(context) }
it 'does not contain any sub menu' do
expect(subject.items).to be_empty
expect(subject.has_items?).to be false
end
describe '#render?' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment