Skip to content

[Notifier][TurboSMS] Process partial accepted response from transport #58242

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.

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

Conversation

ZhukV
Copy link
Contributor

@ZhukV ZhukV commented Sep 12, 2024

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

TurboSMS can return null as message id, if sms not sent to recipient. Example:

{
   "response_code": 802,
   "response_status": "SUCCESS_MESSAGE_PARTIAL_ACCEPTED",
   "response_result": [
      {
         "phone": "recipient_1",
         "response_code": 406,
         "message_id": null,
         "response_status": "NOT_ALLOWED_RECIPIENT_COUNTRY"
      },
      {
         "phone": "recipient_2",
         "response_code": 0,
         "message_id": "f83f8868-5e46-c6cf-e4fb-615e5a293754",
         "response_status": "OK"
      }
   ]
}

And we receive error:

Symfony\Component\Notifier\Message\SentMessage::setMessageId(): Argument #1 ($id) must be of type string, null given, called in /code/vendor/symfony/turbo-sms-notifier/TurboSmsTransport.php on line 93

Symfony use only one phone number for sent, as result we check only first response_result.

@carsonbot carsonbot changed the title Correct process partial accepted response from TurboSMS transport. [Notifier] Correct process partial accepted response from TurboSMS transport. Sep 12, 2024
@OskarStark OskarStark changed the title [Notifier] Correct process partial accepted response from TurboSMS transport. [Notifier][TurboSMS] Process partial accepted response from transport Sep 12, 2024
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Bonus points, if we can get rid of the mocks and use JsonMockResponse in tests. Appreciated for the whole tests file 👍

@nicolas-grekas nicolas-grekas force-pushed the bugfix/notifier-turbosms-process-partial-accepted-errors branch from 8096577 to 932dbe3 Compare September 16, 2024 14:43
@nicolas-grekas
Copy link
Member

Thank you @ZhukV.

@nicolas-grekas nicolas-grekas merged commit 28b1709 into symfony:6.4 Sep 16, 2024
8 of 9 checks passed
fabpot added a commit that referenced this pull request Sep 18, 2024
This PR was squashed before being merged into the 7.2 branch.

Discussion
----------

[Notifier] [TurboSms] Better tests

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | Follows #58242
| License       | MIT

Still waiting for #58242 to get unmerged, to fix also the newly introduced test.

Commits
-------

553951e [Notifier] [TurboSms] Better tests
This was referenced Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants