Merge pull request #5845 from metabase/out-of-date-remappings

Fixup human readable values when field values change [ci drivers]

src/metabase/models/field_values.clj

@@ -79,6 +79,16 @@
       (when (values-less-than-total-max-length? values)
         values))))
 
+(defn- fixup-human-readable-values
+  "Field values and human readable values are lists that are zipped
+  together. If the field values have changes, the human readable
+  values will need to change too. This function reconstructs the
+  human_readable_values to reflect `NEW-VALUES`. If a new field value
+  is found, a string version of that is used"
+  [{old-values :values, old-hrv :human_readable_values} new-values]
+  (when (seq old-hrv)
+    (let [orig-remappings (zipmap old-values old-hrv)]
+      (map #(get orig-remappings % (str %)) new-values))))
 
 (defn create-or-update-field-values!
   "Create or update the FieldValues object for FIELD. If the FieldValues object already exists, then update values for
@@ -92,8 +102,9 @@
       (and field-values values)
       (do
         (log/debug (format "Storing updated FieldValues for Field %s..." field-name))
-        (db/update! FieldValues (u/get-id field-values)
-          :values values))
+        (db/update-non-nil-keys! FieldValues (u/get-id field-values)
+          :values                values
+          :human_readable_values (fixup-human-readable-values field-values values)))
       ;; if FieldValues object doesn't exist create one
       values
       (do

test/metabase/models/field_values_test.clj

 (ns metabase.models.field-values-test
-  (:require [expectations :refer :all]
+  (:require [clojure.java.jdbc :as jdbc]
+            [expectations :refer :all]
+            [metabase
+             [db :as mdb]
+             [sync :as sync]
+             [util :as u]]
             [metabase.models
              [database :refer [Database]]
              [field :refer [Field]]
@@ -50,3 +55,57 @@
      (do
        (clear-field-values! field-id)
        (db/select-one-field :values FieldValues, :field_id field-id))]))
+
+(defn- find-values [field-values-id]
+  (-> (db/select-one FieldValues :id field-values-id)
+      (select-keys [:values :human_readable_values])))
+
+(defn- sync-and-find-values [db field-values-id]
+  (sync/sync-database! db)
+  (find-values field-values-id))
+
+;; Test "fixing" of human readable values when field values change
+(expect
+  (concat (repeat 2 {:values [1 2 3] :human_readable_values ["a" "b" "c"]})
+          (repeat 2 {:values [-2 -1 0 1 2 3] :human_readable_values ["-2" "-1" "0" "a" "b" "c"]})
+          [{:values [-2 -1 0] :human_readable_values ["-2" "-1" "0"]}])
+
+  (binding [mdb/*allow-potentailly-unsafe-connections* true]
+    ;; Create a temp warehouse database that can have it's field values change
+    (jdbc/with-db-connection [conn {:classname "org.h2.Driver", :subprotocol "h2", :subname "mem:temp"}]
+      (jdbc/execute! conn ["drop table foo if exists"])
+      (jdbc/execute! conn ["create table foo (id integer primary key, category_id integer not null, desc text)"])
+      (jdbc/insert-multi! conn :foo [{:id 1 :category_id 1 :desc "foo"}
+                                     {:id 2 :category_id 2 :desc "bar"}
+                                     {:id 3 :category_id 3 :desc "baz"}])
+
+      ;; Create a new in the Database table for this newly created temp database
+      (tt/with-temp Database [db {:engine       :h2
+                                  :name         "foo"
+                                  :is_full_sync true
+                                  :details      "{\"db\": \"mem:temp\"}"}]
+
+        ;; Sync the database so we have the new table and it's fields
+        (do (sync/sync-database! db)
+            (let [table-id        (db/select-one-field :id Table :db_id (u/get-id db) :name "FOO")
+                  field-id        (db/select-one-field :id Field :table_id table-id :name "CATEGORY_ID")
+                  field-values-id (db/select-one-field :id FieldValues :field_id field-id)]
+              ;; Add in human readable values for remapping
+              (db/update! FieldValues field-values-id {:human_readable_values "[\"a\",\"b\",\"c\"]"})
+
+              ;; This is the starting point, the original catgory ids and their remapped values
+              [(find-values field-values-id)
+               ;; There should be no changes to human_readable_values  when resync'd
+               (sync-and-find-values db field-values-id)
+               (do
+                 ;; Add new rows that will have new field values
+                 (jdbc/insert-multi! conn :foo [{:id 4 :category_id -2 :desc "foo"}
+                                                {:id 5 :category_id -1 :desc "bar"}
+                                                {:id 6 :category_id 0 :desc "baz"}])
+                 ;; Sync to pickup the new field values and rebuild the human_readable_values
+                 (sync-and-find-values db field-values-id))
+               ;; Resyncing this (with the new field values) should result in the same human_readable_values
+               (sync-and-find-values db field-values-id)
+               ;; Test that field values can be removed and the corresponding human_readable_values are removed as well
+               (do (jdbc/delete! conn :foo ["id in (?,?,?)" 1 2 3])
+                   (sync-and-find-values db field-values-id))]))))))