Commit 29a3203b authored by blackst0ne's avatar blackst0ne
Browse files

Use relative paths for group/project/user avatars

parent 22f21a897d34
......@@ -77,7 +77,7 @@ def avatar_icon(user_or_email = nil, size = nil, scale = 2)
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
......
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
......@@ -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 @@ def visibility_level_allowed_by_projects
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?
......
......@@ -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 @@ def avatar_in_git
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
......
......@@ -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 @@ def temp_oauth_email?
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
......
---
title: Use relative paths for group/project/user avatars
merge_request: 11001
author: blackst0ne
......@@ -30,7 +30,7 @@
o.claim(:email_verified) { |user| true if user.public_email? }
o.claim(:website) { |user| user.full_website_url if user.website_url? }
o.claim(:profile) { |user| Rails.application.routes.url_helpers.user_url user }
o.claim(:picture) { |user| user.avatar_url }
o.claim(:picture) { |user| user.avatar_url(only_path: false) }
end
end
end
......@@ -5,7 +5,10 @@ class UserSafe < Grape::Entity
end
class UserBasic < UserSafe
expose :id, :state, :avatar_url
expose :id, :state
expose :avatar_url do |user, options|
user.avatar_url(only_path: false)
end
expose :web_url do |user, options|
Gitlab::Routing.url_helpers.user_url(user)
......@@ -97,7 +100,9 @@ class Project < Grape::Entity
expose :creator_id
expose :namespace, using: 'API::Entities::Namespace'
expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda{ |project, options| project.forked? }
expose :avatar_url
expose :avatar_url do |user, options|
user.avatar_url(only_path: false)
end
expose :star_count, :forks_count
expose :open_issues_count, if: lambda { |project, options| project.feature_available?(:issues, options[:current_user]) && project.default_issues_tracker? }
expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] }
......@@ -141,7 +146,9 @@ class AccessRequester < UserBasic
class Group < Grape::Entity
expose :id, :name, :path, :description, :visibility
expose :lfs_enabled?, as: :lfs_enabled
expose :avatar_url
expose :avatar_url do |user, options|
user.avatar_url(only_path: false)
end
expose :web_url
expose :request_access_enabled
expose :full_name, :full_path
......
......@@ -69,7 +69,9 @@ class Project < Grape::Entity
expose :creator_id
expose :namespace, using: 'API::Entities::Namespace'
expose :forked_from_project, using: ::API::Entities::BasicProjectDetails, if: lambda{ |project, options| project.forked? }
expose :avatar_url
expose :avatar_url do |user, options|
user.avatar_url(only_path: false)
end
expose :star_count, :forks_count
expose :open_issues_count, if: lambda { |project, options| project.feature_available?(:issues, options[:current_user]) && project.default_issues_tracker? }
expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] }
......@@ -129,7 +131,9 @@ class MergeRequest < Grape::Entity
class Group < Grape::Entity
expose :id, :name, :path, :description, :visibility_level
expose :lfs_enabled?, as: :lfs_enabled
expose :avatar_url
expose :avatar_url do |user, options|
user.avatar_url(only_path: false)
end
expose :web_url
expose :request_access_enabled
expose :full_name, :full_path
......
......@@ -3,6 +3,8 @@
describe ApplicationHelper do
include UploadHelpers
let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
describe 'current_controller?' do
it 'returns true when controller matches argument' do
stub_controller_name('foo')
......@@ -56,8 +58,14 @@ def stub_action_name(value)
describe 'project_icon' do
it 'returns an url for the avatar' do
project = create(:empty_project, avatar: File.open(uploaded_image_temp_path))
avatar_url = "/uploads/project/avatar/#{project.id}/banana_sample.gif"
expect(helper.project_icon(project.full_path).to_s).
to eq "<img src=\"#{avatar_url}\" alt=\"Banana sample\" />"
allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
avatar_url = "#{gitlab_host}/uploads/project/avatar/#{project.id}/banana_sample.gif"
avatar_url = "http://#{Gitlab.config.gitlab.host}/uploads/project/avatar/#{project.id}/banana_sample.gif"
expect(helper.project_icon(project.full_path).to_s).
to eq "<img src=\"#{avatar_url}\" alt=\"Banana sample\" />"
end
......@@ -67,9 +75,8 @@ def stub_action_name(value)
allow_any_instance_of(Project).to receive(:avatar_in_git).and_return(true)
avatar_url = "http://#{Gitlab.config.gitlab.host}#{namespace_project_avatar_path(project.namespace, project)}"
expect(helper.project_icon(project.full_path).to_s).to match(
image_tag(avatar_url))
avatar_url = "#{gitlab_host}#{namespace_project_avatar_path(project.namespace, project)}"
expect(helper.project_icon(project.full_path).to_s).to match(image_tag(avatar_url))
end
end
......@@ -77,8 +84,14 @@ def stub_action_name(value)
it 'returns an url for the avatar' do
user = create(:user, avatar: File.open(uploaded_image_temp_path))
expect(helper.avatar_icon(user.email).to_s).
to match("/uploads/user/avatar/#{user.id}/banana_sample.gif")
avatar_url = "/uploads/user/avatar/#{user.id}/banana_sample.gif"
expect(helper.avatar_icon(user.email).to_s).to match(avatar_url)
allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
avatar_url = "#{gitlab_host}/uploads/user/avatar/#{user.id}/banana_sample.gif"
expect(helper.avatar_icon(user.email).to_s).to match(avatar_url)
end
it 'returns an url for the avatar with relative url' do
......
......@@ -15,7 +15,7 @@
end
it "contains the user's avatar image" do
is_expected.to include(CGI.escapeHTML(user.avatar_url(16)))
is_expected.to include(CGI.escapeHTML(user.avatar_url(size: 16)))
end
end
end
......@@ -178,16 +178,20 @@
describe '#avatar_url' do
let!(:group) { create(:group, :access_requestable, :with_avatar) }
let(:user) { create(:user) }
subject { group.avatar_url }
let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
let(:avatar_path) { "/uploads/group/avatar/#{group.id}/dk.png" }
context 'when avatar file is uploaded' do
before do
group.add_master(user)
end
before { group.add_master(user) }
let(:avatar_path) { "/uploads/group/avatar/#{group.id}/dk.png" }
it 'shows correct avatar url' do
expect(group.avatar_url).to eq(avatar_path)
expect(group.avatar_url(only_path: false)).to eq([gitlab_host, avatar_path].join)
allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
it { should eq "http://#{Gitlab.config.gitlab.host}#{avatar_path}" }
expect(group.avatar_url).to eq([gitlab_host, avatar_path].join)
end
end
end
......
......@@ -813,8 +813,16 @@
context 'when avatar file is uploaded' do
let(:project) { create(:empty_project, :with_avatar) }
let(:avatar_path) { "/uploads/project/avatar/#{project.id}/dk.png" }
let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
it { should eq "http://#{Gitlab.config.gitlab.host}#{avatar_path}" }
it 'shows correct url' do
expect(project.avatar_url).to eq(avatar_path)
expect(project.avatar_url(only_path: false)).to eq([gitlab_host, avatar_path].join)
allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
expect(project.avatar_url).to eq([gitlab_host, avatar_path].join)
end
end
context 'When avatar file in git' do
......
......@@ -964,12 +964,19 @@
describe '#avatar_url' do
let(:user) { create(:user, :with_avatar) }
subject { user.avatar_url }
context 'when avatar file is uploaded' do
let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
let(:avatar_path) { "/uploads/user/avatar/#{user.id}/dk.png" }
it { should eq "http://#{Gitlab.config.gitlab.host}#{avatar_path}" }
it 'shows correct avatar url' do
expect(user.avatar_url).to eq(avatar_path)
expect(user.avatar_url(only_path: false)).to eq([gitlab_host, avatar_path].join)
allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
expect(user.avatar_url).to eq([gitlab_host, avatar_path].join)
end
end
end
......
......@@ -178,7 +178,7 @@
expect(json_response['path']).to eq(group1.path)
expect(json_response['description']).to eq(group1.description)
expect(json_response['visibility']).to eq(Gitlab::VisibilityLevel.string_level(group1.visibility_level))
expect(json_response['avatar_url']).to eq(group1.avatar_url)
expect(json_response['avatar_url']).to eq(group1.avatar_url(only_path: false))
expect(json_response['web_url']).to eq(group1.web_url)
expect(json_response['request_access_enabled']).to eq(group1.request_access_enabled)
expect(json_response['full_name']).to eq(group1.full_name)
......
......@@ -176,7 +176,7 @@
expect(json_response['path']).to eq(group1.path)
expect(json_response['description']).to eq(group1.description)
expect(json_response['visibility_level']).to eq(group1.visibility_level)
expect(json_response['avatar_url']).to eq(group1.avatar_url)
expect(json_response['avatar_url']).to eq(group1.avatar_url(only_path: false))
expect(json_response['web_url']).to eq(group1.web_url)
expect(json_response['request_access_enabled']).to eq(group1.request_access_enabled)
expect(json_response['full_name']).to eq(group1.full_name)
......
......@@ -6,7 +6,6 @@
let(:project) { create(:empty_project, :public) }
let(:group) { create(:group, avatar: fixture_file_upload(Rails.root + 'spec/fixtures/dk.png')) }
let(:user) { create(:user) }
let(:base_url) { Settings.send(:build_base_gitlab_url) }
let!(:group_member) { create(:group_member, group: group, user: user) }
it 'should return an url for the avatar' do
......@@ -14,7 +13,7 @@
groups = participants.groups
expect(groups.size).to eq 1
expect(groups.first[:avatar_url]).to eq "#{base_url}/uploads/group/avatar/#{group.id}/dk.png"
expect(groups.first[:avatar_url]).to eq("/uploads/group/avatar/#{group.id}/dk.png")
end
it 'should return an url for the avatar with relative url' do
......@@ -25,7 +24,7 @@
groups = participants.groups
expect(groups.size).to eq 1
expect(groups.first[:avatar_url]).to eq "#{base_url}/gitlab/uploads/group/avatar/#{group.id}/dk.png"
expect(groups.first[:avatar_url]).to eq("/gitlab/uploads/group/avatar/#{group.id}/dk.png")
end
end
end
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment