-
Notifications
You must be signed in to change notification settings - Fork 136
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.
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
[REVERTED] issue 157 - warn users when any language is missing a tranlsation and blanking fields #287
Conversation
|
||
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(""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep thats exactly the problem I hit when I ran that test. Fixed now should be good to go!
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
This one should be good to go! resolves #157 |
@lognaturel should be good to go? |
| | opts | opt2 | Opt2 | 1 | | | ||
| settings| | | | | | | ||
| | form_title | form_id | public_key | default_lanuage | | | ||
| | opts_form | abc1 | | English (en) | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eek good point, not sure where my head was at
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was using this to figure something else out, sorry!
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 |
45a4b46
to
4cf106e
Compare
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 |
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 |
@lognaturel or @ukanga got a chance to review? |
I could add a sort of self.warnings attribute if this seems much more readable to everybody |
bump @lognaturel / @ukanga |
6fbfdfe
to
ae3620c
Compare
bit of rebasing craziness today but should be shippable now! |
Anybody around to take another look at this? @ukanga / @lognaturel ? |
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:
This warnings are generated with the column headers as below:
There is less searching in my opinion if the message included the columns that are likely affected such as:
|
@KeynesYouDigIt I apologise for the delay. See my comment above. |
6820351
to
7a666d6
Compare
@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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
843eb04
to
46d3635
Compare
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. |
There was a problem hiding this comment.
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.
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? |
5ed2520
to
01b3a1c
Compare
Awesome, @KeynesYouDigIt!
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. |
I will be okay with one commit if it does not include the |
I created an issue for the refactor at #330. |
The |
03fd64c
to
3dd0213
Compare
75cf659
to
e1f08b3
Compare
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.