Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Jul 19, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b484be
warning (that needs tweak) on default lang with no translation problem
KeynesYouDigIt Apr 15, 2019
e1f08b3
Make and test warning for blanked fields with no translation.
KeynesYouDigIt Apr 15, 2019
4f2e7bd
ws change
KeynesYouDigIt Jul 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 59 additions & 14 deletions pyxform/survey.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ def get_nsmap(self):

return NSMAP

def xml(self):
def xml(self, warnings=None):
"""
calls necessary preparation methods, then returns the xml.
"""
Expand All @@ -191,7 +191,7 @@ def xml(self):

return node(
"h:html",
node("h:head", node("h:title", self.title), self.xml_model()),
node("h:head", node("h:title", self.title), self.xml_model(warnings)),
node("h:body", *self.xml_control(), **body_kwargs),
**nsmap
)
Expand Down Expand Up @@ -432,13 +432,13 @@ def _generate_instances(self):
yield i.instance
seen[i.name] = i

def xml_model(self):
def xml_model(self, warnings=None):
"""
Generate the xform <model> element
"""
self._setup_translations()
self._setup_media()
self._add_empty_translations()
self._add_empty_translations(warnings)

model_children = []
if self._translations:
Expand Down Expand Up @@ -555,25 +555,70 @@ def _setup_choice_translations(name, choice_value, itext_id):
choice_value,
)

def _add_empty_translations(self):
def _add_empty_translations(self, warnings=None):
"""
Adds translations so that every itext element has the same elements \
accross every language.
When translations are not provided "-" will be used.
This disables any of the default_language fallback functionality.
"""
if warnings is None:
warnings = []

paths = {}
for lang, translation in self._translations.items():
for path, content in translation.items():
paths[path] = paths.get(path, set()).union(content.keys())

for lang, translation in self._translations.items():
this_path_has_warning = False
for path, content_types in paths.items():
if path not in self._translations[lang]:
self._translations[lang][path] = {}
# missing question/question thingy thats like a hint
question_and_column = path.split(":")
missing_warning = self._generate_missing_translation_warning(
lang, question_and_column[0], question_and_column[1]
)
warnings.append(missing_warning)
# no need to warn about content types missing translations since the
# whole path has a warning now.
this_path_has_warning = True

for content_type in content_types:
if content_type not in self._translations[lang][path]:
self._translations[lang][path][content_type] = "-"
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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

missing_warning = self._generate_missing_translation_warning(
lang, path.split(":")[0], content_type
)
warnings.append(missing_warning)

def _generate_missing_translation_warning(self, lang, question_name, column_name):
found_on_msg = (
"\n--\n Question missing translation: "
+ question_name
+ "\n Column missing: "
+ column_name
)

if lang == "default":
return (
"\tDefault language not set," + " with missing default translations."
" Please consider setting a `default_language` in your"
+ " settings tab to insure questions and options appear"
+ " as expected."
+ found_on_msg
)

return (
"\tMissing field translations found for "
+ lang
+ " field may not appear as expected."
+ found_on_msg
)

