1
0
mirror of https://github.com/bitwarden/browser synced 2025-12-21 18:53:29 +00:00

[PM-27619] assign tasks component (#17125)

* feat(dirt): add newApplications$ observable to orchestrator

Add reactive observable that filters applicationData for unreviewed apps
(reviewedDate === null). Observable automatically updates when report
state changes through the pipeline.

- Add newApplications$ observable with distinctUntilChanged
- Filters rawReportData$.data.applicationData
- Uses shareReplay for multi-subscriber efficiency

Related to PM-27284

* feat(dirt): add saveApplicationReviewStatus$ to orchestrator

Implement method to save application review status and critical flags.
Updates all applications where reviewedDate === null to set current date,
and marks selected applications as critical.

- Add saveApplicationReviewStatus$() method
- Add _updateReviewStatusAndCriticalFlags() helper
- Uses existing encryption and API update patterns
- Single API call for both review status and critical flags
- Follows same pattern as saveCriticalApplications$()

Related to PM-27284

* feat(dirt): expose newApplications$ in data service

Expose orchestrator's newApplications$ observable and save method
through RiskInsightsDataService facade. Maintains clean separation
between orchestrator (business logic) and components (UI).

- Expose newApplications$ observable
- Expose saveApplicationReviewStatus() delegation method
- Maintains facade pattern consistency

Related to PM-27284

* feat(dirt): make AllActivitiesService reactive to new applications

Update AllActivitiesService to subscribe to orchestrator's newApplications$
observable instead of receiving data through summary updates.

- Subscribe to dataService.newApplications$ in constructor
- Add setNewApplications() helper method
- Remove newApplications update from setAllAppsReportSummary()
- New applications now update reactively when review status changes

Related to PM-27284

* feat(dirt): connect dialog to review status save method

Update NewApplicationsDialogComponent to call the data service's
saveApplicationReviewStatus method when marking applications as critical.

- Inject RiskInsightsDataService
- Replace placeholder onMarkAsCritical() with real implementation
- Handle success/error cases with appropriate toast notifications
- Close dialog on successful save
- Show different messages based on whether apps were marked critical

Related to PM-27284

* feat(dirt): add i18n strings for application review

Add internationalization strings for the new applications review dialog
success and error messages.

- applicationReviewSaved: Success toast title
- applicationsMarkedAsCritical: Success message when apps marked critical
- newApplicationsReviewed: Success message when apps reviewed only
- errorSavingReviewStatus: Error toast title
- pleaseTryAgain: Error toast message

Related to PM-27284

* fix(dirt): add subscription cleanup to AllActivitiesService

Critical fix for production code quality and memory leak prevention.
Adds takeUntil pattern to all subscriptions to comply with ADR-0003
(Observable Data Services) requirements.

**Subscription Cleanup (ADR-0003 Compliance):**
- Add takeUntil pattern to AllActivitiesService subscriptions
- Add _destroy$ Subject and destroy() method
- Prevents memory leaks by properly unsubscribing from observables
- Follows Observable Data Services ADR requirements

Changes:
- Import Subject and takeUntil from rxjs
- Add private _destroy$ Subject for cleanup coordination
- Apply takeUntil(this._destroy$) to all 3 subscriptions:
  - enrichedReportData$ subscription
  - criticalReportResults$ subscription
  - newApplications$ subscription
- Add destroy() method for proper resource cleanup

This ensures proper resource cleanup and follows Bitwarden's
architectural decision records for observable management.

Related to PM-27284

* fix(dirt): replace manual takeUntil with takeUntilDestroyed in AllActivitiesService

Fixes critical memory leak by replacing manual subscription cleanup
with Angular's automatic DestroyRef-based cleanup pattern.

**Changes:**
- Replace `takeUntil(this._destroy$)` with `takeUntilDestroyed()` for all 3 subscriptions
- Remove unused `_destroy$` Subject and manual `destroy()` method
- Update imports to use `@angular/core/rxjs-interop`

**Why:**
- Manual `destroy()` method was never called anywhere in codebase
- Subscriptions accumulated without cleanup, causing memory leaks
- `takeUntilDestroyed()` uses Angular's DestroyRef for automatic cleanup
- Aligns with ADR-0003 and .claude/CLAUDE.md requirements

**Impact:**
- Automatic subscription cleanup when service context is destroyed
- Prevents memory leaks during hot module reloads and route changes
- Reduces code complexity (no manual lifecycle management needed)

Related to PM-27284

* refactor(dirt): remove newApplications from OrganizationReportSummary

Removes redundant newApplications field from summary type and uses
derived newApplications$ observable from orchestrator instead.

**Changes:**
- Remove newApplications from OrganizationReportSummary type definition
- Remove dummy data array from RiskInsightsReportService.getApplicationsSummary()
- Remove newApplications subscription from AllActivitiesService
- Update AllActivityComponent to subscribe directly to dataService.newApplications$

**Why:**
- Eliminates data redundancy (stored vs derived)
- newApplications$ already computes from applicationData.reviewedDate === null
- Single source of truth: applicationData is the source
- Simplifies encrypted payload (less data in summary)
- Better separation: stored data (counts) vs computed data (lists)

**Impact:**
- No functional changes - UI continues to display new applications correctly
- Cleaner architecture with computed observable pattern

* cleanup

* fix(dirt): improve dialog type safety and error logging

Addresses critical PR review issues in NewApplicationsDialogComponent:

**Type Safety:**
- Replace unsafe type casting `(this as any).dialogRef` with proper DialogRef injection
- Inject DialogRef<boolean | undefined> using Angular's inject() function
- Ensures type safety and prevents runtime errors from missing dialogRef

**Error Handling:**
- Add LogService to dialog component
- Log errors with "[NewApplicationsDialog]" for debugging
- Maintain user-facing error toast while adding server-side logging

**Impact:**
- Eliminates TypeScript safety bypasses
- Improves production debugging capabilities
- Follows Angular dependency injection best practices

* fixing mock data and test cases for new apps

* feat(dirt): create assign tasks view component

Create standalone view component for task assignment UI that can be
embedded within dialogs or other containers.

- Add AssignTasksViewComponent with signal-based inputs/outputs
- Use input.required<number>() for selectedApplicationsCount
- Use output<void>() for tasksAssigned and back events
- Implement task calculation using SecurityTasksApiService
- Add onAssignTasks() method with loading state and error handling
- Include task summary card UI matching password-change-metric style
- Add proper subscription cleanup with takeUntilDestroyed (ADR-0003)
- Buttons included in component template (not dialog footer)
- Component retrieves organizationId from route params

Related to PM-27619

* refactor(dirt): add multi-view state management to new applications dialog

Add view state const object and properties to support toggling between
application selection and embedded assign tasks component.

- Add DialogView const object with SelectApplications and AssignTasks states (ADR-0025)
- Add DialogView type for type safety
- Add currentView property to track active view
- Import AssignTasksViewComponent for embedded use
- Add isCalculatingTasks loading state
- Inject AllActivitiesService and SecurityTasksApiService for task checking
- Implement OnInit with organizationId retrieval from route params
- Add proper subscription cleanup with takeUntilDestroyed (ADR-0003)
- Expose DialogView constants to template

Related to PM-27619

* feat(dirt): integrate assign tasks view into dialog

Implement logic to embed AssignTasksViewComponent within dialog and
handle communication via event bindings.

- Update onMarkAsCritical to check for tasks before closing dialog
- Add checkForTasksToAssign() method using SecurityTasksApiService
- Conditionally transition to AssignTasks view when tasks are available
- Add onTasksAssigned() handler to close dialog after successful assignment
- Add onBack() handler to navigate back to SelectApplications view
- Add loading state guard to prevent double-click on Mark as Critical button
- Only show success toast and close dialog if no tasks to assign

Related to PM-27619

* feat(dirt): add embedded assign tasks view to dialog template

Update dialog template to conditionally render embedded
AssignTasksViewComponent using @if directive.

- Add conditional rendering for SelectApplications and AssignTasks views
- Update dialog title dynamically based on currentView
- Embed dirt-assign-tasks-view component in AssignTasks view
- Pass selectedApplicationsCount via input binding
- Listen to tasksAssigned and back output events
- Show footer buttons only for SelectApplications view
- Add loading and disabled states to Mark as Critical button
- Change Cancel button to not auto-close (user must navigate)

Related to PM-27619

* feat(dirt): add i18n keys for assign tasks view

Add localized strings for embedded assign tasks view component.

* 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

* cleanup styling

* refactor(dirt): remove newApplications validation from OrganizationReportSummary type guard

Removes redundant newApplications field validation from the
OrganizationReportSummary type guard and related test cases.

**Changes:**
- Remove "newApplications" from allowed keys in isOrganizationReportSummary()
- Remove newApplications array validation logic
- Remove newApplications validation from validateOrganizationReportSummary()
- Remove 2 test cases for newApplications validation
- Remove newApplications field from 8 test data objects

**Rationale:**
The newApplications field was removed from OrganizationReportSummary type
definition because it's derived data that can be calculated from
applicationData (filtering where reviewedDate === null). The data is now
accessed via the reactive newApplications$ observable instead of being
stored redundantly in the summary object.

**Impact:**
- No functional changes - UI continues to display new applications via observable
- Type guard now correctly validates the actual OrganizationReportSummary structure
- Eliminates data redundancy and maintains single source of truth
- All 43 tests passing

* improve assign tasks view display

- Remove illustration/preview section (mailbox icon and prompt text)
- Show unique member count instead of calculated task count
- Use reportSummary.totalCriticalAtRiskMemberCount from AllActivitiesService
- Remove unused SecurityTasksApiService dependency
- Follow same pattern as all-activity.component.ts for consistency

* logic to fetch totals and new styling

* Fix review applications review view and assign view flow

* Fix null type checks

* refactor assign tasks dialog: use callout component, add video, fix OnPush, improve error handling

* Add columns, description, search, and bulk select to new applications dialog

* Add count placeholder for critical applications marked message

* Address claude comments

---------

Co-authored-by: Tom <ttalty@bitwarden.com>
Co-authored-by: Leslie Tilton <23057410+Banrion@users.noreply.github.com>
Co-authored-by: maxkpower <mpower@bitwarden.com>
This commit is contained in:
Alex
2025-11-03 10:25:19 -05:00
committed by GitHub
parent 5912292680
commit c6d759650b
16 changed files with 831 additions and 303 deletions

View File

@@ -56,6 +56,7 @@ import {
OrganizationReportSummary,
ReportStatus,
ReportState,
ApplicationHealthReportDetail,
} from "../../models/report-models";
import { MemberCipherDetailsApiService } from "../api/member-cipher-details-api.service";
import { RiskInsightsApiService } from "../api/risk-insights-api.service";
@@ -98,18 +99,28 @@ export class RiskInsightsOrchestratorService {
enrichedReportData$ = this._enrichedReportDataSubject.asObservable();
// New applications that haven't been reviewed (reviewedDate === null)
newApplications$: Observable<string[]> = this.rawReportData$.pipe(
newApplications$: Observable<ApplicationHealthReportDetail[]> = this.rawReportData$.pipe(
map((reportState) => {
if (!reportState.data?.applicationData) {
return [];
}
return reportState.data.applicationData
.filter((app) => app.reviewedDate === null)
.map((app) => app.applicationName);
const reportApplications = reportState.data?.applicationData || [];
const newApplications =
reportState?.data?.reportData.filter((reportApp) =>
reportApplications.some(
(app) => app.applicationName == reportApp.applicationName && app.reviewedDate == null,
),
) || [];
return newApplications;
}),
distinctUntilChanged((prev, curr) => {
if (prev.length !== curr.length) {
return false;
}
return prev.every(
(app, i) =>
app.applicationName === curr[i].applicationName &&
app.atRiskPasswordCount === curr[i].atRiskPasswordCount,
);
}),
distinctUntilChanged(
(prev, curr) => prev.length === curr.length && prev.every((app, i) => app === curr[i]),
),
shareReplay({ bufferSize: 1, refCount: true }),
);
@@ -332,9 +343,12 @@ export class RiskInsightsOrchestratorService {
}
// Create a set for quick lookup of the new critical apps
const newCriticalAppNamesSet = new Set(criticalApplications);
const newCriticalAppNamesSet = criticalApplications.map((ca) => ({
applicationName: ca,
isCritical: true,
}));
const existingApplicationData = report!.applicationData || [];
const updatedApplicationData = this._mergeApplicationData(
const updatedApplicationData = this._updateApplicationData(
existingApplicationData,
newCriticalAppNamesSet,
);
@@ -443,18 +457,18 @@ export class RiskInsightsOrchestratorService {
}
/**
* Saves review status for new applications and optionally marks selected ones as critical.
* This method:
* 1. Sets reviewedDate to current date for all applications where reviewedDate === null
* 2. Sets isCritical = true for applications in the selectedCriticalApps array
* Saves review status for new applications and optionally marks
* selected ones as critical
*
* @param selectedCriticalApps Array of application names to mark as critical (can be empty)
* @param reviewedApplications Array of application names to mark as reviewed
* @returns Observable of updated ReportState
*/
saveApplicationReviewStatus$(selectedCriticalApps: string[]): Observable<ReportState> {
this.logService.info("[RiskInsightsOrchestratorService] Saving application review status", {
criticalAppsCount: selectedCriticalApps.length,
});
saveApplicationReviewStatus$(
reviewedApplications: OrganizationReportApplication[],
): Observable<ReportState> {
this.logService.info(
`[RiskInsightsOrchestratorService] Saving application review status for ${reviewedApplications.length} applications`,
);
return this.rawReportData$.pipe(
take(1),
@@ -464,16 +478,43 @@ export class RiskInsightsOrchestratorService {
this._userId$.pipe(filter((userId) => !!userId)),
),
map(([reportState, organizationDetails, userId]) => {
const report = reportState?.data;
if (!report) {
throwError(() => Error("Tried save reviewed applications without a report"));
}
const existingApplicationData = reportState?.data?.applicationData || [];
const updatedApplicationData = this._updateReviewStatusAndCriticalFlags(
const updatedApplicationData = this._updateApplicationData(
existingApplicationData,
selectedCriticalApps,
reviewedApplications,
);
// Updated summary data after changing critical apps
const updatedSummaryData = this.reportService.getApplicationsSummary(
report!.reportData,
updatedApplicationData,
);
// Used for creating metrics with updated application data
const manualEnrichedApplications = report!.reportData.map(
(application): ApplicationHealthReportDetailEnriched => ({
...application,
isMarkedAsCritical: this.reportService.isCriticalApplication(
application,
updatedApplicationData,
),
}),
);
// For now, merge the report with the critical marking flag to make the enriched type
// We don't care about the individual ciphers in this instance
// After the report and enriched report types are consolidated, this mapping can be removed
// and the class will expose getCriticalApplications
const metrics = this._getReportMetrics(manualEnrichedApplications, updatedSummaryData);
const updatedState = {
...reportState,
data: {
...reportState.data,
summaryData: updatedSummaryData,
applicationData: updatedApplicationData,
},
} as ReportState;
@@ -484,9 +525,9 @@ export class RiskInsightsOrchestratorService {
criticalApps: updatedApplicationData.filter((app) => app.isCritical).length,
});
return { reportState, organizationDetails, updatedState, userId };
return { reportState, organizationDetails, updatedState, userId, metrics };
}),
switchMap(({ reportState, organizationDetails, updatedState, userId }) => {
switchMap(({ reportState, organizationDetails, updatedState, userId, metrics }) => {
return from(
this.riskInsightsEncryptionService.encryptRiskInsightsReport(
{
@@ -506,10 +547,11 @@ export class RiskInsightsOrchestratorService {
organizationDetails,
updatedState,
encryptedData,
metrics,
})),
);
}),
switchMap(({ reportState, organizationDetails, updatedState, encryptedData }) => {
switchMap(({ reportState, organizationDetails, updatedState, encryptedData, metrics }) => {
this.logService.debug(
`[RiskInsightsOrchestratorService] Persisting review status - report id: ${reportState?.data?.id}`,
);
@@ -521,26 +563,44 @@ export class RiskInsightsOrchestratorService {
return of({ ...reportState });
}
return this.reportApiService
.updateRiskInsightsApplicationData$(
reportState.data.id,
organizationDetails.organizationId,
{
data: {
applicationData: encryptedData.encryptedApplicationData.toSdk(),
},
// Update applications data with critical marking
const updateApplicationsCall = this.reportApiService.updateRiskInsightsApplicationData$(
reportState.data.id,
organizationDetails.organizationId,
{
data: {
applicationData: encryptedData.encryptedApplicationData.toSdk(),
},
)
.pipe(
map(() => updatedState),
catchError((error: unknown) => {
this.logService.error(
"[RiskInsightsOrchestratorService] Failed to save review status",
error,
);
return of({ ...reportState, error: "Failed to save application review status" });
}),
);
},
);
// Update summary after recomputing
const updateSummaryCall = this.reportApiService.updateRiskInsightsSummary$(
reportState.data.id,
organizationDetails.organizationId,
{
data: {
summaryData: encryptedData.encryptedSummaryData.toSdk(),
metrics: metrics.toRiskInsightsMetricsData(),
},
},
);
return forkJoin([updateApplicationsCall, updateSummaryCall]).pipe(
map(() => updatedState),
tap((finalState) => {
this._flagForUpdatesSubject.next({
...finalState,
});
}),
catchError((error: unknown) => {
this.logService.error(
"[RiskInsightsOrchestratorService] Failed to save review status",
error,
);
return of({ ...reportState, error: "Failed to save application review status" });
}),
);
}),
);
}
@@ -752,67 +812,40 @@ export class RiskInsightsOrchestratorService {
// Updates the existing application data to include critical applications
// Does not remove critical applications not in the set
private _mergeApplicationData(
private _updateApplicationData(
existingApplications: OrganizationReportApplication[],
criticalApplications: Set<string>,
updatedApplications: (Partial<OrganizationReportApplication> & { applicationName: string })[],
): OrganizationReportApplication[] {
const setToMerge = new Set(criticalApplications);
const arrayToMerge = [...updatedApplications];
const updatedApps = existingApplications.map((app) => {
const foundCritical = setToMerge.has(app.applicationName);
// Check if there is an updated app
const foundUpdatedIndex = arrayToMerge.findIndex(
(ua) => ua.applicationName == app.applicationName,
);
if (foundCritical) {
setToMerge.delete(app.applicationName);
let foundApp: Partial<OrganizationReportApplication> | null = null;
// Remove the updated app from the list
if (foundUpdatedIndex >= 0) {
foundApp = arrayToMerge[foundUpdatedIndex];
arrayToMerge.splice(foundUpdatedIndex, 1);
}
return {
...app,
isCritical: foundCritical || app.isCritical,
applicationName: app.applicationName,
isCritical: foundApp?.isCritical || app.isCritical,
reviewedDate: foundApp?.reviewedDate || app.reviewedDate,
};
});
setToMerge.forEach((applicationName) => {
updatedApps.push({
applicationName,
isCritical: true,
const newElements: OrganizationReportApplication[] = arrayToMerge.map(
(newApp): OrganizationReportApplication => ({
applicationName: newApp.applicationName,
isCritical: newApp.isCritical ?? false,
reviewedDate: null,
});
});
}),
);
return updatedApps;
}
/**
* Updates review status and critical flags for applications.
* Sets reviewedDate for all apps with null reviewedDate.
* Sets isCritical flag for apps in the criticalApplications array.
*
* @param existingApplications Current application data
* @param criticalApplications Array of application names to mark as critical
* @returns Updated application data with review dates and critical flags
*/
private _updateReviewStatusAndCriticalFlags(
existingApplications: OrganizationReportApplication[],
criticalApplications: string[],
): OrganizationReportApplication[] {
const criticalSet = new Set(criticalApplications);
const currentDate = new Date();
return existingApplications.map((app) => {
const shouldMarkCritical = criticalSet.has(app.applicationName);
const needsReviewDate = app.reviewedDate === null;
// Only create new object if changes are needed
if (needsReviewDate || shouldMarkCritical) {
return {
...app,
reviewedDate: needsReviewDate ? currentDate : app.reviewedDate,
isCritical: shouldMarkCritical || app.isCritical,
};
}
return app;
});
return updatedApps.concat(newElements);
}
// Toggles the isCritical flag on applications via criticalApplicationName

View File

@@ -10,6 +10,8 @@ import {
DrawerType,
RiskInsightsEnrichedData,
ReportStatus,
ApplicationHealthReportDetail,
OrganizationReportApplication,
} from "../../models";
import { RiskInsightsOrchestratorService } from "../domain/risk-insights-orchestrator.service";
@@ -38,7 +40,7 @@ export class RiskInsightsDataService {
readonly hasCiphers$: Observable<boolean | null> = of(null);
// New applications that need review (reviewedDate === null)
readonly newApplications$: Observable<string[]> = of([]);
readonly newApplications$: Observable<ApplicationHealthReportDetail[]> = of([]);
// ------------------------- Drawer Variables ---------------------
// Drawer variables unified into a single BehaviorSubject
@@ -257,7 +259,7 @@ export class RiskInsightsDataService {
return this.orchestrator.removeCriticalApplication$(hostname);
}
saveApplicationReviewStatus(selectedCriticalApps: string[]) {
saveApplicationReviewStatus(selectedCriticalApps: OrganizationReportApplication[]) {
return this.orchestrator.saveApplicationReviewStatus$(selectedCriticalApps);
}
}