Add latest changes from gitlab-org/gitlab@master

.gitlab/issue_templates/Problem_Validation.md

+## Problem Statement
+
+<!-- What is the problem we hope to validate and solve? -->
+
+## Reach
+
+<!-- Please describe who suffers from this problem. Consider referring to our personas, which are described at https://about.gitlab.com/handbook/marketing/product-marketing/roles-personas/ -->
+
+<!-- Please also quantify the problem's reach using the following values, considering an aggregate across GitLab.com and self-managed:
+
+10.0 = Impacts the vast majority (~80% or greater) of our users, prospects, or customers.
+6.0 = Impacts a large percentage (~50% to ~80%) of the above.
+3.0 = Significant reach (~25% to ~50%).
+1.5 = Small reach (~5% to ~25%).
+0.5 = Minimal reach (Less than ~5%). -->
+
+## Impact
+
+<!-- How do we positively impact the users above and GitLab's business by solving this problem? Please describe briefly, and provide a numerical assessment:
+
+3.0 = Massive impact
+2.0 = High impact
+1.0 = Medium impact
+0.5 = Low impact
+0.25 = Minimal impact -->
+
+## Confidence
+
+<!-- How do we know this is a problem? Please provide and link to any supporting information (e.g. data, customer verbatims) and use this basis to provide a numerical assessment on our confidence level in this problem's severity: 
+
+100% = High confidence
+80% = Medium confidence
+50% = Low confidence -->
+
+## Effort
+
+<!-- How much effort do we think it will be to solve this problem? Please include all counterparts (Product, UX, Engineering, etc) in your assessment and quantify the number of person-months needed to dedicate to the effort. 
+
+For example, if the solution will take a product manager, designer, and engineer two weeks of effort - you may quantify this as 1.5 (based on 0.5 months x 3 people). -->
+
+/label ~"workflow::problem backlog"

app/assets/stylesheets/framework/snippets.scss

@@ -39,10 +39,6 @@
   min-height: $header-height;
 }
-.snippet-edited-ago {
-  color: $gray-darkest;
-}
-
 .snippet-actions {
   @include media-breakpoint-up(sm) {
     float: right;

app/controllers/projects/merge_requests_controller.rb

@@ -211,7 +211,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   end
   def discussions
-    merge_request.preload_discussions_diff_highlight
+    merge_request.discussions_diffs.load_highlight
     super
   end

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

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.

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

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

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

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

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

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

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

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
     {

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

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,

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
     {

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

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

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) }

app/services/releases/concerns.rb

@@ -48,25 +48,29 @@ module Releases
         end
       end
-      def milestone
-        return unless params[:milestone]
-        strong_memoize(:milestone) do
+      def milestones
+        return [] unless param_for_milestone_titles_provided?
+        strong_memoize(:milestones) do
           MilestonesFinder.new(
             project: project,
             current_user: current_user,
             project_ids: Array(project.id),
-            title: params[:milestone]
-          ).execute.first
+            state: 'all',
+            title: params[:milestones]
+          ).execute
         end
       end
-      def inexistent_milestone?
-        params[:milestone] && !params[:milestone].empty? && !milestone
+      def inexistent_milestones
+        return [] unless param_for_milestone_titles_provided?
+
+        existing_milestone_titles = milestones.map(&:title)
+        Array(params[:milestones]) - existing_milestone_titles
       end
-      def param_for_milestone_title_provided?
-        params[:milestone].present? || params[:milestone]&.empty?
+      def param_for_milestone_titles_provided?
+        params.key?(:milestones)
       end
     end
   end