search

mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
127 stars 41 forks source link

AddonType not found should not be a 500 #10200

Closed muffinresearch closed 7 years ago

muffinresearch commented 7 years ago

Looks like certain paths lead to checking addonType and when the addonType is found it throws which results in a 500 error.

One example is user urls which don't have a mobile component.

https://addons-dev.allizom.org/en-US/firefox/user/ensolis/

See both of these for examples of the same error:

I would think that most cases like this should actually be a 404?

muffinresearch commented 7 years ago

Would be nice to fix this for release if possible.

mstriemer commented 7 years ago

Could we set up nginx to redirect the /blah/blah/user/.* URLs to addons-server?

muffinresearch commented 7 years ago

Could we set up nginx to redirect the /blah/blah/user/.* URLs to addons-server?

Could do, but it seems odd to me that there's an assumption that the first part of the url after /%locale%/%clientApp%/ is a addonType.

We're going to need a user page for desktop soon in any case.

tofumatt commented 7 years ago

Could do, but it seems odd to me that there's an assumption that the first part of the url after /%locale%/%clientApp%/ is a addonType.

It's because of how react-router works; we can't supply a regex for the URL (which is really just /themes/ and /extensions/ for our purposes) so the route is /:addonType/ and that catches everything 😞

kumar303 commented 7 years ago

The react-router docs are not great but I don't see an easy way to fix this. I guess we could just add a route for user so that it is forced into a 404? https://github.com/ReactTraining/react-router/blob/master/docs/guides/RouteMatching.md

kumar303 commented 7 years ago

hmm, clicking on the user from here is still a 500 for me https://addons-dev.allizom.org/en-US/firefox/addon/tile-tabs/

ValentinaPC commented 7 years ago

Verified on FF51(Android 6.0.1) using AMO-dev: 500 page for me too. Please see video: videotogif_2017 03 07_16 42 24

muffinresearch commented 7 years ago

@ValentinaPC this patch isn't live yet the rev that's on -dev is 0fc05a5a7a8e4a954eff9e5842661e1c8a2120b5 which is before that patch landed.

muffinresearch commented 7 years ago

-dev deploys are currently blocked due to deps issues but once https://amo.addons-dev.allizom.org/__version__ is beyond 0fc05a5 it should be ready to test.

kumar303 commented 7 years ago

This landed on dev. It's working for me now.

ValentinaPC commented 7 years ago

Verified as fixed on AMO-dev FF51(Android 6.0.1) Postfix video: videotogif_2017 03 08_11 45 47