Fixed <output> inside repeat has absolute path instead of relative #450

gushil · 2020-06-10T10:22:21Z

codecov-commenter · 2020-06-10T10:28:49Z

Codecov Report

Merging #450 into master will increase coverage by 0.27%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     enketo/enketo-core#450      +/-   ##
==========================================
+ Coverage   83.11%   83.39%   +0.27%     
==========================================
  Files          25       25              
  Lines        3595     3595              
  Branches      833      834       +1     
==========================================
+ Hits         2988     2998      +10     
+ Misses        460      454       -6     
+ Partials      147      143       -4

Impacted Files	Coverage Δ
pyxform/survey_element.py	`94.44% <ø> (+4.27%)`	⬆️
pyxform/survey.py	`91.51% <100.00%> (ø)`

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 b0bd9d4...818c452. Read the comment docs.

MartijnR · 2020-06-10T22:37:15Z

Thanks @gushil, and OpenClinica, for stepping in so quickly! I've tried to break the fix with some black-box testing, and was not able to, so that's good. 👍

@ukanga this is the fix for the issue Ona reported recently. I'll add you as a reviewer, in case you have time (and to share the load with @lognaturel). I think, it would be great to get a fix merged soon, considering the problems this is causing for Enketo users. This is the kind of bug that is most urgent, because it is producing incorrect XForms and most (all?) servers don't have a way to automatically re-convert those forms to correct them.

MartijnR · 2020-06-16T21:31:31Z

@ukanga, just pinging to check if you're able to look at this Ona-reported issue. :)

lognaturel

I've done an initial pass with a few questions and observations. Any more hints you can give to how you approached this, what you're confident about, what questions might have come up for you would be really helpful.

pyxform/tests_v1/test_repeat.py

pyxform/survey.py

MartijnR · 2020-07-08T19:37:49Z

Thanks for your review @lognaturel! Just wanted to let you know OpenClinica has slightly delayed deploying the new pyxform version (they use a very old version), and I've heard @gushil may pick this up in a few weeks.

…h instead of relative.

gushil

Added new commit that addressed some of the feedbacks above.

Thanks for the feedbacks @lognaturel !

pyxform/survey.py

pyxform/tests_v1/test_repeat.py

lognaturel

Thanks! This is making more sense to me. I think there's either some code you can cut down or some possible bugs. :D I've also made a couple of test suggestion.

pyxform/survey.py

pyxform/tests_v1/test_repeat.py

pyxform/survey.py

lognaturel · 2020-07-16T04:39:49Z

@MartijnR I don't think there's anything in the spec that specifically addresses output in a text translation for labels in secondary instances. It seems wacky but people use it in lieu of enketo/enketo#721: https://forum.getodk.org/t/xlsform-index-repeat-calculate-filter-choice-odk-collect-v1-27-crash/27411. Is that supposed to be supported? If so, this PR should generate relative refs for those.

MartijnR · 2020-07-16T19:21:46Z

@lognaturel, you're referring to the output below, right? (using the test.xlsx form from that forum thread)

 <instance id="hm_name">
                <root>
                    <item>
                        <filter>1</filter>
                        <label>${name1}</label><!-- uh oh -->
                        <name>1</name>
                    </item>

I tried to figure out a syntax below. Not sure if I succeeded in that, and whether this implementable in pyxform. It wouldn't work in Enketo in the near future. It also wouldn't work for external instances, which to me is a fair argument to not support it all.

It is extremely convoluted xlsform (and xform) syntax indeed. I agree that #38 would seem so much more sensible.

<itext>
      <text id="a1">
             <value><output value="../name1"></value>
      </text>
</itext>

 <instance id="hm_name">
                <root>
                    <item>
                        <filter>1</filter>
                        <itextId>a1</itextId>
                        <name>1</name>
                    </item>

...

            <itemset nodeset="instance('hm_name')/root/item[filter &lt; ( /data/qb_1 +1)]">
                <value ref="name" />
                <label ref="itext(itextId)" />
            </itemset>
</select>

MartijnR · 2020-07-16T19:22:08Z

Oh LOL #403

lognaturel · 2020-07-16T20:41:15Z

Ahhh! I knew I'd seen something fly by related to this but couldn't think of where. Let's not worry about it here and instead try to get enketo/enketo#721 which would be really great. So I think this is very close to being ready.

pbowen-oc · 2020-07-17T00:12:30Z

@lognaturel @MartijnR - I can't tell from the discussion above - will this update remove the existing behavior that allowed item references in choice labels? We have used this to very good effect (never in a translated form configuration). It is very helpful for allowing the user to (for example) associate a medication and an adverse event or associate a procedure with a previously identified tumor.

In our experience, the only restriction we found was that we needed some text in the label other than the item reference itself.

lognaturel · 2020-07-17T01:54:14Z

Can you please share a minimal XLSForm on enketo/enketo#434 that illustrates the pattern you want to use, @pbowen-oc? This PR doesn’t change existing behavior around this so let’s continue the conversation there.

pbowen-oc · 2020-07-17T05:27:05Z

@lognaturel - Thanks, I have added an example file to enketo/enketo#434

Addressing feedbacks.

gushil · 2020-07-17T14:16:05Z

pyxform/survey.py

@@ -743,15 +752,13 @@ def itext(self):
                            )
                        continue

-                    if media_type == "long":
-                        value, output_inserted = self.insert_output_values(media_value)
+                    if media_type == "form":


I have to make changes to this line if I changed translation_key to form.

lognaturel · 2020-07-23T20:26:32Z

pyxform/survey.py

@@ -593,7 +593,7 @@ def _setup_choice_translations(name, choice_value, itext_id):
            for d in element.get_translations(self.default_language):

                translation_path = d["path"]
-                translation_key = "long"
+                translation_key = "form"


I meant changing the name of the variable to form. long and guidance are values for the form attribute.

lognaturel · 2020-07-23T20:29:15Z

pyxform/survey_element.py

@@ -271,6 +271,7 @@ def get_translations(self, default_language):
                for lang, text in constraint_msg.items():
                    yield {
                        "path": self._translation_path("jr:constraintMsg"),
+                        "output_context": self,


Oh, great! There was a bug before because all of these need relative paths as well.

lognaturel · 2020-07-23T21:24:30Z

I took the liberty of renaming the variable that represents the translation form and adding tests for the additional translated columns. Unfortunately it looks like I forgot to run black and I had to quickly leave my computer so things are in a weird state. I’ll fix and merge this evening.

Thanks for doing this and for taking my feedback, @gushil!

Fixed <output> inside repeat has absolute path instead of relative

c5ee870

gushil mentioned this pull request Jun 10, 2020

<output> inside repeat has absolute path instead of relative #446

Closed

Add test

e3a39de

MartijnR requested a review from ukanga June 10, 2020 22:37

MartijnR requested a review from lognaturel June 23, 2020 17:02

lognaturel requested changes Jun 24, 2020

View reviewed changes

pyxform/tests_v1/test_repeat.py Outdated Show resolved Hide resolved

pyxform/survey.py Outdated Show resolved Hide resolved

pyxform/survey.py Outdated Show resolved Hide resolved

pyxform/survey.py Outdated Show resolved Hide resolved

gushil added 2 commits July 11, 2020 12:53

Merge remote-tracking branch 'upstream/master' into fix-446

a15c4b6

Addressing feedbacks of Fixed <output> inside repeat has absolute pat…

b2f8ca5

…h instead of relative.

gushil commented Jul 11, 2020

View reviewed changes

pyxform/survey.py Outdated Show resolved Hide resolved

pyxform/survey.py Outdated Show resolved Hide resolved

pyxform/survey.py Outdated Show resolved Hide resolved

pyxform/tests_v1/test_repeat.py Outdated Show resolved Hide resolved

Run black

0e1fb2b

lognaturel requested changes Jul 16, 2020

View reviewed changes

Fixed <output> inside repeat has absolute path instead of relative.

631ea08

Addressing feedbacks.

gushil commented Jul 17, 2020

View reviewed changes

gushil requested a review from lognaturel July 17, 2020 14:16

lognaturel reviewed Jul 23, 2020

View reviewed changes

lognaturel previously approved these changes Jul 23, 2020

View reviewed changes

lognaturel self-requested a review July 23, 2020 20:31

Rename form variable, run black

0c477f0

lognaturel dismissed their stale review via 0c477f0 July 23, 2020 20:42

Add tests for remaining elements that can have translations

818c452

lognaturel approved these changes Jul 24, 2020

View reviewed changes

lognaturel merged commit b6093a1 into XLSForm:master Jul 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed <output> inside repeat has absolute path instead of relative #450

Fixed <output> inside repeat has absolute path instead of relative #450

gushil commented Jun 10, 2020

codecov-commenter commented Jun 10, 2020 •

edited

Loading

MartijnR commented Jun 10, 2020 •

edited

Loading

MartijnR commented Jun 16, 2020

lognaturel left a comment

MartijnR commented Jul 8, 2020

gushil left a comment •

edited

Loading

lognaturel left a comment •

edited

Loading

lognaturel commented Jul 16, 2020

MartijnR commented Jul 16, 2020

MartijnR commented Jul 16, 2020

lognaturel commented Jul 16, 2020

pbowen-oc commented Jul 17, 2020

lognaturel commented Jul 17, 2020

pbowen-oc commented Jul 17, 2020

gushil Jul 17, 2020

lognaturel Jul 23, 2020

lognaturel Jul 23, 2020

lognaturel commented Jul 23, 2020

Fixed <output> inside repeat has absolute path instead of relative #450

Fixed <output> inside repeat has absolute path instead of relative #450

Conversation

gushil commented Jun 10, 2020

codecov-commenter commented Jun 10, 2020 • edited Loading

Codecov Report

MartijnR commented Jun 10, 2020 • edited Loading

MartijnR commented Jun 16, 2020

lognaturel left a comment

Choose a reason for hiding this comment

MartijnR commented Jul 8, 2020

gushil left a comment • edited Loading

Choose a reason for hiding this comment

lognaturel left a comment • edited Loading

Choose a reason for hiding this comment

lognaturel commented Jul 16, 2020

MartijnR commented Jul 16, 2020

MartijnR commented Jul 16, 2020

lognaturel commented Jul 16, 2020

pbowen-oc commented Jul 17, 2020

lognaturel commented Jul 17, 2020

pbowen-oc commented Jul 17, 2020

gushil Jul 17, 2020

Choose a reason for hiding this comment

lognaturel Jul 23, 2020

Choose a reason for hiding this comment

lognaturel Jul 23, 2020

Choose a reason for hiding this comment

lognaturel commented Jul 23, 2020

codecov-commenter commented Jun 10, 2020 •

edited

Loading

MartijnR commented Jun 10, 2020 •

edited

Loading

gushil left a comment •

edited

Loading

lognaturel left a comment •

edited

Loading