-
Notifications
You must be signed in to change notification settings - Fork 24
[UPG] spp_sms: upgrade spp_sms to 17.0 #785
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 17.0 #785 +/- ##
==========================================
+ Coverage 75.67% 76.00% +0.32%
==========================================
Files 764 778 +14
Lines 20218 20659 +441
Branches 2501 2541 +40
==========================================
+ Hits 15300 15701 +401
- Misses 4382 4416 +34
- Partials 536 542 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reichie020212 please check the codecov report.
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reichie020212 check also the sonarqubecloud report.
…d enhance code readability
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
… handling, and clean up unused code
…egistrant type handling
spp_sms/__manifest__.py
Outdated
| "website": "https://github.com/OpenSPP/openspp-modules", | ||
| "license": "AGPL-3", | ||
| "depends": ["iap", "sms", "mass_mailing_sms", "g2p_registry_base", "g2p_programs"], | ||
| "development_status": "Beta", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Status should be alpha at this stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
spp_sms/tools/sms_api.py
Outdated
| error_messages = original_get_sms_api_error_messages(self) | ||
| error_messages["invalid_to_number"] = _("The number you're trying to reach is not correctly formatted.") | ||
| error_messages["invalid_from_number"] = _("The number you're trying to send from is not correctly formatted.") | ||
| error_messages["cannot_be_reached"] = _("The number you're trying to reach is not correctly formatted.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message is misleading. The message means that the SMS Gateway cannot reach the phone number, and that it might be incorrect. Better error message: "The number cannot be reached. Please check the number."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
spp_sms/tools/sms_api.py
Outdated
| result.append({"state": state, "credit": 0, "res_id": number["number"]}) | ||
| return result | ||
|
|
||
| # NOTE: Removing sns_amazon for now since boto3 is not compatible with the current version of urllib3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this only mentioned here and not in the PR conversation? This is something we should solve. When you say current version of urllib3, which version are you referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what i mean in this comment is the latest version. I haven't thoroughly investigating this since we're not using sms amazon. i'll try to dwell on this issue on a later time.
spp_sms/tools/sms_api.py
Outdated
| return result | ||
|
|
||
| # NOTE: Removing sns_amazon for now since boto3 is not compatible with the current version of urllib3 | ||
| # elif account[0].provider == "sns_amazon": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move all the AWS SNS-specific code to a separate function, i.e awssns_send_sms_batch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| account = self.env["iap.account"].search([("active_status", "=", True)]).get("sms") | ||
|
|
||
| if not account or account[0].provider != "sms_twilio": | ||
| return original_send_sms_batch(self, messages, delivery_reports_url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this SMS batch function handle AWS SNS messages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
…enspp-modules into 54038-spp_mis-upgrade
…o 54038-spp_mis-upgrade
|
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |



Why is this change needed?
spp_sms module is still in another repo and is still in 15.0
How was the change implemented?
Moved the spp_sms module to this repo and migrated it to 17.0
New unit tests
Unit tests executed by the author
How to test manually
Related links