Add latest changes from gitlab-org/gitlab@master

16 files changed, 134 insertions, 92 deletions

diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 0b00cf10714..aa7286a9971 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -108,13 +108,10 @@ class DiffNote < Note
   end
 
   def fetch_diff_file
+    return note_diff_file.raw_diff_file if note_diff_file
+
     file =
-      if note_diff_file
-        diff = Gitlab::Git::Diff.new(note_diff_file.to_hash)
-        Gitlab::Diff::File.new(diff,
-                               repository: repository,
-                               diff_refs: original_position.diff_refs)
-      elsif created_at_diff?(noteable.diff_refs)
+      if created_at_diff?(noteable.diff_refs)
         # We're able to use the already persisted diffs (Postgres) if we're
         # presenting a "current version" of the MR discussion diff.
         # So no need to make an extra Gitaly diff request for it.
@@ -126,9 +123,7 @@ class DiffNote < Note
         original_position.diff_file(repository)
       end
 
-    # Since persisted diff files already have its content "unfolded"
-    # there's no need to make it pass through the unfolding process.
-    file&.unfold_diff_lines(position) unless note_diff_file
+    file&.unfold_diff_lines(position)
 
     file
   end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 90061fe181e..ac26d29ad19 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -454,24 +454,17 @@ class MergeRequest < ApplicationRecord
     true
   end
 
-  def preload_discussions_diff_highlight
-    preloadable_files = note_diff_files.for_commit_or_unresolved
-
-    discussions_diffs.load_highlight(preloadable_files.pluck(:id))
-  end
-
   def discussions_diffs
     strong_memoize(:discussions_diffs) do
+      note_diff_files = NoteDiffFile
+        .joins(:diff_note)
+        .merge(notes.or(commit_notes))
+        .includes(diff_note: :project)
+
       Gitlab::DiscussionsDiff::FileCollection.new(note_diff_files.to_a)
     end
   end
 
-  def note_diff_files
-    NoteDiffFile
-      .where(diff_note: discussion_notes)
-      .includes(diff_note: :project)
-  end
-
   def diff_size
     # Calling `merge_request_diff.diffs.real_size` will also perform
     # highlighting, which we don't need here.
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 4b9fee2bbdf..800c492e8e2 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -27,11 +27,8 @@ class Milestone < ApplicationRecord
   belongs_to :project
   belongs_to :group
 
-  # A one-to-one relationship is set up here as part of a MVC: https://gitlab.com/gitlab-org/gitlab-ce/issues/62402
-  # However, on the long term, we will want a many-to-many relationship between Release and Milestone.
-  # The "has_one through" allows us today to set up this one-to-one relationship while setting up the architecture for the long-term (ie intermediate table).
-  has_one :milestone_release
-  has_one :release, through: :milestone_release
+  has_many :milestone_releases
+  has_many :releases, through: :milestone_releases
 
   has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.milestones&.maximum(:iid) }
   has_internal_id :iid, scope: :group, init: ->(s) { s&.group&.milestones&.maximum(:iid) }
@@ -68,7 +65,7 @@ class Milestone < ApplicationRecord
   validate :milestone_type_check
   validate :start_date_should_be_less_than_due_date, if: proc { |m| m.start_date.present? && m.due_date.present? }
   validate :dates_within_4_digits
-  validates_associated :milestone_release, message: -> (_, obj) { obj[:value].errors.full_messages.join(",") }
+  validates_associated :milestone_releases, message: -> (_, obj) { obj[:value].map(&:errors).map(&:full_messages).join(",") }
 
   strip_attributes :title
 
diff --git a/app/models/milestone_release.rb b/app/models/milestone_release.rb
index c8743a8cad8..f7127df339d 100644
--- a/app/models/milestone_release.rb
+++ b/app/models/milestone_release.rb
@@ -4,9 +4,11 @@ class MilestoneRelease < ApplicationRecord
   belongs_to :milestone
   belongs_to :release
 
-  validates :milestone_id, uniqueness: { scope: [:release_id] }
   validate :same_project_between_milestone_and_release
 
+  # Keep until 2019-11-29
+  self.ignored_columns += %i[id]
+
   private
 
   def same_project_between_milestone_and_release
diff --git a/app/models/note_diff_file.rb b/app/models/note_diff_file.rb
index fcc9e2b3fd8..67a6d5d6d6b 100644
--- a/app/models/note_diff_file.rb
+++ b/app/models/note_diff_file.rb
@@ -3,15 +3,11 @@
 class NoteDiffFile < ApplicationRecord
   include DiffFile
 
