Chris More [:cmore] Reporter
	Description • 11 years ago

Line 420:

http://viewvc.svn.mozilla.org/vc/projects/mozilla.com/trunk/.htaccess?view=markup

It reads:

RewriteCond %{HTTP_USER_AGENT} ^.*Firefox/(17.*|18.*|19.*|20.*|21.*)$

I believe Release Engineering updates this number and it doesn't look like it was updated for the release of Firefox 22.

I think it should be the past 3 versions, current version, next three versions. This will catch people a few versions old, current version, beta, aurora, and nightly visitors. It will need to be updated for every release of Firefox to keep moving the window forward. 

The need for this redirect will be removed when the unified firefox download managed in bug 868199 is resolved. This /firefox redirect will also be removed when the new Firefox Family website is released and hosted at /firefox.

I recommend this change for now and it also makes the regex a bit more simple:

RewriteCond %{HTTP_USER_AGENT} ^.*Firefox/(19|20|21|22|23|24|25)\..*$

This should match major versions 19-25 with any minor versions.

	Kohei Yoshino
	Comment 1 • 11 years ago

I think this update can be automated like Bug 705416.
Will send a PR to make .htaccess maintenance-free!

	Kohei Yoshino
	Comment 2 • 11 years ago

I mean Bug 736934.

	Kohei Yoshino
	Comment 3 • 11 years ago

Umm, please discard my comment 1 and 2. Bug 868199 may be resolved sometime soon, so automation won't be needed here...

(In reply to Chris More [:cmore] from comment #0)
> I recommend this change for now and it also makes the regex a bit more
> simple:
> 
> RewriteCond %{HTTP_USER_AGENT} ^.*Firefox/(19|20|21|22|23|24|25)\..*$
> 
> This should match major versions 19-25 with any minor versions.

Committed to trunk at r117967.

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 4 • 11 years ago

This broke a test, here's the pastebin: http://pastebin.mozilla.org/2623758

The test is https://github.com/mozilla/mcom-tests/blob/master/tests/test_redirect_landing.py#L40

Full stack and results at http://qa-selenium.mv.mozilla.com:8080/view/Mozilla.com/job/mozilla.com.trunk.saucelabs/1741/testReport/tests.test_redirect_landing/TestRedirectLanding/test_redirect_locale_firefox/

	Kohei Yoshino
	Comment 5 • 11 years ago

Will send a PR to update the test.

	Pascal Chevrel:pascalc (PTO until August 21)
	Comment 6 • 11 years ago

Kohei, shouldn't the esr version (17) Not redirect as an obsolete version?

	Chris More [:cmore] Reporter
	Comment 7 • 11 years ago

(In reply to Pascal Chevrel:pascalc from comment #6)
> Kohei, shouldn't the esr version (17) Not redirect as an obsolete version?

I thought about that too, but that is a bit more tricky because you can't tell the difference between someone who is actually running 17 and those who are running ESR 17. Probably most people who are running Firefox ESR in companies don't even have admin permissions to be able to install. The current version of /firefox/new/ doesn't tell them they are obsolete, but the upcoming unified /firefox/new/ will tell them. I will make a note in the unified project to decide how to handle this. We could add 17 only and skip 18 in the regex.

	Paul [:pmac] McLanahan Assignee
	Comment 8 • 11 years ago

I think it's very unlikely that they'd have 17 but not have been auto-upgraded unless they were ESR. And the main point like you said is that they'd have ESR because their IT dept. is managing their Fx install and they can't do anything about upgrading if they wanted to, so it's pointless and confusing to tell them they're out of date.

	Chris More [:cmore] Reporter
	Comment 9 • 11 years ago

I like Pmac's idea to move this logic into bedrock.

How about:

1) Change: https://github.com/mozilla/bedrock/blob/master/bedrock/firefox/firefox_details.py#L31

To:

esr_major_versions = [10, 17, 24, 31, 38, 45, 52, 59, 66, 73, 80, 87, 94] (for future proofing)

2) Do a bedrock redirect using isFirefoxUpToDate logic where:

/firefox/fx/ is up to date and /firefox/new/ is not up to date.

3) Remove the redirect in .htaccess

The only issue is this will all be going away the unified download page goes live or the Firefox family page, whatever is first removes the need for this /firefox redirect. Should we move it into bedrock now or just make the .htaccess change?

	Paul [:pmac] McLanahan Assignee
	Comment 10 • 11 years ago

I'm in favor of going ahead and moving it to bedrock now so that no matter how long the other things take to deploy, we'll not have to do this again.

	Chris More [:cmore] Reporter
	Comment 11 • 11 years ago

(In reply to Paul McLanahan [:pmac] from comment #10)
> I'm in favor of going ahead and moving it to bedrock now so that no matter
> how long the other things take to deploy, we'll not have to do this again.

+1. Let's do it. Kohei?

	Paul [:pmac] McLanahan Assignee
	Comment 12 • 11 years ago

PR for porting /firefox/ to bedrock:

https://github.com/mozilla/bedrock/pull/1061

	Kohei Yoshino
	Comment 13 • 11 years ago

(In reply to Stephen Donner [:stephend] from comment #4)
> This broke a test, here's the pastebin: http://pastebin.mozilla.org/2623758

Sent a PR to fix the test.
https://github.com/mozilla/mcom-tests/pull/214

	[github robot]
	Comment 14 • 11 years ago

Commit pushed to master at https://github.com/mozilla/bedrock

https://github.com/mozilla/bedrock/commit/7af779ebe8900171e8e2972c989a3900ac4d3afb
Bug 892716: Handle /firefox/ redirects in bedrock.

This handles old, current, future, and ESR Fx versions,
as well as non-Fx browsers.

	Paul [:pmac] McLanahan Assignee
	Comment 15 • 11 years ago

(In reply to Kohei Yoshino [:kohei] from comment #13)
Oops... Sorry if I stepped in on you there Kohei. 

Once this is in prod it still may not do much until we remove the old rewrite rule from .htaccess, which we should NOT do until this code is in prod.

	Paul [:pmac] McLanahan Assignee
	Comment 16 • 11 years ago

This has been fixed by moving it to bedrock. The redirect at /firefox/ is now being handled by bedrock, and so this rule in .htaccess should be deleted.

This is however a rule that Release Management updates for every release. So we should tell them that these updates are no longer required and so they're not surprised to find it missing.

	Kohei Yoshino
	Comment 17 • 11 years ago

Closing as per Comment 16.