Commit d2df134f authored by Douwe Maan's avatar Douwe Maan

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
parents b9a4b348 29a3203b
...@@ -77,7 +77,7 @@ module ApplicationHelper ...@@ -77,7 +77,7 @@ module ApplicationHelper
end end
if user if user
user.avatar_url(size) || default_avatar user.avatar_url(size: size) || default_avatar
else else
gravatar_icon(user_or_email, size, scale) gravatar_icon(user_or_email, size, scale)
end 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 ...@@ -4,6 +4,7 @@ class Group < Namespace
include Gitlab::ConfigHelper include Gitlab::ConfigHelper
include Gitlab::VisibilityLevel include Gitlab::VisibilityLevel
include AccessRequestable include AccessRequestable
include Avatarable
include Referable include Referable
include SelectForProjectAuthorization include SelectForProjectAuthorization
...@@ -111,10 +112,10 @@ class Group < Namespace ...@@ -111,10 +112,10 @@ class Group < Namespace
allowed_by_projects allowed_by_projects
end end
def avatar_url(size = nil) def avatar_url(**args)
if self[:avatar].present? # We use avatar_path instead of overriding avatar_url because of carrierwave.
[gitlab_config.url, avatar.url].join # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
end avatar_path(args)
end end
def lfs_enabled? def lfs_enabled?
......
...@@ -6,6 +6,7 @@ class Project < ActiveRecord::Base ...@@ -6,6 +6,7 @@ class Project < ActiveRecord::Base
include Gitlab::VisibilityLevel include Gitlab::VisibilityLevel
include Gitlab::CurrentSettings include Gitlab::CurrentSettings
include AccessRequestable include AccessRequestable
include Avatarable
include CacheMarkdownField include CacheMarkdownField
include Referable include Referable
include Sortable include Sortable
...@@ -798,12 +799,10 @@ class Project < ActiveRecord::Base ...@@ -798,12 +799,10 @@ class Project < ActiveRecord::Base
repository.avatar repository.avatar
end end
def avatar_url def avatar_url(**args)
if self[:avatar].present? # We use avatar_path instead of overriding avatar_url because of carrierwave.
[gitlab_config.url, avatar.url].join # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
elsif avatar_in_git avatar_path(args) || (Gitlab::Routing.url_helpers.namespace_project_avatar_url(namespace, self) if avatar_in_git)
Gitlab::Routing.url_helpers.namespace_project_avatar_url(namespace, self)
end
end end
# For compatibility with old code # For compatibility with old code
......
...@@ -5,6 +5,7 @@ class User < ActiveRecord::Base ...@@ -5,6 +5,7 @@ class User < ActiveRecord::Base
include Gitlab::ConfigHelper include Gitlab::ConfigHelper
include Gitlab::CurrentSettings include Gitlab::CurrentSettings
include Avatarable
include Referable include Referable
include Sortable include Sortable
include CaseSensitivity include CaseSensitivity
...@@ -784,12 +785,10 @@ class User < ActiveRecord::Base ...@@ -784,12 +785,10 @@ class User < ActiveRecord::Base
email.start_with?('temp-email-for-oauth') email.start_with?('temp-email-for-oauth')
end end
def avatar_url(size = nil, scale = 2) def avatar_url(size: nil, scale: 2, **args)
if self[:avatar].present? # We use avatar_path instead of overriding avatar_url because of carrierwave.
[gitlab_config.url, avatar.url].join # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
else avatar_path(args) || GravatarService.new.execute(email, size, scale)
GravatarService.new.execute(email, size, scale)
end
end end
def all_emails def all_emails
......
---
title: Use relative paths for group/project/user avatars
merge_request: 11001
author: blackst0ne
...@@ -30,7 +30,7 @@ Doorkeeper::OpenidConnect.configure do ...@@ -30,7 +30,7 @@ Doorkeeper::OpenidConnect.configure do
o.claim(:email_verified) { |user| true if user.public_email? } o.claim(:email_verified) { |user| true if user.public_email? }
o.claim(:website) { |user| user.full_website_url if user.website_url? } 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(: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 end
end end
...@@ -5,7 +5,10 @@ module API ...@@ -5,7 +5,10 @@ module API
end end
class UserBasic < UserSafe 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| expose :web_url do |user, options|
Gitlab::Routing.url_helpers.user_url(user) Gitlab::Routing.url_helpers.user_url(user)
...@@ -97,7 +100,9 @@ module API ...@@ -97,7 +100,9 @@ module API
expose :creator_id expose :creator_id
expose :namespace, using: 'API::Entities::Namespace' expose :namespace, using: 'API::Entities::Namespace'
expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda{ |project, options| project.forked? } 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 :star_count, :forks_count
expose :open_issues_count, if: lambda { |project, options| project.feature_available?(:issues, options[:current_user]) && project.default_issues_tracker? } 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] } expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] }
...@@ -141,7 +146,9 @@ module API ...@@ -141,7 +146,9 @@ module API
class Group < Grape::Entity class Group < Grape::Entity
expose :id, :name, :path, :description, :visibility expose :id, :name, :path, :description, :visibility
expose :lfs_enabled?, as: :lfs_enabled 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 :web_url
expose :request_access_enabled expose :request_access_enabled
expose :full_name, :full_path expose :full_name, :full_path
......
...@@ -69,7 +69,9 @@ module API ...@@ -69,7 +69,9 @@ module API
expose :creator_id expose :creator_id
expose :namespace, using: 'API::Entities::Namespace' expose :namespace, using: 'API::Entities::Namespace'
expose :forked_from_project, using: ::API::Entities::BasicProjectDetails, if: lambda{ |project, options| project.forked? } 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 :star_count, :forks_count
expose :open_issues_count, if: lambda { |project, options| project.feature_available?(:issues, options[:current_user]) && project.default_issues_tracker? } 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] } expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] }
...@@ -129,7 +131,9 @@ module API ...@@ -129,7 +131,9 @@ module API
class Group < Grape::Entity class Group < Grape::Entity
expose :id, :name, :path, :description, :visibility_level expose :id, :name, :path, :description, :visibility_level
expose :lfs_enabled?, as: :lfs_enabled 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 :web_url
expose :request_access_enabled expose :request_access_enabled
expose :full_name, :full_path expose :full_name, :full_path
......
...@@ -3,6 +3,8 @@ require 'spec_helper' ...@@ -3,6 +3,8 @@ require 'spec_helper'
describe ApplicationHelper do describe ApplicationHelper do
include UploadHelpers include UploadHelpers
let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
describe 'current_controller?' do describe 'current_controller?' do
it 'returns true when controller matches argument' do it 'returns true when controller matches argument' do
stub_controller_name('foo') stub_controller_name('foo')
...@@ -56,8 +58,14 @@ describe ApplicationHelper do ...@@ -56,8 +58,14 @@ describe ApplicationHelper do
describe 'project_icon' do describe 'project_icon' do
it 'returns an url for the avatar' do it 'returns an url for the avatar' do
project = create(:empty_project, avatar: File.open(uploaded_image_temp_path)) 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). expect(helper.project_icon(project.full_path).to_s).
to eq "<img src=\"#{avatar_url}\" alt=\"Banana sample\" />" to eq "<img src=\"#{avatar_url}\" alt=\"Banana sample\" />"
end end
...@@ -67,9 +75,8 @@ describe ApplicationHelper do ...@@ -67,9 +75,8 @@ describe ApplicationHelper do
allow_any_instance_of(Project).to receive(:avatar_in_git).and_return(true) 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)}" avatar_url = "#{gitlab_host}#{namespace_project_avatar_path(project.namespace, project)}"
expect(helper.project_icon(project.full_path).to_s).to match( expect(helper.project_icon(project.full_path).to_s).to match(image_tag(avatar_url))
image_tag(avatar_url))
end end
end end
...@@ -77,8 +84,14 @@ describe ApplicationHelper do ...@@ -77,8 +84,14 @@ describe ApplicationHelper do
it 'returns an url for the avatar' do it 'returns an url for the avatar' do
user = create(:user, avatar: File.open(uploaded_image_temp_path)) user = create(:user, avatar: File.open(uploaded_image_temp_path))
expect(helper.avatar_icon(user.email).to_s). avatar_url = "/uploads/user/avatar/#{user.id}/banana_sample.gif"
to match("/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 end
it 'returns an url for the avatar with relative url' do it 'returns an url for the avatar with relative url' do
......
...@@ -15,7 +15,7 @@ describe AvatarsHelper do ...@@ -15,7 +15,7 @@ describe AvatarsHelper do
end end
it "contains the user's avatar image" do 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 end
end end
...@@ -178,16 +178,20 @@ describe Group, models: true do ...@@ -178,16 +178,20 @@ describe Group, models: true do
describe '#avatar_url' do describe '#avatar_url' do
let!(:group) { create(:group, :access_requestable, :with_avatar) } let!(:group) { create(:group, :access_requestable, :with_avatar) }
let(:user) { create(:user) } 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 context 'when avatar file is uploaded' do
before do before { group.add_master(user) }
group.add_master(user)
end
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
end end
......
...@@ -813,8 +813,16 @@ describe Project, models: true do ...@@ -813,8 +813,16 @@ describe Project, models: true do
context 'when avatar file is uploaded' do context 'when avatar file is uploaded' do
let(:project) { create(:empty_project, :with_avatar) } let(:project) { create(:empty_project, :with_avatar) }
let(:avatar_path) { "/uploads/project/avatar/#{project.id}/dk.png" } 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 end
context 'When avatar file in git' do context 'When avatar file in git' do
......
...@@ -964,12 +964,19 @@ describe User, models: true do ...@@ -964,12 +964,19 @@ describe User, models: true do
describe '#avatar_url' do describe '#avatar_url' do
let(:user) { create(:user, :with_avatar) } let(:user) { create(:user, :with_avatar) }
subject { user.avatar_url }
context 'when avatar file is uploaded' do 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" } 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
end end
......
...@@ -178,7 +178,7 @@ describe API::Groups do ...@@ -178,7 +178,7 @@ describe API::Groups do
expect(json_response['path']).to eq(group1.path) expect(json_response['path']).to eq(group1.path)
expect(json_response['description']).to eq(group1.description) expect(json_response['description']).to eq(group1.description)
expect(json_response['visibility']).to eq(Gitlab::VisibilityLevel.string_level(group1.visibility_level)) 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['web_url']).to eq(group1.web_url)
expect(json_response['request_access_enabled']).to eq(group1.request_access_enabled) expect(json_response['request_access_enabled']).to eq(group1.request_access_enabled)
expect(json_response['full_name']).to eq(group1.full_name) expect(json_response['full_name']).to eq(group1.full_name)
......
...@@ -176,7 +176,7 @@ describe API::V3::Groups do ...@@ -176,7 +176,7 @@ describe API::V3::Groups do
expect(json_response['path']).to eq(group1.path) expect(json_response['path']).to eq(group1.path)
expect(json_response['description']).to eq(group1.description) expect(json_response['description']).to eq(group1.description)
expect(json_response['visibility_level']).to eq(group1.visibility_level) 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['web_url']).to eq(group1.web_url)
expect(json_response['request_access_enabled']).to eq(group1.request_access_enabled) expect(json_response['request_access_enabled']).to eq(group1.request_access_enabled)
expect(json_response['full_name']).to eq(group1.full_name) expect(json_response['full_name']).to eq(group1.full_name)
......
...@@ -6,7 +6,6 @@ describe Projects::ParticipantsService, services: true do ...@@ -6,7 +6,6 @@ describe Projects::ParticipantsService, services: true do
let(:project) { create(:empty_project, :public) } let(:project) { create(:empty_project, :public) }
let(:group) { create(:group, avatar: fixture_file_upload(Rails.root + 'spec/fixtures/dk.png')) } let(:group) { create(:group, avatar: fixture_file_upload(Rails.root + 'spec/fixtures/dk.png')) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:base_url) { Settings.send(:build_base_gitlab_url) }
let!(:group_member) { create(:group_member, group: group, user: user) } let!(:group_member) { create(:group_member, group: group, user: user) }
it 'should return an url for the avatar' do it 'should return an url for the avatar' do
...@@ -14,7 +13,7 @@ describe Projects::ParticipantsService, services: true do ...@@ -14,7 +13,7 @@ describe Projects::ParticipantsService, services: true do
groups = participants.groups groups = participants.groups
expect(groups.size).to eq 1 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 end
it 'should return an url for the avatar with relative url' do it 'should return an url for the avatar with relative url' do
...@@ -25,7 +24,7 @@ describe Projects::ParticipantsService, services: true do ...@@ -25,7 +24,7 @@ describe Projects::ParticipantsService, services: true do
groups = participants.groups groups = participants.groups
expect(groups.size).to eq 1 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 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