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.

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

Make sure the default_language, if any, is first #1975

Merged
merged 2 commits into from
Sep 5, 2018

Conversation

jnm
Copy link
Member

@jnm jnm commented Aug 29, 2018

Closes #1942. Marking as high priority since we accidentally deployed a half-baked version of #1913 and need this to finish the job properly.

@jnm jnm added the high priority To be done soon label Aug 29, 2018
@jnm jnm requested a review from noliveleger August 29, 2018 23:02
@jnm jnm added the coded label Aug 29, 2018
@noliveleger
Copy link
Contributor

@jnm, I could not directly comment on these lines in asset.py

L297    def _prioritize_translation(self, content, translation_name, is_new=False):
L298        _translations = content.get('translations')
L299        _translated = content.get('translated', [])
L300        if is_new and (translation_name in _translations):
L301            raise ValueError('cannot add existing translation')
L302        elif (not is_new) and (translation_name not in _translations):

I don't remember if content always has a translations key but in case it does not, L300 and L302 should crash because of None iteration.
Like L299, I would set a default value (empty list or dict) to content.get('translations') on like L298

@jnm
Copy link
Member Author

jnm commented Aug 30, 2018

@noliveleger, will assert that the content has translations per our discussion.

@jnm
Copy link
Member Author

jnm commented Sep 4, 2018

@noliveleger, what do you think about this approach?

@noliveleger noliveleger merged commit 961f246 into master Sep 5, 2018
@noliveleger noliveleger deleted the 1942-default-language-as-first-translation branch September 5, 2018 13:20
@jnm jnm removed the coded label Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority To be done soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants