Skip to content

feat: (Codecov) add updateRepository function to update local storage and url query params for repo selection #89818

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions static/app/actionCreators/pageFilters.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ describe('PageFilters ActionCreators', function () {
beforeEach(function () {
jest.spyOn(PageFiltersStore, 'updateProjects');
jest.spyOn(PageFiltersStore, 'onInitializeUrlState').mockImplementation();
jest
.spyOn(PageFiltersStore, 'onInitializeUrlStateWithCodecovData')
.mockImplementation();
jest.clearAllMocks();
});

Expand Down Expand Up @@ -275,6 +278,7 @@ describe('PageFilters ActionCreators', function () {
organization,
queryParams: {
project: '1',
repository: 'repo-from-query',
},
memberProjects: projects,
nonMemberProjects: [],
Expand All @@ -296,11 +300,19 @@ describe('PageFilters ActionCreators', function () {
new Set(),
true
);
expect(PageFiltersStore.onInitializeUrlStateWithCodecovData).toHaveBeenCalledWith(
{
repository: 'repo-from-query',
},
new Set(),
true
);
expect(router.replace).toHaveBeenCalledWith(
expect.objectContaining({
query: {
environment: [],
project: ['1'],
repository: 'repo-from-query',
},
})
);
Expand Down Expand Up @@ -373,6 +385,7 @@ describe('PageFilters ActionCreators', function () {
end: null,
period: '14d',
utc: null,
repository: null,
},
pinnedFilters: new Set(),
});
Expand Down Expand Up @@ -405,6 +418,7 @@ describe('PageFilters ActionCreators', function () {
end: null,
period: '7d',
utc: null,
repository: null,
},
pinnedFilters: new Set(['environments', 'datetime', 'projects']),
});
Expand Down Expand Up @@ -696,6 +710,7 @@ describe('PageFilters ActionCreators', function () {
end: null,
period: '14d',
utc: null,
repository: null,
},
pinnedFilters: new Set(['projects', 'environments', 'datetime']),
});
Expand Down
73 changes: 63 additions & 10 deletions static/app/actionCreators/pageFilters.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ import {
setPageFiltersStorage,
} from 'sentry/components/organizations/pageFilters/persistence';
import type {PageFiltersStringified} from 'sentry/components/organizations/pageFilters/types';
import {getDefaultSelection} from 'sentry/components/organizations/pageFilters/utils';
import {
getCodecovDefaultSelection,
getDefaultSelection,
} from 'sentry/components/organizations/pageFilters/utils';
import {parseStatsPeriod} from 'sentry/components/timeRangeSelector/utils';
import {
ALL_ACCESS_PROJECTS,
Expand All @@ -23,7 +26,12 @@ import {
} from 'sentry/constants/pageFilters';
import OrganizationStore from 'sentry/stores/organizationStore';
import PageFiltersStore from 'sentry/stores/pageFiltersStore';
import type {DateString, PageFilters, PinnedPageFilter} from 'sentry/types/core';
import type {
CodecovPageFilters,
DateString,
PageFilters,
PinnedPageFilter,
} from 'sentry/types/core';
import type {InjectedRouter} from 'sentry/types/legacyReactRouter';
import type {Organization} from 'sentry/types/organization';
import type {Environment, MinimalProject, Project} from 'sentry/types/project';
Expand Down Expand Up @@ -57,11 +65,16 @@ type Options = {
storageNamespace?: string;
};

/**
* Union type between Sentry and Codecov Page filters
*/
type PageFiltersUpdate = SentryPageFilterUpdate & CodecovPageFiltersUpdate;

/**
* This is the 'update' object used for updating the page filters. The types
* here are a bit wider to allow for easy updates.
*/
type PageFiltersUpdate = {
type SentryPageFilterUpdate = {
end?: DateString;
environment?: string[] | null;
period?: string | null;
Expand All @@ -70,6 +83,10 @@ type PageFiltersUpdate = {
utc?: boolean | null;
};

type CodecovPageFiltersUpdate = {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a distinction of Codecov vs Sentry? If we are merging Codecov to Sentry, then this should not exist, and it should just be Sentry going forward.

I'm not a fan of this approach, as I'm fairly sure it will linger around until someone ultimately needs to address it (at which point, we'll have a lot of the code relying on this and it'll be a massive PITA to update)

repository?: string | null;
};

/**
* Represents the input for updating the date time of page filters
*/
Expand Down Expand Up @@ -198,6 +215,8 @@ export function initializeUrlState({
// Use period from default if we don't have a period set
pageFilters.datetime.period ??= defaultDatetime.period;

const codecovPageFilters: CodecovPageFilters = getCodecovDefaultSelection();

// Do not set a period if we have absolute start and end
if (pageFilters.datetime.start && pageFilters.datetime.end) {
pageFilters.datetime.period = null;
Expand All @@ -206,6 +225,8 @@ export function initializeUrlState({
const hasDatetimeInUrl = Object.keys(pick(queryParams, DATE_TIME_KEYS)).length > 0;
const hasProjectOrEnvironmentInUrl =
Object.keys(pick(queryParams, [URL_PARAM.PROJECT, URL_PARAM.ENVIRONMENT])).length > 0;
const hasRepositoryInUrl =
Object.keys(pick(queryParams, URL_PARAM.REPOSITORY)).length > 0;

// We should only check and update the desync state if the site has just been loaded
// (not counting route changes). To check this, we can use the `isReady` state: if it's
Expand Down Expand Up @@ -254,6 +275,12 @@ export function initializeUrlState({
}
}

// We want to update the pageFilter's object with a repository if it is in the URL
// or in local storage, in that order.
if (hasRepositoryInUrl) {
codecovPageFilters.repository = parsed.repository ?? null;
}

const storedPageFilters = skipLoadLastUsed
? null
: getPageFilterStorage(orgSlug, storageNamespace);
Expand Down Expand Up @@ -293,6 +320,10 @@ export function initializeUrlState({
pageFilters.datetime = getDatetimeFromState(storedState);
shouldUsePinnedDatetime = true;
}

if (!hasRepositoryInUrl && pinnedFilters.has('repository')) {
codecovPageFilters.repository = storedState.repository ?? null;
}
}

const {projects, environments: environment, datetime} = pageFilters;
Expand Down Expand Up @@ -358,12 +389,16 @@ export function initializeUrlState({
}
}
}

const pinnedFilters = organization.features.includes('new-page-filter')
? new Set<PinnedPageFilter>(['projects', 'environments', 'datetime'])
? new Set<PinnedPageFilter>(['projects', 'environments', 'datetime', 'repository'])
: (storedPageFilters?.pinnedFilters ?? new Set());

PageFiltersStore.onInitializeUrlState(pageFilters, pinnedFilters, shouldPersist);
PageFiltersStore.onInitializeUrlStateWithCodecovData(
codecovPageFilters,
pinnedFilters,
shouldPersist
);
if (shouldUpdateLocalStorage) {
setPageFiltersStorage(organization.slug, new Set(['projects', 'environments']));
}
Expand Down Expand Up @@ -392,10 +427,14 @@ export function initializeUrlState({
};

if (!skipInitializeUrlParams) {
updateParams({project, environment, ...newDatetime}, router, {
replace: true,
keepCursor: true,
});
updateParams(
{project, environment, ...newDatetime, repository: codecovPageFilters.repository},
router,
{
replace: true,
keepCursor: true,
}
);
}
}

Expand Down Expand Up @@ -469,6 +508,19 @@ export function updateDateTime(
persistPageFilters('datetime', options);
}

/**
* Updates store and global repository selection URL param if `router` is supplied
*
* @param repository Object with repository key
* @param router Router object
* @param options Options object
*/
export function updateRepository(repository: string, router?: Router, options?: Options) {
PageFiltersStore.updateRepository(repository);
updateParams({repository}, router, options);
persistPageFilters('repository', options);
}

/**
* Pins a particular filter so that it is read out of local storage
*/
Expand Down Expand Up @@ -669,7 +721,7 @@ function getNewQueryParams(
const extraParams = omit(cleanCurrentQuery, omittedParameters);

// Override parameters
const {project, environment, start, end, utc} = {
const {project, environment, start, end, utc, repository} = {
...currentQueryState,
...obj,
};
Expand All @@ -684,6 +736,7 @@ function getNewQueryParams(
end: statsPeriod ? null : end instanceof Date ? getUtcDateString(end) : end,
utc: utc ? 'true' : null,
statsPeriod,
repository,
...extraParams,
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ describe('DatePicker', function () {
environments: [],
projects: [],
},
codecovSelection: {
repository: null,
},
});
});

Expand Down Expand Up @@ -106,6 +109,9 @@ describe('DatePicker', function () {
environments: [],
projects: [],
},
codecovSelection: {
repository: null,
},
});
});

Expand Down
6 changes: 6 additions & 0 deletions static/app/components/organizations/datePageFilter.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ describe('DatePageFilter', function () {
environments: [],
projects: [],
},
codecovSelection: {
repository: null,
},
});
});

Expand Down Expand Up @@ -111,6 +114,9 @@ describe('DatePageFilter', function () {
environments: [],
projects: [],
},
codecovSelection: {
repository: null,
},
});

// Absolute option is marked as selected
Expand Down
43 changes: 36 additions & 7 deletions static/app/components/organizations/pageFilters/container.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,9 @@ describe('PageFiltersContainer', function () {
environments: ['prod'],
projects: [],
},
codecovSelection: {
repository: null,
},
})
);

