Add latest changes from gitlab-org/gitlab@master

app/controllers/explore/snippets_controller.rb

@@ -5,7 +5,7 @@ class Explore::SnippetsController < Explore::ApplicationController
   include Gitlab::NoteableMetadata
 
   def index
-    @snippets = SnippetsFinder.new(current_user)
+    @snippets = SnippetsFinder.new(current_user, explore: true)
       .execute
       .page(params[:page])
       .inc_author

app/controllers/notification_settings_controller.rb

@@ -50,8 +50,6 @@ class NotificationSettingsController < ApplicationController
   end
 
   def notification_setting_params_for(source)
-    allowed_fields = NotificationSetting.email_events(source).dup
-    allowed_fields << :level
-    params.require(:notification_setting).permit(allowed_fields)
+    params.require(:notification_setting).permit(NotificationSetting.allowed_fields(source))
   end
 end

app/controllers/profiles/groups_controller.rb

@@ -5,7 +5,7 @@ class Profiles::GroupsController < Profiles::ApplicationController
 
   def update
     group = find_routable!(Group, params[:id])
-    notification_setting = current_user.notification_settings.find_by(source: group) # rubocop: disable CodeReuse/ActiveRecord
+    notification_setting = current_user.notification_settings_for(group)
 
     if notification_setting.update(update_params)
       flash[:notice] = "Notification settings for #{group.name} saved"

app/controllers/profiles/notifications_controller.rb

@@ -5,6 +5,11 @@ class Profiles::NotificationsController < Profiles::ApplicationController
   def show
     @user = current_user
     @group_notifications = current_user.notification_settings.for_groups.order(:id)
+    @group_notifications += GroupsFinder.new(
+      current_user,
+      all_available: false,
+      exclude_group_ids: @group_notifications.select(:source_id)
+    ).execute.map { |group| current_user.notification_settings_for(group, inherit: true) }
     @project_notifications = current_user.notification_settings.for_projects.order(:id)
     @global_notification_setting = current_user.global_notification_setting
   end

app/finders/snippets_finder.rb

@@ -41,13 +41,14 @@
 class SnippetsFinder < UnionFinder
   include FinderMethods
 
-  attr_accessor :current_user, :project, :author, :scope
+  attr_accessor :current_user, :project, :author, :scope, :explore
 
   def initialize(current_user = nil, params = {})
     @current_user = current_user
     @project = params[:project]
     @author = params[:author]
     @scope = params[:scope].to_s
