Closed Bug 804980 Opened 12 years ago Closed 12 years ago

submitting an app with an unsupported locale results in missing details

Categories

(Marketplace Graveyard :: Developer Pages, defect, P2)

Product:
Marketplace Graveyard
Component:
Developer Pages
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
2012-11-01

People

(Reporter: eviljeff, Assigned: robhudson)

References

(
URL
)

Details

Some apps are appearing on the site with missing details filled in - i.e. no name, a slug like none-1 and no description.  All of these are in the manifest.  I copied one of the broken manifests to testmanifest.com - see URL - and I believe its the use of a default_locale we don't support (in this case en-GB).

There are a number of things wrong here:
1) if we don't support a locale we should throw a validation error.
2) it would be good if we could do a better job of falling back to a supported locale.  i.e. en-* to en-US, pt-* to pt-BR.
3) I can't get past Step 3 because of the missing details, which leads me to believe these apps may have been submitted via the API (I can't confirm).  Possibly another bug, but if so then it shouldn't be possible to submit a broken app like this.

Noticed on prod, replicated on -dev.
This sounds related to bug 799677.
(In reply to Rob Hudson [:robhudson] from comment #1)
> This sounds related to bug 799677.

In a way, I suppose, though that seems more focused on the review process.  Not horribly breaking like it does now is the first priority.
Assignee: nobody → robhudson.mozbugs
Priority: -- → P2
Target Milestone: --- → 2012-11-01
(In reply to Andrew Williamson [:eviljeff] from comment #0)
> 1) if we don't support a locale we should throw a validation error.

I disagree. We should gracefully ignore locales that we don't support. We should perhaps throw a warning if the developer has not provided *any* locales that we support.

OTOH, the manifest is simply a way for us to pre-populate fields on Marketplace during the submission flow. The developer is responsible for making sure those are filled in. If we find that too many of our apps have little or no information, we should start marking fields are required (requiring the info at submission time rather than development time) instead of forcing the developers to provide it in the manifest.

The API is a different story, and I think we should be (if anything) more strict with what we accept since there probably won't be a human looking at these things before they get sent to us.

> 2) it would be good if we could do a better job of falling back to a
> supported locale.  i.e. en-* to en-US, pt-* to pt-BR.

We've talked about this in the past (cvan and I): we should respect the specificity of the locales. It might be smart, though, to raise a warning if the developer specifies, for instance, only "en-US" that says something like "You might want to change your locale from `en-US` to `en`." We do accept bare language codes, so encouraging their use should be preferred over re-purposing similar locales.

> 3) I can't get past Step 3 because of the missing details, which leads me to
> believe these apps may have been submitted via the API (I can't confirm). 
> Possibly another bug, but if so then it shouldn't be possible to submit a
> broken app like this.

That definitely seems like a bug. Can you link me to these apps (via email, preferably)?
I don't see the testmanifest URL, can you re-post? We should fix the none-1 slug. Aside from that, there isn't much to do here I don't think. We could store these locales in the db in preparation for when that language is supported and non-hidden. This would help us leverage the community of developers to build up locale support.
Summary: submitting a app with an unsupported locale results in missing details → submitting an app with an unsupported locale results in missing details
> > 3) I can't get past Step 3 because of the missing details, which leads me to
> > believe these apps may have been submitted via the API (I can't confirm). 
> > Possibly another bug, but if so then it shouldn't be possible to submit a
> > broken app like this.
> 
> That definitely seems like a bug. Can you link me to these apps (via email,
> preferably)?

What's happening there is that the app.default_locale is set to "en-GB" and the fields that are TranslationFields short circuit and return None b/c of the check here:
https://github.com/mozilla/zamboni/blob/master/apps/translations/fields.py#L171

Locally it's even worse... I was able to submit the form by filling out all the empty fields with proper data. But what happened then is it passed form validation but the saves to the translation table didn't happen and now I have an app with no name, summary, or description.
Since the form now has "en-GB" in the form names and they always return None, you can't get past this form.
looks like Rob has (In reply to Matt Basta [:basta] from comment #3)
> (In reply to Andrew Williamson [:eviljeff] from comment #0)
> > 1) if we don't support a locale we should throw a validation error.
> 
> I disagree. We should gracefully ignore locales that we don't support. We
> should perhaps throw a warning if the developer has not provided *any*
> locales that we support.

I meant default_locale really.  It also not clear what happens if the default_locale is an unsuported locale and there are more than one supported ones included.  e.g.  they provide fr-FR, en-US, pt-BR and the default is fr-FR - which locale do we display on Marketplace (assuming the supported locales are still en-US, pt-BR, es-ES)?

> > 2) it would be good if we could do a better job of falling back to a
> > supported locale.  i.e. en-* to en-US, pt-* to pt-BR.
> 
> We've talked about this in the past (cvan and I): we should respect the
> specificity of the locales. It might be smart, though, to raise a warning if
> the developer specifies, for instance, only "en-US" that says something like
> "You might want to change your locale from `en-US` to `en`." We do accept
> bare language codes, so encouraging their use should be preferred over
> re-purposing similar locales.

okay, that will work.

> > 3) I can't get past Step 3 because of the missing details, which leads me to
> > believe these apps may have been submitted via the API (I can't confirm). 
> > Possibly another bug, but if so then it shouldn't be possible to submit a
> > broken app like this.
> 
> That definitely seems like a bug. Can you link me to these apps (via email,
> preferably)?

They weren't via the API (I thought the API had been used on prod, and apparently it hasn't).  The ones I found I asked the developers to delete or fix but the issue can be easily replicated by adding default_locale="en-GB" to any manifest.

