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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 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
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 @@ -133,6 +133,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 @@ -60,6 +60,11 @@
)
.subscribe();

this.messageListener

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
.messages$(ClEAR_VIEW_CACHE_COMMAND)
.pipe(concatMap(() => this.popupViewCacheState.update(() => null)))

Check warning on line 65 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#L65

Added line #L65 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 @@ -41,6 +41,7 @@ export enum FeatureFlag {
InlineMenuTotp = "inline-menu-totp",
MacOsNativeCredentialSync = "macos-native-credential-sync",
PM11360RemoveProviderExportPermission = "pm-11360-remove-provider-export-permission",
PM9111ExtensionPersistAddEditForm = "pm-9111-extension-persist-add-edit-form",
PM12443RemovePagingLogic = "pm-12443-remove-paging-logic",
PrivateKeyRegeneration = "pm-12241-private-key-regeneration",
ResellerManagedOrgAlert = "PM-15814-alert-owners-of-reseller-managed-orgs",
Expand Down Expand Up @@ -95,6 +96,7 @@ export const DefaultFeatureFlagValue = {
[FeatureFlag.InlineMenuTotp]: FALSE,
[FeatureFlag.MacOsNativeCredentialSync]: FALSE,
[FeatureFlag.PM11360RemoveProviderExportPermission]: FALSE,
[FeatureFlag.PM9111ExtensionPersistAddEditForm]: FALSE,
[FeatureFlag.PM12443RemovePagingLogic]: FALSE,
[FeatureFlag.PrivateKeyRegeneration]: FALSE,
[FeatureFlag.ResellerManagedOrgAlert]: FALSE,
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 @@ -14,7 +14,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 @@ -57,4 +56,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,6 +1,6 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { importProvidersFrom } from "@angular/core";
import { importProvidersFrom, signal } from "@angular/core";

Check warning on line 3 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#L3

Added line #L3 was not covered by tests
import { action } from "@storybook/addon-actions";
import {
applicationConfig,
Expand All @@ -12,6 +12,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 15 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#L15

Added line #L15 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 @@ -20,6 +21,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 24 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#L24

Added line #L24 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 @@ -40,6 +42,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 45 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#L45

Added line #L45 was not covered by tests

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

Check warning on line 201 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#L201

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

Check warning on line 208 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#L208

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

Check warning on line 214 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#L214

Added line #L214 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 @@ -77,11 +77,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 @@ -130,8 +130,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 @@ -136,8 +136,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 @@ -172,8 +174,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
Loading