Adding repeat instance to the model for repeat #380

nribeka · 2019-10-01T21:11:15Z

This will add instance for repeat section.

nribeka · 2019-10-01T21:13:42Z

@lognaturel 2 failed tests. The tests is counting number of elements in the model which obviously going to fail because we're adding new elements to the model. I'm just wondering if this is what we're looking for.

codecov-io · 2019-10-02T12:39:41Z

Codecov Report

Merging #380 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #380      +/-   ##
==========================================
+ Coverage   82.71%   82.75%   +0.03%     
==========================================
  Files          23       23              
  Lines        3304     3323      +19     
  Branches      776      781       +5     
==========================================
+ Hits         2733     2750      +17     
- Misses        429      430       +1     
- Partials      142      143       +1

Impacted Files	Coverage Δ
pyxform/xls2json.py	`78.71% <100%> (+0.13%)`	⬆️
pyxform/section.py	`96.22% <100%> (-1.58%)`	⬇️

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 c131e55...8503d1b. Read the comment docs.

nribeka · 2019-10-02T12:59:24Z

pyxform/section.py

+                result.appendChild(child.xml_instance(append_template=append_template))
+            if append_template and repeating_template:
+                append_template = not append_template
+                result.insertBefore(repeating_template, result._get_lastChild())


result._get_lastChild() vs result.childNodes[-1]?

ukanga · 2019-10-02T20:17:39Z

pyxform/tests_v1/test_repeat_template.py

+        section_template = '<section jr:template="">'
+        self.assertEqual(1, survey_xml.count(section_template))
+        repeat_a_template = '<repeat_a jr:template="">'
+        self.assertEqual(1, survey_xml.count(repeat_a_template))
+        repeat_b_template = '<repeat_b jr:template="">'
+        self.assertEqual(1, survey_xml.count(repeat_b_template))
+
+        section_instance = "<section>"
+        self.assertEqual(1, survey_xml.count(section_instance))
+        repeat_a_instance = "<repeat_a>"
+        self.assertEqual(1, survey_xml.count(repeat_a_instance))
+        repeat_b_instance = "<repeat_b>"
+        self.assertEqual(1, survey_xml.count(repeat_b_instance))
+


pyxform/pyxform/tests_v1/pyxform_test_case.py

Line 298 in 077972c

def assertContains(self, content, text, count=None, msg_prefix=""):

PyxformTestCase.assertContains() does accept a count parameter that does achieve the same result:

self.assertContains(survey_xml, "<repeat_b>", 1)

This works, it's fine too.

Ah I didn't know about that. I just assume to use the equality assertion 👯‍♂

lognaturel

I've only had a chance for a really surface look so far but the tests look great!

lognaturel · 2019-10-03T03:30:13Z

pyxform/section.py

@@ -104,11 +120,11 @@ def xml_control(self):

    # I'm anal about matching function signatures when overriding a function,


This comment is no longer relevant, right?

This was about overriding xml_instance and the name has now changed, right? So yes, the comment should be removed. It's also looking to me like neither this method nor generate_repeating_template need to have a **kwargs parameter, is that right?

lognaturel · 2019-10-03T16:20:13Z

I talked to @seadowg about this who pointed out that it's quite likely form designers are currently using repeats in Collect to represent 0 or more of a thing (rather than 1 or more). For example, if the desire is to collect information about children or siblings, it's quite likely that the flow of getting prompted initially about whether or not to add a repeat and potentially saying no is actually the right flow.

I don't know exactly what to do about this now. Marking as needs discussion.

nribeka · 2019-10-03T16:23:45Z

This would also need input from @MartijnR.

MartijnR · 2019-10-03T20:06:09Z

