[fix bug 1392796] Remove old Fx desktop & private browsing pages.#5070
Conversation
alexgibson
left a comment
alexgibson
left a comment
There was a problem hiding this comment.
Looks good on a first pass. A few other spots that might need updating:
- The family nav functional tests still reference some pages being removed. It may be a good idea to run this branch against
run-integration-tests. media/js/firefox/private-browsing.jscan be removed.tests/functional/firefox/desktop/__init__.pycan be removed.
I'll take another pass for good measure before merging.
| redirect(r'^firefox/customize/?$', 'firefox.desktop.customize'), | ||
| redirect(r'^firefox/(?:performance|happy|speed|memory)/?$', 'firefox.desktop.fast'), | ||
| redirect(r'^firefox/security/?$', 'firefox.desktop.trust'), | ||
| # TODO: verify URL below |
| redirect(r'^firefox/desktop/fast/?', 'firefox.features.fast'), | ||
| redirect(r'^firefox/desktop/trust/?', 'firefox.features.independent'), | ||
| redirect(r'^firefox/desktop/tips/?', 'firefox.features.index'), | ||
| # TODO: verify link below |
| url_test('/firefox/customize/', '/firefox/desktop/customize/'), | ||
| url_test('/firefox/{performance,happy,speed,memory}/', '/firefox/desktop/fast/'), | ||
| url_test('/firefox/security/', '/firefox/desktop/trust/'), | ||
| # TODO: verify URL below |
| # bug 1392796 | ||
| url_test('/firefox/desktop/trust/', '/firefox/features/independent/'), | ||
| url_test('/firefox/desktop/tips/', '/firefox/features/'), | ||
| # TODO: verify URL below |
| url(r'^firefox/ios/testflight', views.ios_testflight, name='firefox.ios.testflight'), | ||
| page('firefox/mobile-download', 'firefox/mobile-download.html'), | ||
| page('firefox/mobile-download/desktop', 'firefox/mobile-download-desktop.html'), | ||
| page('firefox/private-browsing', 'firefox/private-browsing.html', alt_canonical_paths={ |
There was a problem hiding this comment.
Searching through the repo I can still find several references to {{ url('firefox.private-browsing') }} which will need updating.
| @@ -1,87 +0,0 @@ | |||
| # This Source Code Form is subject to the terms of the Mozilla Public | |||
There was a problem hiding this comment.
I can still see references to this file in tests/functional/newsletter/test_newsletter_embed.py
There was a problem hiding this comment.
Is it weird that tests passed even with this outdated reference? Anyway, nice spot - will remove.
There was a problem hiding this comment.
Apparently I forgot functional tests do not run on PRs.
| }), | ||
| page('firefox/desktop/customize', 'firefox/desktop/customize.html'), | ||
| page('firefox/desktop/tips', 'firefox/desktop/tips.html'), | ||
| page('firefox/desktop/trust', 'firefox/desktop/trust.html'), |
There was a problem hiding this comment.
There's still a reference to url('firefox.desktop.trust') in firefox/dnt.html
| {% block body_class %}firefox-desktop{% endblock %} | ||
|
|
||
| {% block extrahead %} | ||
| <script src="{{ static('js/libs/modernizr.custom.cssanimations.js') }}"></script> |
There was a problem hiding this comment.
Can this JS file be removed? Seems like we can just use @supports these days?
There was a problem hiding this comment.
Ah, good catch. Looks like there's another orphaned Modernizr file in there, too.
df58fe7 to
fc1bf72
Compare
| redirect(r'^firefox/desktop/trust/?', 'firefox.features.independent'), | ||
| redirect(r'^firefox/desktop/tips/?', 'firefox.features.index'), | ||
| redirect(r'^firefox/desktop/customize/?', 'https://support.mozilla.org/kb/customize-firefox-controls-buttons-and-toolbars'), | ||
| redirect(r'^firefox/private-browsing/?', 'firefox.features.private-browsing'), |
There was a problem hiding this comment.
Do we need to remove the existing redirects for /en-US/firefox/private-browsing/ above on line 525, and /en-US/firefox/desktop/fast/ on line 529 above now that we have these newer all-locale redirects? We should probably update the redirect tests to reflect these changes too.
There was a problem hiding this comment.
Nice spot. I think we're fine just removing the en-US only redirects and their tests, as the new redirects/tests cover all locales.
There was a problem hiding this comment.
Are we missing a redirect test for /firefox/fast/?
There was a problem hiding this comment.
Are we missing a redirect test for /firefox/fast/?
Was that ever a live URL? 404's on prod currently. There is a redirect & test for /firefox/desktop/fast/...
There was a problem hiding this comment.
Sorry I meant /firefox/features/fast/ I can only see a test for the en-US redirect
There was a problem hiding this comment.
I have that test locally - waiting for integration tests to finish/pass before updating the PR. 😉
|
Latest passing integration test run: https://ci.us-west.moz.works/blue/organizations/jenkins/bedrock_multibranch_pipeline/detail/run-integration-tests/52/pipeline/ |
0f18c3c to
3533c79
Compare
3 participants
Description
Removes some pages deprecated by opening up new Fx hub to all locales:
/firefox/desktop/fast//firefox/desktop/trust//firefox/desktop/tips//firefox/desktop/customize//firefox/private-browsing/DO NOT MERGE while I verify the redirect for/firefox/desktop/customize/. Should be good for review though.I'm going to say the redirect for
/firefox/desktop/customize/is validated by the filing of bug 1393881.Issue / Bugzilla link
https://bugzilla.mozilla.org/show_bug.cgi?id=1392796
Testing
Ensure no regressions and that all associated files have been removed. Make sure appropriate redirects are in place.
Integration tests passing: https://ci.us-west.moz.works/blue/organizations/jenkins/bedrock_multibranch_pipeline/detail/run-integration-tests/51/pipeline/190/