fix(plg): add links to the user nav dropdown #63462
Conversation
…down (#63378) Closes https://github.com/sourcegraph/sourcegraph/issues/63264 1. Adds Cody Pro section to the user nav dropdown on dotcom: - team admins have links to "cody/subscription/manage" and "cody/team/manage" pages - team members have only link only to the "cody/team/manage" page 2. Makes subscription management page only available for team admins. If team members try to navigate this page by changing the URL, they will be redirected to the "/cody/manage" page. - Makes "Manage subscription" link on the "/cody/manage" available only for the team admins (previously - any Pro user) 3. Lifts `QueryClientProvider` (wrapper around react-query `ClientProvider` higher in the component tree so that Cody Pro API query and mutations can be used not only on the Cody Pro routes. [Design](https://www.figma.com/design/FMSdn1oKccJRHQPgf7053o/Cody-PLG-GA?node-id=5351-19126&t=LfNQR7vUCJhlNvyT-4) | Role | Screenshot | |--|--| |Admin|<img width="1605" alt="Screenshot 2024-06-20 at 12 57 13" src="https://github.com/sourcegraph/sourcegraph/assets/25318659/e7b995fd-4322-4f7f-86bb-c7027cacd644"><img width="1605" alt="Screenshot 2024-06-20 at 13 10 28" src="https://github.com/sourcegraph/sourcegraph/assets/25318659/84be111c-76e2-4b14-9b73-ebdd5f8cbb9a">| |Member|<img width="1605" alt="Screenshot 2024-06-20 at 12 56 50" src="https://github.com/sourcegraph/sourcegraph/assets/25318659/aefd8b3b-29fe-4957-87ee-b493eb489922"><img width="1605" alt="Screenshot 2024-06-20 at 13 09 51" src="https://github.com/sourcegraph/sourcegraph/assets/25318659/8209e7a0-4deb-4681-9f6b-3cfde8110842">| <!-- 💡 To write a useful PR description, make sure that your description covers: - WHAT this PR is changing: - How was it PREVIOUSLY. - How it will be from NOW on. - WHY this PR is needed. - CONTEXT, i.e. to which initiative, project or RFC it belongs. The structure of the description doesn't matter as much as covering these points, so use your best judgement based on your context. Learn how to write good pull request description: https://www.notion.so/sourcegraph/Write-a-good-pull-request-description-610a7fd3e613496eb76f450db5a49b6e?pvs=4 --> ## Test plan - Start Sourcegraph instance in dotcom mode - sign in as a Cody Pro team admin - ensure that "Manage subscription" and "Manage team" links are present in the user nav dropdown - ensure "/cody/manage" page has "Manage subscription" link - sign in as a Cody Pro team member (not admin) - ensure "Manage team" link is present in the user nav dropdown - navigate to "/cody/subscription/manage/ page by changing the URL - ensure you've been redirected to the "/cody/manage" page - ensure "/cody/manage" page doesn't have "Manage subscription" link - start Sourcegraph in the enterprise mode - ensure the mentioned links are not rendered <!-- All pull requests REQUIRE a test plan: https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles --> ## Changelog <!-- 1. Ensure your pull request title is formatted as: $type($domain): $what 2. Add bullet list items for each additional detail you want to cover (see example below) 3. You can edit this after the pull request was merged, as long as release shipping it hasn't been promoted to the public. 4. For more information, please see this how-to https://www.notion.so/sourcegraph/Writing-a-changelog-entry-dd997f411d524caabf0d8d38a24a878c? Audience: TS/CSE > Customers > Teammates (in that order). Cheat sheet: $type = chore|fix|feat $domain: source|search|ci|release|plg|cody|local|... --> <!-- Example: Title: fix(search): parse quotes with the appropriate context Changelog section: ## Changelog - When a quote is used with regexp pattern type, then ... - Refactored underlying code. -->
taras-yemets
changed the title
client/web/src/cody/management/api/react-query/CodyProApiProvider.tsx
Outdated
Show resolved
Hide resolved
| @@ -55,18 +56,32 @@ const AuthenticatedCodySubscriptionManagePage: React.FC<Props> = ({ telemetryRec | |||
| return null | |||
| } | |||
|
|
|||
| if (subscriptionSummaryQuery.isError) { | |||
| logger.error('Failed to fetch Cody subscription summary', subscriptionSummaryQuery.error) | |||
There was a problem hiding this comment.
Is just logging an error and returning null the right call here? Or should we have something like return <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.
@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.
| logger.error('Failed to fetch Cody subscription summary', subscriptionSummaryQuery.error) | |
| throw new Error('Failed to fetch Cody subscription summary.') |
There was a problem hiding this comment.
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.
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.
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.
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?
taras-yemets
force-pushed
the
ty/add-cody-pro-links-to-user-nav
branch
from
2211865 to
f1ad572
Compare
|
Cody dashboard link has been added in https://github.com/sourcegraph/sourcegraph/pull/63514. @rrhyne, do you think we need to revisit the Cody PRO section in the user nav drodown inroduced in the current PR (e.g., have a dedicated Cody section)? |
Sorry, I thought I had replied to this elsewhere but did not. Yeah, I do. This is an odd location for the dash and we should pull it all together into a single section so users get used to seeing it in one place throughout the life cycle of Free -> Pro -> Teams. |
Closes https://github.com/sourcegraph/sourcegraph/issues/63264
Re-implements reverted https://github.com/sourcegraph/sourcegraph/pull/63378
dotcom.codyProConfig.useEmebeddedUIsite config param is set totrue, team admins will have links to "cody/subscription/manage" and "cody/team/manage" pagesdotcom.codyProConfig.useEmebeddedUIsite config param is not set or set tofalse, team admins will have only link only to the manage subscription page on accounts.sourcegraph.com (https://accounts.sourcegraph.com/cody/subscription).QueryClientProvider(wrapper around react-queryClientProviderhigher in the component tree so that Cody Pro API query and mutations can be used not only on the Cody Pro routes.Design
useEmbeddedUITest plan
Test plan
dotcom.codyProConfig.useEmbeddedUIsite config param, log in with users with different team roles (member/admin) and check results with the table above.