Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Notifier] Add Sweego bridge #58322

Open
wants to merge 1 commit into
base: 7.2
Choose a base branch
from
Open

[Notifier] Add Sweego bridge #58322

wants to merge 1 commit into from

Conversation

welcoMattic
Copy link
Member

@welcoMattic welcoMattic commented Sep 19, 2024 •
edited by OskarStark
Loading

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues
License MIT

This PR folllows

This Bridge brings Webhook support for sent event for now. Others will follow in the future.
from parameter is handled on Sweego side (AFAIK, it means one from per API Key. @pydubreucq can you confirm my words?).

@carsonbot carsonbot added this to the 7.2 milestone Sep 19, 2024
@welcoMattic welcoMattic changed the title Add Sweego Notifier bridge [Notifier] Add Sweego Notifier bridge Sep 19, 2024
@welcoMattic welcoMattic changed the title [Notifier] Add Sweego Notifier bridge [Notifier] Add Sweego bridge Sep 19, 2024
OskarStark
OskarStark requested changes Sep 19, 2024
View reviewed changes
src/Symfony/Component/Notifier/Bridge/Sweego/CHANGELOG.md Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/Sweego/SweegoTransport.php.orig Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/Sweego/Webhook/SweegoRequestParser.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/Sweego/Webhook/SweegoRequestParser.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/Sweego/Webhook/SweegoRequestParser.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/Sweego/composer.json Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Exception/UnsupportedSchemeException.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Tests/Exception/UnsupportedSchemeExceptionTest.php Outdated Show resolved Hide resolved
@carsonbot carsonbot changed the title [Notifier] Add Sweego bridge Add Sweego bridge Sep 19, 2024
@carsonbot carsonbot changed the title Add Sweego bridge [Notifier] Add Sweego bridge Sep 19, 2024
@Wasta-Geek
Copy link

from parameter is handled on Sweego side (AFAIK, it means one from per API Key. @pydubreucq can you confirm my words?).

Hi, I'm working on Sweego project, to answer about this, indeed the from is managed on our side but it's not different for each API Key. An optional sender ID (alphanumeric sender) can be configured on Sweego App and when sending an SMS, the sender ID will be taken as sender; if missing, a short number will be used as sender

@welcoMattic
Copy link
Member Author

Hi, I'm working on Sweego project, to answer about this, indeed the from is managed on our side but it's not different for each API Key. An optional sender ID (alphanumeric sender) can be configured on Sweego App and when sending an SMS, the sender ID will be taken as sender; if missing, a short number will be used as sender

Hi @Wasta-Geek! Thanks for your advice about the Sender ID. Can you help me to implement it? I didn't find how/where to set this Sender ID in your documentation about SMS sending. We are close to finish this PR, let's do it together!

@Wasta-Geek
Copy link

Hi, I'm working on Sweego project, to answer about this, indeed the from is managed on our side but it's not different for each API Key. An optional sender ID (alphanumeric sender) can be configured on Sweego App and when sending an SMS, the sender ID will be taken as sender; if missing, a short number will be used as sender

Hi @Wasta-Geek! Thanks for your advice about the Sender ID. Can you help me to implement it? I didn't find how/where to set this Sender ID in your documentation about SMS sending. We are close to finish this PR, let's do it together!

Oops, I didn't see your question last time I checked the PR.
I wasn't clear enough maybe, sorry; but a client can create a sender ID on Sweego App page and it will be used for SMS sending automatically for countries supporting it but it's not something that can be configured on API side when sending a message so you have nothing to add on your side.

@welcoMattic
Copy link
Member Author

region and campaign_type parameter has been added to the DSN to let users set them.

@welcoMattic welcoMattic added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 3, 2024
Wasta-Geek
Wasta-Geek reviewed Oct 4, 2024
View reviewed changes
src/Symfony/Component/Notifier/Bridge/Sweego/SweegoTransport.php
'Api-Key' => $this->apiKey,
],
'json' => [
'recipients' => [
Copy link

Choose a reason for hiding this comment

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

After having the PR reviewed with my CTO, we saw that there was some missing optional fields

  1. bat [boolean]: enable test mode (no sms sent, it's for testing purpose)
  2. campaign_id [string]: used for tracking / filtering purpose on our platform; identity an SMS campaign and allow to see logs / stats only for this campaign
  3. shorten_urls [boolean]: True by default, we replaces all url in the SMS content by a shortened url version (reduce the characters of the sms)
  4. shorten_with_protocol [boolean]: True by default, add scheme to shortened url version
  5. template_id [UUID4] / variables [Dictionary [ string, string ]] : it could be used in replacement of the message_txt field. (only one of template_id / message_txt accepted). Users can creates templates on our platform that will contains variables placeholders and use the variables field to fill the placeholders with the given variables

Would it be possible to also add those fields ?

Copy link
Member

Choose a reason for hiding this comment

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

templates should not be added here. Using a template is totally different than passing the message text, and does not correspond to a usage of the notifier component (remember that this package is not a SDK for the full Sweego API but a bridge to make it easy to use the Notifier features with Sweego).

bat probably makes sense as an option in the DSN. The shorten_* settings might also be exposed in the DSN.

For campaign_id, I'm not sure what is the best mapping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@Wasta-Geek Wasta-Geek Wasta-Geek left review comments

@OskarStark OskarStark Awaiting requested review from OskarStark OskarStark is a code owner

Assignees
No one assigned
Labels
Feature Notifier ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Needs Review
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.
5 participants
@welcoMattic @Wasta-Geek @stof @OskarStark @carsonbot