Merge remote-tracking branch 'origin/master' into dz-codeclimate-comparedz-codeclimate-compare

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

14 files changed, 82 insertions, 92 deletions

diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb
index a7bdf5587b2..eee1a36ac6b 100644
--- a/app/models/concerns/discussion_on_diff.rb
+++ b/app/models/concerns/discussion_on_diff.rb
@@ -47,4 +47,12 @@ module DiscussionOnDiff
 
     prev_lines
   end
+
+  def line_code_in_diffs(diff_refs)
+    if active?(diff_refs)
+      line_code
+    elsif diff_refs && created_at_diff?(diff_refs)
+      original_line_code
+    end
+  end
 end
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 14ddd2fcc88..800574d8be0 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -19,21 +19,9 @@ class DiffDiscussion < Discussion
 
   def merge_request_version_params
     return unless for_merge_request?
+    return {} if active?
 
-    if active?
-      {}
-    else
-      diff_refs = position.diff_refs
-
-      if diff = noteable.merge_request_diff_for(diff_refs)
-        { diff_id: diff.id }
-      elsif diff = noteable.merge_request_diff_for(diff_refs.head_sha)
-        {
-          diff_id: diff.id,
-          start_sha: diff_refs.start_sha
-        }
-      end
-    end
+    noteable.version_params_for(position.diff_refs)
   end
 
   def reply_attributes
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 76c59199afd..1764004078e 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -8,6 +8,7 @@ class DiffNote < Note
 
   serialize :original_position, Gitlab::Diff::Position
   serialize :position, Gitlab::Diff::Position
+  serialize :change_position, Gitlab::Diff::Position
 
   validates :original_position, presence: true
   validates :position, presence: true
@@ -25,7 +26,7 @@ class DiffNote < Note
     DiffDiscussion
   end
 
-  %i(original_position position).each do |meth|
+  %i(original_position position change_position).each do |meth|
     define_method "#{meth}=" do |new_position|
       if new_position.is_a?(String)
         new_position = JSON.parse(new_position) rescue nil
@@ -36,6 +37,8 @@ class DiffNote < Note
         new_position = Gitlab::Diff::Position.new(new_position)
       end
 
+      return if new_position == read_attribute(meth)
+
       super(new_position)
     end
   end
@@ -45,7 +48,7 @@ class DiffNote < Note
   end
 
   def diff_line
-    @diff_line ||= diff_file.line_for_position(self.original_position) if diff_file
+    @diff_line ||= diff_file&.line_for_position(self.original_position)
   end
 
   def for_line?(line)
diff --git a/app/models/hooks/service_hook.rb b/app/models/hooks/service_hook.rb
index eef24052a06..40e43c27f91 100644
--- a/app/models/hooks/service_hook.rb
+++ b/app/models/hooks/service_hook.rb
@@ -2,6 +2,6 @@ class ServiceHook < WebHook
   belongs_to :service
 
   def execute(data)
-    super(data, 'service_hook')
+    WebHookService.new(self, data, 'service_hook').execute
   end
 end
diff --git a/app/models/hooks/system_hook.rb b/app/models/hooks/system_hook.rb
index c645805c6da..1584235ab00 100644
--- a/app/models/hooks/system_hook.rb
+++ b/app/models/hooks/system_hook.rb
@@ -3,8 +3,4 @@ class SystemHook < WebHook
 
   default_value_for :push_events, false
   default_value_for :repository_update_events, true
-
-  def async_execute(data, hook_name)
-    Sidekiq::Client.enqueue(SystemHookWorker, id, data, hook_name)
-  end
 end
diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb
index a165fdc312f..7503f3739c3 100644
--- a/app/models/hooks/web_hook.rb
+++ b/app/models/hooks/web_hook.rb
@@ -1,6 +1,5 @@
 class WebHook < ActiveRecord::Base
   include Sortable
-  include HTTParty
 
   default_value_for :push_events, true
   default_value_for :issues_events, false
@@ -13,52 +12,18 @@ class WebHook < ActiveRecord::Base
   default_value_for :repository_update_events, false
   default_value_for :enable_ssl_verification, true
 
+  has_many :web_hook_logs, dependent: :destroy
+
   scope :push_hooks, -> { where(push_events: true) }
   scope :tag_push_hooks, -> { where(tag_push_events: true) }
 
-  # HTTParty timeout
-  default_timeout Gitlab.config.gitlab.webhook_timeout
-
   validates :url, presence: true, url: true
 
   def execute(data, hook_name)
-    parsed_url = URI.parse(url)
-    if parsed_url.userinfo.blank?
-      response = WebHook.post(url,
-                              body: data.to_json,
-                              headers: build_headers(hook_name),
-                              verify: enable_ssl_verification)
-    else
-      post_url = url.gsub("#{parsed_url.userinfo}@", '')
-      auth = {
-        username: CGI.unescape(parsed_url.user),
-        password: CGI.unescape(parsed_url.password)
-      }
-      response = WebHook.post(post_url,
-                              body: data.to_json,
-                              headers: build_headers(hook_name),
-                              verify: enable_ssl_verification,
-                              basic_auth: auth)
-    end
-
-    [response.code, response.to_s]
-  rescue SocketError, OpenSSL::SSL::SSLError, Errno::ECONNRESET, Errno::ECONNREFUSED, Net::OpenTimeout => e
-    logger.error("WebHook Error => #{e}")
-    [false, e.to_s]
+    WebHookService.new(self, data, hook_name).execute
   end
 
   def async_execute(data, hook_name)