+    @explore = params[:explore]
 
     if project && author
       raise(
@@ -66,13 +67,23 @@ class SnippetsFinder < UnionFinder
   private
 
   def init_collection
-    if project
+    if explore
+      snippets_for_explore
+    elsif project
       snippets_for_a_single_project
     else
       snippets_for_multiple_projects
     end
   end
 
+  # Produces a query that retrieves snippets for the Explore page
+  #
+  # We only show personal snippets here because this page is meant for
+  # discovery, and project snippets are of limited interest here.
+  def snippets_for_explore
+    Snippet.public_to_user(current_user).only_personal_snippets
+  end
+
   # Produces a query that retrieves snippets from multiple projects.
   #
   # The resulting query will, depending on the user's permissions, include the
@@ -86,7 +97,7 @@ class SnippetsFinder < UnionFinder
   # Each collection is constructed in isolation, allowing for greater control
   # over the resulting SQL query.
   def snippets_for_multiple_projects
-    queries = [global_snippets]
+    queries = [personal_snippets]
 
     if Ability.allowed?(current_user, :read_cross_project)
       queries << snippets_of_visible_projects
@@ -100,8 +111,8 @@ class SnippetsFinder < UnionFinder
     Snippet.for_project_with_user(project, current_user)
   end
 
-  def global_snippets
-    snippets_for_author_or_visible_to_user.only_global_snippets
+  def personal_snippets
+    snippets_for_author_or_visible_to_user.only_personal_snippets
   end
 
   # Returns the snippets that the current user (logged in or not) can view.

app/models/concerns/stepable.rb

@@ -11,15 +11,15 @@ module Stepable
     initial_result = {}
 
     steps.inject(initial_result) do |previous_result, callback|
-      result = method(callback).call
+      result = method(callback).call(previous_result)
 
-      if result[:status] == :error
-        result[:failed_step] = callback
+      if result[:status] != :success
+        result[:last_step] = callback
 
         break result
       end
 
-      previous_result.merge(result)
+      result
     end
   end
 

app/models/notification_setting.rb

@@ -47,6 +47,10 @@ class NotificationSetting < ApplicationRecord
     EMAIL_EVENTS
   end
 
+  def self.allowed_fields(source = nil)
+    NotificationSetting.email_events(source).dup + %i(level notification_email)
+  end
+
   def email_events
     self.class.email_events(source)
   end

app/models/snippet.rb

@@ -71,7 +71,7 @@ class Snippet < ApplicationRecord
     end
   end
 
-  def self.only_global_snippets
+  def self.only_personal_snippets
     where(project_id: nil)
   end
 

app/models/user.rb

@@ -1317,14 +1317,27 @@ class User < ApplicationRecord
     notification_group&.notification_email_for(self) || notification_email
   end
 
-  def notification_settings_for(source)
+  def notification_settings_for(source, inherit: false)
     if notification_settings.loaded?
       notification_settings.find do |notification|
         notification.source_type == source.class.base_class.name &&
           notification.source_id == source.id
       end
     else
-      notification_settings.find_or_initialize_by(source: source)
+      notification_settings.find_or_initialize_by(source: source) do |ns|
+        next unless source.is_a?(Group) && inherit
+
+        # If we're here it means we're trying to create a NotificationSetting for a group that doesn't have one.
+        # Find the closest parent with a notification_setting that's not Global level, or that has an email set.
+        ancestor_ns = source
+                        .notification_settings(hierarchy_order: :asc)
+                        .where(user: self)
+                        .find_by('level != ? OR notification_email IS NOT NULL', NotificationSetting.levels[:global])
+        # Use it to seed the settings
+        ns.assign_attributes(ancestor_ns&.slice(*NotificationSetting.allowed_fields))
+        ns.source = source
+        ns.user = self
+      end
     end
   end
 

app/services/application_settings/update_service.rb

@@ -20,7 +20,11 @@ module ApplicationSettings
       add_to_outbound_local_requests_whitelist(@params.delete(:add_to_outbound_local_requests_whitelist))
 
       if params.key?(:performance_bar_allowed_group_path)
-        params[:performance_bar_allowed_group_id] = performance_bar_allowed_group_id
+        group_id = process_performance_bar_allowed_group_id
+
+        return false if application_setting.errors.any?
+
+        params[:performance_bar_allowed_group_id] = group_id
       end
 
       if usage_stats_updated? && !params.delete(:skip_usage_stats_user)
@@ -65,12 +69,27 @@ module ApplicationSettings
       @application_setting.reset_memoized_terms
     end
 
-    def performance_bar_allowed_group_id
-      performance_bar_enabled = !params.key?(:performance_bar_enabled) || params.delete(:performance_bar_enabled)
+    def process_performance_bar_allowed_group_id
       group_full_path = params.delete(:performance_bar_allowed_group_path)
-      return unless Gitlab::Utils.to_boolean(performance_bar_enabled)
+      enable_param_on = Gitlab::Utils.to_boolean(params.delete(:performance_bar_enabled))
+      performance_bar_enabled = enable_param_on.nil? || enable_param_on # Default to true
+
+      return if group_full_path.blank?
+      return if enable_param_on == false # Explicitly disabling
+
+      unless performance_bar_enabled
+        application_setting.errors.add(:performance_bar_allowed_group_id, 'not allowed when performance bar is disabled')
+        return
+      end
+
+      group = Group.find_by_full_path(group_full_path.chomp('/'))
+
+      unless group
+        application_setting.errors.add(:performance_bar_allowed_group_id, 'not found')
+        return
+      end
 
-      Group.find_by_full_path(group_full_path)&.id if group_full_path.present?
+      group.id
     end
 
     def bypass_external_auth?

app/views/groups/show.html.haml

@@ -5,6 +5,8 @@
   = auto_discovery_link_tag(:atom, group_url(@group, rss_url_options), title: "#{@group.name} activity")
 
 %div{ class: [("limit-container-width" unless fluid_layout)] }
+  = render_if_exists 'trials/banner', namespace: @group
+
   = render 'groups/home_panel'
 
   .groups-listing{ data: { endpoints: { default: group_children_path(@group, format: :json), shared: group_shared_projects_path(@group, format: :json) } } }

app/views/profiles/notifications/_group_settings.html.haml

@@ -12,5 +12,5 @@
     = render 'shared/notifications/button', notification_setting: setting, emails_disabled: emails_disabled
 
   .table-section.section-30
-    = form_for @user.notification_settings.find { |ns| ns.source == group }, url: profile_notifications_group_path(group), method: :put, html: { class: 'update-notifications' } do |f|
+    = form_for setting, url: profile_notifications_group_path(group), method: :put, html: { class: 'update-notifications' } do |f|
       = f.select :notification_email, @user.all_emails, { include_blank: 'Global notification email' }, class: 'select2 js-group-notification-email'

changelogs/unreleased/29477-notification-settings-display-all-groups.yml

+---
+title: Show all groups user belongs to in Notification settings
+merge_request: 17303
+author:
+type: fixed

changelogs/unreleased/30877-optimize-explore-snippets.yml

+---
+title: Show only personal snippets on explore page
+merge_request: 18092
+author:
+type: performance

changelogs/unreleased/34120-design-system-notes-icon-does-not-appear.yml

+---
+title: Resolve missing design system notes icons
+merge_request: 18693
+author:
+type: fixed

changelogs/unreleased/return-error-message-when-performance-bar-group-is-not-found.yml

+---
+title: Show error message when setting an invalid group ID for the performance bar
+merge_request:
+author:
+type: fixed

changelogs/unreleased/sort-severity-then-confidence.yml

+---
+title: Sort vulnerabilities by severity then confidence for dashboard and pipeline views
+merge_request: 18675
+author:
+type: changed

db/migrate/20191014084150_add_index_on_snippets_project_id_and_visibility_level.rb

+# frozen_string_literal: true
+
+class AddIndexOnSnippetsProjectIdAndVisibilityLevel < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :snippets, [:project_id, :visibility_level]
+  end
+
+  def down
+    remove_concurrent_index :snippets, [:project_id, :visibility_level]
+  end
+end

db/migrate/20191014132931_remove_index_on_snippets_project_id.rb

+# frozen_string_literal: true
+
+class RemoveIndexOnSnippetsProjectId < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    remove_concurrent_index :snippets, [:project_id]
+  end
+
+  def down
+    add_concurrent_index :snippets, [:project_id]
+  end
+end

db/schema.rb

@@ -3449,7 +3449,7 @@ ActiveRecord::Schema.define(version: 2019_10_16_220135) do
     t.index ["author_id"], name: "index_snippets_on_author_id"
     t.index ["content"], name: "index_snippets_on_content_trigram", opclass: :gin_trgm_ops, using: :gin
     t.index ["file_name"], name: "index_snippets_on_file_name_trigram", opclass: :gin_trgm_ops, using: :gin
-    t.index ["project_id"], name: "index_snippets_on_project_id"
+    t.index ["project_id", "visibility_level"], name: "index_snippets_on_project_id_and_visibility_level"
     t.index ["title"], name: "index_snippets_on_title_trigram", opclass: :gin_trgm_ops, using: :gin
     t.index ["updated_at"], name: "index_snippets_on_updated_at"
     t.index ["visibility_level"], name: "index_snippets_on_visibility_level"

doc/user/analytics/productivity_analytics.md

 # Productivity Analytics **(PREMIUM)**
 
-> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/12079) in [GitLab Premium](https://about.gitlab.com/pricing/) 12.3 (enabled by feature flags `productivity_analytics`, `productivity_analytics_scatterplot_enabled`).
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/12079) in [GitLab Premium](https://about.gitlab.com/pricing/) 12.3 (enabled by default using the feature flags `productivity_analytics`, `productivity_analytics_scatterplot_enabled`).
 
 Track development velocity with Productivity Analytics.
 

doc/user/project/clusters/kubernetes_pod_logs.md

@@ -17,8 +17,14 @@ Everything you need to build, test, deploy, and run your app at scale.
 1. On the **Environments** page, you should see the status of the environment's pods with [Deploy Boards](../deploy_boards.md).
 1. When mousing over the list of pods, a tooltip will appear with the exact pod name and status.
    ![Deploy Boards pod list](img/pod_logs_deploy_board.png)
-1. Click on the desired pod to bring up the logs view, which will contain the last 500 lines for that pod. You may switch between pods and environments in this view. Support for pods with multiple containers is coming [in a future release](https://gitlab.com/gitlab-org/gitlab/issues/6502).
-   ![Deploy Boards pod list](img/kubernetes_pod_logs.png)
+1. Click on the desired pod to bring up the logs view, which will contain the last 500 lines for that pod.
+   You may switch between the following in this view:
+   - Pods.
+   - [From GitLab 12.4](https://gitlab.com/gitlab-org/gitlab/issues/5769), environments.
+
+   Support for pods with multiple containers is coming [in a future release](https://gitlab.com/gitlab-org/gitlab/issues/6502).
+
+   ![Deploy Boards pod list](img/kubernetes_pod_logs_v12_4.png)
 
 ## Requirements
 

lib/gitlab/database_importers/self_monitoring/project/create_service.rb

@@ -33,7 +33,7 @@ module Gitlab
 
             if result[:status] == :success
               result
-            elsif STEPS_ALLOWED_TO_FAIL.include?(result[:failed_step])
+            elsif STEPS_ALLOWED_TO_FAIL.include?(result[:last_step])
               success
             else
               raise StandardError, result[:message]
@@ -42,121 +42,124 @@ module Gitlab
 
           private
 
-          def validate_application_settings
+          def validate_application_settings(_result)
             return success if application_settings
 
             log_error('No application_settings found')
             error(_('No application_settings found'))
           end
 
-          def validate_project_created
-            return success unless project_created?
+          def validate_project_created(result)
+            return success(result) unless project_created?
 
             log_error('Project already created')
             error(_('Project already created'))
           end
 
-          def validate_admins
+          def validate_admins(result)
             unless instance_admins.any?
               log_error('No active admin user found')
               return error(_('No active admin user found'))
             end
 
-            success
+            success(result)
           end
 
-          def create_group
+          def create_group(result)
             if project_created?
               log_info(_('Instance administrators group already exists'))
-              @group = application_settings.instance_administration_project.owner
-              return success(group: @group)
+              result[:group] = application_settings.instance_administration_project.owner
+              return success(result)
             end
 
-            @group = ::Groups::CreateService.new(group_owner, create_group_params).execute
+            result[:group] = ::Groups::CreateService.new(group_owner, create_group_params).execute
 
-            if @group.persisted?
-              success(group: @group)
+            if result[:group].persisted?
+              success(result)
             else
               error(_('Could not create group'))
             end
           end
 
-          def create_project
+          def create_project(result)
             if project_created?
               log_info('Instance administration project already exists')
-              @project = application_settings.instance_administration_project
-              return success(project: project)
+              result[:project] = application_settings.instance_administration_project
+              return success(result)
             end
 
-            @project = ::Projects::CreateService.new(group_owner, create_project_params).execute
+            result[:project] = ::Projects::CreateService.new(group_owner, create_project_params(result[:group])).execute
 
-            if project.persisted?
-              success(project: project)
+            if result[:project].persisted?
+              success(result)
             else
-              log_error("Could not create instance administration project. Errors: %{errors}" % { errors: project.errors.full_messages })
+              log_error("Could not create instance administration project. Errors: %{errors}" % { errors: result[:project].errors.full_messages })
               error(_('Could not create project'))
             end
           end
 
-          def save_project_id
+          def save_project_id(result)
             return success if project_created?
 
-            result = application_settings.update(instance_administration_project_id: @project.id)
+            response = application_settings.update(
+              instance_administration_project_id: result[:project].id
+            )
 
-            if result
-              success
+            if response
+              success(result)
             else
               log_error("Could not save instance administration project ID, errors: %{errors}" % { errors: application_settings.errors.full_messages })
               error(_('Could not save project ID'))
             end
           end
 
-          def add_group_members
-            members = @group.add_users(members_to_add, Gitlab::Access::MAINTAINER)
+          def add_group_members(result)
+            group = result[:group]
+            members = group.add_users(members_to_add(group), Gitlab::Access::MAINTAINER)
             errors = members.flat_map { |member| member.errors.full_messages }
 
             if errors.any?
               log_error('Could not add admins as members to self-monitoring project. Errors: %{errors}' % { errors: errors })
               error(_('Could not add admins as members'))
             else
-              success
+              success(result)
             end
           end
 
-          def add_to_whitelist
-            return success unless prometheus_enabled?
-            return success unless prometheus_listen_address.present?
+          def add_to_whitelist(result)
+            return success(result) unless prometheus_enabled?
+            return success(result) unless prometheus_listen_address.present?
 
             uri = parse_url(internal_prometheus_listen_address_uri)
             return error(_('Prometheus listen_address in config/gitlab.yml is not a valid URI')) unless uri
 
             application_settings.add_to_outbound_local_requests_whitelist([uri.normalized_host])
-            result = application_settings.save
+            response = application_settings.save
 
-            if result
+            if response
               # Expire the Gitlab::CurrentSettings cache after updating the whitelist.
               # This happens automatically in an after_commit hook, but in migrations,
               # the after_commit hook only runs at the end of the migration.
               Gitlab::CurrentSettings.expire_current_application_settings
-              success
+              success(result)
             else
               log_error("Could not add prometheus URL to whitelist, errors: %{errors}" % { errors: application_settings.errors.full_messages })
               error(_('Could not add prometheus URL to whitelist'))
             end
           end
 
-          def add_prometheus_manual_configuration
-            return success unless prometheus_enabled?
-            return success unless prometheus_listen_address.present?
+          def add_prometheus_manual_configuration(result)
+            return success(result) unless prometheus_enabled?
+            return success(result) unless prometheus_listen_address.present?
 
-            service = project.find_or_initialize_service('prometheus')
+            service = result[:project].find_or_initialize_service('prometheus')
 
             unless service.update(prometheus_service_attributes)
               log_error('Could not save prometheus manual configuration for self-monitoring project. Errors: %{errors}' % { errors: service.errors.full_messages })
               return error(_('Could not save prometheus manual configuration'))
             end
 
-            success
+            success(result)
           end
 
           def application_settings
@@ -196,11 +199,11 @@ module Gitlab
             instance_admins.first
           end
 
-          def members_to_add
+          def members_to_add(group)
             # Exclude admins who are already members of group because
-            # `@group.add_users(users)` returns an error if the users parameter contains
+            # `group.add_users(users)` returns an error if the users parameter contains
             # users who are already members of the group.
-            instance_admins - @group.members.collect(&:user)
+            instance_admins - group.members.collect(&:user)
           end
 
           def create_group_params
@@ -217,13 +220,13 @@ module Gitlab
             )
           end
 
-          def create_project_params
+          def create_project_params(group)
             {
               initialize_with_readme: true,
               visibility_level: VISIBILITY_LEVEL,
               name: PROJECT_NAME,
               description: "This project is automatically generated and will be used to help monitor this GitLab instance. [More information](#{docs_path})",
-              namespace_id: @group.id
+              namespace_id: group.id
             }
           end
 

locale/gitlab.pot

@@ -10947,6 +10947,9 @@ msgstr ""
 msgid "No data to display"
 msgstr ""
 
+msgid "No deployment platform available"
+msgstr ""
+
 msgid "No deployments found"
 msgstr ""
 
@@ -11013,6 +11016,9 @@ msgstr ""
 msgid "No parent group"
 msgstr ""
 
+msgid "No pods available"
+msgstr ""
+
 msgid "No preview for this file type"
 msgstr ""
 

spec/controllers/profiles/notifications_controller_spec.rb

@@ -20,6 +20,38 @@ describe Profiles::NotificationsController do
 
       expect(response).to render_template :show
     end
+
+    context 'with groups that do not have notification preferences' do
+      set(:group) { create(:group) }
+      set(:subgroup) { create(:group, parent: group) }
+
+      before do
+        group.add_developer(user)
+      end
+
+      it 'still shows up in the list' do
+        sign_in(user)
+
+        get :show
+
+        expect(assigns(:group_notifications).map(&:source_id)).to include(subgroup.id)
+      end
+
+      it 'has an N+1 (but should not)' do
+        sign_in(user)
+
+        control = ActiveRecord::QueryRecorder.new do
+          get :show
+        end
+
+        create_list(:group, 2, parent: group)
+
+        # We currently have an N + 1, switch to `not_to` once fixed
+        expect do
+          get :show
+        end.to exceed_query_limit(control)
+      end
+    end
   end
 
   describe 'POST update' do

spec/finders/snippets_finder_spec.rb

@@ -17,16 +17,27 @@ describe SnippetsFinder do
   end
 
   describe '#execute' do
-    set(:user) { create(:user) }
-    set(:private_personal_snippet) { create(:personal_snippet, :private, author: user) }
-    set(:internal_personal_snippet) { create(:personal_snippet, :internal, author: user) }
-    set(:public_personal_snippet) { create(:personal_snippet, :public, author: user) }
+    let_it_be(:user) { create(:user) }
+    let_it_be(:admin) { create(:admin) }
+    let_it_be(:group) { create(:group, :public) }
+    let_it_be(:project) { create(:project, :public, group: group) }
+
+    let_it_be(:private_personal_snippet) { create(:personal_snippet, :private, author: user) }
+    let_it_be(:internal_personal_snippet) { create(:personal_snippet, :internal, author: user) }
+    let_it_be(:public_personal_snippet) { create(:personal_snippet, :public, author: user) }
+
+    let_it_be(:private_project_snippet) { create(:project_snippet, :private, project: project) }
+    let_it_be(:internal_project_snippet) { create(:project_snippet, :internal, project: project) }
+    let_it_be(:public_project_snippet) { create(:project_snippet, :public, project: project) }
 
     context 'filter by scope' do
       it "returns all snippets for 'all' scope" do
         snippets = described_class.new(user, scope: :all).execute
 
-        expect(snippets).to contain_exactly(private_personal_snippet, internal_personal_snippet, public_personal_snippet)
+        expect(snippets).to contain_exactly(
+          private_personal_snippet, internal_personal_snippet, public_personal_snippet,
+          internal_project_snippet, public_project_snippet
+        )
       end
 
       it "returns all snippets for 'are_private' scope" do
@@ -38,13 +49,13 @@ describe SnippetsFinder do
       it "returns all snippets for 'are_internal' scope" do
         snippets = described_class.new(user, scope: :are_internal).execute
 
-        expect(snippets).to contain_exactly(internal_personal_snippet)
+        expect(snippets).to contain_exactly(internal_personal_snippet, internal_project_snippet)
       end
 
-      it "returns all snippets for 'are_private' scope" do
+      it "returns all snippets for 'are_public' scope" do
         snippets = described_class.new(user, scope: :are_public).execute
 
-        expect(snippets).to contain_exactly(public_personal_snippet)
+        expect(snippets).to contain_exactly(public_personal_snippet, public_project_snippet)
       end
     end
 
@@ -86,7 +97,6 @@ describe SnippetsFinder do
       end
 
       it 'returns all snippets for an admin' do
-        admin = create(:user, :admin)
         snippets = described_class.new(admin, author: user).execute
 
         expect(snippets).to contain_exactly(private_personal_snippet, internal_personal_snippet, public_personal_snippet)
@@ -94,12 +104,6 @@ describe SnippetsFinder do
     end
 
     context 'project snippets' do
-      let(:group) { create(:group, :public) }
-      let(:project) { create(:project, :public, group: group) }
-      let!(:private_project_snippet) { create(:project_snippet, :private, project: project) }
-      let!(:internal_project_snippet) { create(:project_snippet, :internal, project: project) }
-      let!(:public_project_snippet) { create(:project_snippet, :public, project: project) }
-
       it 'returns public personal and project snippets for unauthorized user' do
         snippets = described_class.new(nil, project: project).execute
 
@@ -147,7 +151,6 @@ describe SnippetsFinder do
       end
 
       it 'returns all snippets for an admin' do
-        admin = create(:user, :admin)
         snippets = described_class.new(admin, project: project).execute
 
         expect(snippets).to contain_exactly(private_project_snippet, internal_project_snippet, public_project_snippet)
@@ -174,6 +177,30 @@ describe SnippetsFinder do
       end
     end
 
+    context 'explore snippets' do
+      it 'returns only public personal snippets for unauthenticated users' do
+        snippets = described_class.new(nil, explore: true).execute
+
+        expect(snippets).to contain_exactly(public_personal_snippet)
+      end
+
+      it 'also returns internal personal snippets for authenticated users' do
+        snippets = described_class.new(user, explore: true).execute
+
+        expect(snippets).to contain_exactly(
+          internal_personal_snippet, public_personal_snippet
+        )
+      end
+
+      it 'returns all personal snippets for admins' do
+        snippets = described_class.new(admin, explore: true).execute
+
+        expect(snippets).to contain_exactly(
+          private_personal_snippet, internal_personal_snippet, public_personal_snippet
+        )
+      end
+    end
+
     context 'when the user cannot read cross project' do
       before do
         allow(Ability).to receive(:allowed?).and_call_original

spec/models/concerns/stepable_spec.rb

@@ -7,6 +7,8 @@ describe Stepable do
     Class.new do
       include Stepable
 
+      attr_writer :return_non_success
+
       steps :method1, :method2, :method3
 
       def execute
@@ -15,18 +17,18 @@ describe Stepable do
 
       private
 
-      def method1
+      def method1(_result)
         { status: :success }
       end
 
-      def method2
-        return { status: :error } unless @pass
+      def method2(result)
+        return { status: :not_a_success } if @return_non_success
 
-        { status: :success, variable1: 'var1' }
+        result.merge({ status: :success, variable1: 'var1', excluded_variable: 'a' })
       end
 
-      def method3
-        { status: :success, variable2: 'var2' }
+      def method3(result)
+        result.except(:excluded_variable).merge({ status: :success, variable2: 'var2' })
       end
     end
   end
@@ -41,8 +43,8 @@ describe Stepable do
 
       private
 
-      def appended_method1
-        { status: :success }
+      def appended_method1(previous_result)
+        previous_result.merge({ status: :success })
       end
     end
   end
@@ -51,21 +53,19 @@ describe Stepable do
     described_class.prepend(prepended_module)
   end
 
-  it 'stops after the first error' do
+  it 'stops after the first non success status' do
+    subject.return_non_success = true
+
     expect(subject).not_to receive(:method3)
     expect(subject).not_to receive(:appended_method1)
 
     expect(subject.execute).to eq(
-      status: :error,
-      failed_step: :method2
+      status: :not_a_success,
+      last_step: :method2
     )
   end
 
   context 'when all methods return success' do
-    before do
-      subject.instance_variable_set(:@pass, true)
-    end
-
     it 'calls all methods in order' do
       expect(subject).to receive(:method1).and_call_original.ordered
       expect(subject).to receive(:method2).and_call_original.ordered
@@ -82,6 +82,10 @@ describe Stepable do
         variable2: 'var2'
       )
     end
+
+    it 'can modify results of previous steps' do
+      expect(subject.execute).not_to include(excluded_variable: 'a')
+    end
   end
 
   context 'with multiple stepable classes' do

spec/models/snippet_spec.rb

@@ -183,12 +183,12 @@ describe Snippet do
     end
   end
 
-  describe '.only_global_snippets' do
+  describe '.only_personal_snippets' do
     it 'returns snippets not associated with any projects' do
       create(:project_snippet)
 
       snippet = create(:snippet)
-      snippets = described_class.only_global_snippets
+      snippets = described_class.only_personal_snippets
 
       expect(snippets).to eq([snippet])
     end

spec/models/user_spec.rb

@@ -3735,6 +3735,80 @@ describe User do
     end
   end
 
+  describe '#notification_settings_for' do
+    let(:user) { create(:user) }
+    let(:source) { nil }
+
+    subject { user.notification_settings_for(source) }
+
+    context 'when source is nil' do
+      it 'returns a blank global notification settings object' do
+        expect(subject.source).to eq(nil)
+        expect(subject.notification_email).to eq(nil)
+        expect(subject.level).to eq('global')
+      end
+    end
+
+    context 'when source is a Group' do
+      let(:group) { create(:group) }
+
+      subject { user.notification_settings_for(group, inherit: true) }
+
+      context 'when group has no existing notification settings' do
+        context 'when group has no ancestors' do
+          it 'will be a default Global notification setting' do
+            expect(subject.notification_email).to eq(nil)
+            expect(subject.level).to eq('global')
+          end
+        end
+
+        context 'when group has ancestors' do
+          context 'when an ancestor has a level other than Global' do
+            let(:ancestor) { create(:group) }
+            let(:group) { create(:group, parent: ancestor) }
+
+            before do
+              create(:notification_setting, user: user, source: ancestor, level: 'participating', notification_email: 'ancestor@example.com')
+            end
+
+            it 'has the same level set' do
+              expect(subject.level).to eq('participating')
+            end
+
+            it 'has the same email set' do
+              expect(subject.notification_email).to eq('ancestor@example.com')
+            end
+
+            context 'when inherit is false' do
+              subject { user.notification_settings_for(group) }
+
+              it 'does not inherit settings' do
+                expect(subject.notification_email).to eq(nil)
+                expect(subject.level).to eq('global')
+              end
+            end
+          end
+
+          context 'when an ancestor has a Global level but has an email set' do
+            let(:grand_ancestor) { create(:group) }
+            let(:ancestor) { create(:group, parent: grand_ancestor) }
+            let(:group) { create(:group, parent: ancestor) }
+
+            before do
+              create(:notification_setting, user: user, source: grand_ancestor, level: 'participating', notification_email: 'grand@example.com')
+              create(:notification_setting, user: user, source: ancestor, level: 'global', notification_email: 'ancestor@example.com')
+            end
+
+            it 'has the same email set' do
+              expect(subject.level).to eq('global')
+              expect(subject.notification_email).to eq('ancestor@example.com')
+            end
+          end
+        end
+      end
+    end
+  end
+
   describe '#notification_email_for' do
     let(:user) { create(:user) }
     let(:group) { create(:group) }

spec/services/application_settings/update_service_spec.rb

@@ -149,33 +149,42 @@ describe ApplicationSettings::UpdateService do
     where(:params_performance_bar_enabled,
           :params_performance_bar_allowed_group_path,
           :previous_performance_bar_allowed_group_id,
-      :expected_performance_bar_allowed_group_id) do
-      true | '' | nil | nil
-      true | '' | 42_000_000 | nil
-      true | nil | nil | nil
-      true | nil | 42_000_000 | nil
-      true | 'foo' | nil | nil
-      true | 'foo' | 42_000_000 | nil
-      true | 'group_a' | nil | 42_000_000
-      true | 'group_b' | 42_000_000 | 43_000_000
-      true | 'group_a' | 42_000_000 | 42_000_000
-      false | '' | nil | nil
-      false | '' | 42_000_000 | nil
-      false | nil | nil | nil
-      false | nil | 42_000_000 | nil
-      false | 'foo' | nil | nil
-      false | 'foo' | 42_000_000 | nil
-      false | 'group_a' | nil | nil
-      false | 'group_b' | 42_000_000 | nil
-      false | 'group_a' | 42_000_000 | nil
+          :expected_performance_bar_allowed_group_id,
+          :expected_valid) do
+      true | '' | nil | nil | true
+      true | '' | 42_000_000 | nil | true
+      true | nil | nil | nil | true
+      true | nil | 42_000_000 | nil | true
+      true | 'foo' | nil | nil | false
+      true | 'foo' | 42_000_000 | 42_000_000 | false
+      true | 'group_a' | nil | 42_000_000 | true
+      true | 'group_b' | 42_000_000 | 43_000_000 | true
+      true | 'group_b/' | 42_000_000 | 43_000_000 | true
+      true | 'group_a' | 42_000_000 | 42_000_000 | true
+      false | '' | nil | nil | true
+      false | '' | 42_000_000 | nil | true
+      false | nil | nil | nil | true
+      false | nil | 42_000_000 | nil | true
+      false | 'foo' | nil | nil | true
+      false | 'foo' | 42_000_000 | nil | true
+      false | 'group_a' | nil | nil | true
+      false | 'group_b' | 42_000_000 | nil | true
+      false | 'group_a' | 42_000_000 | nil | true
+      nil | '' | nil | nil | true
+      nil | 'foo' | nil | nil | false
+      nil | 'group_a' | nil | 42_000_000 | true
     end
 
     with_them do
       let(:params) do
         {
-          performance_bar_enabled: params_performance_bar_enabled,
           performance_bar_allowed_group_path: params_performance_bar_allowed_group_path
-        }
+        }.tap do |params_hash|
+          # Treat nil in the table as missing
+          unless params_performance_bar_enabled.nil?
+            params_hash[:performance_bar_enabled] = params_performance_bar_enabled
+          end
+        end
       end
 
       before do
@@ -202,6 +211,14 @@ describe ApplicationSettings::UpdateService do
             .not_to change(application_settings, :performance_bar_allowed_group_id)
         end
       end
+
+      it 'adds errors to the model for invalid params' do
+        expect(subject.execute).to eq(expected_valid)
+
+        unless expected_valid
+          expect(application_settings.errors[:performance_bar_allowed_group_id]).to be_present
+        end
+      end
     end
 
     context 'when :performance_bar_allowed_group_path is not present' do
@@ -221,7 +238,7 @@ describe ApplicationSettings::UpdateService do
       let(:group) { create(:group) }
       let(:params) { { performance_bar_allowed_group_path: group.full_path } }
 
-      it 'implicitely defaults to true' do
+      it 'implicitly defaults to true' do
         expect { subject.execute }
           .to change(application_settings, :performance_bar_allowed_group_id)
           .from(nil).to(group.id)