838011 - [E-Mail][User Story] Add email address to contacts

Status: VERIFIED FIXED

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

Description

[Peter Dolanjski [:pdol]]

UCID: Email-140

User Story:
As a user, I want to be able to save a displayed email address (from a composed or received email) to either an exisitng contact or as a new contact so that I can quickly add a new contact to my list.

Comment 1

[Arun Balachandran Ganesan [:abc]]

Specs:

https://www.dropbox.com/s/ccvpeiqnltexbvb/saving-email-contacts.pdf

Looking for concerns/feedback.

Comment 2

[Arun Balachandran Ganesan [:abc]]

Specs:

https://www.dropbox.com/s/ccvpeiqnltexbvb/saving-email-contacts.pdf

Looking for concerns/feedback.

Comment 3

[Arun Balachandran Ganesan [:abc]]

Specs:

https://www.dropbox.com/s/ccvpeiqnltexbvb/saving-email-contacts.pdf

Looking for concerns/feedback.

Comment 4

[Arun Balachandran Ganesan [:abc]]

Proposed changes: (should this be listed as a different low priority bug?)

When I receive a mail from someone and I try to add the contact, the options should be
i) reply
ii) new message
iii) create new contact
iv) add to existing contact

When I try to add the contact that is CC'ed, the options should be
i) new message
ii) create new contact
iii) add to existing contact

Thoughts welcome.

P.S: Sorry for the previous triple posting :|

Comment 5

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

assign to Arthur

Comment 6

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Patch v1

Proposed patch v1. 

Arun, currently there is no function of composing a new message in the action menu. And the function is out of the scope of this user story. Do you want to create another bug for it?

Comment 7

[Arun Balachandran Ganesan [:abc]]

Arthur, I will discuss with UX team and create a bug for that if necessary. Apart from that, rest is okay?

Thanks!

Comment 8

[Arthur Chen [:arthurcc][inactive after 6/30]]

Okay, thank you Arun!

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Link to https://github.com/mozilla-b2g/gaia/pull/8292

Andrew, Alberto, could you help review the changes made in Email and Contacts app? Details please refer to the pull request, thanks!

Stas, two string IDs were added:
message-contact-menu-create
message-contact-menu-add-existing

Comment 10

[Axel Hecht [:Pike]]

How did this bug get leo+ ? Not that I'm debating, I'm just not finding evidence of it in the history of this bug.

Comment 11

[Axel Hecht [:Pike]]

Comment on attachment 717778 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/8292

There are some inconsistencies between the placeholder text and the extracted strings, like "Create new contact" vs "Create as new contact".

The strings in the .properties look good, I didn't dive much deeper. Clearing review because I don't think that this patch really needs review by stas or me.

Comment 12

[Arthur Chen [:arthurcc][inactive after 6/30]]

Axel, thanks for reviewing. It is a feature of v1.1 and is leo+ by default.  

(In reply to Axel Hecht [:Pike] from comment #10)
> How did this bug get leo+ ? Not that I'm debating, I'm just not finding
> evidence of it in the history of this bug.

Comment 13

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

Comment on attachment 717778 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/8292

Thanks for the patch!

For the e-mail part of the implementation, this will need another rev:

- There's no need to copy/paste the try/catch with the console.log, and preferably you could remove that from the existing code.

- We shouldn't be displaying these options if the e-mail address is already in the address book.  Bug 832824 has a patch (that needs some changes) to perform lookup from the contacts API.  It should probably serve as the basis for doing the lookup here, but should not block your efforts here.  So possibly take the ContactDataManager from that patch and run with, etc.

If the e-mail address is already known, it seems like we should be triggering the contacts app to display the contact card already associated with the e-mail address.  Perhaps an 'open' activity?  I believe current convention is that 'view' is for a URL, and we use 'open' to display existing blobs, so it might be reasonable.  You'd want the contacts app owners to make that decision.

At some point I thought there were wireframes that supported this, but I am having trouble finding them right now.  I'll do a needinfo in a comment now...

Comment 14

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

(In reply to Arun Balachandran Ganesan [:abc] from comment #4)
> Proposed changes: (should this be listed as a different low priority bug?)
> 
> When I receive a mail from someone and I try to add the contact, the options
> should be
> i) reply
> ii) new message
> iii) create new contact
> iv) add to existing contact
> 
> When I try to add the contact that is CC'ed, the options should be
> i) new message
> ii) create new contact
> iii) add to existing contact
> 
> Thoughts welcome.

