From 3452d949465d729f7ce27aaf928f898c350cb51f Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Wed, 28 Aug 2024 10:46:56 +0200 Subject: [PATCH 01/13] Add an alert CTA label This label allows to customize text on a button that contains the alert link. --- api/types/cluster_alert.go | 18 ++++++--- api/types/cluster_alert_test.go | 40 ++++++++++++++----- api/types/constants.go | 4 ++ lib/auth/auth.go | 8 ++-- .../teleport/aggregating/submitter.go | 2 + 5 files changed, 55 insertions(+), 17 deletions(-) diff --git a/api/types/cluster_alert.go b/api/types/cluster_alert.go index e98ee46af2386..3cd056e9732b3 100644 --- a/api/types/cluster_alert.go +++ b/api/types/cluster_alert.go @@ -32,6 +32,8 @@ var matchAlertLabelKey = regexp.MustCompile(`^[a-z0-9\.\-\/]+$`).MatchString // matchAlertLabelVal is a slightly more permissive matcher for label values. var matchAlertLabelVal = regexp.MustCompile(`^[a-z0-9\.\-_\/:|]+$`).MatchString +var matchAlertLabelCTAVal = regexp.MustCompile(`^[a-zA-Z0-9 ]+$`).MatchString + const validLinkDestination = "goteleport.com" type alertOptions struct { @@ -151,12 +153,9 @@ func (c *ClusterAlert) CheckAndSetDefaults() error { if !matchAlertLabelKey(key) { return trace.BadParameter("invalid alert label key: %q", key) } - // for links, we relax the conditions on label values - if key != AlertLink && !matchAlertLabelVal(val) { - return trace.BadParameter("invalid alert label value: %q", val) - } - if key == AlertLink { + switch key { + case AlertLink: u, err := url.Parse(val) if err != nil { return trace.BadParameter("invalid alert: label link %q is not a valid URL", val) @@ -164,6 +163,15 @@ func (c *ClusterAlert) CheckAndSetDefaults() error { if u.Hostname() != validLinkDestination { return trace.BadParameter("invalid alert: label link not allowed %q", val) } + case AlertLinkCTA: + if !matchAlertLabelCTAVal(val) { + return trace.BadParameter("invalid alert: label link CTA not allowed: %q", val) + } + default: + if !matchAlertLabelVal(val) { + // for links, we relax the conditions on label values + return trace.BadParameter("invalid alert label value: %q", val) + } } } return nil diff --git a/api/types/cluster_alert_test.go b/api/types/cluster_alert_test.go index cba58d3012391..5d145ab349ed1 100644 --- a/api/types/cluster_alert_test.go +++ b/api/types/cluster_alert_test.go @@ -84,32 +84,54 @@ func TestAlertSorting(t *testing.T) { } } -// TestCheckAndSetDefaults verifies that only valid URLs are set on the link label. +// TestCheckAndSetDefaults verifies that only valid URLs are set on the link +// label and that only valid CTA text can be used. func TestCheckAndSetDefaultsWithLink(t *testing.T) { tests := []struct { - link string - assert require.ErrorAssertionFunc + options []AlertOption + name string + cta string + assert require.ErrorAssertionFunc }{ { - link: "https://goteleport.com/docs", + name: "valid link", + options: []AlertOption{WithAlertLabel(AlertLink, "https://goteleport.com/docs")}, + assert: require.NoError, + }, + { + name: "valid link with CTA", + options: []AlertOption{ + WithAlertLabel(AlertLink, "https://goteleport.com/support"), + WithAlertLabel(AlertLinkCTA, "Contact Support"), + }, assert: require.NoError, }, { - link: "h{t}tps://goteleport.com/docs", - assert: require.Error, + name: "invalid link", + options: []AlertOption{WithAlertLabel(AlertLink, "h{t}tps://goteleport.com/docs")}, + assert: require.Error, }, { - link: "https://google.com", + name: "external link", + options: []AlertOption{WithAlertLabel(AlertLink, "https://google.com")}, + assert: require.Error, + }, + { + name: "valid link with invalid CTA", + options: []AlertOption{ + WithAlertLabel(AlertLink, "https://goteleport.com/support"), + WithAlertLabel(AlertLinkCTA, "Contact!Support"), + }, assert: require.Error, }, } for i, tt := range tests { - t.Run(tt.link, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { _, err := NewClusterAlert( fmt.Sprintf("name-%d", i), fmt.Sprintf("message-%d", i), - WithAlertLabel(AlertLink, tt.link), + tt.options..., ) tt.assert(t, err) }) diff --git a/api/types/constants.go b/api/types/constants.go index 4e3e6a44c7de8..296771d6b1b2b 100644 --- a/api/types/constants.go +++ b/api/types/constants.go @@ -950,6 +950,10 @@ const ( // AlertLink is an internal label that indicates that an alert is a link. AlertLink = TeleportInternalLabelPrefix + "link" + // AlertLinkCTA is a call-to-action text that will be rendered by Web UI on + // the CTA button accompanying the alert. + AlertLinkCTA = TeleportInternalLabelPrefix + "link-cta" + // AlertVerbPermit is an internal label that permits a user to view the alert if they // hold a specific resource permission verb (e.g. 'node:list'). Note that this label is // a coarser control than it might initially appear and has the potential for accidental diff --git a/lib/auth/auth.go b/lib/auth/auth.go index 87b2ab60167e0..2cae5a28f4e83 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -148,8 +148,9 @@ const ( OSSDesktopsAlertMessage = "Your cluster is beyond its allocation of 5 non-Active Directory Windows desktops. " + "Reach out for unlimited desktops with Teleport Enterprise." - OSSDesktopAlertLink = "https://goteleport.com/r/upgrade-community?utm_campaign=CTA_windows_local" - OSSDesktopsLimit = 5 + OSSDesktopsAlertLink = "https://goteleport.com/r/upgrade-community?utm_campaign=CTA_windows_local" + OSSDesktopsAlertLinkCTA = "Contact Sales" + OSSDesktopsLimit = 5 ) const ( @@ -5687,7 +5688,8 @@ func (a *Server) syncDesktopsLimitAlert(ctx context.Context) { types.WithAlertSeverity(types.AlertSeverity_MEDIUM), types.WithAlertLabel(types.AlertOnLogin, "yes"), types.WithAlertLabel(types.AlertPermitAll, "yes"), - types.WithAlertLabel(types.AlertLink, OSSDesktopAlertLink), + types.WithAlertLabel(types.AlertLink, OSSDesktopsAlertLink), + types.WithAlertLabel(types.AlertLinkCTA, OSSDesktopsAlertLinkCTA), types.WithAlertExpires(time.Now().Add(OSSDesktopsCheckPeriod))) if err != nil { log.Warnf("Can't create OSS non-AD desktops limit alert: %v", err) diff --git a/lib/usagereporter/teleport/aggregating/submitter.go b/lib/usagereporter/teleport/aggregating/submitter.go index 19a81a9a890b0..aa7cb68750a74 100644 --- a/lib/usagereporter/teleport/aggregating/submitter.go +++ b/lib/usagereporter/teleport/aggregating/submitter.go @@ -51,6 +51,7 @@ const ( alertGraceDuration = alertGraceHours * time.Hour alertName = "reporting-failed" alertLink = "https://goteleport.com/support/" + alertLinkCTA = "Contact Support" ) const ( @@ -204,6 +205,7 @@ func submitOnce(ctx context.Context, c SubmitterConfig) { types.WithAlertLabel(types.AlertOnLogin, "yes"), types.WithAlertLabel(types.AlertPermitAll, "yes"), types.WithAlertLabel(types.AlertLink, alertLink), + types.WithAlertLabel(types.AlertLinkCTA, alertLinkCTA), ) if err != nil { c.Log.WithError(err).Errorf("Failed to create cluster alert %v.", alertName) From e5fd1b693baa51399714afef6c0933fe51f57301 Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Wed, 28 Aug 2024 19:31:30 +0200 Subject: [PATCH 02/13] Comment --- api/types/cluster_alert.go | 1 + 1 file changed, 1 insertion(+) diff --git a/api/types/cluster_alert.go b/api/types/cluster_alert.go index 3cd056e9732b3..4f144a384c5fa 100644 --- a/api/types/cluster_alert.go +++ b/api/types/cluster_alert.go @@ -32,6 +32,7 @@ var matchAlertLabelKey = regexp.MustCompile(`^[a-z0-9\.\-\/]+$`).MatchString // matchAlertLabelVal is a slightly more permissive matcher for label values. var matchAlertLabelVal = regexp.MustCompile(`^[a-z0-9\.\-_\/:|]+$`).MatchString +// matchAlertLabelCTAVal only allows alphanumeric characters and spaces. var matchAlertLabelCTAVal = regexp.MustCompile(`^[a-zA-Z0-9 ]+$`).MatchString const validLinkDestination = "goteleport.com" From 35e86f902077e782842a6ab8939e6c1a6bfe17ec Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Wed, 28 Aug 2024 17:35:24 +0200 Subject: [PATCH 03/13] Use the new alert CTA label to display a link button The link button replaces current visual representation of a cluster alert, where the entire alert message is a link. --- web/packages/design/src/Alert/Alert.story.tsx | 27 +++----- web/packages/design/src/Alert/Alert.test.tsx | 23 +++++++ web/packages/design/src/Alert/Alert.tsx | 53 ++++++++++++--- web/packages/design/src/Alert/index.ts | 10 ++- web/packages/teleport/src/Main/Main.tsx | 7 +- .../BannerList/BannerList.story.tsx | 30 ++++++++- .../src/components/BannerList/BannerList.tsx | 2 + .../BannerList/StandardBanner.test.tsx | 62 ++++++++++++----- .../components/BannerList/StandardBanner.tsx | 67 +++++++++++-------- .../teleport/src/services/alerts/alerts.tsx | 1 + 10 files changed, 208 insertions(+), 74 deletions(-) diff --git a/web/packages/design/src/Alert/Alert.story.tsx b/web/packages/design/src/Alert/Alert.story.tsx index f02adc41b785c..3751836206ab9 100644 --- a/web/packages/design/src/Alert/Alert.story.tsx +++ b/web/packages/design/src/Alert/Alert.story.tsx @@ -20,7 +20,7 @@ import React from 'react'; import { Restore } from 'design/Icon'; -import { Box, Link } from '..'; +import { Box } from '..'; import { Alert, AlertProps, Banner } from './Alert'; @@ -102,23 +102,14 @@ export const Banners = () => ( > Greetings, professor Falken. - - Neutral link - - - Info link - - - Warning link - - - Danger link - - - Success link - - - Primary link + + Banner with a link button ); diff --git a/web/packages/design/src/Alert/Alert.test.tsx b/web/packages/design/src/Alert/Alert.test.tsx index 8a5802746b71d..2da03e67feb0e 100644 --- a/web/packages/design/src/Alert/Alert.test.tsx +++ b/web/packages/design/src/Alert/Alert.test.tsx @@ -109,6 +109,29 @@ describe('Banner', () => { expect(primaryCallback).toHaveBeenCalled(); }); + test('action buttons as links', async () => { + render( + + ); + + expect(screen.getByRole('link', { name: 'Primary Link' })).toHaveAttribute( + 'href', + 'https://goteleport.com/1' + ); + expect( + screen.getByRole('link', { name: 'Secondary Link' }) + ).toHaveAttribute('href', 'https://goteleport.com/2'); + }); + test('dismiss button', async () => { const user = userEvent.setup(); const onDismiss = jest.fn(); diff --git a/web/packages/design/src/Alert/Alert.tsx b/web/packages/design/src/Alert/Alert.tsx index 1714fdf364a73..5db4e46ff1813 100644 --- a/web/packages/design/src/Alert/Alert.tsx +++ b/web/packages/design/src/Alert/Alert.tsx @@ -24,7 +24,7 @@ import { space, SpaceProps, width, WidthProps } from 'design/system'; import { Theme } from 'design/theme/themes/types'; import * as Icon from 'design/Icon'; import Flex from 'design/Flex'; -import { Text, Button, Box, ButtonText, ButtonIcon } from 'design'; +import { Text, Button, Box, ButtonIcon } from 'design'; import { IconProps } from 'design/Icon/Icon'; import { ButtonFill, ButtonIntent } from 'design/Button'; @@ -124,10 +124,16 @@ interface Props { onDismiss?: () => void; } -/** Specifies parameters of an action button. */ -interface Action { +/** + * Specifies parameters of an action button. If no `href` is specified, the + * button is rendered as a regular button; otherwise, a link (with a button + * appearance) is used instead, and `href` is used as a link target. A link + * button can still have an `onClick` handler, too. + */ +export interface Action { content: React.ReactNode; - onClick: () => void; + href?: string; + onClick?: () => void; } export interface AlertProps @@ -345,14 +351,14 @@ const ActionButtons = ({ return ( {primaryAction && ( - + )} {secondaryAction && ( - - {secondaryAction.content} - + )} {dismissible && !dismissed && ( @@ -363,6 +369,33 @@ const ActionButtons = ({ ); }; +/** Renders either a regular or a link button, depending on the action. */ +const ActionButton = ({ + action: { href, content, onClick }, + fill, + intent, +}: { + action: Action; + fill: ButtonFill; + intent: ButtonIntent; +}) => + href ? ( + + ) : ( + + ); + export const Danger = (props: AlertProps) => ; export const Info = (props: AlertProps) => ; export const Warning = (props: AlertProps) => ( diff --git a/web/packages/design/src/Alert/index.ts b/web/packages/design/src/Alert/index.ts index 31a13e5b8513c..ab337bb92467b 100644 --- a/web/packages/design/src/Alert/index.ts +++ b/web/packages/design/src/Alert/index.ts @@ -16,4 +16,12 @@ * along with this program. If not, see . */ -export { Alert, Danger, Info, Warning, Success, Banner } from './Alert'; +export { + type Action, + Alert, + Danger, + Info, + Warning, + Success, + Banner, +} from './Alert'; diff --git a/web/packages/teleport/src/Main/Main.tsx b/web/packages/teleport/src/Main/Main.tsx index 7545080a0757f..f673004b491e6 100644 --- a/web/packages/teleport/src/Main/Main.tsx +++ b/web/packages/teleport/src/Main/Main.tsx @@ -44,7 +44,11 @@ import useTeleport from 'teleport/useTeleport'; import { TopBar } from 'teleport/TopBar'; import { BannerList } from 'teleport/components/BannerList'; import { storageService } from 'teleport/services/storageService'; -import { ClusterAlert, LINK_LABEL } from 'teleport/services/alerts/alerts'; +import { + ClusterAlert, + LINK_LABEL_CTA, + LINK_LABEL, +} from 'teleport/services/alerts/alerts'; import { useAlerts } from 'teleport/components/BannerList/useAlerts'; import { FeaturesContextProvider, useFeatures } from 'teleport/FeaturesContext'; import { @@ -185,6 +189,7 @@ export function Main(props: MainProps) { message: alert.spec.message, severity: mapSeverity(alert.spec.severity), link: alert.metadata.labels[LINK_LABEL], + linkCTA: alert.metadata.labels[LINK_LABEL_CTA], id: alert.metadata.name, })); diff --git a/web/packages/teleport/src/components/BannerList/BannerList.story.tsx b/web/packages/teleport/src/components/BannerList/BannerList.story.tsx index d2adc2309425d..e629a9659ea88 100644 --- a/web/packages/teleport/src/components/BannerList/BannerList.story.tsx +++ b/web/packages/teleport/src/components/BannerList/BannerList.story.tsx @@ -27,7 +27,35 @@ export default { export function List() { return ( ); } diff --git a/web/packages/teleport/src/components/BannerList/BannerList.tsx b/web/packages/teleport/src/components/BannerList/BannerList.tsx index 254065050f79f..f075b77676b13 100644 --- a/web/packages/teleport/src/components/BannerList/BannerList.tsx +++ b/web/packages/teleport/src/components/BannerList/BannerList.tsx @@ -62,6 +62,7 @@ export const BannerList = ({ severity={banner.severity} id={banner.id} link={banner.link} + linkCTA={banner.linkCTA} onDismiss={() => removeBanner(banner.id)} key={banner.id} /> @@ -84,5 +85,6 @@ export type BannerType = { severity: Severity; id: string; link?: string; + linkCTA?: string; hidden?: boolean; }; diff --git a/web/packages/teleport/src/components/BannerList/StandardBanner.test.tsx b/web/packages/teleport/src/components/BannerList/StandardBanner.test.tsx index 8409145b487e6..688789c2ef303 100644 --- a/web/packages/teleport/src/components/BannerList/StandardBanner.test.tsx +++ b/web/packages/teleport/src/components/BannerList/StandardBanner.test.tsx @@ -19,6 +19,8 @@ import React from 'react'; import { fireEvent, render, screen, theme } from 'design/utils/testing'; +import { userEventService } from 'teleport/services/userEvent'; + import { StandardBanner } from './StandardBanner'; describe('StandardBanner', () => { @@ -100,7 +102,7 @@ describe('StandardBanner', () => { }); describe('with link', () => { - it('renders valid URLs as links', () => { + it('renders valid URLs as link buttons', () => { const message = 'some-message-with-valid-URL'; render( { severity="info" id="some-id" link="https://goteleport.com/docs" + linkCTA="Open Docs" onDismiss={() => {}} /> ); expect(screen.getByText(message)).toBeInTheDocument(); - expect(screen.getByRole('link', { name: message })).toHaveAttribute( + expect(screen.getByRole('link', { name: 'Open Docs' })).toHaveAttribute( 'href', 'https://goteleport.com/docs' ); }); - it('renders invalid URLs as text', () => { - const message = 'some-message'; + it('renders valid URLs with default link CTA', () => { + const message = 'message-with-default-cta'; render( {}} /> ); expect(screen.getByText(message)).toBeInTheDocument(); - expect( - screen.queryByRole('link', { name: message }) - ).not.toBeInTheDocument(); + expect(screen.getByRole('link', { name: 'Learn More' })).toHaveAttribute( + 'href', + 'https://goteleport.com/docs' + ); }); - it('renders non-teleport URL as text', () => { - const message = 'message'; + it('captures click events', () => { + jest.spyOn(userEventService, 'captureUserEvent'); render( {}} /> ); - expect(screen.getByText(message)).toBeInTheDocument(); - expect( - screen.queryByRole('link', { name: message }) - ).not.toBeInTheDocument(); + fireEvent.click(screen.getByRole('link', { name: 'Learn More' })); + expect(userEventService.captureUserEvent).toHaveBeenCalledWith({ + alert: 'some-id', + event: 'tp.ui.banner.click', + }); }); + + it.each` + case | url | cta | expected + ${'invalid'} | ${'{https://goteleport.com/docs'} | ${undefined} | ${'{https://goteleport.com/docs'} + ${'external'} | ${'https://www.google.com'} | ${undefined} | ${'https://www.google.com'} + ${'external, CTA'} | ${'https://example.com'} | ${'Find Out'} | ${'Find Out: https://example.com'} + `( + 'renders invalid and external URLs as text: $case', + ({ url, cta, expected }) => { + const message = 'some-message'; + render( + {}} + /> + ); + expect(screen.getByText(message)).toBeInTheDocument(); + expect(screen.getByText(expected)).toBeInTheDocument(); + expect(screen.queryByRole('link')).not.toBeInTheDocument(); + } + ); }); }); diff --git a/web/packages/teleport/src/components/BannerList/StandardBanner.tsx b/web/packages/teleport/src/components/BannerList/StandardBanner.tsx index 210b952265332..9f46aab2f7e81 100644 --- a/web/packages/teleport/src/components/BannerList/StandardBanner.tsx +++ b/web/packages/teleport/src/components/BannerList/StandardBanner.tsx @@ -18,7 +18,9 @@ import React from 'react'; -import { Banner, Link } from 'design'; +import { Banner } from 'design'; + +import { Action } from 'design/Alert'; import { CaptureEvent } from 'teleport/services/userEvent/types'; import { userEventService } from 'teleport/services/userEvent'; @@ -30,6 +32,7 @@ type Props = { severity: Severity; id: string; link?: string; + linkCTA?: string; onDismiss: () => void; }; @@ -38,36 +41,46 @@ export function StandardBanner({ message = '', severity = 'info', link = '', + linkCTA = '', onDismiss, }: Props) { - const isValidTeleportLink = (link: string) => { - try { - const url = new URL(link); - return url.hostname === 'goteleport.com'; - } catch { - return false; - } - }; + const linkValid = isValidTeleportLink(link); + const details = linkValid ? undefined : bannerDetails(link, linkCTA); + const primaryAction = linkValid ? action(id, link, linkCTA) : undefined; return ( - - {isValidTeleportLink(link) ? ( - - userEventService.captureUserEvent({ - event: CaptureEvent.BannerClickEvent, - alert: id, - }) - } - > - {message} - - ) : ( - message - )} + + {message} ); } + +const isValidTeleportLink = (link: string) => { + try { + const url = new URL(link); + return url.hostname === 'goteleport.com'; + } catch { + return false; + } +}; + +const bannerDetails = (link: string, cta: string): string => + cta ? `${cta}: ${link}` : link; + +const action = (id: string, link: string, cta: string): Action => { + return { + content: cta || 'Learn More', + href: link, + onClick: () => + userEventService.captureUserEvent({ + event: CaptureEvent.BannerClickEvent, + alert: id, + }), + }; +}; diff --git a/web/packages/teleport/src/services/alerts/alerts.tsx b/web/packages/teleport/src/services/alerts/alerts.tsx index 5dc6cf52658ea..70d25d21a2b89 100644 --- a/web/packages/teleport/src/services/alerts/alerts.tsx +++ b/web/packages/teleport/src/services/alerts/alerts.tsx @@ -20,6 +20,7 @@ import api from 'teleport/services/api'; import cfg from 'teleport/config'; export const LINK_LABEL = 'teleport.internal/link'; +export const LINK_LABEL_CTA = 'teleport.internal/link-cta'; export type ClusterAlert = { kind: string; From 2caaddf3179eaca7ba637ff43554b471e6a797fd Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Thu, 29 Aug 2024 11:43:44 +0200 Subject: [PATCH 04/13] Update api/types/cluster_alert.go Co-authored-by: Zac Bergquist --- api/types/cluster_alert.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/types/cluster_alert.go b/api/types/cluster_alert.go index 4f144a384c5fa..5ce957e96a018 100644 --- a/api/types/cluster_alert.go +++ b/api/types/cluster_alert.go @@ -166,7 +166,7 @@ func (c *ClusterAlert) CheckAndSetDefaults() error { } case AlertLinkCTA: if !matchAlertLabelCTAVal(val) { - return trace.BadParameter("invalid alert: label link CTA not allowed: %q", val) + return trace.BadParameter("invalid alert: label button text not allowed: %q", val) } default: if !matchAlertLabelVal(val) { From a157da8f72a68132a6e2cbfc41da69eb559f6cb6 Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Mon, 2 Sep 2024 13:43:21 +0200 Subject: [PATCH 05/13] Rename CTA to "link text" --- api/types/cluster_alert.go | 8 ++++---- api/types/cluster_alert_test.go | 11 +++++------ api/types/constants.go | 6 +++--- lib/auth/auth.go | 8 ++++---- lib/usagereporter/teleport/aggregating/submitter.go | 4 ++-- 5 files changed, 18 insertions(+), 19 deletions(-) diff --git a/api/types/cluster_alert.go b/api/types/cluster_alert.go index 5ce957e96a018..e5f8b0e3eeb97 100644 --- a/api/types/cluster_alert.go +++ b/api/types/cluster_alert.go @@ -32,8 +32,8 @@ var matchAlertLabelKey = regexp.MustCompile(`^[a-z0-9\.\-\/]+$`).MatchString // matchAlertLabelVal is a slightly more permissive matcher for label values. var matchAlertLabelVal = regexp.MustCompile(`^[a-z0-9\.\-_\/:|]+$`).MatchString -// matchAlertLabelCTAVal only allows alphanumeric characters and spaces. -var matchAlertLabelCTAVal = regexp.MustCompile(`^[a-zA-Z0-9 ]+$`).MatchString +// matchAlertLabelLinkTextVal only allows alphanumeric characters and spaces. +var matchAlertLabelLinkTextVal = regexp.MustCompile(`^[a-zA-Z0-9 ]+$`).MatchString const validLinkDestination = "goteleport.com" @@ -164,8 +164,8 @@ func (c *ClusterAlert) CheckAndSetDefaults() error { if u.Hostname() != validLinkDestination { return trace.BadParameter("invalid alert: label link not allowed %q", val) } - case AlertLinkCTA: - if !matchAlertLabelCTAVal(val) { + case AlertLinkText: + if !matchAlertLabelLinkTextVal(val) { return trace.BadParameter("invalid alert: label button text not allowed: %q", val) } default: diff --git a/api/types/cluster_alert_test.go b/api/types/cluster_alert_test.go index 5d145ab349ed1..b8b76dfdf8795 100644 --- a/api/types/cluster_alert_test.go +++ b/api/types/cluster_alert_test.go @@ -85,12 +85,11 @@ func TestAlertSorting(t *testing.T) { } // TestCheckAndSetDefaults verifies that only valid URLs are set on the link -// label and that only valid CTA text can be used. +// label and that only valid link text can be used. func TestCheckAndSetDefaultsWithLink(t *testing.T) { tests := []struct { options []AlertOption name string - cta string assert require.ErrorAssertionFunc }{ { @@ -99,10 +98,10 @@ func TestCheckAndSetDefaultsWithLink(t *testing.T) { assert: require.NoError, }, { - name: "valid link with CTA", + name: "valid link with link text", options: []AlertOption{ WithAlertLabel(AlertLink, "https://goteleport.com/support"), - WithAlertLabel(AlertLinkCTA, "Contact Support"), + WithAlertLabel(AlertLinkText, "Contact Support"), }, assert: require.NoError, }, @@ -117,10 +116,10 @@ func TestCheckAndSetDefaultsWithLink(t *testing.T) { assert: require.Error, }, { - name: "valid link with invalid CTA", + name: "valid link with invalid link text", options: []AlertOption{ WithAlertLabel(AlertLink, "https://goteleport.com/support"), - WithAlertLabel(AlertLinkCTA, "Contact!Support"), + WithAlertLabel(AlertLinkText, "Contact!Support"), }, assert: require.Error, }, diff --git a/api/types/constants.go b/api/types/constants.go index 296771d6b1b2b..3b82ae0eabf9b 100644 --- a/api/types/constants.go +++ b/api/types/constants.go @@ -950,9 +950,9 @@ const ( // AlertLink is an internal label that indicates that an alert is a link. AlertLink = TeleportInternalLabelPrefix + "link" - // AlertLinkCTA is a call-to-action text that will be rendered by Web UI on - // the CTA button accompanying the alert. - AlertLinkCTA = TeleportInternalLabelPrefix + "link-cta" + // AlertLinkText is a text that will be rendered by Web UI on the action + // button accompanying the alert. + AlertLinkText = TeleportInternalLabelPrefix + "link-text" // AlertVerbPermit is an internal label that permits a user to view the alert if they // hold a specific resource permission verb (e.g. 'node:list'). Note that this label is diff --git a/lib/auth/auth.go b/lib/auth/auth.go index 2cae5a28f4e83..8b89edc96e76a 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -148,9 +148,9 @@ const ( OSSDesktopsAlertMessage = "Your cluster is beyond its allocation of 5 non-Active Directory Windows desktops. " + "Reach out for unlimited desktops with Teleport Enterprise." - OSSDesktopsAlertLink = "https://goteleport.com/r/upgrade-community?utm_campaign=CTA_windows_local" - OSSDesktopsAlertLinkCTA = "Contact Sales" - OSSDesktopsLimit = 5 + OSSDesktopsAlertLink = "https://goteleport.com/r/upgrade-community?utm_campaign=CTA_windows_local" + OSSDesktopsAlertLinkText = "Contact Sales" + OSSDesktopsLimit = 5 ) const ( @@ -5689,7 +5689,7 @@ func (a *Server) syncDesktopsLimitAlert(ctx context.Context) { types.WithAlertLabel(types.AlertOnLogin, "yes"), types.WithAlertLabel(types.AlertPermitAll, "yes"), types.WithAlertLabel(types.AlertLink, OSSDesktopsAlertLink), - types.WithAlertLabel(types.AlertLinkCTA, OSSDesktopsAlertLinkCTA), + types.WithAlertLabel(types.AlertLinkText, OSSDesktopsAlertLinkText), types.WithAlertExpires(time.Now().Add(OSSDesktopsCheckPeriod))) if err != nil { log.Warnf("Can't create OSS non-AD desktops limit alert: %v", err) diff --git a/lib/usagereporter/teleport/aggregating/submitter.go b/lib/usagereporter/teleport/aggregating/submitter.go index aa7cb68750a74..78939e83d897a 100644 --- a/lib/usagereporter/teleport/aggregating/submitter.go +++ b/lib/usagereporter/teleport/aggregating/submitter.go @@ -51,7 +51,7 @@ const ( alertGraceDuration = alertGraceHours * time.Hour alertName = "reporting-failed" alertLink = "https://goteleport.com/support/" - alertLinkCTA = "Contact Support" + alertLinkText = "Contact Support" ) const ( @@ -205,7 +205,7 @@ func submitOnce(ctx context.Context, c SubmitterConfig) { types.WithAlertLabel(types.AlertOnLogin, "yes"), types.WithAlertLabel(types.AlertPermitAll, "yes"), types.WithAlertLabel(types.AlertLink, alertLink), - types.WithAlertLabel(types.AlertLinkCTA, alertLinkCTA), + types.WithAlertLabel(types.AlertLinkText, alertLinkText), ) if err != nil { c.Log.WithError(err).Errorf("Failed to create cluster alert %v.", alertName) From 3709fee7a97d120d559a5f034d19fc1e11813ebf Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Mon, 2 Sep 2024 14:02:33 +0200 Subject: [PATCH 06/13] Rename CTA to "link text" --- web/packages/teleport/src/Main/Main.tsx | 8 ++++---- .../components/BannerList/BannerList.story.tsx | 16 ++++++++-------- .../src/components/BannerList/BannerList.tsx | 8 ++++---- .../BannerList/StandardBanner.test.tsx | 18 +++++++++--------- .../components/BannerList/StandardBanner.tsx | 16 ++++++++-------- .../teleport/src/services/alerts/alerts.tsx | 4 ++-- 6 files changed, 35 insertions(+), 35 deletions(-) diff --git a/web/packages/teleport/src/Main/Main.tsx b/web/packages/teleport/src/Main/Main.tsx index f673004b491e6..2ed373312d8da 100644 --- a/web/packages/teleport/src/Main/Main.tsx +++ b/web/packages/teleport/src/Main/Main.tsx @@ -46,8 +46,8 @@ import { BannerList } from 'teleport/components/BannerList'; import { storageService } from 'teleport/services/storageService'; import { ClusterAlert, - LINK_LABEL_CTA, - LINK_LABEL, + LINK_TEXT_LABEL, + LINK_DESTINATION_LABEL, } from 'teleport/services/alerts/alerts'; import { useAlerts } from 'teleport/components/BannerList/useAlerts'; import { FeaturesContextProvider, useFeatures } from 'teleport/FeaturesContext'; @@ -188,8 +188,8 @@ export function Main(props: MainProps) { const banners: BannerType[] = alerts.map(alert => ({ message: alert.spec.message, severity: mapSeverity(alert.spec.severity), - link: alert.metadata.labels[LINK_LABEL], - linkCTA: alert.metadata.labels[LINK_LABEL_CTA], + link: alert.metadata.labels[LINK_DESTINATION_LABEL], + linkText: alert.metadata.labels[LINK_TEXT_LABEL], id: alert.metadata.name, })); diff --git a/web/packages/teleport/src/components/BannerList/BannerList.story.tsx b/web/packages/teleport/src/components/BannerList/BannerList.story.tsx index e629a9659ea88..b0a60126d97a1 100644 --- a/web/packages/teleport/src/components/BannerList/BannerList.story.tsx +++ b/web/packages/teleport/src/components/BannerList/BannerList.story.tsx @@ -33,27 +33,27 @@ export function List() { id: 'ban2', severity: 'warning', message: 'Click this, or else', - linkCTA: 'Click Me', - link: 'https://goteleport.com/', + linkText: 'Click Me', + linkDestination: 'https://goteleport.com/', }, { id: 'ban3', severity: 'danger', message: 'External link', - linkCTA: 'Click Me', - link: 'https://example.com/', + linkText: 'Click Me', + linkDestination: 'https://example.com/', }, { id: 'ban4', severity: 'danger', - message: 'Default CTA', - link: 'https://goteleport.com/', + message: 'Default link text', + linkDestination: 'https://goteleport.com/', }, { id: 'ban5', severity: 'danger', - message: 'External link, default CTA', - link: 'https://google.com/', + message: 'External link, default link text', + linkDestination: 'https://google.com/', }, ]} /> diff --git a/web/packages/teleport/src/components/BannerList/BannerList.tsx b/web/packages/teleport/src/components/BannerList/BannerList.tsx index f075b77676b13..b26e0bba55bae 100644 --- a/web/packages/teleport/src/components/BannerList/BannerList.tsx +++ b/web/packages/teleport/src/components/BannerList/BannerList.tsx @@ -61,8 +61,8 @@ export const BannerList = ({ message={banner.message} severity={banner.severity} id={banner.id} - link={banner.link} - linkCTA={banner.linkCTA} + link={banner.linkDestination} + linkText={banner.linkText} onDismiss={() => removeBanner(banner.id)} key={banner.id} /> @@ -84,7 +84,7 @@ export type BannerType = { message: string; severity: Severity; id: string; - link?: string; - linkCTA?: string; + linkDestination?: string; + linkText?: string; hidden?: boolean; }; diff --git a/web/packages/teleport/src/components/BannerList/StandardBanner.test.tsx b/web/packages/teleport/src/components/BannerList/StandardBanner.test.tsx index 688789c2ef303..f176eafe3bd50 100644 --- a/web/packages/teleport/src/components/BannerList/StandardBanner.test.tsx +++ b/web/packages/teleport/src/components/BannerList/StandardBanner.test.tsx @@ -110,7 +110,7 @@ describe('StandardBanner', () => { severity="info" id="some-id" link="https://goteleport.com/docs" - linkCTA="Open Docs" + linkText="Open Docs" onDismiss={() => {}} /> ); @@ -121,8 +121,8 @@ describe('StandardBanner', () => { ); }); - it('renders valid URLs with default link CTA', () => { - const message = 'message-with-default-cta'; + it('renders valid URLs with default link text', () => { + const message = 'message-with-default-text'; render( { }); it.each` - case | url | cta | expected - ${'invalid'} | ${'{https://goteleport.com/docs'} | ${undefined} | ${'{https://goteleport.com/docs'} - ${'external'} | ${'https://www.google.com'} | ${undefined} | ${'https://www.google.com'} - ${'external, CTA'} | ${'https://example.com'} | ${'Find Out'} | ${'Find Out: https://example.com'} + case | url | linkText | expected + ${'invalid'} | ${'{https://goteleport.com/docs'} | ${undefined} | ${'{https://goteleport.com/docs'} + ${'external'} | ${'https://www.google.com'} | ${undefined} | ${'https://www.google.com'} + ${'external, link text'} | ${'https://example.com'} | ${'Find Out'} | ${'Find Out: https://example.com'} `( 'renders invalid and external URLs as text: $case', - ({ url, cta, expected }) => { + ({ url, linkText, expected }) => { const message = 'some-message'; render( { severity="info" id="some-id" link={url} - linkCTA={cta} + linkText={linkText} onDismiss={() => {}} /> ); diff --git a/web/packages/teleport/src/components/BannerList/StandardBanner.tsx b/web/packages/teleport/src/components/BannerList/StandardBanner.tsx index 9f46aab2f7e81..62256ed817a7c 100644 --- a/web/packages/teleport/src/components/BannerList/StandardBanner.tsx +++ b/web/packages/teleport/src/components/BannerList/StandardBanner.tsx @@ -32,7 +32,7 @@ type Props = { severity: Severity; id: string; link?: string; - linkCTA?: string; + linkText?: string; onDismiss: () => void; }; @@ -41,12 +41,12 @@ export function StandardBanner({ message = '', severity = 'info', link = '', - linkCTA = '', + linkText = '', onDismiss, }: Props) { const linkValid = isValidTeleportLink(link); - const details = linkValid ? undefined : bannerDetails(link, linkCTA); - const primaryAction = linkValid ? action(id, link, linkCTA) : undefined; + const details = linkValid ? undefined : bannerDetails(link, linkText); + const primaryAction = linkValid ? action(id, link, linkText) : undefined; return ( { } }; -const bannerDetails = (link: string, cta: string): string => - cta ? `${cta}: ${link}` : link; +const bannerDetails = (link: string, linkText: string): string => + linkText ? `${linkText}: ${link}` : link; -const action = (id: string, link: string, cta: string): Action => { +const action = (id: string, link: string, linkText: string): Action => { return { - content: cta || 'Learn More', + content: linkText || 'Learn More', href: link, onClick: () => userEventService.captureUserEvent({ diff --git a/web/packages/teleport/src/services/alerts/alerts.tsx b/web/packages/teleport/src/services/alerts/alerts.tsx index 70d25d21a2b89..39e4336e42db0 100644 --- a/web/packages/teleport/src/services/alerts/alerts.tsx +++ b/web/packages/teleport/src/services/alerts/alerts.tsx @@ -19,8 +19,8 @@ import api from 'teleport/services/api'; import cfg from 'teleport/config'; -export const LINK_LABEL = 'teleport.internal/link'; -export const LINK_LABEL_CTA = 'teleport.internal/link-cta'; +export const LINK_DESTINATION_LABEL = 'teleport.internal/link'; +export const LINK_TEXT_LABEL = 'teleport.internal/link-text'; export type ClusterAlert = { kind: string; From 188d8724dd3f49913a4b555d486eada8d56ca9f5 Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Mon, 2 Sep 2024 14:13:54 +0200 Subject: [PATCH 07/13] review --- .../components/BannerList/StandardBanner.tsx | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/web/packages/teleport/src/components/BannerList/StandardBanner.tsx b/web/packages/teleport/src/components/BannerList/StandardBanner.tsx index 62256ed817a7c..7b52ff63835f1 100644 --- a/web/packages/teleport/src/components/BannerList/StandardBanner.tsx +++ b/web/packages/teleport/src/components/BannerList/StandardBanner.tsx @@ -44,14 +44,30 @@ export function StandardBanner({ linkText = '', onDismiss, }: Props) { - const linkValid = isValidTeleportLink(link); - const details = linkValid ? undefined : bannerDetails(link, linkText); - const primaryAction = linkValid ? action(id, link, linkText) : undefined; + let primaryAction: Action | undefined; + let invalidLinkFallback: string | undefined; + + // We want to only use the provided link if it's valid (that is, when it + // doesn't parse or it's from outside Teleport domain). Otherwise, we display + // it as plain text. + if (isValidTeleportLink(link)) { + primaryAction = { + content: linkText || 'Learn More', + href: link, + onClick: () => + userEventService.captureUserEvent({ + event: CaptureEvent.BannerClickEvent, + alert: id, + }), + }; + } else { + invalidLinkFallback = linkText ? `${linkText}: ${link}` : link; + } return ( Date: Mon, 2 Sep 2024 14:14:28 +0200 Subject: [PATCH 08/13] Remove unnecessary functions --- .../src/components/BannerList/StandardBanner.tsx | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/web/packages/teleport/src/components/BannerList/StandardBanner.tsx b/web/packages/teleport/src/components/BannerList/StandardBanner.tsx index 7b52ff63835f1..5cd2e7988a996 100644 --- a/web/packages/teleport/src/components/BannerList/StandardBanner.tsx +++ b/web/packages/teleport/src/components/BannerList/StandardBanner.tsx @@ -85,18 +85,3 @@ const isValidTeleportLink = (link: string) => { return false; } }; - -const bannerDetails = (link: string, linkText: string): string => - linkText ? `${linkText}: ${link}` : link; - -const action = (id: string, link: string, linkText: string): Action => { - return { - content: linkText || 'Learn More', - href: link, - onClick: () => - userEventService.captureUserEvent({ - event: CaptureEvent.BannerClickEvent, - alert: id, - }), - }; -}; From 7d78c5b840186abf466cd8751827ab5f8cc323e0 Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Thu, 29 Aug 2024 15:27:42 +0200 Subject: [PATCH 09/13] Update the snackbar notification design --- web/packages/design/src/Alert/Alert.tsx | 6 +- web/packages/design/src/Alert/index.ts | 1 + .../Notification/Notification.story.tsx | 127 +++++--- .../components/Notification/Notification.tsx | 296 +++++++++++------- .../shared/components/Notification/index.ts | 6 +- .../shared/components/Notification/types.ts | 20 +- web/packages/teleport/src/Account/Account.tsx | 42 +-- .../src/DesktopSession/WarningDropdown.tsx | 2 - .../showStartupModalsAndNotifications.ts | 4 +- .../Notifcations/Notifications.story.tsx | 9 +- .../components/Notifcations/Notifications.tsx | 25 +- 11 files changed, 304 insertions(+), 234 deletions(-) diff --git a/web/packages/design/src/Alert/Alert.tsx b/web/packages/design/src/Alert/Alert.tsx index 5db4e46ff1813..44d770d78eab1 100644 --- a/web/packages/design/src/Alert/Alert.tsx +++ b/web/packages/design/src/Alert/Alert.tsx @@ -370,14 +370,14 @@ const ActionButtons = ({ }; /** Renders either a regular or a link button, depending on the action. */ -const ActionButton = ({ +export const ActionButton = ({ action: { href, content, onClick }, fill, intent, }: { action: Action; - fill: ButtonFill; - intent: ButtonIntent; + fill?: ButtonFill; + intent?: ButtonIntent; }) => href ? (