-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Supporting changes for Cloud email invites #32439
Conversation
This adds a WIP impl of Teleport email invites. Requires a compatible Enterprise build and Cloud API.
* Add description to UI role resources * Expose various new react-select options * Add new FieldSelectCreatable * Add some typing for validation rules * Tweak invite button for Cloud to use email UI instead of showing both buttons * Partial implementation for onboarding invites
This adds various changes to enable showing the invite collaborators form during initial user onboarding. * Adds a `?initial` URL query parameter for the UI to signify the first user; Cloud will append this to invite appropriate invite links. * Added a new ratelimited public endpoint to return a list of preset roles. This just exposes static data available otherwise available in Git and that could be obtained from the public Teleport version shown in ping responses already.
The `kind` field can allow the UI to group errors together if several invalid emails are entered.
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.
first pass, will continue after a few questions:
-
not sure if i'm doing something wrong (or misunderstanding something), but this these changes should only affect cloud users right? (i'm running enterprise and i'm seeing these changes, i can't add a user through the web ui)
-
what if a user wants to add a non-email user?
-
anyway you can spin a cluster with all your changes so i can test it? 🙏
also there is a weird flash when rendering the dialog because of this height:
@@ -75,6 +84,72 @@ export default function FieldSelect({ | |||
); | |||
} | |||
|
|||
export function FieldSelectCreatable({ |
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'd create a new file for this, and add a story for it too
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.
& /packages/shared
is "shared" (duplicated) with Cloud. Since we just updated Cloud to have parity with Teleport I would love to see a follow up PR at the end of this work to bring this component to Cloud.
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.
Sure, I'll sync this with Cloud once it's merged.
/** | ||
* A function to validate a field value. | ||
*/ | ||
export type Rule<T, R = ValidationResult> = (value: T) => () => R; |
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.
👍
@@ -66,6 +66,9 @@ export function NewCredentials(props: NewCredentialsProps) { | |||
displayOnboardingQuestionnaire = false, | |||
setDisplayOnboardingQuestionnaire = false, | |||
Questionnaire = undefined, | |||
displayInviteCollaborators = false, | |||
setDisplayInviteCollaborators = false, |
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'd avoid naming things setXXX
unless it's a react hook state setter (that's how it's normally named setXXX
), not sure how to give rename suggestions b/c i don't really understand it yet
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, sorry, I was following the pattern from setDisplayOnboardingQuestionnaire
but I think I can just as well set it to null
.
Yes, it should only affect Cloud. I'll look into why you're seeing it on Enterprise.
The decision was that non-email users can no longer be created on Cloud. If someone wants to add one, they'll need to use
Hmm, that's a good idea, I think it'll be possible once the changes in Sales Center are deployed (hopefully soon): https://github.com/gravitational/cloud/pull/5970. |
Various TODOs needed before this can be merged:
|
@@ -75,6 +84,72 @@ export default function FieldSelect({ | |||
); | |||
} | |||
|
|||
export function FieldSelectCreatable({ |
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.
& /packages/shared
is "shared" (duplicated) with Cloud. Since we just updated Cloud to have parity with Teleport I would love to see a follow up PR at the end of this work to bring this component to Cloud.
A few non-threaded changes:
|
More notes: I think we've now got reasonable testing / storybook coverage for this PR (though I have some more work to do in the corresponding .e change). Also, with the cloud changes merged, I've now got a test cluster running this change, so DM me if you'd like an account. Additionally, per some feedback I've renamed the |
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.
thanks a lot of the changes and the cluster 👍
most of my comments are pretty minor so i'll approve this.
I think some of the feedback for below is actually being affected in enteprise:
some rule validation is kinda broken, it sometimes allows dups, sometimes it doesn't, and when it doesn't it duplicates error message
this is an error after making an api call (allows dups):
this prevents me, but the error msg's are duplciated:
nit:
probably caused by some kind of initial loading, i suggest trying out this css property: visibility. you'd probably have to lay the loader on top of this block
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.
types.tsx
-> types.ts
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.
Given defaultRule
is also not a type, I've just renamed this file to shared.tsx
. It still has LabelTip
which needs .tsx.
@@ -112,3 +113,45 @@ function SelectDark({ value, onChange, options }) { | |||
</Flex> | |||
); | |||
} | |||
|
|||
function SelectCreatableDefault() { |
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.
did you try testing this out in storybook? perhaps i'm doing something wrong but i couldn't play with it.
unless you have time to refactor this story, lets ignore the other stories in this file (it's old and kinda broken) and create a separate story just for SelectCreatable
. Ignore DarkStyledSelect
this is no longer used and we forgot to delete upon theme change.
if you intend to put more than one state in a story, i'd label it with a text eg:
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've split it into its own dedicated story and added labels. It definitely works on my machine, though in this mode (without a custom keyDown
handler like we use in InviteCollaboratorsForm
) input is a bit goofy, just due to react-select's defaults. I added some comments to explain what you might want to do to actually use the component in a real world setting.
@@ -45,6 +49,7 @@ export type Props = { | |||
onInputChange?(value: string, actionMeta: ActionMeta): void; | |||
// Whether or not the element is on an elevated platform (such as a dialog). | |||
elevated?: boolean; | |||
stylesConfig?: StylesConfig; |
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.
was this necessary? i'm guessing the use for stylesConfig
was to be able to create circular multi values. is there another use for this (that i'm not aware of).
i think the better way is to use what we have, and create a separate PR that changes this style from the root so then all users of these components renders consistent design (i suggested separate PR because it's probably going to cause mass snapshot breakage, which are easy to fix, but produces a lot of noise)
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.
It does do that, but it also lets us highlight items selectively. That's how specific invalid addresses get highlighted in red:
const recipientStyles = (theme): StylesConfig<Option> => {
return {
multiValue: (styles, { data }) => {
const { valid } = requiredEmailLike(data.label)();
const errorStyle = valid
? null
: {
borderColor: theme.colors.error.main,
borderWidth: '1px',
borderStyle: 'solid',
};
return {
...styles,
...errorStyle,
borderRadius: '1em',
};
},
};
};
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.
LGTM once Lisa's comments are addressed
I've added a new rule to ensure there's no duplicates within the entered field (we only checked against cluster users), and refactored the validation to aggregate all the various error messages so the strings aren't too verbose. I also noticed we didn't error highlight for user exists/duplicates, so added a new validator for that as well. It now looks like this (hopefully worst case): (these changes are mostly in the .e PR)
I've now set it to use |
* Add WIP implementation of Teleport email invites This adds a WIP impl of Teleport email invites. Requires a compatible Enterprise build and Cloud API. * Bump e ref and add new validation rule * Various improvements to enable Cloud email invites * Add description to UI role resources * Expose various new react-select options * Add new FieldSelectCreatable * Add some typing for validation rules * Tweak invite button for Cloud to use email UI instead of showing both buttons * Partial implementation for onboarding invites * Add support for Cloud collaborator invites during onboarding This adds various changes to enable showing the invite collaborators form during initial user onboarding. * Adds a `?initial` URL query parameter for the UI to signify the first user; Cloud will append this to invite appropriate invite links. * Added a new ratelimited public endpoint to return a list of preset roles. This just exposes static data available otherwise available in Git and that could be obtained from the public Teleport version shown in ping responses already. * Update e ref for the invite-collaborators branch * Honor the `inputId` parameter if set * bump e ref * Improve typing for `requiredEmailLike` to add a error category The `kind` field can allow the UI to group errors together if several invalid emails are entered. * bump e ref * Destructure the InviteCollaborators component sanely * Set `setDisplayInviteCollaborators` to `null` instead of `false` * Split `FieldSelectCreatable` into its own file * Fix lint * add story for SelectCreatable * Add tests for `requiredEmailLike` * Rename `initial` flag to `invite` Renaming the flag will hopefully clarify the intent. * Add tests for invite collaborators feedback and users rendering * Add rendering test for the invite collaborators card * Clean up lints * Rename types.tsx -> shared.tsx * Relocate invite constant to `Welcome/const.ts` * Split `SelectCreatable` into its own story * Clarify SelectCreatable story * Simplify story; fix lint * Fix type checker failure * Rename `preset-roles` endpoint to `presetroles` to follow API conventions
* Add WIP implementation of Teleport email invites This adds a WIP impl of Teleport email invites. Requires a compatible Enterprise build and Cloud API. * Bump e ref and add new validation rule * Various improvements to enable Cloud email invites * Add description to UI role resources * Expose various new react-select options * Add new FieldSelectCreatable * Add some typing for validation rules * Tweak invite button for Cloud to use email UI instead of showing both buttons * Partial implementation for onboarding invites * Add support for Cloud collaborator invites during onboarding This adds various changes to enable showing the invite collaborators form during initial user onboarding. * Adds a `?initial` URL query parameter for the UI to signify the first user; Cloud will append this to invite appropriate invite links. * Added a new ratelimited public endpoint to return a list of preset roles. This just exposes static data available otherwise available in Git and that could be obtained from the public Teleport version shown in ping responses already. * Update e ref for the invite-collaborators branch * Honor the `inputId` parameter if set * bump e ref * Improve typing for `requiredEmailLike` to add a error category The `kind` field can allow the UI to group errors together if several invalid emails are entered. * bump e ref * Destructure the InviteCollaborators component sanely * Set `setDisplayInviteCollaborators` to `null` instead of `false` * Split `FieldSelectCreatable` into its own file * Fix lint * add story for SelectCreatable * Add tests for `requiredEmailLike` * Rename `initial` flag to `invite` Renaming the flag will hopefully clarify the intent. * Add tests for invite collaborators feedback and users rendering * Add rendering test for the invite collaborators card * Clean up lints * Rename types.tsx -> shared.tsx * Relocate invite constant to `Welcome/const.ts` * Split `SelectCreatable` into its own story * Clarify SelectCreatable story * Simplify story; fix lint * Fix type checker failure * Rename `preset-roles` endpoint to `presetroles` to follow API conventions
Backport of #32439 for branch/v13 --- * Add WIP implementation of Teleport email invites This adds a WIP impl of Teleport email invites. Requires a compatible Enterprise build and Cloud API. * Bump e ref and add new validation rule * Various improvements to enable Cloud email invites * Add description to UI role resources * Expose various new react-select options * Add new FieldSelectCreatable * Add some typing for validation rules * Tweak invite button for Cloud to use email UI instead of showing both buttons * Partial implementation for onboarding invites * Add support for Cloud collaborator invites during onboarding This adds various changes to enable showing the invite collaborators form during initial user onboarding. * Adds a `?initial` URL query parameter for the UI to signify the first user; Cloud will append this to invite appropriate invite links. * Added a new ratelimited public endpoint to return a list of preset roles. This just exposes static data available otherwise available in Git and that could be obtained from the public Teleport version shown in ping responses already. * Update e ref for the invite-collaborators branch * Honor the `inputId` parameter if set * bump e ref * Improve typing for `requiredEmailLike` to add a error category The `kind` field can allow the UI to group errors together if several invalid emails are entered. * bump e ref * Destructure the InviteCollaborators component sanely * Set `setDisplayInviteCollaborators` to `null` instead of `false` * Split `FieldSelectCreatable` into its own file * Fix lint * add story for SelectCreatable * Add tests for `requiredEmailLike` * Rename `initial` flag to `invite` Renaming the flag will hopefully clarify the intent. * Add tests for invite collaborators feedback and users rendering * Add rendering test for the invite collaborators card * Clean up lints * Rename types.tsx -> shared.tsx * Relocate invite constant to `Welcome/const.ts` * Split `SelectCreatable` into its own story * Clarify SelectCreatable story * Simplify story; fix lint * Fix type checker failure * Rename `preset-roles` endpoint to `presetroles` to follow API conventions
* Add WIP implementation of Teleport email invites This adds a WIP impl of Teleport email invites. Requires a compatible Enterprise build and Cloud API. * Bump e ref and add new validation rule * Various improvements to enable Cloud email invites * Add description to UI role resources * Expose various new react-select options * Add new FieldSelectCreatable * Add some typing for validation rules * Tweak invite button for Cloud to use email UI instead of showing both buttons * Partial implementation for onboarding invites * Add support for Cloud collaborator invites during onboarding This adds various changes to enable showing the invite collaborators form during initial user onboarding. * Adds a `?initial` URL query parameter for the UI to signify the first user; Cloud will append this to invite appropriate invite links. * Added a new ratelimited public endpoint to return a list of preset roles. This just exposes static data available otherwise available in Git and that could be obtained from the public Teleport version shown in ping responses already. * Update e ref for the invite-collaborators branch * Honor the `inputId` parameter if set * bump e ref * Improve typing for `requiredEmailLike` to add a error category The `kind` field can allow the UI to group errors together if several invalid emails are entered. * bump e ref * Destructure the InviteCollaborators component sanely * Set `setDisplayInviteCollaborators` to `null` instead of `false` * Split `FieldSelectCreatable` into its own file * Fix lint * add story for SelectCreatable * Add tests for `requiredEmailLike` * Rename `initial` flag to `invite` Renaming the flag will hopefully clarify the intent. * Add tests for invite collaborators feedback and users rendering * Add rendering test for the invite collaborators card * Clean up lints * Rename types.tsx -> shared.tsx * Relocate invite constant to `Welcome/const.ts` * Split `SelectCreatable` into its own story * Clarify SelectCreatable story * Simplify story; fix lint * Fix type checker failure * Rename `preset-roles` endpoint to `presetroles` to follow API conventions
* [13] Supporting changes for Cloud email invites (#32439) Backport of #32439 for branch/v13 --- * Add WIP implementation of Teleport email invites This adds a WIP impl of Teleport email invites. Requires a compatible Enterprise build and Cloud API. * Bump e ref and add new validation rule * Various improvements to enable Cloud email invites * Add description to UI role resources * Expose various new react-select options * Add new FieldSelectCreatable * Add some typing for validation rules * Tweak invite button for Cloud to use email UI instead of showing both buttons * Partial implementation for onboarding invites * Add support for Cloud collaborator invites during onboarding This adds various changes to enable showing the invite collaborators form during initial user onboarding. * Adds a `?initial` URL query parameter for the UI to signify the first user; Cloud will append this to invite appropriate invite links. * Added a new ratelimited public endpoint to return a list of preset roles. This just exposes static data available otherwise available in Git and that could be obtained from the public Teleport version shown in ping responses already. * Update e ref for the invite-collaborators branch * Honor the `inputId` parameter if set * bump e ref * Improve typing for `requiredEmailLike` to add a error category The `kind` field can allow the UI to group errors together if several invalid emails are entered. * bump e ref * Destructure the InviteCollaborators component sanely * Set `setDisplayInviteCollaborators` to `null` instead of `false` * Split `FieldSelectCreatable` into its own file * Fix lint * add story for SelectCreatable * Add tests for `requiredEmailLike` * Rename `initial` flag to `invite` Renaming the flag will hopefully clarify the intent. * Add tests for invite collaborators feedback and users rendering * Add rendering test for the invite collaborators card * Clean up lints * Rename types.tsx -> shared.tsx * Relocate invite constant to `Welcome/const.ts` * Split `SelectCreatable` into its own story * Clarify SelectCreatable story * Simplify story; fix lint * Fix type checker failure * Rename `preset-roles` endpoint to `presetroles` to follow API conventions * Small compat fix for v13 due to lack of `WithUnauthenticatedHighLimiter`
This adds a number of changes to support email user invites for Cloud.
/preset-roles
endpoint that just returns the preset roles for the current version of Teleport. It's unauthenticated and intended for use in the new onboarding invitation UI. I don't think this has security implications beyond what we already return in theping
replies, since those include the Teleport version.react-select
wrapper, and adds a Creatable variant for email entry.requiresEmailLike
validation rule, and adds more typing to the rulesNote that gravitational/teleport.e#2286 is needed to test the changes. This PR must be merged before the
teleport.e
PR, then another PR here should bump the .e ref.See also: