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

7417 workflows i can send emails using the email account #7431

Merged

Conversation

martmull
Copy link
Contributor

@martmull martmull commented Oct 4, 2024

  • update send-email.workflow-action.ts so it send email via the google sdk
  • remove useless workflow-action.email.ts
  • add send authorization to google api scopes
  • update the front workflow email step form to provide a connectedAccountId from the available connected accounts
  • update the permissions of connected accounts: ask users to reconnect when selecting missing send permission

image

@martmull martmull linked an issue Oct 4, 2024 that may be closed by this pull request
@@ -23,6 +23,7 @@ export class GoogleAPIsOauthCommonStrategy extends PassportStrategy(
'email',
'profile',
'https://www.googleapis.com/auth/gmail.readonly',
'https://www.googleapis.com/auth/gmail.send',
Copy link
Member

Choose a reason for hiding this comment

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

we will need to put that behind a feature flag as we will need to go through the google approval process and it can take up to a month. We will be able to test with a short list of accounts in the meantime and will need the flag :)

Also it's likely that we will need to have a process to ask users to reconnect their account if we detect that the scope is missing (maybe when we try to send an email and get a 403 the first time) We already have similar mechanism for messaging sync

@martmull martmull force-pushed the 7417-workflows-i-can-send-emails-using-the-email-account branch 3 times, most recently from e5101a4 to 0409f15 Compare October 7, 2024 13:58
@martmull martmull force-pushed the 7417-workflows-i-can-send-emails-using-the-email-account branch from 0409f15 to 5bfb2ff Compare October 7, 2024 14:00
@martmull martmull force-pushed the 7417-workflows-i-can-send-emails-using-the-email-account branch 3 times, most recently from 5b3428b to 3b76753 Compare October 8, 2024 08:10
@martmull martmull force-pushed the 7417-workflows-i-can-send-emails-using-the-email-account branch from 3b76753 to 2a0829e Compare October 8, 2024 08:47
@martmull martmull marked this pull request as ready for review October 8, 2024 10:02
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request implements email sending functionality using connected Google accounts in the Twenty application's workflow system. Here's a concise summary of the major changes:

  • Implemented email sending via Google SDK in send-email.workflow-action.ts
  • Added 'https://www.googleapis.com/auth/gmail.send' scope to Google API OAuth scopes
  • Updated WorkflowEditActionFormSendEmail component to select connected accounts for email sending
  • Added 'scopes' field to ConnectedAccountWorkspaceEntity and related repository methods
  • Introduced MailSenderException for handling mail sender-related errors
  • Updated WorkflowSendEmailStepSettings to use connectedAccountId instead of template-based approach

21 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings

@Devessier Devessier self-requested a review October 8, 2024 12:25
eq: action.settings.connectedAccountId,
},
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

could we extract this logic, as long as the select component? I feel this kind of app connection will happen often. It should be re-usable and not specific to SendEmailAction. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think we should wait for the next action to be implemented to factorize this logic

Copy link
Contributor

@Devessier Devessier left a comment

Choose a reason for hiding this comment

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

Great job! I left a few comments for things I think we should discuss 😄

@@ -32,6 +39,7 @@ export type SelectProps<Value extends string | number | null> = {
options: SelectOption<Value>[];
value?: Value;
withSearchInput?: boolean;
callToActionButton?: CallToActionButton;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely convinced we should add specific use cases to the raw Select components. The Select component should allow composition: We should be able to add such a button quickly, but the Select component should not know about it.

One example I like is Headless UI components:

CleanShot 2024-10-08 at 15 51 36@2x

Another solution, if we want to add buttons in selects often, is to make it possible to inject buttons through the options property:

options={[{ label: 'XX', value: "" }, { type: "divider" }, { type: "button", label: "Add one +", onClick: () => {}, icon: "" }]}

If we keep adding specific use cases like this to the Select component itself, it will quickly become bloated.

I don't think we should change something to your PR, this is more a discussion to start with the whole team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I Allowed that to myself as there already is a SearchInput option that can be displayed at the bottom of the select. @charlesBochet @thomtrp wdyt?

onChange={(connectedAccountId) => {
field.onChange(connectedAccountId);
handleSave();
checkConnectedAccountScopes(connectedAccountId);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does happen if the user closes the Google tab we opened before completing the process? The account will be wired to the step – handleSave() is called before checkConnectedAccountScopes() – but the account wouldn't be set up correctly. Is it possible to recover from this error in the app currently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right. In fact I call handleSave() before because we don't have access to the handleSave() method after the google oAuth, so it is not possible to do it.
In fact it is possible, but it would be a lot of work for something "temporary" as the accounts will have the proper scopes after they have been updated
Also, I think this oAuth triggered at dropdown select is a little bit aggressive, a warning banner would be better in my opinion, will discuss that with @Bonapara

@martmull martmull force-pushed the 7417-workflows-i-can-send-emails-using-the-email-account branch from 82b9586 to 80e0d4a Compare October 8, 2024 15:43
@martmull martmull merged commit f138a1c into main Oct 8, 2024
13 of 15 checks passed
@martmull martmull deleted the 7417-workflows-i-can-send-emails-using-the-email-account branch October 8, 2024 21:29
harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
- update `send-email.workflow-action.ts` so it send email via the google
sdk
- remove useless `workflow-action.email.ts`
- add `send` authorization to google api scopes
- update the front workflow email step form to provide a
`connectedAccountId` from the available connected accounts
- update the permissions of connected accounts: ask users to reconnect
when selecting missing send permission


![image](https://github.com/user-attachments/assets/fe3c329d-fd67-4d0d-8450-099c35933645)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Workflows] I can send emails using the email account
4 participants