Skip to content

feat: geosms adapter - #64

Merged
abnegate merged 18 commits into
mainfrom
feat-geosms-adapter
Nov 10, 2023
Merged

feat: geosms adapter#64
abnegate merged 18 commits into
mainfrom
feat-geosms-adapter

Conversation

@loks0n

@loks0nloks0n commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

GEO aware SMS adapter that has Twilio as a global default but can also delegate work to local providers.

$sms = new GEOSMS(new Twilio(...)); // Twilio as default fallback
$sms
->setLocal('+91', new MSG91())
->setLocal('+44', new Vodafone())

loks0n
loks0n commented Nov 7, 2023
View reviewed changes
Comment threadtests/e2e/Email/SendgridTest.php
$this->assertEquals($response, '');
*/

$this->markTestSkipped('Sendgrid: Authenticated user is not authorized to send mail');

@loks0nloks0nNov 7, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to fix our Sendgrid setup

@abnegateabnegateNov 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christyjacob4 Do we have a team test account set up for Sendgrid? I think tests here are still using a personal account

loks0n
loks0n commented Nov 7, 2023
View reviewed changes
Comment threadtests/e2e/SMS/Msg91Test.php Outdated
// $this->assertEquals('success', $result['type']);

$this->markTestSkipped('Msg91 requires business verification to use template and SMS api.');
$this->assertEquals('success', $result['type']);

@loks0nloks0nNov 7, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is fixed now we are verified

@christyjacob4christyjacob4Nov 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://api.msg91.com/apidoc/textsms/send-sms-flow.php
I removed this check since the message property is not really the content instead some sort of an ID that we cannot use for assertion

eldadfux
eldadfux reviewed Nov 7, 2023
View reviewed changes
Comment threadsrc/Utopia/Messaging/Adapters/SMS/GEOSMS.php
@loks0n
loks0n requested a review from eldadfuxNovember 7, 2023 15:08
stnguyen90
stnguyen90 reviewed Nov 7, 2023
View reviewed changes
Comment threadsrc/Utopia/Messaging/Adapters/SMS/GEOSMS.php Outdated
Comment threadsrc/Utopia/Messaging/Adapters/SMS/GEOSMS.php Outdated
Comment threadsrc/Utopia/Messaging/Adapters/SMS/GEOSMS.php Outdated
christyjacob4
christyjacob4 reviewed Nov 7, 2023
View reviewed changes
Comment threadsrc/Utopia/Messaging/Adapters/SMS/GEOSMS.php Outdated
christyjacob4
christyjacob4 reviewed Nov 7, 2023
View reviewed changes
Comment threadsrc/Utopia/Messaging/Adapters/SMS/GEOSMS.php Outdated
abnegate
abnegate reviewed Nov 7, 2023
View reviewed changes
Comment threadsrc/Utopia/Messaging/Adapters/SMS/GEOSMS.php Outdated
Comment threadsrc/Utopia/Messaging/Adapters/SMS/GEOSMS.php Outdated
Comment threadsrc/Utopia/Messaging/Adapters/SMS/GEOSMS.php
Comment threadsrc/Utopia/Messaging/Adapters/SMS/GEOSMS.php Outdated
Comment threadtests/e2e/Email/SendgridTest.php
$this->assertEquals($response, '');
*/

$this->markTestSkipped('Sendgrid: Authenticated user is not authorized to send mail');

@abnegateabnegateNov 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christyjacob4 Do we have a team test account set up for Sendgrid? I think tests here are still using a personal account

Comment threadsrc/Utopia/Messaging/Adapters/SMS/GEOSMS.php Outdated
christyjacob4
christyjacob4 reviewed Nov 8, 2023
View reviewed changes
Comment threadsrc/Utopia/Messaging/Adapters/SMS/GEOSMS.php Outdated
@stnguyen90stnguyen90 mentioned this pull request Nov 8, 2023
Closed
@abnegate
abnegate merged commit 8ba50f7 into mainNov 10, 2023
@abnegate
abnegate deleted the feat-geosms-adapter branch November 10, 2023 03:26
This was referenced Dec 1, 2023
Closed
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christyjacob4christyjacob4christyjacob4 left review comments
@eldadfuxeldadfuxeldadfux approved these changes
@abnegateabnegateabnegate approved these changes
@wesswessAwaiting requested review from wess
@stnguyen90stnguyen90Awaiting requested review from stnguyen90

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@loks0n@eldadfux@stnguyen90@abnegate@christyjacob4