835407 - uploading an app with pt as the default locale leaves an app without a name

Status: VERIFIED FIXED

(Marketplace Graveyard :: Developer Pages, defect)

Description

[Andrew Williamson [:eviljeff]]

From testing, if you upload an app with the default_locale as pt the name is missing.  Other non-supported locale codes, such as fr, seem to work fine.

As far as Rob and me can tell, the locale falls back to pt-PT, which isn't in https://github.com/mozilla/zamboni/blob/master/mkt/settings.py#L287 so the name ends up as missing, with a slug of none-*

Comment 1

[Rob Hudson [:robhudson]]

Verified locally with the test manifest:
http://dromedary6870.testmanifest.com/manifest.webapp

It tries to create an app with name={'pt': '...'} which we don't support so name ends up being NULL. That happens here:
https://github.com/mozilla/zamboni/blob/master/apps/addons/models.py#L431

A few lines down this we figure out that "pt" isn't a supported locale and set the default_locale of the app to "en-US", but the name still isn't set correctly.

I'm not sure what the expected behavior is however. Do we ignore the "pt" and fall back to "en-US" which doesn't seem valid. Do we follow the SHORTER_LANGUAGES which maps "pt" to "pt-PT" and use that? What about a locale that doesn't have a SHORTER_LANGUAGES map but is the only translation provided?

Comment 2

[Rob Hudson [:robhudson]]

(In reply to Rob Hudson [:robhudson] from comment #1)
> A few lines down this we figure out that "pt" isn't a supported locale and
> set the default_locale of the app to "en-US", but the name still isn't set
> correctly.

A slight correction... we find that "pt" isn't in AMO_LANGUAGES + HIDDEN_LANGUAGES and fall back to the user's logged in locale for the app's default_locale.

Comment 3

[Wil Clouser [:clouserw]]

If we can map to a shorter language, let's do that.  If we can't, we shouldn't accept the app.

Comment 4

[Rob Hudson [:robhudson]]

Since we're targeting Brazil should we update Marketplace settings so "pt" maps to "pt-BR" rather than "pt-PT"?

I'm thinking of the following case:

Imagine an app with a default locale of English but with "pt" localizations. We map pt to pt-PT. Someone browses for apps and logged in with pt-BR. They would see the English name rather than the Portuguese name which might be the better match for this person. The reason being there is no translation for pt-BR so we fall back to the app's default_locale.

{
  "name": "Name in English",
  "default_locale": "en-US",
  "locales": {
    "pt": {
      "name": "Name in Portuguese"
    }
  }
}

This is a separate issue but made me wonder if pt-PT is the optimal locale map for Marketplace.

Comment 5

[Wil Clouser [:clouserw]]

(In reply to Rob Hudson [:robhudson] from comment #4)
> Since we're targeting Brazil should we update Marketplace settings so "pt"
> maps to "pt-BR" rather than "pt-PT"?
> 
> I'm thinking of the following case:
> 
> Imagine an app with a default locale of English but with "pt" localizations.
> We map pt to pt-PT. Someone browses for apps and logged in with pt-BR. They
> would see the English name rather than the Portuguese name which might be
> the better match for this person. The reason being there is no translation
> for pt-BR so we fall back to the app's default_locale.
> 
> {
>   "name": "Name in English",
>   "default_locale": "en-US",
>   "locales": {
>     "pt": {
>       "name": "Name in Portuguese"
>     }
>   }
> }
> 
> This is a separate issue but made me wonder if pt-PT is the optimal locale
> map for Marketplace.

We shouldn't change the mappings.  If there is a question about it we probably shouldn't be remapping pt at all.

Comment 6

[Rob Hudson [:robhudson]]

https://github.com/mozilla/zamboni/commit/ec0b807

From the github comment:

    Follow SHORTER_LANGUAGES when creating app manifests
    
    This also fixes a bug in the translations app where we were comparing
    the lowercase locale to settings.HIDDEN_LANGUAGES which (for
    Marketplace) had locales of the form xx-YY. The result was no
    translation saved for locales in HIDDEN_LANGUAGES. This updates them to
    go through amo.utils.to_language which returns the xx-YY form. It also
    switches from using settings.LANGUAGES (lowercase locales) to
    settings.AMO_LANGUAGES.

Comment 7

[Cristian Boldan]

Verified as fixed in https://marketplace-dev.allizom.org/developers/submit/app/manifest on FF21 (Win 7) using http://dromedary6870.testmanifest.com/manifest.webapp
Postfix screencast http://screencast.com/t/AqmafdbHnIrr
Closing bug.