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

VERIFIED FIXED

Status

Firefox OS
Gaia::E-Mail
P1
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: pdol, Assigned: arthurcc)

Tracking

({feature})

unspecified
ARM
Gonk (Firefox OS)
feature
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 verified)

Details

(Whiteboard: [LOE:M][by 3/15], IOT, Poland, Buri)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Keywords: feature
Summary: [B2G][E-Mail][User Story] Add email address to contacts → [E-Mail][User Story] Add email address to contacts

Updated

5 years ago
Flags: needinfo?(aganesan)
Specs:

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

Looking for concerns/feedback.
Flags: needinfo?(aganesan)

Updated

5 years ago
Whiteboard: u=user c=email s=v1.1-sprint-1
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 :|

Updated

5 years ago
Whiteboard: u=user c=email s=v1.1-sprint-1 → u=user c=email s=v1.1-sprint-2
Whiteboard: u=user c=email s=v1.1-sprint-2 → u=aganesan@mozilla.com c=email s=v1.1-sprint-2 p=1
assign to Arthur
Assignee: nobody → achen
Whiteboard: u=aganesan@mozilla.com c=email s=v1.1-sprint-2 p=1 → u=aganesan@mozilla.com c=email s=v1.1-sprint-2 p=1 [LOE:M]
Created attachment 717036 [details]
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?
Flags: needinfo?(aganesan)
Arthur, I will discuss with UX team and create a bug for that if necessary. Apart from that, rest is okay?

Thanks!
Flags: needinfo?(aganesan)
Flags: needinfo?(arthur.chen)

Updated

5 years ago
Depends on: 844312

Updated

5 years ago
Whiteboard: u=aganesan@mozilla.com c=email s=v1.1-sprint-2 p=1 [LOE:M] → [LOE:M]
Okay, thank you Arun!
Flags: needinfo?(arthur.chen)
Assignee: achen → arthur.chen
Created attachment 717778 [details]
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
Attachment #717036 - Attachment is obsolete: true
Attachment #717778 - Flags: review?(stas)
Attachment #717778 - Flags: review?(bugmail)
Attachment #717778 - Flags: review?(alberto.pastor)
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 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.
Attachment #717778 - Flags: review?(stas)
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 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...
Attachment #717778 - Flags: review?(bugmail)
(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.)
Flags: needinfo?(aganesan)

Comment 15

5 years ago
FYI- Arun is out at a conference today. He'll respond later this evening or tomorrow. Thanks!
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.
Attachment #717778 - Flags: review?(alberto.pastor)
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.)
Flags: needinfo?(aganesan) → needinfo?(bugmail)
(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.
Flags: needinfo?(bugmail) → needinfo?(kyee)
> 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

5 years ago
(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.
Flags: needinfo?(kyee)
(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.
regarding time lines, if updated spec can be confirmed by this week (latest 3/8), implementation can be done by 3/15 by :arthurcc
Whiteboard: [LOE:M] → [LOE:M][by 3/15]
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
Arun, given the tight schedule, could you post the draft you have so that I can start to survey how to implement it? Thanks!
Depends on: 848266
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.
Thank you Andrew, it really helps. Please ensure the patch gets uplifted to v1-train.

Comment 28

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

Comment 29

5 years ago
Arun, can you comment here once UX has reviewed and r+'ed the spec?
: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
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.
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!
Flags: needinfo?(aganesan)

(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
Flags: needinfo?(aganesan)
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
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
Created attachment 723875 [details]
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!
Attachment #717778 - Attachment is obsolete: true
Attachment #723875 - Flags: review?(bugmail)
Attachment #723875 - Flags: review?(alberto.pastor)
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...
Attachment #723875 - Flags: review?(bugmail) → feedback+
Andrew, thank you for reviewing. I've updated the patch based on your comment. Please help check it.
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.
Attachment #723875 - Flags: feedback+ → review+
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.
Flags: needinfo?(alberto.pastor)

Comment 41

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

Could you try if it works for you?

Thanks!
Flags: needinfo?(alberto.pastor)
Naoki - We have test case coverage for this, right?
Flags: needinfo?(nhirata.bugzilla)
Flags: in-moztrap?
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.

Updated

5 years ago
Flags: needinfo?(nhirata.bugzilla)
Flags: in-moztrap?(nhirata.bugzilla)
Flags: in-moztrap?

Comment 44

5 years ago
Ok, I'll review the rebased "open" activity after Bug 853379 lands (today, hopefully).
Thanks!
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

5 years ago
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!
Arun, could you help comment on Alberto's questions? Thanks!
Flags: needinfo?(abc)
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

5 years ago
Ok, sounds good to me. Can you create the bug for the follow up?
Bug 855292 created as a follow up.

Updated

5 years ago
Attachment #723875 - Flags: review?(alberto.pastor) → review+
Thanks Alberto!
master: https://github.com/mozilla-b2g/gaia/commit/3879dfeb14791f1556139f089515d0033513b870
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-b2g18: --- → affected
Resolution: --- → FIXED
Blocks: 855292
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
Flags: needinfo?(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
Duplicate of this bug: 854801
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
v1-train: https://github.com/mozilla-b2g/gaia/commit/663101b6eb809383e5882d9bc3868a923a57998a
status-b2g18: affected → fixed
Flags: needinfo?(arthur.chen)
initial test case : 6562; I will have more test cases as I test around it.
Flags: in-moztrap?(nhirata.bugzilla) → in-moztrap+

Updated

5 years ago
Blocks: 869681
Whiteboard: [LOE:M][by 3/15] → [LOE:M][by 3/15], IOT, Poland, Buri

Updated

5 years ago
No longer blocks: 869681
verified fixed on Leo with:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/0c71cbc5fe0c
Gaia   a7b0810580afc734f3d5e441914fe895f9c1923e
BuildID 20130508230207
Version 18.0
Status: RESOLVED → VERIFIED
status-b2g18: fixed → verified
You need to log in before you can comment on or make changes to this bug.