848266 - [email] Have MailAPI resolve e-mail addresses to contacts

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/138

Comment 2

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

The general idea is that before we report message headers or bodies to the UI/MailAPI consumers, we first resolve e-mail addresses by looking them up in mozContacts.  We aggressively cache.  To avoid the cache growing without bound, we just clear the cache once it reaches a certain size.

I need to do some sanity checking relating to composition...

Comment 3

[Dietrich Ayala (:dietrich)]

Blocks a blocker -> blocking.

Comment 4

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

Andrew, I was wondering what if the contact is deleted after the peep object constructed? In this case the isContact value is invalid.

Comment 5

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

Yes, the isContact field could become invalid either by deletion of a contact or addition of a contact.  I didn't worry about it because it adds a fair bit of complexity and requires us to listen for change notifications, but the likelihood of these changes occurring (other than when the contact is added from the e-mail app directly) is relatively low.

The complexity I was worried about was maintaining a map of live MailPeep instances to update when listening to change events.  Because slices need to have die() called on them explicitly, it's not really that hard, I was just trying for quick-and-dirty.

It seems like there are two options:

1) Maintain an "oncontactchange" listener and a map of contact id's and e-mail addresses to live MozPeep instances.  If we hear a "remove", it's straightforward to update the MozPeep.  If we hear a "create" or "update", we would need to fetch the referenced contact and check its email addresses against our e-mail address map as well as our contact id map.  If the contact has an e-mail address in our map, we update the MozPeep to be a contact.  If the e-mail is not in our map, but the id is in our map, it means the e-mail address was removed and we must update it.

2) Let the MozPeep instance have a recheckIsContact() method that asynchronously re-fetches the contact and makes sure the contact is still really a contact.  This would be suitable for your use-case, at least.

A separate but related issue is generating update events on the MozPeep instances.


Arthur, what are your thoughts on these solutions, and also, are you interested in taking over review on this patch since I think Jim is pretty busy?

Comment 6

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

Andrew, I'm not familiar with the architecture of the mail app and afraid that I'm not able to review it. But I can help come up solutions and implement it. 

Regarding your options, with the event of "contactchange", it is more reasonable to use option 1 which is similar to the pattern of data binding. And by MozPeep do you mean MailPeep that you defined in the patch?

Comment 7

[Jim Porter (:squib)]

I should be able to handle review on this tomorrow.

Comment 8

[Wayne Chang [:wchang]]

Andrew, do we need any of the work you suggested in comment 5 to complete this bug or are they more for future improvements?

Jim, did you end up getting this review done?

Comment 9

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

(In reply to Wayne Chang [:wchang] from comment #8)
> Andrew, do we need any of the work you suggested in comment 5 to complete
> this bug or are they more for future improvements?

I'd been treating them as required for this bug, which is why I hadn't landed the changes.

The priority for the past several weeks has been on performance work and the event listening, and the event listening is not entirely trivial, so I've been putting it off.

Since the contact editing stuff (click on a contact, decide what to do), does not depend on these changes, I think I would be okay if we basically landed what I have and then did the update stuff as a follow-up.  Or we can wait until someone else has time to clean the changes up.

Jim, does it sound reasonable to you to land this as-is for now with a high priority follow-up, or should we wait?

Comment 10

[Jim Porter (:squib)]

(In reply to Andrew Sutherland (:asuth) from comment #9)
> Jim, does it sound reasonable to you to land this as-is for now with a high
> priority follow-up, or should we wait?

I suppose that's ok if we really want this soon. However, I'd prefer (not strongly) to wait until we get it right.

Comment 11

[Jim Porter (:squib)]

Stealing this, as discussed in person at the work week.

Comment 12

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

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/186

Pointer to Github pull-request

Comment 13

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

Comment on attachment 721634 [details]
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/138

(the contents of this pull request are now in squib's pull request)

Comment 14

[Jim Porter (:squib)]

PR up for gaia (waiting on Travis to finish): https://github.com/mozilla-b2g/gaia/pull/9390

Comment 15

[Jim Porter (:squib)]

Landed.

Comment 16

[James Lal [:lightsofapollo]]

Uplifted acffeb126b7e852647b460bffdc67d80f1afb950 to:
v1-train: 71a65ef4e98ff530d893688077997c8262054568