-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
Use Elasticsearch to fetch applications for exports #34266
Changes from 6 commits
76c004a
e3bfd41
5985875
e5a12fc
2350424
caefff8
c8361e0
2ce01d0
28e2d6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,9 +15,10 @@ | |
|
||
from corehq.apps.app_manager.analytics import get_exports_by_application | ||
from corehq.apps.app_manager.dbaccessors import get_app, get_apps_in_domain | ||
from corehq.apps.export.const import ALL_CASE_TYPE_EXPORT | ||
from corehq.apps.hqwebapp import crispy as hqcrispy | ||
from corehq.apps.registry.models import DataRegistry | ||
from corehq.apps.registry.utils import get_data_registry_dropdown_options | ||
from corehq.apps.hqwebapp import crispy as hqcrispy | ||
from corehq.apps.reports.analytics.esaccessors import get_case_types_for_domain | ||
from corehq.apps.userreports.app_manager.data_source_meta import ( | ||
DATA_SOURCE_TYPE_CASE, | ||
|
@@ -26,7 +27,6 @@ | |
) | ||
from corehq.apps.userreports.dbaccessors import get_datasources_for_domain | ||
from corehq.toggles import AGGREGATE_UCRS | ||
from corehq.apps.export.const import ALL_CASE_TYPE_EXPORT | ||
|
||
DataSource = collections.namedtuple('DataSource', ['application', 'source_type', 'source', 'registry_slug']) | ||
RMIDataChoice = collections.namedtuple('RMIDataChoice', ['id', 'text', 'data']) | ||
|
@@ -285,8 +285,10 @@ class ApplicationDataRMIHelper(object): | |
APP_TYPE_NONE = 'no_app' | ||
APP_TYPE_UNKNOWN = 'unknown' | ||
|
||
def __init__(self, domain, user, as_dict=True): | ||
def __init__(self, domain, project, user, as_dict=True): | ||
self.domain = domain | ||
self.domain_object = project | ||
|
||
self.user = user | ||
self.as_dict = as_dict | ||
self.form_labels = AppFormRMIPlaceholder( | ||
|
@@ -420,14 +422,16 @@ def _sort_key_form(form): | |
def _all_forms(self): | ||
forms = [] | ||
unknown_forms = [] | ||
for f in get_exports_by_form(self.domain): | ||
|
||
for f in get_exports_by_form(self.domain, use_es=self.domain_object.exports_use_elasticsearch): | ||
form = f['value'] | ||
if form.get('app_deleted') and not form.get('submissions'): | ||
continue | ||
if 'app' in form: | ||
form['has_app'] = True | ||
forms.append(form) | ||
else: | ||
elif not self.domain_object.exports_use_elasticsearch: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi Farid, can you explain what does it mean when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Offlining a response to this) |
||
# If the elasticsearch toggle is on, we don't care about forms without apps | ||
app_id = f['key'][1] or '' | ||
form['app'] = { | ||
'id': app_id | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
import uuid | ||
|
||
from django.test import TestCase | ||
from corehq.apps.app_manager.tests.util import delete_all_apps | ||
|
||
from couchforms.analytics import ( | ||
domain_has_submission_in_last_30_days, | ||
|
@@ -13,6 +14,7 @@ | |
get_number_of_forms_in_domain, | ||
) | ||
|
||
from corehq.apps.es.apps import app_adapter | ||
from corehq.apps.es.client import manager | ||
from corehq.apps.es.forms import form_adapter | ||
from corehq.apps.es.tests.utils import es_test | ||
|
@@ -34,7 +36,7 @@ | |
from testapps.test_pillowtop.utils import process_pillow_changes | ||
|
||
|
||
@es_test(requires=[form_adapter], setup_class=True) | ||
@es_test(requires=[form_adapter, app_adapter], setup_class=True) | ||
@disable_quickcache | ||
class ExportsFormsAnalyticsTest(TestCase, DocTestMixin): | ||
maxDiff = None | ||
|
@@ -46,25 +48,36 @@ def setUpClass(cls): | |
|
||
from corehq.apps.app_manager.models import Application, Form, Module | ||
delete_all_xforms() | ||
delete_all_apps() | ||
|
||
cls.domain = 'exports_forms_analytics_domain' | ||
cls.app_id_1 = 'a' + uuid.uuid4().hex | ||
cls.app_id_2 = 'b' + uuid.uuid4().hex | ||
cls.app_id_3 = 'c' + uuid.uuid4().hex | ||
cls.xmlns_1 = 'my://crazy.xmlns/' | ||
cls.xmlns_2 = 'my://crazy.xmlns/app' | ||
cls.xmlns_3 = 'my://crazy.xmlns/deleted-app' | ||
cls.apps = [ | ||
Application(_id=cls.app_id_2, domain=cls.domain, | ||
modules=[Module(forms=[Form(xmlns=cls.xmlns_2)])]) | ||
modules=[Module(forms=[Form(xmlns=cls.xmlns_2)])]), | ||
Application(_id=cls.app_id_3, domain=cls.domain, | ||
modules=[Module(forms=[Form(xmlns=cls.xmlns_3)])]) | ||
] | ||
for app in cls.apps: | ||
app.save() | ||
cls.apps[1].delete_app() | ||
cls.apps[1].save() | ||
cls.forms = [ | ||
create_form_for_test(domain=cls.domain, app_id=cls.app_id_1, xmlns=cls.xmlns_1, save=False), | ||
create_form_for_test(domain=cls.domain, app_id=cls.app_id_1, xmlns=cls.xmlns_1, save=False), | ||
create_form_for_test(domain=cls.domain, app_id=cls.app_id_2, xmlns=cls.xmlns_2, save=False), | ||
create_form_for_test(domain=cls.domain, app_id=cls.app_id_3, xmlns=cls.xmlns_3, save=False), | ||
] | ||
cls.error_forms = [create_form_for_test(domain=cls.domain, state=XFormInstance.ERROR, save=False)] | ||
cls.all_forms = cls.forms + cls.error_forms | ||
|
||
for app in cls.apps: | ||
app_adapter.index(app, refresh=True) | ||
for form in cls.all_forms: | ||
form_adapter.index(form, refresh=True) | ||
|
||
|
@@ -97,6 +110,8 @@ def test_get_form_analytics_metadata__app(self): | |
|
||
@flaky_slow | ||
def test_get_exports_by_form(self): | ||
# Call this twice, since the couchdb `update_after` to force couchdb to return the right results | ||
get_exports_by_form(self.domain) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does calling it twice actually force it, or does it just take a bit more time, making the second call a bit more likely to return the correct results? In other words, is this possibly a flaky test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it actually forces it. This is also a flaky test, but it was before I added this change (and is annotated as such) |
||
self.assertEqual(get_exports_by_form(self.domain), [{ | ||
'value': {'xmlns': 'my://crazy.xmlns/', 'submissions': 2}, | ||
'key': ['exports_forms_analytics_domain', self.app_id_1, | ||
|
@@ -110,6 +125,43 @@ def test_get_exports_by_form(self): | |
'app_deleted': False, 'submissions': 1}, | ||
'key': ['exports_forms_analytics_domain', self.app_id_2, | ||
'my://crazy.xmlns/app'] | ||
}, { | ||
'value': { | ||
'xmlns': 'my://crazy.xmlns/deleted-app', | ||
'form': {'name': {}, 'id': 0}, | ||
'app': {'langs': [], 'name': None, 'id': self.app_id_3}, | ||
'module': {'name': {}, 'id': 0}, | ||
'app_deleted': True, 'submissions': 1, | ||
}, | ||
'key': ['exports_forms_analytics_domain', self.app_id_3, | ||
'my://crazy.xmlns/deleted-app'] | ||
}]) | ||
|
||
def test_get_exports_by_form_es(self): | ||
self.assertEqual(get_exports_by_form(self.domain, use_es=True), [{ | ||
'value': {'xmlns': 'my://crazy.xmlns/', 'submissions': 2}, | ||
'key': ['exports_forms_analytics_domain', self.app_id_1, | ||
'my://crazy.xmlns/'] | ||
}, { | ||
'value': { | ||
'xmlns': 'my://crazy.xmlns/app', | ||
'form': {'name': {}, 'id': 0}, | ||
'app': {'langs': [], 'name': None, 'id': self.app_id_2}, | ||
'module': {'name': {}, 'id': 0}, | ||
'app_deleted': False, 'submissions': 1}, | ||
'key': ['exports_forms_analytics_domain', self.app_id_2, | ||
'my://crazy.xmlns/app'] | ||
}, { | ||
'value': { | ||
'xmlns': 'my://crazy.xmlns/deleted-app', | ||
'submissions': 1, | ||
'form': {'name': {}, 'id': 0}, | ||
'app': {'langs': [], 'name': None, 'id': self.app_id_3}, | ||
'module': {'name': {}, 'id': 0}, | ||
'app_deleted': True | ||
}, | ||
'key': ['exports_forms_analytics_domain', self.app_id_3, | ||
'my://crazy.xmlns/deleted-app'] | ||
}]) | ||
|
||
|
||
|
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.
Would it work to only pass in the domain object?
self.domain
could also be changed to a property that returnsself.domain_object.name