-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat: [WD-16972] TLS user management spike. #1026
base: main
Are you sure you want to change the base?
Conversation
be0eaaf
to
338503b
Compare
338503b
to
73db4b1
Compare
ee9df96
to
526c441
Compare
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.
Made some comments, I agree with @edlerd that this PR can actually be merged after some improvements. Although I'm also happy to not merge it and keep the PR as a reference for future, depending on if you have time to take it to completion
526c441
to
e5f5f61
Compare
e5f5f61
to
9c5f34b
Compare
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 already works well. Some ideas below to streamline code and appearance.
<Input | ||
id="clientName" | ||
type="text" | ||
label="Client Name" | ||
onBlur={formik.handleBlur} | ||
onChange={formik.handleChange} | ||
value={formik.values.clientName} | ||
/> |
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.
We are creating an identity, this should be called "Identity name", where does the "client name" idea come from?
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 is the terminology that the API and documentation uses. Yes, when the identity surfaces, it appears as the "Identity name", but I believe there is more to it than just the name and that staying true to the API may avoid potential issues in the future if the API changes against our mental model of "Identity 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.
Right good points. I see why the docs are doing it. They are tailoring towards a new command line client. I think in the UI, the target audience is a bit different. A user might create the token for a new client, but it could also be for another person (think an admin creating the token for a colleague). In the API layer we use, this is just "name". I agree, a "Name" label would be confusing here when creating a token. The entity that is created is referred to as "Identity" in the UI. So maybe "Identity name" is the better description to cover all target use cases. Wdyt?
9c5f34b
to
c5fdbcd
Compare
Signed-off-by: Nkeiruka <[email protected]>
c5fdbcd
to
67c3621
Compare
Done
User Management Spike
[✔️] A user needs to be able to create a TLS user (Creates a pending user)
[✔️] A user needs to be able to destroy a TLS user (WD-16894)
[] A user needs to be able to rename a TLS user
[] A user needs to be able to change a TLS user’s publicity
[Out of Scope] A user needs the UI to be able to generate the public/private key pair for them
[Out of Scope] A user needs to be able to use an existing public key that they already have
[Out of Scope] Once a user has a private key generated by the UI, they need to know how to install it on their client machine.
Fixes:
QA
Screenshots