From 973e4030b13adbcc4eb7fad347b928a5164a04ff Mon Sep 17 00:00:00 2001
From: Tomasz Maczukin <tomasz@maczukin.pl>
Date: Thu, 22 Mar 2018 17:52:28 +0100
Subject: [PATCH] Refactor build_metadata

---
 .../jobs/components/sidebar_details_block.vue    | 12 +++++++++---
 app/models/ci/build.rb                           |  2 +-
 app/models/ci/build_metadata.rb                  | 16 ++++------------
 app/serializers/build_details_entity.rb          |  4 +---
 spec/factories/ci/build_metadata.rb              |  9 +++++++++
 spec/models/ci/build_metadata_spec.rb            | 10 +---------
 6 files changed, 25 insertions(+), 28 deletions(-)
 create mode 100644 spec/factories/ci/build_metadata.rb

diff --git a/app/assets/javascripts/jobs/components/sidebar_details_block.vue b/app/assets/javascripts/jobs/components/sidebar_details_block.vue
index 6ff3fa6e099..172de6b3679 100644
--- a/app/assets/javascripts/jobs/components/sidebar_details_block.vue
+++ b/app/assets/javascripts/jobs/components/sidebar_details_block.vue
@@ -44,10 +44,16 @@
       runnerId() {
         return `#${this.job.runner.id}`;
       },
+      hasTimeout() {
+        return this.job.metadata != null && this.job.metadata.timeout_human_readable !== '';
+      },
       timeout() {
-        let t = `${this.job.metadata.timeout_human_readable}`;
+        if (this.job.metadata == null) {
+          return '';
+        }
 
-        if (this.job.metadata.timeout_source != null) {
+        let t = this.job.metadata.timeout_human_readable;
+        if (this.job.metadata.timeout_source !== '') {
           t += ` (from ${this.job.metadata.timeout_source})`;
         }
 
@@ -130,7 +136,7 @@
         />
         <detail-row
           class="js-job-timeout"
-          v-if="job.metadata.timeout_human_readable"
+          v-if="hasTimeout"
           title="Timeout"
           :help-url="runnerHelpUrl"
           :value="timeout"
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 0b3c6ac4fee..355d1c0523f 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -162,7 +162,7 @@ module Ci
     end
 
     def ensure_metadata
-      metadata || build_metadata
+      metadata || build_metadata(project: project)
     end
 
     def detailed_status(current_user)
diff --git a/app/models/ci/build_metadata.rb b/app/models/ci/build_metadata.rb
index c221f43384b..6d8a895d509 100644
--- a/app/models/ci/build_metadata.rb
+++ b/app/models/ci/build_metadata.rb
@@ -11,9 +11,9 @@ module Ci
     belongs_to :build, class_name: 'Ci::Build'
     belongs_to :project
 
-    chronic_duration_attr_reader :timeout_human_readable, :timeout
+    validates :project, presence: true
 
-    after_initialize :set_project_id
+    chronic_duration_attr_reader :timeout_human_readable, :timeout
 
     enum timeout_source: {
         unknown_timeout_source: 1,
@@ -24,19 +24,11 @@ module Ci
     def save_timeout_state!
       return unless build.runner.present?
 
-      project_timeout = build.project&.build_timeout
+      project_timeout = project&.build_timeout
       timeout = [project_timeout, build.runner.maximum_timeout].compact.min
       timeout_source = timeout < project_timeout ? :runner_timeout_source : :project_timeout_source
 
-      update_attributes(timeout: timeout, timeout_source: timeout_source)
-    end
-
-    private
-
-    def set_project_id
-      return unless self.project_id.nil?
-
-      self.project_id = build&.project&.id
+      update!(timeout: timeout, timeout_source: timeout_source)
     end
   end
 end
diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb
index 99ca0bd158b..ca4480fe2b1 100644
--- a/app/serializers/build_details_entity.rb
+++ b/app/serializers/build_details_entity.rb
@@ -5,9 +5,7 @@ class BuildDetailsEntity < JobEntity
   expose :runner, using: RunnerEntity
   expose :pipeline, using: PipelineEntity
 
-  expose :metadata, using: BuildMetadataEntity do |build|
-    build.ensure_metadata
-  end
+  expose :metadata, using: BuildMetadataEntity
 
   expose :erased_by, if: -> (*) { build.erased? }, using: UserEntity
   expose :erase_path, if: -> (*) { build.erasable? && can?(current_user, :erase_build, build) } do |build|
diff --git a/spec/factories/ci/build_metadata.rb b/spec/factories/ci/build_metadata.rb
new file mode 100644
index 00000000000..66bbd977b88
--- /dev/null
+++ b/spec/factories/ci/build_metadata.rb
@@ -0,0 +1,9 @@
+FactoryBot.define do
+  factory :ci_build_metadata, class: Ci::BuildMetadata do
+    build factory: :ci_build
+
+    after(:build) do |build_metadata, _|
+      build_metadata.project ||= build_metadata.build.project
+    end
+  end
+end
diff --git a/spec/models/ci/build_metadata_spec.rb b/spec/models/ci/build_metadata_spec.rb
index 4758738cdd0..d21e9600e42 100644
--- a/spec/models/ci/build_metadata_spec.rb
+++ b/spec/models/ci/build_metadata_spec.rb
@@ -13,15 +13,7 @@ describe Ci::BuildMetadata do
   end
 
   let(:build) { create(:ci_build, pipeline: pipeline) }
-  let(:build_metadata) { described_class.create(build: build) }
-
-  context 'when creating' do
-    subject { build_metadata.project_id }
-
-    it 'saves project_id' do
-      is_expected.to eq(project.id)
-    end
-  end
+  let(:build_metadata) { create(:ci_build_metadata, build: build) }
 
   describe '#save_timeout_state!' do
     subject { build_metadata }