def _setup_media(self):
"""
Expand Down Expand Up @@ -712,10 +757,10 @@ def date_stamp(self):
"""Returns a date string with the format of %Y_%m_%d."""
return self._created.strftime("%Y_%m_%d")

def _to_ugly_xml(self):
return '<?xml version="1.0"?>' + self.xml().toxml()
def _to_ugly_xml(self, warnings=None):
return '<?xml version="1.0"?>' + self.xml(warnings).toxml()

def _to_pretty_xml(self):
def _to_pretty_xml(self, warnings=None):
"""
I want the to_xml method to by default validate the xml we are
producing.
Expand All @@ -724,7 +769,7 @@ def _to_pretty_xml(self):
# space to text
# TODO: check out pyxml
# http://ronrothman.com/public/leftbraned/xml-dom-minidom-toprettyxml-and-silly-whitespace/
xml_with_linebreaks = self.xml().toprettyxml(indent=" ")
xml_with_linebreaks = self.xml(warnings).toprettyxml(indent=" ")
text_re = re.compile(r"(>)\n\s*(\s[^<>\s].*?)\n\s*(\s</)", re.DOTALL)
output_re = re.compile(r"\n.*(<output.*>)\n(\s\s)*")
pretty_xml = text_re.sub(
Expand Down Expand Up @@ -844,9 +889,9 @@ def print_xform_to_file(
try:
with codecs.open(path, mode="w", encoding="utf-8") as file_obj:
if pretty_print:
file_obj.write(self._to_pretty_xml())
file_obj.write(self._to_pretty_xml(warnings))
else:
file_obj.write(self._to_ugly_xml())
file_obj.write(self._to_ugly_xml(warnings))
except Exception as error:
if os.path.exists(path):
os.unlink(path)
Expand Down Expand Up @@ -896,9 +941,9 @@ def to_xml(self, validate=True, pretty_print=True, warnings=None, enketo=False):
if os.path.exists(tmp.name):
os.remove(tmp.name)
if pretty_print:
return self._to_pretty_xml()
return self._to_pretty_xml(warnings)

return self._to_ugly_xml()
return self._to_ugly_xml(warnings)

def instantiate(self):
"""
Expand Down
62 changes: 61 additions & 1 deletion pyxform/tests_v1/test_language_warnings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,66 @@ def test_label_with_unknown_subtag_should_warn(self):
)
os.unlink(tmp.name)

def test_missing_translation_no_default_lang_media_has_no_language(self):
# form should test media tag w NO default language set.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

survey = self.md_to_pyxform_survey(
"""
| survey | | | | |
| | type | name | label::English (en) | media::image|
| | integer | nums | How many nums? | opt1.jpg |
"""
)

warnings = []
tmp = tempfile.NamedTemporaryFile(suffix=".xml", delete=False)
tmp.close()
survey.print_xform_to_file(tmp.name, warnings=warnings)

# In this survey, when 'default' s selected the label of this question will be '-'.
# Also, when 'English (en)` is selected, the medial will be '-'
self.assertTrue(len(warnings) == 2)
os.unlink(tmp.name)

def test_missing_translation_media(self):
survey = self.md_to_pyxform_survey(
"""
| survey | | | | | |
| | type | name | label::English (en) | label::French (fr) | media::image::French (fr)|
| | integer | nums | How many nums? | Combien noms? | opt1.jpg |
"""
)

warnings = []
tmp = tempfile.NamedTemporaryFile(suffix=".xml", delete=False)
tmp.close()
survey.print_xform_to_file(tmp.name, warnings=warnings)
self.assertTrue(len(warnings) == 1)
self.assertIn(
"Question missing translation: /pyxform_autotestname/nums", warnings[0]
)
self.assertIn("Column missing: image", warnings[0])
os.unlink(tmp.name)

def test_missing_translation_hint(self):
survey = self.md_to_pyxform_survey(
"""
| survey | | | | | |
| | type | name | label::English (en) | label::French (fr) | hint::French (fr) |
| | integer | nums | How many nums? | Combien noms? | noms est nombres |
"""
)

warnings = []
tmp = tempfile.NamedTemporaryFile(suffix=".xml", delete=False)
tmp.close()
survey.print_xform_to_file(tmp.name, warnings=warnings)
self.assertTrue(len(warnings) == 1)
self.assertIn(
"Question missing translation: /pyxform_autotestname/nums", warnings[0]
)
self.assertIn("Column missing: hint", warnings[0])
os.unlink(tmp.name)

def test_default_language_only_should_not_warn(self):
survey = self.md_to_pyxform_survey(
"""
Expand All @@ -82,7 +142,7 @@ def test_default_language_only_should_not_warn(self):
| | list_name | name | label | fake |
| | opts | opt1 | Opt1 | 1 |
| | opts | opt2 | Opt2 | 1 |
"""
"""
)

warnings = []
Expand Down