Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-6819] reactive generator ui #8998

Open
wants to merge 83 commits into
base: main
Choose a base branch
from

Conversation

audrey-jensen
Copy link
Contributor

@audrey-jensen audrey-jensen commented May 1, 2024

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

addisonbeck
addisonbeck previously approved these changes May 10, 2024
aj-rosado
aj-rosado previously approved these changes May 13, 2024
r-tome
r-tome previously approved these changes May 13, 2024
justindbaur
justindbaur previously approved these changes May 13, 2024
justindbaur
justindbaur previously approved these changes May 17, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐉 The generator component is now composed almost entirely of paperstrings and bubble tape. Nothing fancy, though. There be dragons here shaped like MacGyver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-qa Marks a PR as requiring QA approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants