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.

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

Sms improvements #13743

Open
wants to merge 9 commits into
base: 5.x
Choose a base branch
from
Open

Sms improvements #13743

wants to merge 9 commits into from

Conversation

escopecz
Copy link
Sponsor Member

@escopecz escopecz commented May 13, 2024

Q A
Bug fix? (use the a.b branch) [n]
New feature/enhancement? (use the a.x branch) [enhancement]
Deprecations? [ ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? [y]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed

Description:

This is an enhancement code that we needed to build a plugin that was adding some new tokens to SMS sending. It improves the code:

  1. Reuses code that was already there and removes duplicates (the owner token handling)
  2. Adds an extra SMS transport that can be used in tests. It simplifies some tests and enables testing the real sending without mocking.
  3. More functional tests were added.

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Test that you can send SMS via Twilio and the tokens are being replaced correctly.

dongilbert
dongilbert previously approved these changes May 13, 2024
Copy link
Member

@dongilbert dongilbert left a comment

Choose a reason for hiding this comment

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

lgtm

@escopecz escopecz added enhancement Any improvement to an existing feature or functionality sms Anything related to SMS unforking Used for PRs in the Acquia's unforking initiative code-review-passed PRs which have passed code review labels May 13, 2024
Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.78%. Comparing base (7f141f6) to head (f05b284).
Report is 2 commits behind head on 5.x.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #13743      +/-   ##
============================================
+ Coverage     61.51%   61.78%   +0.27%     
- Complexity    34068    34095      +27     
============================================
  Files          2241     2241              
  Lines        101852   101921      +69     
============================================
+ Hits          62651    62972     +321     
+ Misses        39201    38949     -252     
Files Coverage Δ
...ndles/LeadBundle/EventListener/OwnerSubscriber.php 97.01% <100.00%> (+0.86%) ⬆️
app/bundles/PageBundle/Model/TrackableModel.php 77.95% <ø> (ø)
...les/PluginBundle/Form/Type/FeatureSettingsType.php 74.07% <100.00%> (ø)
.../bundles/SmsBundle/EventListener/SmsSubscriber.php 83.78% <100.00%> (+23.78%) ⬆️
app/bundles/SmsBundle/Form/Type/SmsType.php 0.00% <ø> (ø)

... and 42 files with indirect coverage changes

@escopecz escopecz added the pending-test-confirmation PR's that require one test before they can be merged label May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review-passed PRs which have passed code review enhancement Any improvement to an existing feature or functionality pending-test-confirmation PR's that require one test before they can be merged sms Anything related to SMS unforking Used for PRs in the Acquia's unforking initiative
Projects
Status: ⏳︎ Needs 1 more test
Development

Successfully merging this pull request may close these issues.

None yet

2 participants