Expand Down Expand Up @@ -173,6 +176,9 @@ describe('PageFiltersContainer', function () {
environments: [],
projects: [],
},
codecovSelection: {
repository: null,
},
})
);
});
Expand Down Expand Up @@ -203,6 +209,9 @@ describe('PageFiltersContainer', function () {
environments: [],
projects: [],
},
codecovSelection: {
repository: null,
},
})
);
});
Expand Down Expand Up @@ -244,6 +253,9 @@ describe('PageFiltersContainer', function () {
environments: [],
projects: [],
},
codecovSelection: {
repository: null,
},
});
});

Expand Down Expand Up @@ -287,11 +299,13 @@ describe('PageFiltersContainer', function () {
});

it('does not load from local storage when there are URL params', function () {
jest
.spyOn(localStorage, 'getItem')
.mockImplementation(() =>
JSON.stringify({projects: [3], environments: ['staging']})
);
jest.spyOn(localStorage, 'getItem').mockImplementation(() =>
JSON.stringify({
projects: [3],
environments: ['staging'],
repository: 'repo-from-store',
})
);

const initializationObj = initializeOrg({
organization: {
Expand All @@ -301,7 +315,10 @@ describe('PageFiltersContainer', function () {
// we need this to be set to make sure org in context is same as
// current org in URL
params: {orgId: 'org-slug'},
location: {pathname: '/test', query: {project: ['1', '2']}},
location: {
pathname: '/test',
query: {project: ['1', '2'], repository: 'repo-from-url'},
},
},
});

Expand All @@ -312,6 +329,7 @@ describe('PageFiltersContainer', function () {
);

expect(PageFiltersStore.getState().selection.projects).toEqual([1, 2]);
expect(PageFiltersStore.getState().codecovSelection.repository).toBe('repo-from-url');

// Since these are coming from URL, there should be no changes and
// router does not need to be called
Expand All @@ -327,7 +345,10 @@ describe('PageFiltersContainer', function () {
// we need this to be set to make sure org in context is same as
// current org in URL
params: {orgId: 'org-slug'},
location: {pathname: '/test', query: {project: ['1', '2']}},
location: {
pathname: '/test',
query: {project: ['1', '2'], repository: 'repo-from-store'},
},
},
});

Expand All @@ -338,6 +359,9 @@ describe('PageFiltersContainer', function () {
);

expect(PageFiltersStore.getState().selection.projects).toEqual([1, 2]);
expect(PageFiltersStore.getState().codecovSelection.repository).toBe(
'repo-from-store'
);

// Since these are coming from URL, there should be no changes and
// router does not need to be called
Expand Down Expand Up @@ -699,6 +723,11 @@ describe('PageFiltersContainer', function () {
projects: [],
})
);
await waitFor(() =>
expect(PageFiltersStore.getState().codecovSelection).toEqual({
repository: null,
})
);

expect(router.push).not.toHaveBeenCalled();
});
Expand Down
Loading
Loading