Ce/connect messaging #35364

calellowitz · 2024-11-14T03:35:13Z

Product Description

This adds connect messaging as a new message type in the messaging framework. It does not currently support form sessions although I will add those prior to merge. That said, I wanted to open this up to review now since it is quite large and touches a fair amount of complex code.

Technical Summary

Tech Spec: https://docs.google.com/document/d/1FgsXr0PLR7Btq2fOBeXIMs_OziBRsUsATgHgWna0h5M/edit?tab=t.0#heading=h.3rzu21agk2m3
Design Doc: https://docs.google.com/document/d/1h3bcZ4oQAnOu5aGW8Rue0TnbiaW2OXWMaqNKwI6RciE/edit?tab=t.0#heading=h.ipgou4bu0qoj

The linked docs provide much of the context

Feature Flag

COMMCARE_CONNECT for now, but I expect to migrate it to its own flag so that it can be enabled independently of the rest of the connect features.

Safety Assurance

Safety story

Existing messaging paths have automated coverage, and are only minimally modified. The new code is almost entirely self contained and will go through a QA process.

Automated test coverage

QA Plan

No QA ticket yet since it is not finished, but I will add it once it starts.

Migrations

The migrations in this code can be safely applied first independently of the code

Rollback instructions

This PR can be reverted after deploy with no further considerations

Labels & Review

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

orangejenny · 2024-11-18T18:28:05Z

corehq/apps/sms/api.py

