From bc7ef8e5b7a002ca6bc2d7a5e6be11b4a59b6710 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Tue, 29 Dec 2015 17:53:55 -0200 Subject: [PATCH 01/32] Add ldap_blocked as new state to users state machine --- app/models/user.rb | 12 ++++++++++- spec/models/user_spec.rb | 44 +++++++++++++++++++++++++--------------- 2 files changed, 39 insertions(+), 17 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 46b36c605b..67b47b0f32 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -198,16 +198,26 @@ class User < ActiveRecord::Base transition active: :blocked end + event :ldap_block do + transition active: :ldap_blocked + end + event :activate do transition blocked: :active end + + state :blocked, :ldap_blocked do + def blocked? + true + end + end end mount_uploader :avatar, AvatarUploader # Scopes scope :admins, -> { where(admin: true) } - scope :blocked, -> { with_state(:blocked) } + scope :blocked, -> { with_states(:blocked, :ldap_blocked) } scope :active, -> { with_state(:active) } scope :not_in_project, ->(project) { project.users.present? ? where("id not in (:ids)", ids: project.users.map(&:id) ) : all } scope :without_projects, -> { where('id NOT IN (SELECT DISTINCT(user_id) FROM members)') } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3cd63b2b0e..0bef68e288 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -569,27 +569,39 @@ describe User, models: true do end end - describe :ldap_user? do - it "is true if provider name starts with ldap" do - user = create(:omniauth_user, provider: 'ldapmain') - expect( user.ldap_user? ).to be_truthy + context 'ldap synchronized user' do + describe :ldap_user? do + it 'is true if provider name starts with ldap' do + user = create(:omniauth_user, provider: 'ldapmain') + expect(user.ldap_user?).to be_truthy + end + + it 'is false for other providers' do + user = create(:omniauth_user, provider: 'other-provider') + expect(user.ldap_user?).to be_falsey + end + + it 'is false if no extern_uid is provided' do + user = create(:omniauth_user, extern_uid: nil) + expect(user.ldap_user?).to be_falsey + end end - it "is false for other providers" do - user = create(:omniauth_user, provider: 'other-provider') - expect( user.ldap_user? ).to be_falsey + describe :ldap_identity do + it 'returns ldap identity' do + user = create :omniauth_user + expect(user.ldap_identity.provider).not_to be_empty + end end - it "is false if no extern_uid is provided" do - user = create(:omniauth_user, extern_uid: nil) - expect( user.ldap_user? ).to be_falsey - end - end + describe '#ldap_block' do + let(:user) { create(:omniauth_user, provider: 'ldapmain', name: 'John Smith') } - describe :ldap_identity do - it "returns ldap identity" do - user = create :omniauth_user - expect(user.ldap_identity.provider).not_to be_empty + it 'blocks user flaging the action caming from ldap' do + user.ldap_block + expect(user.blocked?).to be_truthy + expect(user.ldap_blocked?).to be_truthy + end end end From ba9855d4877998e3574907cc542fcab15a9d1353 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Tue, 29 Dec 2015 18:58:38 -0200 Subject: [PATCH 02/32] Prevent ldap_blocked users from being unblocked by the Admin UI --- app/assets/stylesheets/framework/buttons.scss | 3 ++ app/controllers/admin/users_controller.rb | 4 ++- app/views/admin/users/index.html.haml | 7 +++- .../admin/users_controller_spec.rb | 33 ++++++++++++++----- 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index 97a9463884..e237636348 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -132,6 +132,9 @@ margin-right: 0px; } } + &.disabled { + pointer-events: auto !important; + } } .btn-block { diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index d7c927d444..87f4fb455b 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -40,7 +40,9 @@ class Admin::UsersController < Admin::ApplicationController end def unblock - if user.activate + if user.ldap_blocked? + redirect_back_or_admin_user(alert: "This user cannot be unlocked manually from GitLab") + elsif user.activate redirect_back_or_admin_user(notice: "Successfully unblocked") else redirect_back_or_admin_user(alert: "Error occurred. User was not unblocked") diff --git a/app/views/admin/users/index.html.haml b/app/views/admin/users/index.html.haml index a92c9c152b..911c4d0cf1 100644 --- a/app/views/admin/users/index.html.haml +++ b/app/views/admin/users/index.html.haml @@ -90,7 +90,12 @@   = link_to 'Edit', edit_admin_user_path(user), id: "edit_#{dom_id(user)}", class: "btn btn-xs" - unless user == current_user - - if user.blocked? + - if user.ldap_blocked? + = link_to '#', title: 'Cannot unblock LDAP blocked users', data: {toggle: 'tooltip'}, class: 'btn btn-xs btn-success disabled' do + %i.fa.fa-lock + Unblock + = '' + - elsif user.blocked? = link_to 'Unblock', unblock_admin_user_path(user), method: :put, class: "btn btn-xs btn-success" - else = link_to 'Block', block_admin_user_path(user), data: {confirm: 'USER WILL BE BLOCKED! Are you sure?'}, method: :put, class: "btn btn-xs btn-warning" diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 8b7af4d3a0..5b1f65d7af 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -34,17 +34,34 @@ describe Admin::UsersController do end describe 'PUT unblock/:id' do - let(:user) { create(:user) } + context 'ldap blocked users' do + let(:user) { create(:omniauth_user, provider: 'ldapmain') } - before do - user.block + before do + user.ldap_block + end + + it 'will not unblock user' do + put :unblock, id: user.username + user.reload + expect(user.blocked?).to be_truthy + expect(flash[:alert]).to eq 'This user cannot be unlocked manually from GitLab' + end end - it 'unblocks user' do - put :unblock, id: user.username - user.reload - expect(user.blocked?).to be_falsey - expect(flash[:notice]).to eq 'Successfully unblocked' + context 'manually blocked users' do + let(:user) { create(:user) } + + before do + user.block + end + + it 'unblocks user' do + put :unblock, id: user.username + user.reload + expect(user.blocked?).to be_falsey + expect(flash[:notice]).to eq 'Successfully unblocked' + end end end From 6e7db8e23e169bcbf0847ece27b9e44e00ae572b Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 30 Dec 2015 16:52:02 -0200 Subject: [PATCH 03/32] Prevent ldap_blocked users from being blocked/unblocked by the API --- doc/api/users.md | 6 ++++-- lib/api/users.rb | 12 ++++++++---- spec/requests/api/users_spec.rb | 23 ++++++++++++++++++----- 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/doc/api/users.md b/doc/api/users.md index 773fe36d27..b7fc903825 100644 --- a/doc/api/users.md +++ b/doc/api/users.md @@ -558,7 +558,8 @@ Parameters: - `uid` (required) - id of specified user -Will return `200 OK` on success, or `404 User Not Found` is user cannot be found. +Will return `200 OK` on success, `404 User Not Found` is user cannot be found or +`403 Forbidden` when trying to block an already blocked user by LDAP synchronization. ## Unblock user @@ -572,4 +573,5 @@ Parameters: - `uid` (required) - id of specified user -Will return `200 OK` on success, or `404 User Not Found` is user cannot be found. +Will return `200 OK` on success, `404 User Not Found` is user cannot be found or +`403 Forbidden` when trying to unblock a user blocked by LDAP synchronization. diff --git a/lib/api/users.rb b/lib/api/users.rb index 0d7813428e..01fd90139b 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -284,10 +284,12 @@ module API authenticated_as_admin! user = User.find_by(id: params[:id]) - if user + if !user + not_found!('User') + elsif !user.ldap_blocked? user.block else - not_found!('User') + forbidden!('LDAP blocked users cannot be modified by the API') end end @@ -299,10 +301,12 @@ module API authenticated_as_admin! user = User.find_by(id: params[:id]) - if user + if !user + not_found!('User') + elsif !user.ldap_blocked? user.activate else - not_found!('User') + forbidden!('LDAP blocked users cannot be unblocked by the API') end end end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 4f278551d0..b82c5c7685 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -8,6 +8,8 @@ describe API::API, api: true do let(:key) { create(:key, user: user) } let(:email) { create(:email, user: user) } let(:omniauth_user) { create(:omniauth_user) } + let(:ldap_user) { create(:omniauth_user, provider: 'ldapmain') } + let(:ldap_blocked_user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') } describe "GET /users" do context "when unauthenticated" do @@ -783,6 +785,12 @@ describe API::API, api: true do expect(user.reload.state).to eq('blocked') end + it 'should not re-block ldap blocked users' do + put api("/users/#{ldap_blocked_user.id}/block", admin) + expect(response.status).to eq(403) + expect(ldap_blocked_user.reload.state).to eq('ldap_blocked') + end + it 'should not be available for non admin users' do put api("/users/#{user.id}/block", user) expect(response.status).to eq(403) @@ -797,7 +805,9 @@ describe API::API, api: true do end describe 'PUT /user/:id/unblock' do + let(:blocked_user) { create(:user, state: 'blocked') } before { admin } + it 'should unblock existing user' do put api("/users/#{user.id}/unblock", admin) expect(response.status).to eq(200) @@ -805,12 +815,15 @@ describe API::API, api: true do end it 'should unblock a blocked user' do - put api("/users/#{user.id}/block", admin) + put api("/users/#{blocked_user.id}/unblock", admin) expect(response.status).to eq(200) - expect(user.reload.state).to eq('blocked') - put api("/users/#{user.id}/unblock", admin) - expect(response.status).to eq(200) - expect(user.reload.state).to eq('active') + expect(blocked_user.reload.state).to eq('active') + end + + it 'should not unblock ldap blocked users' do + put api("/users/#{ldap_blocked_user.id}/unblock", admin) + expect(response.status).to eq(403) + expect(ldap_blocked_user.reload.state).to eq('ldap_blocked') end it 'should not be available for non admin users' do From d6dc088affeee4568e771e1d7894e0bcdb955af8 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 30 Dec 2015 20:56:26 -0200 Subject: [PATCH 04/32] LDAP synchronization block/unblock new states --- lib/gitlab/ldap/access.rb | 6 ++--- spec/lib/gitlab/ldap/access_spec.rb | 34 ++++++++++++----------------- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index c438a3d167..76cb48d7aa 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -37,15 +37,15 @@ module Gitlab # Block user in GitLab if he/she was blocked in AD if Gitlab::LDAP::Person.disabled_via_active_directory?(user.ldap_identity.extern_uid, adapter) - user.block + user.ldap_block false else - user.activate if user.blocked? && !ldap_config.block_auto_created_users + user.activate if (user.blocked? && !ldap_config.block_auto_created_users) || user.ldap_blocked? true end else # Block the user if they no longer exist in LDAP/AD - user.block + user.ldap_block false end rescue diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index a628d0c015..f58d70e809 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -13,64 +13,59 @@ describe Gitlab::LDAP::Access, lib: true do end it { is_expected.to be_falsey } - + it 'should block user in GitLab' do access.allowed? expect(user).to be_blocked + expect(user).to be_ldap_blocked end end context 'when the user is found' do before do - allow(Gitlab::LDAP::Person). - to receive(:find_by_dn).and_return(:ldap_user) + allow(Gitlab::LDAP::Person).to receive(:find_by_dn).and_return(:ldap_user) end context 'and the user is disabled via active directory' do before do - allow(Gitlab::LDAP::Person). - to receive(:disabled_via_active_directory?).and_return(true) + allow(Gitlab::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(true) end it { is_expected.to be_falsey } - it "should block user in GitLab" do + it 'should block user in GitLab' do access.allowed? expect(user).to be_blocked + expect(user).to be_ldap_blocked end end context 'and has no disabled flag in active diretory' do before do user.block - - allow(Gitlab::LDAP::Person). - to receive(:disabled_via_active_directory?).and_return(false) + allow(Gitlab::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(false) end it { is_expected.to be_truthy } context 'when auto-created users are blocked' do - before do - allow_any_instance_of(Gitlab::LDAP::Config). - to receive(:block_auto_created_users).and_return(true) + allow_any_instance_of(Gitlab::LDAP::Config).to receive(:block_auto_created_users).and_return(true) end - it "does not unblock user in GitLab" do + it 'does not unblock user in GitLab' do access.allowed? expect(user).to be_blocked + expect(user).not_to be_ldap_blocked # this block is handled by omniauth not by our internal logic end end - context "when auto-created users are not blocked" do - + context 'when auto-created users are not blocked' do before do - allow_any_instance_of(Gitlab::LDAP::Config). - to receive(:block_auto_created_users).and_return(false) + allow_any_instance_of(Gitlab::LDAP::Config).to receive(:block_auto_created_users).and_return(false) end - it "should unblock user in GitLab" do + it 'should unblock user in GitLab' do access.allowed? expect(user).not_to be_blocked end @@ -80,8 +75,7 @@ describe Gitlab::LDAP::Access, lib: true do context 'without ActiveDirectory enabled' do before do allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true) - allow_any_instance_of(Gitlab::LDAP::Config). - to receive(:active_directory).and_return(false) + allow_any_instance_of(Gitlab::LDAP::Config).to receive(:active_directory).and_return(false) end it { is_expected.to be_truthy } From ec67e9be1d7486199b47e19c766202a8bfdefe93 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 6 Jan 2016 05:38:52 -0200 Subject: [PATCH 05/32] Repair ldap_blocked state when no ldap identity exist anymore --- .../admin/identities_controller.rb | 2 + app/models/identity.rb | 4 ++ app/models/user.rb | 1 + .../repair_ldap_blocked_user_service.rb | 15 ++++++++ .../admin/identities_controller_spec.rb | 26 +++++++++++++ spec/models/identity_spec.rb | 38 +++++++++++++++++++ .../repair_ldap_blocked_user_service_spec.rb | 23 +++++++++++ 7 files changed, 109 insertions(+) create mode 100644 app/services/repair_ldap_blocked_user_service.rb create mode 100644 spec/controllers/admin/identities_controller_spec.rb create mode 100644 spec/models/identity_spec.rb create mode 100644 spec/services/repair_ldap_blocked_user_service_spec.rb diff --git a/app/controllers/admin/identities_controller.rb b/app/controllers/admin/identities_controller.rb index e383fe38ea..9ba1048751 100644 --- a/app/controllers/admin/identities_controller.rb +++ b/app/controllers/admin/identities_controller.rb @@ -26,6 +26,7 @@ class Admin::IdentitiesController < Admin::ApplicationController def update if @identity.update_attributes(identity_params) + RepairLdapBlockedUserService.new(@user, @identity).execute redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully updated.' else render :edit @@ -34,6 +35,7 @@ class Admin::IdentitiesController < Admin::ApplicationController def destroy if @identity.destroy + RepairLdapBlockedUserService.new(@user, @identity).execute redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully removed.' else redirect_to admin_user_identities_path(@user), alert: 'Failed to remove user identity.' diff --git a/app/models/identity.rb b/app/models/identity.rb index 8bcdc19495..830b99fa3f 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -18,4 +18,8 @@ class Identity < ActiveRecord::Base validates :provider, presence: true validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider } validates :user_id, uniqueness: { scope: :provider } + + def is_ldap? + provider.starts_with?('ldap') + end end diff --git a/app/models/user.rb b/app/models/user.rb index 67b47b0f32..5eed9cf91c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -196,6 +196,7 @@ class User < ActiveRecord::Base state_machine :state, initial: :active do event :block do transition active: :blocked + transition ldap_blocked: :blocked end event :ldap_block do diff --git a/app/services/repair_ldap_blocked_user_service.rb b/app/services/repair_ldap_blocked_user_service.rb new file mode 100644 index 0000000000..ceca15414e --- /dev/null +++ b/app/services/repair_ldap_blocked_user_service.rb @@ -0,0 +1,15 @@ +class RepairLdapBlockedUserService + attr_accessor :user, :identity + + def initialize(user, identity) + @user, @identity = user, identity + end + + def execute + if identity.destroyed? + user.block if identity.is_ldap? && user.ldap_blocked? && !user.ldap_user? + else + user.block if !identity.is_ldap? && user.ldap_blocked? && !user.ldap_user? + end + end +end diff --git a/spec/controllers/admin/identities_controller_spec.rb b/spec/controllers/admin/identities_controller_spec.rb new file mode 100644 index 0000000000..c131d22a30 --- /dev/null +++ b/spec/controllers/admin/identities_controller_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe Admin::IdentitiesController do + let(:admin) { create(:admin) } + before { sign_in(admin) } + + describe 'UPDATE identity' do + let(:user) { create(:omniauth_user, provider: 'ldapmain', extern_uid: 'uid=myuser,ou=people,dc=example,dc=com') } + + it 'repairs ldap blocks' do + expect_any_instance_of(RepairLdapBlockedUserService).to receive(:execute) + + put :update, user_id: user.username, id: user.ldap_identity.id, identity: { provider: 'twitter' } + end + end + + describe 'DELETE identity' do + let(:user) { create(:omniauth_user, provider: 'ldapmain', extern_uid: 'uid=myuser,ou=people,dc=example,dc=com') } + + it 'repairs ldap blocks' do + expect_any_instance_of(RepairLdapBlockedUserService).to receive(:execute) + + delete :destroy, user_id: user.username, id: user.ldap_identity.id + end + end +end diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb new file mode 100644 index 0000000000..107bfc1778 --- /dev/null +++ b/spec/models/identity_spec.rb @@ -0,0 +1,38 @@ +# == Schema Information +# +# Table name: identities +# +# id :integer not null, primary key +# extern_uid :string(255) +# provider :string(255) +# user_id :integer +# created_at :datetime +# updated_at :datetime +# + +require 'spec_helper' + +RSpec.describe Identity, models: true do + + describe 'relations' do + it { is_expected.to belong_to(:user) } + end + + describe 'fields' do + it { is_expected.to respond_to(:provider) } + it { is_expected.to respond_to(:extern_uid) } + end + + describe '#is_ldap?' do + let(:ldap_identity) { create(:identity, provider: 'ldapmain') } + let(:other_identity) { create(:identity, provider: 'twitter') } + + it 'returns true if it is a ldap identity' do + expect(ldap_identity.is_ldap?).to be_truthy + end + + it 'returns false if it is not a ldap identity' do + expect(other_identity.is_ldap?).to be_falsey + end + end +end diff --git a/spec/services/repair_ldap_blocked_user_service_spec.rb b/spec/services/repair_ldap_blocked_user_service_spec.rb new file mode 100644 index 0000000000..2a2114d038 --- /dev/null +++ b/spec/services/repair_ldap_blocked_user_service_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe RepairLdapBlockedUserService, services: true do + let(:user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') } + let(:identity) { user.ldap_identity } + subject(:service) { RepairLdapBlockedUserService.new(user, identity) } + + describe '#execute' do + it 'change to normal block after destroying last ldap identity' do + identity.destroy + service.execute + + expect(user.reload).not_to be_ldap_blocked + end + + it 'change to normal block after changing last ldap identity to another provider' do + identity.update_attribute(:provider, 'twitter') + service.execute + + expect(user.reload).not_to be_ldap_blocked + end + end +end From 47e4613f4adc2d6ef4b066a87ec772ef8044bdd5 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Thu, 7 Jan 2016 14:01:01 -0200 Subject: [PATCH 06/32] Code style fixes and some code simplified --- app/controllers/admin/identities_controller.rb | 4 ++-- .../repair_ldap_blocked_user_service.rb | 18 ++++++++++-------- lib/gitlab/ldap/access.rb | 4 +++- .../repair_ldap_blocked_user_service_spec.rb | 2 +- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/app/controllers/admin/identities_controller.rb b/app/controllers/admin/identities_controller.rb index 9ba1048751..79a53556f0 100644 --- a/app/controllers/admin/identities_controller.rb +++ b/app/controllers/admin/identities_controller.rb @@ -26,7 +26,7 @@ class Admin::IdentitiesController < Admin::ApplicationController def update if @identity.update_attributes(identity_params) - RepairLdapBlockedUserService.new(@user, @identity).execute + RepairLdapBlockedUserService.new(@user).execute redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully updated.' else render :edit @@ -35,7 +35,7 @@ class Admin::IdentitiesController < Admin::ApplicationController def destroy if @identity.destroy - RepairLdapBlockedUserService.new(@user, @identity).execute + RepairLdapBlockedUserService.new(@user).execute redirect_to admin_user_identities_path(@user), notice: 'User identity was successfully removed.' else redirect_to admin_user_identities_path(@user), alert: 'Failed to remove user identity.' diff --git a/app/services/repair_ldap_blocked_user_service.rb b/app/services/repair_ldap_blocked_user_service.rb index ceca15414e..863cef7ff6 100644 --- a/app/services/repair_ldap_blocked_user_service.rb +++ b/app/services/repair_ldap_blocked_user_service.rb @@ -1,15 +1,17 @@ class RepairLdapBlockedUserService - attr_accessor :user, :identity + attr_accessor :user - def initialize(user, identity) - @user, @identity = user, identity + def initialize(user) + @user = user end def execute - if identity.destroyed? - user.block if identity.is_ldap? && user.ldap_blocked? && !user.ldap_user? - else - user.block if !identity.is_ldap? && user.ldap_blocked? && !user.ldap_user? - end + user.block if ldap_hard_blocked? + end + + private + + def ldap_hard_blocked? + user.ldap_blocked? && !user.ldap_user? end end diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 76cb48d7aa..ebd9260ad5 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -40,7 +40,9 @@ module Gitlab user.ldap_block false else - user.activate if (user.blocked? && !ldap_config.block_auto_created_users) || user.ldap_blocked? + if (user.blocked? && !ldap_config.block_auto_created_users) || user.ldap_blocked? + user.activate + end true end else diff --git a/spec/services/repair_ldap_blocked_user_service_spec.rb b/spec/services/repair_ldap_blocked_user_service_spec.rb index 2a2114d038..ce7d145597 100644 --- a/spec/services/repair_ldap_blocked_user_service_spec.rb +++ b/spec/services/repair_ldap_blocked_user_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe RepairLdapBlockedUserService, services: true do let(:user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') } let(:identity) { user.ldap_identity } - subject(:service) { RepairLdapBlockedUserService.new(user, identity) } + subject(:service) { RepairLdapBlockedUserService.new(user) } describe '#execute' do it 'change to normal block after destroying last ldap identity' do From ac6a10f3e88c5d2081b8638df63016089517a844 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Tue, 12 Jan 2016 12:29:10 -0200 Subject: [PATCH 07/32] Codestyle changes --- app/models/identity.rb | 2 +- lib/api/users.rb | 6 +++--- spec/models/identity_spec.rb | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/identity.rb b/app/models/identity.rb index 830b99fa3f..e1915b079d 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -19,7 +19,7 @@ class Identity < ActiveRecord::Base validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider } validates :user_id, uniqueness: { scope: :provider } - def is_ldap? + def ldap? provider.starts_with?('ldap') end end diff --git a/lib/api/users.rb b/lib/api/users.rb index 01fd90139b..fd2128bd17 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -303,10 +303,10 @@ module API if !user not_found!('User') - elsif !user.ldap_blocked? - user.activate - else + elsif user.ldap_blocked? forbidden!('LDAP blocked users cannot be unblocked by the API') + else + user.activate end end end diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb index 107bfc1778..5afe042e15 100644 --- a/spec/models/identity_spec.rb +++ b/spec/models/identity_spec.rb @@ -28,11 +28,11 @@ RSpec.describe Identity, models: true do let(:other_identity) { create(:identity, provider: 'twitter') } it 'returns true if it is a ldap identity' do - expect(ldap_identity.is_ldap?).to be_truthy + expect(ldap_identity.ldap?).to be_truthy end it 'returns false if it is not a ldap identity' do - expect(other_identity.is_ldap?).to be_falsey + expect(other_identity.ldap?).to be_falsey end end end From 9d7f88c12258e27a189e8229090920db0627e88b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 12 Jan 2016 18:10:06 +0100 Subject: [PATCH 08/32] Show referenced MRs & Issues only when the current viewer can access them --- CHANGELOG | 1 + app/controllers/projects/issues_controller.rb | 2 +- app/models/issue.rb | 4 +- app/views/projects/notes/_notes.html.haml | 1 + features/project/issues/references.feature | 31 +++++ features/steps/project/issues/references.rb | 106 ++++++++++++++++++ features/steps/shared/note.rb | 6 + features/steps/shared/project.rb | 7 ++ 8 files changed, 155 insertions(+), 3 deletions(-) create mode 100644 features/project/issues/references.feature create mode 100644 features/steps/project/issues/references.rb diff --git a/CHANGELOG b/CHANGELOG index ab34661ce0..f765899b42 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -38,6 +38,7 @@ v 8.4.0 (unreleased) - Ajax filter by message for commits page - API: Add support for deleting a tag via the API (Robert Schilling) - Allow subsequent validations in CI Linter + - Show referenced MRs & Issues only when the current viewer can access them v 8.3.3 - Preserve CE behavior with JIRA integration by only calling API if URL is set diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index b59b52291f..f476afb2d9 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -61,7 +61,7 @@ class Projects::IssuesController < Projects::ApplicationController @note = @project.notes.new(noteable: @issue) @notes = @issue.notes.nonawards.with_associations.fresh @noteable = @issue - @merge_requests = @issue.referenced_merge_requests + @merge_requests = @issue.referenced_merge_requests(current_user) respond_with(@issue) end diff --git a/app/models/issue.rb b/app/models/issue.rb index f52e47f3e6..7beba98460 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -85,10 +85,10 @@ class Issue < ActiveRecord::Base reference end - def referenced_merge_requests + def referenced_merge_requests(current_user = nil) Gitlab::ReferenceExtractor.lazily do [self, *notes].flat_map do |note| - note.all_references.merge_requests + note.all_references(current_user).merge_requests end end.sort_by(&:iid) end diff --git a/app/views/projects/notes/_notes.html.haml b/app/views/projects/notes/_notes.html.haml index ca60dd239b..a4ff947c65 100644 --- a/app/views/projects/notes/_notes.html.haml +++ b/app/views/projects/notes/_notes.html.haml @@ -8,4 +8,5 @@ - else - @notes.each do |note| - next unless note.author + - next if note.cross_reference? && note.referenced_mentionables(current_user).empty? = render note diff --git a/features/project/issues/references.feature b/features/project/issues/references.feature new file mode 100644 index 0000000000..bf7a4c6cb9 --- /dev/null +++ b/features/project/issues/references.feature @@ -0,0 +1,31 @@ +@project_issues +Feature: Project Issues References + Background: + Given I sign in as "John Doe" + And "John Doe" owns public project "Community" + And project "Community" has "Public Issue 01" open issue + And I logout + And I sign in as "Mary Jane" + And "Mary Jane" owns private project "Private Library" + And project "Private Library" has "Fix NS-01" open merge request + And project "Private Library" has "Private Issue 01" open issue + And I visit merge request page "Fix NS-01" + And I leave a comment referencing issue "Public Issue 01" from "Fix NS-01" merge request + And I visit issue page "Private Issue 01" + And I leave a comment referencing issue "Public Issue 01" from "Private Issue 01" issue + And I logout + + @javascript + Scenario: Viewing the public issue as a "John Doe" + Given I sign in as "John Doe" + When I visit issue page "Public Issue 01" + Then I should not see any related merge requests + And I should see no notes at all + + @javascript + Scenario: Viewing the public issue as "Mary Jane" + Given I sign in as "Mary Jane" + When I visit issue page "Public Issue 01" + Then I should see the "Fix NS-01" related merge request + And I should see a note linking to "Fix NS-01" merge request + And I should see a note linking to "Private Issue 01" issue diff --git a/features/steps/project/issues/references.rb b/features/steps/project/issues/references.rb new file mode 100644 index 0000000000..9c7725e8c1 --- /dev/null +++ b/features/steps/project/issues/references.rb @@ -0,0 +1,106 @@ +class Spinach::Features::ProjectIssuesReferences < Spinach::FeatureSteps + include SharedAuthentication + include SharedNote + include SharedProject + include SharedUser + + step 'project "Community" has "Public Issue 01" open issue' do + project = Project.find_by(name: 'Community') + create(:issue, + title: 'Public Issue 01', + project: project, + author: project.users.first, + description: '# Description header' + ) + end + + step 'project "Private Library" has "Fix NS-01" open merge request' do + project = Project.find_by(name: 'Private Library') + create(:merge_request, + title: 'Fix NS-01', + source_project: project, + target_project: project, + source_branch: 'fix', + target_branch: 'master', + author: project.users.first, + description: '# Description header' + ) + end + + step 'project "Private Library" has "Private Issue 01" open issue' do + project = Project.find_by(name: 'Private Library') + create(:issue, + title: 'Private Issue 01', + project: project, + author: project.users.first, + description: '# Description header' + ) + end + + step 'I leave a comment referencing issue "Public Issue 01" from "Fix NS-01" merge request' do + project = Project.find_by(name: 'Private Library') + issue = Issue.find_by!(title: 'Public Issue 01') + + page.within(".js-main-target-form") do + fill_in "note[note]", with: "##{issue.to_reference(project)}" + click_button "Add Comment" + end + end + + step 'I leave a comment referencing issue "Public Issue 01" from "Private Issue 01" issue' do + project = Project.find_by(name: 'Private Library') + issue = Issue.find_by!(title: 'Public Issue 01') + + page.within(".js-main-target-form") do + fill_in "note[note]", with: "##{issue.to_reference(project)}" + click_button "Add Comment" + end + end + + step 'I visit merge request page "Fix NS-01"' do + mr = MergeRequest.find_by(title: "Fix NS-01") + visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr) + end + + step 'I visit issue page "Private Issue 01"' do + issue = Issue.find_by(title: "Private Issue 01") + visit namespace_project_issue_path(issue.project.namespace, issue.project, issue) + end + + step 'I visit issue page "Public Issue 01"' do + issue = Issue.find_by(title: "Public Issue 01") + visit namespace_project_issue_path(issue.project.namespace, issue.project, issue) + end + + step 'I should not see any related merge requests' do + page.within '.issue-details' do + expect(page).not_to have_content('.merge-requests') + end + end + + step 'I should see the "Fix NS-01" related merge request' do + page.within '.merge-requests' do + expect(page).to have_content("1 Related Merge Request") + expect(page).to have_content('Fix NS-01') + end + end + + step 'I should see a note linking to "Fix NS-01" merge request' do + project = Project.find_by(name: 'Community') + mr = MergeRequest.find_by(title: 'Fix NS-01') + page.within('.notes') do + expect(page).to have_content('Mary Jane') + expect(page).to have_content("mentioned in merge request #{mr.to_reference(project)}") + end + end + + step 'I should see a note linking to "Private Issue 01" issue' do + project = Project.find_by(name: 'Community') + issue = Issue.find_by(title: 'Private Issue 01') + page.within('.notes') do + expect(page).to have_content('Mary Jane') + expect(page).to have_content("mentioned in issue #{issue.to_reference(project)}") + end + end + +end diff --git a/features/steps/shared/note.rb b/features/steps/shared/note.rb index f6aabfefef..6de58c6e2b 100644 --- a/features/steps/shared/note.rb +++ b/features/steps/shared/note.rb @@ -106,6 +106,12 @@ module SharedNote end end + step 'I should see no notes at all' do + page.within('.notes') do + expect(page).to_not have_css('.note') + end + end + # Markdown step 'I leave a comment with a header containing "Comment with a header"' do diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index da643bf3ba..43a15f4347 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -181,6 +181,13 @@ module SharedProject project.team << [user, :master] end + step '"Mary Jane" owns private project "Private Library"' do + user = user_exists('Mary Jane', username: 'mary_jane') + project = Project.find_by(name: 'Private Library') + project ||= create(:project, name: 'Private Library', namespace: user.namespace) + project.team << [user, :master] + end + step 'public empty project "Empty Public Project"' do create :project_empty_repo, :public, name: "Empty Public Project" end From 5efbfa14d4666655edd6d79a3a352a134382b664 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 13 Jan 2016 16:37:17 +0100 Subject: [PATCH 09/32] Move complex view condition to a model method This is moved to a model method rather than an helper method because the API will need it too. --- app/models/note.rb | 4 ++++ app/views/projects/notes/_notes.html.haml | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/models/note.rb b/app/models/note.rb index 3d5b663c99..3e1375e5ad 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -358,6 +358,10 @@ class Note < ActiveRecord::Base !system? && !is_award end + def cross_reference_not_visible_for?(user) + cross_reference? && referenced_mentionables(user).empty? + end + # Checks if note is an award added as a comment # # If note is an award, this method sets is_award to true diff --git a/app/views/projects/notes/_notes.html.haml b/app/views/projects/notes/_notes.html.haml index a4ff947c65..62db86fb18 100644 --- a/app/views/projects/notes/_notes.html.haml +++ b/app/views/projects/notes/_notes.html.haml @@ -2,11 +2,14 @@ - @discussions.each do |discussion_notes| - note = discussion_notes.first - if note_for_main_target?(note) + - next if note.cross_reference_not_visible_for?(current_user) + = render discussion_notes - else = render 'projects/notes/discussion', discussion_notes: discussion_notes - else - @notes.each do |note| - next unless note.author - - next if note.cross_reference? && note.referenced_mentionables(current_user).empty? + - next if note.cross_reference_not_visible_for?(current_user) + = render note From 5e452d3794ffa4996611ecf53c6098f4a3913d4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 13 Jan 2016 16:38:42 +0100 Subject: [PATCH 10/32] Improve & adds specs for Issue/MR references - Improve specs for private Issue/MR referenced in public Issue - Add specs for private Issue/MR referenced in public MR --- features/project/issues/references.feature | 28 ++-- .../project/merge_requests/references.feature | 31 ++++ features/steps/project/issues/references.rb | 101 +------------ .../project/merge_requests/references.rb | 7 + features/steps/shared/issuable.rb | 134 ++++++++++++++++++ features/steps/shared/note.rb | 4 +- features/steps/shared/project.rb | 48 ++++--- 7 files changed, 218 insertions(+), 135 deletions(-) create mode 100644 features/project/merge_requests/references.feature create mode 100644 features/steps/project/merge_requests/references.rb diff --git a/features/project/issues/references.feature b/features/project/issues/references.feature index bf7a4c6cb9..4ae2d65333 100644 --- a/features/project/issues/references.feature +++ b/features/project/issues/references.feature @@ -2,30 +2,32 @@ Feature: Project Issues References Background: Given I sign in as "John Doe" + And public project "Community" And "John Doe" owns public project "Community" - And project "Community" has "Public Issue 01" open issue + And project "Community" has "Community issue" open issue And I logout And I sign in as "Mary Jane" - And "Mary Jane" owns private project "Private Library" - And project "Private Library" has "Fix NS-01" open merge request - And project "Private Library" has "Private Issue 01" open issue - And I visit merge request page "Fix NS-01" - And I leave a comment referencing issue "Public Issue 01" from "Fix NS-01" merge request - And I visit issue page "Private Issue 01" - And I leave a comment referencing issue "Public Issue 01" from "Private Issue 01" issue + And private project "Enterprise" + And "Mary Jane" owns private project "Enterprise" + And project "Enterprise" has "Enterprise issue" open issue + And project "Enterprise" has "Enterprise fix" open merge request + And I visit issue page "Enterprise issue" + And I leave a comment referencing issue "Community issue" + And I visit merge request page "Enterprise fix" + And I leave a comment referencing issue "Community issue" And I logout @javascript Scenario: Viewing the public issue as a "John Doe" Given I sign in as "John Doe" - When I visit issue page "Public Issue 01" + When I visit issue page "Community issue" Then I should not see any related merge requests And I should see no notes at all @javascript Scenario: Viewing the public issue as "Mary Jane" Given I sign in as "Mary Jane" - When I visit issue page "Public Issue 01" - Then I should see the "Fix NS-01" related merge request - And I should see a note linking to "Fix NS-01" merge request - And I should see a note linking to "Private Issue 01" issue + When I visit issue page "Community issue" + Then I should see the "Enterprise fix" related merge request + And I should see a note linking to "Enterprise fix" merge request + And I should see a note linking to "Enterprise issue" issue diff --git a/features/project/merge_requests/references.feature b/features/project/merge_requests/references.feature new file mode 100644 index 0000000000..571612261a --- /dev/null +++ b/features/project/merge_requests/references.feature @@ -0,0 +1,31 @@ +@project_merge_requests +Feature: Project Merge Requests References + Background: + Given I sign in as "John Doe" + And public project "Community" + And "John Doe" owns public project "Community" + And project "Community" has "Community fix" open merge request + And I logout + And I sign in as "Mary Jane" + And private project "Enterprise" + And "Mary Jane" owns private project "Enterprise" + And project "Enterprise" has "Enterprise issue" open issue + And project "Enterprise" has "Enterprise fix" open merge request + And I visit issue page "Enterprise issue" + And I leave a comment referencing issue "Community fix" + And I visit merge request page "Enterprise fix" + And I leave a comment referencing issue "Community fix" + And I logout + + @javascript + Scenario: Viewing the public issue as a "John Doe" + Given I sign in as "John Doe" + When I visit issue page "Community fix" + Then I should see no notes at all + + @javascript + Scenario: Viewing the public issue as "Mary Jane" + Given I sign in as "Mary Jane" + When I visit issue page "Community fix" + And I should see a note linking to "Enterprise fix" merge request + And I should see a note linking to "Enterprise issue" issue diff --git a/features/steps/project/issues/references.rb b/features/steps/project/issues/references.rb index 9c7725e8c1..69e8b5cbde 100644 --- a/features/steps/project/issues/references.rb +++ b/features/steps/project/issues/references.rb @@ -1,106 +1,7 @@ class Spinach::Features::ProjectIssuesReferences < Spinach::FeatureSteps include SharedAuthentication + include SharedIssuable include SharedNote include SharedProject include SharedUser - - step 'project "Community" has "Public Issue 01" open issue' do - project = Project.find_by(name: 'Community') - create(:issue, - title: 'Public Issue 01', - project: project, - author: project.users.first, - description: '# Description header' - ) - end - - step 'project "Private Library" has "Fix NS-01" open merge request' do - project = Project.find_by(name: 'Private Library') - create(:merge_request, - title: 'Fix NS-01', - source_project: project, - target_project: project, - source_branch: 'fix', - target_branch: 'master', - author: project.users.first, - description: '# Description header' - ) - end - - step 'project "Private Library" has "Private Issue 01" open issue' do - project = Project.find_by(name: 'Private Library') - create(:issue, - title: 'Private Issue 01', - project: project, - author: project.users.first, - description: '# Description header' - ) - end - - step 'I leave a comment referencing issue "Public Issue 01" from "Fix NS-01" merge request' do - project = Project.find_by(name: 'Private Library') - issue = Issue.find_by!(title: 'Public Issue 01') - - page.within(".js-main-target-form") do - fill_in "note[note]", with: "##{issue.to_reference(project)}" - click_button "Add Comment" - end - end - - step 'I leave a comment referencing issue "Public Issue 01" from "Private Issue 01" issue' do - project = Project.find_by(name: 'Private Library') - issue = Issue.find_by!(title: 'Public Issue 01') - - page.within(".js-main-target-form") do - fill_in "note[note]", with: "##{issue.to_reference(project)}" - click_button "Add Comment" - end - end - - step 'I visit merge request page "Fix NS-01"' do - mr = MergeRequest.find_by(title: "Fix NS-01") - visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr) - end - - step 'I visit issue page "Private Issue 01"' do - issue = Issue.find_by(title: "Private Issue 01") - visit namespace_project_issue_path(issue.project.namespace, issue.project, issue) - end - - step 'I visit issue page "Public Issue 01"' do - issue = Issue.find_by(title: "Public Issue 01") - visit namespace_project_issue_path(issue.project.namespace, issue.project, issue) - end - - step 'I should not see any related merge requests' do - page.within '.issue-details' do - expect(page).not_to have_content('.merge-requests') - end - end - - step 'I should see the "Fix NS-01" related merge request' do - page.within '.merge-requests' do - expect(page).to have_content("1 Related Merge Request") - expect(page).to have_content('Fix NS-01') - end - end - - step 'I should see a note linking to "Fix NS-01" merge request' do - project = Project.find_by(name: 'Community') - mr = MergeRequest.find_by(title: 'Fix NS-01') - page.within('.notes') do - expect(page).to have_content('Mary Jane') - expect(page).to have_content("mentioned in merge request #{mr.to_reference(project)}") - end - end - - step 'I should see a note linking to "Private Issue 01" issue' do - project = Project.find_by(name: 'Community') - issue = Issue.find_by(title: 'Private Issue 01') - page.within('.notes') do - expect(page).to have_content('Mary Jane') - expect(page).to have_content("mentioned in issue #{issue.to_reference(project)}") - end - end - end diff --git a/features/steps/project/merge_requests/references.rb b/features/steps/project/merge_requests/references.rb new file mode 100644 index 0000000000..ab2ae6847a --- /dev/null +++ b/features/steps/project/merge_requests/references.rb @@ -0,0 +1,7 @@ +class Spinach::Features::ProjectMergeRequestsReferences < Spinach::FeatureSteps + include SharedAuthentication + include SharedIssuable + include SharedNote + include SharedProject + include SharedUser +end diff --git a/features/steps/shared/issuable.rb b/features/steps/shared/issuable.rb index e6d1b8b8ef..4c5f7488ef 100644 --- a/features/steps/shared/issuable.rb +++ b/features/steps/shared/issuable.rb @@ -5,6 +5,99 @@ module SharedIssuable find(:css, '.issuable-edit').click end + step 'project "Community" has "Community issue" open issue' do + create_issuable_for_project( + project_name: 'Community', + title: 'Community issue' + ) + end + + step 'project "Community" has "Community fix" open merge request' do + create_issuable_for_project( + project_name: 'Community', + type: :merge_request, + title: 'Community fix' + ) + end + + step 'project "Enterprise" has "Enterprise issue" open issue' do + create_issuable_for_project( + project_name: 'Enterprise', + title: 'Enterprise issue' + ) + end + + step 'project "Enterprise" has "Enterprise fix" open merge request' do + create_issuable_for_project( + project_name: 'Enterprise', + type: :merge_request, + title: 'Enterprise fix' + ) + end + + step 'I leave a comment referencing issue "Community issue"' do + leave_reference_comment( + issuable: Issue.find_by(title: 'Community issue'), + from_project_name: 'Enterprise' + ) + end + + step 'I leave a comment referencing issue "Community fix"' do + leave_reference_comment( + issuable: MergeRequest.find_by(title: 'Community fix'), + from_project_name: 'Enterprise' + ) + end + + step 'I visit issue page "Enterprise issue"' do + issue = Issue.find_by(title: 'Enterprise issue') + visit namespace_project_issue_path(issue.project.namespace, issue.project, issue) + end + + step 'I visit merge request page "Enterprise fix"' do + mr = MergeRequest.find_by(title: 'Enterprise fix') + visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr) + end + + step 'I visit issue page "Community issue"' do + issue = Issue.find_by(title: 'Community issue') + visit namespace_project_issue_path(issue.project.namespace, issue.project, issue) + end + + step 'I visit issue page "Community fix"' do + mr = MergeRequest.find_by(title: 'Community fix') + visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr) + end + + step 'I should not see any related merge requests' do + page.within '.issue-details' do + expect(page).not_to have_content('.merge-requests') + end + end + + step 'I should see the "Enterprise fix" related merge request' do + page.within '.merge-requests' do + expect(page).to have_content('1 Related Merge Request') + expect(page).to have_content('Enterprise fix') + end + end + + step 'I should see a note linking to "Enterprise fix" merge request' do + visible_note( + issuable: MergeRequest.find_by(title: 'Enterprise fix'), + from_project_name: 'Community', + user_name: 'Mary Jane' + ) + end + + step 'I should see a note linking to "Enterprise issue" issue' do + visible_note( + issuable: Issue.find_by(title: 'Enterprise issue'), + from_project_name: 'Community', + user_name: 'Mary Jane' + ) + end + step 'I click link "Edit" for the merge request' do edit_issuable end @@ -12,4 +105,45 @@ module SharedIssuable step 'I click link "Edit" for the issue' do edit_issuable end + + def create_issuable_for_project(project_name:, title:, type: :issue) + project = Project.find_by(name: project_name) + + attrs = { + title: title, + author: project.users.first, + description: '# Description header' + } + + case type + when :issue + attrs.merge!(project: project) + when :merge_request + attrs.merge!( + source_project: project, + target_project: project, + source_branch: 'fix', + target_branch: 'master' + ) + end + + create(type, attrs) + end + + def leave_reference_comment(issuable:, from_project_name:) + project = Project.find_by(name: from_project_name) + + page.within('.js-main-target-form') do + fill_in 'note[note]', with: "##{issuable.to_reference(project)}" + click_button 'Add Comment' + end + end + + def visible_note(issuable:, from_project_name:, user_name:) + project = Project.find_by(name: from_project_name) + + expect(page).to have_content(user_name) + expect(page).to have_content("mentioned in #{issuable.class.to_s.titleize.downcase} #{issuable.to_reference(project)}") + end + end diff --git a/features/steps/shared/note.rb b/features/steps/shared/note.rb index 6de58c6e2b..444d6726f9 100644 --- a/features/steps/shared/note.rb +++ b/features/steps/shared/note.rb @@ -107,9 +107,7 @@ module SharedNote end step 'I should see no notes at all' do - page.within('.notes') do - expect(page).to_not have_css('.note') - end + expect(page).to_not have_css('.note') end # Markdown diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index 43a15f4347..5420c45151 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -161,31 +161,33 @@ module SharedProject end step '"John Doe" owns private project "Enterprise"' do - user = user_exists("John Doe", username: "john_doe") - project = Project.find_by(name: "Enterprise") - project ||= create(:empty_project, name: "Enterprise", namespace: user.namespace) - project.team << [user, :master] + user_owns_project( + user_name: 'John Doe', + project_name: 'Enterprise' + ) + end + + step '"Mary Jane" owns private project "Enterprise"' do + user_owns_project( + user_name: 'Mary Jane', + project_name: 'Enterprise' + ) end step '"John Doe" owns internal project "Internal"' do - user = user_exists("John Doe", username: "john_doe") - project = Project.find_by(name: "Internal") - project ||= create :empty_project, :internal, name: 'Internal', namespace: user.namespace - project.team << [user, :master] + user_owns_project( + user_name: 'John Doe', + project_name: 'Internal', + visibility: :internal + ) end step '"John Doe" owns public project "Community"' do - user = user_exists("John Doe", username: "john_doe") - project = Project.find_by(name: "Community") - project ||= create :empty_project, :public, name: 'Community', namespace: user.namespace - project.team << [user, :master] - end - - step '"Mary Jane" owns private project "Private Library"' do - user = user_exists('Mary Jane', username: 'mary_jane') - project = Project.find_by(name: 'Private Library') - project ||= create(:project, name: 'Private Library', namespace: user.namespace) - project.team << [user, :master] + user_owns_project( + user_name: 'John Doe', + project_name: 'Community', + visibility: :public + ) end step 'public empty project "Empty Public Project"' do @@ -220,4 +222,12 @@ module SharedProject expect(page).to have_content("skipped") end end + + def user_owns_project(user_name:, project_name:, visibility: :private) + user = user_exists(user_name, username: user_name.underscore) + project = Project.find_by(name: project_name) + project ||= create(:empty_project, visibility, name: project_name, namespace: user.namespace) + project.team << [user, :master] + end + end From 79361b207ed70e1b954ca85c97a86e459e3e3976 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 31 Dec 2015 14:49:02 -0500 Subject: [PATCH 11/32] Add js-requires-input to form --- app/views/admin/broadcast_messages/index.html.haml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/views/admin/broadcast_messages/index.html.haml b/app/views/admin/broadcast_messages/index.html.haml index 17dffebd36..5d8250f787 100644 --- a/app/views/admin/broadcast_messages/index.html.haml +++ b/app/views/admin/broadcast_messages/index.html.haml @@ -7,7 +7,7 @@ %i.fa.fa-bullhorn %span Your message here -= form_for [:admin, @broadcast_message], html: { class: 'broadcast-message-form form-horizontal'} do |f| += form_for [:admin, @broadcast_message], html: { class: 'broadcast-message-form form-horizontal js-requires-input'} do |f| -if @broadcast_message.errors.any? .alert.alert-danger - @broadcast_message.errors.full_messages.each do |msg| @@ -17,8 +17,7 @@ .col-sm-10 = f.text_area :message, class: "form-control", rows: 2, required: true %div - = link_to '#', class: 'js-toggle-colors-link' do - Customize colors + = link_to 'Customize colors', '#', class: 'js-toggle-colors-link' .form-group.js-toggle-colors-container.hide = f.label :color, "Background Color", class: 'control-label' .col-sm-10 From 1c7febc3e38053e8d01cf7a30d3da23b95f30808 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 31 Dec 2015 15:08:42 -0500 Subject: [PATCH 12/32] Style the broadcast message form --- app/assets/stylesheets/framework/forms.scss | 4 ++++ app/views/admin/broadcast_messages/index.html.haml | 13 ++++++++----- app/views/layouts/_broadcast.html.haml | 2 +- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/assets/stylesheets/framework/forms.scss b/app/assets/stylesheets/framework/forms.scss index 032d343df4..d0cb91a5b6 100644 --- a/app/assets/stylesheets/framework/forms.scss +++ b/app/assets/stylesheets/framework/forms.scss @@ -78,6 +78,10 @@ label { padding: 8px $gl-padding; } +.form-control-inline { + display: inline; +} + .wiki-content { margin-top: 35px; } diff --git a/app/views/admin/broadcast_messages/index.html.haml b/app/views/admin/broadcast_messages/index.html.haml index 5d8250f787..6e8122a513 100644 --- a/app/views/admin/broadcast_messages/index.html.haml +++ b/app/views/admin/broadcast_messages/index.html.haml @@ -1,10 +1,13 @@ - page_title "Broadcast Messages" + %h3.page-title Broadcast Messages %p.light - Broadcast messages are displayed for every user and can be used to notify users about scheduled maintenance, recent upgrades and more. + Broadcast messages are displayed for every user and can be used to notify + users about scheduled maintenance, recent upgrades and more. + .broadcast-message-preview - %i.fa.fa-bullhorn + = icon('bullhorn') %span Your message here = form_for [:admin, @broadcast_message], html: { class: 'broadcast-message-form form-horizontal js-requires-input'} do |f| @@ -21,7 +24,7 @@ .form-group.js-toggle-colors-container.hide = f.label :color, "Background Color", class: 'control-label' .col-sm-10 - = f.color_field :color, value: "#eb9532", class: "form-control" + = f.color_field :color, value: "#E75E40", class: "form-control" .form-group.js-toggle-colors-container.hide = f.label :font, "Font Color", class: 'control-label' .col-sm-10 @@ -29,11 +32,11 @@ .form-group = f.label :starts_at, class: 'control-label' .col-sm-10.datetime-controls - = f.datetime_select :starts_at + = f.datetime_select :starts_at, {}, class: 'form-control form-control-inline' .form-group = f.label :ends_at, class: 'control-label' .col-sm-10.datetime-controls - = f.datetime_select :ends_at + = f.datetime_select :ends_at, {}, class: 'form-control form-control-inline' .form-actions = f.submit "Add broadcast message", class: "btn btn-create" diff --git a/app/views/layouts/_broadcast.html.haml b/app/views/layouts/_broadcast.html.haml index e7d477c225..ad7851c0aa 100644 --- a/app/views/layouts/_broadcast.html.haml +++ b/app/views/layouts/_broadcast.html.haml @@ -1,4 +1,4 @@ - if broadcast_message.present? .broadcast-message{ style: broadcast_styling(broadcast_message) } - %i.fa.fa-bullhorn + = icon('bullhorn') = broadcast_message.message From 7e24c5c45afe1ada044a782f27d3f3413043b1e6 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 31 Dec 2015 15:19:13 -0500 Subject: [PATCH 13/32] Move broadcast message form to a partial --- app/assets/javascripts/admin.js.coffee | 3 +- .../admin/broadcast_messages/_form.html.haml | 37 +++++++++++++++++++ .../admin/broadcast_messages/index.html.haml | 33 +---------------- 3 files changed, 39 insertions(+), 34 deletions(-) create mode 100644 app/views/admin/broadcast_messages/_form.html.haml diff --git a/app/assets/javascripts/admin.js.coffee b/app/assets/javascripts/admin.js.coffee index bcb2e6df7c..e9a26dd720 100644 --- a/app/assets/javascripts/admin.js.coffee +++ b/app/assets/javascripts/admin.js.coffee @@ -10,8 +10,7 @@ class @Admin $('body').on 'click', '.js-toggle-colors-link', (e) -> e.preventDefault() - $('.js-toggle-colors-link').hide() - $('.js-toggle-colors-container').show() + $('.js-toggle-colors-container').toggle() $('input#broadcast_message_color').on 'input', -> previewColor = $('input#broadcast_message_color').val() diff --git a/app/views/admin/broadcast_messages/_form.html.haml b/app/views/admin/broadcast_messages/_form.html.haml new file mode 100644 index 0000000000..0ca3be0bd5 --- /dev/null +++ b/app/views/admin/broadcast_messages/_form.html.haml @@ -0,0 +1,37 @@ +.broadcast-message-preview + = icon('bullhorn') + %span Your message here + += form_for [:admin, @broadcast_message], html: { class: 'broadcast-message-form form-horizontal js-requires-input'} do |f| + -if @broadcast_message.errors.any? + .alert.alert-danger + - @broadcast_message.errors.full_messages.each do |msg| + %p= msg + .form-group + = f.label :message, class: 'control-label' + .col-sm-10 + = f.text_area :message, class: "form-control", rows: 2, required: true + .form-group.js-toggle-colors-container + .col-sm-10.col-sm-offset-2 + = link_to 'Customize colors', '#', class: 'js-toggle-colors-link' + .form-group.js-toggle-colors-container.hide + = f.label :color, "Background Color", class: 'control-label' + .col-sm-10 + = f.color_field :color, value: "#E75E40", class: "form-control" + .form-group.js-toggle-colors-container.hide + = f.label :font, "Font Color", class: 'control-label' + .col-sm-10 + = f.color_field :font, value: "#FFFFFF", class: "form-control" + .form-group + = f.label :starts_at, class: 'control-label' + .col-sm-10.datetime-controls + = f.datetime_select :starts_at, {}, class: 'form-control form-control-inline' + .form-group + = f.label :ends_at, class: 'control-label' + .col-sm-10.datetime-controls + = f.datetime_select :ends_at, {}, class: 'form-control form-control-inline' + .form-actions + - if @broadcast_message.persisted? + = f.submit "Update broadcast message", class: "btn btn-create" + - else + = f.submit "Add broadcast message", class: "btn btn-create" diff --git a/app/views/admin/broadcast_messages/index.html.haml b/app/views/admin/broadcast_messages/index.html.haml index 6e8122a513..04864d4afe 100644 --- a/app/views/admin/broadcast_messages/index.html.haml +++ b/app/views/admin/broadcast_messages/index.html.haml @@ -6,39 +6,8 @@ Broadcast messages are displayed for every user and can be used to notify users about scheduled maintenance, recent upgrades and more. -.broadcast-message-preview - = icon('bullhorn') - %span Your message here += render 'form' -= form_for [:admin, @broadcast_message], html: { class: 'broadcast-message-form form-horizontal js-requires-input'} do |f| - -if @broadcast_message.errors.any? - .alert.alert-danger - - @broadcast_message.errors.full_messages.each do |msg| - %p= msg - .form-group - = f.label :message, class: 'control-label' - .col-sm-10 - = f.text_area :message, class: "form-control", rows: 2, required: true - %div - = link_to 'Customize colors', '#', class: 'js-toggle-colors-link' - .form-group.js-toggle-colors-container.hide - = f.label :color, "Background Color", class: 'control-label' - .col-sm-10 - = f.color_field :color, value: "#E75E40", class: "form-control" - .form-group.js-toggle-colors-container.hide - = f.label :font, "Font Color", class: 'control-label' - .col-sm-10 - = f.color_field :font, value: "#FFFFFF", class: "form-control" - .form-group - = f.label :starts_at, class: 'control-label' - .col-sm-10.datetime-controls - = f.datetime_select :starts_at, {}, class: 'form-control form-control-inline' - .form-group - = f.label :ends_at, class: 'control-label' - .col-sm-10.datetime-controls - = f.datetime_select :ends_at, {}, class: 'form-control form-control-inline' - .form-actions - = f.submit "Add broadcast message", class: "btn btn-create" -if @broadcast_messages.any? %ul.bordered-list.broadcast-messages From 6ae39c2cd1edcc845136739d42baf032120e3ddc Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 31 Dec 2015 15:56:15 -0500 Subject: [PATCH 14/32] Remove alert_type attribute from BroadcastMessage --- .../admin/broadcast_messages_controller.rb | 11 +++++++---- app/models/broadcast_message.rb | 1 - ...02530_remove_alert_type_from_broadcast_messages.rb | 5 +++++ db/schema.rb | 1 - spec/models/broadcast_message_spec.rb | 1 - 5 files changed, 12 insertions(+), 7 deletions(-) create mode 100644 db/migrate/20151231202530_remove_alert_type_from_broadcast_messages.rb diff --git a/app/controllers/admin/broadcast_messages_controller.rb b/app/controllers/admin/broadcast_messages_controller.rb index 497c34f8f4..bc702c022b 100644 --- a/app/controllers/admin/broadcast_messages_controller.rb +++ b/app/controllers/admin/broadcast_messages_controller.rb @@ -31,9 +31,12 @@ class Admin::BroadcastMessagesController < Admin::ApplicationController end def broadcast_message_params - params.require(:broadcast_message).permit( - :alert_type, :color, :ends_at, :font, - :message, :starts_at - ) + params.require(:broadcast_message).permit(%i( + color + ends_at + font + message + starts_at + )) end end diff --git a/app/models/broadcast_message.rb b/app/models/broadcast_message.rb index ad51470616..da0d8a2d65 100644 --- a/app/models/broadcast_message.rb +++ b/app/models/broadcast_message.rb @@ -6,7 +6,6 @@ # message :text not null # starts_at :datetime # ends_at :datetime -# alert_type :integer # created_at :datetime # updated_at :datetime # color :string(255) diff --git a/db/migrate/20151231202530_remove_alert_type_from_broadcast_messages.rb b/db/migrate/20151231202530_remove_alert_type_from_broadcast_messages.rb new file mode 100644 index 0000000000..78fdfeaf5c --- /dev/null +++ b/db/migrate/20151231202530_remove_alert_type_from_broadcast_messages.rb @@ -0,0 +1,5 @@ +class RemoveAlertTypeFromBroadcastMessages < ActiveRecord::Migration + def change + remove_column :broadcast_messages, :alert_type, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index ecbe575bf8..42c3e79f9d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -82,7 +82,6 @@ ActiveRecord::Schema.define(version: 20160113111034) do t.text "message", null: false t.datetime "starts_at" t.datetime "ends_at" - t.integer "alert_type" t.datetime "created_at" t.datetime "updated_at" t.string "color" diff --git a/spec/models/broadcast_message_spec.rb b/spec/models/broadcast_message_spec.rb index e4cac10511..001b2facaa 100644 --- a/spec/models/broadcast_message_spec.rb +++ b/spec/models/broadcast_message_spec.rb @@ -6,7 +6,6 @@ # message :text not null # starts_at :datetime # ends_at :datetime -# alert_type :integer # created_at :datetime # updated_at :datetime # color :string(255) From 12903762d15cef256f579b1ddb692a0254cace52 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 31 Dec 2015 15:56:59 -0500 Subject: [PATCH 15/32] Simplify broadcast message JS Also resets the default example message when the user input is blank. --- app/assets/javascripts/admin.js.coffee | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/admin.js.coffee b/app/assets/javascripts/admin.js.coffee index e9a26dd720..eb951f7171 100644 --- a/app/assets/javascripts/admin.js.coffee +++ b/app/assets/javascripts/admin.js.coffee @@ -13,15 +13,16 @@ class @Admin $('.js-toggle-colors-container').toggle() $('input#broadcast_message_color').on 'input', -> - previewColor = $('input#broadcast_message_color').val() + previewColor = $(@).val() $('div.broadcast-message-preview').css('background-color', previewColor) $('input#broadcast_message_font').on 'input', -> - previewColor = $('input#broadcast_message_font').val() + previewColor = $(@).val() $('div.broadcast-message-preview').css('color', previewColor) $('textarea#broadcast_message_message').on 'input', -> - previewMessage = $('textarea#broadcast_message_message').val() + previewMessage = $(@).val() + previewMessage = "Your message here" if previewMessage.trim() == '' $('div.broadcast-message-preview span').text(previewMessage) $('.log-tabs a').click (e) -> From 8086b2bd2ecb0ff647da766c436eda47b7434599 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 31 Dec 2015 16:04:07 -0500 Subject: [PATCH 16/32] Simplify BroadcastMessage factory Also make the feature tests less brittle. --- features/admin/broadcast_messages.feature | 2 +- features/steps/admin/broadcast_messages.rb | 7 +++---- spec/factories/broadcast_messages.rb | 13 +++++++------ 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/features/admin/broadcast_messages.feature b/features/admin/broadcast_messages.feature index b2c3112320..98d894a4fb 100644 --- a/features/admin/broadcast_messages.feature +++ b/features/admin/broadcast_messages.feature @@ -6,7 +6,7 @@ Feature: Admin Broadcast Messages And I visit admin messages page Scenario: See broadcast messages list - Then I should be all broadcast messages + Then I should see all broadcast messages Scenario: Create a broadcast message When submit form with new broadcast message diff --git a/features/steps/admin/broadcast_messages.rb b/features/steps/admin/broadcast_messages.rb index f6daf85297..b59799c2f4 100644 --- a/features/steps/admin/broadcast_messages.rb +++ b/features/steps/admin/broadcast_messages.rb @@ -4,10 +4,10 @@ class Spinach::Features::AdminBroadcastMessages < Spinach::FeatureSteps include SharedAdmin step 'application already has admin messages' do - FactoryGirl.create(:broadcast_message, message: "Migration to new server") + FactoryGirl.create(:broadcast_message, :expired, message: "Migration to new server") end - step 'I should be all broadcast messages' do + step 'I should see all broadcast messages' do expect(page).to have_content "Migration to new server" end @@ -27,10 +27,9 @@ class Spinach::Features::AdminBroadcastMessages < Spinach::FeatureSteps step 'submit form with new customized broadcast message' do fill_in 'broadcast_message_message', with: 'Application update from 4:00 CST to 5:00 CST' - click_link "Customize colors" fill_in 'broadcast_message_color', with: '#f2dede' fill_in 'broadcast_message_font', with: '#b94a48' - select '2018', from: "broadcast_message_ends_at_1i" + select Date.today.next_year.year, from: "broadcast_message_ends_at_1i" click_button "Add broadcast message" end diff --git a/spec/factories/broadcast_messages.rb b/spec/factories/broadcast_messages.rb index ea0039d39e..0c7bc57361 100644 --- a/spec/factories/broadcast_messages.rb +++ b/spec/factories/broadcast_messages.rb @@ -6,7 +6,6 @@ # message :text not null # starts_at :datetime # ends_at :datetime -# alert_type :integer # created_at :datetime # updated_at :datetime # color :string(255) @@ -18,10 +17,12 @@ FactoryGirl.define do factory :broadcast_message do message "MyText" - starts_at "2013-11-12 13:43:25" - ends_at "2013-11-12 13:43:25" - alert_type 1 - color "#555555" - font "#BBBBBB" + starts_at Date.today + ends_at Date.tomorrow + + trait :expired do + starts_at 5.days.ago + ends_at 3.days.ago + end end end From 5a1706791e46fd69a67fcdebfc384a702b00a7ff Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 31 Dec 2015 16:42:56 -0500 Subject: [PATCH 17/32] Update broadcast_message helper Now it returns the fully-formatted message so we can be consistent about how it's shown. --- app/helpers/application_helper.rb | 4 -- app/helpers/broadcast_messages_helper.rb | 20 ++++++--- app/views/layouts/_broadcast.html.haml | 5 +-- .../helpers/broadcast_messages_helper_spec.rb | 44 +++++++++++++------ 4 files changed, 46 insertions(+), 27 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 436fbcd413..f3a2723ee0 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -181,10 +181,6 @@ module ApplicationHelper end end - def broadcast_message - BroadcastMessage.current - end - # Render a `time` element with Javascript-based relative date and tooltip # # time - Time object diff --git a/app/helpers/broadcast_messages_helper.rb b/app/helpers/broadcast_messages_helper.rb index 6484dca6b5..44bb09b74f 100644 --- a/app/helpers/broadcast_messages_helper.rb +++ b/app/helpers/broadcast_messages_helper.rb @@ -1,16 +1,24 @@ module BroadcastMessagesHelper - def broadcast_styling(broadcast_message) - styling = '' + def broadcast_message(message = BroadcastMessage.current) + return unless message.present? + + content_tag :div, class: 'broadcast-message', style: broadcast_message_style(message) do + icon('bullhorn') << ' ' << message.message + end + end + + def broadcast_message_style(broadcast_message) + style = '' if broadcast_message.color.present? - styling << "background-color: #{broadcast_message.color}" - styling << '; ' if broadcast_message.font.present? + style << "background-color: #{broadcast_message.color}" + style << '; ' if broadcast_message.font.present? end if broadcast_message.font.present? - styling << "color: #{broadcast_message.font}" + style << "color: #{broadcast_message.font}" end - styling + style end end diff --git a/app/views/layouts/_broadcast.html.haml b/app/views/layouts/_broadcast.html.haml index ad7851c0aa..3a7e0929c1 100644 --- a/app/views/layouts/_broadcast.html.haml +++ b/app/views/layouts/_broadcast.html.haml @@ -1,4 +1 @@ -- if broadcast_message.present? - .broadcast-message{ style: broadcast_styling(broadcast_message) } - = icon('bullhorn') - = broadcast_message.message += broadcast_message diff --git a/spec/helpers/broadcast_messages_helper_spec.rb b/spec/helpers/broadcast_messages_helper_spec.rb index c7c6f45d14..0fb8a7284f 100644 --- a/spec/helpers/broadcast_messages_helper_spec.rb +++ b/spec/helpers/broadcast_messages_helper_spec.rb @@ -1,22 +1,40 @@ require 'spec_helper' describe BroadcastMessagesHelper do - describe 'broadcast_styling' do - let(:broadcast_message) { double(color: '', font: '') } - - context "default style" do - it "should have no style" do - expect(broadcast_styling(broadcast_message)).to eq '' - end + describe 'broadcast_message' do + it 'returns nil when no current message' do + expect(helper.broadcast_message(nil)).to be_nil end - context "customized style" do - let(:broadcast_message) { double(color: "#f2dede", font: '#b94a48') } + it 'includes the current message' do + current = double(message: 'Current Message') - it "should have a customized style" do - expect(broadcast_styling(broadcast_message)). - to match('background-color: #f2dede; color: #b94a48') - end + allow(helper).to receive(:broadcast_message_style).and_return(nil) + + expect(helper.broadcast_message(current)).to include 'Current Message' + end + + it 'includes custom style' do + current = double(message: 'Current Message') + + allow(helper).to receive(:broadcast_message_style).and_return('foo') + + expect(helper.broadcast_message(current)).to include 'style="foo"' + end + end + + describe 'broadcast_message_style' do + it 'defaults to no style' do + broadcast_message = spy + + expect(helper.broadcast_message_style(broadcast_message)).to eq '' + end + + it 'allows custom style' do + broadcast_message = double(color: '#f2dede', font: '#b94a48') + + expect(helper.broadcast_message_style(broadcast_message)). + to match('background-color: #f2dede; color: #b94a48') end end end From df496fcaf08b085816a45d31d02f1ea230454b63 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 31 Dec 2015 17:07:11 -0500 Subject: [PATCH 18/32] Update BroadcastMessage model - Adds default values for `color` and `font` attributes - Adds `active?`, `started?`, `ended?`, and 'status' methods --- app/models/broadcast_message.rb | 27 +++++++- spec/factories/broadcast_messages.rb | 5 ++ spec/models/broadcast_message_spec.rb | 91 +++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 6 deletions(-) diff --git a/app/models/broadcast_message.rb b/app/models/broadcast_message.rb index da0d8a2d65..188545f9ae 100644 --- a/app/models/broadcast_message.rb +++ b/app/models/broadcast_message.rb @@ -22,7 +22,32 @@ class BroadcastMessage < ActiveRecord::Base validates :color, allow_blank: true, color: true validates :font, allow_blank: true, color: true + default_value_for :color, '#E75E40' + default_value_for :font, '#FFFFFF' + def self.current - where("ends_at > :now AND starts_at < :now", now: Time.zone.now).last + where("ends_at > :now AND starts_at <= :now", now: Time.zone.now).last + end + + def active? + started? && !ended? + end + + def started? + Time.zone.now >= starts_at + end + + def ended? + ends_at < Time.zone.now + end + + def status + if active? + 'Active' + elsif ended? + 'Expired' + else + 'Pending' + end end end diff --git a/spec/factories/broadcast_messages.rb b/spec/factories/broadcast_messages.rb index 0c7bc57361..978a7d4cec 100644 --- a/spec/factories/broadcast_messages.rb +++ b/spec/factories/broadcast_messages.rb @@ -24,5 +24,10 @@ FactoryGirl.define do starts_at 5.days.ago ends_at 3.days.ago end + + trait :future do + starts_at 5.days.from_now + ends_at 6.days.from_now + end end end diff --git a/spec/models/broadcast_message_spec.rb b/spec/models/broadcast_message_spec.rb index 001b2facaa..57550725ae 100644 --- a/spec/models/broadcast_message_spec.rb +++ b/spec/models/broadcast_message_spec.rb @@ -15,6 +15,8 @@ require 'spec_helper' describe BroadcastMessage, models: true do + include ActiveSupport::Testing::TimeHelpers + subject { create(:broadcast_message) } it { is_expected.to be_valid } @@ -34,20 +36,99 @@ describe BroadcastMessage, models: true do it { is_expected.not_to allow_value('000').for(:font) } end - describe :current do + describe '.current' do it "should return last message if time match" do - broadcast_message = create(:broadcast_message, starts_at: Time.now.yesterday, ends_at: Time.now.tomorrow) - expect(BroadcastMessage.current).to eq(broadcast_message) + message = create(:broadcast_message) + + expect(BroadcastMessage.current).to eq message end it "should return nil if time not come" do - create(:broadcast_message, starts_at: Time.now.tomorrow, ends_at: Time.now + 2.days) + create(:broadcast_message, :future) + expect(BroadcastMessage.current).to be_nil end it "should return nil if time has passed" do - create(:broadcast_message, starts_at: Time.now - 2.days, ends_at: Time.now.yesterday) + create(:broadcast_message, :expired) + expect(BroadcastMessage.current).to be_nil end end + + describe '#active?' do + it 'is truthy when started and not ended' do + message = build(:broadcast_message) + + expect(message).to be_active + end + + it 'is falsey when ended' do + message = build(:broadcast_message, :expired) + + expect(message).not_to be_active + end + + it 'is falsey when not started' do + message = build(:broadcast_message, :future) + + expect(message).not_to be_active + end + end + + describe '#started?' do + it 'is truthy when starts_at has passed' do + message = build(:broadcast_message) + + travel_to(3.days.from_now) do + expect(message).to be_started + end + end + + it 'is falsey when starts_at is in the future' do + message = build(:broadcast_message) + + travel_to(3.days.ago) do + expect(message).not_to be_started + end + end + end + + describe '#ended?' do + it 'is truthy when ends_at has passed' do + message = build(:broadcast_message) + + travel_to(3.days.from_now) do + expect(message).to be_ended + end + end + + it 'is falsey when ends_at is in the future' do + message = build(:broadcast_message) + + travel_to(3.days.ago) do + expect(message).not_to be_ended + end + end + end + + describe '#status' do + it 'returns Active' do + message = build(:broadcast_message) + + expect(message.status).to eq 'Active' + end + + it 'returns Expired' do + message = build(:broadcast_message, :expired) + + expect(message.status).to eq 'Expired' + end + + it 'returns Pending' do + message = build(:broadcast_message, :future) + + expect(message.status).to eq 'Pending' + end + end end From 540ae3c3658d051f852b2c10fa61c557521196e1 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 31 Dec 2015 17:22:40 -0500 Subject: [PATCH 19/32] Update Broadcast Message features - Removes redundant "Create a broadcast message" scenario that was entirely covered by the "Create a customized broadcast message" scenario. - Adds "Edit an existing broadcast message" scenario - Adds "Remove an existing broadcast message" scenario --- features/admin/broadcast_messages.feature | 18 ++++++++----- features/steps/admin/broadcast_messages.rb | 30 ++++++++++++++++------ 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/features/admin/broadcast_messages.feature b/features/admin/broadcast_messages.feature index 98d894a4fb..fd3bac77f8 100644 --- a/features/admin/broadcast_messages.feature +++ b/features/admin/broadcast_messages.feature @@ -2,20 +2,26 @@ Feature: Admin Broadcast Messages Background: Given I sign in as an admin - And application already has admin messages + And application already has a broadcast message And I visit admin messages page Scenario: See broadcast messages list Then I should see all broadcast messages - Scenario: Create a broadcast message - When submit form with new broadcast message - Then I should be redirected to admin messages page - And I should see newly created broadcast message - Scenario: Create a customized broadcast message When submit form with new customized broadcast message Then I should be redirected to admin messages page And I should see newly created broadcast message Then I visit dashboard page And I should see a customized broadcast message + + Scenario: Edit an existing broadcast message + When I edit an existing broadcast message + And I change the broadcast message text + Then I should be redirected to admin messages page + And I should see the updated broadcast message + + Scenario: Remove an existing broadcast message + When I remove an existing broadcast message + Then I should be redirected to admin messages page + And I should not see the removed broadcast message diff --git a/features/steps/admin/broadcast_messages.rb b/features/steps/admin/broadcast_messages.rb index b59799c2f4..6cacdf4764 100644 --- a/features/steps/admin/broadcast_messages.rb +++ b/features/steps/admin/broadcast_messages.rb @@ -1,9 +1,8 @@ class Spinach::Features::AdminBroadcastMessages < Spinach::FeatureSteps include SharedAuthentication include SharedPaths - include SharedAdmin - step 'application already has admin messages' do + step 'application already has a broadcast message' do FactoryGirl.create(:broadcast_message, :expired, message: "Migration to new server") end @@ -11,12 +10,6 @@ class Spinach::Features::AdminBroadcastMessages < Spinach::FeatureSteps expect(page).to have_content "Migration to new server" end - step 'submit form with new broadcast message' do - fill_in 'broadcast_message_message', with: 'Application update from 4:00 CST to 5:00 CST' - select '2018', from: "broadcast_message_ends_at_1i" - click_button "Add broadcast message" - end - step 'I should be redirected to admin messages page' do expect(current_path).to eq admin_broadcast_messages_path end @@ -37,4 +30,25 @@ class Spinach::Features::AdminBroadcastMessages < Spinach::FeatureSteps expect(page).to have_content 'Application update from 4:00 CST to 5:00 CST' expect(page).to have_selector %(div[style="background-color: #f2dede; color: #b94a48"]) end + + step 'I edit an existing broadcast message' do + click_link 'Edit' + end + + step 'I change the broadcast message text' do + fill_in 'broadcast_message_message', with: 'Application update RIGHT NOW' + click_button 'Update broadcast message' + end + + step 'I should see the updated broadcast message' do + expect(page).to have_content "Application update RIGHT NOW" + end + + step 'I remove an existing broadcast message' do + click_link 'Remove' + end + + step 'I should not see the removed broadcast message' do + expect(page).not_to have_content 'Migration to new server' + end end From 00e8700433b3b1ad11252448af5be58913539d85 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 31 Dec 2015 17:55:14 -0500 Subject: [PATCH 20/32] Broadcast Messages can now be edited Closes #3046 --- .../admin/broadcast_messages_controller.rb | 22 +++++++--- .../admin/broadcast_messages/_form.html.haml | 10 ++--- .../admin/broadcast_messages/edit.html.haml | 3 ++ .../admin/broadcast_messages/index.html.haml | 40 +++++++++++-------- config/routes.rb | 2 +- 5 files changed, 49 insertions(+), 28 deletions(-) create mode 100644 app/views/admin/broadcast_messages/edit.html.haml diff --git a/app/controllers/admin/broadcast_messages_controller.rb b/app/controllers/admin/broadcast_messages_controller.rb index bc702c022b..4735b27c65 100644 --- a/app/controllers/admin/broadcast_messages_controller.rb +++ b/app/controllers/admin/broadcast_messages_controller.rb @@ -1,8 +1,12 @@ class Admin::BroadcastMessagesController < Admin::ApplicationController - before_action :broadcast_messages + before_action :finder, only: [:edit, :update, :destroy] def index - @broadcast_message = BroadcastMessage.new + @broadcast_messages = BroadcastMessage.reorder("starts_at ASC").page(params[:page]) + @broadcast_message = BroadcastMessage.new + end + + def edit end def create @@ -15,8 +19,16 @@ class Admin::BroadcastMessagesController < Admin::ApplicationController end end + def update + if @broadcast_message.update(broadcast_message_params) + redirect_to admin_broadcast_messages_path, notice: 'Broadcast Message was successfully updated.' + else + render :edit + end + end + def destroy - BroadcastMessage.find(params[:id]).destroy + @broadcast_message.destroy respond_to do |format| format.html { redirect_back_or_default(default: { action: 'index' }) } @@ -26,8 +38,8 @@ class Admin::BroadcastMessagesController < Admin::ApplicationController protected - def broadcast_messages - @broadcast_messages ||= BroadcastMessage.order("starts_at DESC").page(params[:page]) + def finder + @broadcast_message = BroadcastMessage.find(params[:id]) end def broadcast_message_params diff --git a/app/views/admin/broadcast_messages/_form.html.haml b/app/views/admin/broadcast_messages/_form.html.haml index 0ca3be0bd5..953b8b6936 100644 --- a/app/views/admin/broadcast_messages/_form.html.haml +++ b/app/views/admin/broadcast_messages/_form.html.haml @@ -1,6 +1,6 @@ -.broadcast-message-preview +.broadcast-message-preview{ style: broadcast_message_style(@broadcast_message) } = icon('bullhorn') - %span Your message here + %span= @broadcast_message.message || "Your message here" = form_for [:admin, @broadcast_message], html: { class: 'broadcast-message-form form-horizontal js-requires-input'} do |f| -if @broadcast_message.errors.any? @@ -10,18 +10,18 @@ .form-group = f.label :message, class: 'control-label' .col-sm-10 - = f.text_area :message, class: "form-control", rows: 2, required: true + = f.text_area :message, class: "form-control js-quick-submit", rows: 2, required: true .form-group.js-toggle-colors-container .col-sm-10.col-sm-offset-2 = link_to 'Customize colors', '#', class: 'js-toggle-colors-link' .form-group.js-toggle-colors-container.hide = f.label :color, "Background Color", class: 'control-label' .col-sm-10 - = f.color_field :color, value: "#E75E40", class: "form-control" + = f.color_field :color, class: "form-control" .form-group.js-toggle-colors-container.hide = f.label :font, "Font Color", class: 'control-label' .col-sm-10 - = f.color_field :font, value: "#FFFFFF", class: "form-control" + = f.color_field :font, class: "form-control" .form-group = f.label :starts_at, class: 'control-label' .col-sm-10.datetime-controls diff --git a/app/views/admin/broadcast_messages/edit.html.haml b/app/views/admin/broadcast_messages/edit.html.haml new file mode 100644 index 0000000000..45e053eb31 --- /dev/null +++ b/app/views/admin/broadcast_messages/edit.html.haml @@ -0,0 +1,3 @@ +- page_title "Broadcast Messages" + += render 'form' diff --git a/app/views/admin/broadcast_messages/index.html.haml b/app/views/admin/broadcast_messages/index.html.haml index 04864d4afe..6c72b28ee0 100644 --- a/app/views/admin/broadcast_messages/index.html.haml +++ b/app/views/admin/broadcast_messages/index.html.haml @@ -8,24 +8,30 @@ = render 'form' +%br.clearfix -if @broadcast_messages.any? - %ul.bordered-list.broadcast-messages - - @broadcast_messages.each do |broadcast_message| - %li - .pull-right - - if broadcast_message.starts_at - %strong - #{broadcast_message.starts_at.to_s(:short)} - \... - - if broadcast_message.ends_at - %strong - #{broadcast_message.ends_at.to_s(:short)} -   - = link_to [:admin, broadcast_message], method: :delete, remote: true, class: 'remove-row btn btn-xs' do - %i.fa.fa-times.cred - - .message= broadcast_message.message - + %table.table + %thead + %tr + %th Status + %th Preview + %th Starts + %th Ends + %th   + %tbody + - @broadcast_messages.each do |message| + %tr + %td + = message.status + %td + = broadcast_message(message) + %td + = message.starts_at.to_s(:iso861) + %td + = message.ends_at.to_s(:iso861) + %td + = link_to icon('pencil-square-o'), edit_admin_broadcast_message_path(message), title: 'Edit', class: 'btn btn-xs' + = link_to icon('times'), admin_broadcast_message_path(message), method: :delete, remote: true, title: 'Remove', class: 'js-remove-tr btn btn-xs btn-danger' = paginate @broadcast_messages diff --git a/config/routes.rb b/config/routes.rb index 3d5c70987c..05d6ff1e88 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -219,7 +219,7 @@ Rails.application.routes.draw do get :test end - resources :broadcast_messages, only: [:index, :create, :destroy] + resources :broadcast_messages, only: [:index, :edit, :create, :update, :destroy] resource :logs, only: [:show] resource :background_jobs, controller: 'background_jobs', only: [:show] From c13b5acb16c7813d6913e26cc7ae67f691f914d6 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 31 Dec 2015 20:04:55 -0500 Subject: [PATCH 21/32] Update CHANGELOG [ci skip] --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 4221fb9df5..94c764eb6b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -43,6 +43,7 @@ v 8.4.0 (unreleased) - API: Add support for deleting a tag via the API (Robert Schilling) - Allow subsequent validations in CI Linter - Fix Encoding::CompatibilityError bug when markdown content has some complex URL (Jason Lee) + - Allow broadcast messages to be edited v 8.3.4 - Use gitlab-workhorse 0.5.4 (fixes API routing bug) From 843662821ddbf2d06aa2da72ce32717cebecb7c6 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 13 Jan 2016 11:46:32 -0500 Subject: [PATCH 22/32] Move `BroadcastMessage#status` to a helper since it's presentational --- app/helpers/broadcast_messages_helper.rb | 10 ++++++++++ app/models/broadcast_message.rb | 10 ---------- .../admin/broadcast_messages/index.html.haml | 2 +- .../helpers/broadcast_messages_helper_spec.rb | 20 +++++++++++++++++++ spec/models/broadcast_message_spec.rb | 20 ------------------- 5 files changed, 31 insertions(+), 31 deletions(-) diff --git a/app/helpers/broadcast_messages_helper.rb b/app/helpers/broadcast_messages_helper.rb index 44bb09b74f..1ed8c710f7 100644 --- a/app/helpers/broadcast_messages_helper.rb +++ b/app/helpers/broadcast_messages_helper.rb @@ -21,4 +21,14 @@ module BroadcastMessagesHelper style end + + def broadcast_message_status(broadcast_message) + if broadcast_message.active? + 'Active' + elsif broadcast_message.ended? + 'Expired' + else + 'Pending' + end + end end diff --git a/app/models/broadcast_message.rb b/app/models/broadcast_message.rb index 188545f9ae..6111963371 100644 --- a/app/models/broadcast_message.rb +++ b/app/models/broadcast_message.rb @@ -40,14 +40,4 @@ class BroadcastMessage < ActiveRecord::Base def ended? ends_at < Time.zone.now end - - def status - if active? - 'Active' - elsif ended? - 'Expired' - else - 'Pending' - end - end end diff --git a/app/views/admin/broadcast_messages/index.html.haml b/app/views/admin/broadcast_messages/index.html.haml index 6c72b28ee0..3c4ef4fe42 100644 --- a/app/views/admin/broadcast_messages/index.html.haml +++ b/app/views/admin/broadcast_messages/index.html.haml @@ -23,7 +23,7 @@ - @broadcast_messages.each do |message| %tr %td - = message.status + = broadcast_message_status(message) %td = broadcast_message(message) %td diff --git a/spec/helpers/broadcast_messages_helper_spec.rb b/spec/helpers/broadcast_messages_helper_spec.rb index 0fb8a7284f..157cc4665a 100644 --- a/spec/helpers/broadcast_messages_helper_spec.rb +++ b/spec/helpers/broadcast_messages_helper_spec.rb @@ -37,4 +37,24 @@ describe BroadcastMessagesHelper do to match('background-color: #f2dede; color: #b94a48') end end + + describe 'broadcast_message_status' do + it 'returns Active' do + message = build(:broadcast_message) + + expect(helper.broadcast_message_status(message)).to eq 'Active' + end + + it 'returns Expired' do + message = build(:broadcast_message, :expired) + + expect(helper.broadcast_message_status(message)).to eq 'Expired' + end + + it 'returns Pending' do + message = build(:broadcast_message, :future) + + expect(helper.broadcast_message_status(message)).to eq 'Pending' + end + end end diff --git a/spec/models/broadcast_message_spec.rb b/spec/models/broadcast_message_spec.rb index 57550725ae..f6f84db57e 100644 --- a/spec/models/broadcast_message_spec.rb +++ b/spec/models/broadcast_message_spec.rb @@ -111,24 +111,4 @@ describe BroadcastMessage, models: true do end end end - - describe '#status' do - it 'returns Active' do - message = build(:broadcast_message) - - expect(message.status).to eq 'Active' - end - - it 'returns Expired' do - message = build(:broadcast_message, :expired) - - expect(message.status).to eq 'Expired' - end - - it 'returns Pending' do - message = build(:broadcast_message, :future) - - expect(message.status).to eq 'Pending' - end - end end From 55d0323961e4db8d29450f35b169029d51e30bda Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 13 Jan 2016 12:01:25 -0500 Subject: [PATCH 23/32] Remove (invalid) timestamp formatting --- app/views/admin/broadcast_messages/index.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/admin/broadcast_messages/index.html.haml b/app/views/admin/broadcast_messages/index.html.haml index 3c4ef4fe42..49e33698b6 100644 --- a/app/views/admin/broadcast_messages/index.html.haml +++ b/app/views/admin/broadcast_messages/index.html.haml @@ -27,9 +27,9 @@ %td = broadcast_message(message) %td - = message.starts_at.to_s(:iso861) + = message.starts_at %td - = message.ends_at.to_s(:iso861) + = message.ends_at %td = link_to icon('pencil-square-o'), edit_admin_broadcast_message_path(message), title: 'Edit', class: 'btn btn-xs' = link_to icon('times'), admin_broadcast_message_path(message), method: :delete, remote: true, title: 'Remove', class: 'js-remove-tr btn btn-xs btn-danger' From 1f0b8c32e75b446848cead98c550e750801be534 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 13 Jan 2016 18:18:59 +0100 Subject: [PATCH 24/32] Add spec for Note#cross_reference_not_visible_for? --- spec/models/note_spec.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 151a29e974..65e6a7df3b 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -178,6 +178,30 @@ describe Note, models: true do end end + describe "cross_reference_not_visible_for?" do + let(:private_user) { create(:user) } + let(:private_project) { create(:project, namespace: private_user.namespace).tap { |p| p.team << [private_user, :master] } } + let(:private_issue) { create(:issue, project: private_project) } + + let(:ext_proj) { create(:project, :public) } + let(:ext_issue) { create(:issue, project: ext_proj) } + + let(:note) { + create :note, + noteable: ext_issue, project: ext_proj, + note: "mentioned in issue #{private_issue.to_reference(ext_proj)}", + system: true + } + + it "returns true" do + expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy + end + + it "returns false" do + expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy + end + end + describe "set_award!" do let(:issue) { create :issue } From 0c10aee59677e2dadfef6538a74fe1e28fcdd37e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 13 Jan 2016 19:42:36 +0100 Subject: [PATCH 25/32] Ensure the API doesn't return notes that the current user shouldn't see --- lib/api/notes.rb | 21 ++++++++++++-- spec/requests/api/notes_spec.rb | 51 +++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 3efdfe2d46..174473f537 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -20,7 +20,19 @@ module API # GET /projects/:id/snippets/:noteable_id/notes get ":id/#{noteables_str}/:#{noteable_id_str}/notes" do @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) - present paginate(@noteable.notes), with: Entities::Note + + # We exclude notes that are cross-references and that cannot be viewed + # by the current user. By doing this exclusion at this level and not + # at the DB query level (which we cannot in that case), the current + # page can have less elements than :per_page even if + # there's more than one page. + notes = + # paginate() only works with a relation. This could lead to a + # mismatch between the pagination headers info and the actual notes + # array returned, but this is really a edge-case. + paginate(@noteable.notes). + reject { |n| n.cross_reference_not_visible_for?(current_user) } + present notes, with: Entities::Note end # Get a single +noteable+ note @@ -35,7 +47,12 @@ module API get ":id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id" do @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) @note = @noteable.notes.find(params[:note_id]) - present @note, with: Entities::Note + + if @note.cross_reference_not_visible_for?(current_user) + not_found!("Note") + else + present @note, with: Entities::Note + end end # Create a new +noteable+ note diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 8b177af468..565805d870 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -10,6 +10,24 @@ describe API::API, api: true do let!(:issue_note) { create(:note, noteable: issue, project: project, author: user) } let!(:merge_request_note) { create(:note, noteable: merge_request, project: project, author: user) } let!(:snippet_note) { create(:note, noteable: snippet, project: project, author: user) } + + # For testing the cross-reference of a private issue in a public issue + let(:private_user) { create(:user) } + let(:private_project) { + create(:project, namespace: private_user.namespace). + tap { |p| p.team << [private_user, :master] } + } + let(:private_issue) { create(:issue, project: private_project) } + let(:ext_proj) { create(:project, :public) } + let(:ext_issue) { create(:issue, project: ext_proj) } + + let!(:cross_reference_note) { + create :note, + noteable: ext_issue, project: ext_proj, + note: "mentioned in issue #{private_issue.to_reference(ext_proj)}", + system: true + } + before { project.team << [user, :reporter] } describe "GET /projects/:id/noteable/:noteable_id/notes" do @@ -25,6 +43,24 @@ describe API::API, api: true do get api("/projects/#{project.id}/issues/123/notes", user) expect(response.status).to eq(404) end + + context "that references a private issue" do + it "should return an empty array" do + get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user) + expect(response.status).to eq(200) + expect(json_response).to be_an Array + expect(json_response).to be_empty + end + + context "and current user can view the note" do + it "should return an empty array" do + get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", private_user) + expect(response.status).to eq(200) + expect(json_response).to be_an Array + expect(json_response.first['body']).to eq(cross_reference_note.note) + end + end + end end context "when noteable is a Snippet" do @@ -68,6 +104,21 @@ describe API::API, api: true do get api("/projects/#{project.id}/issues/#{issue.id}/notes/123", user) expect(response.status).to eq(404) end + + context "that references a private issue" do + it "should return a 404 error" do + get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", user) + expect(response.status).to eq(404) + end + + context "and current user can view the note" do + it "should return an issue note by id" do + get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", private_user) + expect(response.status).to eq(200) + expect(json_response['body']).to eq(cross_reference_note.note) + end + end + end end context "when noteable is a Snippet" do From 650a3f2a410660c4ed96119048903b8b91e35421 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 13 Jan 2016 16:07:20 -0500 Subject: [PATCH 26/32] Update button styles for Milestones#show --- app/views/projects/milestones/show.html.haml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/views/projects/milestones/show.html.haml b/app/views/projects/milestones/show.html.haml index 1670ea8741..d1db0f64f8 100644 --- a/app/views/projects/milestones/show.html.haml +++ b/app/views/projects/milestones/show.html.haml @@ -20,16 +20,16 @@ .pull-right - if can?(current_user, :admin_milestone, @project) - if @milestone.active? - = link_to 'Close Milestone', namespace_project_milestone_path(@project.namespace, @project, @milestone, milestone: {state_event: :close }), method: :put, class: "btn btn-close btn-grouped" + = link_to 'Close Milestone', namespace_project_milestone_path(@project.namespace, @project, @milestone, milestone: {state_event: :close }), method: :put, class: "btn btn-close btn-nr btn-grouped" - else - = link_to 'Reopen Milestone', namespace_project_milestone_path(@project.namespace, @project, @milestone, milestone: {state_event: :activate }), method: :put, class: "btn btn-reopen btn-grouped" + = link_to 'Reopen Milestone', namespace_project_milestone_path(@project.namespace, @project, @milestone, milestone: {state_event: :activate }), method: :put, class: "btn btn-reopen btn-nr btn-grouped" - = link_to namespace_project_milestone_path(@project.namespace, @project, @milestone), data: { confirm: 'Are you sure?' }, method: :delete, class: "btn btn-grouped btn-remove" do - %i.fa.fa-trash-o + = link_to namespace_project_milestone_path(@project.namespace, @project, @milestone), data: { confirm: 'Are you sure?' }, method: :delete, class: "btn btn-grouped btn-nr btn-remove" do + = icon('trash-o') Delete - = link_to edit_namespace_project_milestone_path(@project.namespace, @project, @milestone), class: "btn btn-grouped" do - %i.fa.fa-pencil-square-o + = link_to edit_namespace_project_milestone_path(@project.namespace, @project, @milestone), class: "btn btn-grouped btn-nr" do + = icon('pencil-square-o') Edit .detail-page-description.gray-content-block.second-block From da3c57080f54948caab5d12626257c91637ded92 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 13 Jan 2016 16:07:36 -0500 Subject: [PATCH 27/32] Fix misaligned edit button in milestone collection partial --- app/views/projects/milestones/_milestone.html.haml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/views/projects/milestones/_milestone.html.haml b/app/views/projects/milestones/_milestone.html.haml index d6a44c9f0a..67d95ab036 100644 --- a/app/views/projects/milestones/_milestone.html.haml +++ b/app/views/projects/milestones/_milestone.html.haml @@ -21,10 +21,11 @@ = render 'shared/milestone_expired', milestone: milestone .col-sm-6 - if can?(current_user, :admin_milestone, milestone.project) and milestone.active? - = link_to edit_namespace_project_milestone_path(milestone.project.namespace, milestone.project, milestone), class: "btn btn-xs edit-milestone-link btn-grouped" do - %i.fa.fa-pencil-square-o + = link_to edit_namespace_project_milestone_path(milestone.project.namespace, milestone.project, milestone), class: "btn btn-xs" do + = icon('pencil-square-o') Edit + \ = link_to 'Close Milestone', namespace_project_milestone_path(@project.namespace, @project, milestone, milestone: {state_event: :close }), method: :put, remote: true, class: "btn btn-xs btn-close" = link_to namespace_project_milestone_path(milestone.project.namespace, milestone.project, milestone), data: { confirm: 'Are you sure?' }, method: :delete, class: "btn btn-xs btn-remove" do - %i.fa.fa-trash-o + = icon('trash-o') Delete From d11ca78cdcd761f7f9b6388474765ede51011085 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Wed, 13 Jan 2016 17:01:05 -0800 Subject: [PATCH 28/32] Fix the undefinded variable error in Project's safe_import_url method --- app/models/project.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/project.rb b/app/models/project.rb index 31990485f7..7e13115151 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -397,7 +397,7 @@ class Project < ActiveRecord::Base result.password = '*****' unless result.password.nil? result.to_s rescue - original_url + self.import_url end def check_limit From c7f7a1ae923532ec3b2e2d25ce97ebbac83cf2e0 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Wed, 13 Jan 2016 17:20:13 -0800 Subject: [PATCH 29/32] Update to Go 1.5.3 --- doc/install/installation.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/install/installation.md b/doc/install/installation.md index e645445df2..00030729a4 100644 --- a/doc/install/installation.md +++ b/doc/install/installation.md @@ -135,11 +135,11 @@ gitlab-workhorse we need a Go compiler. The instructions below assume you use 64-bit Linux. You can find downloads for other platforms at the [Go download page](https://golang.org/dl). - curl -O --progress https://storage.googleapis.com/golang/go1.5.1.linux-amd64.tar.gz - echo '46eecd290d8803887dec718c691cc243f2175fe0 go1.5.1.linux-amd64.tar.gz' | shasum -c - && \ - sudo tar -C /usr/local -xzf go1.5.1.linux-amd64.tar.gz + curl -O --progress https://storage.googleapis.com/golang/go1.5.3.linux-amd64.tar.gz + echo '43afe0c5017e502630b1aea4d44b8a7f059bf60d7f29dfd58db454d4e4e0ae53 go1.5.3.linux-amd64.tar.gz' | shasum -c - && \ + sudo tar -C /usr/local -xzf go1.5.3.linux-amd64.tar.gz sudo ln -sf /usr/local/go/bin/{go,godoc,gofmt} /usr/local/bin/ - rm go1.5.1.linux-amd64.tar.gz + rm go1.5.3.linux-amd64.tar.gz ## 4. System Users From fc7e6e8ffcb76e92b625b6490a966c830adc750a Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Thu, 14 Jan 2016 03:08:40 -0200 Subject: [PATCH 30/32] Fix Admin/Users view to position buttons without spacing magic --- app/assets/stylesheets/framework/buttons.scss | 3 ++ app/views/admin/users/index.html.haml | 30 +++++++++---------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index e237636348..bb29829b7a 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -131,6 +131,9 @@ &:last-child { margin-right: 0px; } + &.btn-xs { + margin-right: 3px; + } } &.disabled { pointer-events: auto !important; diff --git a/app/views/admin/users/index.html.haml b/app/views/admin/users/index.html.haml index 911c4d0cf1..8312642b6c 100644 --- a/app/views/admin/users/index.html.haml +++ b/app/views/admin/users/index.html.haml @@ -88,19 +88,19 @@ %i.fa.fa-envelope = mail_to user.email, user.email, class: 'light'   - = link_to 'Edit', edit_admin_user_path(user), id: "edit_#{dom_id(user)}", class: "btn btn-xs" - - unless user == current_user - - if user.ldap_blocked? - = link_to '#', title: 'Cannot unblock LDAP blocked users', data: {toggle: 'tooltip'}, class: 'btn btn-xs btn-success disabled' do - %i.fa.fa-lock - Unblock - = '' - - elsif user.blocked? - = link_to 'Unblock', unblock_admin_user_path(user), method: :put, class: "btn btn-xs btn-success" - - else - = link_to 'Block', block_admin_user_path(user), data: {confirm: 'USER WILL BE BLOCKED! Are you sure?'}, method: :put, class: "btn btn-xs btn-warning" - - if user.access_locked? - = link_to 'Unlock', unlock_admin_user_path(user), method: :put, class: "btn btn-xs btn-success", data: { confirm: 'Are you sure?' } - - if user.can_be_removed? - = link_to 'Destroy', [:admin, user], data: { confirm: "USER #{user.name} WILL BE REMOVED! All issues, merge requests and groups linked to this user will also be removed! Maybe block the user instead? Are you sure?" }, method: :delete, class: "btn btn-xs btn-remove" + .pull-right + = link_to 'Edit', edit_admin_user_path(user), id: "edit_#{dom_id(user)}", class: 'btn-grouped btn btn-xs' + - unless user == current_user + - if user.ldap_blocked? + = link_to '#', title: 'Cannot unblock LDAP blocked users', data: {toggle: 'tooltip'}, class: 'btn-grouped btn btn-xs btn-success disabled' do + %i.fa.fa-lock + Unblock + - elsif user.blocked? + = link_to 'Unblock', unblock_admin_user_path(user), method: :put, class: 'btn-grouped btn btn-xs btn-success' + - else + = link_to 'Block', block_admin_user_path(user), data: {confirm: 'USER WILL BE BLOCKED! Are you sure?'}, method: :put, class: 'btn-grouped btn btn-xs btn-warning' + - if user.access_locked? + = link_to 'Unlock', unlock_admin_user_path(user), method: :put, class: 'btn-grouped btn btn-xs btn-success', data: { confirm: 'Are you sure?' } + - if user.can_be_removed? + = link_to 'Destroy', [:admin, user], data: { confirm: "USER #{user.name} WILL BE REMOVED! All issues, merge requests and groups linked to this user will also be removed! Maybe block the user instead? Are you sure?" }, method: :delete, class: 'btn-grouped btn btn-xs btn-remove' = paginate @users, theme: "gitlab" From dd6fc01ff8a073880b67a323a547edeb5d63f167 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Thu, 14 Jan 2016 03:31:27 -0200 Subject: [PATCH 31/32] fixed LDAP activation on login to use new ldap_blocked state --- app/models/user.rb | 1 + lib/gitlab/ldap/access.rb | 4 +--- spec/lib/gitlab/ldap/access_spec.rb | 5 ++--- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 5eed9cf91c..592468933e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -205,6 +205,7 @@ class User < ActiveRecord::Base event :activate do transition blocked: :active + transition ldap_blocked: :active end state :blocked, :ldap_blocked do diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index ebd9260ad5..a659d179b5 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -40,9 +40,7 @@ module Gitlab user.ldap_block false else - if (user.blocked? && !ldap_config.block_auto_created_users) || user.ldap_blocked? - user.activate - end + user.activate if user.ldap_blocked? true end else diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index f58d70e809..32a19bf344 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -42,7 +42,6 @@ describe Gitlab::LDAP::Access, lib: true do context 'and has no disabled flag in active diretory' do before do - user.block allow(Gitlab::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(false) end @@ -50,7 +49,7 @@ describe Gitlab::LDAP::Access, lib: true do context 'when auto-created users are blocked' do before do - allow_any_instance_of(Gitlab::LDAP::Config).to receive(:block_auto_created_users).and_return(true) + user.block end it 'does not unblock user in GitLab' do @@ -62,7 +61,7 @@ describe Gitlab::LDAP::Access, lib: true do context 'when auto-created users are not blocked' do before do - allow_any_instance_of(Gitlab::LDAP::Config).to receive(:block_auto_created_users).and_return(false) + user.ldap_block end it 'should unblock user in GitLab' do From e918493f55eb27cdb779f0bc2d8cbbace8b69aa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 14 Jan 2016 10:04:48 +0100 Subject: [PATCH 32/32] Fix specs and rubocop warnings --- features/steps/shared/project.rb | 2 +- spec/models/note_spec.rb | 4 ++-- spec/requests/api/notes_spec.rb | 11 ++++++----- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index 5420c45151..d3501b5f5c 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -224,7 +224,7 @@ module SharedProject end def user_owns_project(user_name:, project_name:, visibility: :private) - user = user_exists(user_name, username: user_name.underscore) + user = user_exists(user_name, username: user_name.gsub(/\s/, '').underscore) project = Project.find_by(name: project_name) project ||= create(:empty_project, visibility, name: project_name, namespace: user.namespace) project.team << [user, :master] diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 65e6a7df3b..9182b42661 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -186,12 +186,12 @@ describe Note, models: true do let(:ext_proj) { create(:project, :public) } let(:ext_issue) { create(:issue, project: ext_proj) } - let(:note) { + let(:note) do create :note, noteable: ext_issue, project: ext_proj, note: "mentioned in issue #{private_issue.to_reference(ext_proj)}", system: true - } + end it "returns true" do expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 565805d870..d8bbd10726 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -13,20 +13,21 @@ describe API::API, api: true do # For testing the cross-reference of a private issue in a public issue let(:private_user) { create(:user) } - let(:private_project) { + let(:private_project) do create(:project, namespace: private_user.namespace). tap { |p| p.team << [private_user, :master] } - } - let(:private_issue) { create(:issue, project: private_project) } + end + let(:private_issue) { create(:issue, project: private_project) } + let(:ext_proj) { create(:project, :public) } let(:ext_issue) { create(:issue, project: ext_proj) } - let!(:cross_reference_note) { + let!(:cross_reference_note) do create :note, noteable: ext_issue, project: ext_proj, note: "mentioned in issue #{private_issue.to_reference(ext_proj)}", system: true - } + end before { project.team << [user, :reporter] }