From 33d54e53ba266fd8c2411e7b394a496c475421a4 Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Fri, 13 Jan 2023 13:31:26 -0700
Subject: [PATCH] FieldValues skipping but not updating last used at (#27655)
* FieldValues skipping but not updating last used at
Related to #26863
When fieldvalues synching was being skipped, the
`get-or-create-full-field-values!` call was erroneously forgetting to
update the `last_used_at` time; the time was only being updated if new
field values came in since the last sync.
* Add test for inactive field values
* Only update last_used_at when there's an existing fieldvalues
---
src/metabase/models/field_values.clj | 19 ++++++++---
test/metabase/models/field_values_test.clj | 38 +++++++++++++++-------
2 files changed, 40 insertions(+), 17 deletions(-)
diff --git a/src/metabase/models/field_values.clj b/src/metabase/models/field_values.clj
index 06a0f5b5d27..17eb57a6384 100644
--- a/src/metabase/models/field_values.clj
+++ b/src/metabase/models/field_values.clj
@@ -356,7 +356,9 @@
(and (= (:values field-values) values)
(= (:has_more_values field-values) has_more_values))
- (log/debug (trs "FieldValues for Field {0} remain unchanged. Skipping..." field-name))
+ (do
+ (log/debug (trs "FieldValues for Field {0} remain unchanged. Skipping..." field-name))
+ ::fv-skipped)
;; if the FieldValues object already exists then update values in it
(and field-values values)
@@ -395,10 +397,17 @@
(when (field-should-have-field-values? field)
(let [existing (db/select-one FieldValues :field_id field-id :type :full)]
(if (or (not existing) (inactive? existing))
- (when-let [result (#{::fv-created ::fv-updated} (create-or-update-full-field-values! field human-readable-values))]
- (when (= result ::fv-updated)
- (db/update! FieldValues (:id existing) :last_used_at :%now))
- (db/select-one FieldValues :field_id field-id :type :full))
+ (case (create-or-update-full-field-values! field human-readable-values)
+ ::fv-deleted
+ nil
+
+ ::fv-created
+ (db/select-one FieldValues :field_id field-id :type :full)
+
+ (do
+ (when existing
+ (db/update! FieldValues (:id existing) :last_used_at :%now))
+ (db/select-one FieldValues :field_id field-id :type :full)))
(do
(db/update! FieldValues (:id existing) :last_used_at :%now)
existing)))))
diff --git a/test/metabase/models/field_values_test.clj b/test/metabase/models/field_values_test.clj
index 989bd456169..3bdb18a17b1 100644
--- a/test/metabase/models/field_values_test.clj
+++ b/test/metabase/models/field_values_test.clj
@@ -5,6 +5,7 @@
[clojure.java.jdbc :as jdbc]
[clojure.string :as str]
[clojure.test :refer :all]
+ [java-time :as t]
[metabase.db.metadata-queries :as metadata-queries]
[metabase.models.database :refer [Database]]
[metabase.models.dimension :refer [Dimension]]
@@ -125,18 +126,31 @@
(find-values field-values-id))
(deftest get-or-create-full-field-values!-test
- (testing "create a full Fieldvalues if it does not exist"
- (db/delete! FieldValues :field_id (mt/id :categories :name) :type :full)
- (is (= :full (-> (db/select-one Field :id (mt/id :categories :name))
- field-values/get-or-create-full-field-values!
- :type))
- (is (= 1 (db/count FieldValues :field_id (mt/id :categories :name) :type :full))))
-
- (testing "if an Advanced FeildValues Exists, make sure we still returns the full FieldValues"
- (mt/with-temp FieldValues [_ {:field_id (mt/id :categories :name)
- :type :sandbox
- :hash_key "random-hash"}])
- (is (= :full (:type (field-values/get-or-create-full-field-values! (db/select-one Field :id (mt/id :categories :name)))))))))
+ (mt/dataset test-data
+ (testing "create a full Fieldvalues if it does not exist"
+ (db/delete! FieldValues :field_id (mt/id :categories :name) :type :full)
+ (is (= :full (-> (db/select-one Field :id (mt/id :categories :name))
+ field-values/get-or-create-full-field-values!
+ :type))
+ (is (= 1 (db/count FieldValues :field_id (mt/id :categories :name) :type :full))))
+
+ (testing "if an Advanced FieldValues Exists, make sure we still returns the full FieldValues"
+ (mt/with-temp FieldValues [_ {:field_id (mt/id :categories :name)
+ :type :sandbox
+ :hash_key "random-hash"}]
+ (is (= :full (:type (field-values/get-or-create-full-field-values! (db/select-one Field :id (mt/id :categories :name))))))))
+
+ (testing "if an old FieldValues Exists, make sure we still return the full FieldValues and update last_used_at"
+ (db/execute! {:update FieldValues
+ :where [:and
+ [:= :field_id (mt/id :categories :name)]
+ [:= :type "full"]]
+ :set {:last_used_at (t/offset-date-time 2001 12)}})
+ (is (= (t/offset-date-time 2001 12)
+ (:last_used_at (db/select-one FieldValues :field_id (mt/id :categories :name) :type :full))))
+ (is (seq (:values (field-values/get-or-create-full-field-values! (db/select-one Field :id (mt/id :categories :name))))))
+ (is (not= (t/offset-date-time 2001 12)
+ (:last_used_at (db/select-one FieldValues :field_id (mt/id :categories :name) :type :full))))))))
(deftest normalize-human-readable-values-test
(testing "If FieldValues were saved as a map, normalize them to a sequence on the way out"
--
GitLab