Merge branch 'use_relative_path_for_project_avatars' into 'master'

Use relative paths for group/project/user avatars

Closes #13418 and #19662

See merge request !11001

5 files changed, 34 insertions, 17 deletions

diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 6d6bcbaf88a..97cf4863ddc 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -77,7 +77,7 @@ module ApplicationHelper
       end
 
     if user
-      user.avatar_url(size) || default_avatar
+      user.avatar_url(size: size) || default_avatar
     else
       gravatar_icon(user_or_email, size, scale)
     end
diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb
new file mode 100644
index 00000000000..8fbfed11bdf
--- /dev/null
+++ b/app/models/concerns/avatarable.rb
@@ -0,0 +1,18 @@
+module Avatarable
+  extend ActiveSupport::Concern
+
+  def avatar_path(only_path: true)
+    return unless self[:avatar].present?
+
+    # If only_path is true then use the relative path of avatar.
+    # Otherwise use full path (including host).
+    asset_host = ActionController::Base.asset_host
+    gitlab_host = only_path ? gitlab_config.relative_url_root : gitlab_config.url
+
+    # If asset_host is set then it is expected that assets are handled by a standalone host.
+    # That means we do not want to get GitLab's relative_url_root option anymore.
+    host = asset_host.present? ? asset_host : gitlab_host
+
+    [host, avatar.url].join
+  end
+end
diff --git a/app/models/group.rb b/app/models/group.rb
index cbc10b00cf5..6aab477f431 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -4,6 +4,7 @@ class Group < Namespace
   include Gitlab::ConfigHelper
   include Gitlab::VisibilityLevel
   include AccessRequestable
+  include Avatarable
   include Referable
   include SelectForProjectAuthorization
 
@@ -111,10 +112,10 @@ class Group < Namespace
     allowed_by_projects
   end
 
-  def avatar_url(size = nil)
-    if self[:avatar].present?
-      [gitlab_config.url, avatar.url].join
-    end
+  def avatar_url(**args)
+    # We use avatar_path instead of overriding avatar_url because of carrierwave.
+    # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
+    avatar_path(args)
   end
 
   def lfs_enabled?
diff --git a/app/models/project.rb b/app/models/project.rb
index a0413b4e651..1f550cc02e2 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -6,6 +6,7 @@ class Project < ActiveRecord::Base
   include Gitlab::VisibilityLevel
   include Gitlab::CurrentSettings
   include AccessRequestable
+  include Avatarable
   include CacheMarkdownField
   include Referable
   include Sortable
@@ -798,12 +799,10 @@ class Project < ActiveRecord::Base
     repository.avatar
   end
 
-  def avatar_url
-    if self[:avatar].present?
-      [gitlab_config.url, avatar.url].join
-    elsif avatar_in_git
-      Gitlab::Routing.url_helpers.namespace_project_avatar_url(namespace, self)
-    end
+  def avatar_url(**args)
+    # We use avatar_path instead of overriding avatar_url because of carrierwave.
+    # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
+    avatar_path(args) || (Gitlab::Routing.url_helpers.namespace_project_avatar_url(namespace, self) if avatar_in_git)
   end
 
   # For compatibility with old code
diff --git a/app/models/user.rb b/app/models/user.rb
index 77b2b12ee0b..f713a20233c 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -5,6 +5,7 @@ class User < ActiveRecord::Base
 
   include Gitlab::ConfigHelper
   include Gitlab::CurrentSettings
+  include Avatarable
   include Referable
   include Sortable
   include CaseSensitivity
@@ -784,12 +785,10 @@ class User < ActiveRecord::Base
     email.start_with?('temp-email-for-oauth')
   end
 
-  def avatar_url(size = nil, scale = 2)
-    if self[:avatar].present?
-      [gitlab_config.url, avatar.url].join
-    else
-      GravatarService.new.execute(email, size, scale)
-    end
+  def avatar_url(size: nil, scale: 2, **args)
+    # We use avatar_path instead of overriding avatar_url because of carrierwave.
+    # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
+    avatar_path(args) || GravatarService.new.execute(email, size, scale)
   end
 
   def all_emails