-  scope :for_commit_or_unresolved, -> do
-    joins(:diff_note).where("resolved_at IS NULL OR noteable_type = 'Commit'")
-  end
-
   scope :referencing_sha, -> (oids, project_id:) do
     joins(:diff_note).where(notes: { project_id: project_id, commit_id: oids })
   end
 
-  delegate :original_position, :project, to: :diff_note
+  delegate :original_position, :project, :resolved_at, to: :diff_note
 
   belongs_to :diff_note, inverse_of: :note_diff_file
 
diff --git a/app/models/project_services/bugzilla_service.rb b/app/models/project_services/bugzilla_service.rb
index 8b79b5e9f0c..0a498fde95a 100644
--- a/app/models/project_services/bugzilla_service.rb
+++ b/app/models/project_services/bugzilla_service.rb
@@ -3,8 +3,6 @@
 class BugzillaService < IssueTrackerService
   validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
 
-  prop_accessor :project_url, :issues_url, :new_issue_url
-
   def default_title
     'Bugzilla'
   end
diff --git a/app/models/project_services/custom_issue_tracker_service.rb b/app/models/project_services/custom_issue_tracker_service.rb
index 535fcf6b94e..dbc42b1b86d 100644
--- a/app/models/project_services/custom_issue_tracker_service.rb
+++ b/app/models/project_services/custom_issue_tracker_service.rb
@@ -3,8 +3,6 @@
 class CustomIssueTrackerService < IssueTrackerService
   validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
 
-  prop_accessor :title, :description, :project_url, :issues_url, :new_issue_url
-
   def default_title
     'Custom Issue Tracker'
   end
diff --git a/app/models/project_services/data_fields.rb b/app/models/project_services/data_fields.rb
index 438d85098c8..0f5385f8ce2 100644
--- a/app/models/project_services/data_fields.rb
+++ b/app/models/project_services/data_fields.rb
@@ -3,8 +3,56 @@
 module DataFields
   extend ActiveSupport::Concern
 
+  class_methods do
+    # Provide convenient accessor methods for data fields.
+    # TODO: Simplify as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
+    def data_field(*args)
+      args.each do |arg|
+        self.class_eval <<-RUBY, __FILE__, __LINE__ + 1
+          unless method_defined?(arg)
+            def #{arg}
+              data_fields.send('#{arg}') || (properties && properties['#{arg}'])
+            end
+          end
+
+          def #{arg}=(value)
+            @old_data_fields ||= {}
+            @old_data_fields['#{arg}'] ||= #{arg} # set only on the first assignment, IOW we remember the original value only
+            data_fields.send('#{arg}=', value)
+          end
+
+          def #{arg}_touched?
+            @old_data_fields ||= {}
+            @old_data_fields.has_key?('#{arg}')
+          end
+
+          def #{arg}_changed?
+            #{arg}_touched? && @old_data_fields['#{arg}'] != #{arg}
+          end
+
+          def #{arg}_was
+            return unless #{arg}_touched?
+            return if data_fields.persisted? # arg_was does not work for attr_encrypted
+
+            legacy_properties_data['#{arg}']
+          end
+        RUBY
+      end
+    end
+  end
+
   included do
-    has_one :issue_tracker_data
-    has_one :jira_tracker_data
+    has_one :issue_tracker_data, autosave: true
+    has_one :jira_tracker_data, autosave: true
+
+    def data_fields
+      raise NotImplementedError
+    end
+
+    def data_fields_present?
+      data_fields.persisted?
+    rescue NotImplementedError
+      false
+    end
   end
 end
diff --git a/app/models/project_services/gitlab_issue_tracker_service.rb b/app/models/project_services/gitlab_issue_tracker_service.rb
index 51032932eab..ec28602b5e6 100644
--- a/app/models/project_services/gitlab_issue_tracker_service.rb
+++ b/app/models/project_services/gitlab_issue_tracker_service.rb
@@ -5,8 +5,6 @@ class GitlabIssueTrackerService < IssueTrackerService
 
   validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
 
-  prop_accessor :project_url, :issues_url, :new_issue_url
-
   default_value_for :default, true
 
   def default_title
