[REVERTED] issue 157 - warn users when any language is missing a tranlsation and blanking fields #287

KeynesYouDigIt · 2019-04-15T04:09:51Z

I was having trouble detecting media columns and the like early, and I realized the media column wasn't really the problem here, it was the unlabelled column.

So I dug for where the - was being literally added to the XML as an answer value and added the warning there.

lognaturel · 2019-04-15T22:34:58Z

pyxform/tests_v1/test_language_warnings.py

+
+    def test_no_default_language_should_warn_with_unlabeled_media_tags(self):
+        # form should test media tag w NO default language set.
+        survey = self.md_to_pyxform_survey("""


Awesome that you are starting with a test! 👍

I believe that this form is fine and should not result in a warning (a good test to have!) because none of the columns for user-facing strings specify a language. The issue is when only a subset of the columns do. That is, if you had label::French (fr) and media::image, there'd be a problem because the former specifies a language and the later doesn't. Similarly, label::French (fr) and media::image::French (fr) is fine but label and media::image::French (fr) is a problem.

Yep thats exactly the problem I hit when I ran that test. Fixed now should be good to go!

codecov-io · 2019-04-16T00:57:15Z

Codecov Report

Merging #287 into master will increase coverage by 0.48%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master     #287      +/-   ##
==========================================
+ Coverage   81.74%   82.22%   +0.48%     
==========================================
  Files          23       23              
  Lines        3215     3230      +15     
  Branches      750      753       +3     
==========================================
+ Hits         2628     2656      +28     
+ Misses        447      438       -9     
+ Partials      140      136       -4

Impacted Files	Coverage Δ
pyxform/survey.py	`90.29% <89.28%> (+0.87%)`	⬆️
pyxform/xls2json.py	`77.76% <0%> (+0.32%)`	⬆️
pyxform/xform2json.py	`80.5% <0%> (+0.41%)`	⬆️
pyxform/builder.py	`75.4% <0%> (+0.53%)`	⬆️
pyxform/survey_element.py	`90.14% <0%> (+0.98%)`	⬆️
pyxform/validators/util.py	`77.89% <0%> (+2.1%)`	⬆️
pyxform/validators/odk_validate/__init__.py	`74.5% <0%> (+3.92%)`	⬆️

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 9d36f97...4f2e7bd. Read the comment docs.

KeynesYouDigIt · 2019-04-16T01:13:32Z

This one should be good to go! resolves #157

KeynesYouDigIt · 2019-04-18T22:44:39Z

@lognaturel should be good to go?

lognaturel · 2019-04-29T15:32:00Z

pyxform/tests_v1/test_language_warnings.py

+            |         | opts            | opt2    | Opt2                | 1                |             |
+            | settings|                 |         |                     |                  |             |
+            |         | form_title      | form_id | public_key          | default_lanuage  |             |
+            |         | opts_form       | abc1    |                     | English (en)     |             |


This change doesn't make sense -- there is no column that uses the English (en) name. Also, this test was for another feature so please leave it as-is and add add additional tests.

eek good point, not sure where my head was at

I think I was using this to figure something else out, sorry!

lognaturel · 2019-04-29T15:35:51Z

pyxform/tests_v1/test_language_warnings.py

+        os.unlink(tmp.name)
+
+    def test_no_default_language_should_warn_with_unlabeled_media_tags(self):
+        # form should test media tag w NO default language set.


This test should very narrowly identify the issue. For example, a label with English (en) set as the language and media::image with no language and no other columns. It would be good to also have a test with something like label::English (en) and media::image::French (fr). There should be a warning any time that a language doesn't have complete translations.

I think I follow, tests added lmk if they do the trick!

Also going to try and cover untranslated choices and other stuff, not jsut media tags.

So with the current functionality, each language known to miss at least one field will get at least one warning. This prevents warnings from being overly verbose when there are multiple translations missing.... but is that what we want? or should I extend warnings to add a warning for each question/field missing a translation? Slack me if that question is confusing and I will try to jump on.

lognaturel · 2019-04-29T15:36:51Z

I haven't had a chance to look at this in detail yet but I made a couple of comments on the tests. In particular, the coverage is not complete.

Changing several core methods to take in a warnings parameter doesn't look like it should be necessary but I haven't yet dug in very deep. I think it should be possible to address just with local state.

KeynesYouDigIt · 2019-05-04T22:00:03Z

I really think I wasn't updating the tests with my improved understanding of the problem as time went on here, gotta slowdown and do that. @lognaturel check out the new tests and let me know if they are closer to what you were thinking

KeynesYouDigIt · 2019-05-05T18:55:16Z

After some thought I redisigned and added more tests. I'm curious about a particular case though --

"""
| survey  |                     |              |                                   |                            |
|              | type             | name    | label::English (en)      | hint                      |
|              | integer         | nums    | How many nums?      | nums is numbers |
"""

Doesnt seem to generate a warning since theres no attempt to translate anything into default when running this. Why does a media tag generate this mysterious default translation but a hint doesn't?

KeynesYouDigIt · 2019-05-11T20:09:04Z

@lognaturel or @ukanga got a chance to review?

KeynesYouDigIt · 2019-05-11T20:10:40Z

Changing several core methods to take in a warnings parameter doesn't look like it should be necessary but I haven't yet dug in very deep. I think it should be possible to address just with local state.

I could add a sort of self.warnings attribute if this seems much more readable to everybody

KeynesYouDigIt · 2019-05-20T21:04:37Z

bump @lognaturel / @ukanga

KeynesYouDigIt · 2019-06-02T00:23:45Z

bit of rebasing craziness today but should be shippable now!

KeynesYouDigIt · 2019-06-13T04:04:05Z

Anybody around to take another look at this? @ukanga / @lognaturel ?

ukanga · 2019-06-14T10:01:05Z

Would it be ideal to be able to point out which columns a user should check to apply the recommended changes? It is hard to be able to tell which columns a user should, especially in situations where there may be a high number of columns. The warnings:

Warnings:
        Missing field translations found for French (fr) field may not appear as expected
        Missing field translations found for English (en) field may not appear as expected
        Missing field translations found for English field may not appear as expected
        Missing field translations found for French field may not appear as expected
        The following language declarations do not contain valid machine-readable codes: English, French. Learn more: http://xlsform.org#multiple-language-support

This warnings are generated with the column headers as below:

type	name	label::English (en)	label::French (fr)	hint::English	hint::French

There is less searching in my opinion if the message included the columns that are likely affected such as:

Warnings:
        Missing field translations found for French (fr) field may not appear as expected (hint)
        Missing field translations found for English (en) field may not appear as expected (hint)
        Missing field translations found for English field may not appear as expected (label)
        Missing field translations found for French field may not appear as expected (label)
        The following language declarations do not contain valid machine-readable codes: English, French. Learn more: http://xlsform.org#multiple-language-support

#287 (comment)

ukanga · 2019-06-14T10:02:42Z

@KeynesYouDigIt I apologise for the delay. See my comment above.

KeynesYouDigIt · 2019-06-16T20:05:29Z

@ukanga not sure why its failing but 7a666d6 should take care of it!!

KeynesYouDigIt · 2019-06-16T21:35:19Z

@ukanga / @lognaturel code should be good, I am afraid I dont understand the circle errors at all. Is something wrong with the form validator CI is using?

yanokwa

There are three tests failing and it's not a CI issue. To narrow it down, run the following.

nosetests pyxform.tests.xlsform_spec_test
nosetests pyxform.tests.xform2json_test

KeynesYouDigIt · 2019-06-29T22:38:00Z

pyxform/survey.py

+                        self._translations[lang][path][content_type] = u"-"
+                        # missing question thingy thats like media
+                        if not this_path_has_warning:
+                            # the path has a translation but the content type is missing one.


I dont love this path_has_warning pattern at all. is there a list of content_types we support? couldnt find it in the spec for some reason, but id be happy to jsut make sure the content_type actually needs translation. That said if y'all are OK with this pattern then it should be fine to merge.

KeynesYouDigIt · 2019-07-14T00:40:45Z

Yep! squashing and rebasing now, then ill flip warnings to be a class fields. The simplest way to squash was to go to one commit, will that be OK or is it important to you to enforce strictness on commits being a single related change?

lognaturel · 2019-07-14T04:21:53Z

Awesome, @KeynesYouDigIt!

ill flip warnings to be a class fields

If you haven't done this already, I'd say hold off. I think the scope can be tightened rather than broadened. But as @ukanga says, that could be a later refactor pass through.

ukanga · 2019-07-14T04:21:59Z

I will be okay with one commit if it does not include the .circleci changes, you got about 2 commits in this PR.

ukanga · 2019-07-14T05:54:16Z

I created an issue for the refactor at #330.

ukanga · 2019-07-14T10:07:34Z

The OpenJDK-8-JRE-headless install issues have to do with the package being discontinued. We have resolved this failure in the current master branch, please rebase and drop the commits affecting .circleci/config.yml.

lognaturel reviewed Apr 15, 2019

View reviewed changes

KeynesYouDigIt marked this pull request as ready for review April 16, 2019 01:13

KeynesYouDigIt changed the title ~~Vince issue 157~~ issue 157 - warn users when "default" is the default language and its blanking fields Apr 27, 2019

lognaturel reviewed Apr 29, 2019

View reviewed changes

KeynesYouDigIt changed the title ~~issue 157 - warn users when "default" is the default language and its blanking fields~~ issue 157 - warn users when any language is missing a tranlsation and blanking fields May 4, 2019

KeynesYouDigIt force-pushed the Vince_issue_157 branch from 45a4b46 to 4cf106e Compare May 4, 2019 21:53

KeynesYouDigIt force-pushed the Vince_issue_157 branch 2 times, most recently from 6fbfdfe to ae3620c Compare June 1, 2019 22:50

ukanga previously approved these changes Jun 14, 2019

View reviewed changes

KeynesYouDigIt force-pushed the Vince_issue_157 branch from 6820351 to 7a666d6 Compare June 16, 2019 20:19

yanokwa requested changes Jun 21, 2019

View reviewed changes

KeynesYouDigIt force-pushed the Vince_issue_157 branch from 843eb04 to 46d3635 Compare June 29, 2019 21:37

KeynesYouDigIt commented Jun 29, 2019

View reviewed changes

KeynesYouDigIt dismissed ukanga’s stale review via 905137d July 14, 2019 00:44

KeynesYouDigIt force-pushed the Vince_issue_157 branch 12 times, most recently from 5ed2520 to 01b3a1c Compare July 14, 2019 02:22

warning (that needs tweak) on default lang with no translation problem

0b484be

KeynesYouDigIt force-pushed the Vince_issue_157 branch 2 times, most recently from 03fd64c to 3dd0213 Compare July 14, 2019 19:14

Make and test warning for blanked fields with no translation.

e1f08b3

KeynesYouDigIt force-pushed the Vince_issue_157 branch 2 times, most recently from 75cf659 to e1f08b3 Compare July 14, 2019 21:49

ws change

4f2e7bd

ukanga approved these changes Jul 19, 2019

View reviewed changes

ukanga merged commit d4ad1af into XLSForm:master Jul 19, 2019

yanokwa mentioned this pull request Aug 30, 2019

Conversion fails if label has language and choice_filter is used #355

Closed

lognaturel mentioned this pull request Sep 11, 2019

Revert attempt at closing #157 #365

Merged

KeynesYouDigIt changed the title ~~issue 157 - warn users when any language is missing a tranlsation and blanking fields~~ [REVERTED] issue 157 - warn users when any language is missing a tranlsation and blanking fields Oct 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVERTED] issue 157 - warn users when any language is missing a tranlsation and blanking fields #287

[REVERTED] issue 157 - warn users when any language is missing a tranlsation and blanking fields #287

KeynesYouDigIt commented Apr 15, 2019 •

edited

Loading

lognaturel Apr 15, 2019

KeynesYouDigIt Apr 16, 2019

codecov-io commented Apr 16, 2019 •

edited

Loading

KeynesYouDigIt commented Apr 16, 2019 •

edited

Loading

KeynesYouDigIt commented Apr 18, 2019

lognaturel Apr 29, 2019

KeynesYouDigIt May 4, 2019

KeynesYouDigIt May 4, 2019

lognaturel Apr 29, 2019

KeynesYouDigIt May 4, 2019

KeynesYouDigIt May 4, 2019

KeynesYouDigIt May 4, 2019

lognaturel commented Apr 29, 2019

KeynesYouDigIt commented May 4, 2019

KeynesYouDigIt commented May 5, 2019 •

edited

Loading

KeynesYouDigIt commented May 11, 2019

KeynesYouDigIt commented May 11, 2019

KeynesYouDigIt commented May 20, 2019

KeynesYouDigIt commented Jun 2, 2019

KeynesYouDigIt commented Jun 13, 2019

ukanga commented Jun 14, 2019

ukanga commented Jun 14, 2019

KeynesYouDigIt commented Jun 16, 2019

KeynesYouDigIt commented Jun 16, 2019

yanokwa left a comment

KeynesYouDigIt Jun 29, 2019

KeynesYouDigIt commented Jul 14, 2019 •

edited

Loading

lognaturel commented Jul 14, 2019

ukanga commented Jul 14, 2019

ukanga commented Jul 14, 2019

ukanga commented Jul 14, 2019

[REVERTED] issue 157 - warn users when any language is missing a tranlsation and blanking fields #287

[REVERTED] issue 157 - warn users when any language is missing a tranlsation and blanking fields #287

Conversation

KeynesYouDigIt commented Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 16, 2019 • edited Loading

Codecov Report

KeynesYouDigIt commented Apr 16, 2019 • edited Loading

KeynesYouDigIt commented Apr 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lognaturel commented Apr 29, 2019

KeynesYouDigIt commented May 4, 2019

KeynesYouDigIt commented May 5, 2019 • edited Loading

KeynesYouDigIt commented May 11, 2019

KeynesYouDigIt commented May 11, 2019

KeynesYouDigIt commented May 20, 2019

KeynesYouDigIt commented Jun 2, 2019

KeynesYouDigIt commented Jun 13, 2019

ukanga commented Jun 14, 2019

ukanga commented Jun 14, 2019

KeynesYouDigIt commented Jun 16, 2019

KeynesYouDigIt commented Jun 16, 2019

yanokwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KeynesYouDigIt commented Jul 14, 2019 • edited Loading

lognaturel commented Jul 14, 2019

ukanga commented Jul 14, 2019

ukanga commented Jul 14, 2019

ukanga commented Jul 14, 2019

KeynesYouDigIt commented Apr 15, 2019 •

edited

Loading

codecov-io commented Apr 16, 2019 •

edited

Loading

KeynesYouDigIt commented Apr 16, 2019 •

edited

Loading

KeynesYouDigIt commented May 5, 2019 •

edited

Loading

KeynesYouDigIt commented Jul 14, 2019 •

edited

Loading