-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix(discover2): Various fixes #15315
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
Conversation
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.
Seems valid to me
54cfce6
to
de14b90
Compare
|
||
import {URL_PARAM} from 'app/constants/globalSelectionHeader'; | ||
|
||
// Parses URL query parameters for values relevant to global selection header | ||
export function getStateFromQuery(query) { | ||
let start = query[URL_PARAM.START] !== 'null' && query[URL_PARAM.START]; | ||
let end = query[URL_PARAM.END] !== 'null' && query[URL_PARAM.END]; | ||
const parsedParams = getParams(query); |
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.
I'm seeing some issues stemming from the global selection header propagating down to discover2.
Invalid datetime selections can easily throw an error and break the app with no actual recovery flow.
I feel it's better to parse and validate the datetime selection early here; rather than doing it at the API payload generation everywhere.
@@ -2,17 +2,20 @@ import {pick, pickBy, identity} from 'lodash'; | |||
|
|||
import {defined} from 'app/utils'; | |||
import {getUtcToLocalDateObject} from 'app/utils/dates'; | |||
import {getParams} from 'app/views/events/utils/getParams'; |
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.
Should this function be moved to live with the other global selection functions?
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.
I'll see if I can move getParams
to this file.
|
||
const getDateTimeString = ( | ||
maybe: string | string[] | undefined | null | ||
): string | undefined => { |
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 and a few of the other parsers in this file have similar function bodies with only the validator/parsing operation being different. Could we extract that duplication?
21bcc4b
to
8af00b1
Compare
64b3447
to
954c129
Compare
Some various fixes:
The table metadata could be
null
, so we mark it as potentiallynull
, and add guards as necessary.If either
start
/end
query string is provided, it may be missingend
/start
respectively, and easily break discover2; especially when URLs are meant to be shareable. This can occur in copy/paste mishaps. As a fallback, thestatsPeriod
will be coerced to a value of14d
.Parse and validate
statsPeriod
,start
, andend
query strings at the global selection header store.normalize datetime strings from
start
andend
to beYYYY-MM-DDTHH:mm:ss.SSS
Properly parse datetime strings in
EventView.fromLocation()
andEventView. fromSavedQuery()
TODO
getParams
to the global selection header folder.