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 | |
| parent | d2798d607e11e0ebae83ae909404834388733428 (diff) | |
| download | gitlab-ce-84727c8209a4412e21111a07f99b0438b03232de.tar.gz | |
Add latest changes from gitlab-org/gitlab@master
67 files changed, 1164 insertions, 620 deletions
diff --git a/.gitlab/issue_templates/Problem_Validation.md b/.gitlab/issue_templates/Problem_Validation.md new file mode 100644 index 00000000000..d2bab21eb06 --- /dev/null +++ b/.gitlab/issue_templates/Problem_Validation.md @@ -0,0 +1,41 @@ +## 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" 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 diff --git a/changelogs/unreleased/66986-milestone-release-many-to-many.yml b/changelogs/unreleased/66986-milestone-release-many-to-many.yml new file mode 100644 index 00000000000..e814cb2cb59 --- /dev/null +++ b/changelogs/unreleased/66986-milestone-release-many-to-many.yml @@ -0,0 +1,5 @@ +--- +title: Switch Milestone and Release to a many-to-many relationship +merge_request: 16517 +author: +type: changed diff --git a/changelogs/unreleased/language-trends-over-time.yml b/changelogs/unreleased/language-trends-over-time.yml new file mode 100644 index 00000000000..f7a5cb12fa1 --- /dev/null +++ b/changelogs/unreleased/language-trends-over-time.yml @@ -0,0 +1,5 @@ +--- +title: Database table for tracking programming language trends over time +merge_request: 16491 +author: +type: added diff --git a/changelogs/unreleased/osw-improve-discussions-query.yml b/changelogs/unreleased/osw-improve-discussions-query.yml new file mode 100644 index 00000000000..a5cd6c1ea32 --- /dev/null +++ b/changelogs/unreleased/osw-improve-discussions-query.yml @@ -0,0 +1,5 @@ +--- +title: Considerably improve the query performance for MR discussions load +merge_request: 16635 +author: +type: performance diff --git a/config/routes.rb b/config/routes.rb index 9dbcd7fffa2..129f4b68212 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -117,7 +117,7 @@ Rails.application.routes.draw do end Gitlab.ee do - constraints(::Constraints::FeatureConstrainer.new(:analytics)) do + constraints(-> (*) { Gitlab::Analytics.any_features_enabled? }) do draw :analytics end end diff --git a/db/migrate/20190910125852_create_analytics_language_trend_repository_languages.rb b/db/migrate/20190910125852_create_analytics_language_trend_repository_languages.rb new file mode 100644 index 00000000000..07da4c20d55 --- /dev/null +++ b/db/migrate/20190910125852_create_analytics_language_trend_repository_languages.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +class CreateAnalyticsLanguageTrendRepositoryLanguages < ActiveRecord::Migration[5.2] + DOWNTIME = false + INDEX_PREFIX = 'analytics_repository_languages_' + + def change + create_table :analytics_language_trend_repository_languages, id: false do |t| + t.integer :file_count, null: false, default: 0 + t.references :programming_language, { + null: false, + foreign_key: { on_delete: :cascade }, + index: false + } + t.references :project, { + null: false, + foreign_key: { on_delete: :cascade }, + index: { name: INDEX_PREFIX + 'on_project_id' } + } + t.integer :loc, null: false, default: 0 + t.integer :bytes, null: false, default: 0 + # Storing percentage (with 2 decimal places), on 2 bytes. + # 50.25% => 5025 + # Max: 100.00% => 10000 (fits smallint: 32767) + t.integer :percentage, limit: 2, null: false, default: 0 + t.date :snapshot_date, null: false + end + + add_index :analytics_language_trend_repository_languages, %I[ + programming_language_id + project_id + snapshot_date + ], name: INDEX_PREFIX + 'unique_index', unique: true + end +end diff --git a/db/post_migrate/20190904205212_remove_id_column_from_intermediate_release_milestones.rb b/db/post_migrate/20190904205212_remove_id_column_from_intermediate_release_milestones.rb new file mode 100644 index 00000000000..e4b65d26db6 --- /dev/null +++ b/db/post_migrate/20190904205212_remove_id_column_from_intermediate_release_milestones.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class RemoveIdColumnFromIntermediateReleaseMilestones < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def change + remove_column :milestone_releases, :id, :bigint + end +end diff --git a/db/schema.rb b/db/schema.rb index b43779f4126..1460a8b11e8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -81,6 +81,18 @@ ActiveRecord::Schema.define(version: 2019_09_12_061145) do t.index ["start_event_label_id"], name: "index_analytics_ca_project_stages_on_start_event_label_id" end + create_table "analytics_language_trend_repository_languages", id: false, force: :cascade do |t| + t.integer "file_count", default: 0, null: false + t.bigint "programming_language_id", null: false + t.bigint "project_id", null: false + t.integer "loc", default: 0, null: false + t.integer "bytes", default: 0, null: false + t.integer "percentage", limit: 2, default: 0, null: false + t.date "snapshot_date", null: false + t.index ["programming_language_id", "project_id", "snapshot_date"], name: "analytics_repository_languages_unique_index", unique: true + t.index ["project_id"], name: "analytics_repository_languages_on_project_id" + end + create_table "appearances", id: :serial, force: :cascade do |t| t.string "title", null: false t.text "description", null: false @@ -2196,7 +2208,7 @@ ActiveRecord::Schema.define(version: 2019_09_12_061145) do t.index ["user_id"], name: "index_merge_trains_on_user_id" end - create_table "milestone_releases", force: :cascade do |t| + create_table "milestone_releases", id: false, force: :cascade do |t| t.bigint "milestone_id", null: false t.bigint "release_id", null: false t.index ["milestone_id", "release_id"], name: "index_miletone_releases_on_milestone_and_release", unique: true @@ -3762,6 +3774,8 @@ ActiveRecord::Schema.define(version: 2019_09_12_061145) do add_foreign_key "analytics_cycle_analytics_project_stages", "labels", column: "end_event_label_id", on_delete: :cascade add_foreign_key "analytics_cycle_analytics_project_stages", "labels", column: "start_event_label_id", on_delete: :cascade add_foreign_key "analytics_cycle_analytics_project_stages", "projects", on_delete: :cascade + add_foreign_key "analytics_language_trend_repository_languages", "programming_languages", on_delete: :cascade + add_foreign_key "analytics_language_trend_repository_languages", "projects", on_delete: :cascade add_foreign_key "application_settings", "namespaces", column: "custom_project_templates_group_id", on_delete: :nullify add_foreign_key "application_settings", "projects", column: "file_template_project_id", name: "fk_ec757bd087", on_delete: :nullify add_foreign_key "application_settings", "projects", column: "instance_administration_project_id", on_delete: :nullify diff --git a/doc/api/releases/index.md b/doc/api/releases/index.md index 8d5b3a65789..4279821d8ed 100644 --- a/doc/api/releases/index.md +++ b/doc/api/releases/index.md @@ -57,19 +57,34 @@ Example response: "committer_email":"admin@example.com", "committed_date":"2019-01-03T01:55:38.000Z" }, - "milestone":{ - "id":51, - "iid":1, - "project_id":24, - "title":"v1.0-rc", - "description":"Voluptate fugiat possimus quis quod aliquam expedita.", - "state":"closed", - "created_at":"2019-07-12T19:45:44.256Z", - "updated_at":"2019-07-12T19:45:44.256Z", - "due_date":"2019-08-16T11:00:00.256Z", - "start_date":"2019-07-30T12:00:00.256Z", - "web_url":"http://localhost:3000/root/awesome-app/-/milestones/1" - }, + "milestones": [ + { + "id":51, + "iid":1, + "project_id":24, + "title":"v1.0-rc", + "description":"Voluptate fugiat possimus quis quod aliquam expedita.", + "state":"closed", + "created_at":"2019-07-12T19:45:44.256Z", + "updated_at":"2019-07-12T19:45:44.256Z", + "due_date":"2019-08-16T11:00:00.256Z", + "start_date":"2019-07-30T12:00:00.256Z", + "web_url":"https://gitlab.example.com/root/awesome-app/-/milestones/1" + }, + { + "id":52, + "iid":2, + "project_id":24, + "title":"v1.0", + "description":"Voluptate fugiat possimus quis quod aliquam expedita.", + "state":"closed", + "created_at":"2019-07-16T14:00:12.256Z", + "updated_at":"2019-07-16T14:00:12.256Z", + "due_date":"2019-08-16T11:00:00.256Z", + "start_date":"2019-07-30T12:00:00.256Z", + "web_url":"https://gitlab.example.com/root/awesome-app/-/milestones/2" + } + ], "assets":{ "count":6, "sources":[ @@ -218,19 +233,34 @@ Example response: "committer_email":"admin@example.com", "committed_date":"2019-01-03T01:53:28.000Z" }, - "milestone":{ - "id":51, - "iid":1, - "project_id":24, - "title":"v1.0-rc", - "description":"Voluptate fugiat possimus quis quod aliquam expedita.", - "state":"closed", - "created_at":"2019-07-12T19:45:44.256Z", - "updated_at":"2019-07-12T19:45:44.256Z", - "due_date":"2019-08-16T11:00:00.256Z", - "start_date":"2019-07-30T12:00:00.256Z", - "web_url":"http://localhost:3000/root/awesome-app/-/milestones/1" - }, + "milestones": [ + { + "id":51, + "iid":1, + "project_id":24, + "title":"v1.0-rc", + "description":"Voluptate fugiat possimus quis quod aliquam expedita.", + "state":"closed", + "created_at":"2019-07-12T19:45:44.256Z", + "updated_at":"2019-07-12T19:45:44.256Z", + "due_date":"2019-08-16T11:00:00.256Z", + "start_date":"2019-07-30T12:00:00.256Z", + "web_url":"https://gitlab.example.com/root/awesome-app/-/milestones/1" + }, + { + "id":52, + "iid":2, + "project_id":24, + "title":"v1.0", + "description":"Voluptate fugiat possimus quis quod aliquam expedita.", + "state":"closed", + "created_at":"2019-07-16T14:00:12.256Z", + "updated_at":"2019-07-16T14:00:12.256Z", + "due_date":"2019-08-16T11:00:00.256Z", + "start_date":"2019-07-30T12:00:00.256Z", + "web_url":"https://gitlab.example.com/root/awesome-app/-/milestones/2" + } + ], "assets":{ "count":4, "sources":[ @@ -273,7 +303,7 @@ POST /projects/:id/releases | `tag_name` | string | yes | The tag where the release will be created from. | | `description` | string | yes | The description of the release. You can use [markdown](../../user/markdown.md). | | `ref` | string | no | If `tag_name` doesn't exist, the release will be created from `ref`. It can be a commit SHA, another tag name, or a branch name. | -| `milestone` | string | no | The title of the milestone the release is associated with. | +| `milestones` | array of string | no | The title of each milestone the release is associated with. | | `assets:links` | array of hash | no | An array of assets links. | | `assets:links:name`| string | required by: `assets:links` | The name of the link. | | `assets:links:url` | string | required by: `assets:links` | The url of the link. | @@ -283,7 +313,7 @@ Example request: ```sh curl --header 'Content-Type: application/json' --header "PRIVATE-TOKEN: gDybLx3yrUK_HLp3qPjS" \ - --data '{ "name": "New release", "tag_name": "v0.3", "description": "Super nice release", "milestone": "v1.0-rc", "assets": { "links": [{ "name": "hoge", "url": "https://google.com" }] } }' \ + --data '{ "name": "New release", "tag_name": "v0.3", "description": "Super nice release", "milestones": ["v1.0", "v1.0-rc"], "assets": { "links": [{ "name": "hoge", "url": "https://google.com" }] } }' \ --request POST https://gitlab.example.com/api/v4/projects/24/releases ``` @@ -321,19 +351,34 @@ Example response: "committer_email":"admin@example.com", "committed_date":"2019-01-03T01:55:38.000Z" }, - "milestone":{ - "id":51, - "iid":1, - "project_id":24, - "title":"v1.0-rc", - "description":"Voluptate fugiat possimus quis quod aliquam expedita.", - "state":"active", - "created_at":"2019-07-12T19:45:44.256Z", - "updated_at":"2019-07-12T19:45:44.256Z", - "due_date":"2019-08-16T11:00:00.256Z", - "start_date":"2019-07-30T12:00:00.256Z", - "web_url":"http://localhost:3000/root/awesome-app/-/milestones/1" - }, + "milestones": [ + { + "id":51, + "iid":1, + "project_id":24, + "title":"v1.0-rc", + "description":"Voluptate fugiat possimus quis quod aliquam expedita.", + "state":"closed", + "created_at":"2019-07-12T19:45:44.256Z", + "updated_at":"2019-07-12T19:45:44.256Z", + "due_date":"2019-08-16T11:00:00.256Z", + "start_date":"2019-07-30T12:00:00.256Z", + "web_url":"https://gitlab.example.com/root/awesome-app/-/milestones/1" + }, + { + "id":52, + "iid":2, + "project_id":24, + "title":"v1.0", + "description":"Voluptate fugiat possimus quis quod aliquam expedita.", + "state":"closed", + "created_at":"2019-07-16T14:00:12.256Z", + "updated_at":"2019-07-16T14:00:12.256Z", + "due_date":"2019-08-16T11:00:00.256Z", + "start_date":"2019-07-30T12:00:00.256Z", + "web_url":"https://gitlab.example.com/root/awesome-app/-/milestones/2" + } + ], "assets":{ "count":5, "sources":[ @@ -374,19 +419,19 @@ Update a Release. PUT /projects/:id/releases/:tag_name ``` -| Attribute | Type | Required | Description | -| ------------- | -------------- | -------- | --------------------------------------------------------------------------------------------------------- | -| `id` | integer/string | yes | The ID or [URL-encoded path of the project](../README.md#namespaced-path-encoding). | -| `tag_name` | string | yes | The tag where the release will be created from. | -| `name` | string | no | The release name. | -| `description` | string | no | The description of the release. You can use [markdown](../../user/markdown.md). | -| `milestone` | string | no | The title of the milestone to associate with the release (`""` to remove the milestone from the release). | -| `released_at` | datetime | no | The date when the release will be/was ready. Expected in ISO 8601 format (`2019-03-15T08:00:00Z`). | +| Attribute | Type | Required | Description | +| ------------- | --------------- | -------- | ----------------------------------------------------------------------------------------------------------- | +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](../README.md#namespaced-path-encoding). | +| `tag_name` | string | yes | The tag where the release will be created from. | +| `name` | string | no | The release name. | +| `description` | string | no | The description of the release. You can use [markdown](../../user/markdown.md). | +| `milestones` | array of string | no | The title of each milestone to associate with the release (`[]` to remove all milestones from the release). | +| `released_at` | datetime | no | The date when the release will be/was ready. Expected in ISO 8601 format (`2019-03-15T08:00:00Z`). | Example request: ```sh -curl --header 'Content-Type: application/json' --request PUT --data '{"name": "new name", "milestone": "v1.0"}' --header "PRIVATE-TOKEN: gDybLx3yrUK_HLp3qPjS" "https://gitlab.example.com/api/v4/projects/24/releases/v0.1" +curl --header 'Content-Type: application/json' --request PUT --data '{"name": "new name", "milestones": ["v1.2"]}' --header "PRIVATE-TOKEN: gDybLx3yrUK_HLp3qPjS" "https://gitlab.example.com/api/v4/projects/24/releases/v0.1" ``` Example response: @@ -423,19 +468,21 @@ Example response: "committer_email":"admin@example.com", "committed_date":"2019-01-03T01:53:28.000Z" }, - "milestone":{ - "id":53, - "iid":2, - "project_id":24, - "title":"v1.0", - "description":"Voluptate fugiat possimus quis quod aliquam expedita.", - "state":"active", - "created_at":"2019-09-01T13:00:00.256Z", - "updated_at":"2019-09-01T13:00:00.256Z", - "due_date":"2019-09-20T13:00:00.256Z", - "start_date":"2019-09-05T12:00:00.256Z", - "web_url":"http://localhost:3000/root/awesome-app/-/milestones/3" - }, + "milestones": [ + { + "id":53, + "iid":3, + "project_id":24, + "title":"v1.0", + "description":"Voluptate fugiat possimus quis quod aliquam expedita.", + "state":"active", + "created_at":"2019-09-01T13:00:00.256Z", + "updated_at":"2019-09-01T13:00:00.256Z", + "due_date":"2019-09-20T13:00:00.256Z", + "start_date":"2019-09-05T12:00:00.256Z", + "web_url":"https://gitlab.example.com/root/awesome-app/-/milestones/3" + } + ], "assets":{ "count":4, "sources":[ diff --git a/doc/development/api_styleguide.md b/doc/development/api_styleguide.md index 61576236c96..3f81440791e 100644 --- a/doc/development/api_styleguide.md +++ b/doc/development/api_styleguide.md @@ -92,5 +92,18 @@ For instance: Model.create(foo: params[:foo]) ``` +## Using API path helpers in GitLab Rails codebase + +Because we support [installing GitLab under a relative URL], one must take this +into account when using API path helpers generated by Grape. Any such API path +helper usage must be in wrapped into the `expose_path` helper call. + +For instance: + +```haml +- endpoint = expose_path(api_v4_projects_issues_related_merge_requests_path(id: @project.id, issue_iid: @issue.iid)) +``` + [Entity]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/api/entities.rb [validation, and coercion of the parameters]: https://github.com/ruby-grape/grape#parameter-validation-and-coercion +[installing GitLab under a relative URL]: https://docs.gitlab.com/ee/install/relative_url.html diff --git a/doc/raketasks/cleanup.md b/doc/raketasks/cleanup.md index f84d29cca9a..e2ec58be367 100644 --- a/doc/raketasks/cleanup.md +++ b/doc/raketasks/cleanup.md @@ -16,6 +16,11 @@ sudo gitlab-rake gitlab:cleanup:dirs bundle exec rake gitlab:cleanup:dirs RAILS_ENV=production ``` +DANGER: **Danger:** +The following task does not currently work as expected. +The use will probably mark more existing repositories as orphaned. +For more information, see the [issue](https://gitlab.com/gitlab-org/gitlab-ee/issues/24633). + Rename repositories from all repository storage paths if they don't exist in GitLab database. The repositories get a `+orphaned+TIMESTAMP` suffix so that they cannot block new repositories from being created. diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 53774d4db1a..4018ce900e1 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1044,7 +1044,12 @@ module API expose :job_events # Expose serialized properties expose :properties do |service, options| - service.properties.slice(*service.api_field_names) + # TODO: Simplify as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084 + if service.data_fields_present? + service.data_fields.as_json.slice(*service.api_field_names) + else + service.properties.slice(*service.api_field_names) + end end end @@ -1280,7 +1285,7 @@ module API expose :author, using: Entities::UserBasic, if: -> (release, _) { release.author.present? } expose :commit, using: Entities::Commit, if: lambda { |_, _| can_download_code? } expose :upcoming_release?, as: :upcoming_release - expose :milestone, using: Entities::Milestone, if: -> (release, _) { release.milestone.present? } + expose :milestones, using: Entities::Milestone, if: -> (release, _) { release.milestones.present? } expose :assets do expose :assets_count, as: :count do |release, _| diff --git a/lib/api/releases.rb b/lib/api/releases.rb index 5a31581c4da..4238529142c 100644 --- a/lib/api/releases.rb +++ b/lib/api/releases.rb @@ -54,7 +54,7 @@ module API requires :url, type: String end end - optional :milestone, type: String, desc: 'The title of the related milestone' + optional :milestones, type: Array, desc: 'The titles of the related milestones', default: [] optional :released_at, type: DateTime, desc: 'The date when the release will be/was ready. Defaults to the current time.' end post ':id/releases' do @@ -80,7 +80,7 @@ module API optional :name, type: String, desc: 'The name of the release' optional :description, type: String, desc: 'Release notes with markdown support' optional :released_at, type: DateTime, desc: 'The date when the release will be/was ready.' - optional :milestone, type: String, desc: 'The title of the related milestone' + optional :milestones, type: Array, desc: 'The titles of the related milestones' end put ':id/releases/:tag_name', requirements: RELEASE_ENDPOINT_REQUIREMENTS do authorize_update_release! diff --git a/lib/gitlab/discussions_diff/file_collection.rb b/lib/gitlab/discussions_diff/file_collection.rb index 4ab7314f509..6692dd76438 100644 --- a/lib/gitlab/discussions_diff/file_collection.rb +++ b/lib/gitlab/discussions_diff/file_collection.rb @@ -4,11 +4,16 @@ module Gitlab module DiscussionsDiff class FileCollection include Gitlab::Utils::StrongMemoize + include Enumerable def initialize(collection) @collection = collection end + def each(&block) + @collection.each(&block) + end + # Returns a Gitlab::Diff::File with the given ID (`unique_identifier` in # Gitlab::Diff::File). def find_by_id(id) @@ -16,20 +21,12 @@ module Gitlab end # Writes cache and preloads highlighted diff lines for - # object IDs, in @collection. - # - # highlightable_ids - Diff file `Array` responding to ID. The ID will be used - # to generate the cache key. + # highlightable object IDs, in @collection. # # - Highlight cache is written just for uncached diff files # - The cache content is not updated (there's no need to do so) - def load_highlight(highlightable_ids) - preload_highlighted_lines(highlightable_ids) - end - - private - - def preload_highlighted_lines(ids) + def load_highlight + ids = highlightable_collection_ids cached_content = read_cache(ids) uncached_ids = ids.select.each_with_index { |_, i| cached_content[i].nil? } @@ -46,6 +43,12 @@ module Gitlab end end + private + + def highlightable_collection_ids + each.with_object([]) { |file, memo| memo << file.id unless file.resolved_at } + end + def read_cache(ids) HighlightCache.read_multiple(ids) end @@ -57,9 +60,7 @@ module Gitlab end def diff_files - strong_memoize(:diff_files) do - @collection.map(&:raw_diff_file) - end + strong_memoize(:diff_files) { map(&:raw_diff_file) } end # Processes the diff lines highlighting for diff files matching the given diff --git a/lib/gitlab/usage_data.rb b/lib/gitlab/usage_data.rb index 5cd54c302fc..c6c2876033d 100644 --- a/lib/gitlab/usage_data.rb +++ b/lib/gitlab/usage_data.rb @@ -3,6 +3,7 @@ module Gitlab class UsageData APPROXIMATE_COUNT_MODELS = [Label, MergeRequest, Note, Todo].freeze + BATCH_SIZE = 100 class << self def data(force_refresh: false) @@ -13,10 +14,10 @@ module Gitlab def uncached_data license_usage_data.merge(system_usage_data) - .merge(features_usage_data) - .merge(components_usage_data) - .merge(cycle_analytics_usage_data) - .merge(usage_counters) + .merge(features_usage_data) + .merge(components_usage_data) + .merge(cycle_analytics_usage_data) + .merge(usage_counters) end def to_json(force_refresh: false) @@ -96,9 +97,8 @@ module Gitlab todos: count(Todo), uploads: count(Upload), web_hooks: count(WebHook) - } - .merge(services_usage) - .merge(approximate_counts) + }.merge(services_usage) + .merge(approximate_counts) }.tap do |data| data[:counts][:user_preferences] = user_preferences_usage end @@ -173,17 +173,34 @@ module Gitlab def jira_usage # Jira Cloud does not support custom domains as per https://jira.atlassian.com/browse/CLOUD-6999 # so we can just check for subdomains of atlassian.net - services = count( - Service.unscoped.where(type: :JiraService, active: true) - .group("CASE WHEN properties LIKE '%.atlassian.net%' THEN 'cloud' ELSE 'server' END"), - fallback: Hash.new(-1) - ) - { - projects_jira_server_active: services['server'] || 0, - projects_jira_cloud_active: services['cloud'] || 0, - projects_jira_active: services['server'] == -1 ? -1 : services.values.sum + results = { + projects_jira_server_active: 0, + projects_jira_cloud_active: 0, + projects_jira_active: -1 } + + Service.unscoped + .where(type: :JiraService, active: true) + .includes(:jira_tracker_data) + .find_in_batches(batch_size: BATCH_SIZE) do |services| + + counts = services.group_by do |service| + # TODO: Simplify as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084 + service_url = service.data_fields&.url || (service.properties && service.properties['url']) + service_url&.include?('.atlassian.net') ? :cloud : :server + end + + results[:projects_jira_server_active] += counts[:server].count if counts[:server] + results[:projects_jira_cloud_active] += counts[:cloud].count if counts[:cloud] + if results[:projects_jira_active] == -1 + results[:projects_jira_active] = count(services) + else + results[:projects_jira_active] += count(services) + end + end + + results end def user_preferences_usage diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index e1f67054d0a..eda8c282341 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -1289,7 +1289,7 @@ describe Projects::MergeRequestsController do expect_next_instance_of(Gitlab::DiscussionsDiff::FileCollection) do |collection| note_diff_file = commit_diff_note.note_diff_file - expect(collection).to receive(:load_highlight).with([note_diff_file.id]).and_call_original + expect(collection).to receive(:load_highlight).and_call_original expect(collection).to receive(:find_by_id).with(note_diff_file.id).and_call_original end @@ -1306,7 +1306,7 @@ describe Projects::MergeRequestsController do expect_next_instance_of(Gitlab::DiscussionsDiff::FileCollection) do |collection| note_diff_file = diff_note.note_diff_file - expect(collection).to receive(:load_highlight).with([note_diff_file.id]).and_call_original + expect(collection).to receive(:load_highlight).and_call_original expect(collection).to receive(:find_by_id).with(note_diff_file.id).and_call_original end @@ -1319,7 +1319,7 @@ describe Projects::MergeRequestsController do expect_next_instance_of(Gitlab::DiscussionsDiff::FileCollection) do |collection| note_diff_file = diff_note.note_diff_file - expect(collection).to receive(:load_highlight).with([]).and_call_original + expect(collection).to receive(:load_highlight).and_call_original expect(collection).to receive(:find_by_id).with(note_diff_file.id).and_call_original end diff --git a/spec/factories/services.rb b/spec/factories/services.rb index b2d6ada91fa..c4cb3f07fe7 100644 --- a/spec/factories/services.rb +++ b/spec/factories/services.rb @@ -9,11 +9,7 @@ FactoryBot.define do factory :custom_issue_tracker_service, class: CustomIssueTrackerService do project active true - properties( - project_url: 'https://project.url.com', - issues_url: 'https://issues.url.com', - new_issue_url: 'https://newissue.url.com' - ) + issue_tracker end factory :emails_on_push_service do @@ -47,12 +43,24 @@ FactoryBot.define do factory :jira_service do project active true - properties( - url: 'https://jira.example.com', - username: 'jira_user', - password: 'my-secret-password', - project_key: 'jira-key' - ) + + transient do + create_data true + url 'https://jira.example.com' + api_url nil + username 'jira_username' + password 'jira_password' + jira_issue_transition_id '56-1' + end + + after(:build) do |service, evaluator| + if evaluator.create_data + create(:jira_tracker_data, service: service, + url: evaluator.url, api_url: evaluator.api_url, jira_issue_transition_id: evaluator.jira_issue_transition_id, + username: evaluator.username, password: evaluator.password + ) + end + end end factory :bugzilla_service do @@ -80,20 +88,26 @@ FactoryBot.define do end trait :issue_tracker do - properties( - project_url: 'http://issue-tracker.example.com', - issues_url: 'http://issue-tracker.example.com/issues/:id', - new_issue_url: 'http://issue-tracker.example.com' - ) + transient do + create_data true + project_url 'http://issuetracker.example.com' + issues_url 'http://issues.example.com/issues/:id' + new_issue_url 'http://new-issue.example.com' + end + + after(:build) do |service, evaluator| + if evaluator.create_data + create(:issue_tracker_data, service: service, + project_url: evaluator.project_url, issues_url: evaluator.issues_url, new_issue_url: evaluator.new_issue_url + ) + end + end end trait :jira_cloud_service do - properties( - url: 'https://mysite.atlassian.net', - username: 'jira_user', - password: 'my-secret-password', - project_key: 'jira-key' - ) + url 'https://mysite.atlassian.net' + username 'jira_user' + password 'my-secret-password' end factory :hipchat_service do @@ -102,15 +116,21 @@ FactoryBot.define do token 'test_token' end + # this is for testing storing values inside properties, which is deprecated and will be removed in + # https://gitlab.com/gitlab-org/gitlab-ce/issues/63084 trait :without_properties_callback do + jira_tracker_data nil + issue_tracker_data nil + create_data false + after(:build) do |service| - allow(service).to receive(:handle_properties) + IssueTrackerService.skip_callback(:validation, :before, :handle_properties) end - after(:create) do |service| - # we have to remove the stub because the behaviour of - # handle_properties method is tested after the creation - allow(service).to receive(:handle_properties).and_call_original + to_create { |instance| instance.save(validate: false)} + + after(:create) do + IssueTrackerService.set_callback(:validation, :before, :handle_properties) end end end diff --git a/spec/fixtures/api/schemas/public_api/v4/release.json b/spec/fixtures/api/schemas/public_api/v4/release.json index 078b1c0b982..662e61a9c06 100644 --- a/spec/fixtures/api/schemas/public_api/v4/release.json +++ b/spec/fixtures/api/schemas/public_api/v4/release.json @@ -15,7 +15,10 @@ "author": { "oneOf": [{ "type": "null" }, { "$ref": "user/basic.json" }] }, - "milestone": { "type": "string" }, + "milestones": { + "type": "array", + "items": { "$ref": "milestone.json" } + }, "assets": { "required": ["count", "links", "sources"], "properties": { diff --git a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb index 7eb63fea413..47ea273ef3a 100644 --- a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb @@ -34,7 +34,7 @@ describe Banzai::Pipeline::GfmPipeline do result = described_class.call(markdown, project: project)[:output] link = result.css('a').first - expect(link['href']).to eq 'http://issue-tracker.example.com/issues/12' + expect(link['href']).to eq 'http://issues.example.com/issues/12' end it 'parses cross-project references to regular issues' do @@ -63,7 +63,7 @@ describe Banzai::Pipeline::GfmPipeline do result = described_class.call(markdown, project: project)[:output] link = result.css('a').first - expect(link['href']).to eq 'http://issue-tracker.example.com/issues/12' + expect(link['href']).to eq 'http://issues.example.com/issues/12' end it 'allows to use long external reference syntax for Redmine' do @@ -72,7 +72,7 @@ describe Banzai::Pipeline::GfmPipeline do result = described_class.call(markdown, project: project)[:output] link = result.css('a').first - expect(link['href']).to eq 'http://issue-tracker.example.com/issues/12' + expect(link['href']).to eq 'http://issues.example.com/issues/12' end it 'parses cross-project references to regular issues' do diff --git a/spec/lib/gitlab/discussions_diff/file_collection_spec.rb b/spec/lib/gitlab/discussions_diff/file_collection_spec.rb index 0489206458b..6ef1e41450f 100644 --- a/spec/lib/gitlab/discussions_diff/file_collection_spec.rb +++ b/spec/lib/gitlab/discussions_diff/file_collection_spec.rb @@ -22,11 +22,13 @@ describe Gitlab::DiscussionsDiff::FileCollection do note_diff_file_b.id => file_b_caching_content }) .and_call_original - subject.load_highlight([note_diff_file_a.id, note_diff_file_b.id]) + subject.load_highlight end it 'does not write cache for already cached file' do - subject.load_highlight([note_diff_file_a.id]) + file_a_caching_content = diff_note_a.diff_file.highlighted_diff_lines.map(&:to_hash) + Gitlab::DiscussionsDiff::HighlightCache + .write_multiple({ note_diff_file_a.id => file_a_caching_content }) file_b_caching_content = diff_note_b.diff_file.highlighted_diff_lines.map(&:to_hash) @@ -35,27 +37,42 @@ describe Gitlab::DiscussionsDiff::FileCollection do .with({ note_diff_file_b.id => file_b_caching_content }) .and_call_original - subject.load_highlight([note_diff_file_a.id, note_diff_file_b.id]) + subject.load_highlight end - it 'does not err when given ID does not exist in @collection' do - expect { subject.load_highlight([999]) }.not_to raise_error + it 'does not write cache for resolved notes' do + diff_note_a.update_column(:resolved_at, Time.now) + + file_b_caching_content = diff_note_b.diff_file.highlighted_diff_lines.map(&:to_hash) + + expect(Gitlab::DiscussionsDiff::HighlightCache) + .to receive(:write_multiple) + .with({ note_diff_file_b.id => file_b_caching_content }) + .and_call_original + + subject.load_highlight end it 'loaded diff files have highlighted lines loaded' do - subject.load_highlight([note_diff_file_a.id]) + subject.load_highlight - diff_file = subject.find_by_id(note_diff_file_a.id) + diff_file_a = subject.find_by_id(note_diff_file_a.id) + diff_file_b = subject.find_by_id(note_diff_file_b.id) - expect(diff_file.highlight_loaded?).to be(true) + expect(diff_file_a).to be_highlight_loaded + expect(diff_file_b).to be_highlight_loaded end it 'not loaded diff files does not have highlighted lines loaded' do - subject.load_highlight([note_diff_file_a.id]) + diff_note_a.update_column(:resolved_at, Time.now) + + subject.load_highlight - diff_file = subject.find_by_id(note_diff_file_b.id) + diff_file_a = subject.find_by_id(note_diff_file_a.id) + diff_file_b = subject.find_by_id(note_diff_file_b.id) - expect(diff_file.highlight_loaded?).to be(false) + expect(diff_file_a).not_to be_highlight_loaded + expect(diff_file_b).to be_highlight_loaded end end end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 6d573a4f39a..d3be1e86539 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -65,8 +65,8 @@ milestone: - participants - events - boards -- milestone_release -- release +- milestone_releases +- releases snippets: - author - project @@ -77,8 +77,8 @@ releases: - author - project - links -- milestone_release -- milestone +- milestone_releases +- milestones links: - release project_members: diff --git a/spec/lib/gitlab/usage_data_spec.rb b/spec/lib/gitlab/usage_data_spec.rb index 8eb64b97d6a..62787c5abaf 100644 --- a/spec/lib/gitlab/usage_data_spec.rb +++ b/spec/lib/gitlab/usage_data_spec.rb @@ -3,14 +3,16 @@ require 'spec_helper' describe Gitlab::UsageData do - let(:projects) { create_list(:project, 3) } + let(:projects) { create_list(:project, 4) } let!(:board) { create(:board, project: projects[0]) } describe '#data' do before do create(:jira_service, project: projects[0]) - create(:jira_service, project: projects[1]) + create(:jira_service, :without_properties_callback, project: projects[1]) create(:jira_service, :jira_cloud_service, project: projects[2]) + create(:jira_service, :without_properties_callback, project: projects[3], + properties: { url: 'https://mysite.atlassian.net' }) create(:prometheus_service, project: projects[1]) create(:service, project: projects[0], type: 'SlackSlashCommandsService', active: true) create(:service, project: projects[1], type: 'SlackService', active: true) @@ -156,7 +158,7 @@ describe Gitlab::UsageData do count_data = subject[:counts] expect(count_data[:boards]).to eq(1) - expect(count_data[:projects]).to eq(3) + expect(count_data[:projects]).to eq(4) expect(count_data.keys).to include(*expected_keys) expect(expected_keys - count_data.keys).to be_empty end @@ -164,14 +166,14 @@ describe Gitlab::UsageData do it 'gathers projects data correctly' do count_data = subject[:counts] - expect(count_data[:projects]).to eq(3) + expect(count_data[:projects]).to eq(4) expect(count_data[:projects_prometheus_active]).to eq(1) - expect(count_data[:projects_jira_active]).to eq(3) + expect(count_data[:projects_jira_active]).to eq(4) expect(count_data[:projects_jira_server_active]).to eq(2) - expect(count_data[:projects_jira_cloud_active]).to eq(1) + expect(count_data[:projects_jira_cloud_active]).to eq(2) expect(count_data[:projects_slack_notifications_active]).to eq(2) expect(count_data[:projects_slack_slash_active]).to eq(1) - expect(count_data[:projects_with_repositories_enabled]).to eq(2) + expect(count_data[:projects_with_repositories_enabled]).to eq(3) expect(count_data[:projects_with_error_tracking_enabled]).to eq(1) expect(count_data[:clusters_enabled]).to eq(7) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index fde1b096c76..65cc1a4bd6b 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -650,9 +650,35 @@ describe MergeRequest do end end - describe '#preload_discussions_diff_highlight' do + describe '#discussions_diffs' do let(:merge_request) { create(:merge_request) } + shared_examples 'discussions diffs collection' do + it 'initializes Gitlab::DiscussionsDiff::FileCollection with correct data' do + note_diff_file = diff_note.note_diff_file + + expect(Gitlab::DiscussionsDiff::FileCollection) + .to receive(:new) + .with([note_diff_file]) + .and_call_original + + result = merge_request.discussions_diffs + + expect(result).to be_a(Gitlab::DiscussionsDiff::FileCollection) + end + + it 'eager loads relations' do + result = merge_request.discussions_diffs + + recorder = ActiveRecord::QueryRecorder.new do + result.first.diff_note + result.first.diff_note.project + end + + expect(recorder.count).to be_zero + end + end + context 'with commit diff note' do let(:other_merge_request) { create(:merge_request) } @@ -664,40 +690,15 @@ describe MergeRequest do create(:diff_note_on_commit, project: other_merge_request.project) end - it 'preloads diff highlighting' do - expect_next_instance_of(Gitlab::DiscussionsDiff::FileCollection) do |collection| - note_diff_file = diff_note.note_diff_file - - expect(collection) - .to receive(:load_highlight) - .with([note_diff_file.id]).and_call_original - end - - merge_request.preload_discussions_diff_highlight - end + it_behaves_like 'discussions diffs collection' end context 'with merge request diff note' do - let!(:unresolved_diff_note) do + let!(:diff_note) do create(:diff_note_on_merge_request, project: merge_request.project, noteable: merge_request) end - let!(:resolved_diff_note) do - create(:diff_note_on_merge_request, :resolved, project: merge_request.project, noteable: merge_request) - end - - it 'preloads diff highlighting' do - expect_next_instance_of(Gitlab::DiscussionsDiff::FileCollection) do |collection| - note_diff_file = unresolved_diff_note.note_diff_file - - expect(collection) - .to receive(:load_highlight) - .with([note_diff_file.id]) - .and_call_original - end - - merge_request.preload_discussions_diff_highlight - end + it_behaves_like 'discussions diffs collection' end end diff --git a/spec/models/milestone_release_spec.rb b/spec/models/milestone_release_spec.rb index d6f73275977..28cec7bbc17 100644 --- a/spec/models/milestone_release_spec.rb +++ b/spec/models/milestone_release_spec.rb @@ -14,23 +14,29 @@ describe MilestoneRelease do it { is_expected.to belong_to(:release) } end + context 'when trying to create the same record in milestone_releases twice' do + it 'is not committing on the second time' do + create(:milestone_release, milestone: milestone, release: release) + + expect do + subject.save! + end.to raise_error(ActiveRecord::RecordNotUnique) + end + end + describe 'validations' do - it { is_expected.to validate_uniqueness_of(:milestone_id).scoped_to(:release_id) } + subject(:milestone_release) { build(:milestone_release, milestone: milestone, release: release) } context 'when milestone and release do not have the same project' do it 'is not valid' do - other_project = create(:project) - release = build(:release, project: other_project) - milestone_release = described_class.new(milestone: milestone, release: release) + milestone_release.release = build(:release, project: create(:project)) + expect(milestone_release).not_to be_valid end end context 'when milestone and release have the same project' do - it 'is valid' do - milestone_release = described_class.new(milestone: milestone, release: release) - expect(milestone_release).to be_valid - end + it { is_expected.to be_valid } end end end diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 64030f5b92a..0c4952eebd7 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -55,20 +55,20 @@ describe Milestone do end end - describe 'milestone_release' do + describe 'milestone_releases' do let(:milestone) { build(:milestone, project: project) } context 'when it is tied to a release for another project' do it 'creates a validation error' do other_project = create(:project) - milestone.release = build(:release, project: other_project) + milestone.releases << build(:release, project: other_project) expect(milestone).not_to be_valid end end context 'when it is tied to a release for the same project' do it 'is valid' do - milestone.release = build(:release, project: project) + milestone.releases << build(:release, project: project) expect(milestone).to be_valid end end @@ -78,7 +78,8 @@ describe Milestone do describe "Associations" do it { is_expected.to belong_to(:project) } it { is_expected.to have_many(:issues) } - it { is_expected.to have_one(:release) } + it { is_expected.to have_many(:releases) } + it { is_expected.to have_many(:milestone_releases) } end let(:project) { create(:project, :public) } diff --git a/spec/models/project_services/bugzilla_service_spec.rb b/spec/models/project_services/bugzilla_service_spec.rb index 74c85a13c88..e25d87f61d6 100644 --- a/spec/models/project_services/bugzilla_service_spec.rb +++ b/spec/models/project_services/bugzilla_service_spec.rb @@ -48,7 +48,7 @@ describe BugzillaService do create(:bugzilla_service, :without_properties_callback, properties: properties) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when data are stored in separated fields' do @@ -56,7 +56,7 @@ describe BugzillaService do create(:bugzilla_service, title: title, description: description, properties: access_params) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when data are stored in both properties and separated fields' do @@ -65,7 +65,7 @@ describe BugzillaService do create(:bugzilla_service, :without_properties_callback, title: title, description: description, properties: properties) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when no title & description are set' do diff --git a/spec/models/project_services/custom_issue_tracker_service_spec.rb b/spec/models/project_services/custom_issue_tracker_service_spec.rb index 5259357a254..8359bc6807a 100644 --- a/spec/models/project_services/custom_issue_tracker_service_spec.rb +++ b/spec/models/project_services/custom_issue_tracker_service_spec.rb @@ -62,7 +62,7 @@ describe CustomIssueTrackerService do create(:custom_issue_tracker_service, :without_properties_callback, properties: properties) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when data are stored in separated fields' do @@ -70,7 +70,7 @@ describe CustomIssueTrackerService do create(:custom_issue_tracker_service, title: title, description: description, properties: access_params) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when data are stored in both properties and separated fields' do @@ -79,7 +79,7 @@ describe CustomIssueTrackerService do create(:custom_issue_tracker_service, :without_properties_callback, title: title, description: description, properties: properties) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when no title & description are set' do diff --git a/spec/models/project_services/data_fields_spec.rb b/spec/models/project_services/data_fields_spec.rb new file mode 100644 index 00000000000..146db0ae227 --- /dev/null +++ b/spec/models/project_services/data_fields_spec.rb @@ -0,0 +1,138 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DataFields do + let(:url) { 'http://url.com' } + let(:username) { 'username_one' } + let(:properties) do + { url: url, username: username } + end + + shared_examples 'data fields' do + describe '#arg' do + it 'returns an argument correctly' do + expect(service.url).to eq(url) + end + end + + describe '{arg}_changed?' do + it 'returns false when the property has not been assigned a new value' do + service.username = 'new_username' + service.validate + expect(service.url_changed?).to be_falsy + end + + it 'returns true when the property has been assigned a different value' do + service.url = "http://example.com" + service.validate + expect(service.url_changed?).to be_truthy + end + + it 'returns true when the property has been assigned a different value twice' do + service.url = "http://example.com" + service.url = "http://example.com" + service.validate + expect(service.url_changed?).to be_truthy + end + + it 'returns false when the property has been re-assigned the same value' do + service.url = 'http://url.com' + service.validate + expect(service.url_changed?).to be_falsy + end + end + + describe '{arg}_touched?' do + it 'returns false when the property has not been assigned a new value' do + service.username = 'new_username' + service.validate + expect(service.url_changed?).to be_falsy + end + + it 'returns true when the property has been assigned a different value' do + service.url = "http://example.com" + service.validate + expect(service.url_changed?).to be_truthy + end + + it 'returns true when the property has been assigned a different value twice' do + service.url = "http://example.com" + service.url = "http://example.com" + service.validate + expect(service.url_changed?).to be_truthy + end + + it 'returns true when the property has been re-assigned the same value' do + service.url = 'http://url.com' + expect(service.url_touched?).to be_truthy + end + + it 'returns false when the property has been re-assigned the same value' do + service.url = 'http://url.com' + service.validate + expect(service.url_changed?).to be_falsy + end + end + end + + context 'when data are stored in data_fields' do + let(:service) do + create(:jira_service, url: url, username: username) + end + + it_behaves_like 'data fields' + + describe '{arg}_was?' do + it 'returns nil' do + service.url = 'http://example.com' + service.validate + expect(service.url_was).to be_nil + end + end + end + + context 'when data are stored in properties' do + let(:service) { create(:jira_service, :without_properties_callback, properties: properties) } + + it_behaves_like 'data fields' + + describe '{arg}_was?' do + it 'returns nil when the property has not been assigned a new value' do + service.username = 'new_username' + service.validate + expect(service.url_was).to be_nil + end + + it 'returns initial value when the property has been assigned a different value' do + service.url = 'http://example.com' + service.validate + expect(service.url_was).to eq('http://url.com') + end + + it 'returns initial value when the property has been re-assigned the same value' do + service.url = 'http://url.com' + service.validate + expect(service.url_was).to eq('http://url.com') + end + end + end + + context 'when data are stored in both properties and data_fields' do + let(:service) do + create(:jira_service, :without_properties_callback, active: false, properties: properties).tap do |service| + create(:jira_tracker_data, properties.merge(service: service)) + end + end + + it_behaves_like 'data fields' + + describe '{arg}_was?' do + it 'returns nil' do + service.url = 'http://example.com' + service.validate + expect(service.url_was).to be_nil + end + end + end +end diff --git a/spec/models/project_services/gitlab_issue_tracker_service_spec.rb b/spec/models/project_services/gitlab_issue_tracker_service_spec.rb index 0c4fc290a13..4f3736ca65b 100644 --- a/spec/models/project_services/gitlab_issue_tracker_service_spec.rb +++ b/spec/models/project_services/gitlab_issue_tracker_service_spec.rb @@ -65,7 +65,7 @@ describe GitlabIssueTrackerService do create(:gitlab_issue_tracker_service, :without_properties_callback, properties: properties) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when data are stored in separated fields' do @@ -73,7 +73,7 @@ describe GitlabIssueTrackerService do create(:gitlab_issue_tracker_service, title: title, description: description, properties: access_params) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when data are stored in both properties and separated fields' do @@ -82,7 +82,7 @@ describe GitlabIssueTrackerService do create(:gitlab_issue_tracker_service, :without_properties_callback, title: title, description: description, properties: properties) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when no title & description are set' do diff --git a/spec/models/project_services/issue_tracker_data_spec.rb b/spec/models/project_services/issue_tracker_data_spec.rb index aaab654f874..db617cf0abb 100644 --- a/spec/models/project_services/issue_tracker_data_spec.rb +++ b/spec/models/project_services/issue_tracker_data_spec.rb @@ -8,28 +8,4 @@ describe IssueTrackerData do describe 'Associations' do it { is_expected.to belong_to :service } end - - describe 'Validations' do - subject { described_class.new(service: service) } - - context 'url validations' do - context 'when service is inactive' do - it { is_expected.not_to validate_presence_of(:project_url) } - it { is_expected.not_to validate_presence_of(:issues_url) } - end - - context 'when service is active' do - before do - service.update(active: true) - end - - it_behaves_like 'issue tracker service URL attribute', :project_url - it_behaves_like 'issue tracker service URL attribute', :issues_url - it_behaves_like 'issue tracker service URL attribute', :new_issue_url - - it { is_expected.to validate_presence_of(:project_url) } - it { is_expected.to validate_presence_of(:issues_url) } - end - end - end end diff --git a/spec/models/project_services/issue_tracker_service_spec.rb b/spec/models/project_services/issue_tracker_service_spec.rb index 2fc4d69c2db..f1cdee5c4a3 100644 --- a/spec/models/project_services/issue_tracker_service_spec.rb +++ b/spec/models/project_services/issue_tracker_service_spec.rb @@ -7,7 +7,7 @@ describe IssueTrackerService do let(:project) { create :project } describe 'only one issue tracker per project' do - let(:service) { RedmineService.new(project: project, active: true) } + let(:service) { RedmineService.new(project: project, active: true, issue_tracker_data: build(:issue_tracker_data)) } before do create(:custom_issue_tracker_service, project: project) diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb index 02060699e9a..a976745023b 100644 --- a/spec/models/project_services/jira_service_spec.rb +++ b/spec/models/project_services/jira_service_spec.rb @@ -6,10 +6,18 @@ describe JiraService do include Gitlab::Routing include AssetsHelpers + let(:title) { 'custom title' } + let(:description) { 'custom description' } + let(:url) { 'http://jira.example.com' } + let(:api_url) { 'http://api-jira.example.com' } + let(:username) { 'jira-username' } + let(:password) { 'jira-password' } + let(:transition_id) { 'test27' } + describe '#options' do let(:service) do - described_class.new( - project: build_stubbed(:project), + described_class.create( + project: create(:project), active: true, username: 'username', password: 'test', @@ -32,78 +40,6 @@ describe JiraService do describe 'Associations' do it { is_expected.to belong_to :project } it { is_expected.to have_one :service_hook } - it { is_expected.to allow_value(nil).for(:jira_issue_transition_id) } - it { is_expected.to allow_value('1,2,3').for(:jira_issue_transition_id) } - it { is_expected.to allow_value('1;2;3').for(:jira_issue_transition_id) } - it { is_expected.not_to allow_value('a,b,cd').for(:jira_issue_transition_id) } - end - - describe 'Validations' do - context 'when service is active' do - before do - subject.active = true - end - - it { is_expected.to validate_presence_of(:url) } - it_behaves_like 'issue tracker service URL attribute', :url - end - - context 'when service is inactive' do - before do - subject.active = false - end - - it { is_expected.not_to validate_presence_of(:url) } - it { is_expected.not_to validate_presence_of(:username) } - it { is_expected.not_to validate_presence_of(:password) } - end - - context 'validating urls' do - let(:service) do - described_class.new( - project: create(:project), - active: true, - username: 'username', - password: 'test', - jira_issue_transition_id: 24, - url: 'http://jira.test.com' - ) - end - - it 'is valid when all fields have required values' do - expect(service).to be_valid - end - - it 'is not valid when url is not a valid url' do - service.url = 'not valid' - - expect(service).not_to be_valid - end - - it 'is not valid when api url is not a valid url' do - service.api_url = 'not valid' - - expect(service).not_to be_valid - end - - it 'is not valid when username is missing' do - service.username = nil - - expect(service).not_to be_valid - end - - it 'is not valid when password is missing' do - service.password = nil - - expect(service).not_to be_valid - end - - it 'is valid when api url is a valid url' do - service.api_url = 'http://jira.test.com/api' - - expect(service).to be_valid - end - end end describe '.reference_pattern' do @@ -118,55 +54,260 @@ describe JiraService do describe '#create' do let(:params) do { - project: create(:project), title: 'custom title', description: 'custom description' + project: create(:project), + title: 'custom title', description: 'custom description', + url: url, api_url: api_url, + username: username, password: password, + jira_issue_transition_id: transition_id } end subject { described_class.create(params) } - it 'does not store title & description into properties' do - expect(subject.properties.keys).not_to include('title', 'description') + it 'does not store data into properties' do + expect(subject.properties).to be_nil + end + + it 'sets title correctly' do + service = subject + + expect(service.title).to eq('custom title') end - it 'sets title & description correctly' do + it 'sets service data correctly' do service = subject expect(service.title).to eq('custom title') expect(service.description).to eq('custom description') end + + it 'stores data in data_fields correcty' do + service = subject + + expect(service.jira_tracker_data.url).to eq(url) + expect(service.jira_tracker_data.api_url).to eq(api_url) + expect(service.jira_tracker_data.username).to eq(username) + expect(service.jira_tracker_data.password).to eq(password) + expect(service.jira_tracker_data.jira_issue_transition_id).to eq(transition_id) + end end + # we need to make sure we are able to read both from properties and jira_tracker_data table + # TODO: change this as part of #63084 context 'overriding properties' do - let(:url) { 'http://issue_tracker.example.com' } let(:access_params) do - { url: url, username: 'username', password: 'password' } + { url: url, api_url: api_url, username: username, password: password, + jira_issue_transition_id: transition_id } + end + let(:data_params) do + { + url: url, api_url: api_url, + username: username, password: password, + jira_issue_transition_id: transition_id + } + end + + shared_examples 'handles jira fields' do + let(:data_params) do + { + url: url, api_url: api_url, + username: username, password: password, + jira_issue_transition_id: transition_id + } + end + + context 'reading data' do + it 'reads data correctly' do + expect(service.url).to eq(url) + expect(service.api_url).to eq(api_url) + expect(service.username).to eq(username) + expect(service.password).to eq(password) + expect(service.jira_issue_transition_id).to eq(transition_id) + end + end + + context '#update' do + context 'basic update' do + let(:new_username) { 'new_username' } + let(:new_url) { 'http://jira-new.example.com' } + + before do + service.update(username: new_username, url: new_url) + end + + it 'leaves properties field emtpy' do + # expect(service.reload.properties).to be_empty + end + + it 'stores updated data in jira_tracker_data table' do + data = service.jira_tracker_data.reload + + expect(data.url).to eq(new_url) + expect(data.api_url).to eq(api_url) + expect(data.username).to eq(new_username) + expect(data.password).to eq(password) + expect(data.jira_issue_transition_id).to eq(transition_id) + end + end + + context 'stored password invalidation' do + context 'when a password was previously set' do + context 'when only web url present' do + let(:data_params) do + { + url: url, api_url: nil, + username: username, password: password, + jira_issue_transition_id: transition_id + } + end + + it 'resets password if url changed' do + service + service.url = 'http://jira_edited.example.com' + service.save + + expect(service.reload.url).to eq('http://jira_edited.example.com') + expect(service.password).to be_nil + end + + it 'does not reset password if url "changed" to the same url as before' do + service.title = 'aaaaaa' + service.url = 'http://jira.example.com' + service.save + + expect(service.reload.url).to eq('http://jira.example.com') + expect(service.password).not_to be_nil + end + + it 'resets password if url not changed but api url added' do + service.api_url = 'http://jira_edited.example.com/rest/api/2' + service.save + + expect(service.reload.api_url).to eq('http://jira_edited.example.com/rest/api/2') + expect(service.password).to be_nil + end + + it 'does not reset password if new url is set together with password, even if it\'s the same password' do + service.url = 'http://jira_edited.example.com' + service.password = password + service.save + + expect(service.password).to eq(password) + expect(service.url).to eq('http://jira_edited.example.com') + end + + it 'resets password if url changed, even if setter called multiple times' do + service.url = 'http://jira1.example.com/rest/api/2' + service.url = 'http://jira1.example.com/rest/api/2' + service.save + + expect(service.password).to be_nil + end + + it 'does not reset password if username changed' do + service.username = 'some_name' + service.save + + expect(service.reload.password).to eq(password) + end + + it 'does not reset password if password changed' do + service.url = 'http://jira_edited.example.com' + service.password = 'new_password' + service.save + + expect(service.reload.password).to eq('new_password') + end + + it 'does not reset password if the password is touched and same as before' do + service.url = 'http://jira_edited.example.com' + service.password = password + service.save + + expect(service.reload.password).to eq(password) + end + end + + context 'when both web and api url present' do + let(:data_params) do + { + url: url, api_url: 'http://jira.example.com/rest/api/2', + username: username, password: password, + jira_issue_transition_id: transition_id + } + end + + it 'resets password if api url changed' do + service.api_url = 'http://jira_edited.example.com/rest/api/2' + service.save + + expect(service.password).to be_nil + end + + it 'does not reset password if url changed' do + service.url = 'http://jira_edited.example.com' + service.save + + expect(service.password).to eq(password) + end + + it 'resets password if api url set to empty' do + service.update(api_url: '') + + expect(service.reload.password).to be_nil + end + end + end + + context 'when no password was previously set' do + let(:data_params) do + { + url: url, username: username + } + end + + it 'saves password if new url is set together with password' do + service.url = 'http://jira_edited.example.com/rest/api/2' + service.password = 'password' + service.save + expect(service.reload.password).to eq('password') + expect(service.reload.url).to eq('http://jira_edited.example.com/rest/api/2') + end + end + end + end end # this will be removed as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084 context 'when data are stored in properties' do - let(:properties) { access_params.merge(title: title, description: description) } - let(:service) do + let(:properties) { data_params.merge(title: title, description: description) } + let!(:service) do create(:jira_service, :without_properties_callback, properties: properties) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' + it_behaves_like 'handles jira fields' end context 'when data are stored in separated fields' do let(:service) do - create(:jira_service, title: title, description: description, properties: access_params) + create(:jira_service, data_params.merge(properties: {}, title: title, description: description)) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' + it_behaves_like 'handles jira fields' end context 'when data are stored in both properties and separated fields' do - let(:properties) { access_params.merge(title: 'wrong title', description: 'wrong description') } + let(:properties) { data_params.merge(title: title, description: description) } let(:service) do - create(:jira_service, :without_properties_callback, title: title, description: description, properties: properties) + create(:jira_service, :without_properties_callback, active: false, properties: properties).tap do |service| + create(:jira_tracker_data, data_params.merge(service: service)) + end end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' + it_behaves_like 'handles jira fields' end context 'when no title & description are set' do @@ -410,111 +551,6 @@ describe JiraService do end end - describe 'Stored password invalidation' do - let(:project) { create(:project) } - - context 'when a password was previously set' do - before do - @jira_service = described_class.create!( - project: project, - properties: { - url: 'http://jira.example.com/web', - username: 'mic', - password: 'password' - } - ) - end - - context 'when only web url present' do - it 'reset password if url changed' do - @jira_service.url = 'http://jira_edited.example.com/rest/api/2' - @jira_service.save - - expect(@jira_service.password).to be_nil - end - - it 'reset password if url not changed but api url added' do - @jira_service.api_url = 'http://jira_edited.example.com/rest/api/2' - @jira_service.save - - expect(@jira_service.password).to be_nil - end - end - - context 'when both web and api url present' do - before do - @jira_service.api_url = 'http://jira.example.com/rest/api/2' - @jira_service.password = 'password' - - @jira_service.save - end - it 'reset password if api url changed' do - @jira_service.api_url = 'http://jira_edited.example.com/rest/api/2' - @jira_service.save - - expect(@jira_service.password).to be_nil - end - - it 'does not reset password if url changed' do - @jira_service.url = 'http://jira_edited.example.com/rweb' - @jira_service.save - - expect(@jira_service.password).to eq('password') - end - - it 'reset password if api url set to empty' do - @jira_service.api_url = '' - @jira_service.save - - expect(@jira_service.password).to be_nil - end - end - - it 'does not reset password if username changed' do - @jira_service.username = 'some_name' - @jira_service.save - - expect(@jira_service.password).to eq('password') - end - - it 'does not reset password if new url is set together with password, even if it\'s the same password' do - @jira_service.url = 'http://jira_edited.example.com/rest/api/2' - @jira_service.password = 'password' - @jira_service.save - - expect(@jira_service.password).to eq('password') - expect(@jira_service.url).to eq('http://jira_edited.example.com/rest/api/2') - end - - it 'resets password if url changed, even if setter called multiple times' do - @jira_service.url = 'http://jira1.example.com/rest/api/2' - @jira_service.url = 'http://jira1.example.com/rest/api/2' - @jira_service.save - expect(@jira_service.password).to be_nil - end - end - - context 'when no password was previously set' do - before do - @jira_service = described_class.create( - project: project, - properties: { - url: 'http://jira.example.com/rest/api/2', - username: 'mic' - } - ) - end - - it 'saves password if new url is set together with password' do - @jira_service.url = 'http://jira_edited.example.com/rest/api/2' - @jira_service.password = 'password' - @jira_service.save - expect(@jira_service.password).to eq('password') - expect(@jira_service.url).to eq('http://jira_edited.example.com/rest/api/2') - end - end - end - describe 'description and title' do let(:title) { 'Jira One' } let(:description) { 'Jira One issue tracker' } @@ -539,7 +575,7 @@ describe JiraService do context 'when it is set in properties' do it 'values from properties are returned' do - service = create(:jira_service, properties: properties) + service = create(:jira_service, :without_properties_callback, properties: properties) expect(service.title).to eq(title) expect(service.description).to eq(description) @@ -602,8 +638,8 @@ describe JiraService do project = create(:project) service = project.create_jira_service(active: true) - expect(service.properties['url']).to eq('http://jira.sample/projects/project_a') - expect(service.properties['api_url']).to eq('http://jira.sample/api') + expect(service.url).to eq('http://jira.sample/projects/project_a') + expect(service.api_url).to eq('http://jira.sample/api') end end diff --git a/spec/models/project_services/jira_tracker_data_spec.rb b/spec/models/project_services/jira_tracker_data_spec.rb index 1b6ece8531b..6cd3eb33d9b 100644 --- a/spec/models/project_services/jira_tracker_data_spec.rb +++ b/spec/models/project_services/jira_tracker_data_spec.rb @@ -8,35 +8,4 @@ describe JiraTrackerData do describe 'Associations' do it { is_expected.to belong_to(:service) } end - - describe 'Validations' do - subject { described_class.new(service: service) } - - context 'jira_issue_transition_id' do - it { is_expected.to allow_value(nil).for(:jira_issue_transition_id) } - it { is_expected.to allow_value('1,2,3').for(:jira_issue_transition_id) } - it { is_expected.to allow_value('1;2;3').for(:jira_issue_transition_id) } - it { is_expected.not_to allow_value('a,b,cd').for(:jira_issue_transition_id) } - end - - context 'url validations' do - context 'when service is inactive' do - it { is_expected.not_to validate_presence_of(:url) } - it { is_expected.not_to validate_presence_of(:username) } - it { is_expected.not_to validate_presence_of(:password) } - end - - context 'when service is active' do - before do - service.update(active: true) - end - - it_behaves_like 'issue tracker service URL attribute', :url - - it { is_expected.to validate_presence_of(:url) } - it { is_expected.to validate_presence_of(:username) } - it { is_expected.to validate_presence_of(:password) } - end - end - end end diff --git a/spec/models/project_services/redmine_service_spec.rb b/spec/models/project_services/redmine_service_spec.rb index c1ee6546b12..4ef4064d069 100644 --- a/spec/models/project_services/redmine_service_spec.rb +++ b/spec/models/project_services/redmine_service_spec.rb @@ -9,6 +9,15 @@ describe RedmineService do end describe 'Validations' do + # if redmine is set in setting the urls are set to defaults + # therefore the validation passes as the values are not nil + before do + settings = { + 'redmine' => {} + } + allow(Gitlab.config).to receive(:issues_tracker).and_return(settings) + end + context 'when service is active' do before do subject.active = true @@ -17,6 +26,7 @@ describe RedmineService do it { is_expected.to validate_presence_of(:project_url) } it { is_expected.to validate_presence_of(:issues_url) } it { is_expected.to validate_presence_of(:new_issue_url) } + it_behaves_like 'issue tracker service URL attribute', :project_url it_behaves_like 'issue tracker service URL attribute', :issues_url it_behaves_like 'issue tracker service URL attribute', :new_issue_url @@ -54,7 +64,7 @@ describe RedmineService do create(:redmine_service, :without_properties_callback, properties: properties) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when data are stored in separated fields' do @@ -62,7 +72,7 @@ describe RedmineService do create(:redmine_service, title: title, description: description, properties: access_params) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when data are stored in both properties and separated fields' do @@ -71,7 +81,7 @@ describe RedmineService do create(:redmine_service, :without_properties_callback, title: title, description: description, properties: properties) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when no title & description are set' do diff --git a/spec/models/project_services/youtrack_service_spec.rb b/spec/models/project_services/youtrack_service_spec.rb index c48bf487af0..eff9f451b1a 100644 --- a/spec/models/project_services/youtrack_service_spec.rb +++ b/spec/models/project_services/youtrack_service_spec.rb @@ -16,6 +16,7 @@ describe YoutrackService do it { is_expected.to validate_presence_of(:project_url) } it { is_expected.to validate_presence_of(:issues_url) } + it_behaves_like 'issue tracker service URL attribute', :project_url it_behaves_like 'issue tracker service URL attribute', :issues_url end @@ -51,7 +52,7 @@ describe YoutrackService do create(:youtrack_service, :without_properties_callback, properties: properties) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when data are stored in separated fields' do @@ -59,7 +60,7 @@ describe YoutrackService do create(:youtrack_service, title: title, description: description, properties: access_params) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when data are stored in both properties and separated fields' do @@ -68,7 +69,7 @@ describe YoutrackService do create(:youtrack_service, :without_properties_callback, title: title, description: description, properties: properties) end - include_examples 'issue tracker fields' + it_behaves_like 'issue tracker fields' end context 'when no title & description are set' do diff --git a/spec/models/release_spec.rb b/spec/models/release_spec.rb index c690390e24d..8714c67f29d 100644 --- a/spec/models/release_spec.rb +++ b/spec/models/release_spec.rb @@ -13,7 +13,8 @@ RSpec.describe Release do it { is_expected.to belong_to(:project) } it { is_expected.to belong_to(:author).class_name('User') } it { is_expected.to have_many(:links).class_name('Releases::Link') } - it { is_expected.to have_one(:milestone) } + it { is_expected.to have_many(:milestones) } + it { is_expected.to have_many(:milestone_releases) } end describe 'validation' do @@ -38,15 +39,15 @@ RSpec.describe Release do context 'when a release is tied to a milestone for another project' do it 'creates a validation error' do - release.milestone = build(:milestone, project: create(:project)) - expect(release).not_to be_valid + milestone = build(:milestone, project: create(:project)) + expect { release.milestones << milestone }.to raise_error end end context 'when a release is tied to a milestone linked to the same project' do - it 'is valid' do - release.milestone = build(:milestone, project: project) - expect(release).to be_valid + it 'successfully links this release to this milestone' do + milestone = build(:milestone, project: project) + expect { release.milestones << milestone }.to change { MilestoneRelease.count }.by(1) end end end diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index 0797b9a9d83..d96e1398677 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -257,8 +257,8 @@ describe Service do expect(service.title).to eq('random title') end - it 'creates the properties' do - expect(service.properties).to eq({ "project_url" => "http://gitlab.example.com" }) + it 'sets data correctly' do + expect(service.data_fields.project_url).to eq('http://gitlab.example.com') end end diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb index 76a70ab6e9e..7153fcc99d7 100644 --- a/spec/requests/api/services_spec.rb +++ b/spec/requests/api/services_spec.rb @@ -100,9 +100,15 @@ describe API::Services do expect(json_response['properties'].keys).to match_array(service_instance.api_field_names) end - it "returns empty hash if properties are empty" do + it "returns empty hash if properties and data fields are empty" do # deprecated services are not valid for update initialized_service.update_attribute(:properties, {}) + + if initialized_service.data_fields_present? + initialized_service.data_fields.destroy + initialized_service.reload + end + get api("/projects/#{project.id}/services/#{dashed_service}", user) expect(response).to have_gitlab_http_status(200) diff --git a/spec/requests/projects/merge_requests_discussions_spec.rb b/spec/requests/projects/merge_requests_discussions_spec.rb new file mode 100644 index 00000000000..5945561aa7b --- /dev/null +++ b/spec/requests/projects/merge_requests_discussions_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'merge requests discussions' do + # Further tests can be found at merge_requests_controller_spec.rb + describe 'GET /:namespace/:project/merge_requests/:iid/discussions' do + let(:project) { create(:project, :repository) } + let(:user) { project.owner } + let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } + + before do + project.add_developer(user) + login_as(user) + end + + def send_request + get discussions_namespace_project_merge_request_path(namespace_id: project.namespace, project_id: project, id: merge_request.iid) + end + + it 'returns 200' do + send_request + + expect(response.status).to eq(200) + end + + # https://docs.gitlab.com/ee/development/query_recorder.html#use-request-specs-instead-of-controller-specs + it 'avoids N+1 DB queries', :request_store do + control = ActiveRecord::QueryRecorder.new { send_request } + + create(:diff_note_on_merge_request, noteable: merge_request, + project: merge_request.project) + + expect do + send_request + end.not_to exceed_query_limit(control) + end + + it 'limits Gitaly queries', :request_store do + Gitlab::GitalyClient.allow_n_plus_1_calls do + create_list(:diff_note_on_merge_request, 7, noteable: merge_request, + project: merge_request.project) + end + + # The creations above write into the Gitaly counts + Gitlab::GitalyClient.reset_counts + + expect { send_request } + .to change { Gitlab::GitalyClient.get_request_count }.by_at_most(4) + end + end +end diff --git a/spec/services/milestones/destroy_service_spec.rb b/spec/services/milestones/destroy_service_spec.rb index ff1e1256166..4f16421c39f 100644 --- a/spec/services/milestones/destroy_service_spec.rb +++ b/spec/services/milestones/destroy_service_spec.rb @@ -72,7 +72,7 @@ describe Milestones::DestroyService do :release, tag: 'v1.0', project: project, - milestone: milestone + milestones: [milestone] ) expect { service.execute(milestone) }.not_to change { Release.count } diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb index 5c9d6537df1..b624b9475e3 100644 --- a/spec/services/releases/create_service_spec.rb +++ b/spec/services/releases/create_service_spec.rb @@ -75,10 +75,12 @@ describe Releases::CreateService do context 'when a passed-in milestone does not exist for this project' do it 'raises an error saying the milestone is inexistent' do - service = described_class.new(project, user, params.merge!({ milestone: 'v111.0' })) + inexistent_milestone_tag = 'v111.0' + service = described_class.new(project, user, params.merge!({ milestones: [inexistent_milestone_tag] })) result = service.execute + expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Milestone does not exist') + expect(result[:message]).to eq("Milestone(s) not found: #{inexistent_milestone_tag}") end end end @@ -93,10 +95,10 @@ describe Releases::CreateService do context 'when existing milestone is passed in' do let(:title) { 'v1.0' } let(:milestone) { create(:milestone, :active, project: project, title: title) } - let(:params_with_milestone) { params.merge!({ milestone: title }) } + let(:params_with_milestone) { params.merge!({ milestones: [title] }) } + let(:service) { described_class.new(milestone.project, user, params_with_milestone) } it 'creates a release and ties this milestone to it' do - service = described_class.new(milestone.project, user, params_with_milestone) result = service.execute expect(project.releases.count).to eq(1) @@ -104,29 +106,66 @@ describe Releases::CreateService do release = project.releases.last - expect(release.milestone).to eq(milestone) + expect(release.milestones).to match_array([milestone]) end context 'when another release was previously created with that same milestone linked' do it 'also creates another release tied to that same milestone' do - other_release = create(:release, milestone: milestone, project: project, tag: 'v1.0') - service = described_class.new(milestone.project, user, params_with_milestone) + other_release = create(:release, milestones: [milestone], project: project, tag: 'v1.0') service.execute release = project.releases.last - expect(release.milestone).to eq(milestone) - expect(other_release.milestone).to eq(milestone) + expect(release.milestones).to match_array([milestone]) + expect(other_release.milestones).to match_array([milestone]) expect(release.id).not_to eq(other_release.id) end end end + context 'when multiple existing milestone titles are passed in' do + let(:title_1) { 'v1.0' } + let(:title_2) { 'v1.0-rc' } + let!(:milestone_1) { create(:milestone, :active, project: project, title: title_1) } + let!(:milestone_2) { create(:milestone, :active, project: project, title: title_2) } + let!(:params_with_milestones) { params.merge!({ milestones: [title_1, title_2] }) } + + it 'creates a release and ties it to these milestones' do + described_class.new(project, user, params_with_milestones).execute + release = project.releases.last + + expect(release.milestones.map(&:title)).to include(title_1, title_2) + end + end + + context 'when multiple miletone titles are passed in but one of them does not exist' do + let(:title) { 'v1.0' } + let(:inexistent_title) { 'v111.0' } + let!(:milestone) { create(:milestone, :active, project: project, title: title) } + let!(:params_with_milestones) { params.merge!({ milestones: [title, inexistent_title] }) } + let(:service) { described_class.new(milestone.project, user, params_with_milestones) } + + it 'raises an error' do + result = service.execute + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("Milestone(s) not found: #{inexistent_title}") + end + + it 'does not create any release' do + expect do + service.execute + end.not_to change(Release, :count) + end + end + context 'when no milestone is passed in' do it 'creates a release without a milestone tied to it' do - expect(params.key? :milestone).to be_falsey + expect(params.key? :milestones).to be_falsey + service.execute release = project.releases.last - expect(release.milestone).to be_nil + + expect(release.milestones).to be_empty end it 'does not create any new MilestoneRelease object' do @@ -136,10 +175,11 @@ describe Releases::CreateService do context 'when an empty value is passed as a milestone' do it 'creates a release without a milestone tied to it' do - service = described_class.new(project, user, params.merge!({ milestone: '' })) + service = described_class.new(project, user, params.merge!({ milestones: [] })) service.execute release = project.releases.last - expect(release.milestone).to be_nil + + expect(release.milestones).to be_empty end end end diff --git a/spec/services/releases/destroy_service_spec.rb b/spec/services/releases/destroy_service_spec.rb index c3172e5edbc..9d027767cd2 100644 --- a/spec/services/releases/destroy_service_spec.rb +++ b/spec/services/releases/destroy_service_spec.rb @@ -60,7 +60,7 @@ describe Releases::DestroyService do context 'when a milestone is tied to the release' do let!(:milestone) { create(:milestone, :active, project: project, title: 'v1.0') } - let!(:release) { create(:release, milestone: milestone, project: project, tag: tag) } + let!(:release) { create(:release, milestones: [milestone], project: project, tag: tag) } it 'destroys the release but leave the milestone intact' do expect { subject }.not_to change { Milestone.count } diff --git a/spec/services/releases/update_service_spec.rb b/spec/services/releases/update_service_spec.rb index 944f3d8c9ad..178bac3574f 100644 --- a/spec/services/releases/update_service_spec.rb +++ b/spec/services/releases/update_service_spec.rb @@ -50,39 +50,60 @@ describe Releases::UpdateService do end context 'when a milestone is passed in' do - let(:old_title) { 'v1.0' } let(:new_title) { 'v2.0' } - let(:milestone) { create(:milestone, project: project, title: old_title) } + let(:milestone) { create(:milestone, project: project, title: 'v1.0') } let(:new_milestone) { create(:milestone, project: project, title: new_title) } - let(:params_with_milestone) { params.merge!({ milestone: new_title }) } + let(:params_with_milestone) { params.merge!({ milestones: [new_title] }) } + let(:service) { described_class.new(new_milestone.project, user, params_with_milestone) } before do - release.milestone = milestone - release.save! + release.milestones << milestone - described_class.new(new_milestone.project, user, params_with_milestone).execute + service.execute release.reload end it 'updates the related milestone accordingly' do - expect(release.milestone.title).to eq(new_title) + expect(release.milestones.first.title).to eq(new_title) end end context "when an 'empty' milestone is passed in" do let(:milestone) { create(:milestone, project: project, title: 'v1.0') } - let(:params_with_empty_milestone) { params.merge!({ milestone: '' }) } + let(:params_with_empty_milestone) { params.merge!({ milestones: [] }) } before do - release.milestone = milestone - release.save! + release.milestones << milestone - described_class.new(milestone.project, user, params_with_empty_milestone).execute + service.params = params_with_empty_milestone + service.execute release.reload end it 'removes the old milestone and does not associate any new milestone' do - expect(release.milestone).to be_nil + expect(release.milestones).not_to be_present + end + end + + context "when multiple new milestones are passed in" do + let(:new_title_1) { 'v2.0' } + let(:new_title_2) { 'v2.0-rc' } + let(:milestone) { create(:milestone, project: project, title: 'v1.0') } + let(:params_with_milestones) { params.merge!({ milestones: [new_title_1, new_title_2] }) } + let(:service) { described_class.new(project, user, params_with_milestones) } + + before do + create(:milestone, project: project, title: new_title_1) + create(:milestone, project: project, title: new_title_2) + release.milestones << milestone + + service.execute + release.reload + end + + it 'removes the old milestone and update the release with the new ones' do + milestone_titles = release.milestones.map(&:title) + expect(milestone_titles).to match_array([new_title_1, new_title_2]) end end end diff --git a/spec/support/helpers/jira_service_helper.rb b/spec/support/helpers/jira_service_helper.rb index 57c33c81ea3..c23a8d52c84 100644 --- a/spec/support/helpers/jira_service_helper.rb +++ b/spec/support/helpers/jira_service_helper.rb @@ -5,16 +5,16 @@ module JiraServiceHelper JIRA_API = JIRA_URL + "/rest/api/2" def jira_service_settings - properties = { - title: "Jira tracker", - url: JIRA_URL, - username: 'jira-user', - password: 'my-secret-password', - project_key: "JIRA", - jira_issue_transition_id: '1' - } + title = "Jira tracker" + url = JIRA_URL + username = 'jira-user' + password = 'my-secret-password' + jira_issue_transition_id = '1' - jira_tracker.update(properties: properties, active: true) + jira_tracker.update( + title: title, url: url, username: username, password: password, + jira_issue_transition_id: jira_issue_transition_id, active: true + ) end def jira_issue_comments |