Skip to content

Commit

Permalink
Ensure we use mbqlEq when comparing 'time-interval' MBQL clauses. Res…
Browse files Browse the repository at this point in the history 
…olves metabase#4231
  • Loading branch information
@tlrobinson
tlrobinson committed Jan 25, 2017
1 parent 301a449 commit 9c645bc
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 35 deletions.
32 changes: 16 additions & 16 deletions frontend/src/metabase/dashboard/components/parameters/widgets/DateRelativeWidget.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,20 @@ import _ from "underscore";
const SHORTCUTS = [
{ name: "Today", operator: ["=", "<", ">"], values: [["relative_datetime", "current"]]},
{ name: "Yesterday", operator: ["=", "<", ">"], values: [["relative_datetime", -1, "day"]]},
{ name: "Past 7 days", operator: "TIME_INTERVAL", values: [-7, "day"]},
{ name: "Past 30 days", operator: "TIME_INTERVAL", values: [-30, "day"]}
{ name: "Past 7 days", operator: "time-interval", values: [-7, "day"]},
{ name: "Past 30 days", operator: "time-interval", values: [-30, "day"]}
];

const RELATIVE_SHORTCUTS = {
"Last": [
{ name: "Week", operator: "TIME_INTERVAL", values: ["last", "week"]},
{ name: "Month", operator: "TIME_INTERVAL", values: ["last", "month"]},
{ name: "Year", operator: "TIME_INTERVAL", values: ["last", "year"]}
{ name: "Week", operator: "time-interval", values: ["last", "week"]},
{ name: "Month", operator: "time-interval", values: ["last", "month"]},
{ name: "Year", operator: "time-interval", values: ["last", "year"]}
],
"This": [
{ name: "Week", operator: "TIME_INTERVAL", values: ["current", "week"]},
{ name: "Month", operator: "TIME_INTERVAL", values: ["current", "month"]},
{ name: "Year", operator: "TIME_INTERVAL", values: ["current", "year"]}
{ name: "Week", operator: "time-interval", values: ["current", "week"]},
{ name: "Month", operator: "time-interval", values: ["current", "month"]},
{ name: "Year", operator: "time-interval", values: ["current", "year"]}
]
};

Expand Down Expand Up @@ -112,35 +112,35 @@ const FILTERS = {
},
"past7days": {
name: "Past 7 Days",
mapping: ["TIME_INTERVAL", null, -7, "day"]
mapping: ["time-interval", null, -7, "day"]
},
"past30days": {
name: "Past 30 Days",
mapping: ["TIME_INTERVAL", null, -30, "day"]
mapping: ["time-interval", null, -30, "day"]
},
"lastweek": {
name: "Last Week",
mapping: ["TIME_INTERVAL", null, "last", "week"]
mapping: ["time-interval", null, "last", "week"]
},
"lastmonth": {
name: "Last Month",
mapping: ["TIME_INTERVAL", null, "last", "month"]
mapping: ["time-interval", null, "last", "month"]
},
"lastyear": {
name: "Last Year",
mapping: ["TIME_INTERVAL", null, "last", "year"]
mapping: ["time-interval", null, "last", "year"]
},
"thisweek": {
name: "This Week",
mapping: ["TIME_INTERVAL", null, "current", "week"]
mapping: ["time-interval", null, "current", "week"]
},
"thismonth": {
name: "This Month",
mapping: ["TIME_INTERVAL", null, "current", "month"]
mapping: ["time-interval", null, "current", "month"]
},
"thisyear": {
name: "This Year",
mapping: ["TIME_INTERVAL", null, "current", "year"]
mapping: ["time-interval", null, "current", "year"]
}
};

Expand Down
10 changes: 6 additions & 4 deletions frontend/src/metabase/lib/query_time.js
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import moment from "moment";
import inflection from "inflection";

import { mbqlEq } from "metabase/lib/query/util";

