fix(ssh-keys-sidebar): SSH keys sidebar loads invalid key if there ar… #653
Conversation
…e duplicate names for different accounts
| @@ -40,7 +40,7 @@ export class SshKeyListComponent { | |||
| public selectSshKeyPair(sshKeyPair: SSHKeyPair): void { | |||
| this.router.navigate(['view', sshKeyPair.name], { | |||
| relativeTo: this.route, | |||
| queryParamsHandling: 'preserve' | |||
| queryParams: {account: sshKeyPair.account} | |||
There was a problem hiding this comment.
Please, add queryParamsHandling: 'preserve'
There was a problem hiding this comment.
if queryParams and queryParamsHandling: 'preserve' are used together, the query parameters are not adding to the routerLink
There was a problem hiding this comment.
@HeyRoach , is there any other way to pass params to route with queryParamsHandling: 'preserve'?
| } | ||
|
|
||
| public getByName(name: string): Observable<SSHKeyPair> { | ||
| return this.getList({ name }).map(sshKeys => sshKeys[0]); | ||
| return this.getList({ name }).map(sshKeys => { |
There was a problem hiding this comment.
is it possible to fetch only one key by account and name? there will be no need to do subscription in constructor and store it locally
There was a problem hiding this comment.
You also have to handle the situation when account is not present in the query string
| } | ||
|
|
||
| public getByName(name: string): Observable<SSHKeyPair> { | ||
| return this.getList({ name }).map(sshKeys => sshKeys[0]); | ||
| return this.getList({ name }).map(sshKeys => { |
There was a problem hiding this comment.
You also have to handle the situation when account is not present in the query string
…e duplicate names for different accounts - review fixes
| @@ -40,7 +40,7 @@ export class SshKeyListComponent { | |||
| public selectSshKeyPair(sshKeyPair: SSHKeyPair): void { | |||
| this.router.navigate(['view', sshKeyPair.name], { | |||
| relativeTo: this.route, | |||
| queryParamsHandling: 'preserve' | |||
| queryParams: {account: sshKeyPair.account} | |||
There was a problem hiding this comment.
@HeyRoach , is there any other way to pass params to route with queryParamsHandling: 'preserve'?
| } | ||
|
|
||
| public getByName(name: string): Observable<SSHKeyPair> { | ||
| return this.getList({ name }).map(sshKeys => sshKeys[0]); | ||
| public getByName(name: string, account?: string): Observable<SSHKeyPair> { |
There was a problem hiding this comment.
refactor this method to accept params object, rename the method
| private asyncJobService: AsyncJobService, | ||
| protected http: HttpClient | ||
| ) { | ||
| protected account: string; |
There was a problem hiding this comment.
why do you need to store the account? service should not store any data
| super(http); | ||
|
|
||
| this.activatedRoute.queryParams.subscribe((params: Params) => { |
| super(entityService, notificationService, route, router); | ||
| } | ||
|
|
||
| public ngOnInit() { | ||
| this.getEntityAccount(); |
There was a problem hiding this comment.
remove this, use RXJS power not to store the data in the component
| public onDescriptionChange(description: string): void { | ||
| this.description = description; | ||
| this.userTagService.setSshKeyDescription(this.entity, this.description).subscribe(); | ||
| } | ||
|
|
||
| protected loadEntity(name: string): Observable<SSHKeyPair> { | ||
| return this.entityService.getByName(name) | ||
| protected loadEntity(name: string, account?: string): Observable<SSHKeyPair> { |
There was a problem hiding this comment.
can we leave this method with a single parameter and fetch account from stateParams here?
HeyRoach
force-pushed
the
651-ssh-sidebar-load-invalid-key
branch
2 times, most recently
from
2b74a78 to
feff524
Compare
…e duplicate names for different accounts - review fixes 1
HeyRoach
force-pushed
the
651-ssh-sidebar-load-invalid-key
branch
from
feff524 to
9110eec
Compare
| @@ -1,5 +1,6 @@ | |||
| import { HttpClient } from '@angular/common/http'; | |||
| import { Injectable } from '@angular/core'; | |||
| import { ActivatedRoute } from '@angular/router'; | |||
| private asyncJobService: AsyncJobService, | ||
| protected http: HttpClient | ||
| ) { | ||
| constructor(private asyncJobService: AsyncJobService, |
| protected router: Router, | ||
| protected userTagService: UserTagService | ||
| ) { | ||
| constructor(protected entityService: SSHKeyPairService, |
There was a problem hiding this comment.
use correct styling for constructor
| public description: string; | ||
| public account: string; |
| @@ -32,7 +31,10 @@ export class SshKeySidebarComponent extends SidebarComponent<SSHKeyPair> impleme | |||
| } | |||
|
|
|||
| protected loadEntity(name: string): Observable<SSHKeyPair> { | |||
| return this.entityService.getByName(name) | |||
| const account = this.route.snapshot.queryParams['account']; | |||
There was a problem hiding this comment.
do not use snapshot, use switchMap with this.entityService.getByParams
…e duplicate names for different accounts - review fixes 2
…e duplicate names for different accounts