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

[PM-9111] Extension: persist add/edit form #12236

Merged
merged 25 commits into from
Jan 22, 2025
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
83b8a0b
remove todo
nick-livefront Dec 3, 2024
69753d2
Retrieve cache cipher for add-edit form
nick-livefront Dec 3, 2024
7823c00
user prefilled cipher for add-edit form
nick-livefront Dec 3, 2024
feba423
add listener for clearing view cache
nick-livefront Dec 3, 2024
fd38a8c
clear local cache when clearing global state
nick-livefront Dec 3, 2024
fed1991
track initial value of cache for down stream logic that should only oโ€ฆ
nick-livefront Dec 3, 2024
d85f7f4
add feature flag for edit form persistence
nick-livefront Dec 3, 2024
388b1f2
add tests for cipher form cache service
nick-livefront Dec 4, 2024
2f59a3d
Merge branch 'main' of https://github.com/bitwarden/clients into vaulโ€ฆ
nick-livefront Dec 4, 2024
3f6374f
fix optional initialValues
nick-livefront Dec 4, 2024
7b12559
add services to cipher form storybook
nick-livefront Dec 4, 2024
0720a9d
Merge branch 'main' of https://github.com/bitwarden/clients into vaulโ€ฆ
nick-livefront Dec 12, 2024
00266fd
fix strict types
nick-livefront Dec 12, 2024
cb5d689
rename variables to be platform agnostic
nick-livefront Dec 12, 2024
7ef3a7b
use deconstructed collectionIds variable to avoid them be overwritten
nick-livefront Dec 12, 2024
77c0c19
use the originalCipherView for initial values
nick-livefront Dec 13, 2024
5c0a399
add comment about signal equality
nick-livefront Dec 13, 2024
6736292
Merge branch 'main' of https://github.com/bitwarden/clients into vaulโ€ฆ
nick-livefront Dec 19, 2024
1335c3a
Merge branch 'main' into vault/pm-9111/persist-add-edit
nick-livefront Jan 2, 2025
e8b9bf6
Merge branch 'main' of https://github.com/bitwarden/clients into vaulโ€ฆ
nick-livefront Jan 14, 2025
af9dc2e
prevent events from being emitted when adding uris to the existing form
nick-livefront Jan 14, 2025
b8b8c2c
add check for cached cipher when adding initial uris
nick-livefront Jan 14, 2025
e8188f8
Merge branch 'main' of https://github.com/bitwarden/clients into vaulโ€ฆ
nick-livefront Jan 14, 2025
a4a6d2d
Merge branch 'main' of https://github.com/bitwarden/clients into vaulโ€ฆ
nick-livefront Jan 17, 2025
42c1ffd
Merge branch 'main' of https://github.com/bitwarden/clients into vaulโ€ฆ
nick-livefront Jan 22, 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
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ export class PopupViewCacheService implements ViewCacheService {
}

