From 4f10cad9ffdc508dbcecd477e93921367a69477d Mon Sep 17 00:00:00 2001 From: Mateusz Bajorski Date: Mon, 15 Jan 2018 07:46:19 +0100 Subject: Add Inherit quick action Closes #38450 --- .../quick_actions/interpret_service_spec.rb | 65 ++++++++++++++++++++++ 1 file changed, 65 insertions(+) (limited to 'spec/services') diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index f793f55e51b..ebe0c7639a0 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -306,6 +306,23 @@ describe QuickActions::InterpretService do end end + shared_examples 'inherit command' do + it 'fetches issue and copies labels and milestone if content contains /inherit issue_reference' do + issue_father # populate the issue + todo_label # populate this label + inreview_label # populate this label + _, updates = service.execute(content, issuable) + + expect(updates[:add_label_ids]).to match_array([inreview_label.id, todo_label.id]) + + if issue_father.milestone + expect(updates[:milestone_id]).to eq(issue_father.milestone.id) + else + expect(updates).not_to have_key(:milestone_id) + end + end + end + shared_examples 'shrug command' do it 'appends ¯\_(ツ)_/¯ to the comment' do new_content, _ = service.execute(content, issuable) @@ -757,6 +774,54 @@ describe QuickActions::InterpretService do let(:issuable) { issue } end + context '/inherit command' do + let!(:todo_label) { create(:label, project: project, title: 'To Do') } + let!(:inreview_label) { create(:label, project: project, title: 'In Review') } + + it_behaves_like 'inherit command' do + # Without milestone assignment + let(:issue_father) { create(:labeled_issue, project: project, labels: [inreview_label, todo_label]) } + + let(:content) { "/inherit #{issue_father.to_reference}" } + let(:issuable) { issue } + end + + it_behaves_like 'inherit command' do + # With milestone assignment + let(:issue_father) { create(:labeled_issue, project: project, labels: [todo_label, inreview_label], milestone: milestone) } + + let(:content) { "/inherit #{issue_father.to_reference(project)}" } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { '/inherit' } + let(:issuable) { issue } + end + + context 'cross project references' do + it_behaves_like 'empty command' do + let(:other_project) { create(:project, :public) } + let(:issue_father) { create(:labeled_issue, project: other_project, labels: [todo_label, inreview_label]) } + let(:content) { "/inherit #{issue_father.to_reference(project)}" } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { "/inherit imaginary#1234" } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:other_project) { create(:project, :private) } + let(:issue_father) { create(:issue, project: other_project) } + + let(:content) { "/inherit #{issue_father.to_reference(project)}" } + let(:issuable) { issue } + end + end + end + context '/duplicate command' do it_behaves_like 'duplicate command' do let(:issue_duplicate) { create(:issue, project: project) } -- cgit v1.2.1 From a0adf87707e44fda83aca859b41ce18372a1c72b Mon Sep 17 00:00:00 2001 From: Mateusz Bajorski Date: Sun, 21 Jan 2018 18:16:56 +0100 Subject: Changed command name to copy_metadata and added MR support --- .../quick_actions/interpret_service_spec.rb | 46 +++++++++++----------- 1 file changed, 23 insertions(+), 23 deletions(-) (limited to 'spec/services') diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index ebe0c7639a0..9a8240f9491 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -306,17 +306,17 @@ describe QuickActions::InterpretService do end end - shared_examples 'inherit command' do - it 'fetches issue and copies labels and milestone if content contains /inherit issue_reference' do - issue_father # populate the issue + shared_examples 'copy_metadata command' do + it 'fetches issue or merge request and copies labels and milestone if content contains /copy_metadata reference' do + issueable_father # populate the issue todo_label # populate this label inreview_label # populate this label _, updates = service.execute(content, issuable) expect(updates[:add_label_ids]).to match_array([inreview_label.id, todo_label.id]) - if issue_father.milestone - expect(updates[:milestone_id]).to eq(issue_father.milestone.id) + if issueable_father.milestone + expect(updates[:milestone_id]).to eq(issueable_father.milestone.id) else expect(updates).not_to have_key(:milestone_id) end @@ -774,49 +774,49 @@ describe QuickActions::InterpretService do let(:issuable) { issue } end - context '/inherit command' do + context '/copy_metadata command' do let!(:todo_label) { create(:label, project: project, title: 'To Do') } let!(:inreview_label) { create(:label, project: project, title: 'In Review') } - it_behaves_like 'inherit command' do - # Without milestone assignment - let(:issue_father) { create(:labeled_issue, project: project, labels: [inreview_label, todo_label]) } - - let(:content) { "/inherit #{issue_father.to_reference}" } + it_behaves_like 'empty command' do + let(:content) { '/copy_metadata' } let(:issuable) { issue } end - it_behaves_like 'inherit command' do - # With milestone assignment - let(:issue_father) { create(:labeled_issue, project: project, labels: [todo_label, inreview_label], milestone: milestone) } + it_behaves_like 'copy_metadata command' do + let(:issueable_father) { create(:labeled_issue, project: project, labels: [inreview_label, todo_label]) } - let(:content) { "/inherit #{issue_father.to_reference(project)}" } + let(:content) { "/copy_metadata #{issueable_father.to_reference}" } let(:issuable) { issue } end - it_behaves_like 'empty command' do - let(:content) { '/inherit' } - let(:issuable) { issue } + context 'when the parent issueable has a milestone' do + it_behaves_like 'copy_metadata command' do + let(:issueable_father) { create(:labeled_issue, project: project, labels: [todo_label, inreview_label], milestone: milestone) } + + let(:content) { "/copy_metadata #{issueable_father.to_reference(project)}" } + let(:issuable) { issue } + end end context 'cross project references' do it_behaves_like 'empty command' do let(:other_project) { create(:project, :public) } - let(:issue_father) { create(:labeled_issue, project: other_project, labels: [todo_label, inreview_label]) } - let(:content) { "/inherit #{issue_father.to_reference(project)}" } + let(:issueable_father) { create(:labeled_issue, project: other_project, labels: [todo_label, inreview_label]) } + let(:content) { "/copy_metadata #{issueable_father.to_reference(project)}" } let(:issuable) { issue } end it_behaves_like 'empty command' do - let(:content) { "/inherit imaginary#1234" } + let(:content) { "/copy_metadata imaginary#1234" } let(:issuable) { issue } end it_behaves_like 'empty command' do let(:other_project) { create(:project, :private) } - let(:issue_father) { create(:issue, project: other_project) } + let(:issueable_father) { create(:issue, project: other_project) } - let(:content) { "/inherit #{issue_father.to_reference(project)}" } + let(:content) { "/copy_metadata #{issueable_father.to_reference(project)}" } let(:issuable) { issue } end end -- cgit v1.2.1 From 26087ae91c0397054786bed7bcc078b03dd8752b Mon Sep 17 00:00:00 2001 From: Mateusz Bajorski Date: Wed, 31 Jan 2018 18:41:06 +0100 Subject: Fixed typos and improved reference checking --- .../quick_actions/interpret_service_spec.rb | 24 +++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) (limited to 'spec/services') diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 9a8240f9491..4aad2aaef79 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -308,15 +308,15 @@ describe QuickActions::InterpretService do shared_examples 'copy_metadata command' do it 'fetches issue or merge request and copies labels and milestone if content contains /copy_metadata reference' do - issueable_father # populate the issue + source_issuable # populate the issue todo_label # populate this label inreview_label # populate this label _, updates = service.execute(content, issuable) expect(updates[:add_label_ids]).to match_array([inreview_label.id, todo_label.id]) - if issueable_father.milestone - expect(updates[:milestone_id]).to eq(issueable_father.milestone.id) + if source_issuable.milestone + expect(updates[:milestone_id]).to eq(source_issuable.milestone.id) else expect(updates).not_to have_key(:milestone_id) end @@ -784,17 +784,17 @@ describe QuickActions::InterpretService do end it_behaves_like 'copy_metadata command' do - let(:issueable_father) { create(:labeled_issue, project: project, labels: [inreview_label, todo_label]) } + let(:source_issuable) { create(:labeled_issue, project: project, labels: [inreview_label, todo_label]) } - let(:content) { "/copy_metadata #{issueable_father.to_reference}" } + let(:content) { "/copy_metadata #{source_issuable.to_reference}" } let(:issuable) { issue } end - context 'when the parent issueable has a milestone' do + context 'when the parent issuable has a milestone' do it_behaves_like 'copy_metadata command' do - let(:issueable_father) { create(:labeled_issue, project: project, labels: [todo_label, inreview_label], milestone: milestone) } + let(:source_issuable) { create(:labeled_issue, project: project, labels: [todo_label, inreview_label], milestone: milestone) } - let(:content) { "/copy_metadata #{issueable_father.to_reference(project)}" } + let(:content) { "/copy_metadata #{source_issuable.to_reference(project)}" } let(:issuable) { issue } end end @@ -802,8 +802,8 @@ describe QuickActions::InterpretService do context 'cross project references' do it_behaves_like 'empty command' do let(:other_project) { create(:project, :public) } - let(:issueable_father) { create(:labeled_issue, project: other_project, labels: [todo_label, inreview_label]) } - let(:content) { "/copy_metadata #{issueable_father.to_reference(project)}" } + let(:source_issuable) { create(:labeled_issue, project: other_project, labels: [todo_label, inreview_label]) } + let(:content) { "/copy_metadata #{source_issuable.to_reference(project)}" } let(:issuable) { issue } end @@ -814,9 +814,9 @@ describe QuickActions::InterpretService do it_behaves_like 'empty command' do let(:other_project) { create(:project, :private) } - let(:issueable_father) { create(:issue, project: other_project) } + let(:source_issuable) { create(:issue, project: other_project) } - let(:content) { "/copy_metadata #{issueable_father.to_reference(project)}" } + let(:content) { "/copy_metadata #{source_issuable.to_reference(project)}" } let(:issuable) { issue } end end -- cgit v1.2.1 From f4c8517fec6d53e079f465d594ddef531e12c0af Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Wed, 14 Feb 2018 10:16:37 +0100 Subject: Add failing notification_recipient_service spec --- .../notification_recipient_service_spec.rb | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 spec/services/notification_recipient_service_spec.rb (limited to 'spec/services') diff --git a/spec/services/notification_recipient_service_spec.rb b/spec/services/notification_recipient_service_spec.rb new file mode 100644 index 00000000000..7d826377579 --- /dev/null +++ b/spec/services/notification_recipient_service_spec.rb @@ -0,0 +1,22 @@ +require 'spec_helper' + +describe NotificationRecipientService do + describe 'build_recipients' do + it 'due_date' do + # These folks are being filtered out because they can't receive notifications + # notification_recipient.rb#85 + user = create(:user) + assignee = create(:user) + issue = create(:issue, :opened, due_date: Date.today, author: user, assignees: [assignee]) + + recipients = described_class.build_recipients( + issue, + issue.author, + action: "due_date", + skip_current_user: false + ) + + expect(recipients).to match_array([user, assignee]) + end + end +end -- cgit v1.2.1 From ddb23d3b2ba7c646cff6a5d21957194fc3474418 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 30 Mar 2018 12:58:06 +0100 Subject: Send issue due emails to all participants --- .../notification_recipient_service_spec.rb | 22 --------------- spec/services/notification_service_spec.rb | 31 ++++++++++++++++++++++ 2 files changed, 31 insertions(+), 22 deletions(-) delete mode 100644 spec/services/notification_recipient_service_spec.rb (limited to 'spec/services') diff --git a/spec/services/notification_recipient_service_spec.rb b/spec/services/notification_recipient_service_spec.rb deleted file mode 100644 index 7d826377579..00000000000 --- a/spec/services/notification_recipient_service_spec.rb +++ /dev/null @@ -1,22 +0,0 @@ -require 'spec_helper' - -describe NotificationRecipientService do - describe 'build_recipients' do - it 'due_date' do - # These folks are being filtered out because they can't receive notifications - # notification_recipient.rb#85 - user = create(:user) - assignee = create(:user) - issue = create(:issue, :opened, due_date: Date.today, author: user, assignees: [assignee]) - - recipients = described_class.build_recipients( - issue, - issue.author, - action: "due_date", - skip_current_user: false - ) - - expect(recipients).to match_array([user, assignee]) - end - end -end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 3943148f0db..1d117d9abc7 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -933,6 +933,37 @@ describe NotificationService, :mailer do let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) } end end + + describe '#issue_due' do + it 'sends email to issue notification recipients' do + notification.issue_due(issue) + + should_email(issue.assignees.first) + should_email(issue.author) + should_email(@u_watcher) + should_email(@u_guest_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_email(@watcher_and_subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@u_lazy_participant) + end + + it 'sends the email from the author' do + notification.issue_due(issue) + email = find_email_for(@subscriber) + + expect(email.header[:from].display_names).to eq([issue.author.name]) + end + + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { issue } + let(:notification_trigger) { notification.issue_due(issue) } + end + end end describe 'Merge Requests' do -- cgit v1.2.1 From 5ab75649f3ea00b64cb63e7e5283100c6b70cfb5 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 30 Mar 2018 13:25:46 +0100 Subject: Only send issue due emails to participants and custom subscribers --- spec/services/notification_service_spec.rb | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) (limited to 'spec/services') diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index e7c7706b484..cd10a13814e 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -935,16 +935,23 @@ describe NotificationService, :mailer do end describe '#issue_due' do - it 'sends email to issue notification recipients' do + before do + update_custom_notification(:issue_due, @u_guest_custom, resource: project) + update_custom_notification(:issue_due, @u_custom_global) + end + + it 'sends email to issue notification recipients, excluding watchers' do notification.issue_due(issue) should_email(issue.assignees.first) should_email(issue.author) - should_email(@u_watcher) - should_email(@u_guest_watcher) + should_email(@u_guest_custom) + should_email(@u_custom_global) should_email(@u_participant_mentioned) should_email(@subscriber) should_email(@watcher_and_subscriber) + should_not_email(@u_watcher) + should_not_email(@u_guest_watcher) should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_disabled) -- cgit v1.2.1 From f770e0fba5e9eb2ad0682b8e090b532e8e8754be Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 13 Mar 2018 19:33:46 +0100 Subject: Partition job_queue_duration_seconds with jobs_running_for_project --- spec/services/ci/register_job_service_spec.rb | 83 ++++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 2 deletions(-) (limited to 'spec/services') diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 97a563c1ce1..aa7cc268dd7 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -370,10 +370,89 @@ module Ci it_behaves_like 'validation is not active' end end + end - def execute(runner) - described_class.new(runner).execute.build + describe '#register_success' do + let!(:current_time) { Time.new(2018, 4, 5, 14, 0, 0) } + let!(:attempt_counter) { double('Gitlab::Metrics::NullMetric') } + let!(:job_queue_duration_seconds) { double('Gitlab::Metrics::NullMetric') } + + before do + allow(Time).to receive(:now).and_return(current_time) + + # Stub defaults for any metrics other than the ones we're testing + allow(Gitlab::Metrics).to receive(:counter) + .with(any_args) + .and_return(Gitlab::Metrics::NullMetric.instance) + allow(Gitlab::Metrics).to receive(:histogram) + .with(any_args) + .and_return(Gitlab::Metrics::NullMetric.instance) + + # Stub tested metrics + allow(Gitlab::Metrics).to receive(:counter) + .with(:job_register_attempts_total, anything) + .and_return(attempt_counter) + allow(Gitlab::Metrics).to receive(:histogram) + .with(:job_queue_duration_seconds, anything, anything, anything) + .and_return(job_queue_duration_seconds) + + project.update(shared_runners_enabled: true) + pending_job.update(created_at: current_time - 3600, queued_at: current_time - 1800) end + + shared_examples 'metrics collector' do + it 'increments attempt counter' do + allow(job_queue_duration_seconds).to receive(:observe) + expect(attempt_counter).to receive(:increment) + + execute(runner) + end + + it 'counts job queuing time histogram with expected labels' do + allow(attempt_counter).to receive(:increment) + expect(job_queue_duration_seconds).to receive(:observe) + .with({ shared_runner: expected_shared_runner, + jobs_running_for_project: expected_jobs_running_for_project_first_job }, 1800) + + execute(runner) + end + + context 'when project already has running jobs' do + let!(:build2) { create( :ci_build, :running, pipeline: pipeline, runner: shared_runner) } + let!(:build3) { create( :ci_build, :running, pipeline: pipeline, runner: shared_runner) } + + it 'counts job queuing time histogram with expected labels' do + allow(attempt_counter).to receive(:increment) + expect(job_queue_duration_seconds).to receive(:observe) + .with({ shared_runner: expected_shared_runner, + jobs_running_for_project: expected_jobs_running_for_project_third_job }, 1800) + + execute(runner) + end + end + end + + context 'when shared runner is used' do + let(:runner) { shared_runner } + let(:expected_shared_runner) { true } + let(:expected_jobs_running_for_project_first_job) { 0 } + let(:expected_jobs_running_for_project_third_job) { 2 } + + it_behaves_like 'metrics collector' + end + + context 'when specific runner is used' do + let(:runner) { specific_runner } + let(:expected_shared_runner) { false } + let(:expected_jobs_running_for_project_first_job) { '+Inf' } + let(:expected_jobs_running_for_project_third_job) { '+Inf' } + + it_behaves_like 'metrics collector' + end + end + + def execute(runner) + described_class.new(runner).execute.build end end end -- cgit v1.2.1 From 067f5f11c2f5c987697a78a42f23db1db1a90856 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Tue, 10 Apr 2018 15:13:09 +0200 Subject: Protect register_job_service from pending job with queued_at=nil --- spec/services/ci/register_job_service_spec.rb | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) (limited to 'spec/services') diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index aa7cc268dd7..8a537e83d5f 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -400,14 +400,16 @@ module Ci pending_job.update(created_at: current_time - 3600, queued_at: current_time - 1800) end - shared_examples 'metrics collector' do + shared_examples 'attempt counter collector' do it 'increments attempt counter' do allow(job_queue_duration_seconds).to receive(:observe) expect(attempt_counter).to receive(:increment) execute(runner) end + end + shared_examples 'jobs queueing time histogram collector' do it 'counts job queuing time histogram with expected labels' do allow(attempt_counter).to receive(:increment) expect(job_queue_duration_seconds).to receive(:observe) @@ -432,6 +434,11 @@ module Ci end end + shared_examples 'metrics collector' do + it_behaves_like 'attempt counter collector' + it_behaves_like 'jobs queueing time histogram collector' + end + context 'when shared runner is used' do let(:runner) { shared_runner } let(:expected_shared_runner) { true } @@ -439,6 +446,21 @@ module Ci let(:expected_jobs_running_for_project_third_job) { 2 } it_behaves_like 'metrics collector' + + context 'when pending job with queued_at=nil is used' do + before do + pending_job.update(queued_at: nil) + end + + it_behaves_like 'attempt counter collector' + + it "doesn't count job queuing time histogram" do + allow(attempt_counter).to receive(:increment) + expect(job_queue_duration_seconds).not_to receive(:observe) + + execute(runner) + end + end end context 'when specific runner is used' do -- cgit v1.2.1 From a6ddeb54dcdce073bf4cb92110ab9f462945c2ff Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Wed, 11 Apr 2018 16:21:36 -0500 Subject: Add a comma to the time system notes estimates --- spec/services/system_note_service_spec.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'spec/services') diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index e28b0ea5cf2..893804f1470 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -909,7 +909,13 @@ describe SystemNoteService do it 'sets the note text' do noteable.update_attribute(:time_estimate, 277200) - expect(subject.note).to eq "changed time estimate to 1w 4d 5h" + expect(subject.note).to eq "changed time estimate to 1w 4d 5h," + end + + it 'appends a comma to separate the note from the update_at time' do + noteable.update_attribute(:time_estimate, 277200) + + expect(subject.note).to end_with(',') end end -- cgit v1.2.1 From fd4a08203448d5ade7471549fe17169fb19c6484 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 17 Apr 2018 12:44:22 +0100 Subject: Make issue due email more consistent with other mailers --- spec/services/notification_service_spec.rb | 2 ++ 1 file changed, 2 insertions(+) (limited to 'spec/services') diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index cd10a13814e..55bbe954491 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -936,6 +936,8 @@ describe NotificationService, :mailer do describe '#issue_due' do before do + issue.update!(due_date: Date.today) + update_custom_notification(:issue_due, @u_guest_custom, resource: project) update_custom_notification(:issue_due, @u_custom_global) end -- cgit v1.2.1 From 470b3cb5a1009f63265efbd1a049ae7677b6c5c4 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Thu, 19 Apr 2018 08:17:12 +0000 Subject: Fix label links update on project transfer --- spec/services/labels/transfer_service_spec.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'spec/services') diff --git a/spec/services/labels/transfer_service_spec.rb b/spec/services/labels/transfer_service_spec.rb index ae819c011de..80bac590a11 100644 --- a/spec/services/labels/transfer_service_spec.rb +++ b/spec/services/labels/transfer_service_spec.rb @@ -8,6 +8,7 @@ describe Labels::TransferService do let(:group_3) { create(:group) } let(:project_1) { create(:project, namespace: group_2) } let(:project_2) { create(:project, namespace: group_3) } + let(:project_3) { create(:project, namespace: group_1) } let(:group_label_1) { create(:group_label, group: group_1, name: 'Group Label 1') } let(:group_label_2) { create(:group_label, group: group_1, name: 'Group Label 2') } @@ -23,6 +24,7 @@ describe Labels::TransferService do create(:labeled_issue, project: project_1, labels: [group_label_4]) create(:labeled_issue, project: project_1, labels: [project_label_1]) create(:labeled_issue, project: project_2, labels: [group_label_5]) + create(:labeled_issue, project: project_3, labels: [group_label_1]) create(:labeled_merge_request, source_project: project_1, labels: [group_label_1, group_label_2]) create(:labeled_merge_request, source_project: project_2, labels: [group_label_5]) end @@ -52,5 +54,13 @@ describe Labels::TransferService do expect(project_1.labels.where(title: group_label_4.title)).to be_empty end + + it 'updates only label links in the given project' do + service.execute + + targets = LabelLink.where(label_id: group_label_1.id).map(&:target) + + expect(targets).to eq(project_3.issues) + end end end -- cgit v1.2.1 From 92deabb34ae20a2a10f70c2de316f8f235f08e49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Thu, 19 Apr 2018 15:24:32 +0000 Subject: Respect visibility options and description when importing project from template --- .../projects/create_from_template_service_spec.rb | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) (limited to 'spec/services') diff --git a/spec/services/projects/create_from_template_service_spec.rb b/spec/services/projects/create_from_template_service_spec.rb index 609d678caea..d40e6f1449d 100644 --- a/spec/services/projects/create_from_template_service_spec.rb +++ b/spec/services/projects/create_from_template_service_spec.rb @@ -7,7 +7,7 @@ describe Projects::CreateFromTemplateService do path: user.to_param, template_name: 'rails', description: 'project description', - visibility_level: Gitlab::VisibilityLevel::PRIVATE + visibility_level: Gitlab::VisibilityLevel::PUBLIC } end @@ -24,7 +24,23 @@ describe Projects::CreateFromTemplateService do expect(project).to be_saved expect(project.scheduled?).to be(true) - expect(project.description).to match('project description') - expect(project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) + end + + context 'the result project' do + before do + Sidekiq::Testing.inline! do + @project = subject.execute + end + + @project.reload + end + + it 'overrides template description' do + expect(@project.description).to match('project description') + end + + it 'overrides template visibility_level' do + expect(@project.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) + end end end -- cgit v1.2.1 From ab286656b22dd686a659afe908daade6e5a54ff3 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 23 Apr 2018 15:48:26 +0000 Subject: Resolve "Namespace factory is problematic" --- spec/services/groups/nested_create_service_spec.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'spec/services') diff --git a/spec/services/groups/nested_create_service_spec.rb b/spec/services/groups/nested_create_service_spec.rb index 6491fb34777..86fdd43c1e5 100644 --- a/spec/services/groups/nested_create_service_spec.rb +++ b/spec/services/groups/nested_create_service_spec.rb @@ -59,8 +59,11 @@ describe Groups::NestedCreateService do describe "#execute" do it 'returns the group if it already existed' do - parent = create(:group, path: 'a-group', owner: user) - child = create(:group, path: 'a-sub-group', parent: parent, owner: user) + parent = create(:group, path: 'a-group') + child = create(:group, path: 'a-sub-group', parent: parent) + + parent.add_owner(user) + child.add_owner(user) expect(service.execute).to eq(child) end -- cgit v1.2.1 From 35a49922e66ed9dc55685163126e1bee0a4e3dec Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 18 Apr 2018 15:52:55 +0200 Subject: Allow admins to push to empty repos --- spec/services/ci/retry_pipeline_service_spec.rb | 2 ++ 1 file changed, 2 insertions(+) (limited to 'spec/services') diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index 6ce75c65c8c..f1acfc48468 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -235,6 +235,8 @@ describe Ci::RetryPipelineService, '#execute' do context 'when user is not allowed to trigger manual action' do before do project.add_developer(user) + create(:protected_branch, :masters_can_push, + name: pipeline.ref, project: project) end context 'when there is a failed manual action present' do -- cgit v1.2.1 From db3774376aafbdea8db6840c88bd6d875cb9bac3 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 20 Apr 2018 08:28:17 +0100 Subject: Make /copy_metadata only handle the first issuable passed --- spec/services/quick_actions/interpret_service_spec.rb | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) (limited to 'spec/services') diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 4aad2aaef79..bd835a1fca6 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -775,8 +775,8 @@ describe QuickActions::InterpretService do end context '/copy_metadata command' do - let!(:todo_label) { create(:label, project: project, title: 'To Do') } - let!(:inreview_label) { create(:label, project: project, title: 'In Review') } + let(:todo_label) { create(:label, project: project, title: 'To Do') } + let(:inreview_label) { create(:label, project: project, title: 'In Review') } it_behaves_like 'empty command' do let(:content) { '/copy_metadata' } @@ -799,6 +799,17 @@ describe QuickActions::InterpretService do end end + context 'when more than one issuable is passed' do + it_behaves_like 'copy_metadata command' do + let(:source_issuable) { create(:labeled_issue, project: project, labels: [inreview_label, todo_label]) } + let(:other_label) { create(:label, project: project, title: 'Other') } + let(:other_source_issuable) { create(:labeled_issue, project: project, labels: [other_label]) } + + let(:content) { "/copy_metadata #{source_issuable.to_reference} #{other_source_issuable.to_reference}" } + let(:issuable) { issue } + end + end + context 'cross project references' do it_behaves_like 'empty command' do let(:other_project) { create(:project, :public) } -- cgit v1.2.1 From ec4423665cacfe2f0675fb8b9b5bd8dd17a77dd7 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Fri, 13 Apr 2018 12:57:19 +0200 Subject: Gitlab::Shell works on shard name, not path Direct disk access is done through Gitaly now, so the legacy path was deprecated. This path was used in Gitlab::Shell however. This required the refactoring in this commit. Added is the removal of direct path access on the project model, as that lookup wasn't needed anymore is most cases. Closes https://gitlab.com/gitlab-org/gitaly/issues/1111 --- spec/services/groups/destroy_service_spec.rb | 16 ++++++++-------- spec/services/projects/create_service_spec.rb | 5 ++--- spec/services/projects/destroy_service_spec.rb | 16 ++++++++-------- spec/services/projects/fork_service_spec.rb | 2 +- .../hashed_storage/migrate_repository_service_spec.rb | 14 +++++++------- spec/services/projects/transfer_service_spec.rb | 6 +++--- spec/services/projects/update_service_spec.rb | 2 +- spec/services/users/destroy_service_spec.rb | 4 ++-- 8 files changed, 32 insertions(+), 33 deletions(-) (limited to 'spec/services') diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index e8216abb08b..a9baccd061a 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -53,8 +53,8 @@ describe Groups::DestroyService do end it 'verifies that paths have been deleted' do - expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey - expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey + expect(gitlab_shell.exists?(project.repository_storage, group.path)).to be_falsey + expect(gitlab_shell.exists?(project.repository_storage, remove_path)).to be_falsey end end end @@ -71,13 +71,13 @@ describe Groups::DestroyService do after do # Clean up stale directories - gitlab_shell.rm_namespace(project.repository_storage_path, group.path) - gitlab_shell.rm_namespace(project.repository_storage_path, remove_path) + gitlab_shell.rm_namespace(project.repository_storage, group.path) + gitlab_shell.rm_namespace(project.repository_storage, remove_path) end it 'verifies original paths and projects still exist' do - expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_truthy - expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey + expect(gitlab_shell.exists?(project.repository_storage, group.path)).to be_truthy + expect(gitlab_shell.exists?(project.repository_storage, remove_path)).to be_falsey expect(Project.unscoped.count).to eq(1) expect(Group.unscoped.count).to eq(2) end @@ -144,7 +144,7 @@ describe Groups::DestroyService do let!(:project) { create(:project, :legacy_storage, :empty_repo, namespace: group) } it 'removes repository' do - expect(gitlab_shell.exists?(project.repository_storage_path, "#{project.disk_path}.git")).to be_falsey + expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey end end @@ -152,7 +152,7 @@ describe Groups::DestroyService do let!(:project) { create(:project, :empty_repo, namespace: group) } it 'removes repository' do - expect(gitlab_shell.exists?(project.repository_storage_path, "#{project.disk_path}.git")).to be_falsey + expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey end end end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index e35f0f6337a..a8f003b1073 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -171,7 +171,6 @@ describe Projects::CreateService, '#execute' do context 'when another repository already exists on disk' do let(:repository_storage) { 'default' } - let(:repository_storage_path) { Gitlab.config.repositories.storages[repository_storage].legacy_disk_path } let(:opts) do { @@ -186,7 +185,7 @@ describe Projects::CreateService, '#execute' do end after do - gitlab_shell.remove_repository(repository_storage_path, "#{user.namespace.full_path}/existing") + gitlab_shell.remove_repository(repository_storage, "#{user.namespace.full_path}/existing") end it 'does not allow to create a project when path matches existing repository on disk' do @@ -222,7 +221,7 @@ describe Projects::CreateService, '#execute' do end after do - gitlab_shell.remove_repository(repository_storage_path, hashed_path) + gitlab_shell.remove_repository(repository_storage, hashed_path) end it 'does not allow to create a project when path matches existing repository on disk' do diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index a66e3c5e995..b2c52214f48 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -18,8 +18,8 @@ describe Projects::DestroyService do it 'deletes the project' do expect(Project.unscoped.all).not_to include(project) - expect(project.gitlab_shell.exists?(project.repository_storage_path, path + '.git')).to be_falsey - expect(project.gitlab_shell.exists?(project.repository_storage_path, remove_path + '.git')).to be_falsey + expect(project.gitlab_shell.exists?(project.repository_storage, path + '.git')).to be_falsey + expect(project.gitlab_shell.exists?(project.repository_storage, remove_path + '.git')).to be_falsey end end @@ -252,21 +252,21 @@ describe Projects::DestroyService do let(:path) { project.disk_path + '.git' } before do - expect(project.gitlab_shell.exists?(project.repository_storage_path, path)).to be_truthy - expect(project.gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey + expect(project.gitlab_shell.exists?(project.repository_storage, path)).to be_truthy + expect(project.gitlab_shell.exists?(project.repository_storage, remove_path)).to be_falsey # Dont run sidekiq to check if renamed repository exists Sidekiq::Testing.fake! { destroy_project(project, user, {}) } - expect(project.gitlab_shell.exists?(project.repository_storage_path, path)).to be_falsey - expect(project.gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy + expect(project.gitlab_shell.exists?(project.repository_storage, path)).to be_falsey + expect(project.gitlab_shell.exists?(project.repository_storage, remove_path)).to be_truthy end it 'restores the repositories' do Sidekiq::Testing.fake! { described_class.new(project, user).attempt_repositories_rollback } - expect(project.gitlab_shell.exists?(project.repository_storage_path, path)).to be_truthy - expect(project.gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey + expect(project.gitlab_shell.exists?(project.repository_storage, path)).to be_truthy + expect(project.gitlab_shell.exists?(project.repository_storage, remove_path)).to be_falsey end end diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 0f7c46367d0..a93f6f1ddc2 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -112,7 +112,7 @@ describe Projects::ForkService do end after do - gitlab_shell.remove_repository(repository_storage_path, "#{@to_user.namespace.full_path}/#{@from_project.path}") + gitlab_shell.remove_repository(repository_storage, "#{@to_user.namespace.full_path}/#{@from_project.path}") end it 'does not allow creation' do diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb index 747bd4529a0..7dca81eb59e 100644 --- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -16,8 +16,8 @@ describe Projects::HashedStorage::MigrateRepositoryService do it 'renames project and wiki repositories' do service.execute - expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.git")).to be_truthy - expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.wiki.git")).to be_truthy + expect(gitlab_shell.exists?(project.repository_storage, "#{hashed_storage.disk_path}.git")).to be_truthy + expect(gitlab_shell.exists?(project.repository_storage, "#{hashed_storage.disk_path}.wiki.git")).to be_truthy end it 'updates project to be hashed and not read-only' do @@ -52,8 +52,8 @@ describe Projects::HashedStorage::MigrateRepositoryService do service.execute - expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.git")).to be_falsey - expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.wiki.git")).to be_falsey + expect(gitlab_shell.exists?(project.repository_storage, "#{hashed_storage.disk_path}.git")).to be_falsey + expect(gitlab_shell.exists?(project.repository_storage, "#{hashed_storage.disk_path}.wiki.git")).to be_falsey expect(project.repository_read_only?).to be_falsey end @@ -63,11 +63,11 @@ describe Projects::HashedStorage::MigrateRepositoryService do before do hashed_storage.ensure_storage_path_exists - gitlab_shell.mv_repository(project.repository_storage_path, from_name, to_name) + gitlab_shell.mv_repository(project.repository_storage, from_name, to_name) end it 'does not try to move nil repository over hashed' do - expect(gitlab_shell).not_to receive(:mv_repository).with(project.repository_storage_path, from_name, to_name) + expect(gitlab_shell).not_to receive(:mv_repository).with(project.repository_storage, from_name, to_name) expect_move_repository("#{project.disk_path}.wiki", "#{hashed_storage.disk_path}.wiki") service.execute @@ -76,7 +76,7 @@ describe Projects::HashedStorage::MigrateRepositoryService do end def expect_move_repository(from_name, to_name) - expect(gitlab_shell).to receive(:mv_repository).with(project.repository_storage_path, from_name, to_name).and_call_original + expect(gitlab_shell).to receive(:mv_repository).with(project.repository_storage, from_name, to_name).and_call_original end end end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index ff9b2372a35..3e6483d7e28 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -84,7 +84,7 @@ describe Projects::TransferService do end def project_path(project) - File.join(project.repository_storage_path, "#{project.disk_path}.git") + project.repository.path_to_repo end def current_path @@ -94,7 +94,7 @@ describe Projects::TransferService do it 'rolls back repo location' do attempt_project_transfer - expect(Dir.exist?(original_path)).to be_truthy + expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be(true) expect(original_path).to eq current_path end @@ -165,7 +165,7 @@ describe Projects::TransferService do end after do - gitlab_shell.remove_repository(repository_storage_path, "#{group.full_path}/#{project.path}") + gitlab_shell.remove_repository(repository_storage, "#{group.full_path}/#{project.path}") end it { expect(@result).to eq false } diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index f48d466d263..3e6073b9861 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -200,7 +200,7 @@ describe Projects::UpdateService do end after do - gitlab_shell.remove_repository(repository_storage_path, "#{user.namespace.full_path}/existing") + gitlab_shell.remove_repository(repository_storage, "#{user.namespace.full_path}/existing") end it 'does not allow renaming when new path matches existing repository on disk' do diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index 11c75ddfcf8..76f1e625fda 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -176,7 +176,7 @@ describe Users::DestroyService do let!(:project) { create(:project, :empty_repo, :legacy_storage, namespace: user.namespace) } it 'removes repository' do - expect(gitlab_shell.exists?(project.repository_storage_path, "#{project.disk_path}.git")).to be_falsey + expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey end end @@ -184,7 +184,7 @@ describe Users::DestroyService do let!(:project) { create(:project, :empty_repo, namespace: user.namespace) } it 'removes repository' do - expect(gitlab_shell.exists?(project.repository_storage_path, "#{project.disk_path}.git")).to be_falsey + expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey end end end -- cgit v1.2.1 From b5042e5301e86ec7822221ee29679b0fbf5c71ca Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 20 Apr 2018 19:37:38 +0200 Subject: Move NotificationService calls to Sidekiq The NotificationService has to do quite a lot of work to calculate the recipients for an email. Where possible, we should try to avoid doing this in an HTTP request, because the mail are sent by Sidekiq anyway, so there's no need to schedule those emails immediately. This commit creates a generic Sidekiq worker that uses Global ID to serialise and deserialise its arguments, then forwards them to the NotificationService. The NotificationService gains an `#async` method, so you can replace: notification_service.new_issue(issue, current_user) With: notification_service.async.new_issue(issue, current_user) And have everything else work as normal, except that calculating the recipients will be done by Sidekiq, which will then schedule further Sidekiq jobs to send each email. --- spec/services/notification_service_spec.rb | 42 +++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-) (limited to 'spec/services') diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 55bbe954491..48ef5f3c115 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -96,6 +96,37 @@ describe NotificationService, :mailer do it_should_behave_like 'participating by assignee notification' end + describe '#async' do + let(:async) { notification.async } + set(:key) { create(:personal_key) } + + it 'returns an Async object with the correct parent' do + expect(async).to be_a(described_class::Async) + expect(async.parent).to eq(notification) + end + + context 'when receiving a public method' do + it 'schedules a MailScheduler::NotificationServiceWorker' do + expect(MailScheduler::NotificationServiceWorker) + .to receive(:perform_async).with('new_key', key) + + async.new_key(key) + end + end + + context 'when receiving a private method' do + it 'raises NoMethodError' do + expect { async.notifiable?(key) }.to raise_error(NoMethodError) + end + end + + context 'when recieving a non-existent method' do + it 'raises NoMethodError' do + expect { async.foo(key) }.to raise_error(NoMethodError) + end + end + end + describe 'Keys' do describe '#new_key' do let(:key_options) { {} } @@ -982,6 +1013,8 @@ describe NotificationService, :mailer do let(:merge_request) { create :merge_request, source_project: project, assignee: create(:user), description: 'cc @participant' } before do + project.add_master(merge_request.author) + project.add_master(merge_request.assignee) build_team(merge_request.target_project) add_users_with_subscription(merge_request.target_project, merge_request) update_custom_notification(:new_merge_request, @u_guest_custom, resource: project) @@ -1093,15 +1126,18 @@ describe NotificationService, :mailer do end describe '#reassigned_merge_request' do + let(:current_user) { create(:user) } + before do update_custom_notification(:reassign_merge_request, @u_guest_custom, resource: project) update_custom_notification(:reassign_merge_request, @u_custom_global) end it do - notification.reassigned_merge_request(merge_request, merge_request.author) + notification.reassigned_merge_request(merge_request, current_user, merge_request.author) should_email(merge_request.assignee) + should_email(merge_request.author) should_email(@u_watcher) should_email(@u_participant_mentioned) should_email(@subscriber) @@ -1116,7 +1152,7 @@ describe NotificationService, :mailer do end it 'adds "assigned" reason for new assignee' do - notification.reassigned_merge_request(merge_request, merge_request.author) + notification.reassigned_merge_request(merge_request, current_user, merge_request.author) email = find_email_for(merge_request.assignee) @@ -1126,7 +1162,7 @@ describe NotificationService, :mailer do it_behaves_like 'participating notifications' do let(:participant) { create(:user, username: 'user-participant') } let(:issuable) { merge_request } - let(:notification_trigger) { notification.reassigned_merge_request(merge_request, @u_disabled) } + let(:notification_trigger) { notification.reassigned_merge_request(merge_request, current_user, merge_request.author) } end end -- cgit v1.2.1 From bb650ad05240a54497599b6a884ac0f7f8fa1629 Mon Sep 17 00:00:00 2001 From: Jose Date: Wed, 25 Apr 2018 15:56:06 -0500 Subject: Remove comma from the time system notes --- spec/services/system_note_service_spec.rb | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) (limited to 'spec/services') diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 893804f1470..e28b0ea5cf2 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -909,13 +909,7 @@ describe SystemNoteService do it 'sets the note text' do noteable.update_attribute(:time_estimate, 277200) - expect(subject.note).to eq "changed time estimate to 1w 4d 5h," - end - - it 'appends a comma to separate the note from the update_at time' do - noteable.update_attribute(:time_estimate, 277200) - - expect(subject.note).to end_with(',') + expect(subject.note).to eq "changed time estimate to 1w 4d 5h" end end -- cgit v1.2.1 From ad113a3cf803f95950be1a2af12eb3ad6f512ecd Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Fri, 27 Apr 2018 18:30:52 +0100 Subject: Don't automatically remove artifacts for pages jobs after pages:deploy has run --- .../services/projects/update_pages_service_spec.rb | 51 ++++------------------ 1 file changed, 9 insertions(+), 42 deletions(-) (limited to 'spec/services') diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 1b6caeab15d..a418808fd26 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -29,25 +29,10 @@ describe Projects::UpdatePagesService do end describe 'pages artifacts' do - context 'with expiry date' do - before do - build.artifacts_expire_in = "2 days" - build.save! - end - - it "doesn't delete artifacts" do - expect(execute).to eq(:success) - - expect(build.reload.artifacts?).to eq(true) - end - end - - context 'without expiry date' do - it "does delete artifacts" do - expect(execute).to eq(:success) + it "doesn't delete artifacts after deploying" do + expect(execute).to eq(:success) - expect(build.reload.artifacts?).to eq(false) - end + expect(build.reload.artifacts?).to eq(true) end end @@ -100,25 +85,10 @@ describe Projects::UpdatePagesService do end describe 'pages artifacts' do - context 'with expiry date' do - before do - build.artifacts_expire_in = "2 days" - build.save! - end - - it "doesn't delete artifacts" do - expect(execute).to eq(:success) - - expect(build.artifacts?).to eq(true) - end - end - - context 'without expiry date' do - it "does delete artifacts" do - expect(execute).to eq(:success) + it "doesn't delete artifacts after deploying" do + expect(execute).to eq(:success) - expect(build.reload.artifacts?).to eq(false) - end + expect(build.artifacts?).to eq(true) end end @@ -171,13 +141,12 @@ describe Projects::UpdatePagesService do build.reload expect(deploy_status).to be_failed - expect(build.artifacts?).to be_truthy end end context 'when failed to extract zip artifacts' do before do - allow_any_instance_of(described_class) + expect_any_instance_of(described_class) .to receive(:extract_zip_archive!) .and_raise(Projects::UpdatePagesService::FailedToExtractError) end @@ -188,21 +157,19 @@ describe Projects::UpdatePagesService do build.reload expect(deploy_status).to be_failed - expect(build.artifacts?).to be_truthy end end context 'when missing artifacts metadata' do before do - allow(build).to receive(:artifacts_metadata?).and_return(false) + expect(build).to receive(:artifacts_metadata?).and_return(false) end - it 'does not raise an error and remove artifacts as failed job' do + it 'does not raise an error as failed job' do execute build.reload expect(deploy_status).to be_failed - expect(build.artifacts?).to be_falsey end end end -- cgit v1.2.1 From 9cf4e4734192c7234a97f1a7f472eed3ce7a2448 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 25 Apr 2018 08:22:43 +0000 Subject: Merge branch 'security-45689-fix-archive-cache-bug' into 'security-10-7' Serve archive requests with the correct file in all cases (10.7) See merge request gitlab/gitlabhq!2376 --- .../repository_archive_clean_up_service_spec.rb | 68 ++++++++++++++++------ 1 file changed, 50 insertions(+), 18 deletions(-) (limited to 'spec/services') diff --git a/spec/services/repository_archive_clean_up_service_spec.rb b/spec/services/repository_archive_clean_up_service_spec.rb index 2d7fa3f80f7..ab1c638fc39 100644 --- a/spec/services/repository_archive_clean_up_service_spec.rb +++ b/spec/services/repository_archive_clean_up_service_spec.rb @@ -1,15 +1,47 @@ require 'spec_helper' describe RepositoryArchiveCleanUpService do - describe '#execute' do - subject(:service) { described_class.new } + subject(:service) { described_class.new } + describe '#execute (new archive locations)' do + let(:sha) { "0" * 40 } + + it 'removes outdated archives and directories in a new-style path' do + in_directory_with_files("project-999/#{sha}", %w[tar tar.bz2 tar.gz zip], 3.hours) do |dirname, files| + service.execute + + files.each { |filename| expect(File.exist?(filename)).to be_falsy } + expect(File.directory?(dirname)).to be_falsy + expect(File.directory?(File.dirname(dirname))).to be_falsy + end + end + + it 'does not remove directories when they contain outdated non-archives' do + in_directory_with_files("project-999/#{sha}", %w[tar conf rb], 2.hours) do |dirname, files| + service.execute + + expect(File.directory?(dirname)).to be_truthy + end + end + + it 'does not remove in-date archives in a new-style path' do + in_directory_with_files("project-999/#{sha}", %w[tar tar.bz2 tar.gz zip], 1.hour) do |dirname, files| + service.execute + + files.each { |filename| expect(File.exist?(filename)).to be_truthy } + end + end + end + + describe '#execute (legacy archive locations)' do context 'when the downloads directory does not exist' do it 'does not remove any archives' do path = '/invalid/path/' stub_repository_downloads_path(path) + allow(File).to receive(:directory?).and_call_original expect(File).to receive(:directory?).with(path).and_return(false) + expect(service).not_to receive(:clean_up_old_archives) expect(service).not_to receive(:clean_up_empty_directories) @@ -19,7 +51,7 @@ describe RepositoryArchiveCleanUpService do context 'when the downloads directory exists' do shared_examples 'invalid archive files' do |dirname, extensions, mtime| - it 'does not remove files and directoy' do + it 'does not remove files and directory' do in_directory_with_files(dirname, extensions, mtime) do |dir, files| service.execute @@ -43,7 +75,7 @@ describe RepositoryArchiveCleanUpService do end context 'with files older than 2 hours inside invalid directories' do - it_behaves_like 'invalid archive files', 'john_doe/sample.git', %w[conf rb tar tar.gz], 2.hours + it_behaves_like 'invalid archive files', 'john/doe/sample.git', %w[conf rb tar tar.gz], 2.hours end context 'with files newer than 2 hours that matches valid archive extensions' do @@ -58,24 +90,24 @@ describe RepositoryArchiveCleanUpService do it_behaves_like 'invalid archive files', 'sample.git', %w[conf rb tar tar.gz], 1.hour end end + end - def in_directory_with_files(dirname, extensions, mtime) - Dir.mktmpdir do |tmpdir| - stub_repository_downloads_path(tmpdir) - dir = File.join(tmpdir, dirname) - files = create_temporary_files(dir, extensions, mtime) + def in_directory_with_files(dirname, extensions, mtime) + Dir.mktmpdir do |tmpdir| + stub_repository_downloads_path(tmpdir) + dir = File.join(tmpdir, dirname) + files = create_temporary_files(dir, extensions, mtime) - yield(dir, files) - end + yield(dir, files) end + end - def stub_repository_downloads_path(path) - allow(Gitlab.config.gitlab).to receive(:repository_downloads_path).and_return(path) - end + def stub_repository_downloads_path(path) + allow(Gitlab.config.gitlab).to receive(:repository_downloads_path).and_return(path) + end - def create_temporary_files(dir, extensions, mtime) - FileUtils.mkdir_p(dir) - FileUtils.touch(extensions.map { |ext| File.join(dir, "sample.#{ext}") }, mtime: Time.now - mtime) - end + def create_temporary_files(dir, extensions, mtime) + FileUtils.mkdir_p(dir) + FileUtils.touch(extensions.map { |ext| File.join(dir, "sample.#{ext}") }, mtime: Time.now - mtime) end end -- cgit v1.2.1