Fix bug 790784: Port Websites Privacy Policy #933
Conversation
|
Probably easiest to review each of commit individually, as 389ef5f reformats everything. A couple of questions from my end.
|
|
One more: Once this lands, I need to update the following redirects. (We already have redirects set up for these pages in mozilla.org/.htaccess, but they currently point elsewhere).
I assumed I would need to update these in mozilla.org/.htaccess, but I guess I could write them into global.conf instead. Would that be better? I guess it depends on whether we plan to keep mozilla.org/.htaccess around long term. |
|
@sgarrity mentioned that all new redirects should be written into global.conf. These redirects will do some of the same work covered in mozilla.org/.htaccess redirects, so I might want to delete those as part of this work. |
| @@ -14,227 +14,399 @@ | |||
| <div id="main-content"> | |||
| <div class="row"> | |||
There was a problem hiding this comment.
Now that this page is without a sidebar, the <div class="row"> and class="span7" on the <article ...> tag are no longer necessary. The article tag should also get a .span-all(); mixin to provide the left/right margin to make sure the content lines up to the grid.
|
|
@sgarrity: Thanks for the feedback. Updated the PR with that. |
|
Rebased! |
|
Design/layout changes look fine. Did you determine if you'll move the redirects into global.conf? |
|
Yep. I was told that new redirects should be written into global.conf, so I was sure to do that in this commit. The redirects added here should cover all Website Privacy Policy URLs being used right now (see the first comment of bug 878129). The only question is how these redirects will play with the ones already in mozilla.org/.htaccess (which redirect the same from addresses to different to addresses), but in my local testing these redirects in global.conf took precedence. |
|
Actually, I may have missed one redirect. Checking... |
|
I take that back. We need four redirects (three from the pages outlined in bug 878129, and one from the official page) and all of those are covered in my redirects -- it's just that the first However, there might be another issue here. My |
|
Fixed the issue I mentioned in my last comment. This also obsoletes bug 887084, which I opened after writing my last comment. The only thing worth mentioning at this point is that |
|
The layout/styles/content are r+ from me, but I'd defer to a wiser soul to review the redirects. |
|
I think @pmclanahan said he could take a look at that. |
| # NB: The /foundation/privacy-policy.html redirect below must appear before the | ||
| # foundation redirects added with bug 724633. Otherwise, the address will first | ||
| # be prefixed with a locale, and this redirect will not work. | ||
| RewriteRule ^/(\w{2,3}(?:-\w{2}(?:-mac)?)?/)?privacy-policy(\.html)?$ /b/$1privacy/policies/websites/ [L,R=301] |
There was a problem hiding this comment.
These are all confusing. They're redirects, but they're using the /b/ urls. The /b/ is a private url prefix only intended to be used by an apache [PT] rewrite rule. Is there a [PT] rule for this page? If so I think all that needs doing is to remove the /b from these.
There was a problem hiding this comment.
I don't see one. So my guess is that you need to remove those /b prefixes for these rules, and also add:
RewriteRule ^/(\w{2,3}(?:-\w{2})?/)?privacy/policies/websites(/?)$ /b/$1privacy/policies/websites$2 [PT]You also don't need the (?:-mac)? bit anymore.
There was a problem hiding this comment.
Thanks for the feedback. My understanding was that /b/ needed to be used for all Bedrock redirect targets. Is this not correct?
There was a problem hiding this comment.
Posted this reply in IRC already, but for the record:
You need to redirect /path to /b/path for bedrock to handle the request, but that has to be a separate pass-through redirect. If you need to redirect a different URL to that path, it has to be a separate redirect (one to rewrite the old->new URL, and another to force bedrock to serve the new url (/b).
There was a problem hiding this comment.
That is indeed not correct. If you look at all the rules in this file, you'll see that only the ones that end with [PT] use /b/, as the [PT] means "pass through". It tells apache to proxy matching requests to that url and serve the result. It's how we have bedrock installed. When it's a redirect however (one with [L,R=301]) it doesn't (or shouldn't) use the /b/ because we never want the user to ever see the /b/ urls.
|
Makes sense. Thanks for explaining that to me. Updated the commit to fix the issue. |
| Members section, above, it discloses that information only to those of its employees, contractors, service providers, and subsidiaries | ||
| and related organizations that need to know that information in order to process it on Mozilla's behalf and that have agreed not to | ||
| disclose it to others. Mozilla endeavors to maintain an up-to-date list of its subsidiaries and related organizations at | ||
| <a href="http://www.mozilla.org/about/organizations.html">http://www.mozilla.org/about/organizations.html</a>, however we don’t guarantee this list to be complete. As of the date of this update, |
There was a problem hiding this comment.
This organizations URL is on bedrock now and can use url()
|
We talked over this one on the weekly PR review call. pmac says the redirects look good. Any mozilla links should be HTTPS. After that and a rebase, we should be good to go. What about l10n? We don't need to block on it right now. |
|
Rebased and updated the links. Craig is updating the corresponding PDF right now -- I will push my updated PR once that is done. As for l10n, I could wrap the content in l10n blocks, but I thought I should not given that this is policy. At least, that is the precedent I saw with other policies hosted on mozilla.org. |
|
PDF is up, so we should be all set. |
|
Looking good! 🍻 |
Fix bug 790784: Port Websites Privacy Policy
No description provided.