Merge branch '46082-runner-contacted_at-is-not-always-a-time-type' into 'master'

Resolve "Runner#contacted_at is not always a Time type"

Closes #46082

See merge request gitlab-org/gitlab-ce!18810

6 files changed, 97 insertions, 16 deletions

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index e6f1ed519be..530eacf4be0 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -75,7 +75,7 @@ module Ci
       project_type: 3
     }
 
-    cached_attr_reader :version, :revision, :platform, :architecture, :contacted_at, :ip_address
+    cached_attr_reader :version, :revision, :platform, :architecture, :ip_address, :contacted_at
 
     chronic_duration_attr :maximum_timeout_human_readable, :maximum_timeout
 
diff --git a/app/models/concerns/redis_cacheable.rb b/app/models/concerns/redis_cacheable.rb
index b889f4202dc..b5425295130 100644
--- a/app/models/concerns/redis_cacheable.rb
+++ b/app/models/concerns/redis_cacheable.rb
@@ -7,7 +7,11 @@ module RedisCacheable
   class_methods do
     def cached_attr_reader(*attributes)
       attributes.each do |attribute|
-        define_method("#{attribute}") do
+        define_method(attribute) do
+          unless self.has_attribute?(attribute)
+            raise ArgumentError, "`cached_attr_reader` requires the #{self.class.name}\##{attribute} attribute to have a database column"
+          end
+
           cached_attribute(attribute) || read_attribute(attribute)
         end
       end
@@ -15,13 +19,16 @@ module RedisCacheable
   end
 
   def cached_attribute(attribute)
-    (cached_attributes || {})[attribute]
+    cached_value = (cached_attributes || {})[attribute]
+    cast_value_from_cache(attribute, cached_value) if cached_value
   end
 
   def cache_attributes(values)
     Gitlab::Redis::SharedState.with do |redis|
       redis.set(cache_attribute_key, values.to_json, ex: CACHED_ATTRIBUTES_EXPIRY_TIME)
     end
+
+    clear_memoization(:cached_attributes)
   end
 
   private
@@ -38,4 +45,12 @@ module RedisCacheable
       end
     end
   end
+
+  def cast_value_from_cache(attribute, value)
+    if Gitlab.rails5?
+      self.class.type_for_attribute(attribute).cast(value)
+    else
+      self.class.column_for_attribute(attribute).type_cast_from_database(value)
+    end
+  end
 end
diff --git a/app/views/admin/runners/_runner.html.haml b/app/views/admin/runners/_runner.html.haml
index 6e76e7c2768..99fbbaec487 100644
--- a/app/views/admin/runners/_runner.html.haml
+++ b/app/views/admin/runners/_runner.html.haml
@@ -33,7 +33,7 @@
         = tag
   %td
     - if runner.contacted_at
-      #{time_ago_in_words(runner.contacted_at)} ago
+      = time_ago_with_tooltip runner.contacted_at
     - else
       Never
   %td.admin-runner-btn-group-cell
diff --git a/app/views/shared/runners/show.html.haml b/app/views/shared/runners/show.html.haml
index 480a224b6d5..93c6ba86640 100644
--- a/app/views/shared/runners/show.html.haml
+++ b/app/views/shared/runners/show.html.haml
@@ -66,6 +66,6 @@
       %td Last contact
       %td
         - if @runner.contacted_at
-          #{time_ago_in_words(@runner.contacted_at)} ago
+          = time_ago_with_tooltip @runner.contacted_at
         - else
           Never
diff --git a/changelogs/unreleased/46082-runner-contacted_at-is-not-always-a-time-type.yml b/changelogs/unreleased/46082-runner-contacted_at-is-not-always-a-time-type.yml
new file mode 100644
index 00000000000..07f67251b24
--- /dev/null
+++ b/changelogs/unreleased/46082-runner-contacted_at-is-not-always-a-time-type.yml
@@ -0,0 +1,5 @@
+---
+title: Fix Runner contacted at tooltip cache.
+merge_request: 18810
+author:
+type: fixed
diff --git a/spec/models/concerns/redis_cacheable_spec.rb b/spec/models/concerns/redis_cacheable_spec.rb
index 3d7963120b6..23c6c6233e9 100644
--- a/spec/models/concerns/redis_cacheable_spec.rb
+++ b/spec/models/concerns/redis_cacheable_spec.rb
@@ -1,21 +1,36 @@
 require 'spec_helper'
 
 describe RedisCacheable do
-  let(:model) { double }
+  let(:model) do
+    Struct.new(:id, :attributes) do
+      def read_attribute(attribute)
+        attributes[attribute]
+      end
+
+      def cast_value_from_cache(attribute, cached_value)
+        cached_value
+      end
+
+      def has_attribute?(attribute)
+        attributes.has_key?(attribute)
+      end
+    end
+  end
+
+  let(:payload) { { name: 'value', time: Time.zone.now } }
+  let(:instance) { model.new(1, payload) }
+  let(:cache_key) { instance.__send__(:cache_attribute_key) }
 
   before do
-    model.extend(described_class)
-    allow(model).to receive(:cache_attribute_key).and_return('key')
+    model.include(described_class)
   end
 
   describe '#cached_attribute' do
-    let(:payload) { { attribute: 'value' } }
-
-    subject { model.cached_attribute(payload.keys.first) }
+    subject { instance.cached_attribute(payload.keys.first) }
 
     it 'gets the cache attribute' do
       Gitlab::Redis::SharedState.with do |redis|
-        expect(redis).to receive(:get).with('key')
+        expect(redis).to receive(:get).with(cache_key)
           .and_return(payload.to_json)
       end
 
@@ -24,16 +39,62 @@ describe RedisCacheable do
   end
 
   describe '#cache_attributes' do
-    let(:values) { { name: 'new_name' } }
-
-    subject { model.cache_attributes(values) }
+    subject { instance.cache_attributes(payload) }
 
     it 'sets the cache attributes' do
       Gitlab::Redis::SharedState.with do |redis|
-        expect(redis).to receive(:set).with('key', values.to_json, anything)
+        expect(redis).to receive(:set).with(cache_key, payload.to_json, anything)
       end
 
       subject
     end
   end
+
+  describe '#cached_attr_reader', :clean_gitlab_redis_shared_state do
+    subject { instance.name }
+
+    before do
+      model.cached_attr_reader(:name)
+    end
+
+    context 'when there is no cached value' do
+      it 'reads the attribute' do
+        expect(instance).to receive(:read_attribute).and_call_original
+
+        expect(subject).to eq(payload[:name])
+      end
+    end
+
+    context 'when there is a cached value' do
+      it 'reads the cached value' do
+        expect(instance).not_to receive(:read_attribute)
+
+        instance.cache_attributes(payload)
+
+        expect(subject).to eq(payload[:name])
+      end
+    end
+
+    it 'always returns the latest values' do
+      expect(instance.name).to eq(payload[:name])
+
+      instance.cache_attributes(name: 'new_value')
+
+      expect(instance.name).to eq('new_value')
+    end
+  end
+
+  describe '#cast_value_from_cache' do
+    subject { instance.__send__(:cast_value_from_cache, attribute, value) }
+
+    context 'with runner contacted_at' do
+      let(:instance) { Ci::Runner.new }
+      let(:attribute) { :contacted_at }
+      let(:value) { '2018-05-07 13:53:08 UTC' }
+
+      it 'converts cache string to appropriate type' do
+        expect(subject).to be_an_instance_of(ActiveSupport::TimeWithZone)
+      end
+    end
+  end
 end