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
Convert timestamp quickvalues to formatted date #4423
Conversation
…e to formatted date otherwise the search store will end up searching for the unix timestamp which is not supported fixes #4288
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.
There are still a couple of changes we need to do, see inline comments.
|
||
if (this.props.field === 'timestamp') { | ||
// convert unix timestamp to proper formatted value, so that add to search button works correctly | ||
formattedTerm = moment.utc(Number(d.term)).format(DateTime.Formats.TIMESTAMP); |
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.
This does not display date times in the user time zone, providing inconsistent values with the search results. We should use new DateTime(Number(d.term)).toString(DateTime.Formats.TIMESTAMP)
to get the time formatted in the user's time zone.
columns.push(d => this._getAddToSearchButton(d.term)); | ||
if (this.props.field === 'timestamp') { | ||
// convert unix timestamp to proper formatted value, so that add to search button works correctly | ||
columns.push(d => this._getAddToSearchButton(moment.utc(Number(d.term)).format(DateTime.Formats.TIMESTAMP))); |
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.
This also has problems with the user TZ, as the value from here is later converted into UTC (see gif below) and when pressing the search button we can't find any results. We should use, as in the other case new DateTime(Number(d.term)).toString(DateTime.Formats.TIMESTAMP)
to convert the unix time in the user's selected time zone.
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.
By the way, as both date times should be formatted the same way, I would extract the conversion into a function for extra clarity.
extract repeated code into function
Moving to 3.0.0 milestone. |
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 👍
* when displaying quick values for the timestamp field convert unix time to formatted date otherwise the search store will end up searching for the unix timestamp which is not supported fixes #4288 * respect timezones when stringifying extract repeated code into function (cherry picked from commit b4af02b)
otherwise the search store will end up searching for the unix timestamp which is not supported
fixes #4288