[TAN-8120] Sms campaigns by AchrafGoVocal · Pull Request #14226 · CitizenLabDotCo/citizenlab

AchrafGoVocal · 2026-07-06T11:31:31Z

Changelog

Added

[TAN-8120] Adds manual SMS campaigns.
[TAN-8120] Participants can add and change a phone number on their profile and confirm it with an SMS code.

Changed

[TAN-8120] Notification preferences are now split into separate "Email notifications" and "SMS notifications" sections, so participants can opt in per channel.

Technical

[TAN-8120] Adds an sms feature flag holding Twilio credentials in tenant settings, a Twilio provider with delivery-status callbacks, and an SMS delivery model.
[TAN-8120] Namespaces the frontend campaigns API and types into per-channel email/ and sms/ modules.

For translators

…AN-7844-basic-sms-setup

…/citizenlab into TAN-7922-add-sms-manual-campaigns

…code requests

luucvanderzee

Amazing work!! A few changes requested but honestly not that many for such a huge huge PR. I didn't do any functional testing and only quickly skipped through the BE tests. Can take another look at those later.

I think it would be great if we could rename the folder and module, otherwise it's going to be really confusing. But of course that will add even more changes to this PR

luucvanderzee · 2026-07-10T15:19:45Z


+  # This endpoint is used when a logged in user confirms a pending phone-number
+  # change. On success, new_phone is promoted to phone. The phone
+  # number isn't part of the auth token, so there's no JWT cookie to refresh.


The point of reset_jwt_cookie is not to add update the phone number/email in the token, but to invalidate other sessions. But then that seems to be happening with the token_expiry_key which reset_jwt_cookie does not seem to touch... @jamesspeake how does this work again? And do we need to invalidate the token in this case? I guess not, but wanted to check with you

luucvanderzee · 2026-07-10T15:22:17Z

+    ActiveRecord::Base.transaction do
+      user.update!(new_phone: new_phone)
+      confirmation = user.new_phone_confirmation || user.create_new_phone_confirmation!
+      confirmation.reset_code!
+      campaign = EmailCampaigns::Campaigns::NewPhoneConfirmation.first_or_create!
+      EmailCampaigns::DeliveryService.new.send_now_to_user(campaign, user, { code: confirmation.code })
+
+      confirmation.update!(code_sent_at: Time.zone.now)
+      ExpireConfirmationCodeOrDeleteJob.set(
+        wait_until: confirmation.expiration_at
+      ).perform_later(
+        user.id,
+        NewPhoneConfirmation.name,
+        confirmation.code
+      )
+      LogActivityJob.perform_later(user, 'received_confirmation_code', user, Time.now.to_i, payload: { new_phone: new_phone })
+    end


I propose we keep it the same and look into this transaction stuff when we look at the other issue related to being able to request too many codes if you do it really fast

luucvanderzee · 2026-07-10T17:26:43Z

