Review updates for HA doc cleanup

Better titles and some grammar updates

Gemfile

@@ -419,7 +419,8 @@ end
 gem 'octokit', '~> 4.15'
-gem 'mail_room', '~> 0.10.0'
+# https://gitlab.com/gitlab-org/gitlab/issues/207207
+gem 'gitlab-mail_room', '~> 0.0.2', require: 'mail_room'
 gem 'email_reply_trimmer', '~> 0.1'
 gem 'html2text'

Gemfile.lock

@@ -388,6 +388,7 @@ GEM
       opentracing (~> 0.4)
       redis (> 3.0.0, < 5.0.0)
     gitlab-license (1.0.0)
+    gitlab-mail_room (0.0.2)
     gitlab-markup (1.7.0)
     gitlab-net-dns (0.9.1)
     gitlab-puma (4.3.1.gitlab.2)
@@ -616,7 +617,6 @@ GEM
     lumberjack (1.0.13)
     mail (2.7.1)
       mini_mime (>= 0.1.1)
-    mail_room (0.10.0)
     marcel (0.3.3)
       mimemagic (~> 0.3.2)
     marginalia (1.8.0)
@@ -1235,6 +1235,7 @@ DEPENDENCIES
   gitlab-chronic (~> 0.10.5)
   gitlab-labkit (= 0.10.0)
   gitlab-license (~> 1.0)
+  gitlab-mail_room (~> 0.0.2)
   gitlab-markup (~> 1.7.0)
   gitlab-net-dns (~> 0.9.1)
   gitlab-puma (~> 4.3.1.gitlab.2)
@@ -1284,7 +1285,6 @@ DEPENDENCIES
   loofah (~> 2.2)
   lru_redux
   mail (= 2.7.1)
-  mail_room (~> 0.10.0)
   marginalia (~> 1.8.0)
   memory_profiler (~> 0.9)
   method_source (~> 0.8)

app/assets/javascripts/diffs/components/diff_table_cell.vue

