Closed Bug 1034841 Opened 5 years ago Closed 5 years ago

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

Categories

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

All
Linux
defect

Tracking

(firefox34 verified)

VERIFIED FIXED
mozilla34
Tracking Status
firefox34 --- verified

People

(Reporter: unghost, Assigned: aryx)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached image 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.
This is fixed by 1033841, which has not landed yet.
Assignee: nobody → aoprea
(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.
Priority: -- → P1
Target Milestone: --- → 33 Sprint 3- 7/21
Target Milestone: 33 Sprint 3- 7/21 → 34 Sprint 1- 8/4
Duplicate of this bug: 1046034
Updating the title for what this is really about.
Summary: Click on ToS notice in Loop popup window displays ToS notice in Loop popup window instead of browser tab → 'Privacy notice' has the wrong url and misses the link target, also improve l10n situation
Attached patch patch, v1 (obsolete) — Splinter Review
Assignee: aoprea → archaeopteryx
Status: NEW → ASSIGNED
Attachment #8465004 - Flags: review?(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?
Attachment #8465004 - Flags: review?(standard8) → feedback+
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.
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!
Flags: needinfo?(archaeopteryx)
(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.
Flags: needinfo?(archaeopteryx)
Attached patch patch, v3 (obsolete) — Splinter Review
Attachment #8466782 - Flags: review?(standard8)
Comment on attachment 8466782 [details] [diff] [review]
patch, v3

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

Looks great, thanks.
Attachment #8466782 - Flags: review?(standard8) → review+
Attachment #8465004 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/c160e9aa0639
Keywords: checkin-needed
Target Milestone: 34 Sprint 1- 8/4 → mozilla33
This contains a string change, so aurora would need a version without string change. Or do you mean target milestone mozilla34?
(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 ;-)
Target Milestone: mozilla33 → mozilla34
https://hg.mozilla.org/mozilla-central/rev/c160e9aa0639
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
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
(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
Verified fixed 34.0a1 (2014-08-06) based on comment 20.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.