diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2019-09-16 15:06:26 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2019-09-16 15:06:26 +0000 |
commit | 84727c8209a4412e21111a07f99b0438b03232de (patch) | |
tree | 1fcfa02b01548c3cdc561186870a1c807f227f0b /app | |
parent | d2798d607e11e0ebae83ae909404834388733428 (diff) | |
download | gitlab-ce-84727c8209a4412e21111a07f99b0438b03232de.tar.gz |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'app')
23 files changed, 154 insertions, 112 deletions
diff --git a/app/assets/stylesheets/framework/snippets.scss b/app/assets/stylesheets/framework/snippets.scss index 3ab83f4c8e6..f57b1d9f351 100644 --- a/app/assets/stylesheets/framework/snippets.scss +++ b/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; diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 0d992bb35f8..af87e04f9c8 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/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 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) } diff --git a/app/services/releases/concerns.rb b/app/services/releases/concerns.rb index b5412e97284..a0ebaea77c8 100644 --- a/app/services/releases/concerns.rb +++ b/app/services/releases/concerns.rb @@ -48,25 +48,29 @@ module Releases end end - def milestone - return unless params[:milestone] + def milestones + return [] unless param_for_milestone_titles_provided? - strong_memoize(:milestone) do + 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 diff --git a/app/services/releases/create_service.rb b/app/services/releases/create_service.rb index c91d43084d3..9a0a876454f 100644 --- a/app/services/releases/create_service.rb +++ b/app/services/releases/create_service.rb @@ -7,7 +7,7 @@ module Releases def execute return error('Access Denied', 403) unless allowed? return error('Release already exists', 409) if release - return error('Milestone does not exist', 400) if inexistent_milestone? + return error("Milestone(s) not found: #{inexistent_milestones.join(', ')}", 400) if inexistent_milestones.any? tag = ensure_tag @@ -61,7 +61,7 @@ module Releases sha: tag.dereferenced_target.sha, released_at: released_at, links_attributes: params.dig(:assets, 'links') || [], - milestone: milestone + milestones: milestones ) end end diff --git a/app/services/releases/update_service.rb b/app/services/releases/update_service.rb index 70acc68f747..7aa51c4a332 100644 --- a/app/services/releases/update_service.rb +++ b/app/services/releases/update_service.rb @@ -9,9 +9,9 @@ module Releases return error('Release does not exist', 404) unless release return error('Access Denied', 403) unless allowed? return error('params is empty', 400) if empty_params? - return error('Milestone does not exist', 400) if inexistent_milestone? + return error("Milestone(s) not found: #{inexistent_milestones.join(', ')}", 400) if inexistent_milestones.any? - params[:milestone] = milestone if param_for_milestone_title_provided? + params[:milestones] = milestones if param_for_milestone_titles_provided? if release.update(params) success(tag: existing_tag, release: release) diff --git a/app/views/shared/snippets/_form.html.haml b/app/views/shared/snippets/_form.html.haml index 2d2382e469a..73401029da4 100644 --- a/app/views/shared/snippets/_form.html.haml +++ b/app/views/shared/snippets/_form.html.haml @@ -22,7 +22,7 @@ = f.label :file_name, "File" .col-sm-10 .file-holder.snippet - .js-file-title.file-title + .js-file-title.file-title-flex-parent = f.text_field :file_name, placeholder: "Optionally name this file to add code highlighting, e.g. example.rb for Ruby.", class: 'form-control snippet-file-name qa-snippet-file-name' .file-content.code %pre#editor= @snippet.content diff --git a/app/views/shared/snippets/_header.html.haml b/app/views/shared/snippets/_header.html.haml index 1a9ae68f53d..69481293f90 100644 --- a/app/views/shared/snippets/_header.html.haml +++ b/app/views/shared/snippets/_header.html.haml @@ -28,7 +28,7 @@ = @snippet.description - if @snippet.updated_at != @snippet.created_at - = edited_time_ago_with_tooltip(@snippet, placement: 'bottom', html_class: 'snippet-edited-ago', exclude_author: true) + = edited_time_ago_with_tooltip(@snippet, placement: 'bottom', exclude_author: true) - if @snippet.embeddable? .embed-snippet |