Skip to content

Commit

Permalink
Dates Binned by Week Export Formatting Matches App (#41619) (#41704)
Browse files Browse the repository at this point in the history 
* Dates Binned by Week Export Formatting Matches App

Fixes: #41492

Before, the dates binned by week would be formatted as follows in CSV and JSON exports:

`Week 1 - 2024`

But that doesn't match the App's format of a Date Range:

`January 1, 2024 - January 7, 2024`

So, now the exports will apply formatting in this same way.

Note that a second bug (#41616) exists on the Frontend preventing the column formatting being applied in the app. This is
frontend only, and the csv and json exports will match the column formatting settings (eg. abbreviated dates and
alternative separators will be used in the export).

* add test and change default week style

* Week format test change

* also fix incorrect :hour formatting

Co-authored-by: adam-james <21064735+adam-james-v@users.noreply.github.com>
  • Loading branch information
@metabase-bot @adam-james-v
metabase-bot[bot] and adam-james-v committed Apr 22, 2024
1 parent f0ff786 commit 5ecebce
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 19 deletions.
19 changes: 13 additions & 6 deletions src/metabase/formatter/datetime.clj
View file
Expand Up @@ -8,7 +8,6 @@
[metabase.shared.formatting.constants :as constants]
[metabase.shared.models.visualization-settings :as mb.viz]
[metabase.util.date-2 :as u.date]
[metabase.util.i18n :refer [tru]]
[metabase.util.log :as log])
(:import
(com.ibm.icu.text RuleBasedNumberFormat)
Expand Down Expand Up @@ -98,8 +97,8 @@
(cond-> (-> date-style (str/replace #"dddd" "EEEE"))
date-separator (str/replace #"/" date-separator)
date-abbreviate (-> (str/replace #"MMMM" "MMM")
(str/replace #"EEEE" "EEE")
(str/replace #"DDD" "D")))]
(str/replace #"EEEE" "EEE")
(str/replace #"DDD" "D")))]
(-> conditional-changes
;; 'D' formats as Day of year, we want Day of month, which is 'd' (issue #27469)
(str/replace #"D" "d")
Expand Down Expand Up @@ -133,7 +132,7 @@ If neither a unit nor a temporal type is provided, just bottom out by assuming a
(defmethod format-timestring :hour [timezone-id temporal-str _col {:keys [date-style time-style] :as viz-settings}]
(reformat-temporal-str timezone-id temporal-str
(-> (or date-style "MMMM, yyyy")
(-> (or date-style "MMMM d, yyyy")
(str ", " (fix-time-style time-style "h a"))
(post-process-date-style viz-settings))))
Expand All @@ -142,8 +141,16 @@ If neither a unit nor a temporal type is provided, just bottom out by assuming a
(-> (or date-style "EEEE, MMMM d, YYYY")
(post-process-date-style viz-settings))))
(defmethod format-timestring :week [timezone-id temporal-str _col _viz-settings]
(str (tru "Week ") (reformat-temporal-str timezone-id temporal-str "w - YYYY")))
(defmethod format-timestring :week [timezone-id temporal-str _col {:keys [date-style] :as viz-settings}]
(let [date-style (or date-style "MMMM d, YYYY")
end-temporal-str (-> temporal-str
u.date/parse
(u.date/add :day 6)
u.date/format)]
(str
(reformat-temporal-str timezone-id temporal-str (post-process-date-style date-style viz-settings))
" - "
(reformat-temporal-str timezone-id end-temporal-str (post-process-date-style date-style viz-settings)))))
(defmethod format-timestring :month [timezone-id temporal-str _col {:keys [date-style] :as viz-settings}]
(reformat-temporal-str timezone-id temporal-str
Expand Down
4 changes: 2 additions & 2 deletions test/metabase/formatter/datetime_test.clj
View file
Expand Up @@ -35,13 +35,13 @@
(is (= "July, 2020, 6:04 PM"
(datetime/format-temporal-str "UTC" now {:unit :minute}))))
(testing :hour
(is (= "July, 2020, 6 PM"
(is (= "July 16, 2020, 6 PM"
(datetime/format-temporal-str "UTC" now {:unit :hour}))))
(testing :day
(is (= "Thursday, July 16, 2020"
(datetime/format-temporal-str "UTC" now {:unit :day}))))
(testing :week
(is (= "Week 29 - 2020"
(is (= "July 16, 2020 - July 22, 2020"
(datetime/format-temporal-str "UTC" now {:unit :week}))))
(testing :month
(is (= "July, 2020"
Expand Down
35 changes: 24 additions & 11 deletions test/metabase/pulse/pulse_integration_test.clj
View file
Expand Up @@ -301,6 +301,8 @@
EXTRACT(YEAR FROM T.example_timestamp) AS example_year,
EXTRACT(MONTH FROM T.example_timestamp) AS example_month,
EXTRACT(DAY FROM T.example_timestamp) AS example_day,
EXTRACT(WEEK FROM T.example_timestamp) AS example_week_number,
T.example_timestamp AS example_week,
EXTRACT(HOUR FROM T.example_timestamp) AS example_hour,
EXTRACT(MINUTE FROM T.example_timestamp) AS example_minute,
EXTRACT(SECOND FROM T.example_timestamp) AS example_second
Expand All @@ -321,6 +323,8 @@
[:field "EXAMPLE_YEAR" {:base-type :type/Integer}]
[:field "EXAMPLE_MONTH" {:base-type :type/Integer}]
[:field "EXAMPLE_DAY" {:base-type :type/Integer}]
[:field "EXAMPLE_WEEK_NUMBER" {:base-type :type/Integer}]
[:field "EXAMPLE_WEEK" {:base-type :type/DateTime, :temporal-unit :week}]
[:field "EXAMPLE_HOUR" {:base-type :type/Integer}]
[:field "EXAMPLE_MINUTE" {:base-type :type/Integer}]
[:field "EXAMPLE_SECOND" {:base-type :type/Integer}]]
Expand All @@ -346,15 +350,15 @@
:query {:source-table
(format "card__%s" model-card-id)}}
:result_metadata (mapv
(fn [{column-name :name :as col}]
(cond-> col
(= "EXAMPLE_TIMESTAMP_WITH_TIME_ZONE" column-name)
(assoc :settings {:date_separator "-"
:date_style "YYYY/M/D"
:time_style "HH:mm"})
(= "EXAMPLE_TIMESTAMP" column-name)
(assoc :settings {:time_enabled "seconds"})))
model-metadata)
(fn [{column-name :name :as col}]
(cond-> col
(= "EXAMPLE_TIMESTAMP_WITH_TIME_ZONE" column-name)
(assoc :settings {:date_separator "-"
:date_style "YYYY/M/D"
:time_style "HH:mm"})
(= "EXAMPLE_TIMESTAMP" column-name)
(assoc :settings {:time_enabled "seconds"})))
model-metadata)
:visualization_settings {:column_settings {"[\"name\",\"FULL_DATETIME_UTC\"]"
{:date_abbreviate true
:time_enabled "milliseconds"
Expand Down Expand Up @@ -411,10 +415,14 @@
"EXAMPLE_YEAR" "2,023"
"EXAMPLE_MONTH" "12"
"EXAMPLE_DAY" "11"
"EXAMPLE_WEEK_NUMBER" "50"
"EXAMPLE_HOUR" "15"
"EXAMPLE_MINUTE" "30"
"EXAMPLE_SECOND" "45"}
native-results)))
;; the EXAMPLE_WEEK is a normal timestamp.
;; We care about it in the context of the Model, not the native results
;; so dissoc it here.
(dissoc native-results "EXAMPLE_WEEK"))))
(testing "An exported model retains the base format, but does use display names for column names."
(is (= {"Full Datetime Utc" "December 11, 2023, 3:30 PM"
"Full Datetime Pacific" "December 11, 2023, 3:30 PM"
Expand All @@ -425,6 +433,8 @@
"Example Year" "2,023"
"Example Month" "12"
"Example Day" "11"
"Example Week Number" "50"
"Example Week" "December 10, 2023 - December 16, 2023"
"Example Hour" "15"
"Example Minute" "30"
"Example Second" "45"}
Expand All @@ -443,7 +453,10 @@
(metamodel-results "Full Datetime Pacific"))))
(testing "Setting time-enabled to nil for a time column just returns an empty string"
(is (= ""
(metamodel-results "Example Time")))))))))
(metamodel-results "Example Time"))))
(testing "Week Units Are Displayed as a Date Range"
(is (= "December 10, 2023 - December 16, 2023"
(metamodel-results "Example Week")))))))))

(deftest renamed-column-names-are-applied-test
(testing "CSV attachments should have the same columns as displayed in Metabase (#18572)"
Expand Down

0 comments on commit 5ecebce

Please sign in to comment.