Last Comment Bug 836147 - Notify developers of unsupported locales
: Notify developers of unsupported locales
Status: VERIFIED FIXED
[feature]
:
Product: Marketplace
Classification: Server Software
Component: Developer Pages (show other bugs)
: 1.0
: All All
: P2 enhancement (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 888793 (view as bug list)
Depends on:
Blocks: 885149
  Show dependency treegraph
 
Reported: 2013-01-29 18:05 PST by Rob Hudson [:robhudson]
Modified: 2014-03-28 09:39 PDT (History)
10 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments

Description Rob Hudson [:robhudson] 2013-01-29 18:05:21 PST
I believe we can stick this in app submission along with the app-validator errors/warnings. But if this needs to be in the app-validator that could work too.

My idea is this:

1. If a manifest specifies a default_locale that isn't support (at all), it is a validation error.

The change to how we currently deal with this is that currently it's a bug and the app ends up with a name of NULL. So this prevents that bug.

2. If the manifest specifies an additional locale in the manifest "locales" property that isn't support, it's only a warning and we simply drop it.

The change to how we currently deal with this is that we drop it now but silently. And since we don't show locale editing very well it's not obvious that it was dropped.

We'll need some way to notify the developer what the supported locales actually are and link to that -- either MDN or an ecosystem page?
Comment 1 Matt Basta [:basta] 2013-01-31 12:14:47 PST
I like #2, but I think we should talk about #1. If the default_locale isn't supported, we should check if there is an available fallback locale and raise a warning. If there is no fallback locale (i.e.: no locale that the developer provides is a supported locale), we should raise a validation error.

The reason is because default_locale doesn't necessarily imply the language that the app is listed in (as a field in the manifest), it's the language that the "default" text is written in, and serves as the key should the default text have been otherwise listed in the locales field. If the developer has submitted at least one supported locale, we're going to end up using that one at the very least, and the first supported locale should be considered the default within Marketplace.

The only time that we should be failing apps because of locales (IMHO) is when there are no locales that we understand. Everything else could be a warning (which I think is a good idea).
Comment 2 Rob Hudson [:robhudson] 2013-01-31 13:30:33 PST
I'm going to break this down by what I see are the possibilities:

1. App has no default_locale and no locales. Currently we assume the default_locale is the user's logged in locale when they submitted the app, which we know is supported by Marketplace. I think this is ok as it has been done this way on AMO for a long time.

2. App has specified a default_locale and has no locales. This is valid according to the docs and actually recommended. If, however, the specified default_locale isn't one we support, we again fall back to the user's logged in locale. I think we should warn here.

3. App has specified a default_locale and locales. This can further be broken down:

3a. default_locale is supported. Awesome. Any locale in the locales property that isn't supported is silently dropped. I think those should be warnings so the developer knows.

3b. default_locale isn't supported. Currently, like #2, we would fall back to the logged in locale regardless of what's in locales. Any locale in the locales property that isn't supported is silently dropped. I think both should be warnings.

3c. default_locale isn't supported and none of the locales specified are either. This currently isn't possible since we fall back to the logged in locale, but would be if we did what you're suggesting.


What you're suggesting is the same except for case 3b you're saying to instead choose one of the supported locales in the "locales" property as the default, correct?

I'm curious if Python's JSON maintains the order of items, e.g., to iterate over the locales in the order provided and choose the first supported one as the default.

I'd be interested in hearing more arguments one way or the other.

For the sake of moving forward, we could maintain our current way of doing things and add the warnings/errors. Changing how we determine the default_locale if the given one isn't supported could be another bug.
Comment 3 Matt Basta [:basta] 2013-01-31 13:38:31 PST
(In reply to Rob Hudson [:robhudson] from comment #2)
> What you're suggesting is the same except for case 3b you're saying to
> instead choose one of the supported locales in the "locales" property as the
> default, correct?

Exactly.

> I'm curious if Python's JSON maintains the order of items, e.g., to iterate
> over the locales in the order provided and choose the first supported one as
> the default.

If one of the available locales is the user's current logged-in locale, we should prefer that one. Otherwise, the order doesn't matter.

> For the sake of moving forward, we could maintain our current way of doing
> things and add the warnings/errors. Changing how we determine the
> default_locale if the given one isn't supported could be another bug.

Fair enough. I think the only thing required for that is to throw a warning for any unsupported locale (from locales or default_locale), correct?
Comment 4 Matt Basta [:basta] 2013-03-13 10:38:24 PDT
What else is there to do here? It's my understanding that the implementation discussed here is what we've got in production now. robhudson?
Comment 5 Matt Basta [:basta] 2013-03-13 10:38:51 PDT
CCing big rob again
Comment 6 Rob Hudson [:robhudson] 2013-03-13 16:14:58 PDT
Comment 0 mentions how we currently do things, which is we show no warnings or errors on non-supported default_locale and locales, and leads to a bug with a NULL name possible.
Comment 7 Andrew Williamson [:eviljeff] 2013-03-26 05:43:13 PDT
(In reply to Rob Hudson [:robhudson] from comment #6)
> Comment 0 mentions how we currently do things, which is we show no warnings
> or errors on non-supported default_locale and locales, and leads to a bug
> with a NULL name possible.

Any update on a timescale for fixing this bug? We're regularly seeing the NULL name bug - and whats worse is the cron job can't fix the NULL name so the developer has to delete the listing and start again.
Comment 8 Andreas Wagner [:TheOne] PTO until 2016-09-27 2013-05-15 13:22:41 PDT
This is hitting reviewers. I see entries without an app name in the review queues. My locale on mkt is "de".

https://marketplace.firefox.com/reviewers/apps/review/Super-mario-world-interacti for example does not have a name in its default locale "es", but it has in "en-us". The result is that en-us reviewers will see the app name, while de users for example won't. (Apart from that, the app manifest says, the default locale would be "en"...)

This is happening more and more, can we please consider making this P1?
Comment 9 Matt Basta [:basta] 2013-05-16 00:37:54 PDT
(In reply to Andreas Wagner [:TheOne] from comment #8)
> This is hitting reviewers. I see entries without an app name in the review
> queues. My locale on mkt is "de".

That seems like a separate issue with our localization code not falling back on the default locale properly.
Comment 10 Andrew Williamson [:eviljeff] 2013-05-16 08:15:24 PDT
(In reply to Matt Basta [:basta] from comment #9)
> (In reply to Andreas Wagner [:TheOne] from comment #8)
> > This is hitting reviewers. I see entries without an app name in the review
> > queues. My locale on mkt is "de".
> 
> That seems like a separate issue with our localization code not falling back
> on the default locale properly.

well, it is falling back on the default locale value of name - it just that in the default locale the name is null.
Comment 11 Matt Basta [:basta] 2013-05-16 08:22:12 PDT
(In reply to Andrew Williamson [:eviljeff] from comment #10)
> (In reply to Matt Basta [:basta] from comment #9)
> > (In reply to Andreas Wagner [:TheOne] from comment #8)
> > > This is hitting reviewers. I see entries without an app name in the review
> > > queues. My locale on mkt is "de".
> > 
> > That seems like a separate issue with our localization code not falling back
> > on the default locale properly.
> 
> well, it is falling back on the default locale value of name - it just that
> in the default locale the name is null.

It should never try to pick an empty locale. If there's at least one translation, even if it doesn't match the user's locale, it should pick that one.
Comment 12 Andrew Williamson [:eviljeff] 2013-05-16 08:34:10 PDT
(In reply to Matt Basta [:basta] from comment #11)
> (In reply to Andrew Williamson [:eviljeff] from comment #10)
> > (In reply to Matt Basta [:basta] from comment #9)
> > > (In reply to Andreas Wagner [:TheOne] from comment #8)
> > > > This is hitting reviewers. I see entries without an app name in the review
> > > > queues. My locale on mkt is "de".
> > > 
> > > That seems like a separate issue with our localization code not falling back
> > > on the default locale properly.
> > 
> > well, it is falling back on the default locale value of name - it just that
> > in the default locale the name is null.
> 
> It should never try to pick an empty locale. If there's at least one
> translation, even if it doesn't match the user's locale, it should pick that
> one.

Isn't the bug that the default locale should never have a null name?  If it was empty at first upload it shouldn't have been accepted; if it was changed to be null the null name shouldn't stick.
Comment 13 Matt Basta [:basta] 2013-05-16 08:35:30 PDT
(In reply to Andrew Williamson [:eviljeff] from comment #12)
> Isn't the bug that the default locale should never have a null name?  If it
> was empty at first upload it shouldn't have been accepted; if it was changed
> to be null the null name shouldn't stick.

Oh, the value stored in the database is an empty string? Then yes, that's indeed a bug. That should never happen.
Comment 14 Andrew Williamson [:eviljeff] 2013-06-07 11:03:21 PDT
There are two bugs here.  
First is that on submission I guess the app validator shouldn't let an app pass if they don't have a valid default locale. (Robs option 1?)

Second is that the update function for manifest fields in the cron job/new package upload doesn't handle name being NULL.  But if we fix the first bug then we don't need to fix the second.  So lets do that!
Comment 15 Matt Basta [:basta] 2013-06-10 16:50:34 PDT
Option #1 is based on the incorrect assumption that the "default locale" is the locale that the app is targeted for. As I mentioend in comment #2, apps don't have default locales; the "default_locale" field simply means that the default locale is the locale that the app's root name/description/developer fields are written in and that it is the fallback locale for other languages.

We should instead be warning/erroring if the user does not specify *any* supported locales in their manifest. Apps that don't specify a default locale aren't affected by this.

I.e.: the following manifest would be perfectly valid:

{
"name": "Foo Bar",
"description": "This is the description",
"default_locale": "bastalang",
"locales": {
  "en": {},
  "es": {"name": "ASDF"}
}
}

which would produce the following results:

BastaLang (dropped):
- name: Foo Bar
- desc: This is the description

en:
- name: Foo Bar
- desc: This is the description

es:
- name: ASDF
- desc: This is the description


An app whose default locale is unsupported may simply have the same name and description in other locales which it lists as supporting.
Comment 16 Andrew Williamson [:eviljeff] 2013-06-12 06:23:35 PDT
If we don't throw an error on an unsupported default_locale then we have to effectively treat another one of the locales as the new default.
In the example in comment #15, what would Marketplace show me as the user if I visited as a pl user - the en?  the es?  We could hard code the fallback order or rely on the order in the manifest as suggested above.
Comment 17 Matt Basta [:basta] 2013-06-12 11:24:23 PDT
(In reply to Andrew Williamson [:eviljeff] from comment #16)
> If we don't throw an error on an unsupported default_locale then we have to
> effectively treat another one of the locales as the new default.

Yes, exactly. The manifest spec doesn't give the "default_locale" any special meaning, it simply describes what locale the root fields are written in.

We should prefer it as the default language since it's there and likely implies the "most supported" language of the app, but we could treat any other locale as the "Marketplace default" just as easily.

> In the example in comment #15, what would Marketplace show me as the user if
> I visited as a pl user - the en?  the es?  We could hard code the fallback
> order or rely on the order in the manifest as suggested above.

The fallback order should be default_locale, user's locale (if available), then the first supported locale in the locales field. If none of the locales provided by the manifest are valid, it should be a hard validation error.
Comment 18 Andrew Williamson [:eviljeff] 2013-06-13 04:05:25 PDT
okay, sounds good.
Comment 19 Rob Hudson [:robhudson] 2013-07-01 08:32:26 PDT
*** Bug 888793 has been marked as a duplicate of this bug. ***
Comment 20 Wil Clouser [:clouserw] 2013-07-08 08:56:45 PDT
Bumping to the next milestone.  What's the status of this?
Comment 21 Matt Basta [:basta] 2013-07-10 18:31:38 PDT
https://github.com/mozilla/app-validator/commit/68106bfe6fd650e9f2056478637df8ed004ca751

This does what I've been talking about.
Comment 22 Iulian Timis 2013-07-18 08:41:11 PDT
Please add STR here or mark it with [qa-] if no QA is needed.
Comment 23 Matt Basta [:basta] 2013-07-18 08:50:26 PDT
Case 1: Submit an app with an unsupported locale and a supported locale in the manifest. You should get a warning for each unsupported locale.

Case 2: Submit an app with only an unsupported locale in the manifest. You should get an error and be unable to proceed.

Case 3: Submit an app with only supported locales in the manifest. You should not get an error or a warning related to locales.
Comment 24 Iulian Timis 2013-07-18 09:28:38 PDT
Verified as fixed in https://marketplace-dev.allizom.org/developers/submit/ on FF25 (Win 7) for all 3 cases.
Postfix screencast http://screencast.com/t/pazf03vrxb
Closing bug.
Comment 25 Andrew Williamson [:eviljeff] 2013-08-19 07:46:38 PDT
Looks like we might have a recurrence of this

https://marketplace.firefox.com/reviewers/apps/review/none-28

the manifest has: "default_locale": "hu-HU"
Comment 26 Mathieu Pillard [:mat] 2014-02-04 07:59:01 PST
#comment 25 issue is tracked in bug 941075. The original issue was fixed (see #comment 23 for STR).
Comment 27 Iulian Timis 2014-02-05 07:31:12 PST
Marking verified fixed based on comment 26
Postfix screencast http://screencast.com/t/pazf03vrxb

Note You need to log in before you can comment on or make changes to this bug.