Skip to content
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

[NV-3735]: Improve launch darkly #5517

Merged
merged 25 commits into from
May 14, 2024
Merged
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
61 changes: 48 additions & 13 deletions apps/web/cypress/tests/auth.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import * as capitalize from 'lodash.capitalize';
import { JobTitleEnum, jobTitleToLabelMapper } from '@novu/shared';
import { FeatureFlagsKeysEnum, JobTitleEnum, jobTitleToLabelMapper } from '@novu/shared';

describe('User Sign-up and Login', function () {
beforeEach(function () {
cy.mockFeatureFlags({ [FeatureFlagsKeysEnum.IS_INFORMATION_ARCHITECTURE_ENABLED]: false });
});
describe('Sign up', function () {
beforeEach(function () {
cy.clearDatabase();
Expand All @@ -10,7 +13,9 @@ describe('User Sign-up and Login', function () {

it('should allow a visitor to sign-up, login, and logout', function () {
cy.intercept('**/organization/**/switch').as('appSwitch');
cy.visit('/auth/signup');
cy.waitLoadFeatureFlags(() => {
cy.visit('/auth/signup');
});
cy.getByTestId('fullName').type('Test User');
cy.getByTestId('email').type('[email protected]');
cy.getByTestId('password').type('usEr_password_123!');
Expand All @@ -30,7 +35,9 @@ describe('User Sign-up and Login', function () {
});

it('should show account already exists when signing up with already registered mail', function () {
cy.visit('/auth/signup');
cy.waitLoadFeatureFlags(() => {
cy.visit('/auth/signup');
});
cy.getByTestId('fullName').type('Test User');
cy.getByTestId('email').type('[email protected]');
cy.getByTestId('password').type('usEr_password_123!');
Expand All @@ -40,7 +47,9 @@ describe('User Sign-up and Login', function () {
});

it('should show invalid email error when signing up with invalid email', function () {
cy.visit('/auth/signup');
cy.waitLoadFeatureFlags(() => {
cy.visit('/auth/signup');
});
cy.getByTestId('fullName').type('Test User');
cy.getByTestId('email').type('[email protected]');
cy.getByTestId('password').type('usEr_password_123!');
Expand All @@ -54,7 +63,9 @@ describe('User Sign-up and Login', function () {
if (!isCI) return;

cy.intercept('**/organization/**/switch').as('appSwitch');
cy.visit('/auth/signup');
cy.waitLoadFeatureFlags(() => {
cy.visit('/auth/signup');
});

cy.loginWithGitHub();

Expand Down Expand Up @@ -82,7 +93,9 @@ describe('User Sign-up and Login', function () {
const gitHubUserEmail = Cypress.env('GITHUB_USER_EMAIL');

cy.intercept('**/organization/**/switch').as('appSwitch');
cy.visit('/auth/signup');
cy.waitLoadFeatureFlags(() => {
cy.visit('/auth/signup');
});
cy.getByTestId('fullName').type('Test User');
cy.getByTestId('email').type(gitHubUserEmail);
cy.getByTestId('password').type('usEr_password_123!');
Expand Down Expand Up @@ -115,13 +128,19 @@ describe('User Sign-up and Login', function () {
});

it('should request a password reset flow', function () {
cy.visit('/auth/reset/request');
cy.waitLoadFeatureFlags(() => {
cy.visit('/auth/reset/request');
});
cy.getByTestId('email').type(this.session.user.email);
cy.getByTestId('submit-btn').click();
cy.getByTestId('success-screen-reset').should('be.visible');

cy.task('passwordResetToken', this.session.user._id).then((token) => {
cy.visit('/auth/reset/' + token);
});

// unfortunately there seems to be a timing issue in in which inputs are disabled
cy.wait(500);
cy.getByTestId('password').type('A123e3e3e3!');
cy.getByTestId('password-repeat').focus().type('A123e3e3e3!');

Expand All @@ -137,18 +156,24 @@ describe('User Sign-up and Login', function () {

it('should redirect to the dashboard page when a token exists in query', function () {
cy.initializeSession({ disableLocalStorage: true }).then((session) => {
cy.visit('/auth/login?token=' + session.token);
cy.waitLoadFeatureFlags(() => {
cy.visit('/auth/login?token=' + session.token);
});
cy.location('pathname').should('equal', '/workflows');
});
});

it('should be redirect login with no auth', function () {
cy.visit('/');
cy.waitLoadFeatureFlags(() => {
cy.visit('/');
});
cy.location('pathname').should('equal', '/auth/login');
});

it('should successfully login the user', function () {
cy.visit('/auth/login');
cy.waitLoadFeatureFlags(() => {
cy.visit('/auth/login');
});

cy.getByTestId('email').type('[email protected]');
cy.getByTestId('password').type('123qwe!@#');
Expand All @@ -157,7 +182,9 @@ describe('User Sign-up and Login', function () {
});

it('should show incorrect email or password error when authenticating with bad credentials', function () {
cy.visit('/auth/login');
cy.waitLoadFeatureFlags(() => {
cy.visit('/auth/login');
});

cy.getByTestId('email').type('[email protected]');
cy.getByTestId('password').type('123456');
Expand All @@ -166,7 +193,9 @@ describe('User Sign-up and Login', function () {
});

it('should show invalid email error when authenticating with invalid email', function () {
cy.visit('/auth/login');
cy.waitLoadFeatureFlags(() => {
cy.visit('/auth/login');
});

cy.getByTestId('email').type('[email protected]');
cy.getByTestId('password').type('123456');
Expand All @@ -175,7 +204,9 @@ describe('User Sign-up and Login', function () {
});

it('should show incorrect email or password error when authenticating with non-existing email', function () {
cy.visit('/auth/login');
cy.waitLoadFeatureFlags(() => {
cy.visit('/auth/login');
});

cy.getByTestId('email').type('[email protected]');
cy.getByTestId('password').type('123456');
Expand All @@ -197,6 +228,8 @@ describe('User Sign-up and Login', function () {
cy.getByTestId('password').type('123qwe!@#');
cy.getByTestId('submit-btn').click();

cy.waitLoadFeatureFlags();

cy.location('pathname').should('equal', '/workflows');

// setting current time in future, to simulate expired token
Expand All @@ -207,6 +240,8 @@ describe('User Sign-up and Login', function () {

cy.visit('/subscribers');

cy.waitLoadFeatureFlags();

// checking if token is removed from local storage
cy.getLocalStorage('auth_token').should('be.null');
// checking if user is redirected to login page
Expand Down
6 changes: 5 additions & 1 deletion apps/web/cypress/tests/invites.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { FeatureFlagsKeysEnum } from '@novu/shared';
import * as capitalize from 'lodash.capitalize';

describe('Invites module', function () {
beforeEach(function () {
cy.mockFeatureFlags({ [FeatureFlagsKeysEnum.IS_INFORMATION_ARCHITECTURE_ENABLED]: false });
cy.task('clearDatabase');
});

Expand Down Expand Up @@ -120,7 +122,9 @@ describe('Invites module', function () {
cy.initializeSession().as('session');

const invitationPath = `/auth/invitation/${this.token}`;
cy.visit(invitationPath);
cy.waitLoadFeatureFlags(() => {
cy.visit(invitationPath);
});
cy.getByTestId('success-screen-reset').click();

// checking if token is removed from local storage
Expand Down
18 changes: 11 additions & 7 deletions apps/web/src/AppRoutes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import { useSettingsRoutes } from './SettingsRoutes';

export const AppRoutes = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

🗒 note (non-blocking): the /auth/application is not a protected route 😨

const isImprovedOnboardingEnabled = useFeatureFlag(FeatureFlagsKeysEnum.IS_IMPROVED_ONBOARDING_ENABLED);
const isInformationArchitectureEnabled = useFeatureFlag(FeatureFlagsKeysEnum.IS_INFORMATION_ARCHITECTURE_ENABLED);

return (
<Routes>
Expand Down Expand Up @@ -116,13 +117,16 @@ export const AppRoutes = () => {
<Route path={ROUTES.TEAM} element={<MembersInvitePage />} />
<Route path={ROUTES.CHANGES} element={<PromoteChangesPage />} />
<Route path={ROUTES.SUBSCRIBERS} element={<SubscribersList />} />
<Route path={ROUTES.BRAND} element={<BrandPage />}>
<Route path="" element={<BrandingForm />} />
<Route path="layouts" element={<LayoutsListPage />} />
</Route>
<Route path={ROUTES.LAYOUT} element={<LayoutsPage />}>
<Route path="" element={<LayoutsListPage />} />
</Route>
{!isInformationArchitectureEnabled ? (
<Route path={ROUTES.BRAND} element={<BrandPage />}>
<Route path="" element={<BrandingForm />} />
<Route path="layouts" element={<LayoutsListPage />} />
</Route>
) : (
<Route path={ROUTES.LAYOUT} element={<LayoutsPage />}>
<Route path="" element={<LayoutsListPage />} />
</Route>
)}
<Route path="/translations/*" element={<TranslationRoutes />} />
</Route>
</Routes>
Expand Down
46 changes: 31 additions & 15 deletions apps/web/src/Providers.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { CONTEXT_PATH, LAUNCH_DARKLY_CLIENT_SIDE_ID, SegmentProvider } from '@novu/shared-web';
import { Loader } from '@mantine/core';
import { colors } from '@novu/design-system';
import { CONTEXT_PATH, SegmentProvider } from '@novu/shared-web';
import * as Sentry from '@sentry/react';
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
import { withLDProvider } from 'launchdarkly-react-client-sdk';
import { PropsWithChildren } from 'react';
import { HelmetProvider } from 'react-helmet-async';
import { BrowserRouter } from 'react-router-dom';
import { api } from './api/api.client';
import { LaunchDarklyProvider } from './components/launch-darkly';
import { AuthProvider } from './components/providers/AuthProvider';
import { css } from './styled-system/css';

const defaultQueryFn = async ({ queryKey }: { queryKey: string }) => {
const response = await api.get(`${queryKey[0]}`);
Expand All @@ -22,28 +25,41 @@ const queryClient = new QueryClient({
},
});

/** Full-page loader that uses color-preferences for background */
const fallbackDisplay = (
<div
className={css({
h: '100dvh',
w: '100dvw',
display: 'grid',
placeItems: 'center',
bg: 'surface.page',
// Root element may not have loaded so rely on OS
_osDark: { bg: 'legacy.BGDark' },
_osLight: { bg: 'legacy.BGLight' },
})}
>
<Loader size={64} variant="bars" color={colors.gradientMiddle} />
</div>
);

/**
* Centralized Provider hierarchy.
*/
const Providers: React.FC<PropsWithChildren<{}>> = ({ children }) => {
return (
<SegmentProvider>
<HelmetProvider>
<QueryClientProvider client={queryClient}>
<BrowserRouter basename={CONTEXT_PATH}>
<QueryClientProvider client={queryClient}>
<AuthProvider>{children}</AuthProvider>
</QueryClientProvider>
<AuthProvider>
<LaunchDarklyProvider fallbackDisplay={fallbackDisplay}>
<HelmetProvider>{children}</HelmetProvider>
</LaunchDarklyProvider>
</AuthProvider>
</BrowserRouter>
</HelmetProvider>
</QueryClientProvider>
</SegmentProvider>
);
};

export default Sentry.withProfiler(
withLDProvider({
clientSideID: LAUNCH_DARKLY_CLIENT_SIDE_ID,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗒 note (non-blocking): Moved LD provider to its own file in shared-web with logic‏

Copy link
Contributor

Choose a reason for hiding this comment

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

question: why move to shared-web?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of our other similar providers are there, and many of the related behaviors / imports (i.e. useFeatureFlags) are there, so it seemed best IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

🤓 nitpick (non-blocking): I suggest keeping it closer to the source of the web app unless we are sure it would be reused. Otherwise, we might end up with a landfill.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @LetItRock. I feel that we might be overusing shared packages on some occasions. Other than that, great choice to split it into a separate component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, thanks for the feedback! Updated

reactOptions: {
useCamelCaseFlagKeys: false,
},
})(Providers)
);
export default Sentry.withProfiler(Providers);
4 changes: 1 addition & 3 deletions apps/web/src/SettingsRoutes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { ProductLead } from './components/utils/ProductLead';
import { ROUTES } from './constants/routes.enum';
import { useFeatureFlag } from './hooks';
import { BillingRoutes } from './pages/BillingPages';
import { BrandingForm as BrandingFormOld } from './pages/brand/tabs';
import { BrandingPage } from './pages/brand/tabs/v2';
import { MembersInvitePage as MembersInvitePageNew } from './pages/invites/v2/MembersInvitePage';
import { AccessSecurityPage, ApiKeysPage, BillingPage, TeamPage, UserProfilePage } from './pages/settings';
Expand Down Expand Up @@ -42,15 +41,14 @@ export const useSettingsRoutes = () => {
);
}

/* TODO: remove all routes above once information architecture is fully enabled */
/* TODO: remove all routes below once information architecture is fully enabled */
return (
<>
<Route path={ROUTES.SETTINGS} element={<SettingsPageOld />}>
<Route path="" element={<ApiKeysCard />} />
<Route path="billing/*" element={<BillingRoutes />} />
<Route path="email" element={<EmailSettings />} />
<Route path="team" element={<MembersInvitePageNew />} />
<Route path="brand" element={<BrandingFormOld />} />
<Route
path="permissions"
element={
Expand Down
Loading
Loading