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

fix(dev): allow passing mock country for geo-based redirects #5093

Merged
merged 3 commits into from
Sep 29, 2022

Conversation

tinfoil-knight
Copy link
Contributor

@tinfoil-knight tinfoil-knight commented Sep 26, 2022

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixes #5051

The dev command doesn't allow a way for users to pass any flags to mock the current country to test out country based redirects which hinders local dev workflow.

Also, the "country" value used for redirects just defaults to US with no way to over-ride it which causes all requests for a route to be redirected in case a redirect rule with country value as US exists & prevents users from checking how the page without redirect looks.

This PR uses the value from pre-existing country flag for redirects. This allows the user to pass country values easily through the command line flag & test out country-based redirects properly.


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)
🦆

@github-actions
Copy link

github-actions bot commented Sep 26, 2022

📊 Benchmark results

Comparing with 138681d

Package size: 223 MB

(no change)

^  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB  223 MB 
│   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

Comment on lines 36 to 38
const getCountry = function (geoCountry) {
return geoCountry || 'us'
}
Copy link
Contributor Author

@tinfoil-knight tinfoil-knight Sep 26, 2022

Choose a reason for hiding this comment

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

Discussed w/ @kitop on Slack. Not sure why this fallback to 'us' exists but we decided to preserve it for backwards compatibility.

@tinfoil-knight
Copy link
Contributor Author

tinfoil-knight commented Sep 26, 2022

@JWhist Just tagging you here since the country flag was originally added by you in #4708 for edge functions & I'm re-using it for redirects.

eduardoboucas
eduardoboucas previously approved these changes Sep 26, 2022
danez
danez previously approved these changes Sep 27, 2022
@@ -80,7 +80,7 @@ const createRewriter = async function ({ configPath, distDir, jwtRoleClaim, jwtS
const cookieValues = cookie.parse(req.headers.cookie || '')
const headers = {
'x-language': cookieValues.nf_lang || getLanguage(req.headers),
'x-country': cookieValues.nf_country || getCountry(),
'x-country': cookieValues.nf_country || getCountry(geoCountry),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just simplify that instead of having a function for that?

Suggested change
'x-country': cookieValues.nf_country || getCountry(geoCountry),
'x-country': cookieValues.nf_country || geoCountry || 'us',

Copy link
Contributor Author

@tinfoil-knight tinfoil-knight Sep 27, 2022

Choose a reason for hiding this comment

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

@lukasholzer Made the suggested change.

Copy link
Contributor

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution but can we keep it simple? I think we don't need a function for that one

@tinfoil-knight tinfoil-knight dismissed stale reviews from danez and eduardoboucas via f36418f September 27, 2022 09:46
@tinfoil-knight tinfoil-knight added the automerge Add to Kodiak auto merge queue label Sep 29, 2022
@kodiakhq kodiakhq bot merged commit 09009f0 into netlify:main Sep 29, 2022
@tinfoil-knight tinfoil-knight deleted the fix/country-based-redirects branch September 29, 2022 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: command: dev area: redirects automerge Add to Kodiak auto merge queue type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redirect rule with US country condition blocks every country
4 participants