Skip to content

Add application redirects + vary headers #828

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 27, 2016
Merged

Add application redirects + vary headers #828

merged 1 commit into from
Jul 27, 2016

Conversation

muffinresearch
Copy link
Contributor

@muffinresearch muffinresearch commented Jul 25, 2016 •
edited
Loading

Fixes mozilla/addons#9763 and mozilla/addons#9762

This PR:

  • Adds a redirection for client application e.g. firefox/android
  • Moves the lang and application redirects into an express middleware
  • slashes are appended unless configured not to be. This is off for the disco pane since the URL in the pref doesn't end with a slash. (If we are going to need more control over this we'll need to think again) FWIW: AMO currently seems to mostly favour always adding a slash.

NOTE: I updated and removed the slash handling for now. I think this is potentially better added separately if we want it.

The reason for moving to a middleware is because redirections need to happen for URLS that may not match the routes. If it's all done in react-router the redirects can only happen for matching routes. This also means the router prefix for lang and application don't have to be optional which also looks a bit nicer.

Like AMO the reason for having this in one piece of code is that the result is a single redirect. I added logic so vary headers are set only when headers are used for introspection. E.g. if an accept-language fall-back isn't used there won't be a vary on accept-language.

The redirection behaviour should match up to what AMO currently does e.g:

/ -> /en-US/firefox/
/whatever -> /en-US/firefox/whatever
/en-US -> /en-US

/en-US/ -> /en-US/firefox
/pt/ -> /pt-PT/firefox
/pt -> /pt-PT/firefox

/addon/adblock-plus -> /en-US/firefox/addon/adblock-plus
/whatever/firefox/ -> /en-US/firefox/

I'm still not sure if we should take the leading locale and application off the url at this point? That would mean react-router routes wouldn't need to care about locale or app at all. Either way we can consider that separately.

Adding user-agent header introspection to work out the correct application is to follow.

@muffinresearch muffinresearch force-pushed the add-app-redirection-and-vary branch from da6e24f to 42e1776 Compare July 25, 2016 22:14
muffinresearch
muffinresearch reviewed Jul 25, 2016
View reviewed changes
src/amo/routes.js Outdated
<Route path="/(:lang/)" component={App}>
<Route path="/:lang/:application" component={App}>
<IndexRoute component={Home} />
<Route path="addon/:slug(/)" component={DetailPage} />
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can now have the optional trailing slash removed having move to a middleware.

@muffinresearch muffinresearch changed the title Add application redirects + vary headers Add application redirects + vary headers WiP Jul 26, 2016
@muffinresearch muffinresearch changed the title Add application redirects + vary headers WiP Add application redirects + vary headers Jul 26, 2016
@muffinresearch muffinresearch force-pushed the add-app-redirection-and-vary branch 3 times, most recently from 92fbc13 to 9233bf2 Compare July 26, 2016 15:17
kumar303
kumar303 reviewed Jul 26, 2016
View reviewed changes
src/core/i18n/utils.js Outdated
* Looks up the language from the router renderProps.
* When that fails fall-back to accept-language.
* Check validity of language:
* - If invalid fall-back to accept-language.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comma after invalid

@muffinresearch
Copy link
Contributor Author

muffinresearch commented Jul 26, 2016 •
edited
Loading

I need to add a test to make sure query strings aren't mangled in this process.

kumar303
kumar303 reviewed Jul 26, 2016
View reviewed changes
src/core/server/base.js Outdated
} catch (e) {
log.info(dedent`Locale not found or required for locale: "${locale}".
Falling back to default lang: "${config.get('defaultLang')}"`);
log.info(`Locale not found or required for locale: "${locale}"`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this sentence is hard for me to parse

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I'll fix that up...

@kumar303
Copy link
Contributor

r+wc -- mostly nits but I think there is a bit of test duplication that won't be very helpful (it will just slow down the suite).

@muffinresearch muffinresearch force-pushed the add-app-redirection-and-vary branch from 97c66b4 to 8f7d708 Compare July 27, 2016 12:55
@muffinresearch
Copy link
Contributor Author

@kumar303 - can you take a look at the last commit. I ended up needing to refactor some of the i18n utils to make them more clear.

kumar303
kumar303 reviewed Jul 27, 2016
View reviewed changes
tests/client/core/test_utils.js Outdated
describe('isValidClientApp', () => {
const fakeConfig = new Map();
fakeConfig.set('validClientApplications', ['firefox', 'android']);
// eslint-disable-next-line no-underscore-dangle
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added this rule in #819 If you rebase on master you'll get it.

@muffinresearch muffinresearch force-pushed the add-app-redirection-and-vary branch from 8f7d708 to 34232d5 Compare July 27, 2016 15:14
@kumar303
Copy link
Contributor

For amo/TestViews.js and disco/TestViews.js, I think it would be helpful to keep one test in each that asserts the presence of the prefix redirector. Ideally, the best test would be to "assert app uses prefixMiddleWare" but I don't know how to do that. The next best assertion would be just to check one redirect, like /whatever redirects to /en-US/firefox/whatever. This proves that the middleware is configured for the app. It's not necessary to test all redirect behavior though.

@kumar303
Copy link
Contributor

The new changes look good. getLangFromHeader() calls normalizeLang enough times to hurt my head but I can't really find anything wrong with it :)

r+wc

@muffinresearch muffinresearch force-pushed the add-app-redirection-and-vary branch from 34232d5 to 427da74 Compare July 27, 2016 15:30
@muffinresearch
Add application redirects + vary headers
3cc8d2d 
Clean-up and improve naming

Fix review comments, delete duped tests and improve i18n utils

Remove no-underscore-dangle comments
@muffinresearch muffinresearch force-pushed the add-app-redirection-and-vary branch from 427da74 to 3cc8d2d Compare July 27, 2016 15:37
@muffinresearch muffinresearch merged commit a5bdddc into mozilla:master Jul 27, 2016
@muffinresearch muffinresearch deleted the add-app-redirection-and-vary branch July 27, 2016 15:42
@muffinresearch muffinresearch mentioned this pull request Jul 27, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@kumar303 kumar303

@mstriemer mstriemer

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement the client application redirection

3 participants

@muffinresearch @kumar303 @mstriemer