mirror of
https://github.com/bitwarden/browser
synced 2026-02-18 10:23:52 +00:00
resolve organizationId and DI issues in assign tasks flow
- Pass organizationId via dialog data to prevent async race conditions - Pass organizationId as input to AssignTasksViewComponent (embedded components can't access route params) - Add DefaultAdminTaskService to component providers to fix NullInjectorError - Remove unnecessary route subscription from embedded component - Follow password-change-metric.component.ts pattern for consistency - Add detailed comments explaining architectural decisions and bug fixes
This commit is contained in:
@@ -91,8 +91,19 @@ export class AllActivityComponent implements OnInit {
|
||||
* Opens a dialog showing the list of new applications that can be marked as critical.
|
||||
*/
|
||||
onReviewNewApplications = async () => {
|
||||
const organizationId = this.activatedRoute.snapshot.paramMap.get("organizationId");
|
||||
|
||||
if (!organizationId) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Pass organizationId via dialog data instead of having the dialog retrieve it from route.
|
||||
// This ensures organizationId is immediately available when dialog opens, preventing
|
||||
// timing issues where the dialog's checkForTasksToAssign() method runs before
|
||||
// organizationId is populated via async route subscription.
|
||||
const dialogRef = NewApplicationsDialogComponent.open(this.dialogService, {
|
||||
newApplications: this.newApplications,
|
||||
organizationId: organizationId as any,
|
||||
});
|
||||
|
||||
await firstValueFrom(dialogRef.closed);
|
||||
|
||||
@@ -1,9 +1,6 @@
|
||||
import { CommonModule } from "@angular/common";
|
||||
import { Component, DestroyRef, OnInit, inject, input, output } from "@angular/core";
|
||||
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
|
||||
import { ActivatedRoute } from "@angular/router";
|
||||
import { Component, OnInit, inject, input, output } from "@angular/core";
|
||||
import { firstValueFrom } from "rxjs";
|
||||
import { map, filter } from "rxjs/operators";
|
||||
|
||||
import {
|
||||
AllActivitiesService,
|
||||
@@ -15,11 +12,18 @@ import { OrganizationId } from "@bitwarden/common/types/guid";
|
||||
import { ButtonModule, ToastService, TypographyModule } from "@bitwarden/components";
|
||||
import { I18nPipe } from "@bitwarden/ui-common";
|
||||
|
||||
import { DefaultAdminTaskService } from "../../../vault/services/default-admin-task.service";
|
||||
import { AccessIntelligenceSecurityTasksService } from "../shared/security-tasks.service";
|
||||
|
||||
/**
|
||||
* Embedded component for displaying task assignment UI.
|
||||
* Not a dialog - intended to be embedded within a parent dialog.
|
||||
*
|
||||
* Important: This component provides its own instances of AccessIntelligenceSecurityTasksService
|
||||
* and DefaultAdminTaskService. These services are scoped to this component to ensure proper
|
||||
* dependency injection when the component is dynamically rendered within the dialog.
|
||||
* Without these providers, Angular would throw NullInjectorError when trying to inject
|
||||
* DefaultAdminTaskService, which is required by AccessIntelligenceSecurityTasksService.
|
||||
*/
|
||||
// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush
|
||||
// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection
|
||||
@@ -27,6 +31,7 @@ import { AccessIntelligenceSecurityTasksService } from "../shared/security-tasks
|
||||
selector: "dirt-assign-tasks-view",
|
||||
templateUrl: "./assign-tasks-view.component.html",
|
||||
imports: [CommonModule, ButtonModule, TypographyModule, I18nPipe],
|
||||
providers: [AccessIntelligenceSecurityTasksService, DefaultAdminTaskService],
|
||||
})
|
||||
export class AssignTasksViewComponent implements OnInit {
|
||||
/**
|
||||
@@ -34,6 +39,12 @@ export class AssignTasksViewComponent implements OnInit {
|
||||
*/
|
||||
readonly selectedApplicationsCount = input.required<number>();
|
||||
|
||||
/**
|
||||
* Organization ID - passed from parent instead of reading from route
|
||||
* because this component is embedded in a dialog and doesn't have direct route access
|
||||
*/
|
||||
readonly organizationId = input.required<OrganizationId>();
|
||||
|
||||
/**
|
||||
* Emitted when tasks have been successfully assigned
|
||||
*/
|
||||
@@ -47,40 +58,26 @@ export class AssignTasksViewComponent implements OnInit {
|
||||
protected totalTasksToAssign = 0;
|
||||
protected isAssigning = false;
|
||||
|
||||
private destroyRef = inject(DestroyRef);
|
||||
private allActivitiesService = inject(AllActivitiesService);
|
||||
private securityTasksApiService = inject(SecurityTasksApiService);
|
||||
private accessIntelligenceSecurityTasksService = inject(AccessIntelligenceSecurityTasksService);
|
||||
private toastService = inject(ToastService);
|
||||
private i18nService = inject(I18nService);
|
||||
private logService = inject(LogService);
|
||||
private activatedRoute = inject(ActivatedRoute);
|
||||
|
||||
private organizationId: OrganizationId = "" as OrganizationId;
|
||||
|
||||
async ngOnInit(): Promise<void> {
|
||||
// Get organization ID from route params
|
||||
this.activatedRoute.paramMap
|
||||
.pipe(
|
||||
map((params) => params.get("organizationId")),
|
||||
filter((orgId): orgId is string => !!orgId),
|
||||
takeUntilDestroyed(this.destroyRef),
|
||||
)
|
||||
.subscribe((orgId) => {
|
||||
this.organizationId = orgId as OrganizationId;
|
||||
});
|
||||
|
||||
// Calculate tasks to assign
|
||||
// Calculate tasks to assign using organizationId passed from parent
|
||||
await this.calculateTasksToAssign();
|
||||
}
|
||||
|
||||
/**
|
||||
* Calculates the number of tasks that will be assigned
|
||||
* Calculates the number of tasks that will be assigned.
|
||||
* Uses the same logic as password-change-metric.component.ts
|
||||
*/
|
||||
private async calculateTasksToAssign(): Promise<void> {
|
||||
try {
|
||||
const taskMetrics = await firstValueFrom(
|
||||
this.securityTasksApiService.getTaskMetrics(this.organizationId),
|
||||
this.securityTasksApiService.getTaskMetrics(this.organizationId()),
|
||||
);
|
||||
const atRiskPasswordsCount = await firstValueFrom(
|
||||
this.allActivitiesService.atRiskPasswordsCount$,
|
||||
@@ -114,8 +111,9 @@ export class AssignTasksViewComponent implements OnInit {
|
||||
const criticalApps = allApplicationsDetails.filter((app) => app.isMarkedAsCritical);
|
||||
|
||||
// Assign tasks using the security tasks service
|
||||
// Uses the same logic as password-change-metric.component.ts
|
||||
await this.accessIntelligenceSecurityTasksService.assignTasks(
|
||||
this.organizationId,
|
||||
this.organizationId(),
|
||||
criticalApps,
|
||||
);
|
||||
|
||||
|
||||
@@ -62,6 +62,7 @@
|
||||
@if (currentView === DialogView.AssignTasks) {
|
||||
<dirt-assign-tasks-view
|
||||
[selectedApplicationsCount]="selectedApplications.size"
|
||||
[organizationId]="organizationId"
|
||||
(tasksAssigned)="onTasksAssigned()"
|
||||
(back)="onBack()"
|
||||
>
|
||||
|
||||
@@ -1,9 +1,6 @@
|
||||
import { CommonModule } from "@angular/common";
|
||||
import { Component, DestroyRef, inject, OnInit } from "@angular/core";
|
||||
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
|
||||
import { ActivatedRoute } from "@angular/router";
|
||||
import { Component, inject } from "@angular/core";
|
||||
import { firstValueFrom } from "rxjs";
|
||||
import { filter, map } from "rxjs/operators";
|
||||
|
||||
import {
|
||||
AllActivitiesService,
|
||||
@@ -27,6 +24,13 @@ import { AssignTasksViewComponent } from "./assign-tasks-view.component";
|
||||
|
||||
export interface NewApplicationsDialogData {
|
||||
newApplications: string[];
|
||||
/**
|
||||
* Organization ID is passed via dialog data instead of being retrieved from route params.
|
||||
* This ensures organizationId is available immediately when the dialog opens,
|
||||
* preventing async timing issues where user clicks "Mark as critical" before
|
||||
* the route subscription has fired.
|
||||
*/
|
||||
organizationId: OrganizationId;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -53,7 +57,7 @@ export type DialogView = (typeof DialogView)[keyof typeof DialogView];
|
||||
AssignTasksViewComponent,
|
||||
],
|
||||
})
|
||||
export class NewApplicationsDialogComponent implements OnInit {
|
||||
export class NewApplicationsDialogComponent {
|
||||
protected newApplications: string[] = [];
|
||||
protected selectedApplications: Set<string> = new Set<string>();
|
||||
|
||||
@@ -65,7 +69,6 @@ export class NewApplicationsDialogComponent implements OnInit {
|
||||
// Loading states
|
||||
protected isCalculatingTasks = false;
|
||||
|
||||
private destroyRef = inject(DestroyRef);
|
||||
private dialogRef = inject(DialogRef<boolean | undefined>);
|
||||
private dataService = inject(RiskInsightsDataService);
|
||||
private toastService = inject(ToastService);
|
||||
@@ -73,27 +76,19 @@ export class NewApplicationsDialogComponent implements OnInit {
|
||||
private logService = inject(LogService);
|
||||
private allActivitiesService = inject(AllActivitiesService);
|
||||
private securityTasksApiService = inject(SecurityTasksApiService);
|
||||
private activatedRoute = inject(ActivatedRoute);
|
||||
|
||||
private organizationId: OrganizationId = "" as OrganizationId;
|
||||
|
||||
async ngOnInit(): Promise<void> {
|
||||
// Get organization ID from route params
|
||||
this.activatedRoute.paramMap
|
||||
.pipe(
|
||||
map((params) => params.get("organizationId")),
|
||||
filter((orgId): orgId is string => !!orgId),
|
||||
takeUntilDestroyed(this.destroyRef),
|
||||
)
|
||||
.subscribe((orgId) => {
|
||||
this.organizationId = orgId as OrganizationId;
|
||||
});
|
||||
}
|
||||
/**
|
||||
* Organization ID set synchronously by static open() method from dialog data.
|
||||
* Must be available immediately (not async) because checkForTasksToAssign()
|
||||
* needs it when user clicks "Mark as critical" button.
|
||||
* Previous implementation using route subscription had timing issues.
|
||||
*/
|
||||
organizationId: OrganizationId = "" as OrganizationId;
|
||||
|
||||
/**
|
||||
* Opens the new applications dialog
|
||||
* @param dialogService The dialog service instance
|
||||
* @param data Dialog data containing the list of new applications
|
||||
* @param data Dialog data containing the list of new applications and organizationId
|
||||
* @returns Dialog reference
|
||||
*/
|
||||
static open(dialogService: DialogService, data: NewApplicationsDialogData) {
|
||||
@@ -105,9 +100,13 @@ export class NewApplicationsDialogComponent implements OnInit {
|
||||
);
|
||||
|
||||
// Set the component's data after opening
|
||||
// Important: organizationId is set synchronously here, not via async route subscription.
|
||||
// This prevents race conditions where user clicks "Mark as critical" before
|
||||
// organizationId is populated, which would cause checkForTasksToAssign() to fail.
|
||||
const instance = ref.componentInstance as NewApplicationsDialogComponent;
|
||||
if (instance) {
|
||||
instance.newApplications = data.newApplications;
|
||||
instance.organizationId = data.organizationId;
|
||||
}
|
||||
|
||||
return ref;
|
||||
@@ -189,16 +188,34 @@ export class NewApplicationsDialogComponent implements OnInit {
|
||||
*/
|
||||
private async checkForTasksToAssign(): Promise<boolean> {
|
||||
try {
|
||||
this.logService.info(
|
||||
`[NewApplicationsDialog] checkForTasksToAssign - organizationId: ${this.organizationId}`,
|
||||
);
|
||||
|
||||
if (!this.organizationId) {
|
||||
this.logService.warning("[NewApplicationsDialog] organizationId is not set yet");
|
||||
return false;
|
||||
}
|
||||
|
||||
const taskMetrics = await firstValueFrom(
|
||||
this.securityTasksApiService.getTaskMetrics(this.organizationId),
|
||||
);
|
||||
this.logService.info(
|
||||
`[NewApplicationsDialog] taskMetrics: totalTasks=${taskMetrics.totalTasks}, completedTasks=${taskMetrics.completedTasks}`,
|
||||
);
|
||||
|
||||
const atRiskPasswordsCount = await firstValueFrom(
|
||||
this.allActivitiesService.atRiskPasswordsCount$,
|
||||
);
|
||||
this.logService.info(`[NewApplicationsDialog] atRiskPasswordsCount: ${atRiskPasswordsCount}`);
|
||||
|
||||
const canAssignTasks = atRiskPasswordsCount > taskMetrics.totalTasks;
|
||||
const newTasksCount = canAssignTasks ? atRiskPasswordsCount - taskMetrics.totalTasks : 0;
|
||||
|
||||
this.logService.info(
|
||||
`[NewApplicationsDialog] canAssignTasks: ${canAssignTasks}, newTasksCount: ${newTasksCount}, returning: ${canAssignTasks && newTasksCount > 0}`,
|
||||
);
|
||||
|
||||
return canAssignTasks && newTasksCount > 0;
|
||||
} catch (error) {
|
||||
this.logService.error("[NewApplicationsDialog] Failed to check for tasks", error);
|
||||
|
||||
Reference in New Issue
Block a user