Closed Bug 847168 Opened 8 years ago Closed 7 years ago

[User Story] Email Signature


(Firefox OS Graveyard :: Gaia::E-Mail, defect, P1)

Gonk (Firefox OS)


(feature-b2g:2.1, tracking-b2g:backlog, b2g18+)

2.1 S2 (15aug)
feature-b2g 2.1
tracking-b2g backlog
Tracking Status
b2g18 + ---


(Reporter: sync-1, Assigned: awissmann)



(Keywords: feature, Whiteboard: [ucid:Productivity21, 1.5:p3, ft:productivity][2.1-feature-qa+])

User Story

As a user I want to be able to create a signature and include it in every email


(6 files)

Firefox v1.0.1,  build identifier: 20130228114642
 In v1.0, when forwarding email , there is signature. But seems in v1.0.1, the signature is removed.
 As it is an important feature, could we add it back?
 +++ This bug was initially created as a clone of Bug #410950 +++
 There is no email signature when compose a mail
 1.Create email account;
 2.Compose a mail,there is no email signature-->ko
 PS:If forward or replay a mail,then there will be email signature,"Sent from my Firefox OS device"
 There should be email signature
 For FT PR, Please list reference mobile's behavior:
 ++++++++++ end of initial bug #410950 description ++++++++++
 CONTACT INFO (Name,Phone number):
 For FT PR, Please list reference mobile's behavior:
partner requested feature. tracking-b2g18+
tracking-b2g18: --- → +
Assignee: nobody → ffos-product
Keywords: feature
Flags: needinfo?(ffos-product)
Whiteboard: triaged
blocking-b2g: --- → tef?
Andrew, correct me if I'm wrong, but I believe this was removed since there was no UI to control the inclusion of the signature, so rather than forcing such a signature, it was deemed out of scope.
Flags: needinfo?(ffos-product) → needinfo?(bugmail)
clear tef? as this is not required for launch
blocking-b2g: tef? → ---
Right, this was out-of-scoped.
Flags: needinfo?(bugmail)
This is specifically asked for by our partner.  Can we add this back in even without the ability to modify the signature for v1.0.1?  

