feat(OpenRosa): support XLSForm `name` setting TASK-1074 #5137

rajpatel24 · 2024-10-01T20:32:55Z

Checklist

If you've added code that should be tested, add tests
If you've changed APIs, update (or create!) the documentation
Ensure the tests pass
Run ./python-format.sh to make sure that your code lints and that you've followed our coding style
Write a title and, if necessary, a description of your work suitable for publishing in our release notes
Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
Open an issue in the docs if there are UI/UX changes

Description

This is an advanced feature for unusual cases where it's necessary to override the root XML node name used in submissions. This may be used for an upcoming feature to allow customizable confirmation messages in Enketo.

Notes

Implemented logic to check and set the name attribute from XLSForms in the survey object.
The existing behavior where name was not utilized has been modified to honor the name setting, defaulting to the id_string if name is not provided or is invalid.

Related issues

Part of #5112

…nfirmation messages

notion-workspace · 2024-10-08T19:13:09Z

Allow through the name setting from XLSForms

notion-workspace · 2024-10-08T19:13:10Z

Allow through the name setting from XLSForms

notion-workspace · 2024-10-08T19:13:23Z

Allow through the name setting from XLSForms

jnm · 2024-10-10T20:16:08Z

kobo/apps/openrosa/apps/viewer/models/data_dictionary.py

-            survey.update({
-                'name': survey.id_string,
-            })
+            if not survey.name or survey.name == 'None':


Hi @rajpatel24, thanks for working on this! I'm curious: were there conditions where the string None gets passed in as the name? It seems a bit surprising to me

Hi @jnm, When the form lacked a 'name' setting, the name was being set as the string 'None'.

Previously, we ignored the name setting value, whether it was 'None' or an actual string, and always assigned survey.id_string as the default name.

Now, if a valid name string is present, we leave it unchanged. If the name setting is absent, we assign survey.id_string as the default name.

Sorry to hassle on this, but I wanted to know where the string None was coming from. It bothers me because someone could have their XLSForm settings sheet look like this:

name

None

It would be weird, but it'd be valid, and we wouldn't be able to tell the difference between a user-provided None and the one that gets generated when the settings sheet lacks a name altogether.

I did some digging and found that the None string is set by pyxform:

It's a string because of this: https://github.com/XLSForm/pyxform/blob/9d20bfec5128f53ec0d555f7187f3bc4881f667e/pyxform/xls2json.py#L396-L397

None itself comes from default_name=None in https://github.com/XLSForm/pyxform/blob/9d20bfec5128f53ec0d555f7187f3bc4881f667e/pyxform/builder.py#L327

default_name gets overridden by the name value in the settings sheet of the XLSForm here: https://github.com/XLSForm/pyxform/blob/9d20bfec5128f53ec0d555f7187f3bc4881f667e/pyxform/xls2json.py#L469-L470

I see that even though create_survey_from_xls() has a handy default_name parameter, create_survey_from_xls(self.xls, default_name=self.id_string) won't work because our logic relies on create_survey_from_xls() pulling the id_string out of the XLS file in the first place. This is a historical artifact; nowadays, we're always the ones generating the XLS file internally¹, and we always know the id_string in advance.

Let's do a hack, though, instead of blowing up the scope of this PR. Please invent some constant that's unlikely ever to be a user-provided value, like PLACEHOLDER_NAME = '__kobo_placeholder_name_value__', call survey = create_survey_from_xls(self.xls, default_name=PLACEHOLDER_NAME), and then overwrite survey.name only if it's equal to PLACEHOLDER_NAME.

¹ Even when someone uploads their own XLSForm, we convert that to JSON, and then we convert it back to XLS again when deploying.

jnm

Could we have a unit test for this? I think it would ideally:

Create an Asset with the name setting (this can be in the Asset.content; we do not need to mess around with importing an XLSX file)
Deploy that Asset
Inspect the resulting XForm.xml to verify that the first child of the primary instance (the first <instance>) has a node name that matches the name setting set in the Asset.content.

Please ping me on Zulip if you have any questions 🙏

kpi/tests/test_assets.py

jnm · 2024-11-04T17:46:15Z

kpi/tests/test_assets.py

+            'survey': [
+                {
+                    'text': 'select_one',
+                    'label': 'q1'
+                },
+            ],


Interesting! I'm surprised the unit tests don't fail since there's no type. I'll look into it.

It turns out that the entire question gets silently discarded (!) Note the empty "survey": [],

(Pdb) l 809 ) 810 asset.deploy(backend='mock', active=True) 811 breakpoint() 812 813 # Get the deployed XForm and parse it 814 -> xform = XForm.objects.get(id_string=asset.uid) 815 parser = XFormInstanceParser(xform.xml, xform.data_dictionary()) 816 817 # Access the first child element of the <instance> node 818 instance_node = parser.get_root_node().getElementsByTagName('instance')[0] 819 root_element = instance_node.firstChild (Pdb) import json (Pdb) print(json.dumps(asset.content, indent=2)) { "survey": [], "settings": { "name": "custom_root_node_name" }, "translations": [ null ], "translated": [ "label" ], "schema": "1" }

Interesting! I'm surprised the unit tests don't fail since there's no type. I'll look into it.

I'm ashamed. I completely misunderstood this and replaced type with text 🤦
Thanks @jnm for the correction :-)

Made an internal thread to discuss the weird (existing) behavior of silently discarding invalid rows:
https://chat.kobotoolbox.org/#narrow/stream/4-Kobo-Dev/topic/Invalid.20asset.20content.20rows.20silently.20disarded

jnm · 2024-11-04T17:47:42Z

kpi/tests/test_assets.py

+            'survey': [
+                {
+                    'text': 'select_one',
+                    'label': 'q1',
+                },
+            ],


jnm

Reviewed and tested locally. Thanks!

1. [x] If you've added code that should be tested, add tests 2. [ ] If you've changed APIs, update (or create!) the documentation 3. [x] Ensure the tests pass 4. [x] Run `./python-format.sh` to make sure that your code lints and that you've followed [our coding style](https://github.com/kobotoolbox/kpi/blob/main/CONTRIBUTING.md) 5. [x] Write a title and, if necessary, a description of your work suitable for publishing in our [release notes](https://community.kobotoolbox.org/tag/release-notes) 6. [x] Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE) 7. [x] Open an issue in the [docs](https://github.com/kobotoolbox/docs/issues/new) if there are UI/UX changes This is an advanced feature for unusual cases where it's necessary to [override the root XML node name](https://xlsform.org/en/#specify-xforms-root-node-name) used in submissions. This may be used for an upcoming feature to allow customizable confirmation messages in Enketo. - Implemented logic to check and set the name attribute from XLSForms in the survey object. - The existing behavior where name was not utilized has been modified to honor the name setting, defaulting to the id_string if name is not provided or is invalid. Part of #5112 --------- Co-authored-by: John N. Milner <[email protected]>

Enable support for name attribute in XLSForms to facilitate custom co…

8692e30

…nfirmation messages

rajpatel24 added the Back end label Oct 1, 2024

rajpatel24 changed the title ~~Enable support for name attribute in XLSForms to facilitate custom co…~~ Enable support for name attribute in XLSForms Oct 1, 2024

noliveleger closed this Oct 1, 2024

noliveleger reopened this Oct 1, 2024

rajpatel24 requested a review from noliveleger October 2, 2024 05:51

noliveleger changed the base branch from beta-refactored to kobocat-django-app-part-2 October 3, 2024 23:21

Base automatically changed from kobocat-django-app-part-2 to main October 7, 2024 18:40

rajpatel24 assigned noliveleger Oct 10, 2024

jnm requested changes Oct 10, 2024

View reviewed changes

jnm self-requested a review October 10, 2024 20:16

jnm requested changes Oct 10, 2024

View reviewed changes

Add test for handling name setting in the asset content

001425c

rajpatel24 force-pushed the allow_through_the_name_setting_from_xls_forms branch from bcd064b to 001425c Compare October 14, 2024 19:23

rajpatel24 requested a review from jnm October 15, 2024 09:25

jnm requested changes Oct 24, 2024

View reviewed changes

rajpatel24 added 2 commits November 4, 2024 15:00

Use unique placeholder for missing survey name and update tests

4c81d76

Resolve merge conflicts

f63d0d3

rajpatel24 requested a review from magicznyleszek as a code owner November 4, 2024 10:10

rajpatel24 requested a review from jnm November 4, 2024 10:51

magicznyleszek removed their request for review November 4, 2024 16:43

jnm reviewed Nov 4, 2024

View reviewed changes

Correct question structure in unit test

66cff30

jnm mentioned this pull request Nov 4, 2024

We (soon will) support the XLSForm name setting kobotoolbox/docs#379

Open

jnm changed the title ~~Enable support for name attribute in XLSForms~~ Enable support for name setting in XLSForms Nov 4, 2024

jnm changed the title ~~Enable support for name setting in XLSForms~~ Enable support for XLSForm name setting Nov 4, 2024

jnm approved these changes Nov 4, 2024

View reviewed changes

jnm changed the title ~~Enable support for XLSForm name setting~~ feat(OpenRosa): support XLSForm name setting TASK-1074 Nov 4, 2024

jnm merged commit c3ee4c9 into main Nov 4, 2024
7 checks passed

jnm deleted the allow_through_the_name_setting_from_xls_forms branch November 4, 2024 22:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(OpenRosa): support XLSForm `name` setting TASK-1074 #5137

feat(OpenRosa): support XLSForm `name` setting TASK-1074 #5137

rajpatel24 commented Oct 1, 2024 •

edited by jnm

Loading

notion-workspace bot commented Oct 8, 2024

notion-workspace bot commented Oct 8, 2024

notion-workspace bot commented Oct 8, 2024

jnm Oct 10, 2024

rajpatel24 Oct 15, 2024

jnm Oct 24, 2024

jnm left a comment

jnm Nov 4, 2024

jnm Nov 4, 2024 •

edited

Loading

rajpatel24 Nov 4, 2024

jnm Nov 4, 2024

jnm Nov 4, 2024

jnm left a comment

feat(OpenRosa): support XLSForm name setting TASK-1074 #5137

feat(OpenRosa): support XLSForm name setting TASK-1074 #5137

Conversation

rajpatel24 commented Oct 1, 2024 • edited by jnm Loading

Checklist

Description

Notes

Related issues

notion-workspace bot commented Oct 8, 2024

notion-workspace bot commented Oct 8, 2024

notion-workspace bot commented Oct 8, 2024

jnm Oct 10, 2024

Choose a reason for hiding this comment

rajpatel24 Oct 15, 2024

Choose a reason for hiding this comment

jnm Oct 24, 2024

Choose a reason for hiding this comment

jnm left a comment

Choose a reason for hiding this comment

jnm Nov 4, 2024

Choose a reason for hiding this comment

jnm Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

rajpatel24 Nov 4, 2024

Choose a reason for hiding this comment

jnm Nov 4, 2024

Choose a reason for hiding this comment

jnm Nov 4, 2024

Choose a reason for hiding this comment

jnm left a comment

Choose a reason for hiding this comment

feat(OpenRosa): support XLSForm `name` setting TASK-1074 #5137

feat(OpenRosa): support XLSForm `name` setting TASK-1074 #5137

rajpatel24 commented Oct 1, 2024 •

edited by jnm

Loading

jnm Nov 4, 2024 •

edited

Loading