Following up on my review comment, the wire-frames need to address the situation where the e-mail address is already known to belong to a contact.

I would propose amending the above to be somewhat like this.  If you show the resulting sets of menus, there should 4.

== From/To/Cc/Bcc related rules:
If the clicked-on bubble is the bubble is the sender of the message ('from'), add the option:
- Reply

For all bubbles, add the option:
- New messages

== Contact related rules:

When I click on an e-mail bubble and the e-mail address is already present in a contact on the contacts database, add the option:
- View contact

When I click on an e-mail bubble and the e-mail address is not already present in a contact on the contacts database, add the options:
- Create new contact
- Add to existing contact


In regards to comment 6, I agree that there is some very minor scope increase related to the user story in terms of 'new' or what I am mentioning here, but I believe the changes are in the intended spirit of the feature request and are all sufficiently inter-related and simple enough that it makes sense to address them here.

(Our current contacts interaction in the e-mail app is bad/weird, and I think only doing literally what the user story requests would be bad/weird in a different way that would result in a ton of duplicate contacts or e-mail entries on contacts, etc.)

Comment 15

[jachen]

FYI- Arun is out at a conference today. He'll respond later this evening or tomorrow. Thanks!

Comment 16

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 717778 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/8292

Cancel the review request given that we may need to add other functions. Let's wait for UX inputs.

Comment 17

[Arun Balachandran Ganesan [:abc]]

Thanks, Andrew. I agree with most of what you have said in this comment.

If the user taps on view contact, will it take the user to the contacts app? (meaning can we show a contact page within the mail app itself? If we can do that, then we can possibly directly display the contact and the user can tap the email to send a new message instead of showing a dialog)

Also, should we standardize usage of "messages" as "mails" instead? (or maybe I'm the only one abusing it) Do we say new mail, compose mail or something like that already? (or we have to create one now)

I will be posting updated specs soon.


(In reply to Andrew Sutherland (:asuth) from comment #14)
> (In reply to Arun Balachandran Ganesan [:abc] from comment #4)
> > Proposed changes: (should this be listed as a different low priority bug?)
> > 
> > When I receive a mail from someone and I try to add the contact, the options
> > should be
> > i) reply
> > ii) new message
> > iii) create new contact
> > iv) add to existing contact
> > 
> > When I try to add the contact that is CC'ed, the options should be
> > i) new message
> > ii) create new contact
> > iii) add to existing contact
> > 
> > Thoughts welcome.
> 
> Following up on my review comment, the wire-frames need to address the
> situation where the e-mail address is already known to belong to a contact.
> 
> I would propose amending the above to be somewhat like this.  If you show
> the resulting sets of menus, there should 4.
> 
> == From/To/Cc/Bcc related rules:
> If the clicked-on bubble is the bubble is the sender of the message
> ('from'), add the option:
> - Reply
> 
> For all bubbles, add the option:
> - New messages
> 
> == Contact related rules:
> 
> When I click on an e-mail bubble and the e-mail address is already present
> in a contact on the contacts database, add the option:
> - View contact
> 
> When I click on an e-mail bubble and the e-mail address is not already
> present in a contact on the contacts database, add the options:
> - Create new contact
> - Add to existing contact
> 
> 
> In regards to comment 6, I agree that there is some very minor scope
> increase related to the user story in terms of 'new' or what I am mentioning
> here, but I believe the changes are in the intended spirit of the feature
> request and are all sufficiently inter-related and simple enough that it
> makes sense to address them here.
> 
> (Our current contacts interaction in the e-mail app is bad/weird, and I
> think only doing literally what the user story requests would be bad/weird
> in a different way that would result in a ton of duplicate contacts or
> e-mail entries on contacts, etc.)

Comment 18

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