@@ -174,7 +182,7 @@ def send_sms(domain, contact, phone_number, text, metadata=None, logged_subevent
    return queue_outgoing_sms(msg)


-def send_sms_to_verified_number(verified_number, text, metadata=None, logged_subevent=None, events=None):


This is referenced in https://github.com/dimagi/commcare-hq/blob/master/docs/messaging/outbound_sms.rst, can you update that? It'd probably be helpful to update those docs a bit more holitically, at least to mention that connect messages exist.

Good catch, I will definitely do a pass on those docs

orangejenny · 2024-11-18T18:28:46Z

corehq/apps/sms/api.py

@@ -131,6 +132,13 @@ def get_sms_class():
    return QueuedSMS if settings.SMS_QUEUE_ENABLED else SMS


+def get_message_class(phone_number):


What would you think of inlining get_sms_class into this function? Seems a little nicer to have calling code always use this function instead of needing to know when to call this vs get_sms_class.

Sounds reasonable to me, I will double check usage, but it seems right

orangejenny · 2024-11-18T18:33:39Z

corehq/apps/sms/migrations/0061_connectmessage_message_id_connectmessage_received_on.py

+from django.db import migrations, models
+import uuid
+
+class Migration(migrations.Migration):


I think it's standard to squash all the migrations for a single PR, just to reduce the number of migrations in the repo long-term.

Yep, I plan do that

orangejenny · 2024-11-18T18:35:25Z

corehq/apps/sms/models.py

+class ConnectMessage(Log):
+    date_modified = models.DateTimeField(null=True, db_index=True, auto_now=True)
+    text = models.CharField(max_length=300)
+    received_on = models.DateTimeField(null=True, blank=True)


Why would this be blank/null? I'd expected this to default to now.

Outgoing messages won't have a received timestamp until they get to the phone and the phone acknowledges them

orangejenny · 2024-11-18T18:39:49Z

corehq/apps/sms/models.py

+    @property
+    @abstractmethod
+    def backend(self):
+        pass


Can these raise NotImplementedErrors, or does this being an abstract class take care of complaining if a subclass doesn't implement all of the necessary methods?

Oh, I meant to get rid of this class entirely. I had originally planned to have the ConnectNumber and the old PhoneNumber class inherit from it, and treat it as an interface the rest of the messaging code could use, but it turns out its difficult to use abstract base classes with django models because they both do weird things to the class Meta. I may still try to find a way to get both to pass the same type check, but I dont think this class is the answer.

orangejenny · 2024-11-18T20:44:49Z

corehq/messaging/scheduling/forms.py

@@ -513,9 +520,9 @@ def get_layout_fields(self):
                        data_bind='with: message',
                    ),
                    data_bind=(
-                        "visible: $root.content() === '%s' || $root.content() === '%s' "


A f string would make this a whole lot better.

good call. I can make that change

orangejenny · 2024-11-18T20:44:58Z

corehq/messaging/scheduling/forms.py

@@ -534,10 +541,11 @@ def get_layout_fields(self):
                    ),
                    data_bind=(
                        "visible: $root.content() === '%s' || $root.content() === '%s' "
-                        "|| $root.content() === '%s' "
+                        "|| $root.content() === '%s' || $root.content() === '%s' "


Same comment about f strings.

orangejenny · 2024-11-18T20:45:36Z

corehq/messaging/scheduling/forms.py

@@ -1757,6 +1770,10 @@ def create_form_helper():
    def form_choices(self):
        return [(form['code'], form['name']) for form in get_form_list(self.domain)]

+    @property
+    def can_use_connect(self):


Nitpick: is this better than just having the calling code check the feature flag?

orangejenny · 2024-11-18T20:46:33Z

corehq/apps/domain/auth.py

@@ -405,3 +407,19 @@ def user_can_access_domain_specific_pages(request):
        return False

    return couch_user.is_member_of(project) or (couch_user.is_superuser and not project.restrict_superusers)
+
+
+def connectid_token_auth(view_func):


It's weird that connectid is one word. I do see that it's consistent.

I think of connectid as its own thing (its a separate repo and site than connect), using one word feels like it implies that, rather than the id for connect, but I am not wedded to it.

connectid and "the id for connect` being two separate concepts sounds confusing, but I don't think I have enough context on the project to be certain or to make a better suggestion.

orangejenny · 2024-11-18T20:55:20Z

.../messaging/scheduling/migrations/0030_remove_connectmessagesurveycontent_message_and_more.py

            preserve_default=False,
        ),
        migrations.AddField(
            model_name='connectmessagesurveycontent',
            name='form_unique_id',
-            field=models.CharField(default=1, max_length=126),
+            field=models.CharField(default="1", max_length=126),


Why not just allow this to be blank? Or does 1 have a meaning?

Nope, this will be unnecessary once we combine the migrations as you mentioned above, we just needed any value to have a valid migration, even though that table was empty when I made this one, so it is never used.

snopoke

In general this looks good to me but I'm not able to fully review all the implications of the changes.

calellowitz requested review from snopoke and pxwxnvermx November 14, 2024 03:35

calellowitz requested review from esoergel, orangejenny and mkangia as code owners November 14, 2024 03:35

dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Nov 14, 2024

calellowitz added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Nov 14, 2024

calellowitz force-pushed the ce/connect-messaging branch 2 times, most recently from 6a953fa to f22c90d Compare November 14, 2024 21:06

calellowitz added 7 commits November 14, 2024 16:09

add models and rename function

0043322

add connect backend

7bc3854

add connectid settings and urls

c609f0d

add connect messages to scheduling forms

e4b6deb

add necessary auth changes

929204d

update domain deletion code for new models

79154f0

use updated method name

460c794

calellowitz force-pushed the ce/connect-messaging branch from f22c90d to 460c794 Compare November 14, 2024 21:09

calellowitz added 2 commits November 14, 2024 22:24

implement survey model

092dbe0

fix default values

f29ebfc

orangejenny reviewed Nov 18, 2024

View reviewed changes

snopoke reviewed Nov 19, 2024

View reviewed changes

calellowitz added 4 commits November 22, 2024 15:44

move content fields to top level of payload

91ce6e6

use string for the key

4b992c7

return the value

36a800f

typo

472609a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ce/connect messaging #35364

Ce/connect messaging #35364

calellowitz commented Nov 14, 2024 •

edited

Loading

orangejenny Nov 18, 2024

calellowitz Nov 22, 2024

orangejenny Nov 18, 2024

calellowitz Nov 22, 2024

orangejenny Nov 18, 2024

calellowitz Nov 22, 2024

orangejenny Nov 18, 2024

calellowitz Nov 22, 2024

orangejenny Nov 18, 2024

calellowitz Nov 22, 2024

orangejenny Nov 22, 2024

orangejenny Nov 18, 2024

calellowitz Nov 22, 2024

orangejenny Nov 18, 2024

orangejenny Nov 18, 2024

orangejenny Nov 18, 2024

calellowitz Nov 22, 2024

orangejenny Nov 22, 2024

orangejenny Nov 18, 2024

calellowitz Nov 22, 2024

snopoke left a comment

		@@ -174,7 +182,7 @@ def send_sms(domain, contact, phone_number, text, metadata=None, logged_subevent
		return queue_outgoing_sms(msg)


		def send_sms_to_verified_number(verified_number, text, metadata=None, logged_subevent=None, events=None):

		@@ -131,6 +132,13 @@ def get_sms_class():
		return QueuedSMS if settings.SMS_QUEUE_ENABLED else SMS


		def get_message_class(phone_number):

Ce/connect messaging #35364

Are you sure you want to change the base?

Ce/connect messaging #35364

Conversation

calellowitz commented Nov 14, 2024 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Migrations

Rollback instructions

Labels & Review

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

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

snopoke left a comment

Choose a reason for hiding this comment

calellowitz commented Nov 14, 2024 •

edited

Loading