Then file a separate bug to allow users to customize it.
blocking-b2g: --- → tef?
(In reply to Chris Lee [:clee] from comment #5)
> This is specifically asked for by our partner.  Can we add this back in even
> without the ability to modify the signature for v1.0.1?  
> Then file a separate bug to allow users to customize it.

Can you clarify what exactly you are requesting?  It sounds like for v1.0.1 you want us to put back in a hard-coded signature (although it would be partner configured), and in v1.2+ we would make the signature editable.  If that's the case, I feel like cjones' comments in bug 837269 where we removed the hard-coded signature was quite relevant about us not forcing signatures on users, especially ones they can't change, being inappropriate.
It's too late to be making enhancements or feature changes at this point.
blocking-b2g: tef? → -
Depends on: 875803
the issue has been reported again on FF0S1.1 during the Latam certification
Built ID 20130823171628
Device: Ikura
QC RIL version: ""
gaia commit: 4916f82 Merge branch 'zte/ics_strawberry_cdr' of ssh:// into ics_strawberry_cdr
gecko commit: a1c2bb0 ZRL modify ACCEPT Http head for MMS
User Story:

As a user, I want to be able to create an email signature with a default set to “Sent using Firefox OS” such that I can save time by not typing my signature for every email and I can show off my use of Firefox OS.
Summary: [Buri][Email]There is no email signature when compose a mail → [User Story] Email Signature
Adding to 1.4 backlog - will need UX review.
Flags: needinfo?(rmacdonald)
There is an existing UI for this which I will revisit as UX works through the 1.4 backlog.
Flags: needinfo?(rmacdonald)
Whiteboard: triaged → [ucid:Productivity21, 1.4:p2, ft:productivity]
Whiteboard: [ucid:Productivity21, 1.4:p2, ft:productivity] → [ucid:Productivity21, 1.4:p3, ft:productivity]
blocking-b2g: - → backlog
Whiteboard: [ucid:Productivity21, 1.4:p3, ft:productivity] → [ucid:Productivity21, 1.5:p3, ft:productivity]
Duplicate of this bug: 970021
Duplicate of this bug: 977622
Spec attached.
feature-b2g: --- → 2.1
User Story: (updated)
Duplicate of this bug: 1025479
Assignee: ffos-product → awissmann
Attached file backend
Attachment #8449698 - Flags: review?(m)
Attachment #8449698 - Flags: review?(bugmail)
Attached file frontend
Attachment #8449699 - Flags: review?(m)
Attachment #8449699 - Flags: review?(jrburke)
Comment on attachment 8449698 [details] [review]

Marking feedback+ for now, so as to be clear that I've only taken a cursory look so far based upon the code and the times you've showed me what you've had working. Andrew and James have likely thought about how they'd implement this feature and may have other architectural feedback as well (i.e. should Gaia or GELAM be responsible for appending signatures, etc) or maybe historical gotchas we need to watch out for. Also, cooks in the kitchen and all that.
Attachment #8449698 - Flags: review?(m) → feedback+
Attachment #8449699 - Flags: review?(m) → feedback+
Comment on attachment 8449698 [details] [review]

Apologies about the confusion on this... the back-end already supports using signatures.  We had a default one but we took it out because we lacked the UI to let them change it.  The good news is this doesn't constitute too huge a change, since most of the work is UI-related.

Signatures are stored on a per-identity basis, in keeping with Thunderbird.  The concept of an identity is that it bundles display name, email address, reply to address, and signature as a single thing.  (In Thunderbird it also involves a bunch of other preferences like the SMTP server to use and such.)  This is particularly useful for people who use multiple aliases/etc. to funnel to a single inbox and then want to be able to act as all of those various aliases.  Note that this was speculative, as we still do not have any UI to support having more than one identity per account.

You can see us initializing IMAP signatures to "no signature" here:

See for the commit that removed our default signature and the impact on our unit tests.

I think we'd want a modifyIdentity method on MailSenderIdentity in mailapi.js.  Previously signature being truthy meant there was a signature and we should use it.  Falsey meant there was no signature and not to use it.  That logic is here:

As per the spec and your current implementation, we do need to make enabling/disabling a separate thing.  (Well, we could just always reset the signature to the default if the user ever toggles the signature off and then on again, but that seems mean and unintuitive.)  Your current signatureEnabled is a good name.  I would propose that we basically do something like:
  if (identity.signatureEnabled && identity.signature)
in our check.

I also propose that our upgrade just set signatureEnabled to false and an empty signature string by doing:
  signatureEnabled: oldIdentity.signatureEnabled || false,
  signature: oldIdentity.signature || '',

(The UI would still need/want a coercion to the empty string as well since I don't think we're currently going to force any actual upgrade.)

Part of the rationale for this is that for l10n, branding reasons, and our goal of keeping the library reusable by other apps, we can't hardcode such a default signature.  Our options are to:
a) Have tryToCreateAccount take a signature
b) Have the hack MailAPI.useLocalizedStrings pass a default signature into the back-end.
c) Have all accounts default to having no signature, and just having the UI update the the account's default identity to have a signature when the UI step gets to that.

I propose that we do option 'c'.  This reduces the test permutation space since we don't need to worry about the difference between an identity that was initialized with a signature and one that was not.  They all start the same, so it's just the no signature -> have signature, and have signature -> have signature, and have signature -> no signature transitions really.  This also avoids adding more l10n cruft to the back-end.  It also is arguably a UX win in that if the user creates an account and then the email app quits before they complete the configuration process, we will still default to not having a signature so we won't get complaints from them not confirming the signature.

We can also address the upgrade situation by treating a falsey signature as "let's initialize the signature the user sees to the default again".

Test-wise, I'm not sure what feels least icky, but we basically want to get the code coverage for the signature logic back.  Using a variation on that code in test_compose for when the signature is enabled is probably the way to go.  It might make sense to parametrize the test to optionally take a signature and then have that alter the expectations for the text.

