[PM-25611][PM-25612] Update components to use persistance code #16655
Conversation
… classes that properly handle encstring with placeholders for upcoming usage
|
Great job! No new security vulnerabilities introduced in this pull request |
…reportResult summary
… when critical applications change
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16655 +/- ##
==========================================
+ Coverage 38.76% 38.78% +0.01%
==========================================
Files 3406 3407 +1
Lines 96754 96741 -13
Branches 14526 14540 +14
==========================================
+ Hits 37511 37524 +13
+ Misses 57601 57573 -28
- Partials 1642 1644 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Banrion
requested review from
prograhamming
and removed request for
prograhamming
AlexRubik
left a comment
AlexRubik
left a comment
There was a problem hiding this comment.
Sorry, didn't submit these when I initially made them. They show up on my end but I guess you don't see it until I submit review. These are just code quality changes, not blocking. Not requesting changes so you can still merge if needed.
| constructor(private dataService: RiskInsightsDataService) { | ||
| // All application summary changes | ||
| this.dataService.reportResults$.subscribe((report) => { | ||
| if (report) { | ||
| this.setAllAppsReportSummary(report.summaryData); | ||
| } | ||
| }); | ||
| // Critical application summary changes | ||
| this.dataService.criticalReportResults$.subscribe((report) => { | ||
| if (report) { | ||
| this.setCriticalAppsReportSummary(report.summaryData); | ||
| } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Clean up subscriptions to follow best practice/prevent memory leaks.
| constructor(private dataService: RiskInsightsDataService) { | |
| // All application summary changes | |
| this.dataService.reportResults$.subscribe((report) => { | |
| if (report) { | |
| this.setAllAppsReportSummary(report.summaryData); | |
| } | |
| }); | |
| // Critical application summary changes | |
| this.dataService.criticalReportResults$.subscribe((report) => { | |
| if (report) { | |
| this.setCriticalAppsReportSummary(report.summaryData); | |
| } | |
| }); | |
| } | |
| private destroyRef = inject(DestroyRef); | |
| constructor(private dataService: RiskInsightsDataService) { | |
| this.dataService.reportResults$ | |
| .pipe(takeUntilDestroyed(this.destroyRef)) | |
| .subscribe((report) => { | |
| if (report) { | |
| this.setAllAppsReportSummary(report.summaryData); | |
| } | |
| }); | |
| this.dataService.criticalReportResults$ | |
| .pipe(takeUntilDestroyed(this.destroyRef)) | |
| .subscribe((report) => { | |
| if (report) { | |
| this.setCriticalAppsReportSummary(report.summaryData); | |
| } | |
| }); |
There was a problem hiding this comment.
Unnecessary in the service not provided in 'root' but provided at the component level. Automatic cleanup is handled at the component level
| this.criticalAppsService.criticalAppsList$ | ||
| .pipe(withLatestFrom(this.organizationDetails$, this.userId$)) | ||
| .subscribe({ | ||
| next: ([_criticalApps, organizationDetails, userId]) => { | ||
| if (organizationDetails?.organizationId && userId) { | ||
| this.fetchLastReport(organizationDetails?.organizationId, userId); | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Sub cleanup
| this.criticalAppsService.criticalAppsList$ | |
| .pipe(withLatestFrom(this.organizationDetails$, this.userId$)) | |
| .subscribe({ | |
| next: ([_criticalApps, organizationDetails, userId]) => { | |
| if (organizationDetails?.organizationId && userId) { | |
| this.fetchLastReport(organizationDetails?.organizationId, userId); | |
| } | |
| }, | |
| }); | |
| export class RiskInsightsDataService { | |
| private destroyRef = inject(DestroyRef); // Add this line | |
| // ... existing properties ... | |
| constructor( | |
| private accountService: AccountService, | |
| private criticalAppsService: CriticalAppsService, | |
| private organizationService: OrganizationService, | |
| private reportService: RiskInsightsReportService, | |
| ) { | |
| this.criticalAppsService.criticalAppsList$ | |
| .pipe( | |
| withLatestFrom(this.organizationDetails$, this.userId$), | |
| takeUntilDestroyed(this.destroyRef) // Add this line | |
| ) | |
| .subscribe({ | |
| next: ([_criticalApps, organizationDetails, userId]) => { | |
| if (organizationDetails?.organizationId && userId) { | |
| this.fetchLastReport(organizationDetails?.organizationId, userId); | |
| } | |
| }, | |
| }); |
There was a problem hiding this comment.
Unnecessary in the service not provided in 'root' but provided at the component level. Automatic cleanup is handled at the component level
…e-client-duplication' into dirt/pm-25611/remove-client-duplication
AlexRubik
left a comment
AlexRubik
left a comment
There was a problem hiding this comment.
Good, correct displays before and after first report run
|
There was a problem hiding this comment.
Confirmed this fix grabs the org id from the route params. Also, request password change button gives success toast while running local backend server. Fix password change on critical applications
🎟️ Tracking
PM-25611
PM-25612
📔 Objective
This pull request is one of many to add and hookup logic for Risk Insights Persistence of Reports (epic). The following changes have been applied.
Focused changed: Use updated logic added throughout the epic inside of the component for a full client to server loading of reports and report data. Data service is to be used as an orchestrator between components rather than a graph dependency structure.
Changes:
Minor Improvements:
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes