1034841 - 'Privacy notice' has the wrong url and misses the link target, also improve l10n situation

Status: VERIFIED FIXED

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

Description

[Alexander L. Slovesnik]

Attachment: Screenshot

STR:
1) Click on Loop button
2) Click on ToS link

Expected result:
ToS text opens in browser tab

Actual result:
ToS text opens in Loop popup window. Subsesquent clicks on Loop button shows ToS text. You can not get back to call interface.

Comment 1

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

This is fixed by 1033841, which has not landed yet.

Comment 2

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) from comment #1)
> This is fixed by 1033841, which has not landed yet.

The second link still needs fixing. The second link is also missing the http://

Additionally, from bug 1033841 comment 25:

> Also, I'd really wish we had less HTML in those strings and use Javascript
> to manage replacements.

We should consider this as well, to reduce the likelihood of bad copy/paste of html.

Comment 4

[Mark Banner (:standard8)]

Updating the title for what this is really about.

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v1

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8465004 [details] [diff] [review]
patch, v1

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

Thanks for doing this. It looks good, but I'd just like a re-review on the localisation note update before giving r+

::: browser/components/loop/content/js/panel.jsx
@@ +111,5 @@
> +      if (this.state.seenToS == "unseen") {
> +        var terms_of_use_url = "https://accounts.firefox.com/legal/terms";
> +        var privacy_notice_url = "https://www.mozilla.org/privacy/";
> +        var linkTemplate = "<a target=\"_blank\" href=\"{{url}}\">{{label}}</a>";
> +        var linkTos = linkTemplate.replace("{{url}}", terms_of_use_url);

nit: blank line before this line please.

@@ +113,5 @@
> +        var privacy_notice_url = "https://www.mozilla.org/privacy/";
> +        var linkTemplate = "<a target=\"_blank\" href=\"{{url}}\">{{label}}</a>";
> +        var linkTos = linkTemplate.replace("{{url}}", terms_of_use_url);
> +        linkTos = linkTos.replace("{{label}}", __("legal_text_tos"));
> +        var linkPrivacy = linkTemplate.replace("{{url}}", privacy_notice_url);

nit: blank line before this line please.

@@ +115,5 @@
> +        var linkTos = linkTemplate.replace("{{url}}", terms_of_use_url);
> +        linkTos = linkTos.replace("{{label}}", __("legal_text_tos"));
> +        var linkPrivacy = linkTemplate.replace("{{url}}", privacy_notice_url);
> +        linkPrivacy = linkPrivacy.replace("{{label}}", __("legal_text_privacy"));
> +        var tosHTML = __("legal_text_and_links2", {

nit: blank line before this line please.

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +33,5 @@
>  cannot_start_call_session_not_ready=Can't start call, session is not ready.
>  network_disconnected=The network connection terminated abruptly.
>  
>  connection_error_see_console_notification=Call failed; see console for details.
>  ## LOCALIZATION NOTE (legal_text_and_links): In this item, don't translate the

nit: need to update the string name.

Also, would it be worth being explicit about the fact that terms of use & privacy notice are wrapped as links with the titles as specified by legal_text_tos and legal_text_privacy?

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

As React does a very good job as being a template engine, we could probably try doing something like this:

var tosHTML = __("legal_text_and_links2", {
  "terms_of_use": React.renderToString(
    <a href="http://…">{__("legal_text_tos")}</a>
  ),
  "privacy_notice": React.renderToString(
    <a href="http://…">{__("legal_text_privacy")}</a>
  ),
});

We could even fetch these two urls from prefs, using mozLoop.getLoopCharPref.

Comment 8

[Maire Reavy [:mreavy]]

Thanks for taking on this bug!  Do you know when you'll have an updated patch?  I'm eager to get a fix landed.  Thanks for your help!

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #8)
> Thanks for taking on this bug!  Do you know when you'll have an updated
> patch?  I'm eager to get a fix landed.  Thanks for your help!
This weekend.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v3

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Successful Try push: https://tbpl.mozilla.org/?tree=Try&rev=62e629a42fa6

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8466782 [details] [diff] [review]
patch, v3

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

Looks great, thanks.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v4, r=Standard8

Comment 14

[Mark Banner (:standard8)]

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

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This contains a string change, so aurora would need a version without string change. Or do you mean target milestone mozilla34?

Comment 16

[Mark Banner (:standard8)]

(In reply to Archaeopteryx [:aryx] from comment #15)
> This contains a string change, so aurora would need a version without string
> change. Or do you mean target milestone mozilla34?

I was away for the last set of merges, I'm obviously out of date still ;-)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Paul Silaghi, QA [:pauly]]

34.0a1 (2014-08-06), FR build
Couple of issues:
1. the 'tos and privacy' text is in english
2. the tos page is in french, while the privacy page is in english
3. how can a normal user to read both the tos and the privacy? clicking one of them makes the popup disappear

Comment 19

[Francesco Lodolo [:flod]]

1. use Italian if you want to test localization on trunk, French is not updated as fast as it used to be.
2. https://www.mozilla.org/privacy/ is not localized, so it's expected for now

Comment 20

[Mark Banner (:standard8)]

(In reply to Paul Silaghi, QA [:pauly] from comment #18)
> 1. the 'tos and privacy' text is in english

See comment 19.

> 2. the tos page is in french, while the privacy page is in english

The links are likely changing. It'll be up to product folks if they want these localising or not.

> 3. how can a normal user to read both the tos and the privacy? clicking one
> of them makes the popup disappear

Bug 1046039

Comment 21

[Paul Silaghi, QA [:pauly]]

Verified fixed 34.0a1 (2014-08-06) based on comment 20.