So it'd be:
- Account is created as it is currently, no signature.
- Verify no signature by default and disabled.
- Send the messages and verify all the composer states.
- Change the account to have (enabled) signature A.
- Send messages, verify states.
- Change the signature to (enabled) signature B.
- Send messages verify states.
- Disable the signature.
- Send messages, verify states.
- Enable signature B.
- Send messages, verify states.
- Clear the signature
- Send messages, verify states.

Actually, that seems like it might be a fairly extensive amount of coverage, so I'm not sure we want the actual sending process overhead or not.  Just using a canned message and verifying the composer state may be sufficient.  I'm really open to whichever seems least horrible to you.  The key thing is parametrization over copy-and-paste.

NB: In general we probably want a factory to help create/normalize identity structures.
Attachment #8449698 - Flags: review?(bugmail) → review-
Comment on attachment 8449699 [details] [review]

Since the backend stuff needs to be changed, I'm clearing the review? flag against :jrburke and changing it to a feedback.  As noted on the other attachment, I think the UI logic is generally separate from the back-end stuff, but it would be premature to call it a review since there can be spillover from the back-end changes.

In terms of doing the compose manipulation in the UI, note that in general we want all reusable domain specific logic in the back-end since it's intended to be a library that multiple email apps can use.  The front-end is ideally limited to a UI wrapper around that and opinionated decisions about how to use the library.

