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

Add form load time to xforms and es forms mapping #34824

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
32 changes: 20 additions & 12 deletions corehq/apps/app_manager/xform.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
SCHEDULE_UNSCHEDULED_VISIT,
USERCASE_ID,
)
from corehq.apps.app_manager.xpath import XPath, UsercaseXPath
from corehq.apps.app_manager.xpath import XPath
from corehq.apps.formplayer_api.exceptions import FormplayerAPIException
from corehq.toggles import DONT_INDEX_SAME_CASETYPE, NAMESPACE_DOMAIN, SAVE_ONLY_EDITED_FORM_FIELDS
from corehq.util.view_utils import get_request
Expand All @@ -41,7 +41,7 @@
XFormValidationFailed,
DangerousXmlException,
)
from .suite_xml.xml_models import Instance

from .xpath import CaseIDXPath, QualifiedScheduleFormXPath, session_var

VALID_VALUE_FORMS = ('image', 'audio', 'video', 'video-inline', 'markdown')
Expand Down Expand Up @@ -407,7 +407,7 @@ def elem(self):
'case_id': '',
'date_modified': '',
'user_id': '',
}, nsmap={
}, nsmap={
None: namespaces['cx2'][1:-1]
})

Expand Down Expand Up @@ -597,8 +597,10 @@ def add_index_ref(self, reference_id, case_type, ref, relationship='child'):


def autoset_owner_id_for_open_case(actions):
return not ('update_case' in actions and
'owner_id' in actions['update_case'].update)
return not (
'update_case' in actions
and 'owner_id' in actions['update_case'].update
)


def autoset_owner_id_for_subcase(subcase):
Expand Down Expand Up @@ -729,7 +731,7 @@ def odk_intents(self):
def text_references(self, lang=None):
# Accepts lang param for consistency with image_references, etc.,
# but current text references aren't language-specific
nodes = self.findall('{h}head/{odk}intent[@class="org.commcare.dalvik.action.PRINT"]/{f}extra[@key="cc:print_template_reference"]')
nodes = self.findall('{h}head/{odk}intent[@class="org.commcare.dalvik.action.PRINT"]/{f}extra[@key="cc:print_template_reference"]') # noqa
return list(set(n.attrib.get('ref').strip("'") for n in nodes))

def image_references(self, lang=None):
Expand Down Expand Up @@ -1117,7 +1119,7 @@ def _get_select_question_option(item):
if is_group and not include_groups:
continue

if node.tag_name == 'trigger'and not include_triggers:
if node.tag_name == 'trigger' and not include_triggers:
continue

