Merge pull request #9018 from metabase/smart-scalars-no-previous-value

Make smart scalars smart about previous value

frontend/src/metabase/visualizations/visualizations/SmartScalar.jsx

@@ -87,7 +87,10 @@ export default class Smart extends React.Component {
       return null;
     }
 
-    const change = formatNumber(insight["last-change"] * 100);
+    const lastChange = insight["last-change"];
+    const previousValue = insight["previous-value"];
+
+    const change = formatNumber(lastChange * 100);
     const isNegative = (change && Math.sign(change) < 0) || false;
 
     let color = isNegative ? colors["error"] : colors["success"];
@@ -160,25 +163,32 @@ export default class Smart extends React.Component {
           />
         )}
         <Box className="SmartWrapper">
-          <Flex align="center" mt={1} flexWrap="wrap">
-            <Flex align="center" color={color}>
-              <Icon name={isNegative ? "arrowDown" : "arrowUp"} />
-              {changeDisplay}
+          {!lastChange || !previousValue ? (
+            <Box
+              className="text-centered text-bold mt1"
+              color={colors["text-medium"]}
+            >{jt`Nothing to compare for the previous ${granularity}.`}</Box>
+          ) : (
+            <Flex align="center" mt={1} flexWrap="wrap">
+              <Flex align="center" color={color}>
+                <Icon name={isNegative ? "arrowDown" : "arrowUp"} />
+                {changeDisplay}
+              </Flex>
+              <h4
+                id="SmartScalar-PreviousValue"
+                className="flex align-center hide lg-show"
+                style={{
+                  color: colors["text-medium"],
+                }}
+              >
+                {!isFullscreen &&
+                  jt`${separator} was ${formatValue(
+                    previousValue,
+                    settings.column(column),
+                  )} ${granularityDisplay}`}
+              </h4>
             </Flex>
-            <h4
-              id="SmartScalar-PreviousValue"
-              className="flex align-center hide lg-show"
-              style={{
-                color: colors["text-medium"],
-              }}
-            >
-              {!isFullscreen &&
-                jt`${separator} was ${formatValue(
-                  insight["previous-value"],
-                  settings.column(column),
-                )} ${granularityDisplay}`}
-            </h4>
-          </Flex>
+          )}
         </Box>
       </ScalarWrapper>
     );

src/metabase/sync/analyze/fingerprint/insights.clj

@@ -135,6 +135,27 @@
   "We downsize UNIX timestamps to lessen the chance of overflows and numerical instabilities."
   #(/ % (* 1000 60 60 24)))
 
+(defn- about=
+  [a b]
+  (< 0.9 (/ a b) 1.1))
+
+(def ^:private unit->duration
+  {:minute  (/ 1 24 60)
+   :hour    (/ 24)
+   :day     1
+   :week    7
+   :month   30.5
+   :quarter (* 30.4 3)
+   :year    365.1})
+
+(defn- valid-period?
+  [from to unit]
+  (when (and from to)
+    (let [delta (- to from)]
+      (if unit
+        (about= delta (unit->duration unit))
+        (some (partial about= delta) (vals unit->duration))))))
+
 (defn- timeseries-insight
   [{:keys [numbers datetimes]}]
   (let [datetime   (first datetimes)
@@ -151,21 +172,26 @@
                      ;; at this stage in the pipeline the value is still an int, so we can use it
                      ;; directly.
                      (comp (stats/somef ms->day) #(nth % x-position)))]
-    (apply redux/juxt (for [number-col numbers]
-                        (redux/post-complete
-                         (let [y-position (:position number-col)
-                               yfn        #(nth % y-position)]
-                           (redux/juxt ((map yfn) (last-n 2))
-                                       (stats/simple-linear-regression xfn yfn)
-                                       (best-fit xfn yfn)))
-                         (fn [[[previous current] [offset slope] best-fit]]
-                           {:last-value     current
-                            :previous-value previous
-                            :last-change    (change current previous)
-                            :slope          slope
-                            :offset         offset
-                            :best-fit       best-fit
-                            :col            (:name number-col)}))))))
+    (apply redux/juxt
+           (for [number-col numbers]
+             (redux/post-complete
+              (let [y-position (:position number-col)
+                    yfn        #(nth % y-position)]
+                (redux/juxt ((map yfn) (last-n 2))
+                            ((map xfn) (last-n 2))
+                            (stats/simple-linear-regression xfn yfn)
+                            (best-fit xfn yfn)))
+              (fn [[[y-previous y-current] [x-previous x-current] [offset slope] best-fit]]
+                (let [show-change? (valid-period? x-previous x-current (:unit datetime))]
+                  {:last-value     y-current
+                   :previous-value (when show-change?
+                                     y-previous)
+                   :last-change    (when show-change?
+                                     (change y-current y-previous))
+                   :slope          slope
+                   :offset         offset
+                   :best-fit       best-fit
+                   :col            (:name number-col)})))))))
 
 (defn- datetime-truncated-to-year?
   "This is hackish as hell, but we change datetimes with year granularity to strings upstream and

test/metabase/sync/analyze/fingerprint/insights_test.clj

 (ns metabase.sync.analyze.fingerprint.insights-test
   (:require [expectations :refer :all]
-            [metabase.sync.analyze.fingerprint.insights :refer :all]))
+            [metabase.sync.analyze.fingerprint.insights :refer :all :as i]))
 
 (def ^:private cols [{:base_type :type/DateTime} {:base_type :type/Number}])
 
@@ -33,3 +33,45 @@
   (-> (transduce identity (insights cols) [[nil nil]])
       first
       :last-value))
+
+
+(defn- valid-period?
+  ([from to] (valid-period? from to nil))
+  ([from to period]
+   (boolean (#'i/valid-period? (some-> from (.getTime) (#'i/ms->day))
+                               (some-> to (.getTime) (#'i/ms->day))
+                               period))))
+
+(expect
+  true
+  (valid-period? #inst "2015-01" #inst "2015-02"))
+(expect
+  true
+  (valid-period? #inst "2015-02" #inst "2015-03"))
+(expect
+  false
+  (valid-period? #inst "2015-01" #inst "2015-03"))
+(expect
+  false
+  (valid-period? #inst "2015-01" nil))
+(expect
+  true
+  (valid-period? #inst "2015-01-01" #inst "2015-01-02"))
+(expect
+  true
+  (valid-period? #inst "2015-01-01" #inst "2015-01-08"))
+(expect
+  true
+  (valid-period? #inst "2015-01-01" #inst "2015-04-03"))
+(expect
+  true
+  (valid-period? #inst "2015" #inst "2016"))
+(expect
+  false
+  (valid-period? #inst "2015-01-01" #inst "2015-01-09"))
+(expect
+  true
+  (valid-period? #inst "2015-01-01" #inst "2015-04-03" :quarter))
+(expect
+  false
+  (valid-period? #inst "2015-01-01" #inst "2015-04-03" :month))