Note that we haven't always met this standard; various hacks like model.js were temporary kludges we haven't gotten rid of yet.  (And those hacks have bitten us, so we won't be doing that again.)  And iframe-shims.js lives in a weird in-between world.  (MailBody is better, since it hides DOM manipulation in helpers the front-end can use.)
Attachment #8449699 - Flags: review?(jrburke)
Attachment #8449699 - Flags: review-
Attachment #8449699 - Flags: feedback?(jrburke)
No longer depends on: 875803
Duplicate of this bug: 875803
> In terms of doing the compose manipulation in the UI, note that in general
> we want all reusable domain specific logic in the back-end since it's
> intended to be a library that multiple email apps can use.  The front-end is
> ideally limited to a UI wrapper around that and opinionated decisions about
> how to use the library.

I'm a bit stuck here. I have now moved signatures over to identities, but the backend does not currently support any sort of "current" identity, so I have no way to know which signature to append. The frontend currently just does account.identities[0], since we know that our UI only creates on for each account. However, doing this hack on the backend would violate having gelam as a separate library. One option here is to either introduce the concept of a "current" identity, and have it set to the first/only one by default/have the frontend set it to the first/only one.

A second issue how to insert the signature on the backend that makes it work with the UX spec. In some way the signature should be inserted manually on the frontend, since that allows the user to see/edit the signature in each email. The solution here is probably exposing some sort of "default body" field on the account object that any given UI can use. However, this means that the backend can't actually test whether the signature gets inserted, only the content of this field. Not sure what the solution is here.
Let's just assume there's only one identity per account for now and subscript it with [0] as is currently being done.  This may potentially work in the future too if we assume that we will always shift the default identity to be the 0th/first identity for an account.  It is okay for us to cut corners in GELAM.

Let's spin off the "showing the signature in gray" stuff to a follow-up bug and just stick with how the back-end currently works for this.  Specifically, that enhancement is probably best deferred until we have/support HTML composition since at that point we can just set a class on the div/whatever and get it for free.  Roundtripping the color through draft saving and/or re-inferring the color introduces complexity/bug risk that isn't justified at this point.  Juwei, is that okay with you?
Flags: needinfo?(jhuang)
Er, but clarifying, it should ideally still be the front-end doing the 0-subscripting and then explicitly telling that to the back-end.  Having the back-end having some type of inferred current anything is undesirable since that way lies bugs/etc.
Sure! It works for me, thanks!
Flags: needinfo?(jhuang)
Attachment #8449698 - Flags: review- → review?(bugmail)
Comment on attachment 8449698 [details] [review]

Looking good!  I think we'll have this done in the next round of fixes!  Note that I've also commented on the gaia pull request.  I'm not at 100% thoroughness on the gaia one because I think there's a more significant set of changes that will happen there, so it's possible we'll need another round or so on that one after my requests are addressed.
Attachment #8449698 - Flags: review?(bugmail) → review-
Comment on attachment 8449699 [details] [review]

Clearing the feedback request against James for now since currently I think I should do the next review pass, although depending on when that happens I may split non-backendy duties with James.
Attachment #8449699 - Flags: feedback?(jrburke)
Attachment #8449698 - Flags: review- → review?(bugmail)
Attachment #8449699 - Flags: review- → review?(bugmail)
Comment on attachment 8449698 [details] [review]

r=asuth conditional on the few nits and the one minor addition.  Feel free to rebase squash your GELAM commits as you desire.
Attachment #8449698 - Flags: review?(bugmail) → review+
Comment on attachment 8449699 [details] [review]

f=asuth contingent on removing the evt.on('identityModified'/'accountModified') stuff per my comment on the github pull request.

At this point I'm also handing final review of the UI stuff off to James.  I think I'm good with those changes in general, but I don't have the time to give this the human testing required on the device.  Also there may be some other cleanup benefits possible from tryToCreateAccount actually returning a real MailAccount now.

I'm also going to pass check-in responsibility off to James for this bug too.
Attachment #8449699 - Flags: review?(jrburke)
Attachment #8449699 - Flags: review?(bugmail)
Attachment #8449699 - Flags: feedback+
Comment on attachment 8449699 [details] [review]

Looking good! I have a some review feedback, left in the pull request, but it is really about just consolidating what you have already, and some nits. Flip review back to me when addressed, and I will do check of it, then we can prep info for ui-review. Really neat to see it working on the device!
Attachment #8449699 - Flags: review?(jrburke)
QA Whiteboard: [COM=Productivity]
QA Whiteboard: [COM=Productivity] → [COM=Gaia::E-Mail]
Attachment #8449699 - Flags: review?(jrburke)
Comment on attachment 8449699 [details] [review]

r+ if one final nit is addressed:

and it looks like the latest gaia-try run on the pull request indicated one more lint error in apps/email/js/cards/account_prefs_mixins.js, a line too long error.

It is definitely good enough to start the UX feedback cycle, by creating a mosaic of screenshots that are involved with these changes. Attach that to the ticket then set ui-review? for Juwei and :peko to get their OK.

You can also attach a zip of your branch, so they can install it on their device via the app-manager/webide, a way to do that is documented here:
Attachment #8449699 - Flags: review?(jrburke) → review+
Attached file UX Review -- ZIP
Attachment #8469743 - Flags: ui-review?(pchen)
Attachment #8469743 - Flags: ui-review?(jhuang)
Comment on attachment 8469743 [details]
UX Review -- Screenshots

I install the zip file and it looks great!!!!Thank you Alex!
Attachment #8469743 - Flags: ui-review?(jhuang) → ui-review+
Comment on attachment 8469743 [details]
UX Review -- Screenshots

Looks nice!!!!Thanks Alex!
Attachment #8469743 - Flags: ui-review?(pchen) → ui-review+
Is "FirefoxOS" correct? I don't think I've ever seen it written like that, for sure it's not the official branding ("Firefox OS").
Blocks: 1052469
:flod, thank you for catching this! I filed bug 1052469 to get that fixed, we should be able to land it quickly.
Blocks: 1052951
Spec updated due to bug 1052951
Target Milestone: --- → 2.1 S2 (15aug)
Depends on: 1057277
Depends on: 1057280

Gaia      c8e93dadeaec6a2617b4db8b7d65ba14aa8db378
BuildID   20140821160203
Version   34.0a1 May 20 09:29:20 CST 2014

[Functional testing result]
4 out 4 test cases passed for the 2014-08-21 Flame.

[UX bugs]
Bug 1057277, Bug 1057280
QA Contact: nhirata.bugzilla → edchen
Whiteboard: [ucid:Productivity21, 1.5:p3, ft:productivity] → [ucid:Productivity21, 1.5:p3, ft:productivity][2.1-feature-qa+]
Edward - Can you indicate which test cases are covered by this user story?
Flags: in-moztrap?(edchen)
Verified User story is fixed.  Email signature is present.

Gaia      e424c85eda87a40c0fa64d6a779c3fa368bf770b
BuildID   20140825040204
Version   34.0a1 Jun 27 15:57:58 CST 2014
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.