-
-
Notifications
You must be signed in to change notification settings - Fork 180
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
Enable support for name attribute in XLSForms #5137
base: main
Are you sure you want to change the base?
Conversation
…nfirmation messages
2 similar comments
survey.update({ | ||
'name': survey.id_string, | ||
}) | ||
if not survey.name or survey.name == 'None': |
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.
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
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.
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.
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.
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 fromdefault_name=None
in https://github.com/XLSForm/pyxform/blob/9d20bfec5128f53ec0d555f7187f3bc4881f667e/pyxform/builder.py#L327default_name
gets overridden by thename
value in thesettings
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.
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.
Could we have a unit test for this? I think it would ideally:
- Create an
Asset
with thename
setting (this can be in theAsset.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 thename
setting set in theAsset.content
.
Please ping me on Zulip if you have any questions 🙏
bcd064b
to
001425c
Compare
'survey': [ | ||
{ | ||
'type': 'select_one', | ||
'label': 'q1', | ||
'select_from_list_name': 'iu0sl99', | ||
}, | ||
], | ||
'choices': [ | ||
{'name': 'a11', 'label': ['a11'], 'list_name': 'iu0sl99'}, | ||
{'name': 'a3', 'label': ['a3'], 'list_name': 'iu0sl99'}, | ||
], |
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.
Please change the type
to something simple like text
, and then you can eliminate select_from_list_name
as well as choices
. It'll just make the test easier to read since we're not testing any features related to select
questions
root_element = instance_node.firstChild | ||
|
||
# Assert that the name setting matches the root node name | ||
self.assertEqual(asset.content['settings']['name'], root_element.nodeName) |
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.
New tests can use the simple assert
syntax. I prefer to see the hard-coded value in the same line; we're not generating the value custom_root_node_name
dynamically, so it can be written here directly for clarity. Suggestion:
assert root_element.nodeName == 'custom_root_node_name`
'survey': [ | ||
{ | ||
'type': 'select_one', | ||
'label': 'q1', | ||
'select_from_list_name': 'iu0sl99', | ||
}, | ||
], | ||
'choices': [ | ||
{'name': 'a11', 'label': ['a11'], 'list_name': 'iu0sl99'}, | ||
{'name': 'a3', 'label': ['a3'], 'list_name': 'iu0sl99'}, | ||
], |
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.
Please see comment above about simplifying the survey by using a non-select type
def test_asset_without_name_setting(self): | ||
""" | ||
Test if 'name' setting is not provided, the root node should fall back | ||
to xform.id_string |
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.
Let's say "fall back to the asset UID"
# Assert that the root node name is the xform.id_sting | ||
self.assertEqual(xform.id_string, root_element.nodeName) |
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.
Let's actually assert that the root_element.nodeName == asset.uid
instead of xform.id_string
. That's the application behavior we want. It's true that xform.id_string
ends up matching the asset UID, but that's more of a side-effect than the thing we primarily care about.
See also previous note about new assert
syntax.
Checklist
./python-format.sh
to make sure that your code lints and that you've followed our coding styleDescription
This update enhances the OpenRosa to support the name attribute specified in XLSForms. Previously, OpenRosa did not honor the name='data' setting, which affected the implementation of custom confirmation messages. By allowing this attribute to be set and recognized, we ensure that users can utilize custom confirmation messages effectively, improving the overall user experience and flexibility of the forms.
Notes
Related issues
Part of #5112