+    # subject_multiloc is the email subject for content-configurable emails and
+    # the admin-facing label for SMS campaigns (SMS has no subject line).
+    attribute :subject_multiloc, if: proc { |object|
+      content_configurable?(object) || object.sms?


If sms has no subject line, should the check not be || !object.sms?

Maybe the comment is confusing, but what I meant is that the SMS itself has no subject line but the campaign does (it's just an internal label for the admin).

Maybe we can clarify the comment? I still don't really get it.

luucvanderzee · 2026-07-10T17:28:23Z

+        parsed = Phonelib.parse(to)
+        raise Error, "Invalid phone number: #{to}" unless parsed.valid?
+
+        parsed.e164


What is this parser doing exactly? Maybe we can add a comment?

It just checks the string we give it to see if it's a valid phone number and gives us some useful methods like .valid, .country ..etc. I've also added a comment.

luucvanderzee · 2026-07-10T17:51:20Z

+  const [screenReaderMessage, setScreenReaderMessage] = useState<string>('');
+  const { formatMessage } = useIntl();
+
+  useEffect(() => {


Instead of this useEffect and state variable, can't you just render the screenReaderMessage conditionally without adding extra state?

This useEffect is just copied from the email approach to be honest. It seems to be part of a relatively new change for a11y.

Ok, let's make a note to replace it in both places because I can't really see any reason for this to work like this

jamesspeake

Looks good. A few comments to look at and I've not looked in quite as much detail at the FE as it seemed a lot of reusing the same patterns.

Not sure what the e2e coverage is like for email campaigns - Assume you've tested the email campaigns interface well given how much refactoring there has been there with name changes / endpoint changes.

Have the changes to the consent screen been run past product? From reading the code it seems that even if it's turned off, there is a small change to the layout?

jamesspeake · 2026-07-13T07:48:31Z

+
+    new_phone = request_code_phone_change_params[:new_phone]
+    if new_phone.blank?
+      render json: { errors: { new_phone: [{ error: 'cannot be blank' }] } }, status: :unprocessable_entity


Not sure if we use these on the FE to display messages, but usually I'd make these errors identifiers that can be used to map to translations eg cannot_be_blank

We indeed do, it's just that it was pre existing and I just reused it. There are others like it like this and would probably be better to solve in a different PR.

jamesspeake · 2026-07-13T07:53:01Z

+    ActiveRecord::Base.transaction do
+      user.update!(new_phone: new_phone)
+      confirmation = user.new_phone_confirmation || user.create_new_phone_confirmation!
+      confirmation.reset_code!
+      campaign = EmailCampaigns::Campaigns::NewPhoneConfirmation.first_or_create!
+      EmailCampaigns::DeliveryService.new.send_now_to_user(campaign, user, { code: confirmation.code })
+
+      confirmation.update!(code_sent_at: Time.zone.now)
+      ExpireConfirmationCodeOrDeleteJob.set(
+        wait_until: confirmation.expiration_at
+      ).perform_later(
+        user.id,
+        NewPhoneConfirmation.name,
+        confirmation.code
+      )
+      LogActivityJob.perform_later(user, 'received_confirmation_code', user, Time.now.to_i, payload: { new_phone: new_phone })
+    end


If it fails, the whole job should get retried though? If it mirrors email then probably fine as it is and low risk that it will fail as any failure in send_now_to_user I think will just be written to the delivery record

jamesspeake · 2026-07-13T08:38:35Z

I think this works in a slightly different way to Hooks::MailgunEventsController as that one has to switch tenants, but worth a quick look to see if it makes sense for code to maybe share the same language eg Sms::Hooks::EventsController - are these referred to as webhooks? Not a blocker, but feels we're describing both in different ways

I agree, fixed on this commit.

jamesspeake · 2026-07-13T09:30:27Z

+          key={deliveries.data.map((delivery) => delivery.id).join()}
+        >
+          {deliveries.data.map((delivery) => (
+            <TableRow delivery={delivery} key={delivery.id} />


Probably not a conversation for this PR, but wonder why (from an accessibility perspective) we don't use HTML tables where we obviously have tabular data

…ookup

…eQuery

…orator

…itizenlab into TAN-8120-sms-campaigns

…AN-8120-sms-campaigns

AchrafGoVocal and others added 30 commits May 22, 2026 17:45

[TAN-7844] Basic sms service

9fdce32

[TAN-7844] Basic SMS flow

18d958f

[TAN-7844] Refactor sms structure

4f195bb

[TAN-7844] Add callback to twilio

ff1974e

[TAN-7844] Refactor namespace

63b971a

[TAN-7844] Remove aws

4bc1a54

[TAN-7844] Remove aws references from env files

255392b

[TAN-7844] Remove addition of phone number

d1e76d8

[TAN-7844] Fix status overlap

2dfd6ed

[TAN-7844] Revert aws sdk core change

41bb141

[TAN-7844] Fix delivery status spec

d70615e

Merge branch 'master' of github.com:CitizenLabDotCo/citizenlab into T…

a13de6e

…AN-7844-basic-sms-setup

[TAN-7844] Fix structure sql

e71cec8

Merge branch 'master' of github.com:CitizenLabDotCo/citizenlab into T…

7725a0e

…AN-7844-basic-sms-setup

[TAN-7844] Add the phone number to the user model again

a25f0d4

[TAN-7844] Replace phone normalization class with proper gem

f930f9e

[TAN-7844] Add feature flag with settings

19456e0

Merge branch 'master' of github.com:CitizenLabDotCo/citizenlab into T…

521409f

…AN-7844-basic-sms-setup

[TAN-7844] Add campaign id to the sms delivery table

b0f62b7

[TAN-7922] Base manual sms setup

dd400b7

[TAN-7922] Add sms campaign ui

ce60b24

[TAN-7922] Add a title to the sms campaigns

9d58fe4

[TAN-7921] Add phone confirmation to the backend

3c50aef

[TAN-7921] Add basic UI to the frontend

a67ad3d

Translations updated by CI (extract-intl)

fd8982c

Translations updated by CI (extract-intl)

70dc67d

[TAN-7844] Remove unused controller

0ee5d51

[TAN-7844] Remove provider from send_job and rename send method

31076e6

[TAN-7844] Remove dangling SMS routes

6185ec5

Merge branch 'TAN-7844-basic-sms-setup' of github.com:CitizenLabDotCo…

b9f8c43

…/citizenlab into TAN-7922-add-sms-manual-campaigns

[TAN-8120] Harden SMS delivery callbacks and prevent duplicate phone …

9c3b38a

…code requests

AchrafGoVocal requested a review from luucvanderzee July 8, 2026 16:28

luucvanderzee requested changes Jul 10, 2026

View reviewed changes

jamesspeake approved these changes Jul 13, 2026

View reviewed changes

AchrafGoVocal added 7 commits July 13, 2026 12:49

[TAN-8120] Apply suggested change

05f0f5b

[TAN-8120] Hoist claim-token completion out of confirm_user

82258b7

[TAN-8120] Rename campaign_param methods for clarity

c84c24e

[TAN-8120] Fix wrong import location

06ee523

[TAN-8120] Replace interpolated message keys with a STATUS_MESSAGES l…

40f470e

…ookup

[TAN-8120] Add a comment to clarify functionnality

5a5bbf6

[TAN-8120] Paginate SMS campaigns with useQuery instead of useInfinit…

39f456b

…eQuery

AchrafGoVocal added the waiting for translations label Jul 14, 2026

cl-dev-bot and others added 12 commits July 14, 2026 14:25

Machine translations for 1885 strings

8687176

[TAN-8120] Move sms_deliveries association to EmailCampaigns::UserDec…

5c82b7b

…orator

Merge branch 'TAN-8120-sms-campaigns' of github.com:CitizenLabDotCo/c…

892e178

…itizenlab into TAN-8120-sms-campaigns

[TAN-8120] Fix wrong translation

52cfb7e

Translations updated by CI (extract-intl)

5938227

Machine translations for 29 strings

6da0bcf

Machine translations for 29 strings

36a0260

[TAN-8120] Remove unused key

6359fb1

[TAN-8120] Mark SMS settings as not ready for use

d890064

Translations updated by CI (extract-intl)

6e96f33

[TAN-8120] Move SMS status callbacks to Hooks::Sms::EventsController

709f265

Merge branch 'TAN-8120-sms-campaigns' of github.com:CitizenLabDotCo/c…

c98bf4b

…itizenlab into TAN-8120-sms-campaigns

AchrafGoVocal requested a review from luucvanderzee July 14, 2026 15:59

AchrafGoVocal and others added 4 commits July 14, 2026 18:06

Merge branch 'master' of github.com:CitizenLabDotCo/citizenlab into T…

42bc019

…AN-8120-sms-campaigns

[TAN-8120] Revert unintended devcontainer.json reformat

a8eb260

Update da-DK translations (65 changes)

870bb63

Update nl-NL translations (73 changes)

74240ec

luucvanderzee approved these changes Jul 14, 2026

View reviewed changes

Uh oh!

Conversation

AchrafGoVocal commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Added

Changed

Technical

For translators

Uh oh!

luucvanderzee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesspeake left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AchrafGoVocal Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AchrafGoVocal commented Jul 6, 2026 •

edited

Loading

AchrafGoVocal Jul 14, 2026 •

edited

Loading