Sureadhere messaging provider integrated #390

cws1121 · 2024-05-10T16:07:02Z

No description provided.

…integration)

# Conflicts resolved: # apps/channels/models.py # apps/channels/tasks.py

bderenzi · 2024-05-14T09:29:23Z

@cws1121 - can you run ruff on the code to sort the code-style at some point? (fine to wait for further review comments)

apps/channels/datamodels.py

apps/channels/forms.py

snopoke · 2024-05-14T09:56:58Z

apps/channels/models.py



 class ChannelPlatform(models.TextChoices):
    TELEGRAM = "telegram", "Telegram"
    WEB = "web", "Web"
    WHATSAPP = "whatsapp", "WhatsApp"
    FACEBOOK = "facebook", "Facebook"
+    IN_APP = "in_app", "In App"


Can we change the name of this:

Suggested change

IN_APP = "in_app", "In App"

SUREADHERE = "sureadhere", "SureAdhere"

Also any other references e.g. InAppChannelForm

Alright, I've renamed everything to SureAdhere as suggested. However, I'm not entirely convinced this is the best way to go. While SureAdhere is one of the in-app messaging platforms we currently support, what if we decide to add another provider in the future that also offers in-app messaging capabilities? For instance, we already have both Twilio and Turn.io listed under the "whatsapp" option, even though they are separate providers for the WhatsApp channel.That said perhaps it would be better to keep the more generic "in_app" naming convention. What do you think?

While SureAdhere is one of the in-app messaging platforms we currently support, what if we decide to add another provider in the future that also offers in-app messaging capabilities?

I'm not sure I quite follow this. If SA adds support for another provider e.g. telegram then OCS will still want to use the SA channel and SA will decide how to deliver that message to the user based on what provider that user is using to communicate with. But OCS won't know about the different downstream providers will it?

When I mentioned adding another provider in the future, I was referring to the possibility of OCS itself integrating with another in-app messaging platform similar to SureAdhere, not SureAdhere adding support for another provider.
The concern I raised was that if we rename everything to be specific to SureAdhere, it might make it more difficult to integrate with a different in-app messaging platform in the future, should the need arise. By keeping the naming convention more generic (e.g., InAppChannelForm), it would be easier to accommodate additional in-app messaging providers without having to rename everything again.

For instance, the 'whatsapp' option encompasses both Twilio and Turn.io providers.

I feel like we're missing eachother somewhere and maybe we should go to voice but I'll take one more go at explaining what I think.

When you say "in app", do you mean "communication through a mobile app"? Is that not what WhatsApp and Telegram are already?

In terms of this data model, a channel type should be 1-1 with how the user access it e.g. whatsapp vs telegram vs sureadhere

The MessagingProvider ties the channels to a way of sending and receiving messages and is 1-many since the same provider could facilitate communications on multiple channels and multiple providers can support the same channel.

If we take WhatsApp as an example, both Turn and Twilio provide WhatsApp messaging capabilities but which one the user chooses depends on which provider they are using.

If you still have doubts, let's schedule a call.

apps/channels/models.py

apps/chat/channels.py

snopoke · 2024-05-14T10:15:16Z

apps/service_providers/messaging_service.py

+        return response.json()["access_token"]
+
+    def send_text_message(self, message: str, to: str, platform: ChannelPlatform, from_: str = None):
+        access_token = self.get_access_token()


Is there a way we could reuse these? How long do they last for?

I believe the token is valid for 1 hour. I'll double check that.

I confirm, it's 60min

According to the documentation, we could increase that to 24 hours (the upper limit).

apps/service_providers/messaging_service.py

snopoke · 2024-05-14T10:20:59Z

apps/channels/forms.py

@@ -71,6 +71,33 @@ def get_success_message(self, channel: ExperimentChannel):
        return f"Use the following URL when setting up the webhook: {channel.webhook_url}"


+class InAppChannelForm(ExtraFormBase):
+    client_id = forms.CharField(label="Client ID", max_length=100)


Is this client_id different from the one on the MessagingProvider. If they are the same the it doesn't seem useful to have it here as well.

Yes it's a different, it refers to the ID of the SureAdhere client (workspace). This field acts as a unique key (channel_identifier_key property of the ChannelPlatform)

Can you confirm this understanding:

