Skip to content

Commit

Permalink
fix(OpenRosa): correct detection of root node tag name for XForm surv…
Browse files Browse the repository at this point in the history
…eys TASK-1258 (#5272)

### 📣 Summary
Fixes the detection of the root node tag name of the survey in the XForm
XML, respecting the name field in XLSForm settings

### 📖 Description
This bugfix addresses two issues caused by wrong detection of the root
node tag name in the "survey" section of XForm XML:
- an empty `formhub/uuid` on submission creation
- a 500 error when a form is open in Enketo (preview/collection) when a
disclaimer is set

The update ensures that the name field, if provided in the XLSForm
settings, is respected as the root node's tag name.


### 👀 Preview steps

Bug template:
1. Log in as a regular user
2. Create a project (add `name` field with custom value in the
`settings` worksheet)
3. Deploy and submit data
  1. 🔴 [on main] notice that this `formhub/uuid` is empty
  2. 🟢 [on PR] notice that this is `formhub/uuid` matches XForm uuid
  
1. Go to admin and add a disclaimer
2. Try to open the project in Enketo (preview and collection)
  1. 🔴 [on main] notice that it raises a 500 error
  2. 🟢 [on PR] notice that the form opens

### 💭 Notes
This PR relies on the fact that `id_string` and `name` are always
provided in the XForm.json field (according to existing data on
production servers).
In case of preview, If the `name` field is not supplied, the fallback
mechanism uses the asset's UID to determine the tag name (as it is in
`main`).
  • Loading branch information
noliveleger authored Nov 26, 2024
1 parent a11ca28 commit 0474aed
Show file tree
Hide file tree
Showing 8 changed files with 68 additions and 53 deletions.
12 changes: 12 additions & 0 deletions kobo/apps/openrosa/apps/logger/models/xform.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,18 @@ def url(self):
'download_xform', kwargs={'username': self.user.username, 'pk': self.pk}
)

@property
def xform_root_node_name(self):
"""
Retrieves the name of the XML tag representing the root node of the "survey"
in the XForm XML structure.
It should always be present in `self.json`.
"""

form_json = json.loads(self.json)
return form_json['name']

@property
def xml_with_disclaimer(self):
return XMLFormWithDisclaimer(self).get_object().xml
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def _get_xml_for_form(self, xform):
builder = SurveyElementBuilder()
sss = builder.create_survey_element_from_json(xform.json)
xform.xml = sss.to_xml()
xform._mark_start_time_boolean()
xform.mark_start_time_boolean()
xform.save()

def _submit_at_hour(self, hour):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class Command(BaseCommand):
def handle(self, *args, **kwargs):
for dd in DataDictionary.objects.all():
try:
dd._mark_start_time_boolean()
dd.mark_start_time_boolean()
dd.save()
except Exception:
print ("Could not mark start time for DD: %(data)s" % {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def handle(self, *args, **kwargs):
for i, dd in enumerate(
queryset_iterator(DataDictionary.objects.all())):
if dd.xls:
dd.set_uuid_in_xml(id_string=dd.id_string)
dd.set_uuid_in_xml()
super(DataDictionary, dd).save()
if (i + 1) % 10 == 0:
print('Updated %(nb)d XForms...' % {'nb': i})
46 changes: 20 additions & 26 deletions kobo/apps/openrosa/apps/viewer/models/data_dictionary.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# coding: utf-8
import os
import re
from xml.dom import Node
Expand Down Expand Up @@ -65,17 +64,11 @@ def __init__(self, *args, **kwargs):
self.instances_for_export = lambda d: d.instances.all()
super().__init__(*args, **kwargs)

def set_uuid_in_xml(self, file_name=None, id_string=None):
def set_uuid_in_xml(self):
"""
Add bind to automatically set UUID node in XML.
"""

if id_string:
root_node = id_string
else:
if not file_name:
file_name = self.file_name()
root_node, _ = os.path.splitext(file_name)
root_node = self.xform_root_node_name

doc = clean_and_parse_xml(self.xml)
model_nodes = doc.getElementsByTagName("model")
Expand All @@ -98,19 +91,21 @@ def set_uuid_in_xml(self, file_name=None, id_string=None):
instance_node = instance_nodes[0]

# get the first child whose id attribute matches our id_string
survey_nodes = [node for node in instance_node.childNodes
if node.nodeType == Node.ELEMENT_NODE and
(node.tagName == root_node or
node.attributes.get('id'))]

survey_nodes = [
node
for node in instance_node.childNodes
if node.nodeType == Node.ELEMENT_NODE
and node.tagName == root_node
]
if len(survey_nodes) != 1:
raise Exception(
"Multiple survey nodes with the id '{}'".format(root_node))
raise Exception(f'Multiple survey nodes `{root_node}`')

survey_node = survey_nodes[0]
formhub_nodes = [n for n in survey_node.childNodes
if n.nodeType == Node.ELEMENT_NODE and
n.tagName == "formhub"]
formhub_nodes = [
n
for n in survey_node.childNodes
if n.nodeType == Node.ELEMENT_NODE and n.tagName == 'formhub'
]

if len(formhub_nodes) > 1:
raise Exception(
Expand All @@ -130,10 +125,9 @@ def set_uuid_in_xml(self, file_name=None, id_string=None):
if len(formhub_nodes) == 0:
# append the calculate bind node
calculate_node = doc.createElement("bind")
calculate_node.setAttribute(
"nodeset", "/%s/formhub/uuid" % root_node)
calculate_node.setAttribute("type", "string")
calculate_node.setAttribute("calculate", "'%s'" % self.uuid)
calculate_node.setAttribute('nodeset', f'/{root_node}/formhub/uuid')
calculate_node.setAttribute('type', 'string')
calculate_node.setAttribute('calculate', f"'{self.uuid}'")
model_node.appendChild(calculate_node)

self.xml = smart_str(doc.toprettyxml(indent=" ", encoding='utf-8'))
Expand Down Expand Up @@ -166,9 +160,9 @@ def save(self, *args, **kwargs):
survey.name = survey.id_string
self.json = survey.to_json()
self.xml = survey.to_xml()
self._mark_start_time_boolean()
self.mark_start_time_boolean()
set_uuid(self)
self.set_uuid_in_xml(id_string=survey.id_string)
self.set_uuid_in_xml()
super().save(*args, **kwargs)

def file_name(self):
Expand Down Expand Up @@ -418,7 +412,7 @@ def get_data_for_excel(self):
self._expand_geocodes(d, key, e)
yield d

def _mark_start_time_boolean(self):
def mark_start_time_boolean(self):
starttime_substring = 'jr:preloadParams="start"'
if self.xml.find(starttime_substring) != -1:
self.has_start_time = True
Expand Down
24 changes: 7 additions & 17 deletions kobo/apps/openrosa/libs/utils/logger_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -523,26 +523,16 @@ def publish_xls_form(xls_file, user, id_string=None):
return dd


def publish_xml_form(xml_file, user, id_string=None):
def publish_xml_form(xml_file, user):
xml = smart_str(xml_file.read())
survey = create_survey_element_from_xml(xml)
form_json = survey.to_json()
if id_string:
dd = DataDictionary.objects.get(user=user, id_string=id_string)
dd.xml = xml
dd.json = form_json
dd._mark_start_time_boolean()
set_uuid(dd)
dd.set_uuid_in_xml()
dd.save()
return dd
else:
dd = DataDictionary(user=user, xml=xml, json=form_json)
dd._mark_start_time_boolean()
set_uuid(dd)
dd.set_uuid_in_xml(file_name=xml_file.name)
dd.save()
return dd
dd = DataDictionary(user=user, xml=xml, json=form_json)
dd.mark_start_time_boolean()
set_uuid(dd)
dd.set_uuid_in_xml()
dd.save()
return dd


def report_exception(subject, info, exc_info=None):
Expand Down
15 changes: 15 additions & 0 deletions kpi/models/asset_snapshot.py
Original file line number Diff line number Diff line change
Expand Up @@ -242,3 +242,18 @@ def generate_xml_from_source(self,
'warnings': warnings,
})
return xml, details

@property
def xform_root_node_name(self):
"""
Retrieves the name of the XML tag representing the root node of the "survey"
in the XForm XML structure.
This method uses the `name` setting from the XLSForm to determine the tag name.
If no name is provided, it falls back to using the asset UID.
"""

try:
return self.asset.content['settings']['name']
except KeyError:
return self.asset.uid
18 changes: 11 additions & 7 deletions kpi/utils/xml.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# coding: utf-8
from __future__ import annotations

import re
Expand Down Expand Up @@ -373,6 +372,10 @@ class XMLFormWithDisclaimer:
def __init__(self, obj: Union['kpi.AssetSnapshot', 'logger.XForm']):
self._object = obj
self._unique_id = obj.asset.uid

# Avoid accessing the `xform_root_node_name` property immediately to prevent
# extra database queries. It will be set only when it is actually needed.
self._root_tag_name = None
self._add_disclaimer()

def get_object(self):
Expand All @@ -391,6 +394,7 @@ def _add_disclaimer(self):
translated, disclaimers_dict, default_language_code = value

self._root_node = minidom.parseString(self._object.xml)
self._root_tag_name = self._object.xform_root_node_name

if translated:
self._add_translation_nodes(disclaimers_dict, default_language_code)
Expand All @@ -413,17 +417,17 @@ def _add_instance_and_bind_nodes(self):
# Inject <bind nodeset /> inside <model odk:xforms-version="1.0.0">
bind_node = self._root_node.createElement('bind')
bind_node.setAttribute(
'nodeset', f'/{self._unique_id}/_{self._unique_id}__disclaimer'
'nodeset', f'/{self._root_tag_name}/_{self._unique_id}__disclaimer'
)
bind_node.setAttribute('readonly', 'true()')
bind_node.setAttribute('required', 'false()')
bind_node.setAttribute('type', 'string')
bind_node.setAttribute('relevant', 'false()')
model_node.appendChild(bind_node)

# Inject note node inside <{self._unique_id}>
# Inject note node inside <{self._root_tag_name}>
instance_node = model_node.getElementsByTagName('instance')[0]
instance_node = instance_node.getElementsByTagName(self._unique_id)[0]
instance_node = instance_node.getElementsByTagName(self._root_tag_name)[0]
instance_node.appendChild(
self._root_node.createElement(f'_{self._unique_id}__disclaimer')
)
Expand All @@ -442,11 +446,11 @@ def _add_disclaimer_input(
disclaimer_input_label = self._root_node.createElement('label')
disclaimer_input.setAttribute('appearance', 'kobo-disclaimer')
disclaimer_input.setAttribute(
'ref', f'/{self._unique_id}/_{self._unique_id}__disclaimer'
'ref', f'/{self._root_tag_name}/_{self._unique_id}__disclaimer'
)

if translated:
itext = f'/{self._unique_id}/_{self._unique_id}__disclaimer:label'
itext = f'/{self._root_tag_name}/_{self._unique_id}__disclaimer:label'
disclaimer_input_label.setAttribute(
'ref',
f"jr:itext('{itext}')",
Expand Down Expand Up @@ -474,7 +478,7 @@ def _add_translation_nodes(
disclaimer_translation = self._root_node.createElement('text')
disclaimer_translation.setAttribute(
'id',
f'/{self._unique_id}/_{self._unique_id}__disclaimer:label',
f'/{self._root_tag_name}/_{self._unique_id}__disclaimer:label',
)
value = self._root_node.createElement('value')
language = n.getAttribute('lang').lower().strip()
Expand Down

0 comments on commit 0474aed

Please sign in to comment.