Closed Bug 1142532 Opened 10 years ago Closed 10 years ago

Update the invitation email that is sent if context is included in conversations

Categories

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

Product:
Hello (Loop)
Component:
Client
Type:
defect
Points:
3

Tracking

(firefox40 verified, firefox41 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla41
Iteration:
41.1 - May 25
Tracking Flags:
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: standard8, Assigned: mikedeboer)

References

Details

(Whiteboard: [context])

User Story

As a desktop client user, I want the invitation e-mail I send to an invitee to include any attached context about the conversation so that the invitee is better prepared and more engaged when they join the room.

Acceptance criteria:
* When URL context is added to the conversation, the e-mail subject should be changed to: 

Firefox Hello conversation: [Comment or Title of Attached Page]

* When URL context is added to the conversation, the e-mail body should be changed to: 

Hello!

Join me for a video conversation on Firefox Hello about:
[Comment or Title of Attached Page]

It’s the easiest way to connect by video with anyone anywhere. With Hello, you don't have to download or install anything. Just click or paste this link into your Firefox, Opera or Chrome browser to join the conversation:

https://hello.firefox.com/ufujrgQFEQM

If you’d like to learn more about Hello and how you can start your own free video conversations, visit https://www.firefox.com/hello/

Talk to you soon!

Attachments

(2 files, 2 obsolete files)

Patch 1.2: add invitation email subject and body text used when a context is added
(deleted), patch
standard8
: review+
flod
: feedback+
Details | Diff | Splinter Review
Patch 2: compose a different email for Loop room invites
(deleted), patch
standard8
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Implementation bug for the email invitation for conversations.
Rank: 19
Flags: qe-verify+
Flags: firefox-backlog+
Priority: -- → P1
Rank: 19 → 7
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 39.3 - 30 Mar
Points: --- → 3
Comment on attachment 8584510 [details] [diff] [review] Patch 1: add invitation email subject and body text used when a context is added >diff --git a/browser/locales/en-US/chrome/browser/loop/loop.properties b/browser/locales/en-US/chrome/browser/loop/loop.properties >+## LOCALIZATION NOTE (share_email_body_context): In this item, don't translate >+## the part between {{..}} and leave the \n\n part alone. >+share_email_body_context=Hello!\n\nJoin me for a video conversation on {{clientShortname2}} about:\n{{title}}.\n\nIt's the easiest way to connect by video with anyone anywhere. With {{clientSuperShortname}}, you don't have to download or install anything. Just click or paste this link into your {{brandShortname}}, Opera, or Chrome browser to join the conversation:\n\n{{callUrl}}\n\nIf you'd like to learn more about {{clientSuperShortname}} and how you can start your own free video conversations, visit {{learnMoreUrl}}\n\nTalk to you soon! This seems like the kind of string that might be better split it (and then you can remove the \n\n from inside the string and put them in the code). E.g.: share_email_body_context_line1=Hello! share_email_body_context_line2=Join me for a video conversation on {{clientShortname2}} about:\n{{title}}. etc.
You mean something like this?
Attachment #8584510 - Attachment is obsolete: true
Attachment #8584510 - Flags: review?(gavin.sharp)
Attachment #8584633 - Flags: review?(gavin.sharp)
Comment on attachment 8584633 [details] [diff] [review] Patch 1.1: add invitation email subject and body text used when a context is added I guess I didn't notice that share_email_body5 already exists and does something like this. Did that get feedback from the l10n team? It seems to me that localizers might want to control the newline formatting in addition to the actual text, and so that's why I suggested not having notes that include "leave the \n\n part alone" and instead just let them specify arbitrary lines (one of which includes the URL). But maybe that is too much flexibility and leaves too much room for divergence from the spec, I'm not sure. Maybe flod can help.
Attachment #8584633 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8584633 [details] [diff] [review] Patch 1.1: add invitation email subject and body text used when a context is added Review of attachment 8584633 [details] [diff] [review]: ----------------------------------------------------------------- While I personally don't love the current string, it's also the form that gives localizers most flexibility. Splitting the string into pieces assumes that the localization will have the same structure and number of sentences of English, while it might not be the case. For example we've received some complaints for website localization when we tried to split sentences like this. One of the many variations of this string was discusses in bug 1113090 comment 38 and following. I see we have now switched away from \r\n to \n or \n\n, so I assume this also makes bug 1126171 invalid.
Attachment #8584633 - Flags: feedback?(francesco.lodolo) → feedback-
(In reply to Francesco Lodolo [:flod] (UTC+1) from comment #5) > Splitting the string into pieces assumes that the localization will have the > same structure and number of sentences of English, while it might not be the > case. I was assuming we would allow N lines, and just ignore any that are empty, with localization notes explaining how that works. That way localizers can add more/fewer lines than are necessary in en-US as needed. But I do not feel strongly here, if we should just copy share_email_body5 then let's do that.
Attachment #8584633 - Flags: review?(gavin.sharp) → review-
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #6) > I was assuming we would allow N lines, and just ignore any that are empty, > with localization notes explaining how that works. That way localizers can > add more/fewer lines than are necessary in en-US as needed. I assume this needs to be in Firefox 39, so land before merge day? If that's the case, let's use the single string approach and I'll be happy to investigate in dev-l10n if I'm just over-thinking it. Some localization tools don't deal well with empty strings (I'm sure about an empty source, not much about the translation), this makes me a bit reluctant.
Attachment #8584633 - Attachment is obsolete: true
Comment on attachment 8585159 [details] [diff] [review] Patch 1.2: add invitation email subject and body text used when a context is added Review of attachment 8585159 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, as I said I believe this is the safest way to localize this mail at the moment (but I'll investigate if we can change it in the future with less pressure from deadlines).
Attachment #8585159 - Flags: feedback+
Comment on attachment 8585159 [details] [diff] [review] Patch 1.2: add invitation email subject and body text used when a context is added Punting to Mark, as we'd like to land this now.
Attachment #8585159 - Flags: review?(gavin.sharp) → review?(standard8)
Attachment #8585159 - Flags: review?(standard8) → review+
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Iteration: 40.2 - 27 Apr → 41.1 - May 25
Attachment #8604620 - Flags: review?(standard8)
Comment on attachment 8604620 [details] [diff] [review] Patch 2: compose a different email for Loop room invites Review of attachment 8604620 [details] [diff] [review]: ----------------------------------------------------------------- Ok, this looks good. There are a couple of eslint failures reported: content/shared/js/actions.js 364:21 error Unexpected trailing comma comma-dangle test/desktop-local/roomStore_test.js 411:8 error Missing semicolon semi Please make sure those are fixed before landing - and test against the latest fx-team as we just landed a patch to enable more rules.
Attachment #8604620 - Flags: review?(standard8) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/948fc6e57257
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
QA Contact: bogdan.maris
Comment on attachment 8604620 [details] [diff] [review] Patch 2: compose a different email for Loop room invites Approval Request Comment [Feature/regressing bug #]: Context in conversations feature [User impact if declined]: The user won't send a more appropriate email when she or he shares a room with context data attached to it. [Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass. [Risks and why]: minor. [String/UUID change made/needed]: n/a.
Attachment #8604620 - Flags: approval-mozilla-aurora?
Attachment #8604620 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Compared the invitation email from an old build with email from latest Aurora & Nightly across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) and can confirm that the version from user story is the same with one found on latest builds. Only issue I saw is a punctuation issue: 'Opera, or Chrome' instead of 'Opera or Chrome', does this require a new bug? Marking this verified fixed based on my testing.
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
status-firefox41: fixed → verified
Flags: needinfo?(mdeboer)
Bogdan, thanks for noticing that punctuation error! The commit in comment 20 fixes this.
Flags: needinfo?(mdeboer)
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: