Skip to content
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

🪟 🎉 Add optional invite user hint to connector create pages #15799

Merged
merged 17 commits into from
Oct 12, 2022

Conversation

edmundito
Copy link
Contributor

@edmundito edmundito commented Aug 19, 2022

What

Closes #15660

Adds the ability to invite users from the source or destinations create pages when running in cloud mode. It can be enabled from experiment flags and provides the option to show as a link to the page or a direct modal.

Screen Shot 2022-09-08 at 16 12 31
Screen Shot 2022-09-08 at 16 12 40
Screen Shot 2022-09-08 at 16 13 34

Screen Shot 2022-09-08 at 16 14 04

Screen Shot 2022-09-08 at 16 13 41

How

Updates the invite user modal to be invoked from a service.
The service is then used in invite hint component to invoke the modal.
The component is lazy loaded since it's only needed when running in cloud.

Recommended reading order

  1. airbyte-webapp/src/packages/cloud/views/users/InviteUsersHint/InviteUsersHint.tsx
  2. airbyte-webapp/src/components/CloudInviteUsersHint/OptionalInviteUsersHint.tsx
  3. airbyte-webapp/src/pages/SourcePage/pages/CreateSourcePage/CreateSourcePage.tsx
  4. airbyte-webapp/src/pages/DestinationPage/pages/CreateDestinationPage/CreateDestinationPage.tsx
  5. Rest

Tests

TODO

@edmundito edmundito requested a review from a team as a code owner August 19, 2022 18:15
@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Aug 19, 2022
@krishnaglick
Copy link
Contributor

Maybe add [WIP] to the title?

@timroes timroes marked this pull request as draft August 26, 2022 14:31
@edmundito edmundito force-pushed the edmundito/invite-user-from-connector branch from c7fc8e5 to 0975ec5 Compare September 8, 2022 20:01
@edmundito edmundito marked this pull request as ready for review September 8, 2022 20:15
@edmundito edmundito force-pushed the edmundito/invite-user-from-connector branch from 111f796 to 547a909 Compare September 9, 2022 17:41
@edmundito edmundito force-pushed the edmundito/invite-user-from-connector branch 2 times, most recently from 44fc8fd to 449a769 Compare September 19, 2022 14:12
@edmundito edmundito force-pushed the edmundito/invite-user-from-connector branch 2 times, most recently from bc2fa98 to 1ca13bf Compare September 28, 2022 14:39
@edmundito edmundito force-pushed the edmundito/invite-user-from-connector branch from 1ca13bf to fb1c179 Compare October 6, 2022 10:18
@edmundito
Copy link
Contributor Author

Rebased and ready for review!

Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not tested locally

Comment on lines 28 to 30
toggleInviteUsersModalOpen: (open) => {
toggleIsOpen(open);
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just export the setState function instead of wrapping it?

const mockUseExperiment = jest.fn<ReturnType<typeof useExperiment>, Parameters<typeof useExperiment>>();
jest.doMock("hooks/services/Experiment", () => ({
useExperiment: mockUseExperiment,
}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found a different pattern for this. You might find it a little nicer, but up to you!

Comment on lines +32 to +36
onClick={() => {
toggleInviteUsersModalOpen();
}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
onClick={() => {
toggleInviteUsersModalOpen();
}}
onClick={toggleInviteUsersModalOpen}

Copy link
Contributor

@josephkmh josephkmh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, works well! Just left a few questions/stylistic comments. Only thing missing seems to be the launch darkly feature flags, unless I missed something.

@edmundito edmundito force-pushed the edmundito/invite-user-from-connector branch from 8edb341 to 28fd5f3 Compare October 12, 2022 14:23
@edmundito edmundito requested review from josephkmh and a team and removed request for ambirdsall October 12, 2022 14:23
Copy link
Contributor

@josephkmh josephkmh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks good to me!

@edmundito edmundito merged commit 1ddd4d7 into master Oct 12, 2022
@edmundito edmundito deleted the edmundito/invite-user-from-connector branch October 12, 2022 15:33
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
…q#15799)

* Add InviteUsersModalService and migrate UserSettingsView to use it

* Add InviteUsersHint component and show in create source/destination pages

* Update InviteUsersHint to use Text component and spacing variables

* Add experiments for showing invite user hint
Move cloud settings paths to its own file

* Update Invite hint to lazy load with suspense

* Fix invite user modal experiment names and add unit test

* Rename CloudInviteUsersHint component file

* Add notification when users are invited

* Fix copy and remove plural form of invite success dialog

* Show invite users hint in connector create form page

* Fix stylelint issue in InviteUsersHint

* Fix access management path in InviteUsersHint

* Fix button text in UserSettingsView

* Fix linkToUsersPage type in experiments iface

* Cleanup code

* Cleanup scss path in InviteUsersHint.module

* update InviteUsersHint layout to be consistent with or without button
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add user invite possibility in connector form
3 participants