1044411 - Host the ToS

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Tarek Ziadé (:tarek)]

We need to add that https://github.com/mozilla/legal-docs/blob/master/WebRTC_ToS/en-US.md to our repo and host them.

Bob/Shell to decide what will be the url

Comment 1

[Alexis Metaireau (:alexis)]

Cool.

What will be the client for this: will this URL be accessed directly by web browsers or is it something that will be displayed to the end-user via a client that will format it?

(e.g. do we need to return a json blob or should this be a styled page?)

Comment 2

[Mark Banner (:standard8)]

Is there any expected L10n of this file, if so, why don't we use normal www.mozilla.org website?

Comment 3

[Maire Reavy [:mreavy]]

Hi Geoff -- Why is this assigned to you?  What's the status of this?  Thanks!

Comment 4

[Geoff Piper]

I am handling this part as carry over from my previous Loop/Hello! Legal Product duties. I have not heard this bring resolved yet. To date, I have not seen a dedicated URL picked for hosting the TOS. I am not sure who is leading that effort, but I believe Shell and/or Romain were managing this.

Comment 5

[Maire Reavy [:mreavy]]

Both RT and Shell are on PTO this week.  Who is choosing the URL?  (I thought it was Legal Product and/or Product Marketing.)  Can we use a normal www.mozilla.org website as Mark suggests in Comment 2 above?

Comment 6

[Tarek Ziadé (:tarek)]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #5)
> Both RT and Shell are on PTO this week.  Who is choosing the URL?  (I
> thought it was Legal Product and/or Product Marketing.)  

Me too.

> Can we use a normal
> www.mozilla.org website as Mark suggests in Comment 2 above?


FWIW Shell proposed in a mail thread to use something similar to  "https://accounts.firefox.com/legal/terms"

So I'd suggest "https://hello.firefox.com/legal/terms" maybe?

Comment 7

[Maire Reavy [:mreavy]]

