Skip to content

[Notifier] [SMSBiuras] true/false mismatch for test_mode option #48262

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

Merged
merged 1 commit into from
Nov 22, 2022
Merged

[Notifier] [SMSBiuras] true/false mismatch for test_mode option #48262

merged 1 commit into from
Nov 22, 2022

Conversation

StaffNowa
Copy link
Contributor

@StaffNowa StaffNowa commented Nov 20, 2022

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

Moved from LightSMS service into SMSBiuras and found an issue.
SMSBIURAS_DSN=smsbiuras://UID:API_KEY@default?from=FROM&test_mode=0

Documentation is good but in the code bug. If test_mode = 0 - does not send SMS. If test_mode = 1 - sends SMS.
Boolean true | false mismatch.

SMSBiuras API documentation https://docs.smsbiuras.lt/

@@ -85,7 +85,7 @@ protected function doSend(MessageInterface $message): SentMessage
'apikey' => $this->apiKey,
'message' => $message->getSubject(),
'from' => $this->from,
'test' => $this->testMode ? 0 : 1,
'test' => !$this->testMode ? 0 : 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'test' => !$this->testMode ? 0 : 1,
'test' => $this->testMode ? 1 : 0,

@OskarStark OskarStark changed the title SMSBiuras Notifier bugfix. test_mode = 0 (disabled), test_mode = 1 (enabled) [Notigier][SMSBiurs] true/false mismatch for test_mode option Nov 20, 2022
@OskarStark
Copy link
Contributor

Could you please add test case to avoid further regressions?

@OskarStark OskarStark changed the title [Notigier][SMSBiurs] true/false mismatch for test_mode option [Notigier][SMSBiuras] true/false mismatch for test_mode option Nov 20, 2022
@StaffNowa
Copy link
Contributor Author

Could you please add test case to avoid further regressions?

Added tests for test_mode

@OskarStark
Copy link
Contributor

And please make fabbot happy 😄

@StaffNowa
Copy link
Contributor Author

And please make fabbot happy 😄

Fabbot happy too :D By the way, maybe I need to create for another branches changes to next up releases?

@StaffNowa StaffNowa changed the title [Notigier][SMSBiuras] true/false mismatch for test_mode option [Notifier][SMSBiuras] true/false mismatch for test_mode option Nov 21, 2022
@OskarStark
Copy link
Contributor

No, we will handle the upmerge of your PR into newer branches 👌🏻

@carsonbot carsonbot changed the title [Notifier][SMSBiuras] true/false mismatch for test_mode option [Notifier] [SMSBiuras] true/false mismatch for test_mode option Nov 21, 2022
@fabpot
Copy link
Member

fabpot commented Nov 22, 2022

Thank you @StaffNowa.

@fabpot fabpot merged commit 3f6d50f into symfony:5.4 Nov 22, 2022
OskarStark pushed a commit to OskarStark/symfony that referenced this pull request Nov 22, 2022
…`test_mode` option (StaffNowa)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[Notifier] [SMSBiuras] `true`/`false` mismatch for `test_mode` option

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

Moved from LightSMS service into SMSBiuras and found an issue.
SMSBIURAS_DSN=smsbiuras://UID:API_KEY@default?from=FROM&test_mode=0

Documentation is good but in the code bug. If test_mode = 0 - does not send SMS. If test_mode = 1 - sends SMS.
Boolean true | false mismatch.

SMSBiuras API documentation https://docs.smsbiuras.lt/

Commits
-------

cd7e4d6 [Notifier] [SMSBiuras] `true`/`false` mismatch for `test_mode` option
OskarStark added a commit to OskarStark/symfony that referenced this pull request Nov 22, 2022
OskarStark added a commit to OskarStark/symfony that referenced this pull request Nov 22, 2022
OskarStark added a commit to OskarStark/symfony that referenced this pull request Nov 22, 2022
fabpot added a commit that referenced this pull request Nov 23, 2022
…OskarStark)

This PR was merged into the 5.4 branch.

Discussion
----------

[Notifier] [SmsBiuras] Simplify test and data provider

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| License       | MIT
| Doc PR        |

Follows
 * #48262

Commits
-------

9221451 [Notifier][SmsBiuras] Simplify test and data provider
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.

6 participants