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

Replace account service with Input #13045

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Replace account service with Input #13045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 ...lt/popup/components/vault-v2/vault-generator-dialog/vault-generator-dialog.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

<vault-cipher-form-generator
[type]="params.type"
[account]="account"
(valueGenerated)="onValueGenerated($event)"
></vault-cipher-form-generator>

Expand Down
19 changes: 18 additions & 1 deletion ...ault/popup/components/vault-v2/vault-generator-dialog/vault-generator-dialog.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import { DIALOG_DATA, DialogConfig, DialogRef } from "@angular/cdk/dialog";
import { Overlay } from "@angular/cdk/overlay";
import { CommonModule } from "@angular/common";
import { Component, Inject } from "@angular/core";
import { Subject, takeUntil } from "rxjs";

import { AccountService, Account } from "@bitwarden/common/auth/abstractions/account.service";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { ButtonModule, DialogService } from "@bitwarden/components";
import { CipherFormGeneratorComponent } from "@bitwarden/vault";
Expand Down Expand Up @@ -60,11 +62,26 @@ export class VaultGeneratorDialogComponent {
*/
protected generatedValue: string = "";

/**
* The currently active account.
*/
protected account: Account | null = null;

/**
* Emits when the component is destroyed to clean up subscriptions.
*/
private readonly destroyed$ = new Subject<void>();

constructor(
@Inject(DIALOG_DATA) protected params: GeneratorDialogParams,
private dialogRef: DialogRef<GeneratorDialogResult>,
private i18nService: I18nService,
) {}
private accountService: AccountService,
) {
this.accountService.activeAccount$.pipe(takeUntil(this.destroyed$)).subscribe((account) => {
this.account = account;
});
}

/**
* Close the dialog without selecting a value.
Expand Down
1 change: 1 addition & 0 deletions apps/web/src/app/vault/components/web-generator-dialog/web-generator-dialog.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
<vault-cipher-form-generator
[type]="params.type"
(valueGenerated)="onValueGenerated($event)"
[account]="account"
disableMargin
></vault-cipher-form-generator>
</ng-container>
Expand Down
19 changes: 18 additions & 1 deletion apps/web/src/app/vault/components/web-generator-dialog/web-generator-dialog.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
import { DIALOG_DATA, DialogConfig, DialogRef } from "@angular/cdk/dialog";
import { CommonModule } from "@angular/common";
import { Component, Inject } from "@angular/core";
import { takeUntil, Subject } from "rxjs";

import { AccountService, Account } from "@bitwarden/common/auth/abstractions/account.service";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { ButtonModule, DialogModule, DialogService } from "@bitwarden/components";
import { CipherFormGeneratorComponent } from "@bitwarden/vault";
Expand Down Expand Up @@ -48,11 +50,26 @@ export class WebVaultGeneratorDialogComponent {
*/
protected generatedValue: string = "";

/**
* The currently active account.
*/
protected account: Account | null = null;

/**
* Emits when the component is destroyed to clean up subscriptions.
*/
private readonly destroyed$ = new Subject<void>();

constructor(
@Inject(DIALOG_DATA) protected params: WebVaultGeneratorDialogParams,
private dialogRef: DialogRef<WebVaultGeneratorDialogResult>,
private i18nService: I18nService,
) {}
private accountService: AccountService,
) {
this.accountService.activeAccount$.pipe(takeUntil(this.destroyed$)).subscribe((account) => {
this.account = account;
});
}

/**
* Close the dialog without selecting a value.
Expand Down
17 changes: 8 additions & 9 deletions libs/tools/generator/components/src/catchall-settings.component.ts
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

📝 All of the generator components should have a consistent implementation; these comments apply to all of them.

⛏️ The account property should replace the UserId property in all of the modified generator components.

❌ These components should implement OnChanges and populate an account$ member when the account input changes. That member then replaces the existing uses of singleUserId$. Without the OnChange implementation, Angular won't update the component when a new account is transmitted to the component through the @Input.

References:

Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,31 @@ import { Component, EventEmitter, Input, OnDestroy, OnInit, Output } from "@angu
import { FormBuilder } from "@angular/forms";
import { BehaviorSubject, map, skip, Subject, takeUntil, withLatestFrom } from "rxjs";

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { Account } from "@bitwarden/common/auth/abstractions/account.service";
import { UserId } from "@bitwarden/common/types/guid";
import {
CatchallGenerationOptions,
CredentialGeneratorService,
Generators,
} from "@bitwarden/generator-core";

import { completeOnAccountSwitch } from "./util";

/** Options group for catchall emails */
@Component({
selector: "tools-catchall-settings",
templateUrl: "catchall-settings.component.html",
})
export class CatchallSettingsComponent implements OnInit, OnDestroy {
/** Instantiates the component
* @param accountService queries user availability
* @param generatorService settings and policy logic
* @param formBuilder reactive form controls
*/
constructor(
private formBuilder: FormBuilder,
private generatorService: CredentialGeneratorService,
private accountService: AccountService,
) {}

@Input() account: Account | null = null;

/** Binds the component to a specific user's settings.
* When this input is not provided, the form binds to the active
* user
Expand Down Expand Up @@ -84,10 +82,11 @@ export class CatchallSettingsComponent implements OnInit, OnDestroy {
return new BehaviorSubject(this.userId as UserId).asObservable();
}

return this.accountService.activeAccount$.pipe(
completeOnAccountSwitch(),
takeUntil(this.destroyed$),
);
if (this.account) {
return new BehaviorSubject(this.account.id as UserId).asObservable();
}

return new BehaviorSubject<UserId | null>(null).asObservable();
}

private readonly destroyed$ = new Subject<void>();
Expand Down
18 changes: 7 additions & 11 deletions libs/tools/generator/components/src/credential-generator-history-dialog.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
// @ts-strict-ignore
import { DialogRef } from "@angular/cdk/dialog";
import { CommonModule } from "@angular/common";
import { Component } from "@angular/core";
import { Component, Input } from "@angular/core";
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
import { BehaviorSubject, distinctUntilChanged, firstValueFrom, map, switchMap } from "rxjs";
import { BehaviorSubject, firstValueFrom, map, switchMap } from "rxjs";

import { JslibModule } from "@bitwarden/angular/jslib.module";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { Account } from "@bitwarden/common/auth/abstractions/account.service";
import { UserId } from "@bitwarden/common/types/guid";
import { ButtonModule, DialogModule, DialogService } from "@bitwarden/components";
import { GeneratorHistoryService } from "@bitwarden/generator-history";
Expand All @@ -28,22 +28,18 @@ import { EmptyCredentialHistoryComponent } from "./empty-credential-history.comp
],
})
export class CredentialGeneratorHistoryDialogComponent {
@Input() account: Account | null = null;
protected readonly hasHistory$ = new BehaviorSubject<boolean>(false);
protected readonly userId$ = new BehaviorSubject<UserId>(null);

constructor(
private accountService: AccountService,
private history: GeneratorHistoryService,
private dialogService: DialogService,
private dialogRef: DialogRef,
) {
this.accountService.activeAccount$
.pipe(
takeUntilDestroyed(),
map(({ id }) => id),
distinctUntilChanged(),
)
.subscribe(this.userId$);
if (this.account) {
this.userId$.next(this.account.id);
}
Comment on lines +40 to +42
Copy link
Member

Choose a reason for hiding this comment

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

❌ this.account is never populated when the constructor runs. This check will always fail and thus the branch will never be taken. The code this replaces used the constructor to subscribe to the account service, so when the service emitted this.userId$ would populate. Following the ngOnChanges pattern I've described for the settings component should fix this issue.

📝 This feedback also applies to credential-generator-history.component.ts


this.userId$
.pipe(
Expand Down
18 changes: 7 additions & 11 deletions libs/tools/generator/components/src/credential-generator-history.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { CommonModule } from "@angular/common";
import { Component } from "@angular/core";
import { Component, Input } from "@angular/core";
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
import { RouterLink } from "@angular/router";
import { BehaviorSubject, distinctUntilChanged, map, switchMap } from "rxjs";
import { BehaviorSubject, map, switchMap } from "rxjs";

import { JslibModule } from "@bitwarden/angular/jslib.module";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { Account } from "@bitwarden/common/auth/abstractions/account.service";
import { UserId } from "@bitwarden/common/types/guid";
import {
ColorPasswordModule,
Expand Down Expand Up @@ -40,21 +40,17 @@ import { GeneratorModule } from "./generator.module";
],
})
export class CredentialGeneratorHistoryComponent {
@Input() account: Account | null = null;
protected readonly userId$ = new BehaviorSubject<UserId>(null);
protected readonly credentials$ = new BehaviorSubject<GeneratedCredential[]>([]);

constructor(
private accountService: AccountService,
private generatorService: CredentialGeneratorService,
private history: GeneratorHistoryService,
) {
this.accountService.activeAccount$
.pipe(
takeUntilDestroyed(),
map(({ id }) => id),
distinctUntilChanged(),
)
.subscribe(this.userId$);
if (this.account) {
this.userId$.next(this.account.id);
}

this.userId$
.pipe(
Expand Down
16 changes: 7 additions & 9 deletions libs/tools/generator/components/src/credential-generator.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
withLatestFrom,
} from "rxjs";

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { Account } from "@bitwarden/common/auth/abstractions/account.service";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { IntegrationId } from "@bitwarden/common/tools/integration";
Expand Down Expand Up @@ -57,7 +57,6 @@ export class CredentialGeneratorComponent implements OnInit, OnDestroy {
private toastService: ToastService,
private logService: LogService,
private i18nService: I18nService,
private accountService: AccountService,
private zone: NgZone,
private formBuilder: FormBuilder,
) {}
Expand All @@ -68,6 +67,9 @@ export class CredentialGeneratorComponent implements OnInit, OnDestroy {
@Input()
userId: UserId | null;

@Input()
account: Account | null = null;

/** Emits credentials created from a generation request. */
@Output()
readonly onGenerated = new EventEmitter<GeneratedCredential>();
Expand Down Expand Up @@ -97,13 +99,9 @@ export class CredentialGeneratorComponent implements OnInit, OnDestroy {
if (this.userId) {
this.userId$.next(this.userId);
} else {
this.accountService.activeAccount$
.pipe(
map((acct) => acct.id),
distinctUntilChanged(),
takeUntil(this.destroyed),
)
.subscribe(this.userId$);
if (this.account) {
this.userId$.next(this.account.id);
}
Comment on lines +102 to +104
Copy link
Member

Choose a reason for hiding this comment

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

🎨 Once you've implemented ngOnChanges, there won't be a need to set this.userId$ in these functions. ngOnChanges runs before ngOnInit, making the code redundant!

}

this.generatorService
Expand Down
18 changes: 9 additions & 9 deletions libs/tools/generator/components/src/forwarder-settings.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
withLatestFrom,
} from "rxjs";

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { Account } from "@bitwarden/common/auth/abstractions/account.service";
import { IntegrationId } from "@bitwarden/common/tools/integration";
import { UserId } from "@bitwarden/common/types/guid";
import {
Expand All @@ -34,8 +34,6 @@ import {
toCredentialGeneratorConfiguration,
} from "@bitwarden/generator-core";

import { completeOnAccountSwitch } from "./util";

const Controls = Object.freeze({
domain: "domain",
token: "token",
Expand All @@ -49,14 +47,12 @@ const Controls = Object.freeze({
})
export class ForwarderSettingsComponent implements OnInit, OnChanges, OnDestroy {
/** Instantiates the component
* @param accountService queries user availability
* @param generatorService settings and policy logic
* @param formBuilder reactive form controls
*/
constructor(
private formBuilder: FormBuilder,
private generatorService: CredentialGeneratorService,
private accountService: AccountService,
) {}

/** Binds the component to a specific user's settings.
Expand All @@ -66,6 +62,9 @@ export class ForwarderSettingsComponent implements OnInit, OnChanges, OnDestroy
@Input()
userId: UserId | null;

@Input()
account: Account | null = null;

@Input({ required: true })
forwarder: IntegrationId;

Expand Down Expand Up @@ -170,10 +169,11 @@ export class ForwarderSettingsComponent implements OnInit, OnChanges, OnDestroy
return new BehaviorSubject(this.userId as UserId).asObservable();
}

return this.accountService.activeAccount$.pipe(
completeOnAccountSwitch(),
takeUntil(this.destroyed$),
);
if (this.account) {
return new BehaviorSubject(this.account.id as UserId).asObservable();
}

return new BehaviorSubject<UserId | null>(null).asObservable();
}

private readonly refresh$ = new Subject<void>();
Expand Down
18 changes: 9 additions & 9 deletions libs/tools/generator/components/src/passphrase-settings.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
ReplaySubject,
} from "rxjs";

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { Account } from "@bitwarden/common/auth/abstractions/account.service";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { UserId } from "@bitwarden/common/types/guid";
import {
Expand All @@ -22,8 +22,6 @@ import {
PassphraseGenerationOptions,
} from "@bitwarden/generator-core";

import { completeOnAccountSwitch } from "./util";

const Controls = Object.freeze({
numWords: "numWords",
includeNumber: "includeNumber",
Expand All @@ -38,7 +36,6 @@ const Controls = Object.freeze({
})
export class PassphraseSettingsComponent implements OnInit, OnDestroy {
/** Instantiates the component
* @param accountService queries user availability
* @param generatorService settings and policy logic
* @param i18nService localize hints
* @param formBuilder reactive form controls
Expand All @@ -47,7 +44,6 @@ export class PassphraseSettingsComponent implements OnInit, OnDestroy {
private formBuilder: FormBuilder,
private generatorService: CredentialGeneratorService,
private i18nService: I18nService,
private accountService: AccountService,
) {}

/** Binds the component to a specific user's settings.
Expand All @@ -57,6 +53,9 @@ export class PassphraseSettingsComponent implements OnInit, OnDestroy {
@Input()
userId: UserId | null;

@Input()
account: Account | null = null;

/** When `true`, an options header is displayed by the component. Otherwise, the header is hidden. */
@Input()
showHeader: boolean = true;
Expand Down Expand Up @@ -159,10 +158,11 @@ export class PassphraseSettingsComponent implements OnInit, OnDestroy {
return new BehaviorSubject(this.userId as UserId).asObservable();
}

return this.accountService.activeAccount$.pipe(
completeOnAccountSwitch(),
takeUntil(this.destroyed$),
);
if (this.account) {
return new BehaviorSubject(this.account.id as UserId).asObservable();
}

return new BehaviorSubject<UserId | null>(null).asObservable();
}

private readonly destroyed$ = new Subject<void>();
Expand Down
Loading
Loading