Page MenuHomePhabricator
Differential D19982

Add infrastructure for sending SMS via AWS SNS
ClosedPublic
Actions

Authored by amckinley on Jan 16 2019, 7:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Feb 13, 7:02 PM
Unknown Object (File)
Sun, Feb 9, 6:56 AM
Unknown Object (File)
Sun, Feb 9, 6:56 AM
Unknown Object (File)
Sun, Feb 9, 6:56 AM
Unknown Object (File)
Sun, Feb 9, 6:56 AM
Unknown Object (File)
Sun, Feb 9, 6:56 AM
Unknown Object (File)
Sun, Feb 9, 6:56 AM
Unknown Object (File)
Sun, Feb 9, 6:56 AM
View All Files
Subscribers
Korvin

Details

Reviewers
epriestley
Maniphest Tasks
T920: Provide SMS Support
T13235: Replace "SimpleEmailService" with a subclass of "PhutilAWSFuture" that surfaces errors more clearly
Commits
rPe72684a4ba5d: Add infrastructure for sending SMS via AWS SNS
Summary

Ref T920. Ref T13235. This adds a Future, similar to TwilioFuture, for interacting with Amazon's SNS service.

Also updates the documentation.

Also makes the code consistent with the documentation by accepting a media argument.

Test Plan

Clicked the "send test message" button from the Settings UI.

Diff Detail

Repository
rP Phabricator
Branch
sms-redux
Lint
Lint Errors
SeverityLocationCodeMessage
Error/Users/amckinley/src/phacility/core/lib/libphutil/src/filesystem/Filesystem.php:428PHL1Unknown Symbol
Error/Users/amckinley/src/phacility/core/lib/libphutil/src/filesystem/Filesystem.php:568PHL1Unknown Symbol
Unit
Tests Passed
Build Status
Buildable 21677
Build 29561: Run Core Tests
Build 29560: arc lint + arc unit

Unit TestsFailed
View All

TimeTest
141 msPhabricatorAuthInviteTestCase::Unknown Unit Message ("")
Assertion failed, expected values to be equal (at PhabricatorAuthInviteTestCase.php:303): user=out, verified=0, primary=0, should_verify=0, exception=No config value specified for key 'amazon-s3.access-key'. Expected: 'PhabricatorAuthInviteVerifyException' Actual: 'Exception'
93 msPhabricatorAuthPasswordTestCase::Unknown Unit Message ("")
EXCEPTION (Exception): No config value specified for key 'amazon-s3.access-key'. #0 /core/data/drydock/workingcopy-85/repo/phabricator/src/applications/files/engine/PhabricatorS3FileStorageEngine.php(29): PhabricatorEnv::getEnvConfig('amazon-s3.acces...') #1 /core/data/drydock/workingcopy-85/repo/phabricator/src/applications/files/engine/PhabricatorFileStorageEngine.php(264): PhabricatorS3FileStorageEngine->canWriteFiles()
243 msPhabricatorAuthPasswordTestCase::Unknown Unit Message ("")
EXCEPTION (Exception): No config value specified for key 'amazon-s3.access-key'. #0 /core/data/drydock/workingcopy-85/repo/phabricator/src/applications/files/engine/PhabricatorS3FileStorageEngine.php(29): PhabricatorEnv::getEnvConfig('amazon-s3.acces...') #1 /core/data/drydock/workingcopy-85/repo/phabricator/src/applications/files/engine/PhabricatorFileStorageEngine.php(264): PhabricatorS3FileStorageEngine->canWriteFiles()
134 msPhabricatorAuthSSHKeyTestCase::Unknown Unit Message ("")
EXCEPTION (Exception): No config value specified for key 'amazon-s3.access-key'. #0 /core/data/drydock/workingcopy-85/repo/phabricator/src/applications/files/engine/PhabricatorS3FileStorageEngine.php(29): PhabricatorEnv::getEnvConfig('amazon-s3.acces...') #1 /core/data/drydock/workingcopy-85/repo/phabricator/src/applications/files/engine/PhabricatorFileStorageEngine.php(264): PhabricatorS3FileStorageEngine->canWriteFiles()
321 msPhabricatorChangeParserTestCase::Unknown Unit Message ("")
EXCEPTION (Exception): No config value specified for key 'amazon-s3.access-key'. #0 /core/data/drydock/workingcopy-85/repo/phabricator/src/applications/files/engine/PhabricatorS3FileStorageEngine.php(29): PhabricatorEnv::getEnvConfig('amazon-s3.acces...') #1 /core/data/drydock/workingcopy-85/repo/phabricator/src/applications/files/engine/PhabricatorFileStorageEngine.php(264): PhabricatorS3FileStorageEngine->canWriteFiles()
View Full Test Results (23 Failed · 339 Passed)