Looks like Rob has gotten to the bottom of the issue now.
(In reply to Andrew Williamson [:eviljeff] from comment #6)
> I meant default_locale really.  It also not clear what happens if the
> default_locale is an unsuported locale and there are more than one supported
> ones included.  e.g.  they provide fr-FR, en-US, pt-BR and the default is
> fr-FR - which locale do we display on Marketplace (assuming the supported
> locales are still en-US, pt-BR, es-ES)?

`default_locale` is a way to denote the locale that the root nodes are written for. It basically aliases nodes like app.name to app.locales.$default_locale.name.

In that regard, when the developer submits an app, we should be filling in all of the supported translations into their corresponding translated fields. The ones that we display in the submission flow should be a.) the locale that the user is currently logged in with (if it's been specified), b.) any one of the manifest's provided translations that are for supported locales or c.) none of the above: none of the translations are supported locales and we should display blank fields.

`default_locale` should not be what is displayed in the submission flow by default; that is not what that node implies. We should always show the locale that the developer is logged in with (if it's been provided), even if that locale is a member of app.locales and isn't the one provided in the root.
I'm going to try to summarize this bug, let me know if I miss things because it seems like discussion is pretty broad.  Lets assume en-US, es, and pt-BR are the supported locales on the market for the discussion:

1) If a manifest comes in with de and fr only, we allow it, but all the fields in the submission flow are empty (and requiring filling before we allow them to finish submission)

2) We don't need any fallback support at this point (they have es-ES or pt-PT).  That can be a v2 thing if there are requests.

3) default_locale in our database should never be able to be set to any locale we don't support.  In the case of #1, it will be the language they choose in our UI during submission (which may be implied from the language they are browsing the site in)

Does that make sense?  Did I miss anything?  I suspect I'm just rewording what has been said here but bear with me :)
Yes.
That means the following needs to happen (correct if I'm wrong):

* If default_locale doesn't match one we support, show a list of locales for the user to choose from along with the other forms, with the default set to the user's chosen locale. (I'm not sure how tricky this is -- from what I've seen the form comes in pre-set with form names matching the locale)

* I'm not clear what we do in the case where default_locale doesn't match, but one of the other locales does. How do we pick one. My vote would be to still do the above and allow the dev to set it explicitly.

* Update the upload code to not set a default_locale if it doesn't match one we support.

Should there be any messaging around the extra form field to choose the locale telling the dev that a default_locale that we don't support was chosen?

If the dev doesn't update their manifest, are there any problems if/when users install the app?
(In reply to Rob Hudson [:robhudson] from comment #10)
> * If default_locale doesn't match one we support, show a list of locales for
> the user to choose from along with the other forms, with the default set to
> the user's chosen locale. (I'm not sure how tricky this is -- from what I've
> seen the form comes in pre-set with form names matching the locale)

I don't think this has to be anything special. The dev can already switch the language for the app summary on the submission screen. We can stick with that locale selector.

> 
> * I'm not clear what we do in the case where default_locale doesn't match,
> but one of the other locales does. How do we pick one. My vote would be to
> still do the above and allow the dev to set it explicitly.

Our best guess is to set it to the locale that the dev is browsing in. We could show a banner message on the submission screen that alerts the dev of why we selected it. 

> 
> * Update the upload code to not set a default_locale if it doesn't match one
> we support.

Yes. The upload code probably needs to make the locale guess / switch and stick the banner message in the session.
I think the problem is that we don't expose the ability to specify fields for multiple locales during the submission flow. Consider this:

- Developer has his locale on Marketplace set to en-US
- Developer's app supports en-GB (default), foo-BAR, zh-CN, and pt-BR
- Developer submits app to the Marketplace.

In this case, we should throw out the en-GB version and foo-BAR version (since we don't support either of them), but we support both of the other locales. We can do one of a few things at this point:

1.) Show a blank form with the locale set to en-US (the developer's logged-in locale) and make the developer fill in the app's info manually. The locale info from the manifest is ignored.
2.) Fill the form with the first locale we encounter that's supported.
3.) Create an interface that alerts the dev about this problem and ask them to choose what they want to do before they can continue. Perhaps provide a dropdown that says something like, "Prefill this form with information from the locale..." Changing the dropdown would change the locale of the submission form's data and pull in the locale information from the two supported locales in the manifest (in this case, pt-BR and zh-CN).

