Fork Sync: Update from parent repository #49
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
* Use `minWidth` instead of `width` * Reserve left padding for the settings icon
Fixes #36459. Also makes sure that legacy to pMBQL works and that simple value expressions can be named.
* Apply column formatting to CSV exports
This PR applies the same formatting logic to CSV exports as it does to pulse bodies (e.g. HTML).
Formerly, there were two related formatting paths:
- `metabase.query-processor.streaming.common/format-value`, which is from a protocol that takes an object and returns a string. It is what was used to export csv data. It applies no actual formatting, only converts objects to strings.
- `metabase.pulse.render.body/get-format`, which builds a formatter from an optional time zone, column metadata, and visualization settings. This formatter takes a string and formats it. It was only used for rendering inline artifacts, such as embedded HTML in emails.
The first function is insufficient to format row data as it is unaware of the column metadata and viz settings. We need to use that data everywhere data is exported in a uniform way.
The solution is to lift `get-format` from `metabase.pulse.render.body` to a more common location (`metabase.pulse.render.common` in this PR step, but needs to be moved out of the pulse code to be a more shared concern) and use it everywhere artifacts are generated.
For csv export, this was achieved as follows in `metabase.query-processor.streaming.csv`:
```clojure
(defmethod qp.si/streaming-results-writer :csv
[_ ^OutputStream os]
(let [writer (BufferedWriter. (OutputStreamWriter. os StandardCharsets/UTF_8))
formatters (atom {})]
(reify qp.si/StreamingResultsWriter
(begin! [_ {{:keys [ordered-cols results_timezone]} :data} viz-settings]
(swap! formatters (constantly (zipmap
ordered-cols
(map (fn [col]
(p.common/get-format results_timezone col viz-settings))
ordered-cols))))
(csv/write-csv writer [(map (some-fn :display_name :name) ordered-cols)])
(.flush writer))
(write-row! [_ row _row-num cols {:keys [output-order]}]
(let [[ordered-row
ordered-cols] (if output-order
(let [row-v (into [] row)
cols-v (into [] cols)]
[(for [i output-order] (row-v i))
(for [i output-order] (cols-v i))])
[row cols])]
(csv/write-csv writer [(map (fn [c r]
(let [formatter (@Formatters c)]
(formatter (common/format-value r))))
ordered-cols ordered-row)])
(.flush writer)))
(finish! [_ _]
;; TODO -- not sure we need to flush both
(.flush writer)
(.flush os)
(.close writer)))))
```
The formatters for each column are build in the `begin!` function and then looked up in each `write-row!`. The existing `format-value` is used to produce a string then passed into our looked up column formatter.
Note that the new unit tests simulate a pulse and grab the provided temp files as attachments and analyzes those for correctness. This should work in a CI environment so long as the test process has permission to both write attachments to the temp directory and read those attachments back out. Also note that these tests can be slow (but not terribly so).
Primary changes:
- `metabase.email.messages` - fix spelling
- `metabase.pulse.render.body` - move `get-format` out of this ns
- `metabase.pulse.render.common` - move `get-format` into this ns
- `metabase.query-processor.streaming.csv` - new logic to apply pulse renderer formatting to csv exports
- `metabase.pulse.pulse-integration-test` - adding unit tests
One TODO before a final commit of this PR is to move the `get-format` logic out of a pulse ns into something more general. Ultimately, it would be nice if this was a common capability used by both BE and FE.
* Updated formatter state
The rows in qp.si/StreamingResultsWriter are ordered, so there should be no need to do a lookup by col. We should just be able to map the ordered-formatters (using the same order as the cols) with the ordered row data.
* Updating tests in `metabase.query-processor.streaming-test` to reflect new csv formatting.
* Updated fomatter for tests and consistency
This updates several tests (metabase.query-processor.streaming.csv-test) to reflect the new and correct behavior of using a common csv formatter as well as update the formatting code to correctly handle relational types and datetimes.
* require fmt
* Updating metabase.pulse.render.body-test to reflect `:type/DateTime` formatting.
* Updating sqlite test
* Updating report-timezone-test to reflect new CSV rendering
* Fixing e2e test
* [MLv2] drop-stage-if-empty * Addressing PR comments
feat:: white labeling option for help link
* fix types * swap unit test step order
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.