[UPG] spp_sms: upgrade spp_sms to 17.0 #785
Conversation
github-actions
bot
requested review from
emjay0921,
gonzalesedwin1123 and
jeremi
Codecov ReportAttention: 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:
|
…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
There was a problem hiding this comment.
Copilot reviewed 20 out of 28 changed files in this pull request and generated 2 comments.
Files not reviewed (8)
- spp_sms/README.rst: Language not supported
- spp_sms/i18n/ar.po: Language not supported
- spp_sms/i18n/ckb.po: Language not supported
- spp_sms/i18n/fr.po: Language not supported
- spp_sms/i18n/spp_sms.pot: Language not supported
- spp_sms/readme/DESCRIPTION.rst: Language not supported
- spp_sms/security/ir.model.access.csv: Language not supported
- spp_sms/static/description/index.html: Language not supported
spp_sms/models/mailing_mailing.py
Outdated
| elif self.mailing_registrant_type == "Program": | ||
| vals = [] | ||
| for rec in self.mailing_program_ids: | ||
| vals = self._get_enrolled_members(rec.program_id, "program_id", "program_membership_ids") |
There was a problem hiding this comment.
Is there a reason why we are not collecting all members in the for-loop and then making a single call to self._update_mailing_domain(vals)?
There was a problem hiding this comment.
Done fixing this in local, not yet pushed.
spp_sms/models/mailing_mailing.py
Outdated
| main_record = getattr(rec, record_field) | ||
| for rec_line in getattr(main_record, membership_field): | ||
| if rec_line.state == "enrolled": | ||
| if rec_line.partner_id.is_group: |
There was a problem hiding this comment.
This does not seem the handle the case of groups-of-groups?
There was a problem hiding this comment.
@kneckinator yes since the groups-of-groups concept comes from the module spp_registry_group_hierarchy which is not being used by spp_sms module. spp_sms module only uses the module g2p_registry_base.
There was a problem hiding this comment.
So what happens if spp_sms is installed together with spp_registry_group_hierarchy and the user intends to send an SMS to the highest group level?
It operates on the entity that is enrolled into a program/cycle, and it is my understanding that a "group of groups" can be enrolled into a program/cycle. Thus, the system needs to present an error message at least and check that the partner_id has is_registrant set to True.
There was a problem hiding this comment.
@kneckinator I pushed a new commit to accomodate this.
spp_sms/tools/sms_api.py
Outdated
|
|
||
|
|
||
| def _send_sms_batch(self, messages, delivery_reports_url=False): | ||
| account = self.env["iap.account"].search([("active_status", "=", True)]).get("sms") |
There was a problem hiding this comment.
The account name can only be set before changing the provider from Odoo IAP to one of the SMS providers. The name field in the Account information is hidden when one of the SMS providers are selected.
There was a problem hiding this comment.
Done fixing this. Added creation of data when installing spp_sms
spp_sms/tools/sms_api.py
Outdated
| def _send_sms_batch(self, messages, delivery_reports_url=False): | ||
| account = self.env["iap.account"].search([("active_status", "=", True)]).get("sms") | ||
|
|
||
| if not account or account[0].provider != "sms_twilio": |
There was a problem hiding this comment.
How will this work if multiple active SMS accounts have been defined? Is there a default order_by to ensure that the same account is used everytime?
What if both SNS and Twilio accounts have been defined?
Can you provide a setting somewhere so the user can select a single account (or if we want, multiple accounts) that will be used to send the SMS? This will also avoid a query like SELECT * FROM .... WHERE active_status = "true" ... in the .search above.
There was a problem hiding this comment.
Done. Added sequence field in the iap.account
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.
Status should be alpha at this stage.
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.
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."
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.
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.
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.
Please move all the AWS SNS-specific code to a separate function, i.e awssns_send_sms_batch.
| 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.
Can this SMS batch function handle AWS SNS messages?
…enspp-modules into 54038-spp_mis-upgrade
…o 54038-spp_mis-upgrade
|
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