Event Timeline

amckinley created this revision.Jan 16 2019, 7:09 PM
Harbormaster completed remote builds in B21537: Diff 47684.Jan 16 2019, 7:11 PM
amckinley requested review of this revision.Jan 16 2019, 7:11 PM
Herald added a subscriber: Korvin. · View Herald TranscriptJan 16 2019, 7:11 PM
amckinley planned changes to this revision.Jan 16 2019, 7:11 PM
amckinley mentioned this in T13235: Replace "SimpleEmailService" with a subclass of "PhutilAWSFuture" that surfaces errors more clearly.
amckinley updated this revision to Diff 47824.Jan 24 2019, 12:05 AM

Clean up to work like TwilioFuture.

Harbormaster failed remote builds in B21677: Diff 47824!Jan 24 2019, 12:06 AM
amckinley updated this revision to Diff 47825.Jan 24 2019, 12:09 AM

Restore deleted file.

amckinley edited the summary of this revision. (Show Details)Jan 24 2019, 12:10 AM
amckinley edited the test plan for this revision. (Show Details)
Harbormaster completed remote builds in B21678: Diff 47825.Jan 24 2019, 12:11 AM
amckinley updated this revision to Diff 47826.Jan 24 2019, 12:12 AM

Correct link.

epriestley accepted this revision.Jan 24 2019, 12:12 AM
epriestley added inline comments.
src/applications/metamta/future/PhabricatorAmazonSNSFuture.php
5

Is this actually used?

6

Doesn't necessarily need to be in this diff, but can we put this in PhutilAWSFuture?

src/docs/user/configuration/configuring_outbound_email.diviner
100

(I'll hit this document a bit more broadly for SMS in general at some point, too.)

216

Link to SES instead of SNS?

220

"to the SES configuration"?

src/infrastructure/cluster/config/PhabricatorClusterMailersConfigType.php
48

Ah, thanks.

This revision is now accepted and ready to land.Jan 24 2019, 12:12 AM
amckinley updated this revision to Diff 47827.Jan 24 2019, 12:13 AM

Typo fix.

Harbormaster completed remote builds in B21679: Diff 47826.Jan 24 2019, 12:13 AM
amckinley marked 2 inline comments as done.Jan 24 2019, 12:15 AM
amckinley added inline comments.
src/applications/metamta/future/PhabricatorAmazonSNSFuture.php
5

If I implemented getProxiedFuture correctly, I think so?

6

Yeah, good idea.

src/docs/user/configuration/configuring_outbound_email.diviner
216

Hahah I noticed both of these right after submitting.

Harbormaster completed remote builds in B21680: Diff 47827.Jan 24 2019, 12:15 AM
epriestley added a comment.Jan 24 2019, 12:17 AM

Oh, I see what's going on with parameters, the base class is just a little bit unusual. Carry on.

Closed by commit rPe72684a4ba5d: Add infrastructure for sending SMS via AWS SNS (authored by amckinley). · Explain WhyJan 24 2019, 12:30 AM
This revision was automatically updated to reflect the committed changes.
amckinley marked an inline comment as done.

Revision Contents
Changeset List

PathSize
scripts/
sms/
21 lines
src/
applications/
config/
option/
metamta/
adapter/
66 lines
future/
41 lines
docs/
user/
configuration/
10 lines
infrastructure/
cluster/
config/
1 line

Diff 47824

View Options

scripts/sms/manage_sms.php

Loading...
View Options

src/__phutil_library_map__.php

Loading...
View Options

src/applications/config/option/PhabricatorAWSConfigOptions.php

Loading...
View Options

src/applications/metamta/adapter/PhabricatorMailAmazonSNSAdapter.php

Loading...
View Options

src/applications/metamta/future/PhabricatorAmazonSNSFuture.php

Loading...
View Options

src/docs/user/configuration/configuring_outbound_email.diviner

Loading...
View Options

src/infrastructure/cluster/config/PhabricatorClusterMailersConfigType.php

Loading...
Phabricator · Built by Phacility