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.

Sign up for GitHub

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

show upgrade dialog to new users #2788

Merged
merged 5 commits into from
Mar 7, 2025
Merged

show upgrade dialog to new users #2788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dd3ee31
show upgrade dialog to new users
imolorhe Mar 5, 2025
6131a01
updated test
imolorhe Mar 5, 2025
b973ef3
update mock user
imolorhe Mar 5, 2025
d819486
updated test mocks
imolorhe Mar 7, 2025
0443201
use mock user in test
imolorhe Mar 7, 2025
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
1 change: 1 addition & 0 deletions packages/altair-api-utils/src/user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export interface IUserProfile {
firstName: string;
lastName?: string;
picture?: string;
isNewUser?: boolean;
tokens: IToken;
}

Expand Down
3 changes: 2 additions & 1 deletion packages/altair-api/src/auth/auth.controller.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ describe('AuthController', () => {

const tokenMock =
'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c';
let authServiceReturnMock: User & { tokens: IToken };
let authServiceReturnMock: User & { isNewUser: boolean; tokens: IToken };

beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
Expand All @@ -36,6 +36,7 @@ describe('AuthController', () => {

authServiceReturnMock = {
...mockUser(),
isNewUser: false,
tokens: {
accessToken: tokenMock,
refreshToken: tokenMock,
Expand Down
2 changes: 2 additions & 0 deletions packages/altair-api/src/auth/auth.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { PasswordService } from './password/password.service';
import { IToken } from '@altairgraphql/api-utils';
import { getAgent } from 'src/newrelic/newrelic';

const NEW_USER_TIME = 1000 * 60 * 10; // 10 minutes
@Injectable()
export class AuthService {
private readonly agent = getAgent();
Expand Down Expand Up @@ -109,6 +110,7 @@ export class AuthService {
firstName: user.firstName,
lastName: user.lastName,
picture: user.picture,
isNewUser: Date.now() - user.createdAt.getTime() < NEW_USER_TIME,
tokens: this.generateTokens({ userId: user.id }),
};
}
Expand Down
6 changes: 6 additions & 0 deletions packages/altair-api/src/auth/mocks/prisma-service.mock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@ export function mockUser(): User {
return {
id: 'f7102dc9-4c0c-42b4-9a17-e2bd4af94d5a',
stripeCustomerId: 'f7102dc9-4c0c-42b4-9a17-e2bd4af94d5a',
resendContactId: 'f7102dc9-4c0c-42b4-9a17-e2bd4af94d5a',
email: '[email protected]',
firstName: 'John',
lastName: 'Doe',
picture: 'asdf',
isNewUser: false,
emailVerified: new Date(),
password: 'password',
createdAt: new Date(),
updatedAt: new Date(),
} as User;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/altair-api/src/auth/strategies/google.strategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export class GoogleStrategy extends PassportStrategy(Strategy, 'google') {
return this.userService.createUser(
{
email,
firstName: profile?.name?.givenName || profile.displayName,
firstName: profile?.name?.givenName ?? profile.displayName,
lastName: profile?.name?.familyName,
picture: profile?.photos?.[0]?.value,
},
Expand Down
2 changes: 1 addition & 1 deletion packages/altair-api/test/auth.e2e-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe('AuthController', () => {
});

it('/auth/me (GET) should return 200 with user profile when authenticated', () => {
mockUserFn.mockReturnValue({ id: testUser.id });
mockUserFn.mockReturnValue(testUser);
return request(app.getHttpServer())
.get('/auth/me')
.expect(200)
Expand Down
16 changes: 16 additions & 0 deletions packages/altair-api/test/e2e-test-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ export const testUser = {
picture: 'https://example.com/picture.png',
firstName: 'Test',
lastName: 'User',
emailVerified: new Date(),
password: 'password',
createdAt: new Date(),
updatedAt: new Date(),
Workspace: {
create: {
name: 'Test Workspace',
Expand All @@ -44,6 +48,10 @@ export const testUser2 = {
picture: 'https://example.com/picture2.png',
firstName: 'Test2',
lastName: 'User2',
emailVerified: new Date(),
password: 'password',
createdAt: new Date(),
updatedAt: new Date(),
Workspace: {
create: {
name: 'Test 2 Workspace',
Expand All @@ -57,6 +65,10 @@ export const testUser3 = {
picture: 'https://example.com/picture3.png',
firstName: 'Test3',
lastName: 'User3',
emailVerified: new Date(),
password: 'password',
createdAt: new Date(),
updatedAt: new Date(),
Workspace: {
create: {
name: 'Test 3 Workspace',
Expand All @@ -70,6 +82,10 @@ export const testUser4 = {
picture: 'https://example.com/picture4.png',
firstName: 'Test4',
lastName: 'User4',
emailVerified: new Date(),
password: 'password',
createdAt: new Date(),
updatedAt: new Date(),
Workspace: {
create: {
name: 'Test 4 Workspace',
Expand Down
37 changes: 23 additions & 14 deletions packages/altair-app/src/app/modules/altair/effects/account.effect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { createEffect, Actions, ofType } from '@ngrx/effects';
import { Store } from '@ngrx/store';
import { RootState } from 'altair-graphql-core/build/types/state/state.interfaces';
import { environment } from 'environments/environment';
import { EMPTY, forkJoin, from, zip } from 'rxjs';
import { EMPTY, forkJoin, from, of, zip } from 'rxjs';
import {
catchError,
map,
Expand Down Expand Up @@ -57,6 +57,7 @@ export class AccountEffects {
firstName: user?.firstName ?? user?.email ?? '',
lastName: '',
picture: user.picture ?? '',
isNewUser: user.isNewUser,
})
);

Expand Down Expand Up @@ -98,10 +99,11 @@ export class AccountEffects {
);
this.store.dispatch(
Copy link

Choose a reason for hiding this comment

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

issue (complexity): Consider using guard clauses to flatten the nested if statements and improve readability by reducing complexity and improving code flow.

Consider flattening the nested ifs with guard clauses to improve readability. For example, extract the upgrade logic into its own helper or simply return early to avoid deep nesting. One option:

// Before:
if (plan.canUpgradePro) {
  const selectedPlan = consumeQueryParam('plan_select');
  if (selectedPlan === 'pro') {
    this.store.dispatch(new windowsMetaActions.ShowUpgradeDialogAction({ value: true }));
    this.accountService.getUpgradeProUrl().then(({ url }) => {
      externalLink(url);
    });
  } else if (user.isNewUser) {
    this.store.dispatch(new windowsMetaActions.ShowUpgradeDialogAction({ value: true }));
  }
}

// After:
if (!plan.canUpgradePro) {
  return;
}

const selectedPlan = consumeQueryParam('plan_select');

if (selectedPlan === 'pro') {
  this.store.dispatch(new windowsMetaActions.ShowUpgradeDialogAction({ value: true }));
  this.accountService.getUpgradeProUrl().then(({ url }) => {
    externalLink(url);
  });
  return;
}

if (user.isNewUser) {
  this.store.dispatch(new windowsMetaActions.ShowUpgradeDialogAction({ value: true }));
}

This maintains the original functionality while reducing nesting and clarifying your intent.

new accountActions.AccountIsLoggedInAction({
email: user.email || '',
firstName: user.firstName || user.email || '',
email: user.email ?? '',
firstName: user.firstName ?? user.email ?? '',
lastName: '',
picture: user.picture || '',
picture: user.picture ?? '',
isNewUser: user.isNewUser,
})
);
this.store.dispatch(
Expand Down Expand Up @@ -224,24 +226,31 @@ export class AccountEffects {
() => {
return this.actions$.pipe(
ofType(accountActions.ACCOUNT_IS_LOGGED_IN),
switchMap((action) =>
switchMap((action: accountActions.AccountIsLoggedInAction) =>
forkJoin([
fromPromise(this.accountService.getStats()),
fromPromise(this.accountService.getPlan()),
fromPromise(this.accountService.getPlanInfos()),
fromPromise(this.accountService.getAvailableCredits()),
of(action.payload),
])
),
map(([stats, plan, planInfos, availableCredits]) => {
map(([stats, plan, planInfos, availableCredits, user]) => {
// check query parameter and show the update plan dialog if necessary
const selectedPlan = consumeQueryParam('plan_select');
if (plan.canUpgradePro && selectedPlan === 'pro') {
this.store.dispatch(
new windowsMetaActions.ShowUpgradeDialogAction({ value: true })
);
this.accountService.getUpgradeProUrl().then(({ url }) => {
externalLink(url);
});
if (plan.canUpgradePro) {
const selectedPlan = consumeQueryParam('plan_select');
if (selectedPlan === 'pro') {
this.store.dispatch(
new windowsMetaActions.ShowUpgradeDialogAction({ value: true })
);
this.accountService.getUpgradeProUrl().then(({ url }) => {
externalLink(url);
});
} else if (user.isNewUser) {
this.store.dispatch(
new windowsMetaActions.ShowUpgradeDialogAction({ value: true })
);
}
Comment on lines +240 to +253
Copy link

Choose a reason for hiding this comment

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

medium

The plan.canUpgradePro check is performed, and then the selectedPlan === 'pro' check is nested inside. If plan.canUpgradePro is false, the inner block will never execute. Consider simplifying this logic by removing the outer if statement, since the inner logic depends on it anyway. Also, it might be clearer to use an else if instead of two separate if statements.

if (plan.canUpgradePro && selectedPlan === 'pro') {
              this.store.dispatch(
                new windowsMetaActions.ShowUpgradeDialogAction({ value: true })
              );
              this.accountService.getUpgradeProUrl().then(({ url }) => {
                externalLink(url);
              });
            } else if (user.isNewUser) {
              this.store.dispatch(
                new windowsMetaActions.ShowUpgradeDialogAction({ value: true })
              );

}

this.store.dispatch(
Expand Down
1 change: 1 addition & 0 deletions packages/altair-app/src/app/modules/altair/store/account/account.action.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export class AccountIsLoggedInAction implements NGRXAction {
firstName: string;
lastName: string;
picture: string;
isNewUser?: boolean;
}
) {}
}
Expand Down
Loading