if (exclude_select_with_itemsets and cnode.data_type in ['Select', 'MSelect']
Expand Down Expand Up @@ -1219,7 +1221,9 @@ def _get_select_question_option(item):
if data_node.tag_name == 'entry':
parent = next(data_node.xml.iterancestors())
if len(parent):
is_stock_element = any([namespace == COMMTRACK_REPORT_XMLNS for namespace in parent.nsmap.values()])
is_stock_element = any(
[namespace == COMMTRACK_REPORT_XMLNS for namespace in parent.nsmap.values()]
)
if is_stock_element:
question.update({
"stock_entry_attributes": dict(data_node.xml.attrib),
Expand Down Expand Up @@ -1504,6 +1508,7 @@ def _add_meta_2(self, form):
'{orx}instanceID',
'{cc}appVersion',
'{orx}drift',
'{orx}formLoadTime',
)
if form.get_auto_gps_capture():
tags += ('{cc}location',)
Expand Down Expand Up @@ -1596,8 +1601,8 @@ def add_instance(self, id, src):
"""
instance_xpath = 'instance[@id="%s"]' % id
conflicting = (
self.model_node.find('{f}%s' % instance_xpath).exists() or
self.model_node.find(instance_xpath).exists()
self.model_node.find('{f}%s' % instance_xpath).exists()
or self.model_node.find(instance_xpath).exists()
)
if not conflicting:
# insert right after the main <instance> block
Expand Down Expand Up @@ -1938,8 +1943,10 @@ def check_case_type(action):
)

module = form.get_module()
has_schedule = (module.has_schedule and getattr(form, 'schedule', False) and form.schedule.enabled and
getattr(form.get_phase(), 'anchor', False))
has_schedule = (
module.has_schedule and getattr(form, 'schedule', False) and form.schedule.enabled
and getattr(form.get_phase(), 'anchor', False)
)
last_real_action = next(
(action for action in reversed(form.actions.load_update_cases)
if not (action.auto_select) and action.case_type == module.case_type),
Expand Down Expand Up @@ -2146,6 +2153,7 @@ def get_scheduler_case_updates(self):
def _add_scheduler_case_update(self, case_type, case_property):
self._scheduler_case_updates[case_type].add(case_property)


VELLUM_TYPES = {
"AndroidIntent": {
'tag': 'input',
Expand Down
3 changes: 3 additions & 0 deletions corehq/apps/es/mappings/xform_mapping.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@
},
"username": {
"type": "keyword"
},
"formLoadTime": {
"type": "keyword"
Copy link
Contributor

Choose a reason for hiding this comment

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

@AmitPhulera
Just confirming that this is the right type for this?

This value is going to be in miliseconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AmitPhulera suggested that we think about storing this as a date field. I'm quickly finding out what the timeStart refers to. If it refers to the timestamp a user clicked to open a form, we could store the formLoadTime as a date and not a difference. The benefit of doing this is that we could later do all sorts of date queries on this field if we wanted to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @Charl1996
Whats the challenge with how it is currently?

I don't believe we can do this. I believe we need the time it took to load the form. I don't think we can get that using timeStart and any other timestamp.

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that we are storing epoch time in formLoadTime, Is that correct. If so that is a essentially a date field so it should be treated as date in the mappings unless there is a strong reason to keep it as a keyword.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AmitPhulera

My understanding is that we are storing epoch time in formLoadTime

The value stored in this field is something like "230" or "190", which denotes the duration in milliseconds it took to load the form. It's not epoch time

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh! I see. Feel free to discard my suggestion about storing it as date.
Another question, why are we not storing it as integer then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm, I suppose integer would be better, yes.

}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 4.2.11 on 2024-06-25 10:17

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('form_processor', '0096_add_partial_index_cases'),
]

operations = [
migrations.AddField(
model_name='xforminstance',
name='form_load_time',
field=models.CharField(blank=True, max_length=8, null=True),
),
]
3 changes: 3 additions & 0 deletions corehq/form_processor/models/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,7 @@ class XFormInstance(PartitionedModel, models.Model, RedisLockableMixIn,
time_start = models.DateTimeField(null=True, blank=True)
commcare_version = models.CharField(max_length=8, blank=True, null=True)
app_version = models.PositiveIntegerField(null=True, blank=True)
form_load_time = models.CharField(max_length=8, null=True, blank=True)

def __init__(self, *args, **kwargs):
super(XFormInstance, self).__init__(*args, **kwargs)
Expand Down Expand Up @@ -805,6 +806,7 @@ class XFormPhoneMetadata(jsonobject.JsonObject):
<Meta>
<timeStart />
<timeEnd />
<formLoadTime />
<instanceID />
<userID />
<deviceID />
Expand All @@ -828,6 +830,7 @@ class XFormPhoneMetadata(jsonobject.JsonObject):
username = jsonobject.StringProperty()
appVersion = jsonobject.StringProperty()
location = GeoPointProperty()
formLoadTime = jsonobject.StringProperty()

@property
def commcare_version(self):
Expand Down
1 change: 1 addition & 0 deletions migrations.lock
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,7 @@ form_processor
0094_add_partial_index_xforms
0095_remove_xforminstance_deleted_state
0096_add_partial_index_cases
0097_xforminstance_form_load_time
formplayer_api
0001_drop_old_tables
generic_inbound
Expand Down