847168 - [User Story] Email Signature

Status: VERIFIED FIXED

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

Description

[sync-1]

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 +++
 
 DEFECT DESCRIPTION:
 There is no email signature when compose a mail
 
 REPRODUCING PROCEDURES:
 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"
 
 EXPECTED BEHAVIOUR:
 There should be email signature
 
 ASSOCIATE SPECIFICATION:
 
 TEST PLAN REFERENCE:
 
 TOOLS AND PLATFORMS USED:
 
 USER IMPACT:
 
 REPRODUCING RATE:
 
 For FT PR, Please list reference mobile's behavior:
 
 ++++++++++ end of initial bug #410950 description ++++++++++
 
 
 
 CONTACT INFO (Name,Phone number):
 
 DEFECT DESCRIPTION:
 
 REPRODUCING PROCEDURES:
 
 EXPECTED BEHAVIOUR:
 
 ASSOCIATE SPECIFICATION:
 
 TEST PLAN REFERENCE:
 
 TOOLS AND PLATFORMS USED:
 
 USER IMPACT:
 
 REPRODUCING RATE:
 
 For FT PR, Please list reference mobile's behavior:

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

partner requested feature. tracking-b2g18+

Comment 2

[Peter Dolanjski [:pdol]]

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.

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

clear tef? as this is not required for launch

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Right, this was out-of-scoped.

Comment 5

[Chris Lee [:clee]]

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.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(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.

Comment 7

[Alex Keybl [:akeybl]]

It's too late to be making enhancements or feature changes at this point.

Comment 8

[FangChen]

the issue has been reported again on FF0S1.1 during the Latam certification
Built ID 20130823171628
Device: Ikura
QC RIL version: "ro.build.firmware_revision=V1.01.00.01.019.180"
gaia commit: 4916f82 Merge branch 'zte/ics_strawberry_cdr' of ssh://10.67.16.41:29418/quic/lf/releases/gaia into ics_strawberry_cdr
gecko commit: a1c2bb0 ZRL modify ACCEPT Http head for MMS

Comment 9

[Peter Dolanjski [:pdol]]

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.

Comment 10

[Adam Rogers (:arog)]

Adding to 1.4 backlog - will need UX review.

Comment 11

[Rob MacDonald [:robmac]]

There is an existing UI for this which I will revisit as UX works through the 1.4 backlog.

Comment 14

[Juwei Huang[:juwei]]

Attachment: [Email] Signature_1.0.pdf

Spec attached.

Comment 16

[Alex Wissmann [:awiss]]

Attachment: backend

Comment 17

[Alex Wissmann [:awiss]]

Attachment: frontend

Comment 18

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8449698 [details] [review]
backend

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.

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8449698 [details] [review]
backend

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:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/mailapi/composite/configurator.js#L227

See https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/0d4848b38e33ecbe9dad303e71a4725104f5c188 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:

https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/mailapi/mailchew.js#L177

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.

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8449699 [details] [review]
frontend

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.)

Comment 22

[Alex Wissmann [:awiss]]

> 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.

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

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?

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 25

[Juwei Huang[:juwei]]

Sure! It works for me, thanks!

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8449698 [details] [review]
backend

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.

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8449699 [details] [review]
frontend

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.

Comment 28

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8449698 [details] [review]
backend

r=asuth conditional on the few nits and the one minor addition.  Feel free to rebase squash your GELAM commits as you desire.

Comment 29

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8449699 [details] [review]
frontend

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.

Comment 30

[James Burke [:jrburke]]

Comment on attachment 8449699 [details] [review]
frontend

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!

Comment 31

[James Burke [:jrburke]]

Comment on attachment 8449699 [details] [review]
frontend

r+ if one final nit is addressed:

https://github.com/mozilla-b2g/gaia/pull/21303/files#discussion_r15917891

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:

https://github.com/jrburke/gaia-dev-zip#for-the-developer

Comment 32

[Alex Wissmann [:awiss]]

Attachment: UX Review -- ZIP

Comment 33

[Alex Wissmann [:awiss]]

Attachment: UX Review -- Screenshots

Comment 34

[Juwei Huang[:juwei]]

Comment on attachment 8469743 [details]
UX Review -- Screenshots

I install the zip file and it looks great!!!!Thank you Alex!

Comment 35

[Peko Chen [:pekochen]]

Comment on attachment 8469743 [details]
UX Review -- Screenshots

Looks nice!!!!Thanks Alex!

Comment 36

[James Burke [:jrburke]]

GELAM changes:

Merged in master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/af384252f8bf137e873a8728b1d439d77819b70e

from pull request:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/318

Gaia changes:

Merged in master:
https://github.com/mozilla-b2g/gaia/commit/39eb04d4da71e5734578265a511200d4613e3879

from pull request:
https://github.com/mozilla-b2g/gaia/pull/21303

Nice work :aswiss!

Comment 37

[Francesco Lodolo [:flod]]

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").
https://github.com/mozilla-b2g/gaia/blob/master/shared/locales/branding/official/branding.en-US.properties

Comment 38

[James Burke [:jrburke]]

:flod, thank you for catching this! I filed bug 1052469 to get that fixed, we should be able to land it quickly.

Comment 39

[Juwei Huang[:juwei]]

Attachment: [Email] Signature_1.1.pdf

Spec updated due to bug 1052951

Comment 40

[Edward Chen[:edchen]]

[Devices]
Flame

[Environment]
Gaia      c8e93dadeaec6a2617b4db8b7d65ba14aa8db378
Gecko     https://hg.mozilla.org/mozilla-central/rev/c14e5feadc61
BuildID   20140821160203
Version   34.0a1
ro.build.version.incremental=94
ro.build.date=Tue 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

Comment 41

[Jason Smith [:jsmith]]

Edward - Can you indicate which test cases are covered by this user story?

Comment 42

[Edward Chen[:edchen]]

Test cases: https://moztrap.mozilla.org/manage/cases/?&pagenumber=1&pagesize=100&sortfield=created_on&sortdirection=desc&filter-tag=3007&filter-productversion=196&filter-suite=717

Comment 43

[Tony Chung [:tchung]]

Verified User story is fixed.  Email signature is present.

Gaia      e424c85eda87a40c0fa64d6a779c3fa368bf770b
Gecko     https://hg.mozilla.org/mozilla-central/rev/daa84204a11a
BuildID   20140825040204
Version   34.0a1
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014
B1TC00011230