[EC-775] [Technical Dependency] Refactor Vault Filters to be routable #4733
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| import { Injectable } from "@angular/core"; | ||
| import { Router } from "@angular/router"; | ||
| import { combineLatest, map, Observable } from "rxjs"; | ||
|
|
||
| import { ITreeNodeObject, TreeNode } from "@bitwarden/common/models/domain/tree-node"; | ||
|
|
||
| import { RoutedVaultFilterBridge } from "../shared/models/routed-vault-filter-bridge.model"; | ||
| import { RoutedVaultFilterModel, Unassigned } from "../shared/models/routed-vault-filter.model"; | ||
| import { VaultFilter } from "../shared/models/vault-filter.model"; | ||
| import { CipherTypeFilter } from "../shared/models/vault-filter.type"; | ||
|
|
||
| import { VaultFilterService } from "./abstractions/vault-filter.service"; | ||
| import { RoutedVaultFilterService } from "./routed-vault-filter.service"; | ||
|
|
||
| @Injectable() | ||
| export class RoutedVaultFilterBridgeService { | ||
|
Member
There was a problem hiding this comment. suggestion: you could add a jsdoc comment here explaining the purpose of the bridge service
Member
There was a problem hiding this comment. Something I'm wondering about 😁 is it transitional? If so, do we need a tech debt ticket to remove the old services? I assume that when we build the new filter list in the left-hand nav, they'll be routing directly to the query URL rather than going via a service, and then it'll be much simpler to clean up the old code. This feels like something EC pod should do as part of VVR, before handing off to Vault team. Note that we also have EDIT: ok, after reviewing the rest of the code, (I think) I understand that this is basically a wrapper around existing logic to implement routing with a minimum of code changes. The downside is that it adds complexity in the short term, but I'm OK with this if we're deprecating it quickly after VVR (because the new components won't use it).
Contributor
Author
There was a problem hiding this comment. @jlf0dev Sure I can add the file descriptions I've used in the PR description field into the code! @eliykat Yes exactly, this is a transitional class. I'm glad that it becomes apparent when reading the code, means it's not too complex/confusing. As I mention in the Objective part of the PR description:
You have a good point, maybe we should add a TD ticket, but really all of this should be removed as part of VVR
Contributor
Author
There was a problem hiding this comment. Instead of a TD ticket I added a task to the VVR story instead EC-208 [Admin Console] Add Filters under Left Nav
Member
There was a problem hiding this comment. Looks good, and serves me right for not reading your PR description properly 🙈 |
||
| readonly activeFilter$: Observable<VaultFilter>; | ||
|
|
||
| constructor( | ||
| private router: Router, | ||
| private routedVaultFilterService: RoutedVaultFilterService, | ||
| legacyVaultFilterService: VaultFilterService | ||
| ) { | ||
| this.activeFilter$ = combineLatest([ | ||
| routedVaultFilterService.filter$, | ||
| legacyVaultFilterService.collectionTree$, | ||
| legacyVaultFilterService.folderTree$, | ||
| legacyVaultFilterService.organizationTree$, | ||
| legacyVaultFilterService.cipherTypeTree$, | ||
| ]).pipe( | ||
| map(([filter, collectionTree, folderTree, organizationTree, cipherTypeTree]) => { | ||
| const legacyFilter = new VaultFilter(); | ||
|
|
||
| if (filter.collectionId !== undefined && filter.collectionId === Unassigned) { | ||
| legacyFilter.selectedCollectionNode = this.findNode(collectionTree, null); | ||
| } | ||
|
|
||
| if (filter.collectionId !== undefined && filter.collectionId !== Unassigned) { | ||
| legacyFilter.selectedCollectionNode = this.findNode(collectionTree, filter.collectionId); | ||
| } | ||
|
|
||
| if (filter.folderId !== undefined && filter.folderId === Unassigned) { | ||
| legacyFilter.selectedFolderNode = this.findNode(folderTree, null); | ||
| } | ||
|
|
||
| if (filter.folderId !== undefined && filter.folderId !== Unassigned) { | ||
| legacyFilter.selectedFolderNode = this.findNode(folderTree, filter.folderId); | ||
| } | ||
|
|
||
| if (filter.organizationId !== undefined) { | ||
| legacyFilter.selectedOrganizationNode = this.findNode( | ||
| organizationTree, | ||
| filter.organizationId | ||
| ); | ||
| } | ||
|
|
||
| if (filter.type !== undefined && filter.type === "trash") { | ||
| legacyFilter.selectedCipherTypeNode = { | ||
| node: { id: "trash", name: "", icon: "", type: "trash" }, | ||
|
Member
There was a problem hiding this comment. minor suggestion: you could use the legacyFilter.selectedCipherTypeNode = new TreeNode<CipherTypeFilter>({ id: "trash", name: "", type: "trash", icon: ""}, null);we really should refactor this constructor to be more useful, I don't even think this will save you that many characters 😄
Member
There was a problem hiding this comment. My question was going to be whether we could find the existing Trash node, or export & import the Trash node object, rather than recreating it. It looks like it's currently declared in the In any case I agree you should use a real
Contributor
Author
Member
There was a problem hiding this comment. lgtm but I'll let @jlf0dev resolve the thread when he's happy |
||
| } as unknown as TreeNode<CipherTypeFilter>; | ||
| } | ||
|
|
||
| if (filter.type !== undefined && filter.type !== "trash") { | ||
| legacyFilter.selectedCipherTypeNode = this.findNode(cipherTypeTree, filter.type); | ||
| } | ||
|
|
||
| const bridgeModel = new RoutedVaultFilterBridge(filter, legacyFilter, this); | ||
|
|
||
| return bridgeModel; | ||
|
Member
There was a problem hiding this comment. suggestion: unnecessary variable here, just return the model
Contributor
Author
There was a problem hiding this comment. Huh, I wonder why I did this. Must've been a |
||
| }) | ||
| ); | ||
| } | ||
|
|
||
| navigate(filter: RoutedVaultFilterModel) { | ||
| const route = this.routedVaultFilterService.createRoute(filter); | ||
| this.router.navigate(route.commands, route.extras); | ||
| } | ||
|
|
||
| private findNode<T extends ITreeNodeObject>( | ||
|
eliykat marked this conversation as resolved.
Outdated
|
||
| node: TreeNode<T>, | ||
| id: string | ||
| ): TreeNode<T> | undefined { | ||
| if (node.node.id === id) { | ||
|
Member
There was a problem hiding this comment. comment: we need to rename the
Member
There was a problem hiding this comment. There's probably some background discussion here that I'm not familiar with, but -
Contributor
Author
There was a problem hiding this comment. Haha yeah, hopefully we'll get rid of the TreeNode completely and not have to worry about it :D |
||
| return node; | ||
| } | ||
|
|
||
| for (const child of node.children) { | ||
| const result = this.findNode(child, id); | ||
| if (result !== undefined) { | ||
| return result; | ||
| } | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import { Injectable, OnDestroy } from "@angular/core"; | ||
| import { ActivatedRoute, NavigationExtras } from "@angular/router"; | ||
| import { combineLatest, map, Observable, Subject, takeUntil } from "rxjs"; | ||
|
|
||
| import { RoutedVaultFilterModel } from "../shared/models/routed-vault-filter.model"; | ||
|
|
||
| @Injectable() | ||
| export class RoutedVaultFilterService implements OnDestroy { | ||
|
Member
There was a problem hiding this comment. suggestion: a comment here might be helpful as well. The name |
||
| private onDestroy = new Subject<void>(); | ||
|
|
||
| filter$: Observable<RoutedVaultFilterModel>; | ||
|
|
||
| constructor(activatedRoute: ActivatedRoute) { | ||
| this.filter$ = combineLatest([activatedRoute.paramMap, activatedRoute.queryParamMap]).pipe( | ||
| map(([params, queryParams]) => { | ||
| return { | ||
| collectionId: queryParams.get("collectionId") ?? undefined, | ||
| folderId: queryParams.get("folderId") ?? undefined, | ||
| organizationId: | ||
| params.get("organizationId") ?? queryParams.get("organizationId") ?? undefined, | ||
| organizationIdParamType: | ||
| params.get("organizationId") != undefined ? ("path" as const) : ("query" as const), | ||
| type: queryParams.get("type") ?? undefined, | ||
| }; | ||
| }), | ||
| takeUntil(this.onDestroy) | ||
| ); | ||
| } | ||
|
|
||
| createRoute(filter: RoutedVaultFilterModel): { commands: any[]; extras?: NavigationExtras } { | ||
| return { | ||
| commands: [], | ||
| extras: { | ||
| queryParams: { | ||
| collectionId: filter.collectionId ?? null, | ||
|
eliykat marked this conversation as resolved.
Outdated
|
||
| folderId: filter.folderId ?? null, | ||
| organizationId: | ||
| filter.organizationIdParamType === "path" ? null : filter.organizationId ?? null, | ||
| type: filter.type ?? null, | ||
| }, | ||
| queryParamsHandling: "merge", | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| ngOnDestroy(): void { | ||
| this.onDestroy.next(); | ||
| this.onDestroy.complete(); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.