Updating default form_name to data instead of using filename.

[nribeka]

Fix #130

Defaulting the tag name to 'data' instead of using filename. This will alleviate issue when filename is not valid for tag name.

The tag name can be changed by overriding it using the 'name' column in the setting sheet.

More conversation in #368.

[nribeka]

I needed to add the skip here because the test will always set the form name. I will never get the data tag without this.

[lognaturel]

I'm not thrilled by this approach because it means adding a code path only for testing. I'm wondering whether it might be appropriate to have an "old style" test for this particular change since the code that is changed and needs to be exercised is in xls2json.parse_file_to_json. @ukanga might be best position to answer that. I know he doesn't want to use those old style tests anymore for testing the actual conversion process which I agree strongly with. In this case the goal is to test a change to how the XLS file itself is treated, though.

[nribeka]

Yeah, the old tests I think will better suited for this case. I needed to add the skipping part because the test will set the form name when I don't pass name field in the markdown setting part.

I can add one old unit test if everyone is OK with that? I was under the impression that we are not touching the old unit tests.

[ukanga]

I agree adding a skip_name is unnecessary here. I don't think we need to change this one at all. We could change all tests that refer to pyxform_autotestname such that the paths only has /data/field-name-path. Perhaps we could even default it to data i.e.

survey.name = kwargs.get("name", "data")

We can still use the auto-generated id_string and title.

I am very okay with tests in pyxform/tests_v1 being updated to reflect the new default. My main opposition was on changes to fixtures on the old pyxform/tests/. The one XML fixture here I understand why that was unavoidable.

[nribeka]

Base unit test updated. Reverted some code related to the skipping part. I think this is ready for review again.

[nribeka]

Setting the name will override the default data tag name.

[lognaturel]

Maybe it'd be good to do this with a settings sheet to even more closely match the code paths used by real users?

[nribeka]

Setting sheet will not work. See my other comments.

[lognaturel]

Agreed that you can remove the default_name is None code path. Couple of questions inline.

[lognaturel]

I'm not thrilled by this approach because it means adding a code path only for testing. I'm wondering whether it might be appropriate to have an "old style" test for this particular change since the code that is changed and needs to be exercised is in xls2json.parse_file_to_json. @ukanga might be best position to answer that. I know he doesn't want to use those old style tests anymore for testing the actual conversion process which I agree strongly with. In this case the goal is to test a change to how the XLS file itself is treated, though.

[codecov-io]

Codecov Report

Merging #375 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
+ Coverage   82.31%   82.33%   +0.01%     
==========================================
  Files          23       23              
  Lines        3246     3244       -2     
  Branches      758      757       -1     
==========================================
- Hits         2672     2671       -1     
- Misses        435      436       +1     
+ Partials      139      137       -2

Impacted Files	Coverage Δ
pyxform/file_utils.py	89.47% <100%> (ø)	⬆️
pyxform/xls2json.py	77.88% <100%> (-0.07%)	⬇️
pyxform/builder.py	75.4% <100%> (ø)	⬆️
pyxform/xform2json.py	79.87% <0%> (+0.2%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a2cfa5...db6ca73. Read the comment docs.

[lognaturel]

A couple more questions!

[lognaturel]

The XML should be the same as for the test below, right? I think it'd be good to have that be explicitl

[nribeka]

Added explicit xml comparison. I had to manually set the id_string and title because the parent class override them with None even though I pass it using the settings tab. This part I think have issue.

[nribeka]

        survey = create_survey_element_from_dict(imported_survey_json)
        survey.name = kwargs.get("name", "data")
        survey.title = kwargs.get("title")
        survey.id_string = kwargs.get("id_string")

The code above will create survey object based on the parsed markdown string. But after the survey is created, the next line will overwrite the id_string and title with whatever value from the kwargs. So if you don't set the values in the kwargs, it will set it to None.

[ukanga]

It seems the way to test the default_name="data" is by testing parse_file_to_json() function directly. The markdown implementation may need to be updated to match the same but it will be fine to handle that outside of this PR.

[lognaturel]

Maybe it'd be good to do this with a settings sheet to even more closely match the code paths used by real users?

[nribeka]

@lognaturel,

I can't use the settings tab here too. If I add settings tab in the markdown, it will correctly create the survey using the settings tab. But the next line will override it with the values from the kwargs. And the values in the kwargs is set in _autoname_inputs.

This unit test will show this issue:

    def test_should_use_setting_tab(self):
        """
        Test using data as the name of the form which will generate <data />.
        """
        self.assertPyxformXform(
            md="""
               | survey   |           |      |           |
               |          | type      | name | label     |
               |          | text      | city | City Name |
               | settings |           |      |           |
               |          | id_string | name |
               |          | some-id   | data |
               """,
            debug=True,
            instance__contains=['<data id="some-id">'],
            model__contains=['<bind nodeset="/data/city" type="string"/>'],
            xml__contains=[
                '<input ref="/data/city">',
                '<label>City Name</label>',
                '</input>',
            ],
        )

[nribeka]

The above test will fail because it will generate this xml file (tag name is not data):

<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:ev="http://www.w3.org/2001/xml-events" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:odk="http://www.opendatakit.org/xforms" xmlns:orx="http://openrosa.org/xforms" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
  <h:head>
    <h:title>pyxform_autotesttitle</h:title>
    <model>
      <instance>
        <pyxform_autotestname id="pyxform_autotest_id_string">
          <city/>
          <meta>
            <instanceID/>
          </meta>
        </pyxform_autotestname>
      </instance>
      <bind nodeset="/pyxform_autotestname/city" type="string"/>
      <bind jr:preload="uid" nodeset="/pyxform_autotestname/meta/instanceID" readonly="true()" type="string"/>
    </model>
  </h:head>
  <h:body>
    <input ref="/pyxform_autotestname/city">
      <label>City Name</label>
    </input>
  </h:body>
</h:html>

[ukanga]

Looks good, minimal changes to tests as I would have prefered.

[ukanga]

It seems the way to test the default_name="data" is by testing parse_file_to_json() function directly. The markdown implementation may need to be updated to match the same but it will be fine to handle that outside of this PR.

[ukanga]

@nribeka If you can fix the black issues, this should be good to merge.

[lognaturel]

Thanks!