Enketo creates the first repeat automatically (and has been since 2012). In the past users have expressed a desire to have 0, but I believe this has been addressed sufficiently by introducing support for repeat-count and allowing the first repeat to be deleted manually (which initially wasn't possible). Since then, here have been no issues reported. If it is important to (be able to) start with 0, I agree we need to address that though.

craigappl · 2019-10-13T18:33:32Z

@ukanga , note that we need to review the comments from today's session. Link

lognaturel

We have agreed in #182 to issue a warning about the behavior change and link out to some documentation.

@lognaturel draft documentation
@nribeka add warning when a form definition contains one or more repeat

We've agreed to add a warning and some documentation.

nribeka · 2019-10-24T14:41:26Z

@lognaturel or @ukanga do you guys aware of way to do warning assertions in the v1? There's an old test that assert the warning, but it's the old unit test.

Warning added

pyxform/xls2json.py

lognaturel · 2019-11-05T22:06:35Z

I think you can see an example of verifying warnings with v1 tests in the test_language_warnings test.

nribeka · 2019-11-06T20:36:35Z

@lognaturel I just checked that unit tests and the warning testing will not work on this use case because the warning is generated when we parse the xls to json, but the v1 tests doesn't use the same function. It's using custom function to convert the md to json.

lognaturel

I'm comfortable not having an explicit test for the warning. I think we'll remove it in a few months anyway and it's very easy to manually verify.

nribeka added 3 commits October 1, 2019 14:25

Updating repeating template.

16741c2

Updating when to attach the template of repeat.

89ad9b4

Adding flag to determine when to append the template.

af9000b

nribeka added 6 commits October 1, 2019 17:31

Resetting the attach flag to ensure attaching all repeats.

a405cee

Adding basic form for unit testing template and instance in model.

7f13b47

Using insert_before to put template before instance.

0fa9fba

Updating old unit tests to match new structure.

b2f0a63

Adding unit tests to test the template and instance.

fb04c10

Black formatting changes.

df0924c

nribeka marked this pull request as ready for review October 2, 2019 12:49

nribeka commented Oct 2, 2019

View reviewed changes

ukanga previously approved these changes Oct 2, 2019

View reviewed changes

lognaturel reviewed Oct 3, 2019

View reviewed changes

lognaturel added the needs discussion label Oct 3, 2019

lognaturel mentioned this pull request Oct 23, 2019

Add a first repeat instance in addition to the template #182

Closed

lognaturel removed the needs discussion label Oct 23, 2019

lognaturel requested a review from ukanga October 23, 2019 16:21

lognaturel previously requested changes Oct 23, 2019

View reviewed changes

lognaturel mentioned this pull request Oct 23, 2019

Add section on representing zero repeats and add subheadings to repeat section XLSForm/xlsform.github.io#179

Merged

Adding repeat behavior change warning.

14fc480

lognaturel self-requested a review November 5, 2019 21:55

lognaturel reviewed Nov 5, 2019

View reviewed changes

pyxform/xls2json.py Outdated Show resolved Hide resolved

Updating documentation for the new repeat section behavior.

efcf405

nribeka requested a review from lognaturel November 6, 2019 20:34

lognaturel previously approved these changes Nov 6, 2019

View reviewed changes

lognaturel mentioned this pull request Nov 12, 2019

Output no-repeat-reference-error in some manner or the pyxform version or the xforms version #393

Closed

yanokwa added this to the v1.0.0 milestone Nov 18, 2019

Merge branch 'master' into pyxform-182-v2

7305dce

yanokwa dismissed lognaturel’s stale review via 7305dce December 19, 2019 19:01

Update test to match expected behavior

8503d1b

yanokwa merged commit 154ae78 into XLSForm:master Dec 19, 2019

obdulia-losantos mentioned this pull request May 19, 2020

feat(odk): upgrade [email protected] eHealthAfrica/aether#835

Merged

This was referenced Jan 17, 2024

dev: update dependencies, testing, packaging, and linting/formatting #685

Merged

Custom instance attribute not applied to repeat template #686

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding repeat instance to the model for repeat #380

Adding repeat instance to the model for repeat #380

nribeka commented Oct 1, 2019

nribeka commented Oct 1, 2019

codecov-io commented Oct 2, 2019 •

edited

Loading

nribeka Oct 2, 2019

ukanga Oct 2, 2019

nribeka Oct 3, 2019

lognaturel left a comment

lognaturel Oct 3, 2019

nribeka Oct 3, 2019

lognaturel Nov 5, 2019

lognaturel commented Oct 3, 2019

nribeka commented Oct 3, 2019

MartijnR commented Oct 3, 2019

craigappl commented Oct 13, 2019

lognaturel left a comment •

edited

Loading

nribeka commented Oct 24, 2019

lognaturel commented Nov 5, 2019

nribeka commented Nov 6, 2019

lognaturel left a comment

		@@ -104,11 +120,11 @@ def xml_control(self):

		# I'm anal about matching function signatures when overriding a function,

Adding repeat instance to the model for repeat #380

Adding repeat instance to the model for repeat #380

Conversation

nribeka commented Oct 1, 2019

nribeka commented Oct 1, 2019

codecov-io commented Oct 2, 2019 • edited Loading

Codecov Report

nribeka Oct 2, 2019

Choose a reason for hiding this comment

ukanga Oct 2, 2019

Choose a reason for hiding this comment

nribeka Oct 3, 2019

Choose a reason for hiding this comment

lognaturel left a comment

Choose a reason for hiding this comment

lognaturel Oct 3, 2019

Choose a reason for hiding this comment

nribeka Oct 3, 2019

Choose a reason for hiding this comment

lognaturel Nov 5, 2019

Choose a reason for hiding this comment

lognaturel commented Oct 3, 2019

nribeka commented Oct 3, 2019

MartijnR commented Oct 3, 2019

craigappl commented Oct 13, 2019

lognaturel left a comment • edited Loading

Choose a reason for hiding this comment

nribeka commented Oct 24, 2019

lognaturel commented Nov 5, 2019

nribeka commented Nov 6, 2019

lognaturel left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 2, 2019 •

edited

Loading

lognaturel left a comment •

edited

Loading