Add Enterprise SMS Report #35437
Add Enterprise SMS Report #35437
Conversation
mjriley
added
the
product/admin
corehq/apps/enterprise/enterprise.py
Outdated
|
|
||
|
|
||
| class EnterpriseSMSReport(EnterpriseReport): | ||
| title = gettext_lazy('SMS Sent') |
There was a problem hiding this comment.
Change the title to align with the design doc
There was a problem hiding this comment.
Addressed in fc4fccd
| def __init__(self, account, couch_user, start_date=None, end_date=None, num_days=30): | ||
| super().__init__(account, couch_user) | ||
|
|
||
| if not end_date: |
There was a problem hiding this comment.
Can you make it as a util function so other tiles can reuse it?
There was a problem hiding this comment.
I believe I did this in the iterators PR, which is still in review. Re-adding it here would cause conflicts, so I'm going to wait until that PR is merged for this functionality
corehq/apps/enterprise/enterprise.py
Outdated
| @property | ||
| def headers(self): | ||
| headers = super().headers | ||
| headers = [_('Project Space Name'), _('# Sent'), _('# Received'), _('# Errors')] |
There was a problem hiding this comment.
Simplify the header to be Project Space
corehq/apps/enterprise/enterprise.py
Outdated
| .annotate(error_count=error_subquery) | ||
| .values_list('domain', 'sent_count', 'received_count', 'error_count') | ||
| ) | ||
| num_sent = sum([result['direction_count'] for result in results if result['direction'] == OUTGOING]) |
There was a problem hiding this comment.
Should we only include sms that is succesfully sent and received? If we don't charge them, then we should not include errored sms in num_sent and num_received
There was a problem hiding this comment.
total_for_domain should also align with the number in report, when people sum up num_sent for all domains
There was a problem hiding this comment.
Addressed in 1adcaf3
| REPORT_SLUG = EnterpriseReport.SMS | ||
|
|
||
| def get_report_task(self, request): | ||
| start_date = request.GET.get('startdate', None) |
|
@mjriley Can review again once the conflicts with master are resolved. Btw, do you plan to put it through QA? |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Product Description
Adds a new Enterprise SMS Report, including associated tile and API.
Technical Summary
Associated ticket: https://dimagi.atlassian.net/browse/SAAS-16131
At the moment, the new API is implemented using the old population logic, as the new iterator logic was only written for Elasticsearch queries and didn't take into account how to generate summary statistics. The intent is to eventually use the new iterator logic, but more work will need to be done to determine how best to support the SMS queries with that framework
Feature Flag
We mentioned we would prefer to do an iterative approach to these new tiles, so this is ready for release without a feature flag
Safety Assurance
Safety story
Tested locally -- generated the report, accessed the API through the browser, and accessed it through PowerBI. Verified that UI worked as expected.
Automated test coverage
None at the moment. As it stands, the code to be tested is the
EnterpriseSMSReportcode, but I'd like to eventually move the logic out of there into a query class and test against that.QA Plan
TBD
Rollback instructions
Labels & Review