mirror of
https://github.com/bitwarden/browser
synced 2025-12-16 16:23:44 +00:00
[PM-27808] Save modal dismissal to state (#17227)
* Prevent modal from opening twice for same user * Claude's feedback
This commit is contained in:
@@ -7,16 +7,21 @@ import { AccountService, Account } from "@bitwarden/common/auth/abstractions/acc
|
|||||||
import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
|
import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
|
||||||
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
|
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
|
||||||
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
|
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
|
||||||
|
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
|
||||||
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
|
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
|
||||||
import { SyncService } from "@bitwarden/common/platform/sync/sync.service";
|
import { SyncService } from "@bitwarden/common/platform/sync/sync.service";
|
||||||
import { DialogRef, DialogService } from "@bitwarden/components";
|
import { DialogRef, DialogService } from "@bitwarden/components";
|
||||||
|
import { StateProvider } from "@bitwarden/state";
|
||||||
|
|
||||||
import {
|
import {
|
||||||
UnifiedUpgradeDialogComponent,
|
UnifiedUpgradeDialogComponent,
|
||||||
UnifiedUpgradeDialogStatus,
|
UnifiedUpgradeDialogStatus,
|
||||||
} from "../unified-upgrade-dialog/unified-upgrade-dialog.component";
|
} from "../unified-upgrade-dialog/unified-upgrade-dialog.component";
|
||||||
|
|
||||||
import { UnifiedUpgradePromptService } from "./unified-upgrade-prompt.service";
|
import {
|
||||||
|
UnifiedUpgradePromptService,
|
||||||
|
PREMIUM_MODAL_DISMISSED_KEY,
|
||||||
|
} from "./unified-upgrade-prompt.service";
|
||||||
|
|
||||||
describe("UnifiedUpgradePromptService", () => {
|
describe("UnifiedUpgradePromptService", () => {
|
||||||
let sut: UnifiedUpgradePromptService;
|
let sut: UnifiedUpgradePromptService;
|
||||||
@@ -29,6 +34,8 @@ describe("UnifiedUpgradePromptService", () => {
|
|||||||
const mockOrganizationService = mock<OrganizationService>();
|
const mockOrganizationService = mock<OrganizationService>();
|
||||||
const mockDialogOpen = jest.spyOn(UnifiedUpgradeDialogComponent, "open");
|
const mockDialogOpen = jest.spyOn(UnifiedUpgradeDialogComponent, "open");
|
||||||
const mockPlatformUtilsService = mock<PlatformUtilsService>();
|
const mockPlatformUtilsService = mock<PlatformUtilsService>();
|
||||||
|
const mockStateProvider = mock<StateProvider>();
|
||||||
|
const mockLogService = mock<LogService>();
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Creates a mock DialogRef that implements the required properties for testing
|
* Creates a mock DialogRef that implements the required properties for testing
|
||||||
@@ -59,6 +66,8 @@ describe("UnifiedUpgradePromptService", () => {
|
|||||||
mockDialogService,
|
mockDialogService,
|
||||||
mockOrganizationService,
|
mockOrganizationService,
|
||||||
mockPlatformUtilsService,
|
mockPlatformUtilsService,
|
||||||
|
mockStateProvider,
|
||||||
|
mockLogService,
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -72,6 +81,7 @@ describe("UnifiedUpgradePromptService", () => {
|
|||||||
mockAccountService.activeAccount$ = accountSubject.asObservable();
|
mockAccountService.activeAccount$ = accountSubject.asObservable();
|
||||||
mockPlatformUtilsService.isSelfHost.mockReturnValue(false);
|
mockPlatformUtilsService.isSelfHost.mockReturnValue(false);
|
||||||
mockConfigService.getFeatureFlag$.mockReturnValue(of(true));
|
mockConfigService.getFeatureFlag$.mockReturnValue(of(true));
|
||||||
|
mockStateProvider.getUserState$.mockReturnValue(of(false));
|
||||||
|
|
||||||
setupTestService();
|
setupTestService();
|
||||||
});
|
});
|
||||||
@@ -82,6 +92,7 @@ describe("UnifiedUpgradePromptService", () => {
|
|||||||
|
|
||||||
describe("displayUpgradePromptConditionally", () => {
|
describe("displayUpgradePromptConditionally", () => {
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
|
accountSubject.next(mockAccount); // Reset account to mockAccount
|
||||||
mockAccountService.activeAccount$ = accountSubject.asObservable();
|
mockAccountService.activeAccount$ = accountSubject.asObservable();
|
||||||
mockDialogOpen.mockReset();
|
mockDialogOpen.mockReset();
|
||||||
mockReset(mockDialogService);
|
mockReset(mockDialogService);
|
||||||
@@ -90,11 +101,16 @@ describe("UnifiedUpgradePromptService", () => {
|
|||||||
mockReset(mockVaultProfileService);
|
mockReset(mockVaultProfileService);
|
||||||
mockReset(mockSyncService);
|
mockReset(mockSyncService);
|
||||||
mockReset(mockOrganizationService);
|
mockReset(mockOrganizationService);
|
||||||
|
mockReset(mockStateProvider);
|
||||||
|
|
||||||
// Mock sync service methods
|
// Mock sync service methods
|
||||||
mockSyncService.fullSync.mockResolvedValue(true);
|
mockSyncService.fullSync.mockResolvedValue(true);
|
||||||
mockSyncService.lastSync$.mockReturnValue(of(new Date()));
|
mockSyncService.lastSync$.mockReturnValue(of(new Date()));
|
||||||
mockReset(mockPlatformUtilsService);
|
mockReset(mockPlatformUtilsService);
|
||||||
|
|
||||||
|
// Default: modal has not been dismissed
|
||||||
|
mockStateProvider.getUserState$.mockReturnValue(of(false));
|
||||||
|
mockStateProvider.setUserState.mockResolvedValue(undefined);
|
||||||
});
|
});
|
||||||
it("should subscribe to account and feature flag observables when checking display conditions", async () => {
|
it("should subscribe to account and feature flag observables when checking display conditions", async () => {
|
||||||
// Arrange
|
// Arrange
|
||||||
@@ -256,5 +272,71 @@ describe("UnifiedUpgradePromptService", () => {
|
|||||||
expect(result).toBeNull();
|
expect(result).toBeNull();
|
||||||
expect(mockDialogOpen).not.toHaveBeenCalled();
|
expect(mockDialogOpen).not.toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("should not show dialog when user has previously dismissed the modal", async () => {
|
||||||
|
// Arrange
|
||||||
|
mockConfigService.getFeatureFlag$.mockReturnValue(of(true));
|
||||||
|
mockBillingService.hasPremiumFromAnySource$.mockReturnValue(of(false));
|
||||||
|
mockOrganizationService.memberOrganizations$.mockReturnValue(of([]));
|
||||||
|
const recentDate = new Date();
|
||||||
|
recentDate.setMinutes(recentDate.getMinutes() - 3); // 3 minutes old
|
||||||
|
mockVaultProfileService.getProfileCreationDate.mockResolvedValue(recentDate);
|
||||||
|
mockPlatformUtilsService.isSelfHost.mockReturnValue(false);
|
||||||
|
mockStateProvider.getUserState$.mockReturnValue(of(true)); // User has dismissed
|
||||||
|
setupTestService();
|
||||||
|
|
||||||
|
// Act
|
||||||
|
const result = await sut.displayUpgradePromptConditionally();
|
||||||
|
|
||||||
|
// Assert
|
||||||
|
expect(result).toBeNull();
|
||||||
|
expect(mockDialogOpen).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should save dismissal state when user closes the dialog", async () => {
|
||||||
|
// Arrange
|
||||||
|
mockConfigService.getFeatureFlag$.mockReturnValue(of(true));
|
||||||
|
mockBillingService.hasPremiumFromAnySource$.mockReturnValue(of(false));
|
||||||
|
mockOrganizationService.memberOrganizations$.mockReturnValue(of([]));
|
||||||
|
const recentDate = new Date();
|
||||||
|
recentDate.setMinutes(recentDate.getMinutes() - 3); // 3 minutes old
|
||||||
|
mockVaultProfileService.getProfileCreationDate.mockResolvedValue(recentDate);
|
||||||
|
mockPlatformUtilsService.isSelfHost.mockReturnValue(false);
|
||||||
|
|
||||||
|
const expectedResult = { status: UnifiedUpgradeDialogStatus.Closed };
|
||||||
|
mockDialogOpenMethod(createMockDialogRef(expectedResult));
|
||||||
|
setupTestService();
|
||||||
|
|
||||||
|
// Act
|
||||||
|
await sut.displayUpgradePromptConditionally();
|
||||||
|
|
||||||
|
// Assert
|
||||||
|
expect(mockStateProvider.setUserState).toHaveBeenCalledWith(
|
||||||
|
PREMIUM_MODAL_DISMISSED_KEY,
|
||||||
|
true,
|
||||||
|
mockAccount.id,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should not save dismissal state when user upgrades to premium", async () => {
|
||||||
|
// Arrange
|
||||||
|
mockConfigService.getFeatureFlag$.mockReturnValue(of(true));
|
||||||
|
mockBillingService.hasPremiumFromAnySource$.mockReturnValue(of(false));
|
||||||
|
mockOrganizationService.memberOrganizations$.mockReturnValue(of([]));
|
||||||
|
const recentDate = new Date();
|
||||||
|
recentDate.setMinutes(recentDate.getMinutes() - 3); // 3 minutes old
|
||||||
|
mockVaultProfileService.getProfileCreationDate.mockResolvedValue(recentDate);
|
||||||
|
mockPlatformUtilsService.isSelfHost.mockReturnValue(false);
|
||||||
|
|
||||||
|
const expectedResult = { status: UnifiedUpgradeDialogStatus.UpgradedToPremium };
|
||||||
|
mockDialogOpenMethod(createMockDialogRef(expectedResult));
|
||||||
|
setupTestService();
|
||||||
|
|
||||||
|
// Act
|
||||||
|
await sut.displayUpgradePromptConditionally();
|
||||||
|
|
||||||
|
// Assert
|
||||||
|
expect(mockStateProvider.setUserState).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -8,16 +8,29 @@ import { AccountService } from "@bitwarden/common/auth/abstractions/account.serv
|
|||||||
import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
|
import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
|
||||||
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
|
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
|
||||||
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
|
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
|
||||||
|
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
|
||||||
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
|
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
|
||||||
import { SyncService } from "@bitwarden/common/platform/sync/sync.service";
|
import { SyncService } from "@bitwarden/common/platform/sync/sync.service";
|
||||||
import { UserId } from "@bitwarden/common/types/guid";
|
import { UserId } from "@bitwarden/common/types/guid";
|
||||||
import { DialogRef, DialogService } from "@bitwarden/components";
|
import { DialogRef, DialogService } from "@bitwarden/components";
|
||||||
|
import { BILLING_DISK, StateProvider, UserKeyDefinition } from "@bitwarden/state";
|
||||||
|
|
||||||
import {
|
import {
|
||||||
UnifiedUpgradeDialogComponent,
|
UnifiedUpgradeDialogComponent,
|
||||||
UnifiedUpgradeDialogResult,
|
UnifiedUpgradeDialogResult,
|
||||||
|
UnifiedUpgradeDialogStatus,
|
||||||
} from "../unified-upgrade-dialog/unified-upgrade-dialog.component";
|
} from "../unified-upgrade-dialog/unified-upgrade-dialog.component";
|
||||||
|
|
||||||
|
// State key for tracking premium modal dismissal
|
||||||
|
export const PREMIUM_MODAL_DISMISSED_KEY = new UserKeyDefinition<boolean>(
|
||||||
|
BILLING_DISK,
|
||||||
|
"premiumModalDismissed",
|
||||||
|
{
|
||||||
|
deserializer: (value: boolean) => value,
|
||||||
|
clearOn: [],
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
@Injectable({
|
@Injectable({
|
||||||
providedIn: "root",
|
providedIn: "root",
|
||||||
})
|
})
|
||||||
@@ -32,6 +45,8 @@ export class UnifiedUpgradePromptService {
|
|||||||
private dialogService: DialogService,
|
private dialogService: DialogService,
|
||||||
private organizationService: OrganizationService,
|
private organizationService: OrganizationService,
|
||||||
private platformUtilsService: PlatformUtilsService,
|
private platformUtilsService: PlatformUtilsService,
|
||||||
|
private stateProvider: StateProvider,
|
||||||
|
private logService: LogService,
|
||||||
) {}
|
) {}
|
||||||
|
|
||||||
private shouldShowPrompt$: Observable<boolean> = this.accountService.activeAccount$.pipe(
|
private shouldShowPrompt$: Observable<boolean> = this.accountService.activeAccount$.pipe(
|
||||||
@@ -45,22 +60,36 @@ export class UnifiedUpgradePromptService {
|
|||||||
return of(false);
|
return of(false);
|
||||||
}
|
}
|
||||||
|
|
||||||
const isProfileLessThanFiveMinutesOld = from(
|
const isProfileLessThanFiveMinutesOld$ = from(
|
||||||
this.isProfileLessThanFiveMinutesOld(account.id),
|
this.isProfileLessThanFiveMinutesOld(account.id),
|
||||||
);
|
);
|
||||||
const hasOrganizations = from(this.hasOrganizations(account.id));
|
const hasOrganizations$ = from(this.hasOrganizations(account.id));
|
||||||
|
const hasDismissedModal$ = this.hasDismissedModal$(account.id);
|
||||||
|
|
||||||
return combineLatest([
|
return combineLatest([
|
||||||
isProfileLessThanFiveMinutesOld,
|
isProfileLessThanFiveMinutesOld$,
|
||||||
hasOrganizations,
|
hasOrganizations$,
|
||||||
this.billingAccountProfileStateService.hasPremiumFromAnySource$(account.id),
|
this.billingAccountProfileStateService.hasPremiumFromAnySource$(account.id),
|
||||||
this.configService.getFeatureFlag$(FeatureFlag.PM24996_ImplementUpgradeFromFreeDialog),
|
this.configService.getFeatureFlag$(FeatureFlag.PM24996_ImplementUpgradeFromFreeDialog),
|
||||||
|
hasDismissedModal$,
|
||||||
]).pipe(
|
]).pipe(
|
||||||
map(([isProfileLessThanFiveMinutesOld, hasOrganizations, hasPremium, isFlagEnabled]) => {
|
map(
|
||||||
return (
|
([
|
||||||
isProfileLessThanFiveMinutesOld && !hasOrganizations && !hasPremium && isFlagEnabled
|
isProfileLessThanFiveMinutesOld,
|
||||||
);
|
hasOrganizations,
|
||||||
}),
|
hasPremium,
|
||||||
|
isFlagEnabled,
|
||||||
|
hasDismissed,
|
||||||
|
]) => {
|
||||||
|
return (
|
||||||
|
isProfileLessThanFiveMinutesOld &&
|
||||||
|
!hasOrganizations &&
|
||||||
|
!hasPremium &&
|
||||||
|
isFlagEnabled &&
|
||||||
|
!hasDismissed
|
||||||
|
);
|
||||||
|
},
|
||||||
|
),
|
||||||
);
|
);
|
||||||
}),
|
}),
|
||||||
take(1),
|
take(1),
|
||||||
@@ -114,6 +143,17 @@ export class UnifiedUpgradePromptService {
|
|||||||
const result = await firstValueFrom(this.unifiedUpgradeDialogRef.closed);
|
const result = await firstValueFrom(this.unifiedUpgradeDialogRef.closed);
|
||||||
this.unifiedUpgradeDialogRef = null;
|
this.unifiedUpgradeDialogRef = null;
|
||||||
|
|
||||||
|
// Save dismissal state when the modal is closed without upgrading
|
||||||
|
if (result?.status === UnifiedUpgradeDialogStatus.Closed) {
|
||||||
|
try {
|
||||||
|
await this.stateProvider.setUserState(PREMIUM_MODAL_DISMISSED_KEY, true, account.id);
|
||||||
|
} catch (error) {
|
||||||
|
// Log the error but don't block the dialog from closing
|
||||||
|
// The modal will still close properly even if persistence fails
|
||||||
|
this.logService.error("Failed to save premium modal dismissal state:", error);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Return the result or null if the dialog was dismissed without a result
|
// Return the result or null if the dialog was dismissed without a result
|
||||||
return result || null;
|
return result || null;
|
||||||
}
|
}
|
||||||
@@ -145,4 +185,15 @@ export class UnifiedUpgradePromptService {
|
|||||||
|
|
||||||
return memberOrganizations.length > 0;
|
return memberOrganizations.length > 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Checks if the user has previously dismissed the premium modal
|
||||||
|
* @param userId User ID to check
|
||||||
|
* @returns Observable that emits true if modal was dismissed, false otherwise
|
||||||
|
*/
|
||||||
|
private hasDismissedModal$(userId: UserId): Observable<boolean> {
|
||||||
|
return this.stateProvider
|
||||||
|
.getUserState$(PREMIUM_MODAL_DISMISSED_KEY, userId)
|
||||||
|
.pipe(map((dismissed) => dismissed ?? false));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user