-    Sidekiq::Client.enqueue(ProjectWebHookWorker, id, data, hook_name)
-  end
-
-  private
-
-  def build_headers(hook_name)
-    headers = {
-      'Content-Type' => 'application/json',
-      'X-Gitlab-Event' => hook_name.singularize.titleize
-    }
-    headers['X-Gitlab-Token'] = token if token.present?
-    headers
+    WebHookService.new(self, data, hook_name).async_execute
   end
 end
diff --git a/app/models/hooks/web_hook_log.rb b/app/models/hooks/web_hook_log.rb
new file mode 100644
index 00000000000..2738b229d84
--- /dev/null
+++ b/app/models/hooks/web_hook_log.rb
@@ -0,0 +1,13 @@
+class WebHookLog < ActiveRecord::Base
+  belongs_to :web_hook
+
+  serialize :request_headers, Hash
+  serialize :request_data, Hash
+  serialize :response_headers, Hash
+
+  validates :web_hook, presence: true
+
+  def success?
+    response_status =~ /^2/
+  end
+end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 2fffc7bb0a8..f53f9e2f267 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -416,13 +416,24 @@ class MergeRequest < ActiveRecord::Base
     @merge_request_diffs_by_diff_refs_or_sha[diff_refs_or_sha]
   end
 
+  def version_params_for(diff_refs)
+    if diff = merge_request_diff_for(diff_refs)
+      { diff_id: diff.id }
+    elsif diff = merge_request_diff_for(diff_refs.head_sha)
+      {
+        diff_id: diff.id,
+        start_sha: diff_refs.start_sha
+      }
+    end
+  end
+
   def reload_diff_if_branch_changed
     if source_branch_changed? || target_branch_changed?
       reload_diff
     end
   end
 
-  def reload_diff
+  def reload_diff(current_user = nil)
     return unless open?
 
     old_diff_refs = self.diff_refs
@@ -432,7 +443,8 @@ class MergeRequest < ActiveRecord::Base
 
     update_diff_notes_positions(
       old_diff_refs: old_diff_refs,
-      new_diff_refs: new_diff_refs
+      new_diff_refs: new_diff_refs,
+      current_user: current_user
     )
   end
 
@@ -861,7 +873,7 @@ class MergeRequest < ActiveRecord::Base
     diff_sha_refs && diff_sha_refs.complete?
   end
 
-  def update_diff_notes_positions(old_diff_refs:, new_diff_refs:)
+  def update_diff_notes_positions(old_diff_refs:, new_diff_refs:, current_user: nil)
     return unless has_complete_diff_refs?
     return if new_diff_refs == old_diff_refs
 
