This repository was archived by the owner on Sep 30, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(plg): add links to the user nav dropdown #63462
Open
taras-yemets
wants to merge
11
commits into
main
Choose a base branch
from
ty/add-cody-pro-links-to-user-nav
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
ebef38f
fix(plg): Cody Pro routes conditions & add links to the user nav drop…
taras-yemets 3424ebb
feat(plg): add Cody Pro routes to the user nav dropdown
taras-yemets 7ec61a0
Merge remote-tracking branch 'origin/main' into ty/add-cody-pro-links…
taras-yemets 889ecc9
add test
taras-yemets 75803fe
refactor test
taras-yemets 45bf8f5
bazel configure
taras-yemets 9c551f7
Merge branch 'main' into ty/add-cody-pro-links-to-user-nav
taras-yemets 19d5eeb
Merge remote-tracking branch 'origin/main' into ty/add-cody-pro-links…
taras-yemets f1ad572
remove unused props
taras-yemets d884491
Merge remote-tracking branch 'origin/main' into ty/add-cody-pro-links…
taras-yemets 9e63554
Merge remote-tracking branch 'origin/main' into ty/add-cody-pro-links…
taras-yemets File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
import { renderHook } from '@testing-library/react' | ||
import { vi, describe, afterEach, test, expect, beforeEach } from 'vitest' | ||
|
||
import { CodyProRoutes } from './codyProRoutes' | ||
import * as subscriptionQueries from './management/api/react-query/subscriptions' | ||
import type { SubscriptionSummary } from './management/api/teamSubscriptions' | ||
import { useCodyProNavLinks } from './useCodyProNavLinks' | ||
|
||
describe('useCodyProNavLinks', () => { | ||
taras-yemets marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const useSubscriptionSummaryMock = vi.spyOn(subscriptionQueries, 'useSubscriptionSummary') | ||
|
||
afterEach(() => { | ||
useSubscriptionSummaryMock.mockReset() | ||
}) | ||
|
||
const mockSubscriptionSummary = (summary?: SubscriptionSummary): void => { | ||
useSubscriptionSummaryMock.mockReturnValue({ data: summary } as ReturnType< | ||
typeof subscriptionQueries.useSubscriptionSummary | ||
>) | ||
} | ||
|
||
test('returns empty array if subscription summary is undefined', () => { | ||
mockSubscriptionSummary() | ||
const { result } = renderHook(() => useCodyProNavLinks()) | ||
expect(result.current).toHaveLength(0) | ||
}) | ||
|
||
test('returns empty array user is not admin', () => { | ||
const summary: SubscriptionSummary = { | ||
teamId: '018ff1b3-118c-7789-82e4-ab9106eed204', | ||
userRole: 'member', | ||
teamCurrentMembers: 2, | ||
teamMaxMembers: 6, | ||
subscriptionStatus: 'active', | ||
cancelAtPeriodEnd: false, | ||
} | ||
mockSubscriptionSummary(summary) | ||
const { result } = renderHook(() => useCodyProNavLinks()) | ||
expect(result.current).toHaveLength(0) | ||
}) | ||
|
||
describe('user is admin', () => { | ||
const summary: SubscriptionSummary = { | ||
teamId: '018ff1b3-118c-7789-82e4-ab9106eed204', | ||
userRole: 'admin', | ||
teamCurrentMembers: 2, | ||
teamMaxMembers: 6, | ||
subscriptionStatus: 'active', | ||
cancelAtPeriodEnd: false, | ||
} | ||
|
||
const setUseEmbeddedUI = (useEmbeddedUI: boolean) => { | ||
vi.stubGlobal('context', { | ||
frontendCodyProConfig: { | ||
stripePublishableKey: 'pk_test_123', | ||
sscBaseUrl: '', | ||
useEmbeddedUI, | ||
}, | ||
}) | ||
} | ||
|
||
beforeEach(() => { | ||
vi.stubGlobal('context', {}) | ||
mockSubscriptionSummary(summary) | ||
}) | ||
|
||
test.skip('returns links to subscription and team management pages if embedded UI is enabled', () => { | ||
setUseEmbeddedUI(true) | ||
const { result } = renderHook(() => useCodyProNavLinks()) | ||
expect(result.current).toHaveLength(2) | ||
expect(result.current[0].label).toBe('Manage subscription') | ||
expect(result.current[0].to).toBe(CodyProRoutes.SubscriptionManage) | ||
expect(result.current[1].label).toBe('Manage team') | ||
expect(result.current[1].to).toBe(CodyProRoutes.ManageTeam) | ||
}) | ||
|
||
test('returns link to subscription management page if embedded UI is disabled', () => { | ||
setUseEmbeddedUI(false) | ||
const { result } = renderHook(() => useCodyProNavLinks()) | ||
expect(result.current).toHaveLength(1) | ||
expect(result.current[0].label).toBe('Manage subscription') | ||
expect(result.current[0].to).toBe('https://accounts.sourcegraph.com/cody/subscription') | ||
}) | ||
}) | ||
}) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import { useMemo } from 'react' | ||
|
||
import { CodyProRoutes } from './codyProRoutes' | ||
import { useSubscriptionSummary } from './management/api/react-query/subscriptions' | ||
import { getManageSubscriptionPageURL, isEmbeddedCodyProUIEnabled } from './util' | ||
|
||
export const useCodyProNavLinks = (): { to: string; label: string }[] => { | ||
const { data } = useSubscriptionSummary() | ||
|
||
return useMemo(() => { | ||
if (!data || data.userRole !== 'admin') { | ||
return [] | ||
} | ||
|
||
const items = [{ to: getManageSubscriptionPageURL(), label: 'Manage subscription' }] | ||
|
||
if (isEmbeddedCodyProUIEnabled()) { | ||
items.push({ to: CodyProRoutes.ManageTeam, label: 'Manage team' }) | ||
} | ||
|
||
return items | ||
}, [data]) | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is just logging an error and returning
null
the right call here? Or should we have something likereturn <GenericErrorPage message="{...}"></GenericErrorPage>
?It just seems like this could lead to a situation where the user has a blank page or some awkward looking UI, which is a little worse than having a clear "error" state.
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.
@chrsmith, WDYT about throwing an error that is propagated to the closest
ErrorBoundary
and results in a page looking like this?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 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.
If it's fine, then we may want to handle the other failed API calls inside components in a similar way (search).
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.
cc: @rrhyne
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 think that error message is fine for now but it would be better to update the text to be more end user friendly by removing “your site admin” and linking “Sourcegraph support” to the proper channel on discord, or the best place to resolve the error.
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 SGTM. The "fall back to the default error handler" seems super reasonable. And I also like Rob's suggestions. (Which I'm guessing would just be within the
<GenericErrorHandlerDodad>
use slightly different wording if in dotcom mode.)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.
Replacing
logger.error
calls with error propagation to the nearest error boundary might not be sufficient.For instance, a 404 error for the subscription summary should redirect the user to the "/cody/manage" page and possibly provide UI feedback indicating they tried to access the page they are not allowed to because of their team status or role on the team.
Additionally, I think we need a consistent approach to handling API call failures across the entire PLG flow. To address this comprehensively, I’ve created a dedicated issue, and I suggest we handle it in a separate PR.
WDYT, @chrsmith?