(In reply to Tarek Ziadé (:tarek) from comment #6)
> (In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #5)
> > Both RT and Shell are on PTO this week.  Who is choosing the URL?  (I
> > thought it was Legal Product and/or Product Marketing.)  
> 
> Me too.
> 
> > Can we use a normal
> > www.mozilla.org website as Mark suggests in Comment 2 above?
> 
> 
> FWIW Shell proposed in a mail thread to use something similar to 
> "https://accounts.firefox.com/legal/terms"
> 
> So I'd suggest "https://hello.firefox.com/legal/terms" maybe?

This works for me. Thanks, Tarek.  

Geoff -- To unblock this bug, I'd like to go with it.  Is there anyone other than you that we need to check with?   I'd like to unblock this bug today.  Thanks.

Comment 8

[Geoff Piper]

For everyone's benefit, the link (https://hello.firefox.com/legal/terms) works for now with the caveat that it may well migrate later relating to Firefox Cloud Services and that overall plan and vision.

I believe what the actual URL is to be is better decided by Arcadio with others. 

Arcadio, is this the main domain for URLs for Hello!?

Comment 9

[alainez]

Hi All

Confirming https://hello.firefox.com/legal/terms is the URL. Thanks for adding the additional context around the accounts.firefox.com legal terms.

Comment 10

[Maire Reavy [:mreavy]]

Thanks, Arcadio!

Hi Tarek -- We're good to go.  Please use https://hello.firefox.com/legal/terms and reassign this bug to proper Loop server developer as soon as you can.  I'll ask someone to update the client side (bug 1035846) as soon as this bug is resolved.  Thanks!

Comment 11

[Tarek Ziadé (:tarek)]

This should be directly dealt by an Ops - assigning Benson. Benson, please let me know if I should assign someone else.

Comment 12

[Benson Wong [:mostlygeek]]

So a task list to make https://hello.firefox.com/legal/terms work: 

- get hello.firefox.com delegated to R53 so we can manage it (ops)
- get an SSL cert for hello.firefox.com (ops)
- get loop-client updated w/ the TOS static files (:standard8?)
- update puppet/cloudformation to serve new hello.f.c (ops)
- coordinate a release that uses hello.m.c (loop-dev, ops, fxa-dev?)
- done :)

Anything I'm missing?

Comment 13

[Tarek Ziadé (:tarek)]

(In reply to Ben Wong [:mostlygeek] from comment #12)
> So a task list to make https://hello.firefox.com/legal/terms work: 
> 
> - get hello.firefox.com delegated to R53 so we can manage it (ops)
> - get an SSL cert for hello.firefox.com (ops)
> - get loop-client updated w/ the TOS static files (:standard8?)

The TOS static files need to be in their own repo, e.g. https://github.com/mozilla/legal-docs
And that is to be dealt by Geoff I believe. 

> - update puppet/cloudformation to serve new hello.f.c (ops)
> - coordinate a release that uses hello.m.c (loop-dev, ops, fxa-dev?)
> - done :)
> 
> Anything I'm missing?

Comment 14

[Geoff Piper]

Tarek, I believe they already located in GitHub here: 

https://github.com/mozilla/legal-docs/blob/master/WebRTC_ToS/en-US.md

Is this not enough for the interim?

Are we saying they need to be in this different repository? If so why?

I believe the longer range vision of Firefox Cloud Services may change organization further, and I believe we are getting the Hello! Terms of Service up now (knowing we will be moving this again probably) for Nightly and Aurora purposes as it has needed to be there since its first release.

Comment 15

[Benson Wong [:mostlygeek]]

Yes, the TOS is already in the legal-docs repo. However, I can't serve a markdown file, it needs to be converted to HTML and presumably with some CSS/HTML styling on it.

Comment 16

[Tarek Ziadé (:tarek)]

I can add what's missing to convert all the .md into static files, sure. 

Chris, could you point me to the code that does it for FxA so I can copy it over ? thanks!

Also, are we using an Nginx rule for picking the right language page or something else ?

Comment 17

[Chris Karlof [:ckarlof]]

> Chris, could you point me to the code that does it for FxA so I can copy it over ? thanks!

We have the legal repo as a bower dep: https://github.com/mozilla/fxa-content-server/blob/master/bower.json#L21

We rely on a the grunt plugin "marked", and have a grunt task to convert the md files to html:

https://github.com/mozilla/fxa-content-server/blob/master/grunttasks/l10n-generate-tos-pp.js
https://github.com/mozilla/fxa-content-server/blob/master/grunttasks/marked.js

Vlad or Zaach can help with the details if you have further questions.

> Also, are we using an Nginx rule for picking the right language page or something else ?

In FxA, it's handled in our app using the path (e.g., /en/legal/privacy), but it also uses the Accept-Language header if the language isn't specified in the path.

Comment 18

[Chris Karlof [:ckarlof]]

Here's the grunt plugin we use: https://github.com/gobwas/grunt-marked

Comment 19

[Tarek Ziadé (:tarek)]

Thanks for all the details. I guess we can server that page from loop-server to simplify the deployment.

Comment 20

[Tarek Ziadé (:tarek)]

Natim will take care of the md => html conversion bit copying over FxA's behavior while Benson sets the routing/aws work.

Comment 21

[Rémy Hubscher (:natim)]

Is loop-server going to be served on hello.firefox.com or will it be loop-client?

I am wondering if we shouldn't have the ToS served as part as Loop-Client rather than on the server.
I am going to set it up on the loop-server for now and we can move it later.

For now the production URL will be https://loop.services.mozilla.com/legal/terms with en-US locale only.
The script will handle multiple locale the same way as Firefox Account does.

Comment 22

[Rémy Hubscher (:natim)]

Attachment: Link to github PR

There is still a problem about the {{ lang_dir }} and {{ lang }} rendering. Any idea of what part is missing there?

Also I removed the {{#t}} {{/t}} for now but we probably want them. Any idea of how I can configure it correctly?

Thank you,

Rémy

Comment 23

[Chris Karlof [:ckarlof]]

> There is still a problem about the {{ lang_dir }} and {{ lang }} rendering. Any idea of what part is missing there?

I believe you can get it from the i18n-abide module:

https://github.com/mozilla/i18n-abide/blob/master/docs/USAGE.md#variables

e.g., see:

https://github.com/mozilla/fxa-content-server/blob/555633b3e4f2071e3c2237683cd7f809bd4237b8/server/lib/i18n.js

Comment 24

[Chris Karlof [:ckarlof]]

Comments in the github PR.

Comment 25

[:shell escalante]

Urmika (legal for ToS - working with Geoff) confirmed "US for now is fine - we'll work on localization when it's final, and get it pushed to production to the legal center (shd be done in Q3)."

Comment 26

[Rémy Hubscher (:natim)]

Ok I did this PoC to have the terms served on loop-client side instead of loop-server one: https://github.com/mozilla/loop-client/pull/40

Comment 27

[Rémy Hubscher (:natim)]

This will be handled on loop-client.

Comment 28

[Rémy Hubscher (:natim)]

Attachment: Link to github PR

Comment 29

[Maire Reavy [:mreavy]]

Hi Remy -- Standard8 is on PTO this week, and the tentative agreement/understanding last week was that once the Loop server work for this was done, we would simply update the ToS URL that the client points to (Bug 1035846).   

Can you tell me what has changed?  Specifically, what needs to be done on the client side to resolve this bug and Bug 1035846?   Thanks.

Comment 30

[Rémy Hubscher (:natim)]

Well the thing is that we need to serve the ToS somewhere.

The first attempt was to serve it on loop-server, but it doesn't make sense since loop-server doesn't handle nor HTML nor i18n.

The change on the loop-server side were made here: https://github.com/mozilla-services/loop-server/pull/171

The we decide to serve them with loop-client.
It make more sense for the URL chosen as well as for the management on CSS/JS that is already taken care of for loop-client.

At first we though it will be more complicated to have it on the loop-client side but it looks like it is quite fast forward.

The code stand here: https://github.com/mozilla/loop-client/pull/40

If Mark is on PTO, Dan should be able to handle it as well. All we need is to make a patch from the PR to m-c and once merge we will release loop-client 0.4.1 and update it.

Comment 31

[Maire Reavy [:mreavy]]

Remy -- Even though this would be change on the Loop client, is this something you could do for us?  

We are extremely short staffed this week.  (Lots of folks are on PTO.)  Today is Dmose's birthday (Happy Birthday, Dan!) and he's working today simply because we are trying to get so much done.  His head is deep in other code/other bugs which also need to get fixed.  Can you help us out on this bug?

Comment 32

[Rémy Hubscher (:natim)]

It is done it just needs a review and get in m-c.
Therefore it can wait until the next loop-client release we don't need it for tomorrow.

Comment 33

[Alexis Metaireau (:alexis)]

Maire, I'm probably one source of confusion here, and I apologies if that's the case.

Like Remy said, the only location where we currently have HTML + CSS (+l10n) is inside gecko (what we call the loop-webclient). It seems to make a lot of sense to have the ToS served there (it will be served at the same url the web client is served, which will be hello.firefox.com I believe).

The proposal remy did is all ready to be reviewed and merged, but the patch needs to be done against m-c, and we don't have any power to push code there.

Comment 34

[Adam Roach [:abr]]

Attachment: Add ToS on Client Side

Comment 35

[Adam Roach [:abr]]

The patch I just attached is pull request 40, but as a patch applied against m-c.

Comment 36

[Adam Roach [:abr]]

Comment on attachment 8475266 [details] [diff] [review]
Add ToS on Client Side

Review of attachment 8475266 [details] [diff] [review]:
-----------------------------------------------------------------

Putting the review request for Dan back on here (at least, for now)

Comment 37

[Maire Reavy [:mreavy]]

Adam -- Is this something you could review since Remy wrote the code or do you feel uncomfortable reviewing it?

Comment 38

[Adam Roach [:abr]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #37)
> Adam -- Is this something you could review since Remy wrote the code or do
> you feel uncomfortable reviewing it?

I'm looking it over right to see whether I understand enough of the related technologies to do a sufficient job of vetting it. That's why I said "(at least, for now)" in my previous comment. :-)

Comment 39

[Adam Roach [:abr]]

I might need some help with my understanding of Node.js deployments here, but a naïve view of what's going on here makes me think that we've got some path and component issues going on here.

When I try to load up the TOS, it's trying to get to the following files:

> /shared/libs/jquery-2.1.0.js
> /config.js

And these don't appear to be anywhere in the "standalone" tree (unless "config.js" is supposed to come from bower or grunt). What am I missing? Is there some way to run these locally so I can see everything working all together?

Comment 40

[Adam Roach [:abr]]

Attachment: Add ToS on Client Side

Comment 41

[Adam Roach [:abr]]

It turns out that the github patch export facility does not include binary files. This version of the patch has been updated to include the binary assets.

Comment 42

[Adam Roach [:abr]]

Attachment: Add ToS on Client Side

Comment 43

[Adam Roach [:abr]]

Comment on attachment 8475389 [details] [diff] [review]
Add ToS on Client Side

Review of attachment 8475389 [details] [diff] [review]:
-----------------------------------------------------------------

r-, at the very least for missing font files. I also have some concerns about incomplete l10n support.

In terms of the images, the approach on other moz properties appears to be that we land 4x oversized pictures and then scale them with css, so that they look good on hiDPI displays.

I don't understand grunt well enough to feel confident evaluating those files. I also think that someone more familiar with CSS should take a look at that file.

Finally, I don't see the fontello font being used anywhere, which leads me to believe that we can remove those assets altogether. If we don't remove them, then we need to know what our normal license-related process is for using SIL-licensed fonts on web sites is and make sure we're in compliance.

::: browser/components/loop/standalone/content/legal/fonts/icons/fontello.svg
@@ +1,4 @@
> +<?xml version="1.0" standalone="no"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +<svg xmlns="http://www.w3.org/2000/svg">
> +<metadata>Copyright (C) 2014 by original authors @ fontello.com</metadata>

For this and the other fontello files: we probably need to touch base with Gerv to make sure we're doing the right thing regarding license terms. I believe the license that applies here is http://scripts.sil.org/cms/scripts/page.php?site_id=nrsi&id=OFL

::: browser/components/loop/standalone/content/legal/privacy/index.html
@@ +17,5 @@
> +        <div id="fox-logo" class="static"></div>
> +        <div id="stage">
> +          <div id="main-content">
> +            <!--[if lt IE 10]>
> +                <p class="error browsehappy">You are using an <strong>outdated</strong> browser. Please <a href="http://browsehappy.com/">upgrade your browser</a> to improve your experience.</p>

How does this string get localized?

@@ +21,5 @@
> +                <p class="error browsehappy">You are using an <strong>outdated</strong> browser. Please <a href="http://browsehappy.com/">upgrade your browser</a> to improve your experience.</p>
> +            <![endif]-->
> +            <header id="legal-header">
> +              <h3>WebRTC</h3>
> +              <h1 id="webrtc-pp-header">Privacy Notice</h1>

How does this string get localized?

@@ +26,5 @@
> +            </header>
> +
> +            <section>
> +              <article id="legal-copy">
> +                  Loading...

It seems a little counter-intuitive to have an English word stand in for "hold on while I load your language" -- shouldn't this be something iconic, like an hourglass? I note that the fontello sets appear to include a couple of hourglass glyphs that we could choose from.

@@ +30,5 @@
> +                  Loading...
> +              </article>
> +
> +              <div class="button-row">
> +                <a href="/" id="webrtc-pp-home" class="button">Home</a>

Given that the root directory of the Hello server is of very limited utility, I think this should be removed. (If not removed: how does this get localized?)

@@ +36,5 @@
> +            </section>
> +          </div>
> +        </div>
> +
> +        <a id="about-mozilla" rel="author" target="_blank" href="https://www.mozilla.org/about/?utm_source=firefox-accounts&amp;utm_medium=Referral"></a>

"utm_source=firefox-accounts" doesn't seem right here.

::: browser/components/loop/standalone/content/legal/styles/main.scss
@@ +1,3 @@
> +@font-face { font-family: fontello; src: url(/fonts/icons/fontello.eot#iefix) format(embedded-opentype), url(/legal/fonts/icons/fontello.woff) format(woff), url(/legal/fonts/icons/fontello.ttf) format(truetype), url(/legal/fonts/icons/fontello.svg#fontello) format(svg); font-weight: 400; font-style: normal; }
> +
> +@font-face { font-family: 'Clear Sans'; font-style: normal; font-weight: 400; src: local(Clear Sans), local(ClearSans), url(/legal/fonts/latin/clearsans-regular.eot?#iefix) format(embedded-opentype), url(/legal/fonts/latin/clearsans-regular.woff) format(woff), url(/legal/fonts/latin/clearsans-regular.ttf) format(truetype), url(/legal/fonts/latin/clearsans-regular.svg#clearsans-regular) format(svg); }

This font doesn't appear to be in the patch (or the associated pull request that I created it from).

@@ +1,5 @@
> +@font-face { font-family: fontello; src: url(/fonts/icons/fontello.eot#iefix) format(embedded-opentype), url(/legal/fonts/icons/fontello.woff) format(woff), url(/legal/fonts/icons/fontello.ttf) format(truetype), url(/legal/fonts/icons/fontello.svg#fontello) format(svg); font-weight: 400; font-style: normal; }
> +
> +@font-face { font-family: 'Clear Sans'; font-style: normal; font-weight: 400; src: local(Clear Sans), local(ClearSans), url(/legal/fonts/latin/clearsans-regular.eot?#iefix) format(embedded-opentype), url(/legal/fonts/latin/clearsans-regular.woff) format(woff), url(/legal/fonts/latin/clearsans-regular.ttf) format(truetype), url(/legal/fonts/latin/clearsans-regular.svg#clearsans-regular) format(svg); }
> +
> +@font-face { font-family: 'Fira Sans'; font-style: normal; font-weight: 400; src: local(Fira Sans), local(FiraSans-Regular), url(/legal/fonts/latin/firasans-regular.eot?#iefix) format(embedded-opentype), url(/legal/fonts/latin/firasans-regular.woff) format(woff), url(/legal/fonts/latin/firasans-regular.ttf) format(truetype), url(/legal/fonts/latin/firasans-regular.svg#firasans-regular) format(svg); }

Neither does this one.

@@ +3,5 @@
> +@font-face { font-family: 'Clear Sans'; font-style: normal; font-weight: 400; src: local(Clear Sans), local(ClearSans), url(/legal/fonts/latin/clearsans-regular.eot?#iefix) format(embedded-opentype), url(/legal/fonts/latin/clearsans-regular.woff) format(woff), url(/legal/fonts/latin/clearsans-regular.ttf) format(truetype), url(/legal/fonts/latin/clearsans-regular.svg#clearsans-regular) format(svg); }
> +
> +@font-face { font-family: 'Fira Sans'; font-style: normal; font-weight: 400; src: local(Fira Sans), local(FiraSans-Regular), url(/legal/fonts/latin/firasans-regular.eot?#iefix) format(embedded-opentype), url(/legal/fonts/latin/firasans-regular.woff) format(woff), url(/legal/fonts/latin/firasans-regular.ttf) format(truetype), url(/legal/fonts/latin/firasans-regular.svg#firasans-regular) format(svg); }
> +
> +@font-face { font-family: 'Fira Sans'; font-style: normal; font-weight: 300; src: local(Fira Sans Light), local(FiraSans-Light), url(/legal/fonts/latin/firasans-light.eot?#iefix) format(embedded-opentype), url(/legal/fonts/latin/firasans-light.woff) format(woff), url(/legal/fonts/latin/firasans-light.ttf) format(truetype), url(/legal/fonts/latin/firasans-light.svg#firasans-light) format(svg); }

Neither does this one.

::: browser/components/loop/standalone/content/legal/terms/index.html
@@ +17,5 @@
> +        <div id="fox-logo" class="static"></div>
> +        <div id="stage">
> +          <div id="main-content">
> +            <!--[if lt IE 10]>
> +                <p class="error browsehappy">You are using an <strong>outdated</strong> browser. Please <a href="http://browsehappy.com/">upgrade your browser</a> to improve your experience.</p>

How does this get localized?

@@ +20,5 @@
> +            <!--[if lt IE 10]>
> +                <p class="error browsehappy">You are using an <strong>outdated</strong> browser. Please <a href="http://browsehappy.com/">upgrade your browser</a> to improve your experience.</p>
> +            <![endif]-->
> +            <header id="legal-header">
> +              <h3>WebRTC</h3>

Shouldn't this say "Firefox Hello"?

@@ +21,5 @@
> +                <p class="error browsehappy">You are using an <strong>outdated</strong> browser. Please <a href="http://browsehappy.com/">upgrade your browser</a> to improve your experience.</p>
> +            <![endif]-->
> +            <header id="legal-header">
> +              <h3>WebRTC</h3>
> +              <h1 id="webrtc-tos-header">Terms of Service</h1>

How does this get localized?

@@ +26,5 @@
> +            </header>
> +
> +            <section>
> +              <article id="legal-copy">
> +                Loading...

See earlier comments on privacy "Loading..."

@@ +30,5 @@
> +                Loading...
> +              </article>
> +
> +              <div class="button-row">
> +                <a href="/" id="webrtc-tos-home" class="button">Home</a>

See earlier comments on privacy "Home"

@@ +35,5 @@
> +              </div>
> +            </section>
> +          </div>
> +        </div>
> +        <a id="about-mozilla" rel="author" target="_blank" href="https://www.mozilla.org/about/?utm_source=firefox-accounts&amp;utm_medium=Referral"></a>

"utm_source=firefox-accounts" is probably not right here.

::: browser/components/loop/standalone/package.json
@@ +3,5 @@
>    "description": "Video conferencing app powered by WebRTC.",
>    "version": "0.0.1",
> +  "repository": {
> +    "type": "git",
> +    "url": "[e-mail]:mozilla/loop-client.git"

Given that this is not the authoritative repo for the client, it seems potentially confusing to include it here.

Comment 44

[Adam Roach [:abr]]

Comment on attachment 8475389 [details] [diff] [review]
Add ToS on Client Side

Dan -- I'm going to be fixing the issues with this patch (at least, the ones I can). I don't, however, understand grunt well enough to be comfortable signing off on those files. Could you give those a look?

Also, while I gave the CSS a quick look, I'm not a CSS expert, and would appreciate a second set of eyes there.

Comment 45

[Rémy Hubscher (:natim)]

> /config.js

This file is coming from make config

> jquery-2.1.0.js

I have it here: https://github.com/mozilla/loop-client/tree/master/content/shared/libs

> About the fonts

Feel free to remove the fonts, I just took the CSS from the FxA ToS.

> About l10n

I think we need to use the same mechanism that we have with the loop-client standalone even if for now there is only the English translation of the ToS.


> "url": "[e-mail]:mozilla/loop-client.git"

Well it is in sync with mozcentral and it is used for deployement. But we can change it to mozcentral.

dmose also asked me to add a README to the content/legal folder.

Comment 46

[Adam Roach [:abr]]

Remy: Thanks for your responses! I probably should have been clearer -- the code comments, I plan to take care of myself. The reason I tagged you with a needinfo is this question:

"Is there some way to run these locally so I can see everything working all together?"

Comment 47

[Rémy Hubscher (:natim)]

Following up our discussion on IRC, the way I was using it is going to loop-client/content:

   cd loop-client
   make tos config
   cd content/
   python -m SimpleHTTPServer 3000

   cd loop-server/
   make runserver

   firefox http://localhost:3000/legal/terms

Comment 48

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: TOS changes with fixed dev server.

Comment 49

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Add term on client side.

Comment 50

[Dan Mosedale (:dmosedale, :dmose)]

This version of the patch has an updated dev-server so that the legal magic can be seen through "make runserver", and it has a syntax error fix from github, as well as some gitignore changes.

I now really really really want the dev-server (at the very least all the remapping stuff) to go away ASAP, as we currently have multiple views on the same HTML without robust, DRY infrastructure to support that, which I expect us to trip on repeatedly between dev and deployment until we fix.  I'll file a bug about that. :-)

Comment 51

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Generate Loop ToS static content, with dev-server fixes

Comment 52

[Rémy Hubscher (:natim)]

Comment on attachment 8479297 [details] [diff] [review]
Generate Loop ToS static content, with dev-server fixes

We still have to translate english inside both privacy/index.html and terms/index.html files

Also as far as I see the terms/index.html is missing from this patch.

Comment 53

[Mark Banner (:standard8)]

I think we could split that translation of the odd button/link out to a different bug. Its likely we could pick something up from the mozilla.org site, but that needs some thinking about.

Given last I looked our ToS only had a English translation, I think its reasonable to do.

Comment 54

[Rémy Hubscher (:natim)]

> Given last I looked our ToS only had a English translation, I think its reasonable to do.

Yes I agree.

Comment 55

[Adam Roach [:abr]]

Attachment: Generate Loop ToS static content

Comment 56

[Adam Roach [:abr]]

Comment on attachment 8479869 [details] [diff] [review]
Generate Loop ToS static content

r? to dmose for the grunt files. I still haven't had time to address my own review comments, although I've fixed the image resolution issue with the Firefox logo and fixed some font issues (so please don't respin this patch from git yet again!)

I'm going to have to back-burner this for a short while as I land some things that I'd like to have on m-c prior to uplift (notably, the load-management mechanisms) -- I'll resume this later, probably next week.

Comment 57

[Dan Mosedale (:dmosedale, :dmose)]

Shell mentioned in standup today that in ~3 months, legal is planning to centralize hosting of all the ToS / privacy docs elsewhere, so we can definitely live with "good enough" here.


FWIW, I probably won't get back to this until next week either (apologies)!

Comment 58

[Dan Mosedale (:dmosedale, :dmose)]

Sorry for the long delay.  I just chatted with Adam, and he's going to try to get his parts of the review addressed and posted either today or Monday, and I'll be spending some time on Monday reviewing/updating the grunt bits.

Comment 59

[Adam Roach [:abr]]

Attachment: Generate Loop ToS static contentBug 1044411 - Generate Loop ToS static content

This addresses my review comments, except those involving localization. Given
that we will be migrating to a new hosting setup for legal documents shortly,
I'm happy waiting for that final solution to be put in place before we settle
on a final solution for localizing things like headers and titles.

Comment 60

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8485951 [details] [diff] [review]
Generate Loop ToS static contentBug 1044411 - Generate Loop ToS static content

Review of attachment 8485951 [details] [diff] [review]:
-----------------------------------------------------------------

We're not too far off now.  I'll finish up the grunt/CSS review and addressing of questions tomorrow.

Remy, if you could answer the questions above, it'd be super helpful in figuring out how to move the various bits forward.  Thanks!

::: browser/components/loop/standalone/.gitignore
@@ +4,5 @@
>  *.pyc
>  content/config.js
>  content/VERSION.txt
> +content/legal/styles/*.css
> +content/legal/terms/*.html

The above line is what caused terms/index.html to be omitted in previous patches.  For now, I'm going to fix this by adding 

content/legal/terms/!index.html

to .gitignore.

Ultimately, we're going to want to separate src files and built files into separate directories.

::: browser/components/loop/standalone/Gruntfile.js
@@ +7,5 @@
> +// # Globbing
> +// for performance reasons we're only matching one level down:
> +// 'test/spec/{,*/}*.js'
> +// use this if you want to recursively match all subfolders:
> +// 'test/spec/**/*.js'

What's the above comment related to?  Is it a leftover from something?

@@ +15,5 @@
> +  // show elapsed time at the end
> +  require('time-grunt')(grunt);
> +
> +  // load all grunt tasks
> +  require('load-grunt-tasks')(grunt, {scope: 'dependencies'});

Presumably, we'll want to change this to devDependencies assuming we edit package.json that way as well.

::: browser/components/loop/standalone/bower.json
@@ +1,3 @@
> +{
> +  "name": "loop-client",
> +  "version": "0.0.0",

Could we live without specifying a version, given that we don't have an real plans to publish and update this via bower?  More to the point, it's just one more version number to update that we're likely to forget.

::: browser/components/loop/standalone/grunttasks/replace.js
@@ +17,5 @@
> +        to: ''
> +      }, {
> +        from: /^#\s.*?\n$/m,
> +        to: ''
> +      }]

These need comments.  I'll also pull in pdehaan's changes from the github PR.

Remy, what's the high-level intent of the above replacements (and why do we need to do them)?

::: browser/components/loop/standalone/grunttasks/sass.js
@@ +7,5 @@
> +
> +  grunt.config('sass', {
> +    styles: {
> +      files: {
> +        '<%= yeoman.app %>/legal/styles/main.css': '<%= yeoman.app %>/legal/styles/main.scss'

These really want to be in separate dirs so that we don't serve up the SCSS, but since this still will be going away in a few months, it's probably not worth worrying about now.

::: browser/components/loop/standalone/grunttasks/yeoman.js
@@ +7,5 @@
> +
> +  var TEMPLATE_ROOT = 'content/legal';
> +  var TOS_PP_REPO_ROOT = 'bower_components/tos-pp';
> +
> +  grunt.config('yeoman', {

Remy, why is this task called yeoman?  It seems to suggest some relation to the well known yeoman project templating package, but it's not obvious to me what that would be.  Unless I'm misunderstanding, I'd prefer to call it something more explicitly related to its function so that it's easier to understand why it's here.  project-config maybe?

::: browser/components/loop/standalone/package.json
@@ +9,5 @@
>    "engines": {
>      "node": "0.10.x",
>      "npm":"1.3.x"
>    },
>    "dependencies": {

As discussed on IRC a while back, most of this stuff wants to be in devDependencies.  

Remy, you had also mentioned a mechanism that loop-server uses to keep these versions up-to-date, which I think we need to land at the same time as this file if we're really going to be specifying down to the minor version.

@@ +21,5 @@
> +    "time-grunt": "0.4.0",
> +    "load-grunt-tasks": "0.6.0",
> +    "i18n-abide": "0.0.22",
> +    "consolidate": "0.10.0",
> +    "handlebars": "1.3.0"

Why do we need handlebars?

Comment 61

[Dan Mosedale (:dmosedale, :dmose)]

s/if you could answer the questions above/if you could answer the questions below/

:-)

Comment 62

[Rémy Hubscher (:natim)]

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #60)
> Comment on attachment 8485951 [details] [diff] [review]
> Generate Loop ToS static contentBug 1044411 - Generate Loop ToS static
> content
> 
> Review of attachment 8485951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We're not too far off now.  I'll finish up the grunt/CSS review and
> addressing of questions tomorrow.
> 
> Remy, if you could answer the questions above, it'd be super helpful in
> figuring out how to move the various bits forward.  Thanks!

Yes sure.

> ::: browser/components/loop/standalone/.gitignore
> @@ +4,5 @@
> >  *.pyc
> >  content/config.js
> >  content/VERSION.txt
> > +content/legal/styles/*.css
> > +content/legal/terms/*.html
> 
> The above line is what caused terms/index.html to be omitted in previous
> patches.  For now, I'm going to fix this by adding 
> 
> content/legal/terms/!index.html
> 
> to .gitignore.
> 
> Ultimately, we're going to want to separate src files and built files into
> separate directories.

Ok it makes sense.

> 
> ::: browser/components/loop/standalone/Gruntfile.js
> @@ +7,5 @@
> > +// # Globbing
> > +// for performance reasons we're only matching one level down:
> > +// 'test/spec/{,*/}*.js'
> > +// use this if you want to recursively match all subfolders:
> > +// 'test/spec/**/*.js'
> 
> What's the above comment related to?  Is it a leftover from something?

Yes a leftover.

> @@ +15,5 @@
> > +  // show elapsed time at the end
> > +  require('time-grunt')(grunt);
> > +
> > +  // load all grunt tasks
> > +  require('load-grunt-tasks')(grunt, {scope: 'dependencies'});
> 
> Presumably, we'll want to change this to devDependencies assuming we edit
> package.json that way as well.

I have no idea, if it works why not.

> ::: browser/components/loop/standalone/bower.json
> @@ +1,3 @@
> > +{
> > +  "name": "loop-client",
> > +  "version": "0.0.0",
> 
> Could we live without specifying a version, given that we don't have an real
> plans to publish and update this via bower?  More to the point, it's just
> one more version number to update that we're likely to forget.

I have no idea, if it works why not.
 
> ::: browser/components/loop/standalone/grunttasks/replace.js
> @@ +17,5 @@
> > +        to: ''
> > +      }, {
> > +        from: /^#\s.*?\n$/m,
> > +        to: ''
> > +      }]
> 
> These need comments.  I'll also pull in pdehaan's changes from the github PR.
> 
> Remy, what's the high-level intent of the above replacements (and why do we
> need to do them)?

This is to remove comments that are not handled during the conversion to HTML.
The previous one is to remove tags also not handled. https://github.com/mozilla/legal-docs/blame/master/WebRTC_ToS/en-US.md#L4

> 
> ::: browser/components/loop/standalone/grunttasks/sass.js
> @@ +7,5 @@
> > +
> > +  grunt.config('sass', {
> > +    styles: {
> > +      files: {
> > +        '<%= yeoman.app %>/legal/styles/main.css': '<%= yeoman.app %>/legal/styles/main.scss'
> 
> These really want to be in separate dirs so that we don't serve up the SCSS,
> but since this still will be going away in a few months, it's probably not
> worth worrying about now.

Why could it be a problem to serve main.scss?

> ::: browser/components/loop/standalone/grunttasks/yeoman.js
> @@ +7,5 @@
> > +
> > +  var TEMPLATE_ROOT = 'content/legal';
> > +  var TOS_PP_REPO_ROOT = 'bower_components/tos-pp';
> > +
> > +  grunt.config('yeoman', {
> 
> Remy, why is this task called yeoman?  It seems to suggest some relation to
> the well known yeoman project templating package, but it's not obvious to me
> what that would be.  Unless I'm misunderstanding, I'd prefer to call it
> something more explicitly related to its function so that it's easier to
> understand why it's here.  project-config maybe?

I thought it was a convention, I took all this tasks from fxa-content-server
You can probably rename it if you want.
 
> ::: browser/components/loop/standalone/package.json
> @@ +9,5 @@
> >    "engines": {
> >      "node": "0.10.x",
> >      "npm":"1.3.x"
> >    },
> >    "dependencies": {
> 
> As discussed on IRC a while back, most of this stuff wants to be in
> devDependencies.  

It doesn't change anything because all of them are devDependencies. Also we don't release loop-client never. So basically devDependencies === dependencies for this specific project.

> Remy, you had also mentioned a mechanism that loop-server uses to keep these
> versions up-to-date, which I think we need to land at the same time as this
> file if we're really going to be specifying down to the minor version.

Yes, https://github.com/mozilla-services/loop-server/blob/master/package.json#L60

> @@ +21,5 @@
> > +    "time-grunt": "0.4.0",
> > +    "load-grunt-tasks": "0.6.0",
> > +    "i18n-abide": "0.0.22",
> > +    "consolidate": "0.10.0",
> > +    "handlebars": "1.3.0"
> 
> Why do we need handlebars?

At first we needed it to put the ToS inside the index.html but maybe we don't need it anymore.
Then you can probably remove consolidate as well.

Comment 63

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Rémy Hubscher (:natim) from comment #62)
>
> > What's the above comment related to?  Is it a leftover from something?
> 
> Yes a leftover.

Great; removed.

> > > +  require('load-grunt-tasks')(grunt, {scope: 'dependencies'});
> > 
> > Presumably, we'll want to change this to devDependencies assuming we edit
> > package.json that way as well.
> 
> I have no idea, if it works why not.

Done.
 
> > ::: browser/components/loop/standalone/bower.json
> > @@ +1,3 @@
> > > +{
> > > +  "name": "loop-client",
> > > +  "version": "0.0.0",
> > 
> > Could we live without specifying a version, given that we don't have an real
> > plans to publish and update this via bower?  More to the point, it's just
> > one more version number to update that we're likely to forget.
> 
> I have no idea, if it works why not.

Done.

> > ::: browser/components/loop/standalone/grunttasks/replace.js
> >
> > These need comments.  I'll also pull in pdehaan's changes from the github PR.
> > 
> > Remy, what's the high-level intent of the above replacements (and why do we
> > need to do them)?
> 
> This is to remove comments that are not handled during the conversion to
> HTML.
> The previous one is to remove tags also not handled.
> https://github.com/mozilla/legal-docs/blame/master/WebRTC_ToS/en-US.md#L4

Thanks.  Comments added, and pdehaan's changes from github hoisted into this patch.

> > These really want to be in separate dirs so that we don't serve up the SCSS,
> > but since this still will be going away in a few months, it's probably not
> > worth worrying about now.
> 
> Why could it be a problem to serve main.scss?

It's not really a problem, just feels out-of-place.

> > ::: browser/components/loop/standalone/grunttasks/yeoman.js
> > @@ +7,5 @@
> > > +
> > > +  var TEMPLATE_ROOT = 'content/legal';
> > > +  var TOS_PP_REPO_ROOT = 'bower_components/tos-pp';
> > > +
> > > +  grunt.config('yeoman', {
> > 
> > Remy, why is this task called yeoman?  It seems to suggest some relation to
> > the well known yeoman project templating package, but it's not obvious to me
> > what that would be.  Unless I'm misunderstanding, I'd prefer to call it
> > something more explicitly related to its function so that it's easier to
> > understand why it's here.  project-config maybe?
> 
> I thought it was a convention, I took all this tasks from fxa-content-server
> You can probably rename it if you want.

Talked to :zac, who didn't know, so I'm going to rename this to project_vars.
 
> > Remy, you had also mentioned a mechanism that loop-server uses to keep these
> > versions up-to-date, which I think we need to land at the same time as this
> > file if we're really going to be specifying down to the minor version.
> 
> Yes,
> https://github.com/mozilla-services/loop-server/blob/master/package.json#L60

Turns out that line is used only indirectly by travis (which may well be what we want to do!).  pdehaan has kindly filed bug 1065075, so we'll worry about that later. 
 
> > Why do we need handlebars?
> 
> At first we needed it to put the ToS inside the index.html but maybe we
> don't need it anymore.
> Then you can probably remove consolidate as well.

Good to know; thanks!

Comment 64

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8485951 [details] [diff] [review]
Generate Loop ToS static contentBug 1044411 - Generate Loop ToS static content

Review of attachment 8485951 [details] [diff] [review]:
-----------------------------------------------------------------

Right now, patch this overwrites source files, mixes mass-generated files with the source, and has no way to clean out the generated files.  This makes me quite uncomfortable.  That said given that this is supposed to be short-lived, and we need to land this, I'll live with spinning out separate bugs for adding a grunt-contrib-clean step, and, later, a real build step for the whole client.

Since we now know that this will be moving elsewhere in a few months, I'm going to assert that there are more important things to do than go over the CSS here with a fine-tooth comb, and put rs=dmose on that.

Overall, r+ with the changes mentioned here addressed, and new bugs filed as requested.

::: browser/components/loop/standalone/grunttasks/marked.js
@@ +9,5 @@
> +  'use strict';
> +
> +  // convert localized TOS/PP agreements from markdown to html partials.
> +
> +  function rename(destPath, destFile) {

According to <http://gruntjs.com/configuring-tasks>, the second argument here is actually the srcFile, not the destFile, so I'll change the name here.

::: browser/components/loop/standalone/grunttasks/replace.js
@@ +10,5 @@
> +      src: [
> +        '<%= yeoman.pp_md_src %>/*.md',
> +        '<%= yeoman.tos_md_src %>/*.md'
> +      ],
> +      overwrite: true,

Overwritting stuff in the src directory seems fragile.  If I'm understanding correctly, the way this all gets tied together is that "make install" runs "npm install" which runs "bower install", and then the grunt stuff effectively rebuilds everything without checking dependencies, so that is what should ensure consistency.  However, there's no way to get a tree back to "clean" state, as far as I can see.

Remy, is that correct?

Comment 65

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Generate Loop ToS static content,

Bug 1044411 - Generate Loop ToS static content

Comment 66

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8487505 [details] [diff] [review]
Generate Loop ToS static content,

I've done a bunch of testing, and things seems to look reasonable.  Remy, if you can answer my latest question and file the two spin-off bugs I've requested, I'll mark a final r+ and land this patch in the morning.  

I may need to get separate signoff on the .hgignore changes; I'm looking into that now.

Comment 67

[Rémy Hubscher (:natim)]

`bower install` will get you the source files.
`grunt replace` will run the replace grunt task that will override source files
`grunt marked` will convert the TOS.md files into TOS.html files
`grunt sass` will convert main.scss files into main.css file.


If I understand correctly, you want to file two new bugs:

 - One to provide a grunt-contrib-clean command that will remove all *.html generated by `grunt marked`?
 - One to provide a real build step for the client, is `make tos` not enough for that?

Comment 68

[Dan Mosedale (:dmosedale, :dmose)]

So grunt-contrib-clean exists in npm.  We should just add a task that drives that to remove all generated stuff, both HTML, and CSS.

Don't worry about the second bug; it'd take me more time to describe what I'm thinking of here than just file it myself.

Comment 69

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Generate Loop ToS static content,

Bug 1044411 - Generate Loop ToS static content

Comment 70

[Dan Mosedale (:dmosedale, :dmose)]

Added a few comments and README clauses after discussing with Standard8, and removed an unnecessary .hgignore line that he noticed in this latest patch.

Comment 71

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Generate Loop ToS static content, rs=ted for .hgignore changes

Bug 1044411 - Generate Loop ToS static content

Comment 72

[Dan Mosedale (:dmosedale, :dmose)]

The most recent patch added comments to the .hgignore after discussion with ted, and also the gitignore as well, so that we know when things can be cleaned up.

However, I see now that the privacy policy URL introduced by this patch is broken, as it was on the original github PR.  Looking through this bug and talking to shell seems to suggest that instead of fixing the privacy policy stuff, we should just remove it before landing, but she's sent mail to legal to verify.

Remy, why is the privacy policy stuff in this patch?

Comment 73

[Rémy Hubscher (:natim)]

We don't have Privacy Policy for loop yet, but this was in prevision of it being added. See https://github.com/mozilla/legal-docs/tree/master/WebRTC_PrivacyNotice

I agree that you can just remove it from this patch and that we will make a new patch if needed.

Comment 74

[Rémy Hubscher (:natim)]

I have created Bug 1066491 to handle the grunt contrib-clean command.

Comment 75

[Dan Mosedale (:dmosedale, :dmose)]

I've filed bug 1068059 to track a build/minification step for the whole standalone client (rather than just the legal bits).

Comment 76

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Generate Loop ToS static content, rs=ted for .hgignore changes

Bug 1044411 - Generate Loop ToS static content, priv-policy bits removed

Comment 77

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/b83b13e93c23

Comment 78

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8490113 [details] [diff] [review]
Generate Loop ToS static content,  rs=ted for .hgignore changes

r=abr,dmose; rs=ted for .hgignore changes

Comment 79

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b83b13e93c23

Comment 80

[Mark Banner (:standard8)]

I landed a follow-up to fix the .gitignore file change:

https://hg.mozilla.org/integration/fx-team/rev/c7f0d5c38afa

It appears git didn't like/match the ! part way through the filename (despite some of the docs implying this), moving it to the start of the filename fixed it.

This was discovered at it stopped loop-client extraction from working.

Comment 81

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c7f0d5c38afa

Comment 82

[u279076]

James, can you please verify this in the latest Nightly? We'll skip verification in the Fig branch.

Comment 83

[Richard Pappalardo [:rpapa][:rpappalardo]]

Attachment: Loop ToS link - screen shot

The link is now there in Nightly - pointing to:
https://call.mozilla.com/legal/terms/

Comment 84

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8490113 [details] [diff] [review]
Generate Loop ToS static content,  rs=ted for .hgignore changes

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 85

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6733cf96370
https://hg.mozilla.org/releases/mozilla-aurora/rev/076a776862b4

Comment 86

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8490113 [details] [diff] [review]
Generate Loop ToS static content,  rs=ted for .hgignore changes

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.

Comment 87

[u279076]

I verify that I can view the Terms of Service in today's Aurora build. Marking this verified fixed.

Comment 88

[u279076]

I just realized that our Privacy Policy is translated but the Terms of Use document is not. I think it's unrealistic to expect our non-English users to read and understand our Terms of Use as it stands. I'm not sure if translation of the page is tracked somewhere else but I think it should be a blocker for 35. I'm not sure where the right place to escalate this is so I'm raising it here.

Comment 89

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #88)
> I just realized that our Privacy Policy is translated but the Terms of Use
> document is not. I think it's unrealistic to expect our non-English users to
> read and understand our Terms of Use as it stands. I'm not sure if
> translation of the page is tracked somewhere else but I think it should be a
> blocker for 35. I'm not sure where the right place to escalate this is so
> I'm raising it here.

Just to be clear, I'm referring to translating the following page:
https://hello.firefox.com/legal/terms/

Comment 90

[Maire Reavy [:mreavy]]

Arcadio, Mika  -- Per Anthony's comments above (in Comment 88 and Comment 89), we need to get the ToS page for Hello translated, and we need to decide if/when we're moving it (like we talked about earlier in this bug.)

Shell -- I'm needinfo'ing you so this is on your radar.

Thanks.  (And Thanks for flagging this, Anthony!)

Comment 91

[Mika]

Hi Maire - the Hello ToS & PN docs were created on the legal & privacy hubs (just yesterday!).  

Three things need to be done:
(1) switch the link in the product so that the "Terms of Service" link redirects a user to: https://www.mozilla.org/about/legal/terms/firefox-hello/
(2) decommission this legal page: https://hello.firefox.com/legal/terms/ 
(3) do the same for the "Privacy Notice" link so that it redirects a user to: https://www.mozilla.org/privacy/firefox-hello/ 

For Fx35 we will have all translations completed before it goes to release.  We work with a vendor to do those.  The translations are stored in our legal github repo: https://github.com/mozilla/legal-docs 

We make updates and translations in the legal repo, and the changes get pulled directly onto the Legal & Privacy pages by the webdev team.  So your team does not need to do anything.

Let me know if we should do a quick 1:1 meeting to go over this, or if you have any questions.  Please also confirm when the links have been changed in the product.

Comment 92

[Mark Banner (:standard8)]

(In reply to Mika from comment #91)
> Three things need to be done:
> (1) switch the link in the product so that the "Terms of Service" link
> redirects a user to: https://www.mozilla.org/about/legal/terms/firefox-hello/
> (2) decommission this legal page: https://hello.firefox.com/legal/terms/ 
> (3) do the same for the "Privacy Notice" link so that it redirects a user
> to: https://www.mozilla.org/privacy/firefox-hello/ 

Thanks for this update.

- Bug 1105488 will update the desktop client and the local standalone server instances that are used for dev
- Bug 1105496 and Bug 1105497 will update the standalone staging and production configs respectively.