From 293999cabbceab43ca82c5178285d52dfb55cb08 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 5 Feb 2016 08:22:29 +0100 Subject: Fix name of build erasable, remove superfluous method from it --- app/models/ci/build.rb | 2 +- app/models/ci/build/erasable.rb | 38 ++++++++ app/models/ci/build/eraseable.rb | 44 --------- app/views/projects/builds/show.html.haml | 17 ++-- .../20160202091601_add_erasable_to_ci_build.rb | 6 ++ .../20160202091601_add_eraseable_to_ci_build.rb | 6 -- lib/api/builds.rb | 2 +- spec/models/ci/build/erasable_spec.rb | 97 +++++++++++++++++++ spec/models/ci/build/eraseable_spec.rb | 107 --------------------- spec/requests/api/builds_spec.rb | 4 +- 10 files changed, 154 insertions(+), 169 deletions(-) create mode 100644 app/models/ci/build/erasable.rb delete mode 100644 app/models/ci/build/eraseable.rb create mode 100644 db/migrate/20160202091601_add_erasable_to_ci_build.rb delete mode 100644 db/migrate/20160202091601_add_eraseable_to_ci_build.rb create mode 100644 spec/models/ci/build/erasable_spec.rb delete mode 100644 spec/models/ci/build/eraseable_spec.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 4e22ee3962b..9172a46b35f 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -38,7 +38,7 @@ module Ci class Build < CommitStatus include Gitlab::Application.routes.url_helpers - include Build::Eraseable + include Build::Erasable LAZY_ATTRIBUTES = ['trace'] diff --git a/app/models/ci/build/erasable.rb b/app/models/ci/build/erasable.rb new file mode 100644 index 00000000000..95e1bef58e4 --- /dev/null +++ b/app/models/ci/build/erasable.rb @@ -0,0 +1,38 @@ +module Ci + class Build + module Erasable + extend ActiveSupport::Concern + + included do + belongs_to :erased_by, class_name: 'User' + end + + def erase!(opts = {}) + raise StandardError, 'Build not erasable!' unless erasable? + + remove_artifacts_file! + remove_artifacts_metadata! + erase_trace! + update_erased!(opts[:erased_by]) + end + + def erasable? + complete? && (artifacts? || has_trace?) + end + + def erased? + !self.erased_at.nil? + end + + private + + def erase_trace! + self.trace = nil + end + + def update_erased!(user = nil) + self.update(erased_by: user, erased_at: Time.now) + end + end + end +end diff --git a/app/models/ci/build/eraseable.rb b/app/models/ci/build/eraseable.rb deleted file mode 100644 index 9d3d0627b45..00000000000 --- a/app/models/ci/build/eraseable.rb +++ /dev/null @@ -1,44 +0,0 @@ -module Ci - class Build - module Eraseable - extend ActiveSupport::Concern - - included do - belongs_to :erased_by, class_name: 'User' - end - - def erase!(opts = {}) - raise StandardError, 'Build not eraseable!' unless eraseable? - - remove_artifacts_file! - remove_artifacts_metadata! - erase_trace! - update_erased!(opts[:erased_by]) - end - - def eraseable? - complete? && (artifacts? || has_trace?) - end - - def erase_url - if eraseable? - erase_namespace_project_build_path(project.namespace, project, self) - end - end - - def erased? - !self.erased_at.nil? - end - - private - - def erase_trace! - self.trace = nil - end - - def update_erased!(user = nil) - self.update(erased_by: user, erased_at: Time.now) - end - end - end -end diff --git a/app/views/projects/builds/show.html.haml b/app/views/projects/builds/show.html.haml index 5809fa2d11b..0c1dc4bdeee 100644 --- a/app/views/projects/builds/show.html.haml +++ b/app/views/projects/builds/show.html.haml @@ -76,15 +76,15 @@ = link_to '#down-build-trace', class: 'btn' do %i.fa.fa-angle-down - - unless @build.erased? + - if @build.erased? + .erased.alert.alert-warning + - erased_by = "by #{@build.erased_by.username}" if @build.erased_by + Build has been erased #{erased_by} #{time_ago_with_tooltip(@build.erased_at)} + - else %pre.trace#build-trace %code.bash = preserve do = raw @build.trace_html - - else - .erased.alert.alert-warning - - erased_by = "by #{@build.erased_by.username}" if @build.erased_by - Build has been erased #{erased_by} #{time_ago_with_tooltip(@build.erased_at)} %div#down-build-trace @@ -119,9 +119,10 @@ - elsif @build.retry_url = link_to "Retry", @build.retry_url, class: 'btn btn-sm btn-primary', method: :post - - if @build.eraseable? - = link_to @build.erase_url, class: 'btn btn-sm btn-warning', method: :delete, - data: { confirm: 'Are you sure you want to erase this build?' } do + - if @build.erasable? + = link_to erase_namespace_project_build_path(@project.namespace, @project, @build), + class: 'btn btn-sm btn-warning', method: :delete, + data: { confirm: 'Are you sure you want to erase this build?' } do = icon('eraser') Erase diff --git a/db/migrate/20160202091601_add_erasable_to_ci_build.rb b/db/migrate/20160202091601_add_erasable_to_ci_build.rb new file mode 100644 index 00000000000..f9912f2274e --- /dev/null +++ b/db/migrate/20160202091601_add_erasable_to_ci_build.rb @@ -0,0 +1,6 @@ +class AddErasableToCiBuild < ActiveRecord::Migration + def change + add_reference :ci_builds, :erased_by, references: :users, index: true + add_column :ci_builds, :erased_at, :datetime + end +end diff --git a/db/migrate/20160202091601_add_eraseable_to_ci_build.rb b/db/migrate/20160202091601_add_eraseable_to_ci_build.rb deleted file mode 100644 index 3db7180c1ae..00000000000 --- a/db/migrate/20160202091601_add_eraseable_to_ci_build.rb +++ /dev/null @@ -1,6 +0,0 @@ -class AddEraseableToCiBuild < ActiveRecord::Migration - def change - add_reference :ci_builds, :erased_by, references: :users, index: true - add_column :ci_builds, :erased_at, :datetime - end -end diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 06ec03fc4e0..9b7a3f12d2b 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -136,7 +136,7 @@ module API build = get_build(params[:build_id]) return not_found!(build) unless build - return forbidden!('Build is not eraseable!') unless build.eraseable? + return forbidden!('Build is not erasable!') unless build.erasable? build.erase! present build, with: Entities::Build, diff --git a/spec/models/ci/build/erasable_spec.rb b/spec/models/ci/build/erasable_spec.rb new file mode 100644 index 00000000000..7cf00748dca --- /dev/null +++ b/spec/models/ci/build/erasable_spec.rb @@ -0,0 +1,97 @@ +require 'spec_helper' + +describe Ci::Build::Erasable, models: true do + shared_examples 'erasable' do + it 'should remove artifact file' do + expect(build.artifacts_file.exists?).to be_falsy + end + + it 'should remove artifact metadata file' do + expect(build.artifacts_metadata.exists?).to be_falsy + end + + it 'should erase build trace in trace file' do + expect(build.trace).to be_empty + end + + it 'should set erased to true' do + expect(build.erased?).to be true + end + + it 'should set erase date' do + expect(build.erased_at).to_not be_falsy + end + end + + context 'build is not erasable' do + let!(:build) { create(:ci_build) } + + describe '#erase!' do + it { expect { build.erase! }.to raise_error(StandardError, /Build not erasable!/ )} + end + + describe '#erasable?' do + subject { build.erasable? } + it { is_expected.to eq false } + end + end + + context 'build is erasable' do + let!(:build) { create(:ci_build_with_trace, :success, :artifacts) } + + describe '#erase!' do + before { build.erase!(erased_by: user) } + + context 'erased by user' do + let!(:user) { create(:user, username: 'eraser') } + + include_examples 'erasable' + + it 'should record user who erased a build' do + expect(build.erased_by).to eq user + end + end + + context 'erased by system' do + let(:user) { nil } + + include_examples 'erasable' + + it 'should not set user who erased a build' do + expect(build.erased_by).to be_nil + end + end + end + + describe '#erasable?' do + subject { build.erasable? } + it { is_expected.to eq true } + end + + describe '#erased?' do + let!(:build) { create(:ci_build_with_trace, :success, :artifacts) } + subject { build.erased? } + + context 'build has not been erased' do + it { is_expected.to be false } + end + + context 'build has been erased' do + before { build.erase! } + + it { is_expected.to be true } + end + end + + context 'metadata and build trace are not available' do + let!(:build) { create(:ci_build, :success, :artifacts) } + before { build.remove_artifacts_metadata! } + + describe '#erase!' do + it 'should not raise error' do + expect { build.erase! }.to_not raise_error + end + end + end + end +end diff --git a/spec/models/ci/build/eraseable_spec.rb b/spec/models/ci/build/eraseable_spec.rb deleted file mode 100644 index 9e5099ce796..00000000000 --- a/spec/models/ci/build/eraseable_spec.rb +++ /dev/null @@ -1,107 +0,0 @@ -require 'spec_helper' - -describe Ci::Build::Eraseable, models: true do - shared_examples 'eraseable' do - it 'should remove artifact file' do - expect(build.artifacts_file.exists?).to be_falsy - end - - it 'should remove artifact metadata file' do - expect(build.artifacts_metadata.exists?).to be_falsy - end - - it 'should erase build trace in trace file' do - expect(build.trace).to be_empty - end - - it 'should set erased to true' do - expect(build.erased?).to be true - end - - it 'should set erase date' do - expect(build.erased_at).to_not be_falsy - end - end - - context 'build is not eraseable' do - let!(:build) { create(:ci_build) } - - describe '#erase!' do - it { expect { build.erase! }.to raise_error(StandardError, /Build not eraseable!/ )} - end - - describe '#eraseable?' do - subject { build.eraseable? } - it { is_expected.to eq false } - end - - describe '#erase_url' do - subject { build.erase_url } - it { is_expected.to be_falsy } - end - end - - context 'build is eraseable' do - let!(:build) { create(:ci_build_with_trace, :success, :artifacts) } - - describe '#erase!' do - before { build.erase!(erased_by: user) } - - context 'erased by user' do - let!(:user) { create(:user, username: 'eraser') } - - include_examples 'eraseable' - - it 'should record user who erased a build' do - expect(build.erased_by).to eq user - end - end - - context 'erased by system' do - let(:user) { nil } - - include_examples 'eraseable' - - it 'should not set user who erased a build' do - expect(build.erased_by).to be_nil - end - end - end - - describe '#eraseable?' do - subject { build.eraseable? } - it { is_expected.to eq true } - end - - describe '#erase_url' do - subject { build.erase_url } - it { is_expected.to be_truthy } - end - - describe '#erased?' do - let!(:build) { create(:ci_build_with_trace, :success, :artifacts) } - subject { build.erased? } - - context 'build has not been erased' do - it { is_expected.to be false } - end - - context 'build has been erased' do - before { build.erase! } - - it { is_expected.to be true } - end - end - - context 'metadata and build trace are not available' do - let!(:build) { create(:ci_build, :success, :artifacts) } - before { build.remove_artifacts_metadata! } - - describe '#erase!' do - it 'should not raise error' do - expect { build.erase! }.to_not raise_error - end - end - end - end -end diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index 56ab303268f..0ea6c2c7356 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -175,7 +175,7 @@ describe API::API, api: true do delete api("/projects/#{project.id}/builds/#{build.id}/content", user) end - context 'build is eraseable' do + context 'build is erasable' do let(:build) { create(:ci_build_with_trace, :artifacts, :success, project: project, commit: commit) } it 'should erase build content' do @@ -186,7 +186,7 @@ describe API::API, api: true do end end - context 'build is not eraseable' do + context 'build is not erasable' do let(:build) { create(:ci_build_with_trace, project: project, commit: commit) } it 'should respond with forbidden' do -- cgit v1.2.1