@@ -875,7 +887,7 @@ class MergeRequest < ActiveRecord::Base
 
     service = Notes::DiffPositionUpdateService.new(
       self.project,
-      nil,
+      current_user,
       old_diff_refs: old_diff_refs,
       new_diff_refs: new_diff_refs,
       paths: paths
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index f0a3c30ea74..6e3917a10a3 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -175,12 +175,11 @@ class MergeRequestDiff < ActiveRecord::Base
     self == merge_request.merge_request_diff
   end
 
-  def compare_with(sha, straight: true)
+  def compare_with(sha)
     # When compare merge request versions we want diff A..B instead of A...B
     # so we handle cases when user does squash and rebase of the commits between versions.
     # For this reason we set straight to true by default.
-    CompareService.new(project, head_commit_sha)
-      .execute(project, sha, straight: straight)
+    CompareService.new(project, head_commit_sha).execute(project, sha, straight: true)
   end
 
   def commits_count
diff --git a/app/models/note.rb b/app/models/note.rb
index 46d0a4f159f..60257aac93b 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -124,13 +124,12 @@ class Note < ActiveRecord::Base
       groups = {}
 
       diff_notes.fresh.discussions.each do |discussion|
-        if discussion.active?(diff_refs)
-          discussions = groups[discussion.line_code] ||= []
-        elsif diff_refs && discussion.created_at_diff?(diff_refs)
-          discussions = groups[discussion.original_line_code] ||= []
-        end
+        line_code = discussion.line_code_in_diffs(diff_refs)
 
-        discussions << discussion if discussions
+        if line_code
+          discussions = groups[line_code] ||= []
+          discussions << discussion
+        end
       end
 
       groups
diff --git a/app/models/project.rb b/app/models/project.rb
index cfca0dcd2f2..29af57d7664 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -205,8 +205,8 @@ class Project < ActiveRecord::Base
     presence: true,
     dynamic_path: true,
     length: { maximum: 255 },
-    format: { with: Gitlab::Regex.project_path_format_regex,
-              message: Gitlab::Regex.project_path_regex_message },
+    format: { with: Gitlab::PathRegex.project_path_format_regex,
+              message: Gitlab::PathRegex.project_path_format_message },
     uniqueness: { scope: :namespace_id }
 
   validates :namespace, presence: true
@@ -380,11 +380,9 @@ class Project < ActiveRecord::Base
     end
 
     def reference_pattern
-      name_pattern = Gitlab::Regex::FULL_NAMESPACE_REGEX_STR
-
       %r{
-        ((?<namespace>#{name_pattern})\/)?
-        (?<project>#{name_pattern})
+        ((?<namespace>#{Gitlab::PathRegex::FULL_NAMESPACE_FORMAT_REGEX})\/)?
+        (?<project>#{Gitlab::PathRegex::PROJECT_PATH_FORMAT_REGEX})
       }x
     end
 
diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb
index b2494a0be6e..8977a7cdafe 100644
--- a/app/models/project_services/kubernetes_service.rb
+++ b/app/models/project_services/kubernetes_service.rb
@@ -77,6 +77,14 @@ class KubernetesService < DeploymentService
     ]
   end
 
+  def actual_namespace
+    if namespace.present?
+      namespace
+    else
+      default_namespace
+    end
+  end
+
   # Check we can connect to the Kubernetes API
   def test(*args)
     kubeclient = build_kubeclient!
@@ -91,7 +99,7 @@ class KubernetesService < DeploymentService
     variables = [
       { key: 'KUBE_URL', value: api_url, public: true },
       { key: 'KUBE_TOKEN', value: token, public: false },
-      { key: 'KUBE_NAMESPACE', value: namespace_variable, public: true }
+      { key: 'KUBE_NAMESPACE', value: actual_namespace, public: true }
     ]
 
     if ca_pem.present?
@@ -110,7 +118,7 @@ class KubernetesService < DeploymentService
     with_reactive_cache do |data|
       pods = data.fetch(:pods, nil)
       filter_pods(pods, app: environment.slug).
-        flat_map { |pod| terminals_for_pod(api_url, namespace, pod) }.
+        flat_map { |pod| terminals_for_pod(api_url, actual_namespace, pod) }.
         each { |terminal| add_terminal_auth(terminal, terminal_auth) }
     end
   end
@@ -124,7 +132,7 @@ class KubernetesService < DeploymentService
 
     # Store as hashes, rather than as third-party types
     pods = begin
-      kubeclient.get_pods(namespace: namespace).as_json
+      kubeclient.get_pods(namespace: actual_namespace).as_json
     rescue KubeException => err
       raise err unless err.error_code == 404
       []
@@ -142,20 +150,12 @@ class KubernetesService < DeploymentService
     default_namespace || TEMPLATE_PLACEHOLDER
   end
 
-  def namespace_variable
-    if namespace.present?
-      namespace
-    else
-      default_namespace
-    end
-  end
-
   def default_namespace
     "#{project.path}-#{project.id}" if project.present?
   end
 
   def build_kubeclient!(api_path: 'api', api_version: 'v1')
-    raise "Incomplete settings" unless api_url && namespace && token
+    raise "Incomplete settings" unless api_url && actual_namespace && token
 
     ::Kubeclient::Client.new(
       join_api_url(api_path),
diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb
index b44f4fe000c..414c95f7705 100644
--- a/app/models/system_note_metadata.rb
+++ b/app/models/system_note_metadata.rb
@@ -2,6 +2,7 @@ class SystemNoteMetadata < ActiveRecord::Base
   ICON_TYPES = %w[
     commit description merge confidential visible label assignee cross_reference
     title time_tracking branch milestone discussion task moved opened closed merged
+    outdated
   ].freeze
 
   validates :note, presence: true
diff --git a/app/models/user.rb b/app/models/user.rb
index 837ab78228b..9b0c1ebd7c5 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -15,6 +15,7 @@ class User < ActiveRecord::Base
 
   add_authentication_token_field :authentication_token
   add_authentication_token_field :incoming_email_token
+  add_authentication_token_field :rss_token
 
   default_value_for :admin, false
   default_value_for(:external) { current_application_settings.user_default_external }
@@ -367,7 +368,7 @@ class User < ActiveRecord::Base
     def reference_pattern
       %r{
         #{Regexp.escape(reference_prefix)}
-        (?<user>#{Gitlab::Regex::FULL_NAMESPACE_REGEX_STR})
+        (?<user>#{Gitlab::PathRegex::FULL_NAMESPACE_FORMAT_REGEX})
       }x
     end
 
@@ -1004,6 +1005,13 @@ class User < ActiveRecord::Base
     save
   end
 
+  # each existing user needs to have an `rss_token`.
+  # we do this on read since migrating all existing users is not a feasible
+  # solution.
+  def rss_token
+    ensure_rss_token!
+  end
+
   protected
 
   # override, from Devise::Validatable