From b504df7c63a47bda8425052beb648c39c62abf6d Mon Sep 17 00:00:00 2001 From: Cam Saul <cammsaul@gmail.com> Date: Tue, 6 Nov 2018 12:57:31 -0800 Subject: [PATCH] Typo fixes and other code cleanup :shower: [ci bigquery] --- src/metabase/api/routes.clj | 4 +- src/metabase/api/util.clj | 2 +- src/metabase/email/messages.clj | 37 ++++++++++--------- src/metabase/mbql/util.clj | 2 +- src/metabase/models/params.clj | 5 ++- src/metabase/models/permissions.clj | 1 + src/metabase/models/user.clj | 4 +- src/metabase/query_processor.clj | 4 +- .../middleware/parameters/sql.clj | 4 +- src/metabase/query_processor/util.clj | 13 ------- test/metabase/driver/bigquery_test.clj | 25 +++++++++++++ .../models/query/permissions_test.clj | 4 +- test/metabase/query_processor/util_test.clj | 18 --------- .../test/data/dataset_definitions.clj | 6 +-- 14 files changed, 63 insertions(+), 66 deletions(-) diff --git a/src/metabase/api/routes.clj b/src/metabase/api/routes.clj index 616d55cce3e..27615f3a823 100644 --- a/src/metabase/api/routes.clj +++ b/src/metabase/api/routes.clj @@ -4,7 +4,7 @@ [route :as route]] [metabase.api [activity :as activity] - [alert :as alert] + [alert :as alert] [automagic-dashboards :as magic] [card :as card] [collection :as collection] @@ -30,8 +30,8 @@ [setup :as setup] [slack :as slack] [table :as table] - [tiles :as tiles] [task :as task] + [tiles :as tiles] [user :as user] [util :as util]] [metabase.middleware :as middleware] diff --git a/src/metabase/api/util.clj b/src/metabase/api/util.clj index b6313460eb8..16b74b33bd0 100644 --- a/src/metabase/api/util.clj +++ b/src/metabase/api/util.clj @@ -29,7 +29,7 @@ (stats/anonymous-usage-stats)) (api/defendpoint GET "/random_token" - "Return a cryptographically secure random 32-byte token, encoded as a hexidecimal string. + "Return a cryptographically secure random 32-byte token, encoded as a hexadecimal string. Intended for use when creating a value for `embedding-secret-key`." [] {:token (crypto-random/hex 32)}) diff --git a/src/metabase/email/messages.clj b/src/metabase/email/messages.clj index da49274b311..59c746b67cf 100644 --- a/src/metabase/email/messages.clj +++ b/src/metabase/email/messages.clj @@ -14,15 +14,14 @@ [metabase.util [date :as du] [export :as export] - [i18n :refer [tru]] + [i18n :refer [trs tru]] [quotation :as quotation] [urls :as url]] [stencil [core :as stencil] [loader :as stencil-loader]] [toucan.db :as db]) - (:import [java.io File FileOutputStream IOException] - java.util.Arrays)) + (:import [java.io File IOException])) ;; Dev only -- disable template caching (when config/is-dev? @@ -36,19 +35,21 @@ {:quotation (:quote data-quote) :quotationAuthor (:author data-quote)})) -(def ^:private ^:const notification-context +(def ^:private notification-context {:emailType "notification" :logoHeader true}) -(def ^:private ^:const abandonment-context - {:heading "We’d love your feedback." - :callToAction "It looks like Metabase wasn’t quite a match for you. Would you mind taking a fast 5 question survey to help the Metabase team understand why and make things better in the future?" - :link "http://www.metabase.com/feedback/inactive"}) +(defn- abandonment-context [] + {:heading (str (trs "We’d love your feedback.")) + :callToAction (str (trs "It looks like Metabase wasn’t quite a match for you.") + " " + (trs "Would you mind taking a fast 5 question survey to help the Metabase team understand why and make things better in the future?")) + :link "https://www.metabase.com/feedback/inactive"}) -(def ^:private ^:const follow-up-context - {:heading "We hope you've been enjoying Metabase." - :callToAction "Would you mind taking a fast 6 question survey to tell us how it’s going?" - :link "http://www.metabase.com/feedback/active"}) +(defn- follow-up-context [] + {:heading (str (trs "We hope you've been enjoying Metabase.")) + :callToAction (str (trs "Would you mind taking a fast 6 question survey to tell us how it’s going?")) + :link "https://www.metabase.com/feedback/active"}) ;;; ### Public Interface @@ -75,8 +76,9 @@ (defn- all-admin-recipients "Return a sequence of email addresses for all Admin users. - The first recipient will be the site admin (or oldest admin if unset), which is the address that should be used in `mailto` links - (e.g., for the new user to email with any questions)." + + The first recipient will be the site admin (or oldest admin if unset), which is the address that should be used in + `mailto` links (e.g., for the new user to email with any questions)." [] (concat (when-let [admin-email (public-settings/admin-email)] [admin-email]) @@ -124,7 +126,8 @@ :message-type :html :message message-body))) -;; TODO - I didn't write these function and I don't know what it's for / what it's supposed to be doing. If this is determined add appropriate documentation +;; TODO - I didn't write these function and I don't know what it's for / what it's supposed to be doing. If this is +;; determined add appropriate documentation (defn- model-name->url-fn [model] (case model @@ -172,8 +175,8 @@ context (merge notification-context (random-quote-context) (if (= "abandon" msg-type) - abandonment-context - follow-up-context)) + (abandonment-context) + (follow-up-context))) message-body (stencil/render-file "metabase/email/follow_up_email" context)] (email/send-message! :subject subject diff --git a/src/metabase/mbql/util.clj b/src/metabase/mbql/util.clj index d6fac37a891..2687a3be36c 100644 --- a/src/metabase/mbql/util.clj +++ b/src/metabase/mbql/util.clj @@ -265,7 +265,7 @@ (throw (Exception. (str - (tru "Error: query's source query has not been resolved. You probably need to `preprocess` the query first.")))) + (tru "Error: query''s source query has not been resolved. You probably need to `preprocess` the query first.")))) ;; otherwise resolve the source Table :else diff --git a/src/metabase/models/params.clj b/src/metabase/models/params.clj index ce25a08cb15..dd36ef34d2b 100644 --- a/src/metabase/models/params.clj +++ b/src/metabase/models/params.clj @@ -5,6 +5,7 @@ [db :as mdb] [util :as u]] [metabase.mbql.util :as mbql.u] + [metabase.util.i18n :as ui18n :refer [trs]] [toucan [db :as db] [hydrate :refer [hydrate]]])) @@ -29,7 +30,7 @@ field-form :else - (throw (IllegalArgumentException. (str "Don't know what to do with: " field-form))))) + (throw (IllegalArgumentException. (str (trs "Don't know what to do with:") " " field-form))))) (defn wrap-field-id-if-needed "Wrap a raw Field ID in a `:field-id` clause if needed." @@ -42,7 +43,7 @@ [:field-id field-id-or-form] :else - (throw (IllegalArgumentException. (str "Don't know how to wrap:" field-id-or-form))))) + (throw (IllegalArgumentException. (str (trs "Don't know how to wrap:") " " field-id-or-form))))) (defn- field-ids->param-field-values "Given a collection of PARAM-FIELD-IDS return a map of FieldValues for the Fields they reference. diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj index d086491ee80..76ad328003c 100644 --- a/src/metabase/models/permissions.clj +++ b/src/metabase/models/permissions.clj @@ -559,6 +559,7 @@ [old-graph new-graph] (when (not= (:revision old-graph) (:revision new-graph)) (throw (ui18n/ex-info (str (tru "Looks like someone else edited the permissions and your data is out of date.") + " " (tru "Please fetch new data and try again.")) {:status-code 409})))) diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj index 6f2dd9a0051..a723f9ee394 100644 --- a/src/metabase/models/user.clj +++ b/src/metabase/models/user.clj @@ -181,7 +181,7 @@ (s/defn create-new-google-auth-user! "Convenience for creating a new user via Google Auth. This account is considered active immediately; thus all active - admins will recieve an email right away." + admins will receive an email right away." [new-user :- NewUser] (u/prog1 (insert-new-user! (assoc new-user :google_auth true)) ;; send an email to everyone including the site admin if that's set @@ -189,7 +189,7 @@ (s/defn create-new-ldap-auth-user! "Convenience for creating a new user via LDAP. This account is considered active immediately; thus all active admins - will recieve an email right away." + will receive an email right away." [new-user :- NewUser] (insert-new-user! (-> new-user ;; We should not store LDAP passwords diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj index 4589eaed64c..912afa8b898 100644 --- a/src/metabase/query_processor.clj +++ b/src/metabase/query_processor.clj @@ -169,7 +169,7 @@ ;; ...which ends up getting caught by the `catch-exceptions` middleware. Add a final post-processing function ;; around that which will return whatever we delivered into the `:results-promise`. - recieve-native-query + receive-native-query (fn [qp] (fn [query] (let [results-promise (promise) @@ -183,7 +183,7 @@ ;; everyone a favor and throw an Exception (let [results (m/dissoc-in results [:query :results-promise])] (throw (ex-info (str (tru "Error preprocessing query")) results)))))))] - (recieve-native-query (qp-pipeline deliver-native-query)))) + (receive-native-query (qp-pipeline deliver-native-query)))) (defn query->preprocessed "Return the fully preprocessed form for `query`, the way it would look immediately before `mbql->native` is called. diff --git a/src/metabase/query_processor/middleware/parameters/sql.clj b/src/metabase/query_processor/middleware/parameters/sql.clj index 3763d0b6b07..f916796c6c5 100644 --- a/src/metabase/query_processor/middleware/parameters/sql.clj +++ b/src/metabase/query_processor/middleware/parameters/sql.clj @@ -191,10 +191,10 @@ "Return the `:default` value for a param if no explicit values were passsed. This only applies to non-Dimension (non-Field Filter) params. Default values for Dimension (Field Filter) params are handled above in `default-value-for-dimension`." - [{:keys [default display_name required]} :- TagParam] + [{:keys [default display-name required]} :- TagParam] (or default (when required - (throw (Exception. (str (tru "''{0}'' is a required param." display_name))))))) + (throw (Exception. (str (tru "''{0}'' is a required param." display-name))))))) ;;; Parsing Values diff --git a/src/metabase/query_processor/util.clj b/src/metabase/query_processor/util.clj index 283b0f3a08a..c47482e887a 100644 --- a/src/metabase/query_processor/util.clj +++ b/src/metabase/query_processor/util.clj @@ -56,19 +56,6 @@ (recur (assoc m :query source-query) ks not-found) (get-in m (cons :query ks) not-found)))) -(defn assoc-in-query - "Similar to `assoc-in but will look in either `:query` or recursively in `[:query :source-query]`. Using - this function will avoid having to check if there's a nested query vs. top-level query." - [m ks v] - (if-let [source-query (get-in m [:query :source-query])] - ;; We have a soure-query, we need to recursively `assoc-in` with the source query as the query - (assoc-in m - [:query :source-query] - (-> (assoc m :query source-query) - (assoc-in-query ks v) - :query)) - (assoc-in m (cons :query ks) v))) - ;;; ---------------------------------------------------- Hashing ----------------------------------------------------- diff --git a/test/metabase/driver/bigquery_test.clj b/test/metabase/driver/bigquery_test.clj index 1b711da80f5..8d899f96ff7 100644 --- a/test/metabase/driver/bigquery_test.clj +++ b/test/metabase/driver/bigquery_test.clj @@ -216,3 +216,28 @@ :details (assoc (:details (Database (data/id))) :use-jvm-timezone true)}]] (native-timestamp-query db "2018-08-31 00:00:00+07" "Asia/Jakarta")))) + +;; if I run a BigQuery query, does it get a remark added to it? +(expect-with-engine :bigquery + (str + "-- Metabase:: userID: 1000 queryType: MBQL queryHash: 01020304\n" + "SELECT `test_data.venues`.`id` AS `venues___id`," + " `test_data.venues`.`name` AS `venues___name`," + " `test_data.venues`.`category_id` AS `venues___category_id`," + " `test_data.venues`.`latitude` AS `venues___latitude`," + " `test_data.venues`.`longitude` AS `venues___longitude`," + " `test_data.venues`.`price` AS `venues___price` " + "FROM `test_data.venues` " + "LIMIT 1") + (let [native-query (atom nil)] + (with-redefs [bigquery/process-native* (fn [_ sql] + (reset! native-query sql) + (throw (Exception. "Done.")))] + (qp/process-query {:database (data/id) + :type :query + :query {:source-table (data/id :venues) + :limit 1} + :info {:executed-by 1000 + :query-type "MBQL" + :query-hash (byte-array [1 2 3 4])}}) + @native-query))) diff --git a/test/metabase/models/query/permissions_test.clj b/test/metabase/models/query/permissions_test.clj index ad67536744e..c95efc541db 100644 --- a/test/metabase/models/query/permissions_test.clj +++ b/test/metabase/models/query/permissions_test.clj @@ -13,9 +13,9 @@ [metabase.models.query.permissions :as query-perms] [metabase.test.data :as data] [metabase.test.data.users :as users] + [metabase.test.util.log :as tu.log] [metabase.util :as u] - [toucan.util.test :as tt] - [metabase.test.util.log :as tu.log])) + [toucan.util.test :as tt])) ;;; ---------------------------------------------- Permissions Checking ---------------------------------------------- diff --git a/test/metabase/query_processor/util_test.clj b/test/metabase/query_processor/util_test.clj index 68d5fa188ae..2c3c18c6f53 100644 --- a/test/metabase/query_processor/util_test.clj +++ b/test/metabase/query_processor/util_test.clj @@ -147,21 +147,3 @@ (expect not-found (qputil/get-in-query {} [:test] not-found))) - -(def ^:private updated-test-map - {:test {:value 11}}) - -;; assoc-in-query works with a non-nested query -(expect - {:query updated-test-map} - (qputil/assoc-in-query {:query test-inner-map} [:test :value] 11)) - -;; assoc-in-query works with a nested query -(expect - {:query {:source-query updated-test-map}} - (qputil/assoc-in-query {:query {:source-query test-inner-map}} [:test :value] 11)) - -;; Not supported yet, but assoc-in-query should do the right thing with a double nested query -(expect - {:query {:source-query {:source-query updated-test-map}}} - (qputil/assoc-in-query {:query {:source-query {:source-query test-inner-map}}} [:test :value] 11)) diff --git a/test/metabase/test/data/dataset_definitions.clj b/test/metabase/test/data/dataset_definitions.clj index 7da35d959bf..0d4c6b2f719 100644 --- a/test/metabase/test/data/dataset_definitions.clj +++ b/test/metabase/test/data/dataset_definitions.clj @@ -1,8 +1,6 @@ (ns metabase.test.data.dataset-definitions "Definitions of various datasets for use in tests with `with-temp-db`." - (:require [clojure.tools.reader.edn :as edn] - [metabase.test.data.interface :as di] - [metabase.util.date :as du]) + (:require [metabase.test.data.interface :as di]) (:import java.sql.Time java.util.Calendar)) @@ -23,7 +21,7 @@ (di/def-database-definition-edn places-cam-likes) ;; A small dataset with users and a set of messages between them. Each message has *2* foreign keys to user -- -;; sender and reciever -- allowing us to test situations where multiple joins for a *single* table should occur. +;; sender and receiver -- allowing us to test situations where multiple joins for a *single* table should occur. (di/def-database-definition-edn avian-singles) (defn- date-only -- GitLab