(In reply to Arun Balachandran Ganesan [:abc] from comment #17)
> Thanks, Andrew. I agree with most of what you have said in this comment.
> 
> If the user taps on view contact, will it take the user to the contacts app?
> (meaning can we show a contact page within the mail app itself? If we can do
> that, then we can possibly directly display the contact and the user can tap
> the email to send a new message instead of showing a dialog)

We can make this happen, yes.  It will require some small additional changes to the contacts app.

> Also, should we standardize usage of "messages" as "mails" instead? (or
> maybe I'm the only one abusing it) Do we say new mail, compose mail or
> something like that already? (or we have to create one now)

I don't have a strong opinion on this, needsinfo-ing Casey.  I think the obvious options for standardizing are mail/email/e-mail/message.  Message has the benefit that we're less likely to pick the wrong choice amongst mail/email/e-mail.  Message, although slightly longer, may also be a more realistic length for what the localization of "mail" would be in other languages, so it might help us avoid creating awkward situations for localizers.

Here is our current set of strings in use, if you want to check those out:
https://github.com/mozilla-b2g/gaia/blob/master/apps/email/locales/email.en-US.properties


PS: no need to hit the reply button unless you're going to take advantage of context for quoting.  And if you do quote, it's best to trim down to the core of the question, say a paragraph or two.

Comment 19

[Arun Balachandran Ganesan [:abc]]

> I don't have a strong opinion on this, needsinfo-ing Casey.  I think the
> obvious options for standardizing are mail/email/e-mail/message.  

Intention is to not confuse "messages" in messaging with "mails".

> PS: no need to hit the reply button unless you're going to take advantage of
> context for quoting.  And if you do quote, it's best to trim down to the
> core of the question, say a paragraph or two.

Thanks for this :D Definitely useful. Still a bugzilla n00b.

Comment 20

[Casey Yee]

(In reply to Andrew Sutherland (:asuth) from comment #18)
> (In reply to Arun Balachandran Ganesan [:abc] from comment #17)
> > Thanks, Andrew. I agree with most of what you have said in this comment.
> > 
> > If the user taps on view contact, will it take the user to the contacts app?
> > (meaning can we show a contact page within the mail app itself? If we can do
> > that, then we can possibly directly display the contact and the user can tap
> > the email to send a new message instead of showing a dialog)
> 
> We can make this happen, yes.  It will require some small additional changes
> to the contacts app.

I think we should keep the dialogue options instead of going directly to the contact.  This keeps the user in-context with the email app with the email/reply options and avoids us having to do all that heavy loading of the contact.

> 
> > Also, should we standardize usage of "messages" as "mails" instead? (or
> > maybe I'm the only one abusing it) Do we say new mail, compose mail or
> > something like that already? (or we have to create one now)
> 
> I don't have a strong opinion on this, needsinfo-ing Casey.  I think the
> obvious options for standardizing are mail/email/e-mail/message.  Message
> has the benefit that we're less likely to pick the wrong choice amongst
> mail/email/e-mail.  Message, although slightly longer, may also be a more
> realistic length for what the localization of "mail" would be in other
> languages, so it might help us avoid creating awkward situations for
> localizers.
> 
> Here is our current set of strings in use, if you want to check those out:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/email/locales/email.en-
> US.properties

Let's stick with "mail".  This seems to be consistent with the strings already in place.  We also don't want to confuse e-mail with messaging/sms/mms.

Comment 21

[Arun Balachandran Ganesan [:abc]]

(In reply to Casey Yee [:cyee] from comment #20)
 
> I think we should keep the dialogue options instead of going directly to the
> contact.  This keeps the user in-context with the email app with the
> email/reply options and avoids us having to do all that heavy loading of the
> contact.

Yeah. I think it makes sense with the existing patterns in place. 
 
> Let's stick with "mail".  This seems to be consistent with the strings
> already in place.  We also don't want to confuse e-mail with
> messaging/sms/mms.

Sure.

Comment 22

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

regarding time lines, if updated spec can be confirmed by this week (latest 3/8), implementation can be done by 3/15 by :arthurcc

Comment 23

[Arun Balachandran Ganesan [:abc]]

Thanks for the heads up, Joe. It's always good to know the time lines. I will post updated specs within this week (it's currently in internal UX review).

Cheers,
Arun

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

Arun, given the tight schedule, could you post the draft you have so that I can start to survey how to implement it? Thanks!

Comment 25

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

I have a patch up for review for the back-end (gaia-email-libs-and-more) on bug 848266 that makes it so that what previously was just { name, address } is now { isContact: Boolean, name, address }.

So that works for a received email context.  Then for a compose context, I've exposed an async method MailAPI.resolveEmailAddressToPeep() that can be used to get the same representation.

Comment 26

[Arthur Chen [:arthurcc][inactive after 6/30]]

Thank you Andrew, it really helps. Please ensure the patch gets uplifted to v1-train.

Comment 27

[Arun Balachandran Ganesan [:abc]]

Arthur:

The draft is here (It is pending UX review):

https://www.dropbox.com/s/0i7s7kurju2gmsa/saving-contacts-in-email.pdf

Comment 28

[Chris Lee [:clee]]

Arthur, are you blocked on anything else here?  Let us know if you have concern about completing this for next Friday.  Thanks.

Comment 29

[Chris Lee [:clee]]

Arun, can you comment here once UX has reviewed and r+'ed the spec?

Comment 30

[Arun Balachandran Ganesan [:abc]]

:clee:

Sure. 

https://www.dropbox.com/s/0i7s7kurju2gmsa/saving-contacts-in-email.pdf

I updated the draft again. Once I get the final review done, I will confirm again here.

Cheers,
Arun

Comment 31

[Arthur Chen [:arthurcc][inactive after 6/30]]

Chris, I've started the implementation based on the draft. It will be completed on time if the spec can be confirmed by this Monday.

Comment 32

[Arthur Chen [:arthurcc][inactive after 6/30]]

Arun, two things that need your clarification:
1. On page 4, when users click on the address of the author, should we provide the 'reply' option?
2. On page 11, how to determine the contact without requesting users to pick one?

Thanks!

Comment 33

[Arun Balachandran Ganesan [:abc]]

(In reply to Arthur Chen [:arthurcc] from comment #32)
> Arun, two things that need your clarification:
> 1. On page 4, when users click on the address of the author, should we
> provide the 'reply' option?

Yes, I have included it in the updated specs.

> 2. On page 11, how to determine the contact without requesting users to pick
> one?

I don't understand. Can you please rephrase it for me? (you mean point 11, I assume)

Cheers,
Arun

Comment 34

[Arun Balachandran Ganesan [:abc]]

Arthur:

Here's the specs post-UX review:

https://www.dropbox.com/s/0i7s7kurju2gmsa/saving-contacts-in-email.pdf

Let me know if you have any clarifications to make. Thanks!

Cheers,
Arun

Comment 35

[Arthur Chen [:arthurcc][inactive after 6/30]]

I mean page 11 of the draft, it seems miss a step. But never mind, I will follow the reviewed specs and it looks good! Thanks!

(In reply to Arun Balachandran Ganesan [:abc] from comment #33)
> 
> (In reply to Arthur Chen [:arthurcc] from comment #32)
> > Arun, two things that need your clarification:
> > 1. On page 4, when users click on the address of the author, should we
> > provide the 'reply' option?
> 
> Yes, I have included it in the updated specs.
> 
> > 2. On page 11, how to determine the contact without requesting users to pick
> > one?
> 
> I don't understand. Can you please rephrase it for me? (you mean point 11, I
> assume)
> 
> Cheers,
> Arun

Comment 36

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Link to https://github.com/mozilla-b2g/gaia/pull/8596

Andrew, could you help review the patch? Please be informed that the address is not replaced with the name in the contact. I believe that part will be handled in bug 848266 that you mentioned earlier.

Albert, I've added an 'open' activity and the support of passing a default email while creating a new contact in this patch. Please help review it, thanks!

Comment 37

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

Comment on attachment 723875 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/8596

I tried this out on the phone and this works really well and is a great improvement on what we have right now!  Thanks!

I have a few requested changes in there that I'd like to see made before I give final review, but that should be a very quick review.

In terms of my comments about providing the name instead of splitting up the name into two parts, I am making some asssumptions about how the contacts app and/or mozContacts might handle it that may be wrong...

Comment 38

[Arthur Chen [:arthurcc][inactive after 6/30]]

Andrew, thank you for reviewing. I've updated the patch based on your comment. Please help check it.

Comment 39

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

Comment on attachment 723875 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/8596

r=asuth with the one requested change, noting that, of course, I can't give review on the contacts app, so that still needs review by Alberto or someone.

Comment 40

[Arthur Chen [:arthurcc][inactive after 6/30]]

Alberto, are you available of reviewing the change in these few days? If not, is there anyone you recommended to do the review instead? Thanks.

Comment 41

[alberto.pastor]

Hi Arthur, I missed this bug and that feature already landed -> Bug 853379

Could you try if it works for you?

Thanks!

Comment 42

[Jason Smith [:jsmith]]

Naoki - We have test case coverage for this, right?

Comment 43

[Arthur Chen [:arthurcc][inactive after 6/30]]

Alberto, the update activity with email is what I need, thanks! But it seems not landed in master yet. In this patch I added an 'open' activity which is not included in yours, so I need to rebase based on your patch of Bug 853379.

Comment 44

[alberto.pastor]

Ok, I'll review the rebased "open" activity after Bug 853379 lands (today, hopefully).
Thanks!

Comment 45

[Arthur Chen [:arthurcc][inactive after 6/30]]

Alberto, I've updated my pull request based on your work. Please help review it. Thanks! https://github.com/mozilla-b2g/gaia/pull/8596

Comment 46

[alberto.pastor]

Hi all, I have some questions regarding the email-contacts activity:

- Should the "Edit" button active in the "View" contact activity?
- Should we change the icon for cancelling the activity instead of using the current "back" icon?

Thanks!

Comment 47

[Arthur Chen [:arthurcc][inactive after 6/30]]

Arun, could you help comment on Alberto's questions? Thanks!

Comment 48

[Arthur Chen [:arthurcc][inactive after 6/30]]

Albert, since this is a leo+ bug. Can we land this patch at first and create another follow up of fine tuning the UI?

Comment 49

[alberto.pastor]

Ok, sounds good to me. Can you create the bug for the follow up?

Comment 50

[Arthur Chen [:arthurcc][inactive after 6/30]]

Bug 855292 created as a follow up.

Comment 51

[Arthur Chen [:arthurcc][inactive after 6/30]]

Thanks Alberto!
master: https://github.com/mozilla-b2g/gaia/commit/3879dfeb14791f1556139f089515d0033513b870

Comment 52

[Arun Balachandran Ganesan [:abc]]

Good questions. Thanks, Alberto!

(In reply to Alberto Pastor [:albertopq] from comment #46)
> Hi all, I have some questions regarding the email-contacts activity:
> 
> - Should the "Edit" button active in the "View" contact activity?

Actually, can we have the edit button removed in the "View" contact activity?

> - Should we change the icon for cancelling the activity instead of using the
> current "back" icon?

Good point. The other option that I was debating earlier was using a close button instead. (I don't know of anything else that exists within our current patterns) I am discussing it with the UX team. Let me get back on this.

Cheers,
Arun

Comment 53

[Arun Balachandran Ganesan [:abc]]

I have updated the specs to address concerns made in comment #46:

https://www.dropbox.com/s/0i7s7kurju2gmsa/saving-contacts-in-email.pdf

Thanks, Alberto & Arthur. Any thoughts?

Cheers,
Arun

Comment 55

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 3879dfeb14791f1556139f089515d0033513b870
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 56

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Arthur, could you help please? https://bugzilla.mozilla.org/show_bug.cgi?id=838011#c55

Comment 57

[Arthur Chen [:arthurcc][inactive after 6/30]]

v1-train: https://github.com/mozilla-b2g/gaia/commit/663101b6eb809383e5882d9bc3868a923a57998a

Comment 58

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

initial test case : 6562; I will have more test cases as I test around it.

Comment 59

[Tracy Walker [:tracy]]

verified fixed on Leo with:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/0c71cbc5fe0c
Gaia   a7b0810580afc734f3d5e441914fe895f9c1923e
BuildID 20130508230207
Version 18.0