-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend param substitution to handle datetimes #38695
Conversation
Prior to this only dates were supported as values for native timestamp field parameters.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one test is strange. Is there a GH issue for the problem this PR is fixing? If there is, we should include it,
@@ -93,7 +93,7 @@ | |||
(mt/with-clock #t "2018-07-01T12:30:00.000Z" | |||
(mt/with-metadata-provider meta/metadata-provider | |||
(let [field-filter (params/map->FieldFilter | |||
{:field (lib.metadata/field (qp.store/metadata-provider) (meta/id :checkins :date)) | |||
{:field (lib.metadata/field (qp.store/metadata-provider) (meta/id :orders :created-at)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unexpected for me that the rest of the test doesn't need changing. Where does/should "PUBLIC"."CHECKINS"
come from?
@snoe Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
* Extend param substitution to handle datetimes Prior to this only dates were supported as values for native timestamp field parameters. * Check for exclusion filter when determining alignment unit
* Extend param substitution to handle datetimes Prior to this only dates were supported as values for native timestamp field parameters. * Check for exclusion filter when determining alignment unit Co-authored-by: Case Nelson <case@metabase.com>
@metabase-bot backport release-x.48.x |
@snoe I'm backporting to 48 because it looks like this never hit the 48 branch. |
@calherries something went wrong while creating a backport [Logs] |
* Extend param substitution to handle datetimes Prior to this only dates were supported as values for native timestamp field parameters. * Check for exclusion filter when determining alignment unit Co-authored-by: Case Nelson <case@metabase.com>
Fixes #38037
Fixes #33492
Prior to this only dates were supported as values for native timestamp field parameters. By properly aligning the temporal unit, we can ensure that native filters match gui filters.