export function computeFilterTimeRange(filter) {
let expandedFilter;
if (filter[0] === "TIME_INTERVAL") {
if (mbqlEq(filter[0], "time-interval")) {
expandedFilter = expandTimeIntervalFilter(filter);
} else {
expandedFilter = filter;
Expand Down Expand Up @@ -34,8 +36,8 @@ export function computeFilterTimeRange(filter) {
export function expandTimeIntervalFilter(filter) {
let [operator, field, n, unit] = filter;

if (operator !== "TIME_INTERVAL") {
throw new Error("translateTimeInterval expects operator TIME_INTERVAL");
if (!mbqlEq(operator, "time-interval")) {
throw new Error("translateTimeInterval expects operator 'time-interval'");
}

if (n === "current") {
Expand Down Expand Up @@ -63,7 +65,7 @@ export function generateTimeFilterValuesDescriptions(filter) {
let [operator, field, ...values] = filter;
let bucketing = parseFieldBucketing(field);

if (operator === "TIME_INTERVAL") {
if (mbqlEq(operator, "time-interval")) {
let [n, unit] = values;
return generateTimeIntervalDescription(n, unit);
} else {
Expand Down
12 changes: 7 additions & 5 deletions frontend/src/metabase/query_builder/components/filters/pickers/DatePicker.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import Calendar from "metabase/components/Calendar";

import moment from "moment";

import { mbqlEq } from "metabase/lib/query/util";

import _ from "underscore";

import type { FieldFilter, TimeIntervalFilter } from "metabase/meta/types/Query";
Expand Down Expand Up @@ -79,8 +81,8 @@ class CurrentPicker extends Component<*, CurentPickerProps, CurrentPickerState>
}


const getIntervals = ([op, field, value, unit]) => op === "time-interval" && typeof value === "number" ? Math.abs(value) : 30;
const getUnit = ([op, field, value, unit]) => op === "time-interval" && unit ? unit : "day";
const getIntervals = ([op, field, value, unit]) => mbqlEq(op, "time-interval") && typeof value === "number" ? Math.abs(value) : 30;
const getUnit = ([op, field, value, unit]) => mbqlEq(op, "time-interval") && unit ? unit : "day";
const getDate = (value) => typeof value === "string" && moment(value).isValid() ? value : moment().format("YYYY-MM-DD");


Expand All @@ -96,20 +98,20 @@ const OPERATORS: Operator[] = [
name: "Previous",
init: (filter) => ["time-interval", filter[1], -getIntervals(filter), getUnit(filter)],
// $FlowFixMe
test: ([op, field, value]) => op === "time-interval" && value < 0 || Object.is(value, -0),
test: ([op, field, value]) => mbqlEq(op, "time-interval") && value < 0 || Object.is(value, -0),
widget: PreviousPicker,
},
{
name: "Next",
init: (filter) => ["time-interval", filter[1], getIntervals(filter), getUnit(filter)],
// $FlowFixMe
test: ([op, field, value]) => op === "time-interval" && value >= 0,
test: ([op, field, value]) => mbqlEq(op, "time-interval") && value >= 0,
widget: NextPicker,
},
{
name: "Current",
init: (filter) => ["time-interval", filter[1], "current", getUnit(filter)],
test: ([op, field, value]) => op === "time-interval" && value === "current",
test: ([op, field, value]) => mbqlEq(op, "time-interval") && value === "current",
widget: CurrentPicker,
},
{
Expand Down
43 changes: 33 additions & 10 deletions frontend/test/unit/lib/query_time.spec.js
View file
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
import moment from "moment";

import { expandTimeIntervalFilter, computeFilterTimeRange, absolute } from 'metabase/lib/query_time';
import { expandTimeIntervalFilter, computeFilterTimeRange, absolute, generateTimeFilterValuesDescriptions } from 'metabase/lib/query_time';

describe('query_time', () => {
describe('expandTimeIntervalFilter', () => {
it('translate ["current" "month"] correctly', () => {
expect(
JSON.stringify(expandTimeIntervalFilter(["TIME_INTERVAL", 100, "current", "month"]))
).toBe(
JSON.stringify(["=", ["datetime_field", 100, "as", "month"], ["relative_datetime", "current"]])
expandTimeIntervalFilter(["time-interval", 100, "current", "month"])
).toEqual(
["=", ["datetime_field", 100, "as", "month"], ["relative_datetime", "current"]]
);
});
it('translate [-30, "day"] correctly', () => {
expect(
JSON.stringify(expandTimeIntervalFilter(["TIME_INTERVAL", 100, -30, "day"]))
).toBe(
JSON.stringify(["BETWEEN", ["datetime_field", 100, "as", "day"], ["relative_datetime", -31, "day"], ["relative_datetime", -1, "day"]])
expandTimeIntervalFilter(["time-interval", 100, -30, "day"])
).toEqual(
["BETWEEN", ["datetime_field", 100, "as", "day"], ["relative_datetime", -31, "day"], ["relative_datetime", -1, "day"]]
);
});
});
Expand Down Expand Up @@ -46,6 +46,29 @@ describe('query_time', () => {
});
});

describe("generateTimeFilterValuesDescriptions", () => {
it ("should format simple operator values correctly", () => {
expect(generateTimeFilterValuesDescriptions(["<", null, "2016-01-01"])).toEqual(["January 1, 2016"])
})
it ("should format 'time-interval' correctly", () => {
expect(generateTimeFilterValuesDescriptions(["time-interval", null, -30, "day"])).toEqual(["Past 30 Days"])
expect(generateTimeFilterValuesDescriptions(["time-interval", null, 1, "month"])).toEqual(["Next 1 Month"])
expect(generateTimeFilterValuesDescriptions(["time-interval", null, 2, "month"])).toEqual(["Next 2 Months"])
expect(generateTimeFilterValuesDescriptions(["time-interval", null, 0, "month"])).toEqual(["This Month"])
expect(generateTimeFilterValuesDescriptions(["time-interval", null, -1, "month"])).toEqual(["Past 1 Month"])
expect(generateTimeFilterValuesDescriptions(["time-interval", null, -2, "month"])).toEqual(["Past 2 Months"])
});
it ("should format 'time-interval' short names correctly", () => {
expect(generateTimeFilterValuesDescriptions(["time-interval", null, -1, "day"])).toEqual(["Yesterday"])
expect(generateTimeFilterValuesDescriptions(["time-interval", null, 0, "day"])).toEqual(["Today"])
expect(generateTimeFilterValuesDescriptions(["time-interval", null, "current", "day"])).toEqual(["Today"])
expect(generateTimeFilterValuesDescriptions(["time-interval", null, 1, "day"])).toEqual(["Tomorrow"])
});
it ("should format legacy 'TIME_INTERVAL' correctly", () => {
expect(generateTimeFilterValuesDescriptions(["TIME_INTERVAL", null, -30, "day"])).toEqual(["Past 30 Days"])
});
});

describe('computeFilterTimeRange', () => {
describe('absolute dates', () => {
it ('should handle "="', () => {
Expand Down Expand Up @@ -93,14 +116,14 @@ describe('query_time', () => {
});
});

describe('TIME_INTERVAL', () => {
describe('time-interval', () => {
it ('should handle "Past x days"', () => {
let [start, end] = computeFilterTimeRange(["TIME_INTERVAL", 1, -7, "day"]);
let [start, end] = computeFilterTimeRange(["time-interval", 1, -7, "day"]);
expect(start.format("YYYY-MM-DD HH:mm:ss")).toEqual(moment().subtract(8, "day").format("YYYY-MM-DD 00:00:00"));
expect(end.format("YYYY-MM-DD HH:mm:ss")).toEqual(moment().subtract(1, "day").format("YYYY-MM-DD 23:59:59"));
});
// it ('should handle "last week"', () => {
// let [start, end] = computeFilterTimeRange(["TIME_INTERVAL", 1, "last", "week"]);
// let [start, end] = computeFilterTimeRange(["time-interval", 1, "last", "week"]);
// expect(start.format("YYYY-MM-DD HH:mm:ss")).toEqual(moment().subtract(1, "week").startOf("week").format("YYYY-MM-DD 00:00:00"));
// expect(end.format("YYYY-MM-DD HH:mm:ss")).toEqual(moment().subtract(1, "week").endOf("week")..format("YYYY-MM-DD 23:59:59"));
// });
Expand Down

0 comments on commit 9c645bc

Please sign in to comment.