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

Explicit nginx routes for amo mobile pages #10266

Closed
muffinresearch opened this issue Mar 16, 2017 · 17 comments
Closed

Explicit nginx routes for amo mobile pages #10266

muffinresearch opened this issue Mar 16, 2017 · 17 comments
Assignees
Labels

Comments

@muffinresearch
Copy link
Contributor

muffinresearch commented Mar 16, 2017

Relates to #10240

Currently we are routing traffic to the mobile pages based on:

  • Presence of mamo cookie with value "on"
  • OR, a mobile UA string

In addition to this we have a fallback which will fallback based on the mobile pages providing a 404. The fallback mostly works but there are a few cases which are causing problems:

  • Any time the addons-server app issues a redirect it will be subject to being routed to the mobile pages again.
  • If the mobile page urls do not match the addons-server server urls there can be a game of redirect tennis which results in an infinite redir loop.
  • If when on a fallback page the client issues an XHR that might be routed incorrectly.

To fix this we could set up an explicit list of routes that the mobile pages responds to as this will be less prone to problems - the only downside will be one of maintenance. I'll add comments to the routes to make it clear that ops need to be told when new routes are added.

Based on https://github.com/mozilla/addons-frontend/blob/e356c928debbb226b57540b28e8a9b3d49cbaa91/src/amo/routes.js#L27

Here's the list of routes:

All routes are optionally prefixed with /%locale%/(android|firefox) though we also fill in both the locale and or the app if one does not exist.

/
/addon/%slug%/
/addon/%slug%/reviews/
/(dictionary|extensions|language|search|themes)/categories/
/(dictionary|extensions|language|search|themes)/featured/
/(dictionary|extensions|language|search|themes)/%slug%/
/search/
/simulate-async-error/
/simulate-sync-error/
/simulate-client-error/
/(dictionary|extensions|language|search|themes)/
@muffinresearch
Copy link
Contributor Author

@bqbn If it's possible to set this up we can verify it on -dev and stage and once verified get this out into the production as soon as possible.

@muffinresearch
Copy link
Contributor Author

@bqbn I just made a change because I just noticed this line: https://github.com/mozilla/addons-frontend/blob/master/src/core/constants.js#L65

persona -> themes
extension -> extensions

@bqbn
Copy link
Contributor

bqbn commented Mar 17, 2017

I've pushed some nginx changes to -dev for this. Please test and let us know how things are looking.

@muffinresearch
Copy link
Contributor Author

@bqbn thanks will check it out.

@ValentinaPC
Copy link

@muffinresearch : I've checked all the routes that I've mentioned in mozilla/addons-server#2037 using AMO-dev FF52(Win7) - clean profile - and did not see any changes.
As far as read above, some changes should be o -dev, yes?

@bqbn
Copy link
Contributor

bqbn commented Mar 17, 2017

FWIW, /users/edit/ (which is #10240 about) isn't mentioned in the above list of routes thus it's not included in the nginx changes (I can add it if needed be).

@muffinresearch
Copy link
Contributor Author

@bqbn /users/edit/ is a addon-server route only so there should be no need to have config to send it to the mobile pages.

@bqbn
Copy link
Contributor

bqbn commented Mar 17, 2017

@muffinreseach, so to clarify, is this ticket to not do 404 fallback for the listed routes on mobile site?

@muffinresearch
Copy link
Contributor Author

@bqbn correct, it's to send traffic to only those routes assuming a mamo cookie is set to "on" or the UA is a mobile UA.

@bqbn
Copy link
Contributor

bqbn commented Mar 17, 2017

After talking to @muffinresearch, I've updated our nginx configs and pushed them to -dev.

Please test it again.

@muffinresearch
Copy link
Contributor Author

The nginx config is broken currently and will be reverted see also #4171

Once this is fixed this can be re-tested.

@bqbn
Copy link
Contributor

bqbn commented Mar 20, 2017

The issue was because nginx didn't try the main site when UA is not mobile, and no mamo cookie. I've fixed it and pushed it to -dev again. Can we re-test it?

@ValentinaPC
Copy link

I've verified all routes I've mentioned in mozilla/addons-server#2037 using AMO-dev FF52(Win 7) and all seem to redirect correctly (updated the issue).
Please let me know if anything else should be checked here.

@bqbn
Copy link
Contributor

bqbn commented Mar 21, 2017

@muffinresearch I took the liberty and just pushed the nginx changes to -stage for this.

Can you let me know what is your plan to release this to -prod?

@muffinresearch
Copy link
Contributor Author

@bqbn, brilliant thanks - I meant to asked you to do this when tagging yesterday. Assuming no issues are found we should push this on Thursday. Thanks again!

@muffinresearch
Copy link
Contributor Author

Closing this since it's ready.

@ValentinaPC
Copy link

ValentinaPC commented Mar 23, 2017

Verified as fixed on -stage and -dev using FF52(Win 7) for the routes I've mentioned in the other issue.

@KevinMind KevinMind added migration:no-jira repository:addons-frontend Issue relating to addons-frontend labels May 5, 2024
@KevinMind KevinMind transferred this issue from mozilla/addons-frontend May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants