clients icon indicating copy to clipboard operation
clients copied to clipboard

[PM-6819] reactive generator ui

Open audreyality opened this issue 1 year ago • 3 comments

Type of change

- [ ] Bug fix
- [ ] New feature development
- [X] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Update password and passphrase generators to load data through reactive subscriptions.

Background

The reactive generator picks up where #8605 and #8606 left off. Those PRs attempted to integrate backwards-compatible promise-based adapters to the existing UI, but failed to load policy properly. The root cause of this issue is a behavior change in the PolicyService.

The promise interfaces blocked during synchronization. The new interfaces emit the last-known policy, and emit the latest policy once synchronization completes. Several workaround were attempted, but the reliance on promises in the UI caused each of these to fail.

It is likely that #6944 would reduce the likelihood of this bug occurring, but it is unlikely to merge in time for #8605 to utilize it.

In response to these events, this PR integrates #8605 and #8606 as a baseline. It then extends it to drive UI updates through subscriptions.

Code changes

ℹ️ The following change list omits changes documented in #8605 and #8606.

Remove page reload on sync completion. This workaround had edge cases that were rejected by QA:

  • apps/browser/src/tools/popup/generator/generator.component.ts
  • apps/web/src/app/tools/generator.component.ts
  • libs/angular/src/tools/generator/components/generator.component.ts: reads email from account service; actively subscribes to options updates
  • libs/common/src/tools/generator/legacy-password-generation.service.spec.t:s fix unit tests inconsistent with policy evaluation
  • libs/common/src/tools/generator/legacy-password-generation.service.ts: detect policy updates so that the generator automatically regenerates when policy emits
  • libs/common/src/tools/generator/password/password-generator-options.ts: communicate policy updates to the component

Introduce observable options endpoints in generator services:

  • libs/common/src/tools/generator/abstractions/password-generation.service.abstraction.ts
  • libs/common/src/tools/generator/abstractions/username-generation.service.abstraction.ts
  • libs/common/src/tools/generator/legacy-username-generation.service.ts
  • libs/common/src/tools/generator/password/password-generation.service.ts: this isn't used but is needed for compatibility with the abstraction
  • libs/common/src/tools/generator/username/username-generation.service.ts: this isn't used but is needed for compatibility with the abstraction

Use mockResolvedValue feedback from @justindbaur:

  • libs/common/src/state-migrations/migrations/61-migrate-password-settings.spec.ts
  • libs/common/src/tools/generator/legacy-username-generation.service.spec.ts

Added missing unit tests:

  • libs/common/src/tools/generator/key-definition.spec.ts

Removed obsolete documentation and code:

  • libs/common/src/tools/generator/default-generator.service.spec.ts
  • libs/common/src/tools/generator/history/legacy-password-history-decryptor.ts
  • libs/common/src/tools/generator/key-definitions.ts

audreyality avatar May 01 '24 16:05 audreyality

Codecov Report

Attention: Patch coverage is 62.32394% with 107 lines in your changes are missing coverage. Please review.

Project coverage is 28.03%. Comparing base (97c7ef3) to head (4d52a57).

Files Patch % Lines
.../tools/generator/components/generator.component.ts 0.00% 59 Missing :warning:
...ls/generator/legacy-password-generation.service.ts 82.22% 6 Missing and 2 partials :warning:
...rator/history/legacy-password-history-decryptor.ts 46.15% 7 Missing :warning:
...r/src/tools/popup/generator/generator.component.ts 0.00% 3 Missing :warning:
...nerator/history/local-generator-history.service.ts 76.92% 3 Missing :warning:
apps/browser/src/background/main.background.ts 0.00% 2 Missing :warning:
apps/desktop/src/app/tools/generator.component.ts 33.33% 0 Missing and 2 partials :warning:
apps/web/src/app/tools/generator.component.ts 33.33% 0 Missing and 2 partials :warning:
libs/angular/src/services/jslib-services.module.ts 0.00% 2 Missing :warning:
...rations/migrations/63-migrate-password-settings.ts 90.47% 0 Missing and 2 partials :warning:
... and 14 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8998      +/-   ##
==========================================
+ Coverage   27.91%   28.03%   +0.12%     
==========================================
  Files        2356     2360       +4     
  Lines       69622    69778     +156     
  Branches    13117    13125       +8     
==========================================
+ Hits        19435    19564     +129     
- Misses      48647    48666      +19     
- Partials     1540     1548       +8     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 01 '24 16:05 codecov[bot]

Logo Checkmarx One – Scan Summary & Details0e15301d-7997-4eb5-8127-bf5229513d4e

No New Or Fixed Issues Found

github-actions[bot] avatar May 01 '24 17:05 github-actions[bot]

📝 In my initial analysis, I thought I'd need to wire up an angular control group to push the reactive updates to the UI. This wasn't necessary because it turned out that I can simply update the data properties already bound to the UI from the reactive subscription.

audreyality avatar May 03 '24 16:05 audreyality