-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Filter Auth methods by name or type #20747
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed I needed to clean this up from my last test writing for the secret engine filtering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good but I'm interested in the sorting by type and name logic. If I'm off base with my thinking around that then I'll go ahead and approve.
get authMethodList() { | ||
// return an options list to filter by engine type, ex: 'kv' | ||
if (this.selectedAuthType) { | ||
// check first if the user has also filtered by name. | ||
if (this.selectedAuthName) { | ||
return this.model.filter((method) => this.selectedAuthName === method.id); | ||
} | ||
// otherwise filter by auth type | ||
return this.model.filter((method) => this.selectedAuthType === method.type); | ||
} | ||
// return an options list to filter by auth name, ex: 'my-userpass' | ||
if (this.selectedAuthName) { | ||
return this.model.filter((method) => this.selectedAuthName === method.id); | ||
} | ||
// no filters, return full sorted list. | ||
return this.model; | ||
} | ||
|
||
get authMethodArrayByType() { | ||
const arrayOfAllAuthTypes = this.authMethodList.map((modelObject) => modelObject.type); | ||
// filter out repeated auth types (e.g. [userpass, userpass] => [userpass]) | ||
const arrayOfUniqueAuthTypes = [...new Set(arrayOfAllAuthTypes)]; | ||
|
||
return arrayOfUniqueAuthTypes.map((authType) => ({ | ||
name: authType, | ||
id: authType, | ||
})); | ||
} | ||
|
||
get authMethodArrayByName() { | ||
return this.authMethodList.map((modelObject) => ({ | ||
name: modelObject.id, | ||
id: modelObject.id, | ||
})); | ||
} | ||
|
||
@action | ||
filterAuthType([type]) { | ||
this.selectedAuthType = type; | ||
} | ||
|
||
@action | ||
filterAuthName([name]) { | ||
this.selectedAuthName = name; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker but it seems like some of this filtering logic could be moved to a decorator or utility so that it can be shared since this is the second instance of this functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thought. The only issue is that the sorted list that is passed around is pretty unique. For KV we need to combine and filter two lists (supported and unsupported) and here we're just using the this.model and filtering there. With KV the unique list situation meant a couple of extra lines of code not seen here. But it's a great consideration. I'll think about this with the KV work as the new models may allow us to standardize the list.
if (this.selectedAuthType) { | ||
// check first if the user has also filtered by name. | ||
if (this.selectedAuthName) { | ||
return this.model.filter((method) => this.selectedAuthName === method.id); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious if we should be filtering by both type and name if they are both set? I think there could be a case where the same name is used for different types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For secret engines you can't have the same name as used by another secret engine. We do some validation work on that and say as much. And if they passed validation, then the API would return the same error.
But great question for auth methods. I had to check if this was the same. Turns out it is. We don't do any validation but we do return the API error.
So names are individualized across all auth methods. Which I believes answers your question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok good to know thanks! Maybe nice to add a comment in there stating as much so it's clear why type is ignored if name is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't help myself. I opened the file and had to glimmerize.
remove spaces
Pulled from the previous work done here. This adds the same functionality to the authentication list view.
Inspired by this GH Issue.
Movie of the filtering in action:
Screen.Recording.2023-05-24.at.12.37.36.PM.mov