Add `ChannelType.check_credentials` and use for Twilio claims and updates #4275

norkans7 · 2023-01-30T14:04:33Z

Addresses #4227

Old UI

New UI

codecov · 2023-01-30T14:13:31Z

Codecov Report

Merging #4275 (1ae190e) into main (97b7ded) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #4275   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          472       472           
  Lines        26130     26178   +48     
=========================================
+ Hits         26130     26178   +48

Impacted Files	Coverage Δ
temba/channels/models.py	`100.00% <100.00%> (ø)`
temba/channels/types/twilio/type.py	`100.00% <100.00%> (ø)`
temba/channels/types/twilio/views.py	`100.00% <100.00%> (ø)`
...ba/channels/types/twilio_messaging_service/type.py	`100.00% <100.00%> (ø)`
temba/channels/types/twilio_whatsapp/type.py	`100.00% <100.00%> (ø)`
temba/channels/views.py	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

rowanseymour · 2023-01-30T14:37:28Z

temba/channels/models.py

@@ -141,6 +141,9 @@ def get_update_form(self):
            return UpdateChannelForm
        return self.update_form

+    def check_credentials(self, cleaned_data):


I think we want something more like...

def check_credentials(self, config: dict) -> bool:

so then we have something like this on Channel

def check_credentials(self) -> bool: return self.type.check_credentials(self.config)

and then at any time you can verify the credentials of a channel

norkans7 · 2023-01-30T15:51:22Z

temba/channels/types/twilio/views.py

+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+
+        self.add_config_field(


I exatended the channel update to include the credentials field for for the channel type

norkans7 · 2023-01-30T15:52:45Z

temba/channels/types/twilio/views.py

+    def clean(self) -> Dict[str, Any]:
+        account_sid = self.cleaned_data.get("account_sid", None)
+        account_token = self.cleaned_data.get("account_token", None)
+
+        try:
+            client = TwilioClient(account_sid, account_token)
+
+            # get the actual primary auth tokens from twilio and use them
+            account = client.api.account.fetch()
+            self.cleaned_data["account_sid"] = account.sid
+            self.cleaned_data["account_token"] = account.auth_token
+        except Exception:  # pragma: needs cover
+            raise ValidationError(
+                _("The Twilio account SID and Token seem invalid. Please check them again and retry.")
+            )
+
+        return self.cleaned_data


We need to clean the credentials, and in case of Twilio we need to save the primary token of the account in case we were given others

So each channel type update form will do its own validation if needed

norkans7 · 2023-01-30T16:01:01Z

temba/channels/models.py

@@ -74,6 +74,7 @@ class IVRProtocol(Enum):
    show_public_addresses = False

    update_form = None
+    verify_credentials = False


For now we'll be switching this flag once we have added the channel type check_credentials

could we just have ChannelType.check_credentials default to True ?

I thought we'll remove the flag once the method is implemented for all channel type we have but I guess that is fine to have that default to True

rowanseymour · 2023-01-31T16:41:26Z

temba/channels/types/twilio/views.py

+            "account_sid",
+            forms.CharField(
+                max_length=128,
+                label=_("Your Twilio Account SID"),


Remove Your

rowanseymour · 2023-01-31T16:41:33Z

temba/channels/types/twilio/views.py

+            "auth_token",
+            forms.CharField(
+                max_length=128,
+                label=_("Your Twilio Account Auth Token"),


Remove Your

rowanseymour · 2023-01-31T16:48:52Z

temba/channels/types/twilio/views.py

+        account_sid = self.cleaned_data.get("account_sid", None)
+        account_token = self.cleaned_data.get("account_token", None)
+
+        try:


We want to use check_credentials here as well - calling it with the config + these new values, e.g.

self.instance.check_credentials({**self.instance.config, "account_sid": account_sid, "auth_token": auth_token})

the main thing here is we want to grab the primary credentials so check_credentials is not enough alone

otherwise we would have relied on the base class clean from that use the method that way

https://github.com/nyaruka/rapidpro/pull/4275/files#diff-6aff4c43504e1e48908ece639cb7e6a4d98608c0ce761fc2bfd298b10a57ddedR674-R680

rowanseymour · 2023-01-31T16:52:51Z

temba/channels/views.py

@@ -670,6 +671,13 @@ def add_config_field(self, config_key: str, field, *, default):
        self.fields[config_key] = field
        self.config_fields.append(config_key)

+    def clean(self) -> Dict[str, Any]:


python 3.10 prefers dict[str, Any].. we need to come up with a style guide for typing in Python. I've generally avoided complex types and probably would have just used dict here, but we need some rules to keep ourselves consistent.

rowanseymour · 2023-01-31T16:53:56Z

temba/channels/models.py

@@ -74,6 +74,7 @@ class IVRProtocol(Enum):
    show_public_addresses = False

    update_form = None
+    verify_credentials = False


could we just have ChannelType.check_credentials default to True ?

rowanseymour · 2023-01-31T17:15:42Z

temba/channels/views.py

+        cleaned_data = super().clean()
+        channel_type = Channel.get_type_from_code(self.object.channel_type)
+        if channel_type.verify_credentials and not channel_type.check_credentials(cleaned_data):
+            raise ValidationError(_("Error checking credentials for channel type %s" % self.object.channel_type))


This should be a user facing error message like Channel credentials don't appear to be valid.

susanm74

@norkans7 i don't know this area of the product super well, so just dropping a comment (vs approving) after reviewing the changes to say LGTM

norkans7 · 2023-03-07T16:32:18Z

@rowanseymour @ericnewcomer when you get a chance please review

rowanseymour · 2023-03-07T17:00:27Z

temba/channels/types/twilio/views.py

+        )
+
+    # We override the clean method for Twilio we need to make sure we grab the primary auth tokens
+    def clean(self) -> Dict[str, Any]:


def clean(self) -> dict[str, Any]: """ We override the clean method for Twilio we need to make sure we grab the primary auth tokens """

rowanseymour · 2023-03-07T17:03:11Z

temba/channels/views.py

@@ -424,6 +425,18 @@ def add_config_field(self, config_key: str, field, *, default):
        self.fields[config_key] = field
        self.config_fields.append(config_key)

+    def transform_form_data_credentials_config(self, cleaned_data: dict, extra: dict) -> dict:


this doesn't need to be a separate method.. or have the longest name in the repo lol

rowanseymour · 2023-03-08T15:42:09Z

temba/channels/views.py

+
+    def clean(self) -> dict[str, Any]:
+        cleaned_data = super().clean()
+        credentials_config = self.creds_config(cleaned_data, {})


Am still confused by this line which is just cloning cleaned_data

Shouldn't this be self.object.config | cleaned_data ? i.e. merge existing config and new values from form?

The idea is that channel types subclass this and override creds_config?

In which case make it something like...

def to_config(self, data) -> dict: return {}

That just returns form data transformed into config values, and do the merging in clean and form_valid with self.instance.config | self.to_config(cleaned_data)

Right the idea is to allow the subclass to override that in case the form fields names do not match the config keys.
For Twilio so far added I see I had changed to have them match so I think the to_config is probably not needed for now.

norkans7 · 2023-03-08T18:02:29Z

The keys are not the same

…

On Wed, Mar 8, 2023, 8:00 PM Rowan Seymour ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In temba/channels/views.py <#4275 (comment)>: > @@ -424,6 +425,21 @@ def add_config_field(self, config_key: str, field, *, default): self.fields[config_key] = field self.config_fields.append(config_key) + def creds_config(self, cleaned_data: dict, extra: dict) -> dict: + """ + Transform form data credentials to config format dict + """ + config = cleaned_data.copy() + config.update(**extra) + return config + + def clean(self) -> dict[str, Any]: + cleaned_data = super().clean() + credentials_config = self.creds_config(cleaned_data, {}) Am still confused by this line which is just cloning cleaned_data Shouldn't this be self.object.config | cleaned_data ? i.e. merge existing config and new values from form? — Reply to this email directly, view it on GitHub <#4275 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH6BO2HQD3N4U5UICIAWMDW3DCL5ANCNFSM6AAAAAAULGCU4E> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…ntials

rowanseymour reviewed Jan 30, 2023

View reviewed changes

norkans7 commented Jan 30, 2023

View reviewed changes

rowanseymour requested changes Jan 31, 2023

View reviewed changes

rowanseymour changed the title ~~Twilio update~~ Add ChannelType.check_credentials and use for Twilio claims and updates Jan 31, 2023

rowanseymour reviewed Jan 31, 2023

View reviewed changes

rowanseymour mentioned this pull request Jan 31, 2023

Twilio credentials update #4251

Closed

norkans7 force-pushed the twilio-update branch 3 times, most recently from 6ad473c to d38c66f Compare February 2, 2023 10:40

norkans7 marked this pull request as ready for review February 2, 2023 10:53

norkans7 requested review from ericnewcomer, susanm74 and rowanseymour February 2, 2023 10:53

susanm74 reviewed Feb 28, 2023

View reviewed changes

norkans7 force-pushed the twilio-update branch from eb0723d to e0a2e86 Compare March 7, 2023 15:59

rowanseymour requested changes Mar 7, 2023

View reviewed changes

rowanseymour reviewed Mar 8, 2023

View reviewed changes

norkans7 added 8 commits March 10, 2023 13:20

Extend the channel update view for twilio to include the twilio crede…

4ff9ed7

…ntials

Fix conflicts

e0d8406

Channel type check credentials should to return a boolean

e81a405

Add channel type check_credentials default to True

171e09b

Fix conflicts

1613637

Update tests and add coverage

2d8bfd8

Support update twilio credentials for TMS and TWA channels

f02a0f6

Fix tests

0e10eb5

norkans7 added 2 commits March 10, 2023 13:20

Adjust comment and method name

fdbd9e8

Check channel creadentials for merged config

1ae190e

norkans7 force-pushed the twilio-update branch from 9e64f00 to 1ae190e Compare March 10, 2023 12:55

rowanseymour approved these changes Mar 13, 2023

View reviewed changes

rowanseymour merged commit 90bb50b into main Mar 13, 2023

rowanseymour deleted the twilio-update branch March 13, 2023 16:47

github-actions bot locked and limited conversation to collaborators Mar 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ChannelType.check_credentials` and use for Twilio claims and updates #4275

Add `ChannelType.check_credentials` and use for Twilio claims and updates #4275

norkans7 commented Jan 30, 2023 •

edited

Loading

codecov bot commented Jan 30, 2023 •

edited

Loading

rowanseymour Jan 30, 2023

norkans7 Jan 30, 2023

norkans7 Jan 30, 2023

norkans7 Jan 30, 2023 •

edited

Loading

norkans7 Jan 30, 2023

rowanseymour Jan 31, 2023

norkans7 Jan 31, 2023

rowanseymour Jan 31, 2023

rowanseymour Jan 31, 2023

rowanseymour Jan 31, 2023

norkans7 Jan 31, 2023

rowanseymour Jan 31, 2023

rowanseymour Jan 31, 2023

rowanseymour Jan 31, 2023

susanm74 left a comment

norkans7 commented Mar 7, 2023

rowanseymour Mar 7, 2023

rowanseymour Mar 7, 2023

rowanseymour Mar 8, 2023

rowanseymour Mar 8, 2023

norkans7 Mar 10, 2023

norkans7 commented Mar 8, 2023 via email

Add ChannelType.check_credentials and use for Twilio claims and updates #4275

Add ChannelType.check_credentials and use for Twilio claims and updates #4275

Conversation

norkans7 commented Jan 30, 2023 • edited Loading

codecov bot commented Jan 30, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norkans7 Jan 30, 2023 • edited Loading

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

susanm74 left a comment

Choose a reason for hiding this comment

norkans7 commented Mar 7, 2023

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

norkans7 commented Mar 8, 2023 via email

Add `ChannelType.check_credentials` and use for Twilio claims and updates #4275

Add `ChannelType.check_credentials` and use for Twilio claims and updates #4275

norkans7 commented Jan 30, 2023 •

edited

Loading

codecov bot commented Jan 30, 2023 •

edited

Loading

norkans7 Jan 30, 2023 •

edited

Loading