From 2d5cc9b36a28c4978a4a20d24da71807125bf0f8 Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Fri, 30 May 2025 15:06:25 -0700 Subject: [PATCH] feat(issues): Only send has_seen when user is member fixes https://linear.app/getsentry/issue/RTC-930/fix-mark-as-seen-functionality-on-issue-platform The optimistic update is confusing when user seen is not actually stored. --- .../views/issueDetails/groupDetails.spec.tsx | 128 +++++++++--------- .../app/views/issueDetails/groupDetails.tsx | 13 +- 2 files changed, 67 insertions(+), 74 deletions(-) diff --git a/static/app/views/issueDetails/groupDetails.spec.tsx b/static/app/views/issueDetails/groupDetails.spec.tsx index fb771c09aaec0c..503e5e1afe7c69 100644 --- a/static/app/views/issueDetails/groupDetails.spec.tsx +++ b/static/app/views/issueDetails/groupDetails.spec.tsx @@ -18,17 +18,12 @@ import OrganizationStore from 'sentry/stores/organizationStore'; import PageFiltersStore from 'sentry/stores/pageFiltersStore'; import ProjectsStore from 'sentry/stores/projectsStore'; import {IssueCategory} from 'sentry/types/group'; -import {useNavigate} from 'sentry/utils/useNavigate'; import GroupDetails from 'sentry/views/issueDetails/groupDetails'; import {useHasStreamlinedUI} from 'sentry/views/issueDetails/utils'; const SAMPLE_EVENT_ALERT_TEXT = 'You are viewing a sample error. Configure Sentry to start viewing real errors.'; -jest.mock('sentry/utils/useNavigate', () => ({ - useNavigate: jest.fn(), -})); - jest.mock('sentry/views/issueDetails/utils', () => ({ ...jest.requireActual('sentry/views/issueDetails/utils'), useHasStreamlinedUI: jest.fn(), @@ -39,31 +34,16 @@ describe('groupDetails', () => { const group = GroupFixture({issueCategory: IssueCategory.ERROR}); const event = EventFixture(); const project = ProjectFixture({teams: [TeamFixture()]}); + let hasSeenMock!: jest.Mock; - const routes = [ - {path: '/', childRoutes: []}, - {childRoutes: []}, - { - path: '/organizations/:orgId/issues/:groupId/', - childRoutes: [], - }, - {}, - ]; - - const initRouter = { + const initialRouterConfig = { location: { pathname: `/organizations/org-slug/issues/${group.id}/`, - query: {project: group.project.id}, - }, - params: { - groupId: group.id, }, - routes, + route: '/organizations/:orgId/issues/:groupId/', }; - const defaultInit = initializeOrg<{groupId: string}>({ - router: initRouter, - }); + const defaultInit = initializeOrg<{groupId: string}>({}); const recommendedUser = UserFixture({ options: { @@ -88,17 +68,19 @@ describe('groupDetails', () => { return
Group Details Mock
; } - const createWrapper = (init = defaultInit) => { + const createWrapper = ( + routerConfig = initialRouterConfig, + organization = defaultInit.organization + ) => { // Add project id to the url to skip over the _allp redirect window.location.search = qs.stringify({project: group.project.id}); return render( - + , { - organization: init.organization, - router: init.router, - deprecatedRouterMocks: true, + organization, + initialRouterConfig: routerConfig, } ); }; @@ -109,7 +91,6 @@ describe('groupDetails', () => { MockApiClient.clearMockResponses(); OrganizationStore.onUpdate(defaultInit.organization); act(() => ProjectsStore.loadInitialData(defaultInit.projects)); - jest.mocked(useNavigate).mockReturnValue(mockNavigate); MockApiClient.addMockResponse({ url: `/assistant/`, @@ -126,7 +107,7 @@ describe('groupDetails', () => { ...event, }, }); - MockApiClient.addMockResponse({ + hasSeenMock = MockApiClient.addMockResponse({ url: `/projects/org-slug/${project.slug}/issues/`, method: 'PUT', body: { @@ -190,6 +171,7 @@ describe('groupDetails', () => { // Sample event alert should not show up expect(screen.queryByText(SAMPLE_EVENT_ALERT_TEXT)).not.toBeInTheDocument(); + expect(hasSeenMock).toHaveBeenCalled(); }); it('renders error when issue is not found', async function () { @@ -237,16 +219,14 @@ describe('groupDetails', () => { body: group, }); - const init = initializeOrg({ - router: { - ...initRouter, - location: LocationFixture({ - ...initRouter.location, - query: {environment: 'staging', project: group.project.id}, - }), - }, - }); - createWrapper(init); + const routerConfig = { + ...initialRouterConfig, + location: LocationFixture({ + ...initialRouterConfig.location, + query: {environment: 'staging', project: group.project.id}, + }), + }; + createWrapper(routerConfig); await waitFor(() => expect(mock).toHaveBeenCalledWith( @@ -272,10 +252,7 @@ describe('groupDetails', () => { substatus: 'ongoing', }, }); - createWrapper({ - ...defaultInit, - organization: {...defaultInit.organization}, - }); + createWrapper(); expect(await screen.findByText('Ongoing')).toBeInTheDocument(); }); @@ -336,30 +313,24 @@ describe('groupDetails', () => { }, }); - createWrapper({ - ...defaultInit, - router: { - ...defaultInit.router, - location: LocationFixture({ - query: { - query: 'foo:bar', - statsPeriod: '14d', - }, - }), - }, - }); + const routerConfig = { + ...initialRouterConfig, + location: LocationFixture({ + ...initialRouterConfig.location, + query: {query: 'foo:bar', statsPeriod: '14d'}, + }), + }; + const {router} = createWrapper(routerConfig); await waitFor(() => expect(recommendedWithSearchMock).toHaveBeenCalledTimes(1)); await waitFor(() => - expect(mockNavigate).toHaveBeenCalledWith( - expect.objectContaining( - // query and period should be removed - {query: {}} - ), - { - replace: true, - } + expect(router.location).toEqual( + expect.objectContaining({ + pathname: routerConfig.location.pathname, + // Query has been removed + query: {}, + }) ) ); }); @@ -460,4 +431,31 @@ describe('groupDetails', () => { await waitFor(() => expect(recommendedMock).toHaveBeenCalledTimes(1)); }); + + it('does not send hasSeen request when user is not a project member', async function () { + const nonMemberProject = ProjectFixture({ + teams: [TeamFixture()], + isMember: false, + }); + + // Mock the group to belong to the non-member project + MockApiClient.addMockResponse({ + url: `/organizations/${defaultInit.organization.slug}/issues/${group.id}/`, + body: { + ...group, + project: nonMemberProject, + hasSeen: false, + }, + }); + + act(() => ProjectsStore.loadInitialData([nonMemberProject])); + + createWrapper(); + + // Wait for the component to render + expect(await screen.findByText(group.shortId)).toBeInTheDocument(); + + // Verify that the hasSeen request was NOT made + expect(hasSeenMock).not.toHaveBeenCalled(); + }); }); diff --git a/static/app/views/issueDetails/groupDetails.tsx b/static/app/views/issueDetails/groupDetails.tsx index a8dbde358cf036..0e4f32d1d979f4 100644 --- a/static/app/views/issueDetails/groupDetails.tsx +++ b/static/app/views/issueDetails/groupDetails.tsx @@ -235,6 +235,7 @@ function useFetchGroupDetails(): FetchGroupDetailsState { const navigate = useNavigate(); const defaultIssueEvent = useDefaultIssueEvent(); const hasStreamlinedUI = useHasStreamlinedUI(); + const {projects} = useProjects(); const [allProjectChanged, setAllProjectChanged] = useState(false); @@ -399,17 +400,11 @@ function useFetchGroupDetails(): FetchGroupDetailsState { return; } - if (!group.hasSeen) { + const project = projects.find(p => p.id === group?.project.id); + if (!group.hasSeen && project?.isMember) { markEventSeen(api, organization.slug, matchingProjectSlug, params.groupId); } - }, [ - api, - group?.hasSeen, - group?.project?.id, - group?.project?.slug, - organization.slug, - params.groupId, - ]); + }, [api, group?.hasSeen, group?.project, organization.slug, params.groupId, projects]); useEffect(() => { const locationQuery = qs.parse(window.location.search) || {};