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

feat(providers): Add Whatsapp business as provider #5232

Merged
merged 11 commits into from
Apr 30, 2024

Conversation

vitoorgomes
Copy link
Contributor

What change does this PR introduce?

This PR adds the option to use WhatsApp Business as a provider. The requested issue is #1019.

Although there is already a PR opened, I decide to create this fresh one for two main reasons: firstly because I don't think the OP it's working anymore on that based on replies not being answered and secondly because it only implements sending message as a template, where the Meta API allows to send it as a simple text as well, I wanted to give a little more customization to the user.

There some improvements that could be done here, but they aren't breaking changes, so I think it's worth to open this now, like allowing some other configurations like preview_url and other fields available in the Meta's API.

There is also the #2689 issue, while it's valid, it would be nice to have this provider as a SMS now, and port this later when Chat Providers are able to receive more data following this issue thread updates.

Why was this change needed?

This Closes #1019 and Closes #4128

Other information (Screenshots)

SCR-20240223-pxj
SCR-20240223-pxe

Copy link

netlify bot commented Feb 23, 2024

Deploy Preview for dev-web-novu ready!

Name Link
🔨 Latest commit 86d79e6
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/66316b659506260008267f07
😎 Deploy Preview https://deploy-preview-5232--dev-web-novu.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Cliftonz
Copy link
Contributor

Cliftonz commented Mar 4, 2024

@vitoorgomes Thanks for bringing this to our attention, we will take a look at it as soon as we can!

@Cliftonz
Copy link
Contributor

Cliftonz commented Mar 4, 2024

@vitoorgomes What are the other improvements you think could be done?

@vitoorgomes
Copy link
Contributor Author

@vitoorgomes What are the other improvements you think could be done?

Meta's API docs have lots of options to send a message, with media, location and so on, but since they don't affect the usability as of right now, I've decided to not try to implement it, because it would need a lot effort to be able to define each property but could be done in the future as devs request

Copy link
Contributor

@Cliftonz Cliftonz left a comment

Choose a reason for hiding this comment

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

@vitoorgomes There is two things you need to fix here.

  1. The hard coded authentication header
  2. adding the whatsapp names to the cspell list

@vitoorgomes vitoorgomes force-pushed the feat/whatsapp-business-provider branch from 017f3d0 to 4f0343b Compare March 15, 2024 20:43
@vitoorgomes
Copy link
Contributor Author

@vitoorgomes There is two things you need to fix here.

  1. The hard coded authentication header
  2. adding the whatsapp names to the cspell list

done

@github-actions github-actions bot added the stale Pull Request that needs to be reviewed label Mar 29, 2024
@vitoorgomes vitoorgomes requested a review from a team as a code owner April 30, 2024 15:02
Copy link

netlify bot commented Apr 30, 2024

Deploy Preview for novu-design ready!

Name Link
🔨 Latest commit 86d79e6
🔍 Latest deploy log https://app.netlify.com/sites/novu-design/deploys/66316b659304f10008d64b08
😎 Deploy Preview https://deploy-preview-5232--novu-design.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@antonjoel82 antonjoel82 left a comment

Choose a reason for hiding this comment

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

Thanks for the hard work here, Vitor! Added a couple comments regarding CI and to ensure we have good handling around phone numbers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider a more semantic name for the file: whatsapp-message-type.ts

channelType = ChannelTypeEnum.SMS as ChannelTypeEnum.SMS;

private readonly axiosClient: AxiosInstance;
private readonly baseUrl = 'https://graph.facebook.com/v18.0/';
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ question: Is this correct?‏

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. tested it out.

@@ -1120,3 +1120,21 @@ export const eazySmsConfig: IConfigCredentials[] = [
description: 'Your SMS Channel Id',
},
];

export const whatsaAppBusinessConfig: IConfigCredentials[] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

chore: Please correct this typo‏

Suggested change
export const whatsaAppBusinessConfig: IConfigCredentials[] = [
export const whatsAppBusinessConfig: IConfigCredentials[] = [

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

{
key: CredentialsKeyEnum.phoneNumberIdentification,
displayName: 'Phone Number Identification',
description: 'Your WhatsApp Business phone number identification',
Copy link
Contributor

Choose a reason for hiding this comment

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

question: What are the phone number requirements for WhatsApp? Do they require a certain format?

chore: ‏‏If there is a format required, please describe / define it here, and enforce it with pattern matching

Copy link
Contributor

Choose a reason for hiding this comment

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

This is provided in the WhatsApp interface, you can't really control it. The needs to copy paste this. I will add this to the doccs section of the whole process

Comment on lines +114 to +119
chatChannels.push({
providerId: ChatProviderIdEnum.WhatsAppBusiness,
credentials: {
phoneNumber: phone,
},
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love this implementation, but until we refactor the whole "Chat" channel flow, this workaround is needed.

const channelSpecification = subscriberChannel.credentials?.channel;

if (!chatWebhookUrl) {
if (!chatWebhookUrl && integration.providerId !== ChatProviderIdEnum.WhatsAppBusiness) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Those workaround will be handeled during a more thorough refactoring of the "Chat" Channel

@@ -26,6 +26,7 @@ export abstract class SendMessageType {
) {
const errorString =
stringifyObject(error?.response?.body) ||
stringifyObject(error?.response?.data) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This needed to gracefully map axios errors

Comment on lines +88 to +90
headers: {
'X-Novu-Custom-Header': 'test-data',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This fixes a failing test on next branch

@scopsy scopsy changed the title Add Whatsapp business as provider feat(providers): Add Whatsapp business as provider Apr 30, 2024
Copy link
Contributor

@SokratisVidros SokratisVidros left a comment

Choose a reason for hiding this comment

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

Nice!

I've added some comments regarding code structure aiming to isolate the necessary patch for now in dedicated functions that will be easier to refactor in the future.

@@ -239,8 +267,8 @@ export class SendMessageChat extends SendMessageBase {
})
);

if (chatWebhookUrl && integration) {
await this.sendMessage(chatWebhookUrl, integration, content, message, command, channelSpecification);
if ((chatWebhookUrl && integration) || (phoneNumber && integration)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In an attempt to start defining a naive strategy pattern until we do the proper refactoring, I want to suggest introducing two private methods:

  • sendChannelMessageViaWebhookUrl
  • sendChannelMessageViaAPIKey

That way, we will ensure that messages created in Mongo have only defined fields per use case and have fewer and shorter if conditions across the file.

The current sendError can be implemented inside the above two private methods separately for now.

@scopsy scopsy merged commit 00cebdd into novuhq:next Apr 30, 2024
12 of 14 checks passed
@vitoorgomes
Copy link
Contributor Author

thanks a lot for helping out @scopsy I didn't saw the last request changes notification, totally my bad

really happy that this was merged 🥳🤝

@scopsy
Copy link
Contributor

scopsy commented Apr 30, 2024

Thank you @vitoorgomes so much for your contribution ❤️ and sorry it took a while to merge 🙏

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.

[NV-782] - WhatsApp Provider
5 participants