diff options
author | Zeger-Jan van de Weg <zegerjan@gitlab.com> | 2016-03-15 21:13:26 +0300 |
---|---|---|
committer | Zeger-Jan van de Weg <zegerjan@gitlab.com> | 2016-03-15 21:16:16 +0300 |
commit | 59064aeeef8562a87d4d03efa9b11012a007e261 (patch) | |
tree | 1ecb34e1355a4eb714615b2a9c2727155e8f3ec9 /spec | |
parent | aaf4434b0e24da916d4392aa9cd001cdb8e0c7dc (diff) | |
parent | bc590ce63bd2f1af5545b648e6d028a557e7c792 (diff) |
Merge branch 'master' into 4009-external-users
Diffstat (limited to 'spec')
25 files changed, 570 insertions, 148 deletions
diff --git a/spec/controllers/namespaces_controller_spec.rb b/spec/controllers/namespaces_controller_spec.rb index d4a380cc2ee..77436958711 100644 --- a/spec/controllers/namespaces_controller_spec.rb +++ b/spec/controllers/namespaces_controller_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe NamespacesController do - let!(:user) { create(:user, :with_avatar) } + let!(:user) { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) } describe "GET show" do context "when the namespace belongs to a user" do diff --git a/spec/controllers/profiles/avatars_controller_spec.rb b/spec/controllers/profiles/avatars_controller_spec.rb index 85dff009bcf..ad5855df0a4 100644 --- a/spec/controllers/profiles/avatars_controller_spec.rb +++ b/spec/controllers/profiles/avatars_controller_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Profiles::AvatarsController do - let(:user) { create(:user, :with_avatar) } + let(:user) { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png")) } before do sign_in(user) diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index 0d9f4b299bc..af5d043cf02 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe UploadsController do - let!(:user) { create(:user, :with_avatar) } + let!(:user) { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) } describe "GET show" do context "when viewing a user avatar" do diff --git a/spec/factories/labels.rb b/spec/factories/labels.rb index 6e70af10af3..ea2be8928d5 100644 --- a/spec/factories/labels.rb +++ b/spec/factories/labels.rb @@ -13,7 +13,7 @@ FactoryGirl.define do factory :label do - title "Bug" + sequence(:title) { |n| "label#{n}" } color "#990000" project end diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index ca1c636fce4..a9df5fa1d3a 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -56,6 +56,10 @@ FactoryGirl.define do target_branch "feature" end + trait :merged do + state :merged + end + trait :closed do state :closed end @@ -84,6 +88,7 @@ FactoryGirl.define do merge_user author end + factory :merged_merge_request, traits: [:merged] factory :closed_merge_request, traits: [:closed] factory :reopened_merge_request, traits: [:reopened] factory :merge_request_with_diffs, traits: [:with_diffs] diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 785c2a3d811..a5c60c51c5b 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -23,13 +23,6 @@ FactoryGirl.define do end end - trait :with_avatar do - avatar { fixture_file_upload(Rails.root.join(*%w(spec fixtures dk.png)), 'image/png') } - avatar_crop_x 0 - avatar_crop_y 0 - avatar_crop_size 256 - end - factory :omniauth_user do transient do extern_uid '123456' diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 8013b31524f..f6c1005d265 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -77,7 +77,7 @@ describe ApplicationHelper do let(:avatar_file_path) { File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif') } it 'should return an url for the avatar' do - user = create(:user, :with_avatar, avatar: File.open(avatar_file_path)) + user = create(:user, avatar: File.open(avatar_file_path)) expect(helper.avatar_icon(user.email).to_s). to match("/uploads/user/avatar/#{user.id}/banana_sample.gif") @@ -88,7 +88,7 @@ describe ApplicationHelper do # Must be stubbed after the stub above, and separately stub_config_setting(url: Settings.send(:build_gitlab_url)) - user = create(:user, :with_avatar, avatar: File.open(avatar_file_path)) + user = create(:user, avatar: File.open(avatar_file_path)) expect(helper.avatar_icon(user.email).to_s). to match("/gitlab/uploads/user/avatar/#{user.id}/banana_sample.gif") @@ -102,7 +102,7 @@ describe ApplicationHelper do describe 'using a User' do it 'should return an URL for the avatar' do - user = create(:user, :with_avatar, avatar: File.open(avatar_file_path)) + user = create(:user, avatar: File.open(avatar_file_path)) expect(helper.avatar_icon(user).to_s). to match("/uploads/user/avatar/#{user.id}/banana_sample.gif") diff --git a/spec/lib/banzai/filter/sanitization_filter_spec.rb b/spec/lib/banzai/filter/sanitization_filter_spec.rb index 4a7b00c7660..27ce312b11c 100644 --- a/spec/lib/banzai/filter/sanitization_filter_spec.rb +++ b/spec/lib/banzai/filter/sanitization_filter_spec.rb @@ -149,10 +149,20 @@ describe Banzai::Filter::SanitizationFilter, lib: true do output: '<a href="java"></a>' }, + 'protocol-based JS injection: invalid URL char' => { + input: '<img src=java\script:alert("XSS")>', + output: '<img>' + }, + 'protocol-based JS injection: spaces and entities' => { input: '<a href="  javascript:alert(\'XSS\');">foo</a>', output: '<a href="">foo</a>' }, + + 'protocol whitespace' => { + input: '<a href=" http://example.com/"></a>', + output: '<a href="http://example.com/"></a>' + } } protocols.each do |name, data| @@ -177,6 +187,16 @@ describe Banzai::Filter::SanitizationFilter, lib: true do expect(output.to_html).to eq '<a>XSS</a>' end + it 'disallows invalid URIs' do + expect(Addressable::URI).to receive(:parse).with('foo://example.com'). + and_raise(Addressable::URI::InvalidURIError) + + input = '<a href="foo://example.com">Foo</a>' + output = filter(input) + + expect(output.to_html).to eq '<a>Foo</a>' + end + it 'allows non-standard anchor schemes' do exp = %q{<a href="irc://irc.freenode.net/git">IRC</a>} act = filter(exp) diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index 1e98280d045..fab6412d29f 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -397,7 +397,7 @@ module Ci services: ["mysql"], before_script: ["pwd"], rspec: { - artifacts: { paths: ["logs/", "binaries/"], untracked: true }, + artifacts: { paths: ["logs/", "binaries/"], untracked: true, name: "custom_name" }, script: "rspec" } }) @@ -417,6 +417,7 @@ module Ci image: "ruby:2.1", services: ["mysql"], artifacts: { + name: "custom_name", paths: ["logs/", "binaries/"], untracked: true } @@ -427,6 +428,73 @@ module Ci end end + describe "Dependencies" do + let(:config) do + { + build1: { stage: 'build', script: 'test' }, + build2: { stage: 'build', script: 'test' }, + test1: { stage: 'test', script: 'test', dependencies: dependencies }, + test2: { stage: 'test', script: 'test' }, + deploy: { stage: 'test', script: 'test' } + } + end + + subject { GitlabCiYamlProcessor.new(YAML.dump(config)) } + + context 'no dependencies' do + let(:dependencies) { } + + it { expect { subject }.to_not raise_error } + end + + context 'dependencies to builds' do + let(:dependencies) { [:build1, :build2] } + + it { expect { subject }.to_not raise_error } + end + + context 'undefined dependency' do + let(:dependencies) { [:undefined] } + + it { expect { subject }.to raise_error(GitlabCiYamlProcessor::ValidationError, 'test1 job: undefined dependency: undefined') } + end + + context 'dependencies to deploy' do + let(:dependencies) { [:deploy] } + + it { expect { subject }.to raise_error(GitlabCiYamlProcessor::ValidationError, 'test1 job: dependency deploy is not defined in prior stages') } + end + end + + describe "Hidden jobs" do + let(:config) do + YAML.dump({ + '.hidden_job' => { script: 'test' }, + 'normal_job' => { script: 'test' } + }) + end + + let(:config_processor) { GitlabCiYamlProcessor.new(config) } + + subject { config_processor.builds_for_stage_and_ref("test", "master") } + + it "doesn't create jobs that starts with dot" do + expect(subject.size).to eq(1) + expect(subject.first).to eq({ + except: nil, + stage: "test", + stage_idx: 1, + name: :normal_job, + only: nil, + commands: "\ntest", + tag_list: [], + options: {}, + when: "on_success", + allow_failure: false + }) + end + end + describe "YAML Alias/Anchor" do it "is correctly supported for jobs" do config = <<EOT @@ -629,6 +697,13 @@ EOT end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: when parameter should be on_success, on_failure or always") end + it "returns errors if job artifacts:name is not an a string" do + config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", artifacts: { name: 1 } } }) + expect do + GitlabCiYamlProcessor.new(config) + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: artifacts:name parameter should be a string") + end + it "returns errors if job artifacts:untracked is not an array of strings" do config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", artifacts: { untracked: "string" } } }) expect do @@ -684,6 +759,13 @@ EOT GitlabCiYamlProcessor.new(config) end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: cache:paths parameter should be an array of strings") end + + it "returns errors if job dependencies is not an array of strings" do + config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", dependencies: "string" } }) + expect do + GitlabCiYamlProcessor.new(config) + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: dependencies parameter should be an array of strings") + end end end end diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb index 5b575da34f3..c6758ccad39 100644 --- a/spec/mailers/emails/profile_spec.rb +++ b/spec/mailers/emails/profile_spec.rb @@ -11,7 +11,7 @@ describe Notify do let(:example_site_path) { root_path } let(:new_user) { create(:user, email: new_user_address, created_by_id: 1) } let(:token) { 'kETLwRaayvigPq_x3SNM' } - + subject { Notify.new_user_email(new_user.id, token) } it_behaves_like 'an email sent from GitLab' @@ -77,6 +77,10 @@ describe Notify do it 'includes a link to ssh keys page' do is_expected.to have_body_text /#{profile_keys_path}/ end + + context 'with SSH key that does not exist' do + it { expect { Notify.new_ssh_key_email('foo') }.not_to raise_error } + end end describe 'user added email' do diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 232a11245a6..f910424d85b 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -100,6 +100,34 @@ describe Notify do end end + describe 'that have been relabeled' do + subject { Notify.relabeled_issue_email(recipient.id, issue.id, %w[foo bar baz], current_user.id) } + + it_behaves_like 'a multiple recipients email' + it_behaves_like 'an answer to an existing thread', 'issue' + it_behaves_like 'it should show Gmail Actions View Issue link' + it_behaves_like 'a user cannot unsubscribe through footer link' + it_behaves_like 'an email with a labels subscriptions link in its footer' + + it 'is sent as the author' do + sender = subject.header[:from].addrs[0] + expect(sender.display_name).to eq(current_user.name) + expect(sender.address).to eq(gitlab_sender) + end + + it 'has the correct subject' do + is_expected.to have_subject /#{issue.title} \(##{issue.iid}\)/ + end + + it 'contains the names of the added labels' do + is_expected.to have_body_text /foo, bar, and baz/ + end + + it 'contains a link to the issue' do + is_expected.to have_body_text /#{namespace_project_issue_path project.namespace, project, issue}/ + end + end + describe 'status changed' do let(:status) { 'closed' } subject { Notify.issue_status_changed_email(recipient.id, issue.id, status, current_user.id) } @@ -219,6 +247,34 @@ describe Notify do end end + describe 'that have been relabeled' do + subject { Notify.relabeled_merge_request_email(recipient.id, merge_request.id, %w[foo bar baz], current_user.id) } + + it_behaves_like 'a multiple recipients email' + it_behaves_like 'an answer to an existing thread', 'merge_request' + it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like 'a user cannot unsubscribe through footer link' + it_behaves_like 'an email with a labels subscriptions link in its footer' + + it 'is sent as the author' do + sender = subject.header[:from].addrs[0] + expect(sender.display_name).to eq(current_user.name) + expect(sender.address).to eq(gitlab_sender) + end + + it 'has the correct subject' do + is_expected.to have_subject /#{merge_request.title} \(##{merge_request.iid}\)/ + end + + it 'contains the names of the added labels' do + is_expected.to have_body_text /foo, bar, and baz/ + end + + it 'contains a link to the merge request' do + is_expected.to have_body_text /#{namespace_project_merge_request_path project.namespace, project, merge_request}/ + end + end + describe 'status changed' do let(:status) { 'reopened' } subject { Notify.merge_request_status_email(recipient.id, merge_request.id, status, current_user.id) } diff --git a/spec/mailers/shared/notify.rb b/spec/mailers/shared/notify.rb index 48c851ebbd6..6019af544d3 100644 --- a/spec/mailers/shared/notify.rb +++ b/spec/mailers/shared/notify.rb @@ -112,6 +112,10 @@ shared_examples 'an unsubscribeable thread' do it { is_expected.to have_body_text /unsubscribe/ } end -shared_examples "a user cannot unsubscribe through footer link" do +shared_examples 'a user cannot unsubscribe through footer link' do it { is_expected.not_to have_body_text /unsubscribe/ } end + +shared_examples 'an email with a labels subscriptions link in its footer' do + it { is_expected.to have_body_text /label subscriptions/ } +end diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb index e3d3d453653..b7457808040 100644 --- a/spec/models/build_spec.rb +++ b/spec/models/build_spec.rb @@ -9,7 +9,7 @@ describe Ci::Build, models: true do it { is_expected.to respond_to :trace_html } - describe :first_pending do + describe '#first_pending' do let(:first) { FactoryGirl.create :ci_build, commit: commit, status: 'pending', created_at: Date.yesterday } let(:second) { FactoryGirl.create :ci_build, commit: commit, status: 'pending' } before { first; second } @@ -19,7 +19,7 @@ describe Ci::Build, models: true do it('returns with the first pending build') { is_expected.to eq(first) } end - describe :create_from do + describe '#create_from' do before do build.status = 'success' build.save @@ -33,7 +33,7 @@ describe Ci::Build, models: true do end end - describe :ignored? do + describe '#ignored?' do subject { build.ignored? } context 'if build is not allowed to fail' do @@ -69,7 +69,7 @@ describe Ci::Build, models: true do end end - describe :trace do + describe '#trace' do subject { build.trace_html } it { is_expected.to be_empty } @@ -101,7 +101,7 @@ describe Ci::Build, models: true do # it { is_expected.to eq(commit.project.timeout) } # end - describe :options do + describe '#options' do let(:options) do { image: "ruby:2.1", @@ -122,25 +122,25 @@ describe Ci::Build, models: true do # it { is_expected.to eq(project.allow_git_fetch) } # end - describe :project do + describe '#project' do subject { build.project } it { is_expected.to eq(commit.project) } end - describe :project_id do + describe '#project_id' do subject { build.project_id } it { is_expected.to eq(commit.project_id) } end - describe :project_name do + describe '#project_name' do subject { build.project_name } it { is_expected.to eq(project.name) } end - describe :extract_coverage do + describe '#extract_coverage' do context 'valid content & regex' do subject { build.extract_coverage('Coverage 1033 / 1051 LOC (98.29%) covered', '\(\d+.\d+\%\) covered') } @@ -172,7 +172,7 @@ describe Ci::Build, models: true do end end - describe :variables do + describe '#variables' do context 'returns variables' do subject { build.variables } @@ -242,7 +242,7 @@ describe Ci::Build, models: true do end end - describe :can_be_served? do + describe '#can_be_served?' do let(:runner) { FactoryGirl.create :ci_runner } before { build.project.runners << runner } @@ -277,7 +277,7 @@ describe Ci::Build, models: true do end end - describe :any_runners_online? do + describe '#any_runners_online?' do subject { build.any_runners_online? } context 'when no runners' do @@ -312,8 +312,8 @@ describe Ci::Build, models: true do end end - describe :show_warning? do - subject { build.show_warning? } + describe '#stuck?' do + subject { build.stuck? } %w(pending).each do |state| context "if commit_status.status is #{state}" do @@ -343,35 +343,7 @@ describe Ci::Build, models: true do end end - describe :artifacts_download_url do - subject { build.artifacts_download_url } - - context 'artifacts file does not exist' do - before { build.update_attributes(artifacts_file: nil) } - it { is_expected.to be_nil } - end - - context 'artifacts file exists' do - let(:build) { create(:ci_build, :artifacts) } - it { is_expected.to_not be_nil } - end - end - - describe :artifacts_browse_url do - subject { build.artifacts_browse_url } - - it "should be nil if artifacts browser is unsupported" do - allow(build).to receive(:artifacts_metadata?).and_return(false) - is_expected.to be_nil - end - - it 'should not be nil if artifacts browser is supported' do - allow(build).to receive(:artifacts_metadata?).and_return(true) - is_expected.to_not be_nil - end - end - - describe :artifacts? do + describe '#artifacts?' do subject { build.artifacts? } context 'artifacts archive does not exist' do @@ -386,7 +358,7 @@ describe Ci::Build, models: true do end - describe :artifacts_metadata? do + describe '#artifacts_metadata?' do subject { build.artifacts_metadata? } context 'artifacts metadata does not exist' do it { is_expected.to be_falsy } @@ -398,7 +370,7 @@ describe Ci::Build, models: true do end end - describe :repo_url do + describe '#repo_url' do let(:build) { FactoryGirl.create :ci_build } let(:project) { build.project } @@ -412,7 +384,7 @@ describe Ci::Build, models: true do it { is_expected.to include(project.web_url[7..-1]) } end - describe :depends_on_builds do + describe '#depends_on_builds' do let!(:build) { FactoryGirl.create :ci_build, commit: commit, name: 'build', stage_idx: 0, stage: 'build' } let!(:rspec_test) { FactoryGirl.create :ci_build, commit: commit, name: 'rspec', stage_idx: 1, stage: 'test' } let!(:rubocop_test) { FactoryGirl.create :ci_build, commit: commit, name: 'rubocop', stage_idx: 1, stage: 'test' } @@ -444,7 +416,7 @@ describe Ci::Build, models: true do created_at: created_at) end - describe :merge_request do + describe '#merge_request' do context 'when a MR has a reference to the commit' do before do @merge_request = create_mr(build, commit, factory: :merge_request) diff --git a/spec/models/ci/commit_spec.rb b/spec/models/ci/commit_spec.rb index 4dc309a4255..412842337ba 100644 --- a/spec/models/ci/commit_spec.rb +++ b/spec/models/ci/commit_spec.rb @@ -32,50 +32,6 @@ describe Ci::Commit, models: true do it { is_expected.to respond_to :git_author_email } it { is_expected.to respond_to :short_sha } - describe :ordered do - let(:project) { FactoryGirl.create :empty_project } - - it 'returns ordered list of commits' do - commit1 = FactoryGirl.create :ci_commit, committed_at: 1.hour.ago, project: project - commit2 = FactoryGirl.create :ci_commit, committed_at: 2.hours.ago, project: project - expect(project.ci_commits.ordered).to eq([commit2, commit1]) - end - - it 'returns commits ordered by committed_at and id, with nulls last' do - commit1 = FactoryGirl.create :ci_commit, committed_at: 1.hour.ago, project: project - commit2 = FactoryGirl.create :ci_commit, committed_at: nil, project: project - commit3 = FactoryGirl.create :ci_commit, committed_at: 2.hours.ago, project: project - commit4 = FactoryGirl.create :ci_commit, committed_at: nil, project: project - expect(project.ci_commits.ordered).to eq([commit2, commit4, commit3, commit1]) - end - end - - describe :last_build do - subject { commit.last_build } - before do - @first = FactoryGirl.create :ci_build, commit: commit, created_at: Date.yesterday - @second = FactoryGirl.create :ci_build, commit: commit - end - - it { is_expected.to be_a(Ci::Build) } - it('returns with the most recently created build') { is_expected.to eq(@second) } - end - - describe :retry do - before do - @first = FactoryGirl.create :ci_build, commit: commit, created_at: Date.yesterday - @second = FactoryGirl.create :ci_build, commit: commit - end - - it "creates only a new build" do - expect(commit.builds.count(:all)).to eq 2 - expect(commit.statuses.count(:all)).to eq 2 - commit.retry - expect(commit.builds.count(:all)).to eq 3 - expect(commit.statuses.count(:all)).to eq 3 - end - end - describe :valid_commit_sha do context 'commit.sha can not start with 00000000' do before do diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index aff384c2949..be29b6d66ff 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -113,6 +113,48 @@ describe Issue, "Issuable" do end end + describe '#subscribed?' do + context 'user is not a participant in the issue' do + before { allow(issue).to receive(:participants).with(user).and_return([]) } + + it 'returns false when no subcription exists' do + expect(issue.subscribed?(user)).to be_falsey + end + + it 'returns true when a subcription exists and subscribed is true' do + issue.subscriptions.create(user: user, subscribed: true) + + expect(issue.subscribed?(user)).to be_truthy + end + + it 'returns false when a subcription exists and subscribed is false' do + issue.subscriptions.create(user: user, subscribed: false) + + expect(issue.subscribed?(user)).to be_falsey + end + end + + context 'user is a participant in the issue' do + before { allow(issue).to receive(:participants).with(user).and_return([user]) } + + it 'returns false when no subcription exists' do + expect(issue.subscribed?(user)).to be_truthy + end + + it 'returns true when a subcription exists and subscribed is true' do + issue.subscriptions.create(user: user, subscribed: true) + + expect(issue.subscribed?(user)).to be_truthy + end + + it 'returns false when a subcription exists and subscribed is false' do + issue.subscriptions.create(user: user, subscribed: false) + + expect(issue.subscribed?(user)).to be_falsey + end + end + end + describe "#to_hook_data" do let(:data) { issue.to_hook_data(user) } let(:project) { issue.project } diff --git a/spec/models/concerns/subscribable_spec.rb b/spec/models/concerns/subscribable_spec.rb new file mode 100644 index 00000000000..e31fdb0bffb --- /dev/null +++ b/spec/models/concerns/subscribable_spec.rb @@ -0,0 +1,57 @@ +require 'spec_helper' + +describe Subscribable, 'Subscribable' do + let(:resource) { create(:issue) } + let(:user) { create(:user) } + + describe '#subscribed?' do + it 'returns false when no subcription exists' do + expect(resource.subscribed?(user)).to be_falsey + end + + it 'returns true when a subcription exists and subscribed is true' do + resource.subscriptions.create(user: user, subscribed: true) + + expect(resource.subscribed?(user)).to be_truthy + end + + it 'returns false when a subcription exists and subscribed is false' do + resource.subscriptions.create(user: user, subscribed: false) + + expect(resource.subscribed?(user)).to be_falsey + end + end + describe '#subscribers' do + it 'returns [] when no subcribers exists' do + expect(resource.subscribers).to be_empty + end + + it 'returns the subscribed users' do + resource.subscriptions.create(user: user, subscribed: true) + resource.subscriptions.create(user: create(:user), subscribed: false) + + expect(resource.subscribers).to eq [user] + end + end + + describe '#toggle_subscription' do + it 'toggles the current subscription state for the given user' do + expect(resource.subscribed?(user)).to be_falsey + + resource.toggle_subscription(user) + + expect(resource.subscribed?(user)).to be_truthy + end + end + + describe '#unsubscribe' do + it 'unsubscribes the given current user' do + resource.subscriptions.create(user: user, subscribed: true) + expect(resource.subscribed?(user)).to be_truthy + + resource.unsubscribe(user) + + expect(resource.subscribed?(user)).to be_falsey + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 59c5ffa6b9c..b8b9a455b83 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -634,6 +634,12 @@ describe Project, models: true do it 'returns projects with a matching namespace name regardless of the casing' do expect(described_class.search(project.namespace.name.upcase)).to eq([project]) end + + it 'returns projects when eager loading namespaces' do + relation = described_class.all.includes(:namespace) + + expect(relation.search(project.namespace.name)).to eq([project]) + end end describe '#rename_repo' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ca0c84c8632..0ab7fd88ce6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -174,32 +174,6 @@ describe User, models: true do end end end - - describe 'avatar' do - it 'only validates when avatar is present and changed' do - user = build(:user, :with_avatar) - - user.avatar_crop_x = nil - user.avatar_crop_y = nil - user.avatar_crop_size = nil - - expect(user).not_to be_valid - expect(user.errors.keys). - to match_array %i(avatar_crop_x avatar_crop_y avatar_crop_size) - end - - it 'does not validate when avatar has not changed' do - user = create(:user, :with_avatar) - - expect { user.avatar_crop_x = nil }.not_to change(user, :valid?) - end - - it 'does not validate when avatar is not present' do - user = create(:user) - - expect { user.avatar_crop_y = nil }.not_to change(user, :valid?) - end - end end describe "Respond to" do diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index a7e2e1b1792..145bc937560 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -401,6 +401,45 @@ describe GitPushService, services: true do end end + describe "housekeeping" do + let(:housekeeping) { Projects::HousekeepingService.new(project) } + + before do + allow(Projects::HousekeepingService).to receive(:new).and_return(housekeeping) + end + + it 'does not perform housekeeping when not needed' do + expect(housekeeping).not_to receive(:execute) + + execute_service(project, user, @oldrev, @newrev, @ref) + end + + context 'when housekeeping is needed' do + before do + allow(housekeeping).to receive(:needed?).and_return(true) + end + + it 'performs housekeeping' do + expect(housekeeping).to receive(:execute) + + execute_service(project, user, @oldrev, @newrev, @ref) + end + + it 'does not raise an exception' do + allow(housekeeping).to receive(:try_obtain_lease).and_return(false) + + execute_service(project, user, @oldrev, @newrev, @ref) + end + end + + + it 'increments the push counter' do + expect(housekeeping).to receive(:increment!) + + execute_service(project, user, @oldrev, @newrev, @ref) + end + end + def execute_service(project, user, oldrev, newrev, ref) service = described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref ) service.execute diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index e579e49dfa7..4ffe753fef5 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -6,6 +6,7 @@ describe Issues::UpdateService, services: true do let(:user3) { create(:user) } let(:issue) { create(:issue, title: 'Old title', assignee_id: user3.id) } let(:label) { create(:label) } + let(:label2) { create(:label) } let(:project) { issue.project } before do @@ -48,7 +49,7 @@ describe Issues::UpdateService, services: true do it { expect(@issue.assignee).to eq(user2) } it { expect(@issue).to be_closed } it { expect(@issue.labels.count).to eq(1) } - it { expect(@issue.labels.first.title).to eq('Bug') } + it { expect(@issue.labels.first.title).to eq(label.name) } it 'should send email to user2 about assign of new issue and email to user3 about issue unassignment' do deliveries = ActionMailer::Base.deliveries @@ -148,6 +149,48 @@ describe Issues::UpdateService, services: true do end end + context 'when the issue is relabeled' do + let!(:non_subscriber) { create(:user) } + let!(:subscriber) { create(:user).tap { |u| label.toggle_subscription(u) } } + + it 'sends notifications for subscribers of newly added labels' do + opts = { label_ids: [label.id] } + + perform_enqueued_jobs do + @issue = Issues::UpdateService.new(project, user, opts).execute(issue) + end + + should_email(subscriber) + should_not_email(non_subscriber) + end + + context 'when issue has the `label` label' do + before { issue.labels << label } + + it 'does not send notifications for existing labels' do + opts = { label_ids: [label.id, label2.id] } + + perform_enqueued_jobs do + @issue = Issues::UpdateService.new(project, user, opts).execute(issue) + end + + should_not_email(subscriber) + should_not_email(non_subscriber) + end + + it 'does not send notifications for removed labels' do + opts = { label_ids: [label2.id] } + + perform_enqueued_jobs do + @issue = Issues::UpdateService.new(project, user, opts).execute(issue) + end + + should_not_email(subscriber) + should_not_email(non_subscriber) + end + end + end + context 'when Issue has tasks' do before { update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" }) } diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 99703c7a8ec..cb8cff2fa8c 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -7,6 +7,7 @@ describe MergeRequests::UpdateService, services: true do let(:merge_request) { create(:merge_request, :simple, title: 'Old title', assignee_id: user3.id) } let(:project) { merge_request.project } let(:label) { create(:label) } + let(:label2) { create(:label) } before do project.team << [user, :master] @@ -53,7 +54,7 @@ describe MergeRequests::UpdateService, services: true do it { expect(@merge_request.assignee).to eq(user2) } it { expect(@merge_request).to be_closed } it { expect(@merge_request.labels.count).to eq(1) } - it { expect(@merge_request.labels.first.title).to eq('Bug') } + it { expect(@merge_request.labels.first.title).to eq(label.name) } it { expect(@merge_request.target_branch).to eq('target') } it 'should execute hooks with update action' do @@ -176,6 +177,48 @@ describe MergeRequests::UpdateService, services: true do end end + context 'when the issue is relabeled' do + let!(:non_subscriber) { create(:user) } + let!(:subscriber) { create(:user).tap { |u| label.toggle_subscription(u) } } + + it 'sends notifications for subscribers of newly added labels' do + opts = { label_ids: [label.id] } + + perform_enqueued_jobs do + @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + end + + should_email(subscriber) + should_not_email(non_subscriber) + end + + context 'when issue has the `label` label' do + before { merge_request.labels << label } + + it 'does not send notifications for existing labels' do + opts = { label_ids: [label.id, label2.id] } + + perform_enqueued_jobs do + @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + end + + should_not_email(subscriber) + should_not_email(non_subscriber) + end + + it 'does not send notifications for removed labels' do + opts = { label_ids: [label2.id] } + + perform_enqueued_jobs do + @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + end + + should_not_email(subscriber) + should_not_email(non_subscriber) + end + end + end + context 'when MergeRequest has tasks' do before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 2d0b5df4224..b5407397c1d 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -224,6 +224,15 @@ describe NotificationService, services: true do should_not_email(issue.assignee) end + + it "emails subscribers of the issue's labels" do + subscriber = create(:user) + label = create(:label, issues: [issue]) + label.toggle_subscription(subscriber) + notification.new_issue(issue, @u_disabled) + + should_email(subscriber) + end end describe :reassigned_issue do @@ -296,6 +305,35 @@ describe NotificationService, services: true do end end + describe '#relabeled_issue' do + let(:label) { create(:label, issues: [issue]) } + let(:label2) { create(:label) } + let!(:subscriber_to_label) { create(:user).tap { |u| label.toggle_subscription(u) } } + let!(:subscriber_to_label2) { create(:user).tap { |u| label2.toggle_subscription(u) } } + + it "emails subscribers of the issue's added labels only" do + notification.relabeled_issue(issue, [label2], @u_disabled) + + should_not_email(subscriber_to_label) + should_email(subscriber_to_label2) + end + + it "doesn't send email to anyone but subscribers of the given labels" do + notification.relabeled_issue(issue, [label2], @u_disabled) + + should_not_email(issue.assignee) + should_not_email(issue.author) + should_not_email(@u_watcher) + should_not_email(@u_participant_mentioned) + should_not_email(@subscriber) + should_not_email(@watcher_and_subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(subscriber_to_label) + should_email(subscriber_to_label2) + end + end + describe :close_issue do it 'should sent email to issue assignee and issue author' do notification.close_issue(issue, @u_disabled) @@ -349,6 +387,15 @@ describe NotificationService, services: true do should_not_email(@u_participating) should_not_email(@u_disabled) end + + it "emails subscribers of the merge request's labels" do + subscriber = create(:user) + label = create(:label, merge_requests: [merge_request]) + label.toggle_subscription(subscriber) + notification.new_merge_request(merge_request, @u_disabled) + + should_email(subscriber) + end end describe :reassigned_merge_request do @@ -366,6 +413,35 @@ describe NotificationService, services: true do end end + describe :relabel_merge_request do + let(:label) { create(:label, merge_requests: [merge_request]) } + let(:label2) { create(:label) } + let!(:subscriber_to_label) { create(:user).tap { |u| label.toggle_subscription(u) } } + let!(:subscriber_to_label2) { create(:user).tap { |u| label2.toggle_subscription(u) } } + + it "emails subscribers of the merge request's added labels only" do + notification.relabeled_merge_request(merge_request, [label2], @u_disabled) + + should_not_email(subscriber_to_label) + should_email(subscriber_to_label2) + end + + it "doesn't send email to anyone but subscribers of the given labels" do + notification.relabeled_merge_request(merge_request, [label2], @u_disabled) + + should_not_email(merge_request.assignee) + should_not_email(merge_request.author) + should_not_email(@u_watcher) + should_not_email(@u_participant_mentioned) + should_not_email(@subscriber) + should_not_email(@watcher_and_subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(subscriber_to_label) + should_email(subscriber_to_label2) + end + end + describe :closed_merge_request do it do notification.close_mr(merge_request, @u_disabled) @@ -467,16 +543,4 @@ describe NotificationService, services: true do # Make the watcher a subscriber to detect dupes issuable.subscriptions.create(user: @watcher_and_subscriber, subscribed: true) end - - def sent_to_user?(user) - ActionMailer::Base.deliveries.map(&:to).flatten.count(user.email) == 1 - end - - def should_email(user) - expect(sent_to_user?(user)).to be_truthy - end - - def should_not_email(user) - expect(sent_to_user?(user)).to be_falsey - end end diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb new file mode 100644 index 00000000000..93bf1b81fbe --- /dev/null +++ b/spec/services/projects/housekeeping_service_spec.rb @@ -0,0 +1,48 @@ +require 'spec_helper' + +describe Projects::HousekeepingService do + subject { Projects::HousekeepingService.new(project) } + let(:project) { create :project } + + describe 'execute' do + before do + project.pushes_since_gc = 3 + project.save! + end + + it 'enqueues a sidekiq job' do + expect(subject).to receive(:try_obtain_lease).and_return(true) + expect(GitlabShellWorker).to receive(:perform_async).with(:gc, project.path_with_namespace) + + subject.execute + expect(project.pushes_since_gc).to eq(0) + end + + it 'does not enqueue a job when no lease can be obtained' do + expect(subject).to receive(:try_obtain_lease).and_return(false) + expect(GitlabShellWorker).not_to receive(:perform_async) + + expect { subject.execute }.to raise_error(Projects::HousekeepingService::LeaseTaken) + expect(project.pushes_since_gc).to eq(0) + end + end + + describe 'needed?' do + it 'when the count is low enough' do + expect(subject.needed?).to eq(false) + end + + it 'when the count is high enough' do + allow(project).to receive(:pushes_since_gc).and_return(10) + expect(subject.needed?).to eq(true) + end + end + + describe 'increment!' do + it 'increments the pushes_since_gc counter' do + expect(project.pushes_since_gc).to eq(0) + subject.increment! + expect(project.pushes_since_gc).to eq(1) + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 7d939ca7509..596d607f2a1 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -32,6 +32,7 @@ RSpec.configure do |config| config.include LoginHelpers, type: :feature config.include LoginHelpers, type: :request config.include StubConfiguration + config.include EmailHelpers config.include RelativeUrl, type: feature config.include TestEnv config.include ActiveJob::TestHelper diff --git a/spec/support/email_helpers.rb b/spec/support/email_helpers.rb new file mode 100644 index 00000000000..a85ab22ce36 --- /dev/null +++ b/spec/support/email_helpers.rb @@ -0,0 +1,13 @@ +module EmailHelpers + def sent_to_user?(user) + ActionMailer::Base.deliveries.map(&:to).flatten.count(user.email) == 1 + end + + def should_email(user) + expect(sent_to_user?(user)).to be_truthy + end + + def should_not_email(user) + expect(sent_to_user?(user)).to be_falsey + end +end |