mirror of
https://github.com/bitwarden/browser
synced 2025-12-18 01:03:35 +00:00
[PM-27284] new applications card real data (#17088)
* 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 * 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 --------- Co-authored-by: Tom <ttalty@bitwarden.com>
This commit is contained in:
@@ -69,8 +69,13 @@ export class AllActivityComponent implements OnInit {
|
||||
this.totalCriticalAppsAtRiskMemberCount = summary.totalCriticalAtRiskMemberCount;
|
||||
this.totalCriticalAppsCount = summary.totalCriticalApplicationCount;
|
||||
this.totalCriticalAppsAtRiskCount = summary.totalCriticalAtRiskApplicationCount;
|
||||
this.newApplications = summary.newApplications;
|
||||
this.newApplicationsCount = summary.newApplications.length;
|
||||
});
|
||||
|
||||
this.dataService.newApplications$
|
||||
.pipe(takeUntilDestroyed(this.destroyRef))
|
||||
.subscribe((newApps) => {
|
||||
this.newApplications = newApps;
|
||||
this.newApplicationsCount = newApps.length;
|
||||
});
|
||||
|
||||
this.allActivitiesService.passwordChangeProgressMetricHasProgressBar$
|
||||
|
||||
@@ -1,10 +1,14 @@
|
||||
import { CommonModule } from "@angular/common";
|
||||
import { Component, inject } from "@angular/core";
|
||||
import { firstValueFrom } from "rxjs";
|
||||
|
||||
import { RiskInsightsDataService } from "@bitwarden/bit-common/dirt/reports/risk-insights";
|
||||
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
|
||||
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
|
||||
import {
|
||||
ButtonModule,
|
||||
DialogModule,
|
||||
DialogRef,
|
||||
DialogService,
|
||||
ToastService,
|
||||
TypographyModule,
|
||||
@@ -25,8 +29,11 @@ export class NewApplicationsDialogComponent {
|
||||
protected newApplications: string[] = [];
|
||||
protected selectedApplications: Set<string> = new Set<string>();
|
||||
|
||||
private dialogRef = inject(DialogRef<boolean | undefined>);
|
||||
private dataService = inject(RiskInsightsDataService);
|
||||
private toastService = inject(ToastService);
|
||||
private i18nService = inject(I18nService);
|
||||
private logService = inject(LogService);
|
||||
|
||||
/**
|
||||
* Opens the new applications dialog
|
||||
@@ -35,7 +42,7 @@ export class NewApplicationsDialogComponent {
|
||||
* @returns Dialog reference
|
||||
*/
|
||||
static open(dialogService: DialogService, data: NewApplicationsDialogData) {
|
||||
const ref = dialogService.open<boolean, NewApplicationsDialogData>(
|
||||
const ref = dialogService.open<boolean | undefined, NewApplicationsDialogData>(
|
||||
NewApplicationsDialogComponent,
|
||||
{
|
||||
data,
|
||||
@@ -73,16 +80,34 @@ export class NewApplicationsDialogComponent {
|
||||
};
|
||||
|
||||
/**
|
||||
* Placeholder handler for mark as critical functionality.
|
||||
* Shows a toast notification with count of selected applications.
|
||||
* TODO: Implement actual mark as critical functionality (PM-26203 follow-up)
|
||||
* Handles the "Mark as Critical" button click.
|
||||
* Saves review status for all new applications and marks selected ones as critical.
|
||||
* Closes the dialog on success.
|
||||
*/
|
||||
onMarkAsCritical = () => {
|
||||
const selectedCount = this.selectedApplications.size;
|
||||
this.toastService.showToast({
|
||||
variant: "info",
|
||||
title: this.i18nService.t("markAsCritical"),
|
||||
message: `${selectedCount} ${this.i18nService.t("applicationsSelected")}`,
|
||||
});
|
||||
onMarkAsCritical = async () => {
|
||||
const selectedCriticalApps = Array.from(this.selectedApplications);
|
||||
|
||||
try {
|
||||
await firstValueFrom(this.dataService.saveApplicationReviewStatus(selectedCriticalApps));
|
||||
|
||||
this.toastService.showToast({
|
||||
variant: "success",
|
||||
title: this.i18nService.t("applicationReviewSaved"),
|
||||
message:
|
||||
selectedCriticalApps.length > 0
|
||||
? this.i18nService.t("applicationsMarkedAsCritical", selectedCriticalApps.length)
|
||||
: this.i18nService.t("newApplicationsReviewed"),
|
||||
});
|
||||
|
||||
// Close dialog with success indicator
|
||||
this.dialogRef.close(true);
|
||||
} catch {
|
||||
this.logService.error("[NewApplicationsDialog] Failed to save review status");
|
||||
this.toastService.showToast({
|
||||
variant: "error",
|
||||
title: this.i18nService.t("errorSavingReviewStatus"),
|
||||
message: this.i18nService.t("pleaseTryAgain"),
|
||||
});
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user