diff --git a/app/models/project_services/issue_tracker_data.rb b/app/models/project_services/issue_tracker_data.rb
index 2c1d28ed421..b1d67657fe6 100644
--- a/app/models/project_services/issue_tracker_data.rb
+++ b/app/models/project_services/issue_tracker_data.rb
@@ -6,9 +6,6 @@ class IssueTrackerData < ApplicationRecord
   delegate :activated?, to: :service, allow_nil: true
 
   validates :service, presence: true
-  validates :project_url, presence: true, public_url: { enforce_sanitization: true }, if: :activated?
-  validates :issues_url, presence: true, public_url: { enforce_sanitization: true }, if: :activated?
-  validates :new_issue_url, public_url: { enforce_sanitization: true }, if: :activated?
 
   def self.encryption_options
     {
diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb
index b6ad46513db..c201bd2ea18 100644
--- a/app/models/project_services/issue_tracker_service.rb
+++ b/app/models/project_services/issue_tracker_service.rb
@@ -3,9 +3,14 @@
 class IssueTrackerService < Service
   validate :one_issue_tracker, if: :activated?, on: :manual_change
 
+  # TODO: we can probably just delegate as part of
+  # https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
+  data_field :project_url, :issues_url, :new_issue_url
+
   default_value_for :category, 'issue_tracker'
 
-  before_save :handle_properties
+  before_validation :handle_properties
+  before_validation :set_default_data, on: :create
 
   # Pattern used to extract links from comments
   # Override this method on services that uses different patterns
@@ -43,12 +48,31 @@ class IssueTrackerService < Service
   end
 
   def handle_properties
-    properties.slice('title', 'description').each do |key, _|
+    # this has been moved from initialize_properties and should be improved
+    # as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
+    return unless properties
+
+    @legacy_properties_data = properties.dup
+    data_values = properties.slice!('title', 'description')
+    properties.each do |key, _|
       current_value = self.properties.delete(key)
       value = attribute_changed?(key) ? attribute_change(key).last : current_value
 
       write_attribute(key, value)
     end
+
+    data_values.reject! { |key| data_fields.changed.include?(key) }
+    data_fields.assign_attributes(data_values) if data_values.present?
+
+    self.properties = {}
+  end
+
+  def legacy_properties_data
+    @legacy_properties_data ||= {}
+  end
+
+  def data_fields
+    issue_tracker_data || self.build_issue_tracker_data
   end
 
   def default?
@@ -56,7 +80,7 @@ class IssueTrackerService < Service
   end
 
   def issue_url(iid)
-    self.issues_url.gsub(':id', iid.to_s)
+    issues_url.gsub(':id', iid.to_s)
   end
 
   def issue_tracker_path
@@ -80,25 +104,22 @@ class IssueTrackerService < Service
     ]
   end
 
+  def initialize_properties
+    {}
+  end
+
   # Initialize with default properties values
-  # or receive a block with custom properties
-  def initialize_properties(&block)
-    return unless properties.nil?
-
-    if enabled_in_gitlab_config
-      if block_given?
-        yield
-      else
-        self.properties = {
-          title: issues_tracker['title'],
-          project_url: issues_tracker['project_url'],
-          issues_url: issues_tracker['issues_url'],
-          new_issue_url: issues_tracker['new_issue_url']
-        }
-      end
-    else
-      self.properties = {}
-    end
+  def set_default_data
+    return unless issues_tracker.present?
+
+    self.title ||= issues_tracker['title']
+
+    # we don't want to override if we have set something
+    return if project_url || issues_url || new_issue_url
+
+    data_fields.project_url = issues_tracker['project_url']
+    data_fields.issues_url = issues_tracker['issues_url']
+    data_fields.new_issue_url = issues_tracker['new_issue_url']
   end
 
   def self.supported_events
diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb
index 0728c83005e..61ae78a0b95 100644
--- a/app/models/project_services/jira_service.rb
+++ b/app/models/project_services/jira_service.rb
@@ -17,7 +17,10 @@ class JiraService < IssueTrackerService
   # Jira Cloud version is deprecating authentication via username and password.
   # We should use username/password for Jira Server and email/api_token for Jira Cloud,
   # for more information check: https://gitlab.com/gitlab-org/gitlab-ce/issues/49936.
-  prop_accessor :username, :password, :url, :api_url, :jira_issue_transition_id
+
+  # TODO: we can probably just delegate as part of
+  # https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
+  data_field :username, :password, :url, :api_url, :jira_issue_transition_id
 
   before_update :reset_password
 
@@ -35,24 +38,34 @@ class JiraService < IssueTrackerService
   end
 
   def initialize_properties