@@ -166,6 +166,7 @@ export default {
         :href="lineHref"
         @click="setHighlightedRow(lineCode)"
       >
+        {{ lineNumber }}
       </a>
       <diff-gutter-avatars
         v-if="shouldShowAvatarsOnGutter"

app/assets/javascripts/notes/stores/getters.js

@@ -28,6 +28,8 @@ export const getUserData = state => state.userData || {};
 export const getUserDataByProp = state => prop => state.userData && state.userData[prop];
+export const descriptionVersion = state => state.descriptionVersion;
+
 export const notesById = state =>
   state.discussions.reduce((acc, note) => {
     note.notes.every(n => Object.assign(acc, { [n.id]: n }));

app/assets/javascripts/vue_shared/components/notes/system_note.vue

@@ -54,8 +54,8 @@ export default {
     };
   },
   computed: {
-    ...mapGetters(['targetNoteHash']),
-    ...mapState(['descriptionVersion', 'isLoadingDescriptionVersion']),
+    ...mapGetters(['targetNoteHash', 'descriptionVersion']),
+    ...mapState(['isLoadingDescriptionVersion']),
     noteAnchorId() {
       return `note_${this.note.id}`;
     },

app/assets/stylesheets/pages/diff.scss

@@ -485,10 +485,6 @@ table.code {
           }
         }
       }
-
-      &:not(.js-unfold-bottom) a::before {
-        content: attr(data-linenumber);
-      }
     }
     &.line_content {

app/models/concerns/cache_markdown_field.rb

@@ -20,6 +20,10 @@ def skip_project_check?
     false
   end
+  def can_cache_field?(field)
+    true
+  end
+
   # Returns the default Banzai render context for the cached markdown field.
   def banzai_render_context(field)
     raise ArgumentError.new("Unknown field: #{field.inspect}") unless
@@ -38,17 +42,23 @@ def banzai_render_context(field)
     context
   end
-  # Update every column in a row if any one is invalidated, as we only store
-  # one version per row
-  def refresh_markdown_cache
+  def rendered_field_content(markdown_field)
+    return unless can_cache_field?(markdown_field)
+
     options = { skip_project_check: skip_project_check? }
+    Banzai::Renderer.cacheless_render_field(self, markdown_field, options)
+  end
+  # Update every applicable column in a row if any one is invalidated, as we only store
+  # one version per row
+  def refresh_markdown_cache
     updates = cached_markdown_fields.markdown_fields.map do |markdown_field|
       [
         cached_markdown_fields.html_field(markdown_field),
-        Banzai::Renderer.cacheless_render_field(self, markdown_field, options)
+        rendered_field_content(markdown_field)
       ]
     end.to_h
+
     updates['cached_markdown_version'] = latest_cached_markdown_version
     updates.each { |field, data| write_markdown_field(field, data) }

app/models/group.rb

@@ -406,11 +406,15 @@ def mattermost_team_params
   end
   def ci_variables_for(ref, project)
-    list_of_ids = [self] + ancestors
-    variables = Ci::GroupVariable.where(group: list_of_ids)
-    variables = variables.unprotected unless project.protected_for?(ref)
-    variables = variables.group_by(&:group_id)
-    list_of_ids.reverse.flat_map { |group| variables[group.id] }.compact
+    cache_key = "ci_variables_for:group:#{self&.id}:project:#{project&.id}:ref:#{ref}"
+
+    ::Gitlab::SafeRequestStore.fetch(cache_key) do
+      list_of_ids = [self] + ancestors
+      variables = Ci::GroupVariable.where(group: list_of_ids)
+      variables = variables.unprotected unless project.protected_for?(ref)
+      variables = variables.group_by(&:group_id)
+      list_of_ids.reverse.flat_map { |group| variables[group.id] }.compact
+    end
   end
   def group_member(user)

app/models/project.rb

@@ -1963,6 +1963,14 @@ def default_environment
   end
   def ci_variables_for(ref:, environment: nil)
+    cache_key = "ci_variables_for:project:#{self&.id}:ref:#{ref}:environment:#{environment}"
+
+    ::Gitlab::SafeRequestStore.fetch(cache_key) do
+      uncached_ci_variables_for(ref: ref, environment: environment)
+    end
+  end
+
+  def uncached_ci_variables_for(ref:, environment: nil)
     result = if protected_for?(ref)
                variables
              else

app/models/concerns/resource_event_tools.rb → app/models/resource_event.rb

 # frozen_string_literal: true
-module ResourceEventTools
-  extend ActiveSupport::Concern
-  included do
-    belongs_to :user
-    validates :user, presence: { unless: :importing? }, on: :create
-    validate :exactly_one_issuable
-    scope :created_after, ->(time) { where('created_at > ?', time) }
+class ResourceEvent < ApplicationRecord
+  include Gitlab::Utils::StrongMemoize
+  include Importable
+  self.abstract_class = true
+  validates :user, presence: { unless: :importing? }, on: :create
+  belongs_to :user
+  scope :created_after, ->(time) { where('created_at > ?', time) }
+
+  def discussion_id
+    strong_memoize(:discussion_id) do
+      Digest::SHA1.hexdigest(discussion_id_key.join("-"))
+    end
+  end
+
+  private
+
+  def discussion_id_key
+    [self.class.name, created_at, user_id]
   end
   def exactly_one_issuable

app/models/resource_label_event.rb

 # frozen_string_literal: true
-class ResourceLabelEvent < ApplicationRecord
-  include Importable
-  include Gitlab::Utils::StrongMemoize
+class ResourceLabelEvent < ResourceEvent
   include CacheMarkdownField
-  include ResourceEventTools
   cache_markdown_field :reference
@@ -13,8 +10,11 @@ class ResourceLabelEvent < ApplicationRecord
   belongs_to :label
   scope :inc_relations, -> { includes(:label, :user) }
+  scope :by_issue, ->(issue) { where(issue_id: issue.id) }
+  scope :by_merge_request, ->(merge_request) { where(merge_request_id: merge_request.id) }
   validates :label, presence: { unless: :importing? }, on: :create
+  validate :exactly_one_issuable
   after_save :expire_etag_cache
   after_destroy :expire_etag_cache
@@ -41,12 +41,6 @@ def issuable
     issue || merge_request
   end
-  def discussion_id(resource = nil)
-    strong_memoize(:discussion_id) do
-      Digest::SHA1.hexdigest(discussion_id_key.join("-"))
-    end
-  end
-
   def project
     issuable.project
   end
@@ -109,10 +103,6 @@ def local_label?
   def resource_parent
     issuable.project || issuable.group
   end
-
-  def discussion_id_key
-    [self.class.name, created_at, user_id]
-  end
 end
 ResourceLabelEvent.prepend_if_ee('EE::ResourceLabelEvent')

app/models/resource_milestone_event.rb

 # frozen_string_literal: true
-class ResourceMilestoneEvent < ApplicationRecord
-  include Gitlab::Utils::StrongMemoize
-  include Importable
-  include ResourceEventTools
-
+class ResourceMilestoneEvent < ResourceEvent
   belongs_to :issue
   belongs_to :merge_request
   belongs_to :milestone
@@ -12,6 +8,8 @@ class ResourceMilestoneEvent < ApplicationRecord
   scope :by_issue, ->(issue) { where(issue_id: issue.id) }
   scope :by_merge_request, ->(merge_request) { where(merge_request_id: merge_request.id) }
+  validate :exactly_one_issuable
+
   enum action: {
          add: 1,
          remove: 2
@@ -23,8 +21,4 @@ class ResourceMilestoneEvent < ApplicationRecord
   def self.issuable_attrs
     %i(issue merge_request).freeze
   end
-
-  def resource
-    issue || merge_request
-  end
 end

app/models/resource_weight_event.rb

 # frozen_string_literal: true
-class ResourceWeightEvent < ApplicationRecord
-  include Gitlab::Utils::StrongMemoize
-
-  validates :user, presence: true
+class ResourceWeightEvent < ResourceEvent
   validates :issue, presence: true
-  belongs_to :user
   belongs_to :issue
   scope :by_issue, ->(issue) { where(issue_id: issue.id) }
-  scope :created_after, ->(time) { where('created_at > ?', time) }
-
-  def discussion_id(resource = nil)
-    strong_memoize(:discussion_id) do
-      Digest::SHA1.hexdigest(discussion_id_key.join("-"))
-    end
-  end
-
-  private
-
-  def discussion_id_key
-    [self.class.name, created_at, user_id]
-  end
 end

app/models/snippet.rb

@@ -301,6 +301,10 @@ def track_snippet_repository
     repository.update!(shard_name: repository_storage, disk_path: disk_path)
   end
+  def can_cache_field?(field)
+    field != :content || MarkupHelper.gitlab_markdown?(file_name)
+  end
+
   class << self
     # Searches for snippets with a matching title or file name.
     #

app/models/user_bot_type_enums.rb

@@ -2,7 +2,7 @@
 module UserBotTypeEnums
   def self.bots
-    # When adding a new key, please ensure you are not conflicting with EE-only keys in app/models/user_bot_types_enums.rb
+    # When adding a new key, please ensure you are not conflicting with EE-only keys in app/models/user_bot_type_enums.rb
     {
       alert_bot: 2
     }

bin/mail_room

@@ -19,7 +19,7 @@ get_mail_room_pid()
 start()
 {
-  bin/daemon_with_pidfile $mail_room_pidfile bundle exec mail_room -q -c $mail_room_config >> $mail_room_logfile 2>&1
+  bin/daemon_with_pidfile $mail_room_pidfile bundle exec mail_room --log-exit-as json -q -c $mail_room_config >> $mail_room_logfile 2>&1
 }
 stop()

changelogs/unreleased/201771.yml

+---
+title: Replace content_viewer_spec setTimeouts with semantic actions / events
+merge_request:
+author: Oregand
+type: other

changelogs/unreleased/202639.yml

+---
+title: Replace line diff number css selector with actual HTML inside MRs
+merge_request:
+author: Oregand
+type: other

changelogs/unreleased/207126-more-descriptive-error-messages-in-migration-helpers.yml

+---
+title: Improve error messages of failed migrations
+merge_request: 25457
+author:
+type: changed

changelogs/unreleased/207976-stop-markdown-caching-of-non-markdown-snippet-content.yml

+---
+title: Fix Snippet content incorrectly caching
+merge_request: 25985
+author:
+type: fixed