Skip to content
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

Introduce inheritance to the Parameter structure #4049

Merged
merged 30 commits into from
Oct 24, 2019
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c81f46e
Start draft for new Parameter structure
gabrieldutra Aug 7, 2019
20a2f83
Add the rest of the methods
gabrieldutra Aug 7, 2019
5c7e745
EnumParameter
gabrieldutra Aug 7, 2019
a05d4b2
QueryBasedDropdownParameter
gabrieldutra Aug 7, 2019
3c52dcb
DateParameter
gabrieldutra Aug 7, 2019
db009df
DateRangeParameter
gabrieldutra Aug 8, 2019
8cccf8b
Update Parameter usage on code
gabrieldutra Aug 8, 2019
a1a3421
Merge dynamicValue into normalizedValue
gabrieldutra Aug 11, 2019
6b98f3d
Merge branch 'master' into parameters-structure
gabrieldutra Sep 4, 2019
622240d
Add updateLocals and omit unwanted props
gabrieldutra Sep 5, 2019
5dcacbe
Allow null NumberParameter and omit parentQueryId
gabrieldutra Sep 7, 2019
f7a382b
Merge branch 'master' into parameters-structure
gabrieldutra Sep 11, 2019
3279e88
Rename parameter getValue to getExecutionValue
gabrieldutra Sep 11, 2019
c4c0418
Update $$value to normalizedValue + omit on save
gabrieldutra Sep 11, 2019
bd09b34
Merge branch 'master' into parameters-structure
gabrieldutra Sep 16, 2019
a6df8b3
Add a few comments
gabrieldutra Sep 16, 2019
8e0a684
Remove ngModel property from Parameter
gabrieldutra Sep 17, 2019
6b09a84
Use value directly in DateRangeParameter
gabrieldutra Sep 18, 2019
0374841
Use simpler separator for DateRange url param
gabrieldutra Sep 18, 2019
0e5782e
Add backward compatibility
gabrieldutra Sep 19, 2019
3718047
Merge branch 'master' into parameters-structure
gabrieldutra Sep 23, 2019
b63b584
Merge branch 'master' into parameters-structure
gabrieldutra Sep 29, 2019
da7255e
Use normalizeValue null value for isEmpty
gabrieldutra Sep 29, 2019
927783d
Start creating jest tests
gabrieldutra Oct 5, 2019
3a3c08f
Add more tests
gabrieldutra Oct 6, 2019
b970fb0
Merge branch 'master' into parameters-structure
gabrieldutra Oct 6, 2019
567536d
Merge branch 'master' into parameters-structure
gabrieldutra Oct 10, 2019
d269a38
Normalize null value for multi mode in Enum
gabrieldutra Oct 10, 2019
82c1104
Use saved value for param isEmpty
gabrieldutra Oct 11, 2019
56268b3
Merge branch 'master' into parameters-structure
arikfr Oct 24, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions client/app/components/ParameterMappingInput.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import Form from 'antd/lib/form';
import Tooltip from 'antd/lib/tooltip';
import ParameterValueInput from '@/components/ParameterValueInput';
import { ParameterMappingType } from '@/services/widget';
import { Parameter } from '@/services/query';
import { Parameter } from '@/services/parameters';
import { HelpTrigger } from '@/components/HelpTrigger';

import './ParameterMappingInput.less';
Expand Down Expand Up @@ -536,11 +536,11 @@ export class ParameterMappingListInput extends React.Component {
param = param.clone().setValue(mapping.value);
}

let value = Parameter.getValue(param);
let value = Parameter.getExecutionValue(param);

// in case of dynamic value display the name instead of value
if (param.hasDynamicValue) {
value = param.dynamicValue.name;
value = param.normalizedValue.name;
}

