From 9c1df7bcf10e362442057b1df43a753b621d85ee Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Tue, 21 Jun 2022 03:08:34 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .../concerns/product_analytics_tracking.rb | 5 +- app/controllers/projects/blame_controller.rb | 2 +- .../projects/merge_requests_controller.rb | 1 - app/controllers/search_controller.rb | 6 +- app/models/integration.rb | 4 - .../integrations/base_chat_notification.rb | 38 ++--- app/models/integrations/discord.rb | 4 - app/models/integrations/hangouts_chat.rb | 3 - app/models/integrations/mattermost.rb | 6 + app/models/integrations/microsoft_teams.rb | 3 - app/models/integrations/slack.rb | 5 + app/models/integrations/unify_circuit.rb | 3 - app/models/integrations/webex_teams.rb | 3 - app/presenters/gitlab/blame_presenter.rb | 2 +- app/serializers/integrations/event_entity.rb | 10 +- app/services/projects/blame_service.rb | 4 +- .../application_settings/reporting.html.haml | 4 +- config/events/1655281022_i_search_total.yml | 25 ++++ .../development/file_identifier_hash.yml | 8 - lib/gitlab/diff/file_collection/base.rb | 16 +- lib/gitlab/diff/formatters/base_formatter.rb | 11 +- lib/gitlab/diff/position.rb | 7 +- .../diff/position_tracer/image_strategy.rb | 9 +- .../diff/position_tracer/line_strategy.rb | 30 ++-- spec/controllers/search_controller_spec.rb | 11 +- spec/features/projects/blobs/blame_spec.rb | 6 + .../diff/formatters/image_formatter_spec.rb | 1 - .../diff/formatters/text_formatter_spec.rb | 1 - spec/lib/gitlab/diff/position_spec.rb | 80 ---------- .../position_tracer/image_strategy_spec.rb | 113 -------------- .../position_tracer/line_strategy_spec.rb | 138 ------------------ spec/models/diff_note_spec.rb | 30 +--- .../base_chat_notification_spec.rb | 18 +++ .../presenters/gitlab/blame_presenter_spec.rb | 3 +- spec/requests/api/integrations_spec.rb | 15 +- .../draft_notes/publish_service_spec.rb | 2 +- spec/spec_helper.rb | 4 - .../snowplow_event_tracking_examples.rb | 37 +++++ .../position_formatters_shared_examples.rb | 16 +- 39 files changed, 179 insertions(+), 505 deletions(-) create mode 100644 config/events/1655281022_i_search_total.yml delete mode 100644 config/feature_flags/development/file_identifier_hash.yml create mode 100644 spec/support/shared_examples/controllers/snowplow_event_tracking_examples.rb diff --git a/app/controllers/concerns/product_analytics_tracking.rb b/app/controllers/concerns/product_analytics_tracking.rb index 0b51b3dd380..dc7ba8295b9 100644 --- a/app/controllers/concerns/product_analytics_tracking.rb +++ b/app/controllers/concerns/product_analytics_tracking.rb @@ -28,7 +28,10 @@ module ProductAnalyticsTracking def event_enabled?(event) events_to_ff = { g_analytics_valuestream: :route_hll_to_snowplow, - i_search_paid: :route_hll_to_snowplow_phase2 + + i_search_paid: :route_hll_to_snowplow_phase2, + i_search_total: :route_hll_to_snowplow_phase2, + i_search_advanced: :route_hll_to_snowplow_phase2 } Feature.enabled?(events_to_ff[event.to_sym], tracking_namespace_source) diff --git a/app/controllers/projects/blame_controller.rb b/app/controllers/projects/blame_controller.rb index 64ced43311a..5bfda526fb0 100644 --- a/app/controllers/projects/blame_controller.rb +++ b/app/controllers/projects/blame_controller.rb @@ -25,7 +25,7 @@ class Projects::BlameController < Projects::ApplicationController blame_service = Projects::BlameService.new(@blob, @commit, params.permit(:page)) - @blame = Gitlab::View::Presenter::Factory.new(blame_service.blame, project: @project, path: @path).fabricate! + @blame = Gitlab::View::Presenter::Factory.new(blame_service.blame, project: @project, path: @path, page: blame_service.page).fabricate! render locals: { blame_pagination: blame_service.pagination } end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index ed661b3d9b6..6b564412dbd 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -32,7 +32,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo before_action :check_user_can_push_to_source_branch!, only: [:rebase] before_action only: [:show] do - push_frontend_feature_flag(:file_identifier_hash) push_frontend_feature_flag(:merge_request_widget_graphql, project) push_frontend_feature_flag(:core_security_mr_widget_counts, project) push_frontend_feature_flag(:confidential_notes, project) diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 7d251ba555c..6b5fb266fd8 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -9,7 +9,7 @@ class SearchController < ApplicationController RESCUE_FROM_TIMEOUT_ACTIONS = [:count, :show, :autocomplete].freeze - track_redis_hll_event :show, name: 'i_search_total' + track_event :show, name: 'i_search_total', destinations: [:redis_hll, :snowplow] around_action :allow_gitaly_ref_name_caching @@ -203,6 +203,10 @@ class SearchController < ApplicationController render status: :request_timeout end end + + def tracking_namespace_source + search_service.project&.namespace || search_service.group + end end SearchController.prepend_mod_with('SearchController') diff --git a/app/models/integration.rb b/app/models/integration.rb index 726e95b7cbf..fe2f6806754 100644 --- a/app/models/integration.rb +++ b/app/models/integration.rb @@ -494,10 +494,6 @@ class Integration < ApplicationRecord self.class.event_names end - def event_field(event) - nil - end - def api_field_names fields.reject { _1[:type] == 'password' }.pluck(:name) end diff --git a/app/models/integrations/base_chat_notification.rb b/app/models/integrations/base_chat_notification.rb index 33d4eecbf49..0cfdcdbaf5f 100644 --- a/app/models/integrations/base_chat_notification.rb +++ b/app/models/integrations/base_chat_notification.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Base class for Chat notifications services +# Base class for Chat notifications integrations # This class is not meant to be used directly, but only to inherit from. module Integrations @@ -46,7 +46,7 @@ module Integrations # `notify_only_default_branch`. Now we have a string property named # `branches_to_be_notified`. Instead of doing a background migration, we # opted to set a value for the new property based on the old one, if - # users haven't specified one already. When users edit the service and + # users haven't specified one already. When users edit the integration and # select a value for this new property, it will override everything. self.branches_to_be_notified ||= notify_only_default_branch? ? "default" : "all" @@ -134,11 +134,9 @@ module Integrations end def event_channel_names - supported_events.map { |event| event_channel_name(event) } - end + return [] unless configurable_channels? - def event_field(event) - fields.find { |field| field[:name] == event_channel_name(event) } + supported_events.map { |event| event_channel_name(event) } end def global_fields @@ -153,6 +151,21 @@ module Integrations raise NotImplementedError end + # With some integrations the webhook is already tied to a specific channel, + # for others the channels are configurable for each event. + def configurable_channels? + false + end + + def event_channel_name(event) + EVENT_CHANNEL[event] + end + + def get_channel_field(event) + field_name = event_channel_name(event) + self.public_send(field_name) # rubocop:disable GitlabSecurity/PublicSend + end + private def log_usage(_, _) @@ -213,21 +226,12 @@ module Integrations end end - def get_channel_field(event) - field_name = event_channel_name(event) - self.public_send(field_name) # rubocop:disable GitlabSecurity/PublicSend - end - def build_event_channels - supported_events.reduce([]) do |channels, event| - channels << { type: 'text', name: event_channel_name(event), placeholder: default_channel_placeholder } + event_channel_names.map do |channel_field| + { type: 'text', name: channel_field, placeholder: default_channel_placeholder } end end - def event_channel_name(event) - EVENT_CHANNEL[event] - end - def project_name project.full_name end diff --git a/app/models/integrations/discord.rb b/app/models/integrations/discord.rb index 790e41e5a2a..231d1c62340 100644 --- a/app/models/integrations/discord.rb +++ b/app/models/integrations/discord.rb @@ -23,10 +23,6 @@ module Integrations s_('Send notifications about project events to a Discord channel. %{docs_link}').html_safe % { docs_link: docs_link.html_safe } end - def event_field(event) - # No-op. - end - def default_channel_placeholder # No-op. end diff --git a/app/models/integrations/hangouts_chat.rb b/app/models/integrations/hangouts_chat.rb index 8c68c9ff95a..4667bbffa1b 100644 --- a/app/models/integrations/hangouts_chat.rb +++ b/app/models/integrations/hangouts_chat.rb @@ -19,9 +19,6 @@ module Integrations s_('Before enabling this integration, create a webhook for the room in Google Chat where you want to receive notifications from this project. %{docs_link}').html_safe % { docs_link: docs_link.html_safe } end - def event_field(event) - end - def default_channel_placeholder end diff --git a/app/models/integrations/mattermost.rb b/app/models/integrations/mattermost.rb index d9ccbb7ea34..dae11b99bc5 100644 --- a/app/models/integrations/mattermost.rb +++ b/app/models/integrations/mattermost.rb @@ -3,6 +3,7 @@ module Integrations class Mattermost < BaseChatNotification include SlackMattermostNotifier + extend ::Gitlab::Utils::Override def title s_('Mattermost notifications') @@ -28,5 +29,10 @@ module Integrations def webhook_placeholder 'http://mattermost.example.com/hooks/' end + + override :configurable_channels? + def configurable_channels? + true + end end end diff --git a/app/models/integrations/microsoft_teams.rb b/app/models/integrations/microsoft_teams.rb index 625ee0bc522..949b24ca03f 100644 --- a/app/models/integrations/microsoft_teams.rb +++ b/app/models/integrations/microsoft_teams.rb @@ -22,9 +22,6 @@ module Integrations 'https://outlook.office.com/webhook/…' end - def event_field(event) - end - def default_channel_placeholder end diff --git a/app/models/integrations/slack.rb b/app/models/integrations/slack.rb index 0381db3a67e..93263229109 100644 --- a/app/models/integrations/slack.rb +++ b/app/models/integrations/slack.rb @@ -55,5 +55,10 @@ module Integrations Gitlab::UsageDataCounters::HLLRedisCounter.track_event(key, values: user_id) end + + override :configurable_channels? + def configurable_channels? + true + end end end diff --git a/app/models/integrations/unify_circuit.rb b/app/models/integrations/unify_circuit.rb index f085423d229..40570c098c2 100644 --- a/app/models/integrations/unify_circuit.rb +++ b/app/models/integrations/unify_circuit.rb @@ -19,9 +19,6 @@ module Integrations s_('Integrations|Send notifications about project events to a Unify Circuit conversation. %{docs_link}').html_safe % { docs_link: docs_link.html_safe } end - def event_field(event) - end - def default_channel_placeholder end diff --git a/app/models/integrations/webex_teams.rb b/app/models/integrations/webex_teams.rb index 345dd98cbc1..7bf90259267 100644 --- a/app/models/integrations/webex_teams.rb +++ b/app/models/integrations/webex_teams.rb @@ -19,9 +19,6 @@ module Integrations s_("WebexTeamsService|Send notifications about project events to a Webex Teams conversation. %{docs_link}") % { docs_link: docs_link.html_safe } end - def event_field(event) - end - def default_channel_placeholder end diff --git a/app/presenters/gitlab/blame_presenter.rb b/app/presenters/gitlab/blame_presenter.rb index 81a954761ea..6230e61d2be 100644 --- a/app/presenters/gitlab/blame_presenter.rb +++ b/app/presenters/gitlab/blame_presenter.rb @@ -66,7 +66,7 @@ module Gitlab previous_commit_id = commit.parent_id return unless previous_commit_id && !previous_path.nil? - link_to project_blame_path(project, tree_join(previous_commit_id, previous_path)), + link_to project_blame_path(project, tree_join(previous_commit_id, previous_path), page: page), title: _('View blame prior to this change'), aria: { label: _('View blame prior to this change') }, class: 'version-link', diff --git a/app/serializers/integrations/event_entity.rb b/app/serializers/integrations/event_entity.rb index 170f660f334..f603c0e035e 100644 --- a/app/serializers/integrations/event_entity.rb +++ b/app/serializers/integrations/event_entity.rb @@ -18,12 +18,12 @@ module Integrations IntegrationsHelper.integration_event_description(integration, event) end - expose :field, if: ->(_, _) { event_field } do + expose :field, if: ->(_, _) { integration.try(:configurable_channels?) } do expose :name do |event| - event_field[:name] + integration.event_channel_name(event) end expose :value do |event| - integration.public_send(event_field[:name]) # rubocop:disable GitlabSecurity/PublicSend + integration.get_channel_field(event) end end @@ -35,10 +35,6 @@ module Integrations IntegrationsHelper.integration_event_field_name(event) end - def event_field - @event_field ||= integration.event_field(event) - end - def integration request.integration end diff --git a/app/services/projects/blame_service.rb b/app/services/projects/blame_service.rb index f7c1240a3ba..976ba970769 100644 --- a/app/services/projects/blame_service.rb +++ b/app/services/projects/blame_service.rb @@ -12,6 +12,8 @@ module Projects @page = extract_page(params) end + attr_reader :page + def blame Gitlab::Blame.new(blob, commit, range: blame_range) end @@ -27,7 +29,7 @@ module Projects private - attr_reader :blob, :commit, :page + attr_reader :blob, :commit def blame_range return unless pagination_enabled? diff --git a/app/views/admin/application_settings/reporting.html.haml b/app/views/admin/application_settings/reporting.html.haml index b15fcd93d1a..60781f86f2e 100644 --- a/app/views/admin/application_settings/reporting.html.haml +++ b/app/views/admin/application_settings/reporting.html.haml @@ -4,7 +4,7 @@ %section.settings.as-spam.no-animate#js-spam-settings{ class: ('expanded' if expanded_by_default?) } .settings-header - %h4 + %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only = _('Spam and Anti-bot Protection') = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do = expanded_by_default? ? _('Collapse') : _('Expand') @@ -18,7 +18,7 @@ %section.settings.as-abuse.no-animate#js-abuse-settings{ class: ('expanded' if expanded_by_default?) } .settings-header - %h4 + %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only = _('Abuse reports') = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do = expanded_by_default? ? _('Collapse') : _('Expand') diff --git a/config/events/1655281022_i_search_total.yml b/config/events/1655281022_i_search_total.yml new file mode 100644 index 00000000000..1fe8432fff0 --- /dev/null +++ b/config/events/1655281022_i_search_total.yml @@ -0,0 +1,25 @@ +--- +description: Triggered from backend layer Search controller performs search operation +category: SearchController +action: i_search_total +label_description: +property_description: +value_description: +extra_properties: +identifiers: + - project + - user + - namespace +product_section: enablement +product_stage: enablement +product_group: group::global search +product_category: global_search +milestone: "15.1" +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/90224 +distributions: + - ce + - ee +tiers: + - free + - premium + - ultimate diff --git a/config/feature_flags/development/file_identifier_hash.yml b/config/feature_flags/development/file_identifier_hash.yml deleted file mode 100644 index 84879e6c33e..00000000000 --- a/config/feature_flags/development/file_identifier_hash.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: file_identifier_hash -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33028 -rollout_issue_url: -milestone: '13.1' -type: development -group: group::code review -default_enabled: false diff --git a/lib/gitlab/diff/file_collection/base.rb b/lib/gitlab/diff/file_collection/base.rb index 7fa1bd6b5ec..924de132840 100644 --- a/lib/gitlab/diff/file_collection/base.rb +++ b/lib/gitlab/diff/file_collection/base.rb @@ -68,20 +68,12 @@ module Gitlab end end - def diff_file_with_old_path(old_path, a_mode = nil) - if Feature.enabled?(:file_identifier_hash) && a_mode.present? - diff_files.find { |diff_file| diff_file.old_path == old_path && diff_file.a_mode == a_mode } - else - diff_files.find { |diff_file| diff_file.old_path == old_path } - end + def diff_file_with_old_path(old_path) + diff_files.find { |diff_file| diff_file.old_path == old_path } end - def diff_file_with_new_path(new_path, b_mode = nil) - if Feature.enabled?(:file_identifier_hash) && b_mode.present? - diff_files.find { |diff_file| diff_file.new_path == new_path && diff_file.b_mode == b_mode } - else - diff_files.find { |diff_file| diff_file.new_path == new_path } - end + def diff_file_with_new_path(new_path) + diff_files.find { |diff_file| diff_file.new_path == new_path } end def clear_cache diff --git a/lib/gitlab/diff/formatters/base_formatter.rb b/lib/gitlab/diff/formatters/base_formatter.rb index e24150a2330..19fc028594c 100644 --- a/lib/gitlab/diff/formatters/base_formatter.rb +++ b/lib/gitlab/diff/formatters/base_formatter.rb @@ -6,7 +6,6 @@ module Gitlab class BaseFormatter attr_reader :old_path attr_reader :new_path - attr_reader :file_identifier_hash attr_reader :base_sha attr_reader :start_sha attr_reader :head_sha @@ -16,7 +15,6 @@ module Gitlab attrs[:diff_refs] = diff_file.diff_refs attrs[:old_path] = diff_file.old_path attrs[:new_path] = diff_file.new_path - attrs[:file_identifier_hash] = diff_file.file_identifier_hash end if diff_refs = attrs[:diff_refs] @@ -27,7 +25,6 @@ module Gitlab @old_path = attrs[:old_path] @new_path = attrs[:new_path] - @file_identifier_hash = attrs[:file_identifier_hash] @base_sha = attrs[:base_sha] @start_sha = attrs[:start_sha] @head_sha = attrs[:head_sha] @@ -38,7 +35,7 @@ module Gitlab end def to_h - out = { + { base_sha: base_sha, start_sha: start_sha, head_sha: head_sha, @@ -46,12 +43,6 @@ module Gitlab new_path: new_path, position_type: position_type } - - if Feature.enabled?(:file_identifier_hash) - out[:file_identifier_hash] = file_identifier_hash - end - - out end def position_type diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb index 74c33c46598..40b6ae2f14e 100644 --- a/lib/gitlab/diff/position.rb +++ b/lib/gitlab/diff/position.rb @@ -9,7 +9,6 @@ module Gitlab delegate :old_path, :new_path, - :file_identifier_hash, :base_sha, :start_sha, :head_sha, @@ -161,11 +160,7 @@ module Gitlab def find_diff_file_from(diffable) diff_files = diffable.diffs(diff_options).diff_files - if Feature.enabled?(:file_identifier_hash) && file_identifier_hash.present? - diff_files.find { |df| df.file_identifier_hash == file_identifier_hash } - else - diff_files.first - end + diff_files.first end def multiline? diff --git a/lib/gitlab/diff/position_tracer/image_strategy.rb b/lib/gitlab/diff/position_tracer/image_strategy.rb index 046a6782dda..aac52b536f7 100644 --- a/lib/gitlab/diff/position_tracer/image_strategy.rb +++ b/lib/gitlab/diff/position_tracer/image_strategy.rb @@ -7,24 +7,21 @@ module Gitlab def trace(position) a_path = position.old_path b_path = position.new_path - diff_file = diff_file(position) - a_mode = diff_file&.a_mode - b_mode = diff_file&.b_mode # If file exists in B->D (e.g. updated, renamed, removed), let the # note become outdated. - bd_diff = bd_diffs.diff_file_with_old_path(b_path, b_mode) + bd_diff = bd_diffs.diff_file_with_old_path(b_path) return { position: new_position(position, bd_diff), outdated: true } if bd_diff # If file still exists in the new diff, update the position. - cd_diff = cd_diffs.diff_file_with_new_path(b_path, b_mode) + cd_diff = cd_diffs.diff_file_with_new_path(b_path) return { position: new_position(position, cd_diff), outdated: false } if cd_diff # If file exists in A->C (e.g. rebased and same changes were present # in target branch), let the note become outdated. - ac_diff = ac_diffs.diff_file_with_old_path(a_path, a_mode) + ac_diff = ac_diffs.diff_file_with_old_path(a_path) return { position: new_position(position, ac_diff), outdated: true } if ac_diff diff --git a/lib/gitlab/diff/position_tracer/line_strategy.rb b/lib/gitlab/diff/position_tracer/line_strategy.rb index 0f0b8f0c4f3..d7a7e3f5425 100644 --- a/lib/gitlab/diff/position_tracer/line_strategy.rb +++ b/lib/gitlab/diff/position_tracer/line_strategy.rb @@ -76,20 +76,16 @@ module Gitlab def trace_added_line(position) b_path = position.new_path b_line = position.new_line - diff_file = diff_file(position) - b_mode = diff_file&.b_mode - bd_diff = bd_diffs.diff_file_with_old_path(b_path, b_mode) + bd_diff = bd_diffs.diff_file_with_old_path(b_path) d_path = bd_diff&.new_path || b_path - d_mode = bd_diff&.b_mode || b_mode d_line = LineMapper.new(bd_diff).old_to_new(b_line) if d_line - cd_diff = cd_diffs.diff_file_with_new_path(d_path, d_mode) + cd_diff = cd_diffs.diff_file_with_new_path(d_path) c_path = cd_diff&.old_path || d_path - c_mode = cd_diff&.a_mode || d_mode c_line = LineMapper.new(cd_diff).new_to_old(d_line) if c_line @@ -102,7 +98,7 @@ module Gitlab else # If the line is no longer in the MR, we unfortunately cannot show # the current state on the CD diff, so we treat it as outdated. - ac_diff = ac_diffs.diff_file_with_new_path(c_path, c_mode) + ac_diff = ac_diffs.diff_file_with_new_path(c_path) { position: new_position(ac_diff, nil, c_line, position.line_range), outdated: true } end @@ -119,26 +115,22 @@ module Gitlab def trace_removed_line(position) a_path = position.old_path a_line = position.old_line - diff_file = diff_file(position) - a_mode = diff_file&.a_mode - ac_diff = ac_diffs.diff_file_with_old_path(a_path, a_mode) + ac_diff = ac_diffs.diff_file_with_old_path(a_path) c_path = ac_diff&.new_path || a_path - c_mode = ac_diff&.b_mode || a_mode c_line = LineMapper.new(ac_diff).old_to_new(a_line) if c_line - cd_diff = cd_diffs.diff_file_with_old_path(c_path, c_mode) + cd_diff = cd_diffs.diff_file_with_old_path(c_path) d_path = cd_diff&.new_path || c_path - d_mode = cd_diff&.b_mode || c_mode d_line = LineMapper.new(cd_diff).old_to_new(c_line) if d_line # If the line is still in C but also in D, it has turned from a # removed line into an unchanged one. - bd_diff = bd_diffs.diff_file_with_new_path(d_path, d_mode) + bd_diff = bd_diffs.diff_file_with_new_path(d_path) { position: new_position(bd_diff, nil, d_line, position.line_range), outdated: true } else @@ -156,21 +148,17 @@ module Gitlab a_line = position.old_line b_path = position.new_path b_line = position.new_line - diff_file = diff_file(position) - a_mode = diff_file&.a_mode - b_mode = diff_file&.b_mode - ac_diff = ac_diffs.diff_file_with_old_path(a_path, a_mode) + ac_diff = ac_diffs.diff_file_with_old_path(a_path) c_path = ac_diff&.new_path || a_path - c_mode = ac_diff&.b_mode || a_mode c_line = LineMapper.new(ac_diff).old_to_new(a_line) - bd_diff = bd_diffs.diff_file_with_old_path(b_path, b_mode) + bd_diff = bd_diffs.diff_file_with_old_path(b_path) d_line = LineMapper.new(bd_diff).old_to_new(b_line) - cd_diff = cd_diffs.diff_file_with_old_path(c_path, c_mode) + cd_diff = cd_diffs.diff_file_with_old_path(c_path) if c_line && d_line # If the line is still in C and D, it is still unchanged. diff --git a/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb index 4abcd414e51..c664b2269d7 100644 --- a/spec/controllers/search_controller_spec.rb +++ b/spec/controllers/search_controller_spec.rb @@ -67,7 +67,7 @@ RSpec.describe SearchController do end end - describe 'GET #show' do + describe 'GET #show', :snowplow do it_behaves_like 'when the user cannot read cross project', :show, { search: 'hello' } do it 'still allows accessing the search page' do get :show @@ -257,6 +257,15 @@ RSpec.describe SearchController do end end + it_behaves_like 'Snowplow event tracking' do + let(:category) { described_class.to_s } + let(:action) { 'i_search_total' } + let(:namespace) { create(:group) } + let(:feature_flag_name) { :route_hll_to_snowplow_phase2 } + + subject { get :show, params: { group_id: namespace.id, scope: 'blobs', search: 'term' } } + end + context 'on restricted projects' do context 'when signed out' do before do diff --git a/spec/features/projects/blobs/blame_spec.rb b/spec/features/projects/blobs/blame_spec.rb index bb3b5cd931c..4e422786f36 100644 --- a/spec/features/projects/blobs/blame_spec.rb +++ b/spec/features/projects/blobs/blame_spec.rb @@ -49,6 +49,12 @@ RSpec.describe 'File blame', :js do expect(page).to have_css('#L3') expect(find('.page-link.active')).to have_text('2') end + + it 'correctly redirects to the prior blame page' do + find('.version-link').click + + expect(find('.page-link.active')).to have_text('2') + end end context 'when feature flag disabled' do diff --git a/spec/lib/gitlab/diff/formatters/image_formatter_spec.rb b/spec/lib/gitlab/diff/formatters/image_formatter_spec.rb index 579776d44aa..73c0d0dba88 100644 --- a/spec/lib/gitlab/diff/formatters/image_formatter_spec.rb +++ b/spec/lib/gitlab/diff/formatters/image_formatter_spec.rb @@ -10,7 +10,6 @@ RSpec.describe Gitlab::Diff::Formatters::ImageFormatter do head_sha: 789, old_path: 'old_image.png', new_path: 'new_image.png', - file_identifier_hash: '777', position_type: 'image' } end diff --git a/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb b/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb index b6bdc5ff493..290585d0991 100644 --- a/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb +++ b/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb @@ -10,7 +10,6 @@ RSpec.describe Gitlab::Diff::Formatters::TextFormatter do head_sha: 789, old_path: 'old_path.txt', new_path: 'new_path.txt', - file_identifier_hash: '777', line_range: nil } end diff --git a/spec/lib/gitlab/diff/position_spec.rb b/spec/lib/gitlab/diff/position_spec.rb index c9a20f40462..bb3522eb579 100644 --- a/spec/lib/gitlab/diff/position_spec.rb +++ b/spec/lib/gitlab/diff/position_spec.rb @@ -574,86 +574,6 @@ RSpec.describe Gitlab::Diff::Position do end end - describe '#find_diff_file_from' do - context "position for a diff file that has changed from symlink to regular file" do - let(:commit) { project.commit("81e6355ce4e1544a3524b230952c12455de0777b") } - - let(:old_symlink_file_identifier_hash) { "bfa430463f33619872d52a6b85ced59c973e42dc" } - let(:new_regular_file_identifier_hash) { "e25b60c2e5ffb977d2b1431b96c6f7800c3c3529" } - let(:file_identifier_hash) { new_regular_file_identifier_hash } - - let(:args) do - { - file_identifier_hash: file_identifier_hash, - old_path: "symlink", - new_path: "symlink", - old_line: nil, - new_line: 1, - diff_refs: commit.diff_refs - } - end - - let(:diffable) { commit.diff_refs.compare_in(project) } - - subject(:diff_file) { described_class.new(args).find_diff_file_from(diffable) } - - context 'when file_identifier_hash is disabled' do - before do - stub_feature_flags(file_identifier_hash: false) - end - - it "returns the first diff file" do - expect(diff_file.file_identifier_hash).to eq(old_symlink_file_identifier_hash) - end - end - - context 'when file_identifier_hash is enabled' do - before do - stub_feature_flags(file_identifier_hash: true) - end - - context 'for new regular file' do - it "returns the correct diff file" do - expect(diff_file.file_identifier_hash).to eq(new_regular_file_identifier_hash) - end - end - - context 'for old symlink file' do - let(:args) do - { - file_identifier_hash: old_symlink_file_identifier_hash, - old_path: "symlink", - new_path: "symlink", - old_line: 1, - new_line: nil, - diff_refs: commit.diff_refs - } - end - - it "returns the correct diff file" do - expect(diff_file.file_identifier_hash).to eq(old_symlink_file_identifier_hash) - end - end - - context 'when file_identifier_hash is missing' do - let(:file_identifier_hash) { nil } - - it "returns the first diff file" do - expect(diff_file.file_identifier_hash).to eq(old_symlink_file_identifier_hash) - end - end - - context 'when file_identifier_hash cannot be found' do - let(:file_identifier_hash) { "missingidentifier" } - - it "returns nil" do - expect(diff_file).to be_nil - end - end - end - end - end - describe '#==' do let(:commit) { project.commit("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") } diff --git a/spec/lib/gitlab/diff/position_tracer/image_strategy_spec.rb b/spec/lib/gitlab/diff/position_tracer/image_strategy_spec.rb index 1414056ad6a..563480d214b 100644 --- a/spec/lib/gitlab/diff/position_tracer/image_strategy_spec.rb +++ b/spec/lib/gitlab/diff/position_tracer/image_strategy_spec.rb @@ -234,118 +234,5 @@ RSpec.describe Gitlab::Diff::PositionTracer::ImageStrategy do end end end - - describe 'symlink scenarios' do - let(:new_file) { old_file_status == :new } - let(:deleted_file) { old_file_status == :deleted } - let(:renamed_file) { old_file_status == :renamed } - - let(:file_identifier) { "#{file_name}-#{new_file}-#{deleted_file}-#{renamed_file}" } - let(:file_identifier_hash) { Digest::SHA1.hexdigest(file_identifier) } - let(:old_position) { position(old_path: file_name, new_path: file_name, position_type: 'image', file_identifier_hash: file_identifier_hash) } - - let(:update_file_commit) do - initial_commit - - update_file( - branch_name, - file_name, - Base64.encode64('morecontent') - ) - end - - let(:delete_file_commit) do - initial_commit - - delete_file(branch_name, file_name) - end - - let(:create_second_file_commit) do - initial_commit - - create_file( - branch_name, - second_file_name, - Base64.encode64('morecontent') - ) - end - - before do - stub_feature_flags(file_identifier_hash: true) - end - - describe 'from symlink to image' do - let(:initial_commit) { project.commit('a19c7f9a147e35e535c797cf148d29c24dac5544') } - let(:symlink_to_image_commit) { project.commit('8cfca8420812e5bd7479aa32cf33e0c95a3ca576') } - let(:branch_name) { 'diff-files-symlink-to-image' } - let(:file_name) { 'symlink-to-image.png' } - - context "when the old position is on the new image file" do - let(:old_file_status) { :new } - - context "when the image file's content was unchanged between the old and the new diff" do - let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_image_commit) } - let(:new_diff_refs) { diff_refs(initial_commit, create_second_file_commit) } - - it "returns the new position" do - expect_new_position( - old_path: file_name, - new_path: file_name - ) - end - end - - context "when the image file's content was changed between the old and the new diff" do - let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_image_commit) } - let(:new_diff_refs) { diff_refs(initial_commit, update_file_commit) } - let(:change_diff_refs) { diff_refs(symlink_to_image_commit, update_file_commit) } - - it "returns the position of the change" do - expect_change_position( - old_path: file_name, - new_path: file_name - ) - end - end - - context "when the image file was removed between the old and the new diff" do - let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_image_commit) } - let(:new_diff_refs) { diff_refs(initial_commit, delete_file_commit) } - let(:change_diff_refs) { diff_refs(symlink_to_image_commit, delete_file_commit) } - - it "returns the position of the change" do - expect_change_position( - old_path: file_name, - new_path: file_name - ) - end - end - end - end - - describe 'from image to symlink' do - let(:initial_commit) { project.commit('d10dcdfbbb2b59a959a5f5d66a4adf28f0ea4008') } - let(:image_to_symlink_commit) { project.commit('3e94fdaa60da8aed38401b91bc56be70d54ca424') } - let(:branch_name) { 'diff-files-image-to-symlink' } - let(:file_name) { 'image-to-symlink.png' } - - context "when the old position is on the added image file" do - let(:old_file_status) { :new } - - context "when the image file gets changed to a symlink between the old and the new diff" do - let(:old_diff_refs) { diff_refs(initial_commit.parent, initial_commit) } - let(:new_diff_refs) { diff_refs(initial_commit.parent, image_to_symlink_commit) } - let(:change_diff_refs) { diff_refs(initial_commit, image_to_symlink_commit) } - - it "returns the position of the change" do - expect_change_position( - old_path: file_name, - new_path: file_name - ) - end - end - end - end - end end end diff --git a/spec/lib/gitlab/diff/position_tracer/line_strategy_spec.rb b/spec/lib/gitlab/diff/position_tracer/line_strategy_spec.rb index ea56a87dec2..2b21084d8e5 100644 --- a/spec/lib/gitlab/diff/position_tracer/line_strategy_spec.rb +++ b/spec/lib/gitlab/diff/position_tracer/line_strategy_spec.rb @@ -1860,143 +1860,5 @@ RSpec.describe Gitlab::Diff::PositionTracer::LineStrategy, :clean_gitlab_redis_c end end end - - describe 'symlink scenarios' do - let(:new_file) { old_file_status == :new } - let(:deleted_file) { old_file_status == :deleted } - let(:renamed_file) { old_file_status == :renamed } - - let(:file_identifier) { "#{file_name}-#{new_file}-#{deleted_file}-#{renamed_file}" } - let(:file_identifier_hash) { Digest::SHA1.hexdigest(file_identifier) } - - let(:update_line_commit) do - update_file( - branch_name, - file_name, - <<-CONTENT.strip_heredoc - A - BB - C - CONTENT - ) - end - - let(:delete_file_commit) do - delete_file(branch_name, file_name) - end - - let(:create_second_file_commit) do - create_file( - branch_name, - second_file_name, - <<-CONTENT.strip_heredoc - D - E - CONTENT - ) - end - - before do - stub_feature_flags(file_identifier_hash: true) - end - - describe 'from symlink to text' do - let(:initial_commit) { project.commit('0e5b363105e9176a77bac94d7ff6d8c4fb35c3eb') } - let(:symlink_to_text_commit) { project.commit('689815e617abc6889f1fded4834d2dd7d942a58e') } - let(:branch_name) { 'diff-files-symlink-to-text' } - let(:file_name) { 'symlink-to-text.txt' } - let(:old_position) { position(old_path: file_name, new_path: file_name, new_line: 3, file_identifier_hash: file_identifier_hash) } - - before do - create_branch('diff-files-symlink-to-text-test', branch_name) - end - - context "when the old position is on the new text file" do - let(:old_file_status) { :new } - - context "when the text file's content was unchanged between the old and the new diff" do - let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_text_commit) } - let(:new_diff_refs) { diff_refs(initial_commit, create_second_file_commit) } - - it "returns the new position" do - expect_new_position( - new_path: old_position.new_path, - new_line: old_position.new_line - ) - end - end - - context "when the text file's content has change, but the line was unchanged between the old and the new diff" do - let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_text_commit) } - let(:new_diff_refs) { diff_refs(initial_commit, update_line_commit) } - - it "returns the new position" do - expect_new_position( - new_path: old_position.new_path, - new_line: old_position.new_line - ) - end - end - - context "when the text file's line was changed between the old and the new diff" do - let(:old_position) { position(old_path: file_name, new_path: file_name, new_line: 2, file_identifier_hash: file_identifier_hash) } - - let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_text_commit) } - let(:new_diff_refs) { diff_refs(initial_commit, update_line_commit) } - let(:change_diff_refs) { diff_refs(symlink_to_text_commit, update_line_commit) } - - it "returns the position of the change" do - expect_change_position( - old_path: file_name, - new_path: file_name, - old_line: 2, - new_line: nil - ) - end - end - - context "when the text file was removed between the old and the new diff" do - let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_text_commit) } - let(:new_diff_refs) { diff_refs(initial_commit, delete_file_commit) } - let(:change_diff_refs) { diff_refs(symlink_to_text_commit, delete_file_commit) } - - it "returns the position of the change" do - expect_change_position( - old_path: file_name, - new_path: file_name, - old_line: 3, - new_line: nil - ) - end - end - end - - describe 'from text to symlink' do - let(:initial_commit) { project.commit('3db7bd90bab8ce8f02c9818590b84739a2e97230') } - let(:text_to_symlink_commit) { project.commit('5e2c2708c2e403dece5dd25759369150aac51644') } - let(:branch_name) { 'diff-files-text-to-symlink' } - let(:file_name) { 'text-to-symlink.txt' } - - context "when the position is on the added text file" do - let(:old_file_status) { :new } - - context "when the text file gets changed to a symlink between the old and the new diff" do - let(:old_diff_refs) { diff_refs(initial_commit.parent, initial_commit) } - let(:new_diff_refs) { diff_refs(initial_commit.parent, text_to_symlink_commit) } - let(:change_diff_refs) { diff_refs(initial_commit, text_to_symlink_commit) } - - it "returns the position of the change" do - expect_change_position( - old_path: file_name, - new_path: file_name, - old_line: 3, - new_line: nil - ) - end - end - end - end - end - end end end diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index f377b34679c..d379ffeee02 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -260,32 +260,12 @@ RSpec.describe DiffNote do end context 'when the discussion was created in the diff' do - context 'when file_identifier_hash is disabled' do - before do - stub_feature_flags(file_identifier_hash: false) - end + it 'returns correct diff file' do + diff_file = subject.diff_file - it 'returns correct diff file' do - diff_file = subject.diff_file - - expect(diff_file.old_path).to eq(position.old_path) - expect(diff_file.new_path).to eq(position.new_path) - expect(diff_file.diff_refs).to eq(position.diff_refs) - end - end - - context 'when file_identifier_hash is enabled' do - before do - stub_feature_flags(file_identifier_hash: true) - end - - it 'returns correct diff file' do - diff_file = subject.diff_file - - expect(diff_file.old_path).to eq(position.old_path) - expect(diff_file.new_path).to eq(position.new_path) - expect(diff_file.diff_refs).to eq(position.diff_refs) - end + expect(diff_file.old_path).to eq(position.old_path) + expect(diff_file.new_path).to eq(position.new_path) + expect(diff_file.diff_refs).to eq(position.diff_refs) end end diff --git a/spec/models/integrations/base_chat_notification_spec.rb b/spec/models/integrations/base_chat_notification_spec.rb index 672d8de1e14..8cbefbb9b47 100644 --- a/spec/models/integrations/base_chat_notification_spec.rb +++ b/spec/models/integrations/base_chat_notification_spec.rb @@ -285,4 +285,22 @@ RSpec.describe Integrations::BaseChatNotification do expect { subject.webhook_placeholder }.to raise_error(NotImplementedError) end end + + describe '#event_channel_name' do + it 'returns the channel field name for the given event' do + expect(subject.event_channel_name(:event)).to eq('event_channel') + end + end + + describe '#get_channel_field' do + it 'returns the channel field value for the given event' do + subject.push_channel = '#pushes' + + expect(subject.get_channel_field(:push)).to eq('#pushes') + end + + it 'raises an error for unsupported events' do + expect { subject.get_channel_field(:foo) }.to raise_error(NoMethodError) + end + end end diff --git a/spec/presenters/gitlab/blame_presenter_spec.rb b/spec/presenters/gitlab/blame_presenter_spec.rb index ff128416692..b3b9e133a73 100644 --- a/spec/presenters/gitlab/blame_presenter_spec.rb +++ b/spec/presenters/gitlab/blame_presenter_spec.rb @@ -8,8 +8,9 @@ RSpec.describe Gitlab::BlamePresenter do let(:commit) { project.commit('master') } let(:blob) { project.repository.blob_at(commit.id, path) } let(:blame) { Gitlab::Blame.new(blob, commit) } + let(:page) { 1 } - subject { described_class.new(blame, project: project, path: path) } + subject { described_class.new(blame, project: project, path: path, page: page) } it 'precalculates necessary data on init' do expect_any_instance_of(described_class) diff --git a/spec/requests/api/integrations_spec.rb b/spec/requests/api/integrations_spec.rb index cd9a0746581..a23c9b9f2ba 100644 --- a/spec/requests/api/integrations_spec.rb +++ b/spec/requests/api/integrations_spec.rb @@ -55,25 +55,20 @@ RSpec.describe API::Integrations do describe "PUT /projects/:id/#{endpoint}/#{integration.dasherize}" do include_context integration - # NOTE: Some attributes are not supported for PUT requests, even though in most cases they should be. - # For some of them the problem is somewhere else, i.e. most chat integrations don't support the `*_channel` - # fields but they're incorrectly included in `#fields`. - # + # NOTE: Some attributes are not supported for PUT requests, even though they probably should be. # We can fix these manually, or with a generic approach like https://gitlab.com/gitlab-org/gitlab/-/issues/348208 - let(:missing_channel_attributes) { %i[push_channel issue_channel confidential_issue_channel merge_request_channel note_channel confidential_note_channel tag_push_channel pipeline_channel wiki_page_channel] } let(:missing_attributes) do { datadog: %i[archive_trace_events], - discord: missing_channel_attributes + %i[branches_to_be_notified notify_only_broken_pipelines], - hangouts_chat: missing_channel_attributes + %i[notify_only_broken_pipelines], + discord: %i[branches_to_be_notified notify_only_broken_pipelines], + hangouts_chat: %i[notify_only_broken_pipelines], jira: %i[issues_enabled project_key vulnerabilities_enabled vulnerabilities_issuetype], mattermost: %i[deployment_channel labels_to_be_notified], - microsoft_teams: missing_channel_attributes, mock_ci: %i[enable_ssl_verification], prometheus: %i[manual_configuration], slack: %i[alert_events alert_channel deployment_channel labels_to_be_notified], - unify_circuit: missing_channel_attributes + %i[branches_to_be_notified notify_only_broken_pipelines], - webex_teams: missing_channel_attributes + %i[branches_to_be_notified notify_only_broken_pipelines] + unify_circuit: %i[branches_to_be_notified notify_only_broken_pipelines], + webex_teams: %i[branches_to_be_notified notify_only_broken_pipelines] } end diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb index 51ef30c91c0..81443eed7d3 100644 --- a/spec/services/draft_notes/publish_service_spec.rb +++ b/spec/services/draft_notes/publish_service_spec.rb @@ -168,7 +168,7 @@ RSpec.describe DraftNotes::PublishService do # NOTE: This should be reduced as we work on reducing Gitaly calls. # Gitaly requests shouldn't go above this threshold as much as possible # as it may add more to the Gitaly N+1 issue we are experiencing. - expect { publish }.to change { Gitlab::GitalyClient.get_request_count }.by(21) + expect { publish }.to change { Gitlab::GitalyClient.get_request_count }.by(20) end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b39153e79fc..6bd16edcb72 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -296,10 +296,6 @@ RSpec.configure do |config| stub_feature_flags(flag => enable_rugged) end - # Disable the usage of file_identifier_hash by default until it is ready - # See https://gitlab.com/gitlab-org/gitlab/-/issues/33867 - stub_feature_flags(file_identifier_hash: false) - # Disable `main_branch_over_master` as we migrate # from `master` to `main` accross our codebase. # It's done in order to preserve the concistency in tests diff --git a/spec/support/shared_examples/controllers/snowplow_event_tracking_examples.rb b/spec/support/shared_examples/controllers/snowplow_event_tracking_examples.rb new file mode 100644 index 00000000000..2f908c4b428 --- /dev/null +++ b/spec/support/shared_examples/controllers/snowplow_event_tracking_examples.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true +# +# Requires a context containing: +# - subject +# - feature_flag_name +# - category +# - action +# - namespace +# - user + +shared_examples 'Snowplow event tracking' do + let(:label) { nil } + let(:project) { nil } + + it 'is not emitted if FF is disabled' do + stub_feature_flags(feature_flag_name => false) + + subject + + expect_no_snowplow_event + end + + it 'is emitted' do + params = { + category: category, + action: action, + namespace: namespace, + user: user, + project: project, + label: label + }.compact + + subject + + expect_snowplow_event(**params) + end +end diff --git a/spec/support/shared_examples/lib/gitlab/position_formatters_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/position_formatters_shared_examples.rb index 326800e6dc2..c9300aff3e6 100644 --- a/spec/support/shared_examples/lib/gitlab/position_formatters_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/position_formatters_shared_examples.rb @@ -32,21 +32,7 @@ RSpec.shared_examples "position formatter" do subject { formatter.to_h } - context 'when file_identifier_hash is disabled' do - before do - stub_feature_flags(file_identifier_hash: false) - end - - it { is_expected.to eq(formatter_hash.except(:file_identifier_hash)) } - end - - context 'when file_identifier_hash is enabled' do - before do - stub_feature_flags(file_identifier_hash: true) - end - - it { is_expected.to eq(formatter_hash) } - end + it { is_expected.to eq(formatter_hash) } end describe '#==' do