Allow strings to be empty in metadata (#21880)

* Allow strings to be empty in metadata

not sure why it originally required a nil or a non-blank string. But
when you want to override a description and clear it out, the front-end
text boxes will send over an empty string "".

This value is useful on its own though. Particularly in mbql queries or
in native models where you've identified an underlying field, it will
merge the metadata of the underlying field with the metadata of the
model. So if we keep a nil there the underlying can show up. Especially
for mbql queries, where you have the underlying description to begin
with, you clear it out and then it appears to remain. Because of this
merging. Using an affirmative "" empty string value prevents the merge
and we all are happy

* Lets also fix #11097 while we're near

not identical, but same problem enforced at the route rather than some
schema though

* Unneeded thread macro

src/metabase/api/table.clj

@@ -96,9 +96,9 @@
   {display_name            (s/maybe su/NonBlankString)
    entity_type             (s/maybe su/EntityTypeKeywordOrString)
    visibility_type         (s/maybe TableVisibilityType)
-   description             (s/maybe su/NonBlankString)
-   caveats                 (s/maybe su/NonBlankString)
-   points_of_interest      (s/maybe su/NonBlankString)
+   description             (s/maybe s/Str)
+   caveats                 (s/maybe s/Str)
+   points_of_interest      (s/maybe s/Str)
    show_in_getting_started (s/maybe s/Bool)
    field_order             (s/maybe FieldOrder)}
   (first (update-tables! [id] body)))
@@ -111,9 +111,9 @@
    display_name            (s/maybe su/NonBlankString)
    entity_type             (s/maybe su/EntityTypeKeywordOrString)
    visibility_type         (s/maybe TableVisibilityType)
-   description             (s/maybe su/NonBlankString)
-   caveats                 (s/maybe su/NonBlankString)
-   points_of_interest      (s/maybe su/NonBlankString)
+   description             (s/maybe s/Str)
+   caveats                 (s/maybe s/Str)
+   points_of_interest      (s/maybe s/Str)
    show_in_getting_started (s/maybe s/Bool)}
   (update-tables! ids body))
 

src/metabase/sync/analyze/query_results.clj

@@ -29,7 +29,7 @@
   ;; the QP, everything will be normalized.
   {:name                           s/Str
    :display_name                   s/Str
-   (s/optional-key :description)   (s/maybe su/NonBlankString)
+   (s/optional-key :description)   (s/maybe s/Str)
    :base_type                      su/FieldTypeKeywordOrString
    (s/optional-key :semantic_type) (s/maybe su/FieldSemanticOrRelationTypeKeywordOrString)
    (s/optional-key :unit)          (s/maybe DateTimeUnitKeywordOrString)

test/metabase/api/card_test.clj

@@ -1980,7 +1980,10 @@
           to-native      (fn [q]
                            {:database (:database q)
                             :type     :native
-                            :native   (mt/compile q)})]
+                            :native   (mt/compile q)})
+          update-card!  (fn [card]
+                          (mt/user-http-request :rasta :put 202
+                                                (str "card/" (u/the-id card)) card))]
       (doseq [[query-type query modified-query] [["mbql"   query modified-query]
                                                  ["native" (to-native query) (to-native modified-query)]]]
         (testing (str "For: " query-type)
@@ -1994,19 +1997,16 @@
                                                   :dataset true))]
               (is (= ["ID" "NAME"] (map norm metadata)))
               (is (= ["EDITED DISPLAY" "EDITED DISPLAY"]
-                     (->> (mt/user-http-request
-                           :rasta :put 202 (str "card/" card-id)
-                           (assoc card :result_metadata (map #(assoc % :display_name "EDITED DISPLAY")
-                                                             metadata)))
+                     (->> (update-card!
+                           (assoc card
+                                  :result_metadata (map #(assoc % :display_name "EDITED DISPLAY") metadata)))
                           :result_metadata (map :display_name))))
               ;; simulate a user changing the query without rerunning the query
               (is (= ["EDITED DISPLAY" "EDITED DISPLAY" "PRICE"]
-                     (->> (mt/user-http-request
-                           :rasta :put 202 (str "card/" card-id)
-                           (assoc card
-                                  :dataset_query modified-query
-                                  :result_metadata (map #(assoc % :display_name "EDITED DISPLAY")
-                                                        metadata)))
+                     (->> (update-card! (assoc card
+                                               :dataset_query modified-query
+                                               :result_metadata (map #(assoc % :display_name "EDITED DISPLAY")
+                                                                     metadata)))
                           :result_metadata
                           (map (comp str/upper-case :display_name)))))
               (is (= ["EDITED DISPLAY" "EDITED DISPLAY" "PRICE"]
@@ -2014,7 +2014,17 @@
                           (db/select-one-field :result_metadata Card :id card-id))))
               (testing "Even if you only send the new query and not existing metadata"
                 (is (= ["EDITED DISPLAY" "EDITED DISPLAY"]
-                     (->> (mt/user-http-request
-                           :rasta :put 202 (str "card/" card-id)
-                           {:dataset_query query})
-                          :result_metadata (map :display_name))))))))))))
+                     (->> (update-card! {:id (u/the-id card) :dataset_query query}) :result_metadata (map :display_name)))))
+              (testing "Descriptions can be cleared (#20517)"
+                (is (= ["foo" "foo"]
+                       (->> (update-card! (update card
+                                                  :result_metadata (fn [m]
+                                                                     (map #(assoc % :description "foo") m))))
+                            :result_metadata
+                            (map :description))))
+                (is (= ["" ""]
+                       (->> (update-card! (update card
+                                                  :result_metadata (fn [m]
+                                                                     (map #(assoc % :description "") m))))
+                            :result_metadata
+                            (map :description))))))))))))

test/metabase/api/table_test.clj

@@ -283,6 +283,13 @@
                :pk_field        (table/pk-field-id table)})
              (dissoc (mt/user-http-request :crowberto :get 200 (format "table/%d" (u/the-id table)))
                      :updated_at))))
+    (testing "Can update description, caveat, points of interest to be empty (#11097)"
+      (doseq [property [:caveats :points_of_interest :description]]
+        (mt/with-temp Table [table]
+          (is (= ""
+                 (get (mt/user-http-request :crowberto :put 200 (format "table/%d" (u/the-id table))
+                                            {property ""})
+                      property))))))
 
     (testing "A table can only be updated by a superuser"
       (mt/with-temp Table [table]