return this.getStringValue(value);
Expand Down
4 changes: 2 additions & 2 deletions client/app/components/ParameterValueInput.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import Input from 'antd/lib/input';
import InputNumber from 'antd/lib/input-number';
import DateParameter from '@/components/dynamic-parameters/DateParameter';
import DateRangeParameter from '@/components/dynamic-parameters/DateRangeParameter';
import { toString } from 'lodash';
import { isEqual } from 'lodash';
import { QueryBasedParameterInput } from './QueryBasedParameterInput';

import './ParameterValueInput.less';
Expand Down Expand Up @@ -61,7 +61,7 @@ class ParameterValueInput extends React.Component {
}

onSelect = (value) => {
const isDirty = toString(value) !== toString(this.props.value);
const isDirty = !isEqual(value, this.props.value);
this.setState({ value, isDirty });
this.props.onSelect(value, isDirty);
}
Expand Down
4 changes: 2 additions & 2 deletions client/app/components/Parameters.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { size, filter, forEach, extend } from 'lodash';
import { react2angular } from 'react2angular';
import { sortableContainer, sortableElement, sortableHandle } from 'react-sortable-hoc';
import { $location } from '@/services/ng';
import { Parameter } from '@/services/query';
import { Parameter } from '@/services/parameters';
import ParameterApplyButton from '@/components/ParameterApplyButton';
import ParameterValueInput from '@/components/ParameterValueInput';
import EditParameterSettingsDialog from './EditParameterSettingsDialog';
Expand Down Expand Up @@ -128,7 +128,7 @@ export class Parameters extends React.Component {
.result.then((updated) => {
this.setState(({ parameters }) => {
const updatedParameter = extend(parameter, updated);
parameters[index] = new Parameter(updatedParameter, updatedParameter.parentQueryId);
parameters[index] = Parameter.create(updatedParameter, updatedParameter.parentQueryId);
onParametersEdit();
return { parameters };
});
Expand Down
14 changes: 7 additions & 7 deletions client/app/components/dynamic-parameters/DateParameter.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import PropTypes from 'prop-types';
import classNames from 'classnames';
import moment from 'moment';
import { includes } from 'lodash';
import { isDynamicDate, getDynamicDate } from '@/services/query';
import { isDynamicDate, getDynamicDateFromString } from '@/services/parameters/DateParameter';
import DateInput from '@/components/DateInput';
import DateTimeInput from '@/components/DateTimeInput';
import DynamicButton from '@/components/dynamic-parameters/DynamicButton';
Expand All @@ -12,11 +12,11 @@ import './DynamicParameters.less';

const DYNAMIC_DATE_OPTIONS = [
{ name: 'Today/Now',
value: 'd_now',
label: () => getDynamicDate('d_now').value().format('MMM D') },
value: getDynamicDateFromString('d_now'),
label: () => getDynamicDateFromString('d_now').value().format('MMM D') },
{ name: 'Yesterday',
value: 'd_yesterday',
label: () => getDynamicDate('d_yesterday').value().format('MMM D') },
value: getDynamicDateFromString('d_yesterday'),
label: () => getDynamicDateFromString('d_yesterday').value().format('MMM D') },
];

class DateParameter extends React.Component {
Expand Down Expand Up @@ -44,7 +44,7 @@ class DateParameter extends React.Component {
onDynamicValueSelect = (dynamicValue) => {
const { onSelect, parameter } = this.props;
if (dynamicValue === 'static') {
const parameterValue = parameter.getValue();
const parameterValue = parameter.getExecutionValue();
if (parameterValue) {
onSelect(moment(parameterValue));
} else {
Expand Down Expand Up @@ -77,7 +77,7 @@ class DateParameter extends React.Component {
}

if (hasDynamicValue) {
const dynamicDate = getDynamicDate(value);
const dynamicDate = value;
additionalAttributes.placeholder = dynamicDate && dynamicDate.name;
additionalAttributes.value = null;
}
Expand Down
47 changes: 27 additions & 20 deletions client/app/components/dynamic-parameters/DateRangeParameter.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import PropTypes from 'prop-types';
import classNames from 'classnames';
import moment from 'moment';
import { includes, isArray, isObject } from 'lodash';
import { isDynamicDateRange, getDynamicDateRange } from '@/services/query';
import { isDynamicDateRange, getDynamicDateRangeFromString } from '@/services/parameters/DateRangeParameter';
import DateRangeInput from '@/components/DateRangeInput';
import DateTimeRangeInput from '@/components/DateTimeRangeInput';
import DynamicButton from '@/components/dynamic-parameters/DynamicButton';
Expand All @@ -12,29 +12,37 @@ import './DynamicParameters.less';

const DYNAMIC_DATE_OPTIONS = [
{ name: 'This week',
value: 'd_this_week',
label: () => getDynamicDateRange('d_this_week').value()[0].format('MMM D') + ' - ' +
getDynamicDateRange('d_this_week').value()[1].format('MMM D') },
{ name: 'This month', value: 'd_this_month', label: () => getDynamicDateRange('d_this_month').value()[0].format('MMMM') },
{ name: 'This year', value: 'd_this_year', label: () => getDynamicDateRange('d_this_year').value()[0].format('YYYY') },
value: getDynamicDateRangeFromString('d_this_week'),
label: () => getDynamicDateRangeFromString('d_this_week').value()[0].format('MMM D') + ' - ' +
getDynamicDateRangeFromString('d_this_week').value()[1].format('MMM D') },
{ name: 'This month',
value: getDynamicDateRangeFromString('d_this_month'),
label: () => getDynamicDateRangeFromString('d_this_month').value()[0].format('MMMM') },
{ name: 'This year',
value: getDynamicDateRangeFromString('d_this_year'),
label: () => getDynamicDateRangeFromString('d_this_year').value()[0].format('YYYY') },
{ name: 'Last week',
value: 'd_last_week',
label: () => getDynamicDateRange('d_last_week').value()[0].format('MMM D') + ' - ' +
getDynamicDateRange('d_last_week').value()[1].format('MMM D') },
{ name: 'Last month', value: 'd_last_month', label: () => getDynamicDateRange('d_last_month').value()[0].format('MMMM') },
{ name: 'Last year', value: 'd_last_year', label: () => getDynamicDateRange('d_last_year').value()[0].format('YYYY') },
value: getDynamicDateRangeFromString('d_last_week'),
label: () => getDynamicDateRangeFromString('d_last_week').value()[0].format('MMM D') + ' - ' +
getDynamicDateRangeFromString('d_last_week').value()[1].format('MMM D') },
{ name: 'Last month',
value: getDynamicDateRangeFromString('d_last_month'),
label: () => getDynamicDateRangeFromString('d_last_month').value()[0].format('MMMM') },
{ name: 'Last year',
value: getDynamicDateRangeFromString('d_last_year'),
label: () => getDynamicDateRangeFromString('d_last_year').value()[0].format('YYYY') },
{ name: 'Last 7 days',
value: 'd_last_7_days',
label: () => getDynamicDateRange('d_last_7_days').value()[0].format('MMM D') + ' - Today' },
value: getDynamicDateRangeFromString('d_last_7_days'),
label: () => getDynamicDateRangeFromString('d_last_7_days').value()[0].format('MMM D') + ' - Today' },
];

const DYNAMIC_DATETIME_OPTIONS = [
{ name: 'Today',
value: 'd_today',
label: () => getDynamicDateRange('d_today').value()[0].format('MMM D') },
value: getDynamicDateRangeFromString('d_today'),
label: () => getDynamicDateRangeFromString('d_today').value()[0].format('MMM D') },
{ name: 'Yesterday',
value: 'd_yesterday',
label: () => getDynamicDateRange('d_yesterday').value()[0].format('MMM D') },
value: getDynamicDateRangeFromString('d_yesterday'),
label: () => getDynamicDateRangeFromString('d_yesterday').value()[0].format('MMM D') },
...DYNAMIC_DATE_OPTIONS,
];

Expand Down Expand Up @@ -73,7 +81,7 @@ class DateRangeParameter extends React.Component {
onDynamicValueSelect = (dynamicValue) => {
const { onSelect, parameter } = this.props;
if (dynamicValue === 'static') {
const parameterValue = parameter.getValue();
const parameterValue = parameter.getExecutionValue();
if (isObject(parameterValue) && parameterValue.start && parameterValue.end) {
onSelect([moment(parameterValue.start), moment(parameterValue.end)]);
} else {
Expand Down Expand Up @@ -107,8 +115,7 @@ class DateRangeParameter extends React.Component {
}

if (hasDynamicValue) {
const dynamicDateRange = getDynamicDateRange(value);
additionalAttributes.placeholder = [dynamicDateRange && dynamicDateRange.name];
additionalAttributes.placeholder = [value && value.name];
additionalAttributes.value = null;
}

Expand Down
4 changes: 3 additions & 1 deletion client/app/components/dynamic-parameters/DynamicButton.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import Dropdown from 'antd/lib/dropdown';
import Icon from 'antd/lib/icon';
import Menu from 'antd/lib/menu';
import Typography from 'antd/lib/typography';
import { DynamicDateType } from '@/services/parameters/DateParameter';
import { DynamicDateRangeType } from '@/services/parameters/DateRangeParameter';

import './DynamicButton.less';

Expand Down Expand Up @@ -62,7 +64,7 @@ function DynamicButton({ options, selectedDynamicValue, onSelect, enabled }) {

DynamicButton.propTypes = {
options: PropTypes.arrayOf(PropTypes.object), // eslint-disable-line react/forbid-prop-types
selectedDynamicValue: PropTypes.string,
selectedDynamicValue: PropTypes.oneOfType([DynamicDateType, DynamicDateRangeType]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuffs

onSelect: PropTypes.func,
enabled: PropTypes.bool,
};
Expand Down
4 changes: 2 additions & 2 deletions client/app/pages/queries/view.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { pick, some, find, minBy, map, intersection, isEmpty, isArray, omit } from 'lodash';
import { pick, some, find, minBy, map, intersection, isEmpty, isArray } from 'lodash';
import { SCHEMA_NOT_SUPPORTED, SCHEMA_LOAD_ERROR } from '@/services/data-source';
import getTags from '@/services/getTags';
import { policy } from '@/services/policy';
Expand Down Expand Up @@ -261,7 +261,7 @@ function QueryViewCtrl(
if (request.options && request.options.parameters) {
request.options = {
...request.options,
parameters: map(request.options.parameters, p => omit(p, 'pendingValue')),
parameters: map(request.options.parameters, p => p.toSaveableObject()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuffs

};
}

Expand Down
95 changes: 95 additions & 0 deletions client/app/services/parameters/DateParameter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { findKey, startsWith, has, includes, isNull, values } from 'lodash';
import moment from 'moment';
import PropTypes from 'prop-types';
import { Parameter } from '.';

const DATETIME_FORMATS = {
// eslint-disable-next-line quote-props
'date': 'YYYY-MM-DD',
'datetime-local': 'YYYY-MM-DD HH:mm',
'datetime-with-seconds': 'YYYY-MM-DD HH:mm:ss',
};

const DYNAMIC_PREFIX = 'd_';

const DYNAMIC_DATES = {
now: {
name: 'Today/Now',
value: () => moment(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: shouldn't it be moment.utc()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we can and want to use the user's TZ (I believe it's less confusing and it reaches more use cases).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When user selects a dynamic value for parameter - what do we pass to a server when query is executed: "magic" constant or computed value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Computed value (actual "dates/date ranges")

Copy link
Collaborator

@kravets-levko kravets-levko Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I think, it may be an issue. Imagine that there is a query like SELECT * FROM some_table WHERE created_at >= '{{ param }}', and that DB server is configured to use UTC, so created_at column contains UTC values. Now, imagine that I want to pass yesterday as param value. My local time zone is GMT+3, and if I'll try to use that param with yesterday, server will return results not for last 24 hours, but for last 21 hours - because client will pass my local time, but server will treat it as UTC - so there will be 3 hours difference. Also, this example can be extrapolated for a case with few users in different time zones - then each of them will get results for different period.

So shouldn't we always pass date/time/range params in UTC to make them predictable and not dependent on DB config and user's locale settings?

Copy link
Member Author

@gabrieldutra gabrieldutra Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 you have a point, from my experience I've always used CONVERT_TZ in the queries so it would be less confusing for the people that were only going to view it and get the results. Though everyone had the same TZ in there.

I still have the feeling that the user TZ is the less confusing when considering the user, so I believe it's not a bug, it's one acceptable behavior for this. But we could eventually offer more options to solve this.

This is certainly not in the scope of this PR, so I say we should move this discussion to somewhere else with more visibility (forum or create an issue).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point about CONVERT_TZ, but I described a bit different case: when we send param value to DB server - it does not know in which TZ that value actually is, and what's the difference between server's TZ and provided value. Even more: values that come from different users may be in different TZ, which is totally impossible to handle in a single query. So actually there are two solutions: 1) pass TZ in param value (ISO8601 allows this), or 2) pass UTC (we can show value in any TZ in UI, but when executing query - convert it to UTC).

It's definitely not in the scope of this PR, but we have to discuss it somewhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have it as moment.utc() in this PR (I assume this is the current behavior?) and start a discussion about how to handle timezones apart from this PR. It's a broad enough topic that requires deeper discussion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behaviour is moment(), so I'm assuming that's the one you meant we should keep in this scope.

},
yesterday: {
name: 'Yesterday',
value: () => moment().subtract(1, 'day'),
},
};

export const DynamicDateType = PropTypes.oneOf(values(DYNAMIC_DATES));

function isDynamicDateString(value) {
return startsWith(value, DYNAMIC_PREFIX) && has(DYNAMIC_DATES, value.substring(DYNAMIC_PREFIX.length));
}

export function isDynamicDate(value) {
return includes(DYNAMIC_DATES, value);
}

export function getDynamicDateFromString(value) {
if (!isDynamicDateString(value)) {
return null;
}
return DYNAMIC_DATES[value.substring(DYNAMIC_PREFIX.length)];
}

class DateParameter extends Parameter {
constructor(parameter, parentQueryId) {
super(parameter, parentQueryId);
this.useCurrentDateTime = parameter.useCurrentDateTime;
this.setValue(parameter.value);
}

get hasDynamicValue() {
return isDynamicDate(this.normalizedValue);
}

// eslint-disable-next-line class-methods-use-this
normalizeValue(value) {
if (isDynamicDateString(value)) {
return getDynamicDateFromString(value);
}

if (isDynamicDate(value)) {
return value;
}

const normalizedValue = moment(value);
return normalizedValue.isValid() ? normalizedValue : null;
}

setValue(value) {
const normalizedValue = this.normalizeValue(value);
if (isDynamicDate(normalizedValue)) {
this.value = DYNAMIC_PREFIX + findKey(DYNAMIC_DATES, normalizedValue);
} else if (moment.isMoment(normalizedValue)) {
this.value = normalizedValue.format(DATETIME_FORMATS[this.type]);
} else {
this.value = normalizedValue;
}
this.$$value = normalizedValue;

this.updateLocals();
this.clearPendingValue();
return this;
}

getExecutionValue() {
if (this.hasDynamicValue) {
return this.normalizedValue.value().format(DATETIME_FORMATS[this.type]);
}
if (isNull(this.value) && this.useCurrentDateTime) {
return moment().format(DATETIME_FORMATS[this.type]);
}
return this.value;
}
}

export default DateParameter;
Loading