private clearState() {
this._cache = {}; // clear local cache
this.messageSender.send(ClEAR_VIEW_CACHE_COMMAND, {});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,15 @@ describe("popup view cache", () => {
});

it("should clear on 2nd navigation", async () => {
await initServiceWithState({});
await initServiceWithState({ temp: "state" });

await router.navigate(["a"]);
expect(messageSenderMock.send).toHaveBeenCalledTimes(0);
expect(service["_cache"]).toEqual({ temp: "state" });

await router.navigate(["b"]);
expect(messageSenderMock.send).toHaveBeenCalledWith(ClEAR_VIEW_CACHE_COMMAND, {});
expect(service["_cache"]).toEqual({});
});

it("should ignore cached values when feature flag is off", async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@
)
.subscribe();

this.messageListener

Check warning on line 61 in apps/browser/src/platform/services/popup-view-cache-background.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/platform/services/popup-view-cache-background.service.ts#L61

Added line #L61 was not covered by tests
.messages$(ClEAR_VIEW_CACHE_COMMAND)
.pipe(concatMap(() => this.popupViewCacheState.update(() => null)))

Check warning on line 63 in apps/browser/src/platform/services/popup-view-cache-background.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/platform/services/popup-view-cache-background.service.ts#L63

Added line #L63 was not covered by tests
.subscribe();

merge(
// on tab changed, excluding extension tabs
fromChromeEvent(chrome.tabs.onActivated).pipe(
Expand Down
2 changes: 2 additions & 0 deletions libs/common/src/enums/feature-flag.enum.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export enum FeatureFlag {
NewDeviceVerificationPermanentDismiss = "new-device-permanent-dismiss",
DisableFreeFamiliesSponsorship = "PM-12274-disable-free-families-sponsorship",
PM11360RemoveProviderExportPermission = "pm-11360-remove-provider-export-permission",
PM9111ExtensionPersistAddEditForm = "pm-9111-extension-persist-add-edit-form",
}

export type AllowedFeatureFlagTypes = boolean | number | string;
Expand Down Expand Up @@ -90,6 +91,7 @@ export const DefaultFeatureFlagValue = {
[FeatureFlag.NewDeviceVerificationPermanentDismiss]: FALSE,
[FeatureFlag.DisableFreeFamiliesSponsorship]: FALSE,
[FeatureFlag.PM11360RemoveProviderExportPermission]: FALSE,
[FeatureFlag.PM9111ExtensionPersistAddEditForm]: FALSE,
} satisfies Record<FeatureFlag, AllowedFeatureFlagTypes>;

export type DefaultFeatureFlagValueType = typeof DefaultFeatureFlagValue;
Expand Down
9 changes: 8 additions & 1 deletion libs/vault/src/cipher-form/cipher-form-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import { SshKeySectionComponent } from "./components/sshkey-section/sshkey-secti

/**
* The complete form for a cipher. Includes all the sub-forms from their respective section components.
* TODO: Add additional form sections as they are implemented.
*/
export type CipherForm = {
itemDetails?: ItemDetailsSectionComponent["itemDetailsForm"];
Expand Down Expand Up @@ -55,4 +54,12 @@ export abstract class CipherFormContainer {
* @param updateFn - A function that takes the current cipherView and returns the updated cipherView
*/
abstract patchCipher(updateFn: (current: CipherView) => CipherView): void;

/**
* Returns initial values for the CipherView, either from the config or the cached cipher
*/
abstract getInitialCipherView(): CipherView | null;

/** Returns true when the `CipherFormContainer` was initialized with a cached cipher available. */
abstract initializedWithCachedCipher(): boolean;
}
24 changes: 23 additions & 1 deletion libs/vault/src/cipher-form/cipher-form.stories.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { importProvidersFrom } from "@angular/core";
import { importProvidersFrom, signal } from "@angular/core";

Check warning on line 1 in libs/vault/src/cipher-form/cipher-form.stories.ts

View check run for this annotation

Codecov / codecov/patch

libs/vault/src/cipher-form/cipher-form.stories.ts#L1

Added line #L1 was not covered by tests
import { action } from "@storybook/addon-actions";
import {
applicationConfig,
Expand All @@ -10,6 +10,7 @@
import { BehaviorSubject } from "rxjs";

import { CollectionView } from "@bitwarden/admin-console/common";
import { ViewCacheService } from "@bitwarden/angular/platform/abstractions/view-cache.service";

Check warning on line 13 in libs/vault/src/cipher-form/cipher-form.stories.ts

View check run for this annotation

Codecov / codecov/patch

libs/vault/src/cipher-form/cipher-form.stories.ts#L13

Added line #L13 was not covered by tests
import { AuditService } from "@bitwarden/common/abstractions/audit.service";
import { EventCollectionService } from "@bitwarden/common/abstractions/event/event-collection.service";
import { Organization } from "@bitwarden/common/admin-console/models/domain/organization";
Expand All @@ -18,6 +19,7 @@
import { DomainSettingsService } from "@bitwarden/common/autofill/services/domain-settings.service";
import { ClientType } from "@bitwarden/common/enums";
import { UriMatchStrategy } from "@bitwarden/common/models/domain/domain-service";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";

Check warning on line 22 in libs/vault/src/cipher-form/cipher-form.stories.ts

View check run for this annotation

Codecov / codecov/patch

libs/vault/src/cipher-form/cipher-form.stories.ts#L22

Added line #L22 was not covered by tests
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { CipherType } from "@bitwarden/common/vault/enums";
import { Cipher } from "@bitwarden/common/vault/models/domain/cipher";
Expand All @@ -38,6 +40,7 @@
import { TotpCaptureService } from "./abstractions/totp-capture.service";
import { CipherFormModule } from "./cipher-form.module";
import { CipherFormComponent } from "./components/cipher-form.component";
import { CipherFormCacheService } from "./services/default-cipher-form-cache.service";

Check warning on line 43 in libs/vault/src/cipher-form/cipher-form.stories.ts

View check run for this annotation

Codecov / codecov/patch

libs/vault/src/cipher-form/cipher-form.stories.ts#L43

Added line #L43 was not covered by tests

const defaultConfig: CipherFormConfig = {
mode: "add",
Expand Down Expand Up @@ -190,6 +193,25 @@
activeAccount$: new BehaviorSubject({ email: "[email protected]" }),
},
},
{
provide: CipherFormCacheService,
useValue: {
getCachedCipherView: (): null => null,

Check warning on line 199 in libs/vault/src/cipher-form/cipher-form.stories.ts

View check run for this annotation

Codecov / codecov/patch

libs/vault/src/cipher-form/cipher-form.stories.ts#L199

Added line #L199 was not covered by tests
initializedWithValue: false,
},
},
{
provide: ViewCacheService,
useValue: {
signal: () => signal(null),

Check warning on line 206 in libs/vault/src/cipher-form/cipher-form.stories.ts

View check run for this annotation

Codecov / codecov/patch

libs/vault/src/cipher-form/cipher-form.stories.ts#L206

Added line #L206 was not covered by tests
},
},
{
provide: ConfigService,
useValue: {
getFeatureFlag: () => Promise.resolve(false),

Check warning on line 212 in libs/vault/src/cipher-form/cipher-form.stories.ts

View check run for this annotation

Codecov / codecov/patch

libs/vault/src/cipher-form/cipher-form.stories.ts#L212

Added line #L212 was not covered by tests
},
},
],
}),
componentWrapperDecorator(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,12 @@ describe("AdditionalOptionsSectionComponent", () => {
let passwordRepromptService: MockProxy<PasswordRepromptService>;
let passwordRepromptEnabled$: BehaviorSubject<boolean>;

const getInitialCipherView = jest.fn(() => null);

beforeEach(async () => {
cipherFormProvider = mock<CipherFormContainer>();
getInitialCipherView.mockClear();

cipherFormProvider = mock<CipherFormContainer>({ getInitialCipherView });
passwordRepromptService = mock<PasswordRepromptService>();
passwordRepromptEnabled$ = new BehaviorSubject(true);
passwordRepromptService.enabled$ = passwordRepromptEnabled$;
Expand Down Expand Up @@ -94,11 +97,11 @@ describe("AdditionalOptionsSectionComponent", () => {
expect(component.additionalOptionsForm.disabled).toBe(true);
});

it("initializes 'additionalOptionsForm' with original cipher view values", () => {
(cipherFormProvider.originalCipherView as any) = {
it("initializes 'additionalOptionsForm' from `getInitialCipherValue`", () => {
getInitialCipherView.mockReturnValueOnce({
notes: "original notes",
reprompt: 1,
} as CipherView;
} as CipherView);

component.ngOnInit();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,12 @@ export class AdditionalOptionsSectionComponent implements OnInit {
}

ngOnInit() {
if (this.cipherFormContainer.originalCipherView) {
const prefillCipher = this.cipherFormContainer.getInitialCipherView();

if (prefillCipher) {
this.additionalOptionsForm.patchValue({
notes: this.cipherFormContainer.originalCipherView.notes,
reprompt:
this.cipherFormContainer.originalCipherView.reprompt === CipherRepromptType.Password,
notes: prefillCipher.notes,
reprompt: prefillCipher.reprompt === CipherRepromptType.Password,
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@ describe("AutofillOptionsComponent", () => {
let domainSettingsService: MockProxy<DomainSettingsService>;
let autofillSettingsService: MockProxy<AutofillSettingsServiceAbstraction>;
let platformUtilsService: MockProxy<PlatformUtilsService>;
const getInitialCipherView = jest.fn(() => null);

beforeEach(async () => {
cipherFormContainer = mock<CipherFormContainer>();
getInitialCipherView.mockClear();
cipherFormContainer = mock<CipherFormContainer>({ getInitialCipherView });
liveAnnouncer = mock<LiveAnnouncer>();
platformUtilsService = mock<PlatformUtilsService>();
domainSettingsService = mock<DomainSettingsService>();
Expand Down Expand Up @@ -107,12 +109,14 @@ describe("AutofillOptionsComponent", () => {
existingLogin.uri = "https://example.com";
existingLogin.match = UriMatchStrategy.Exact;

(cipherFormContainer.originalCipherView as CipherView) = new CipherView();
cipherFormContainer.originalCipherView.login = {
const cipher = new CipherView();
cipher.login = {
autofillOnPageLoad: true,
uris: [existingLogin],
} as LoginView;

getInitialCipherView.mockReturnValueOnce(cipher);

fixture.detectChanges();

expect(component.autofillOptionsForm.value.uris).toEqual([
Expand All @@ -138,12 +142,14 @@ describe("AutofillOptionsComponent", () => {
existingLogin.uri = "https://example.com";
existingLogin.match = UriMatchStrategy.Exact;

(cipherFormContainer.originalCipherView as CipherView) = new CipherView();
cipherFormContainer.originalCipherView.login = {
const cipher = new CipherView();
cipher.login = {
autofillOnPageLoad: true,
uris: [existingLogin],
} as LoginView;

getInitialCipherView.mockReturnValueOnce(cipher);

fixture.detectChanges();

expect(component.autofillOptionsForm.value.uris).toEqual([
Expand All @@ -159,12 +165,14 @@ describe("AutofillOptionsComponent", () => {
existingLogin.uri = "https://example.com";
existingLogin.match = UriMatchStrategy.Exact;

(cipherFormContainer.originalCipherView as CipherView) = new CipherView();
cipherFormContainer.originalCipherView.login = {
const cipher = new CipherView();
cipher.login = {
autofillOnPageLoad: true,
uris: [existingLogin],
} as LoginView;

getInitialCipherView.mockReturnValueOnce(cipher);

fixture.detectChanges();

expect(component.autofillOptionsForm.value.uris).toEqual([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,9 @@ export class AutofillOptionsComponent implements OnInit {
}

ngOnInit() {
if (this.cipherFormContainer.originalCipherView?.login) {
this.initFromExistingCipher(this.cipherFormContainer.originalCipherView.login);
const prefillCipher = this.cipherFormContainer.getInitialCipherView();
if (prefillCipher) {
this.initFromExistingCipher(prefillCipher.login);
} else {
this.initNewCipher();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ describe("CardDetailsSectionComponent", () => {
let registerChildFormSpy: jest.SpyInstance;
let patchCipherSpy: jest.SpyInstance;

const getInitialCipherView = jest.fn(() => null);

beforeEach(async () => {
cipherFormProvider = mock<CipherFormContainer>();
cipherFormProvider = mock<CipherFormContainer>({ getInitialCipherView });
registerChildFormSpy = jest.spyOn(cipherFormProvider, "registerChildForm");
patchCipherSpy = jest.spyOn(cipherFormProvider, "patchCipher");

Expand Down Expand Up @@ -94,7 +96,7 @@ describe("CardDetailsSectionComponent", () => {
expect(component.cardDetailsForm.disabled).toBe(true);
});

it("initializes `cardDetailsForm` with current values", () => {
it("initializes `cardDetailsForm` from `getInitialCipherValue`", () => {
const cardholderName = "Ron Burgundy";
const number = "4242 4242 4242 4242";
const code = "619";
Expand All @@ -105,9 +107,7 @@ describe("CardDetailsSectionComponent", () => {
cardView.code = code;
cardView.brand = "Visa";

component.originalCipherView = {
card: cardView,
} as CipherView;
getInitialCipherView.mockReturnValueOnce({ card: cardView });

component.ngOnInit();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,10 @@ export class CardDetailsSectionComponent implements OnInit {
}

ngOnInit() {
if (this.originalCipherView?.card) {
this.setInitialValues();
const prefillCipher = this.cipherFormContainer.getInitialCipherView();

if (prefillCipher) {
this.setInitialValues(prefillCipher);
}

if (this.disabled) {
Expand Down Expand Up @@ -170,8 +172,8 @@ export class CardDetailsSectionComponent implements OnInit {
}

/** Set form initial form values from the current cipher */
private setInitialValues() {
const { cardholderName, number, brand, expMonth, expYear, code } = this.originalCipherView.card;
private setInitialValues(cipherView: CipherView) {
const { cardholderName, number, brand, expMonth, expYear, code } = cipherView.card;

this.cardDetailsForm.setValue({
cardholderName: cardholderName,
Expand Down
Loading