From a UX perspective, #3 is obviously the most friendly, but it'll require the most time and churn to build. #2 is very developer unfriendly, because the dev probably doesn't speak the language of whatever locale we chose. OTOH, it carries over the most info from the manifest. #1 is the easiest to implement and is probably the least confusing to the developer, but it also requires the most effort on the dev's part because we're not helping them out at all.

For "v1", I'd say #1 is probably sufficient. This is just an issue of prefilling; the dev can come back and provide the other locales' information later. We've got to ship code.

> Update the upload code to not set a default_locale if it doesn't match one we support.

Yes, this is definitely something that needs to be changed. We shouldn't be populating the submission form with a locale we don't support, and the translation fields' locales should never be given unsupported locale codes.
(In reply to Kumar McMillan [:kumar] from comment #11)
> (In reply to Rob Hudson [:robhudson] from comment #10)
> The dev can already switch
> the language for the app summary on the submission screen. We can stick with
> that locale selector.

oops, it looks like that locale selector on the submission screen is only on AMO. We should add it to Marketplace :)
(In reply to Matt Basta [:basta] from comment #12)
> 1.) Show a blank form with the locale set to en-US (the developer's
> logged-in locale) and make the developer fill in the app's info manually.
> The locale info from the manifest is ignored.
> 2.) Fill the form with the first locale we encounter that's supported.
> 3.) Create an interface that alerts the dev about this problem and ask them
> to choose what they want to do before they can continue. Perhaps provide a
> dropdown that says something like, "Prefill this form with information from
> the locale..." Changing the dropdown would change the locale of the
> submission form's data and pull in the locale information from the two
> supported locales in the manifest (in this case, pt-BR and zh-CN).
> 
> From a UX perspective, #3 is obviously the most friendly, but it'll require
> the most time and churn to build. #2 is very developer unfriendly, because
> the dev probably doesn't speak the language of whatever locale we chose.
> OTOH, it carries over the most info from the manifest. #1 is the easiest to
> implement and is probably the least confusing to the developer, but it also
> requires the most effort on the dev's part because we're not helping them
> out at all.
> 
> For "v1", I'd say #1 is probably sufficient. This is just an issue of
> prefilling; the dev can come back and provide the other locales' information
> later. We've got to ship code.

Yes, do #1 for sure.
I'm going to do the following:
1. If the default_locale isn't one we support, set the default_locale to the user's logged in locale.
2. The user will need to manually enter name, summary, and description fields.

If there's anything else that needs to be done to make the user experience more delightful, we can file more bugs...
https://github.com/mozilla/zamboni/commit/afd1578 

For QA:
* Try to submit a webapp with a default locale for a locale not in our settings:
https://github.com/mozilla/zamboni/blob/master/lib/settings_base.py#L119 (An example I used is "en-GB".
* Verify that the rest of the submission flow works and you can fill in the app details and save it and it persists.
* Bonus points: Verify the form names are in the logged in locale, which is used as a fallback since the specified default_locale is unsupported.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Verified as fixed . I added "default_locale": "en-GB" to a new manifest generated with http://testmanifest.com/ and the submission flow worked as expected.
Also , the fallback language was the locale used for login(en).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.