-    super do
-      self.properties = {
-        url: issues_tracker['url'],
-        api_url: issues_tracker['api_url']
-      }
-    end
+    {}
+  end
+
+  def data_fields
+    jira_tracker_data || self.build_jira_tracker_data
   end
 
   def reset_password
-    self.password = nil if reset_password?
+    data_fields.password = nil if reset_password?
+  end
+
+  def set_default_data
+    return unless issues_tracker.present?
+
+    self.title ||= issues_tracker['title']
+
+    return if url
+
+    data_fields.url ||= issues_tracker['url']
+    data_fields.api_url ||= issues_tracker['api_url']
   end
 
   def options
     url = URI.parse(client_url)
 
     {
-      username: self.username,
-      password: self.password,
+      username: username,
+      password: password,
       site: URI.join(url, '/').to_s, # Intended to find the root
       context_path: url.path,
       auth_type: :basic,
diff --git a/app/models/project_services/jira_tracker_data.rb b/app/models/project_services/jira_tracker_data.rb
index 4f528e3d81b..e4e0f64150a 100644
--- a/app/models/project_services/jira_tracker_data.rb
+++ b/app/models/project_services/jira_tracker_data.rb
@@ -6,13 +6,6 @@ class JiraTrackerData < ApplicationRecord
   delegate :activated?, to: :service, allow_nil: true
 
   validates :service, presence: true
-  validates :url, public_url: { enforce_sanitization: true }, presence: true, if: :activated?
-  validates :api_url, public_url: { enforce_sanitization: true }, allow_blank: true
-  validates :username, presence: true, if: :activated?
-  validates :password, presence: true, if: :activated?
-  validates :jira_issue_transition_id,
-            format: { with: Gitlab::Regex.jira_transition_id_regex, message: s_("JiraService|transition ids can have only numbers which can be split with , or ;") },
-            allow_blank: true
 
   def self.encryption_options
     {
diff --git a/app/models/project_services/redmine_service.rb b/app/models/project_services/redmine_service.rb
index 5ca057ca833..a4ca0d20669 100644
--- a/app/models/project_services/redmine_service.rb
+++ b/app/models/project_services/redmine_service.rb
@@ -3,8 +3,6 @@
 class RedmineService < IssueTrackerService
   validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
 
-  prop_accessor :project_url, :issues_url, :new_issue_url
-
   def default_title
     'Redmine'
   end
diff --git a/app/models/project_services/youtrack_service.rb b/app/models/project_services/youtrack_service.rb
index f9de1f7dc49..0416eaa5be0 100644
--- a/app/models/project_services/youtrack_service.rb
+++ b/app/models/project_services/youtrack_service.rb
@@ -3,8 +3,6 @@
 class YoutrackService < IssueTrackerService
   validates :project_url, :issues_url, presence: true, public_url: true, if: :activated?
 
-  prop_accessor :project_url, :issues_url
-
   # {PROJECT-KEY}-{NUMBER} Examples: YT-1, PRJ-1, gl-030
   def self.reference_pattern(only_long: false)
     if only_long
diff --git a/app/models/release.rb b/app/models/release.rb
index b2e65974aa0..cd63b4d5fef 100644
--- a/app/models/release.rb
+++ b/app/models/release.rb
@@ -12,11 +12,8 @@ class Release < ApplicationRecord
 
   has_many :links, class_name: 'Releases::Link'
 
-  # A one-to-one relationship is set up here as part of a MVC: https://gitlab.com/gitlab-org/gitlab-ce/issues/62402
-  # However, on the long term, we will want a many-to-many relationship between Release and Milestone.
-  # The "has_one through" allows us today to set up this one-to-one relationship while setting up the architecture for the long-term (ie intermediate table).
-  has_one :milestone_release
-  has_one :milestone, through: :milestone_release
+  has_many :milestone_releases
+  has_many :milestones, through: :milestone_releases
 
   default_value_for :released_at, allows_nil: false do
     Time.zone.now
@@ -26,7 +23,7 @@ class Release < ApplicationRecord
 
   validates :description, :project, :tag, presence: true
   validates :name, presence: true, on: :create
-  validates_associated :milestone_release, message: -> (_, obj) { obj[:value].errors.full_messages.join(",") }
+  validates_associated :milestone_releases, message: -> (_, obj) { obj[:value].map(&:errors).map(&:full_messages).join(",") }
 
   scope :sorted, -> { order(released_at: :desc) }