MessagingProvider.client_id: this is an Oauth client ID used to authenticate with SureAdhere

Channel.client_id: this is a workspace ID in SureAdhere (is a workspace SureAdhere's 'tenent'?)

Is Client ID the right name for it then?

It's also not clear to me what this is used for since it isn't included in the requests to send messages so it appears to only be used for the webhook.

I think that leads to the question of how the webhooks in SA are configured. Is there documentation on that somewhere? How does SA know which webhook to call for a specific message?

Yes, I can confirm your understanding of the parameter names.

We can rename that parameter to sureadhere_client_id perhaps?

Initially, I was using client_id (the SureAdhere tenant ID) for the webhook configuration. However, based on your suggestion, I've replaced it with channel_external_id. At the moment, client_id is only being used to distinguish different SureAdhere channels on OCS.
As per my understanding channels need to have a variable as unique identifier (I am referring to channel_identifier_key -> 8a0f7b3#diff-e86734efa3030e0fee86bb735b2db59c0349a54045cc1a0b48d3b187b82d722bR61)

There is no documentation available on how the webhooks in SureAdhere are configured. Currently, we only have one hardcoded webhook on staging that we're using for testing purposes. This is still an open question, and I haven't received clear instructions from the product team on how that part of the integration is supposed to work. Here are a few thoughts based on the potential scenarios:

If SureAdhere provides an interface where we can assign webhooks for each client (tenant), then the current implementation of using channel_external_id for the webhook configuration seems like a reasonable approach.
However, if SureAdhere is expected to know which OCS webhook to use when forwarding client messages (without any configuration), then it would be a good idea to revert channel_external_id back to client_id in the OCS webhook URLs. Does that sound reasonable to you?

I'll follow up with the product team tomorrow to get more clarity on how the SureAdhere integration is intended to work and how the webhooks should be configured.

Typically webhooks are configured in the sending system so that it know where to send messages. There are 2 options:

A generic webhook where the request body contains some information that identifies where the message is coming from e.g. client_id

A webhook that contains some identifying info in it e.g. experiment_id or client_id

Either of these approaches are fine but both require some way to configure them in SA.

For now we can revert to using client_id since that's what allows you to test but I'd like to hear what the product team has to say.

apps/channels/views.py

snopoke · 2024-05-15T11:25:09Z

apps/channels/forms.py

@@ -71,6 +71,33 @@ def get_success_message(self, channel: ExperimentChannel):
        return f"Use the following URL when setting up the webhook: {channel.webhook_url}"


+class InAppChannelForm(ExtraFormBase):
+    client_id = forms.CharField(label="Client ID", max_length=100)


Can you confirm this understanding:

MessagingProvider.client_id: this is an Oauth client ID used to authenticate with SureAdhere

Channel.client_id: this is a workspace ID in SureAdhere (is a workspace SureAdhere's 'tenent'?)

Is Client ID the right name for it then?

It's also not clear to me what this is used for since it isn't included in the requests to send messages so it appears to only be used for the webhook.

I think that leads to the question of how the webhooks in SA are configured. Is there documentation on that somewhere? How does SA know which webhook to call for a specific message?

apps/channels/datamodels.py

apps/channels/tests/message_examples/sureadhere_messages.py

apps/channels/views.py

apps/service_providers/forms.py

apps/channels/forms.py

snopoke · 2024-05-15T11:41:33Z

apps/channels/models.py



 class ChannelPlatform(models.TextChoices):
    TELEGRAM = "telegram", "Telegram"
    WEB = "web", "Web"
    WHATSAPP = "whatsapp", "WhatsApp"
    FACEBOOK = "facebook", "Facebook"
+    IN_APP = "in_app", "In App"


While SureAdhere is one of the in-app messaging platforms we currently support, what if we decide to add another provider in the future that also offers in-app messaging capabilities?

I'm not sure I quite follow this. If SA adds support for another provider e.g. telegram then OCS will still want to use the SA channel and SA will decide how to deliver that message to the user based on what provider that user is using to communicate with. But OCS won't know about the different downstream providers will it?

apps/channels/models.py

…l_id

snopoke

Thanks for all the updates. This is very close now. I think there are only 2 outstanding things I'd like addressed before approval:

Revert the webhook to use client_id so that you can continue testing
Add help text to SureAdhereChannelForm.client_id indicating what it is

snopoke · 2024-05-16T09:04:51Z

apps/channels/models.py

+    PLATFORM = (
+        (TELEGRAM, "Telegram"),
+        (WEB, "Web"),
+        (WHATSAPP, "WhatsApp"),
+        (FACEBOOK, "Facebook"),
+        (SUREADHERE, "SureAdhere"),
+    )


this doesn't appear to be used anywhere so you could delete it

snopoke · 2024-05-16T09:06:44Z

apps/channels/forms.py

-        return f"Use the following URL when setting up the webhook: {channel.webhook_url}"
+
+class SureAdhereChannelForm(WebhookUrlFormBase):
+    client_id = forms.CharField(label="Client ID", max_length=100)


Please add some help text here assist the user in understanding what this field is.

What's the difference between this client_id field and the client_id field from the SureAdhereService? This client id identifies a SureAdhere "project" so to speak whereas the other client id is an Azure id, yes?

Ah I just saw this comment explaining the difference

SmittieC · 2024-05-21T06:38:31Z

apps/channels/tasks.py

@@ -50,12 +54,29 @@ def handle_twilio_message(self, message_data: str):
        extra_data__contains={channel_id_key: message.to}, messaging_provider__type=MessagingProviderType.twilio
    ).first()
    if not experiment_channel:
+        logger.info(f"No experiment channel found for {channel_id_key}: {message.to}")


cws1121 added 6 commits May 3, 2024 10:03

SureAdhere channel added

8df96d0

SureAdhere channel added - debug mode switched off

5bf1e89

SureAdhere channel - automated tests added (inbound message, channel …

f92299f

…integration)

SureAdhere channel - automated tests added (SA provider fixture added)

7b2adaf

SureAdhere channel - userId removed

fd25b16

readme - small fix

cbe0008

cws1121 requested a review from snopoke May 10, 2024 16:09

cws1121 added 2 commits May 10, 2024 16:04

Merge branch 'main' into sureadhere-messaging-provider

cc7e436

# Conflicts resolved: # apps/channels/models.py # apps/channels/tasks.py

small fix - from_experiment_session method - in_app platform added

37d3571

snopoke reviewed May 14, 2024

View reviewed changes

cws1121 added 3 commits May 15, 2024 00:22

renamed in_app -> sureadhere, small fixes

8a0f7b3

test_sureadhere_integration.py -> client_id renamed to experiment_id

95eab0d

apps/channels/forms.py -> client_id added to SA form

4bba977

snopoke reviewed May 15, 2024

View reviewed changes

sureadhere webhook experiment_id parameter changed to channel_externa…

a07cbbe

…l_id

snopoke reviewed May 16, 2024

View reviewed changes

SmittieC reviewed May 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sureadhere messaging provider integrated #390

Sureadhere messaging provider integrated #390

cws1121 commented May 10, 2024

bderenzi commented May 14, 2024

snopoke May 14, 2024

snopoke May 14, 2024

cws1121 May 15, 2024

snopoke May 15, 2024

cws1121 May 16, 2024

cws1121 May 16, 2024

snopoke May 16, 2024

snopoke May 14, 2024

cws1121 May 15, 2024

cws1121 May 16, 2024

cws1121 May 16, 2024

snopoke May 14, 2024

cws1121 May 15, 2024 •

edited

snopoke May 15, 2024

cws1121 May 16, 2024 •

edited

snopoke May 16, 2024

snopoke May 15, 2024

snopoke May 15, 2024

snopoke left a comment

snopoke May 16, 2024

snopoke May 16, 2024

SmittieC May 21, 2024 •

edited

SmittieC May 22, 2024

SmittieC May 21, 2024

	IN_APP = "in_app", "In App"
	SUREADHERE = "sureadhere", "SureAdhere"

Sureadhere messaging provider integrated #390

Are you sure you want to change the base?

Sureadhere messaging provider integrated #390

Conversation

cws1121 commented May 10, 2024

bderenzi commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cws1121 May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cws1121 May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snopoke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmittieC May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cws1121 May 15, 2024 •

edited

cws1121 May 16, 2024 •

edited

SmittieC May 21, 2024 •

edited