summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--.gitlab/issue_templates/Problem_Validation.md41
-rw-r--r--app/assets/stylesheets/framework/snippets.scss4
-rw-r--r--app/controllers/projects/merge_requests_controller.rb2
-rw-r--r--app/models/diff_note.rb13
-rw-r--r--app/models/merge_request.rb17
-rw-r--r--app/models/milestone.rb9
-rw-r--r--app/models/milestone_release.rb4
-rw-r--r--app/models/note_diff_file.rb6
-rw-r--r--app/models/project_services/bugzilla_service.rb2
-rw-r--r--app/models/project_services/custom_issue_tracker_service.rb2
-rw-r--r--app/models/project_services/data_fields.rb52
-rw-r--r--app/models/project_services/gitlab_issue_tracker_service.rb2
-rw-r--r--app/models/project_services/issue_tracker_data.rb3
-rw-r--r--app/models/project_services/issue_tracker_service.rb63
-rw-r--r--app/models/project_services/jira_service.rb33
-rw-r--r--app/models/project_services/jira_tracker_data.rb7
-rw-r--r--app/models/project_services/redmine_service.rb2
-rw-r--r--app/models/project_services/youtrack_service.rb2
-rw-r--r--app/models/release.rb9
-rw-r--r--app/services/releases/concerns.rb22
-rw-r--r--app/services/releases/create_service.rb4
-rw-r--r--app/services/releases/update_service.rb4
-rw-r--r--app/views/shared/snippets/_form.html.haml2
-rw-r--r--app/views/shared/snippets/_header.html.haml2
-rw-r--r--changelogs/unreleased/66986-milestone-release-many-to-many.yml5
-rw-r--r--changelogs/unreleased/language-trends-over-time.yml5
-rw-r--r--changelogs/unreleased/osw-improve-discussions-query.yml5
-rw-r--r--config/routes.rb2
-rw-r--r--db/migrate/20190910125852_create_analytics_language_trend_repository_languages.rb35
-rw-r--r--db/post_migrate/20190904205212_remove_id_column_from_intermediate_release_milestones.rb9
-rw-r--r--db/schema.rb16
-rw-r--r--doc/api/releases/index.md173
-rw-r--r--doc/development/api_styleguide.md13
-rw-r--r--doc/raketasks/cleanup.md5
-rw-r--r--lib/api/entities.rb9
-rw-r--r--lib/api/releases.rb4
-rw-r--r--lib/gitlab/discussions_diff/file_collection.rb29
-rw-r--r--lib/gitlab/usage_data.rb49
-rw-r--r--spec/controllers/projects/merge_requests_controller_spec.rb6
-rw-r--r--spec/factories/services.rb74
-rw-r--r--spec/fixtures/api/schemas/public_api/v4/release.json5
-rw-r--r--spec/lib/banzai/pipeline/gfm_pipeline_spec.rb6
-rw-r--r--spec/lib/gitlab/discussions_diff/file_collection_spec.rb39
-rw-r--r--spec/lib/gitlab/import_export/all_models.yml8
-rw-r--r--spec/lib/gitlab/usage_data_spec.rb16
-rw-r--r--spec/models/merge_request_spec.rb59
-rw-r--r--spec/models/milestone_release_spec.rb22
-rw-r--r--spec/models/milestone_spec.rb9
-rw-r--r--spec/models/project_services/bugzilla_service_spec.rb6
-rw-r--r--spec/models/project_services/custom_issue_tracker_service_spec.rb6
-rw-r--r--spec/models/project_services/data_fields_spec.rb138
-rw-r--r--spec/models/project_services/gitlab_issue_tracker_service_spec.rb6
-rw-r--r--spec/models/project_services/issue_tracker_data_spec.rb24
-rw-r--r--spec/models/project_services/issue_tracker_service_spec.rb2
-rw-r--r--spec/models/project_services/jira_service_spec.rb428
-rw-r--r--spec/models/project_services/jira_tracker_data_spec.rb31
-rw-r--r--spec/models/project_services/redmine_service_spec.rb16
-rw-r--r--spec/models/project_services/youtrack_service_spec.rb7
-rw-r--r--spec/models/release_spec.rb13
-rw-r--r--spec/models/service_spec.rb4
-rw-r--r--spec/requests/api/services_spec.rb8
-rw-r--r--spec/requests/projects/merge_requests_discussions_spec.rb52
-rw-r--r--spec/services/milestones/destroy_service_spec.rb2
-rw-r--r--spec/services/releases/create_service_spec.rb66
-rw-r--r--spec/services/releases/destroy_service_spec.rb2
-rw-r--r--spec/services/releases/update_service_spec.rb45
-rw-r--r--spec/support/helpers/jira_service_helper.rb18
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