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.

Sign up for GitHub

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

Jump to bottom

ref(pageFilters): Constrict types on updateParams better #31073

Merged
merged 1 commit into from
Jan 14, 2022
Merged

ref(pageFilters): Constrict types on updateParams better #31073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
127 changes: 71 additions & 56 deletions static/app/actionCreators/pageFilters.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {InjectedRouter} from 'react-router';
import * as Sentry from '@sentry/react';
import {Location} from 'history';
import isInteger from 'lodash/isInteger';
import omit from 'lodash/omit';
import pick from 'lodash/pick';
Expand All @@ -15,6 +16,7 @@ import {
import {DATE_TIME, URL_PARAM} from 'sentry/constants/pageFilters';
import PageFiltersStore from 'sentry/stores/pageFiltersStore';
import {
DateString,
Environment,
MinimalProject,
Organization,
Expand Down Expand Up @@ -52,9 +54,9 @@ type Options = {
/**
* Represents the datetime portion of page filters staate
*/
type DateTimeObject = {
start?: Date | string | null;
end?: Date | string | null;
type DateTimeUpdate = {
start?: DateString;
end?: DateString;
statsPeriod?: string | null;
utc?: string | boolean | null;
/**
Expand All @@ -64,17 +66,25 @@ type DateTimeObject = {
};

/**
* Cast project ids to strings, as everything is assumed to be a string in URL params
*
* We also handle internal types so Dates and booleans can show up in the start/end/utc
* keys. Long term it would be good to narrow down these types.
* Object used to update the page filter parameters
*/
type UrlParams = {
type UpdateParams = {
project?: ProjectId[] | null;
environment?: EnvironmentId[] | null;
} & DateTimeObject & {
[others: string]: any;
};
} & DateTimeUpdate;

/**
* Output object used for updating query parameters
*/
type PageFilterQuery = {
project?: string[] | null;
environment?: string[] | null;
start?: string | null;
end?: string | null;
statsPeriod?: string | null;
utc?: string | null;
[extra: string]: Location['query'][string];
};

/**
* This can be null which will not perform any router side effects, and instead updates store.
Expand All @@ -94,7 +104,7 @@ function getProjectIdFromProject(project: MinimalProject) {

type InitializeUrlStateParams = {
organization: Organization;
queryParams: InjectedRouter['location']['query'];
queryParams: Location['query'];
router: InjectedRouter;
memberProjects: Project[];
shouldForceProject?: boolean;
Expand Down Expand Up @@ -257,7 +267,7 @@ function isProjectsValid(projects: ProjectId[]) {
* @param {String[]} [options.resetParams] List of parameters to remove when changing URL params
*/
export function updateDateTime(
datetime: DateTimeObject,
datetime: DateTimeUpdate,
router?: Router,
options?: Options
) {
Expand Down Expand Up @@ -291,7 +301,7 @@ export function updateEnvironments(
* @param [router] React router object
* @param [options] Options object
*/
export function updateParams(obj: UrlParams, router?: Router, options?: Options) {
export function updateParams(obj: UpdateParams, router?: Router, options?: Options) {
// Allow another component to handle routing
if (!router) {
return;
Expand All @@ -317,58 +327,63 @@ export function updateParams(obj: UrlParams, router?: Router, options?: Options)
}

/**
* Creates a new query parameter object given new params and old params
* Preserves the old query params, except for `cursor` (can be overriden with keepCursor option)
* Merges an UpdateParams object into a Location['query'] object. Results in a
* PageFilterQuery
*
* Preserves the old query params, except for `cursor` (can be overriden with
* keepCursor option)
*
* @param obj New query params
* @param oldQueryParams Old query params
* @param currentQuery The current query parameters
* @param [options] Options object
*/
function getNewQueryParams(
obj: UrlParams,
oldQueryParams: UrlParams,
{resetParams, keepCursor}: Options = {}
obj: UpdateParams,
currentQuery: Location['query'],
options: Options = {}
) {
const {cursor, statsPeriod, ...oldQuery} = oldQueryParams;
const oldQueryWithoutResetParams = !!resetParams?.length
? omit(oldQuery, resetParams)
: oldQuery;

const newQuery = getParams({
...oldQueryWithoutResetParams,
// Some views update using `period`, and some `statsPeriod`, we should make this uniform
period: !obj.start && !obj.end ? obj.period || statsPeriod : null,
...obj,
const {resetParams, keepCursor} = options;

const cleanCurrentQuery = !!resetParams?.length
? omit(currentQuery, resetParams)
: currentQuery;

// Normalize existing query parameters
const currentQueryState = getStateFromQuery(cleanCurrentQuery, {
allowEmptyPeriod: true,
});

if (newQuery.start) {
newQuery.start = getUtcDateString(newQuery.start);
}
// Extract non page filter parameters.
const cursorParam = !keepCursor ? 'cursor' : null;
const omittedParameters = [...Object.values(URL_PARAM), cursorParam].filter(defined);

if (newQuery.end) {
newQuery.end = getUtcDateString(newQuery.end);
}
const extraParams = omit(cleanCurrentQuery, omittedParameters);

if (keepCursor) {
newQuery.cursor = cursor;
}
// Override parameters
const {project, environment, start, end, utc} = {
...currentQueryState,
...obj,
};

return newQuery;
}
// Only set a stats period if we don't have an absolute date
//
// `period` is deprecated as a url parameter, though some pages may still
// include it (need to actually validate this?), normalize period and stats
// period together
const statsPeriod =
!start && !end ? obj.statsPeriod || obj.period || currentQueryState.period : null;

const newQuery: PageFilterQuery = {
project: project?.map(String),
environment,
start: statsPeriod ? null : start instanceof Date ? getUtcDateString(start) : start,
end: statsPeriod ? null : end instanceof Date ? getUtcDateString(end) : end,
Copy link
Member

Choose a reason for hiding this comment

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

@evanpurkhiser We should probably check if the dates here are valid, the instanceof new Date("invalid date") will still pass here otherwise. UX wise, I'm not sure if it's best to discard invalid dates or throw an error. I'm in favour of throwing so that in case this happens we have visibility and can fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We probably should check somewhere else if the dates are valid or not.

This code is a bit of a rats nest, before it wasn't even handling string-ification of date objects afaict.

For now I am OK with leaving this as it is, since it's at least a big improvement over how it worked before 😬

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense - I'm lacking context on some of these components and how they all work together in the greater deal of things, so I may say something dumb every now and then 😅 Def a nice improvement from before though 🙏🏼

utc: utc ? 'true' : null,
statsPeriod,
...extraParams,
};

const paramEntries = Object.entries(newQuery).filter(([_, value]) => defined(value));

function getParams(params: UrlParams): UrlParams {
const {start, end, period, statsPeriod, ...otherParams} = params;

// `statsPeriod` takes precedence for now
const coercedPeriod = statsPeriod || period;

// Filter null values
return Object.fromEntries(
Object.entries({
statsPeriod: coercedPeriod,
start: coercedPeriod ? null : start,
end: coercedPeriod ? null : end,
...otherParams,
}).filter(([, value]) => defined(value))
);
return Object.fromEntries(paramEntries) as PageFilterQuery;
}
18 changes: 5 additions & 13 deletions static/app/components/organizations/pageFilters/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import pickBy from 'lodash/pickBy';

import {DATE_TIME_KEYS, URL_PARAM} from 'sentry/constants/pageFilters';
import OrganizationsStore from 'sentry/stores/organizationsStore';
import {Environment, PageFilters} from 'sentry/types';
import {PageFilters} from 'sentry/types';
import localStorage from 'sentry/utils/localStorage';

import {normalizeDateTimeParams} from './parse';
Expand Down Expand Up @@ -85,8 +85,8 @@ function makeLocalStorageKey(orgSlug: string) {
}

type UpdateData = {
project?: Array<string | number> | string | number | null;
environment?: Environment['id'][] | null;
project?: string[] | null;
environment?: string[] | null;
};

/**
Expand Down Expand Up @@ -116,17 +116,9 @@ export function setPageFiltersStorage(
return;
}

const {project, environment} = update;
const validatedProject = project
? (Array.isArray(project) ? project : [project])
.map(Number)
.filter(value => !isNaN(value))
: undefined;
const validatedEnvironment = environment;

const dataToSave = {
projects: validatedProject || current.projects,
environments: validatedEnvironment || current.environments,
projects: update.project || current.projects,
environments: update.environment || current.environments,
};

const localStorageKey = makeLocalStorageKey(org.slug);
Expand Down
12 changes: 6 additions & 6 deletions tests/js/spec/actionCreators/pageFilters.spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe('PageFilters ActionCreators', function () {
expect.objectContaining({
query: {
environment: [],
project: [1],
project: ['1'],
},
})
);
Expand Down Expand Up @@ -126,7 +126,7 @@ describe('PageFilters ActionCreators', function () {
expect.objectContaining({
query: {
cursor: undefined,
project: [1],
project: ['1'],
environment: [],
statsPeriod: '1h',
},
Expand All @@ -153,7 +153,7 @@ describe('PageFilters ActionCreators', function () {
expect.objectContaining({
query: {
cursor: undefined,
project: [1],
project: ['1'],
environment: [],
start: '2020-03-22T00:53:38',
end: '2020-04-21T00:53:38',
Expand Down Expand Up @@ -186,7 +186,7 @@ describe('PageFilters ActionCreators', function () {
expect.objectContaining({
query: {
environment: [],
project: [1],
project: ['1'],
},
})
);
Expand Down Expand Up @@ -336,7 +336,7 @@ describe('PageFilters ActionCreators', function () {

expect(router.push).toHaveBeenCalledWith({
pathname: '/test/',
query: {project: [1]},
query: {project: ['1']},
});
});
it('does not update history when queries are the same', function () {
Expand Down Expand Up @@ -368,7 +368,7 @@ describe('PageFilters ActionCreators', function () {

expect(router.replace).toHaveBeenCalledWith({
pathname: '/test/',
query: {project: [1]},
query: {project: ['1']},
});
});
it('does not update history when queries are the same', function () {
Expand Down
Loading