974867 - [MMS]Auto suggestion for email address

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Taichi Matsuura]

[User story]
1. Run message app
2. Create new messge
3. Manuallu type email adress as a recipient.
4. Show suggestion when email adress matches an existing contact.
5. Set email address when user chooose one contacts.

This is same feature as auto-suggestion for phone number.

Comment 1

[Naoya Matsumoto]

Hi, Francisco.

I registered as a local branch of the corresponding one for the following bugs:
 bug 974867
 bug 974878
 bug 982029
 
Can I PR as a branch of one of these bugs?
Or, do I need to PR as a branch of one to one bug?

Comment 2

[Francisco Jordano [:arcturus] [:francisco]]

Hi Naoya,

is better always smaller PRs, so yes, I would do 3 prs one per bug, also for reviewers will be easy to do the review.

F.

Comment 3

[Naoya Matsumoto]

Hi, Francisco.
Thanks your answer.

I PR bug 974867.
https://github.com/mozilla-b2g/gaia/pull/18874

Would you please review it?

Comment 4

[Naoya Matsumoto]

How might it be the result of the review?
Is there any findings?

Comment 5

[Julien Wajsberg [:julienw]]

Francisco is in holiday until next week, that's why he didn't see your request.

I'm taking the needinfo, I'll look at it today or tomorrow.

Comment 6

[Naoya Matsumoto]

Hi, Julien. Thank you for answer.
Thanks in advance.

Comment 7

[Julien Wajsberg [:julienw]]

Are there any visual spec for this? Who did them? Were them validated by our visual design team?

Comment 8

[Julien Wajsberg [:julienw]]

Attachment: github PR

I reviewed the current pull request and it's not ready yet.

While some parts are really good in my opinion (expecially the parts that deal with displaying the contacts) some other parts are confusing to me, and other parts should not be part of this patch but should be in other bugs.

Please answer the questions and remove the things I asked to remove and I'll review again.

Thanks!

Comment 9

[Julien Wajsberg [:julienw]]

Also, I'm sorry that this review took this long. I didn't notice it was a review request and as a result I wanted to wait for Francisco's return.

Comment 10

[Julien Wajsberg [:julienw]]

Thought of something now: I think something is missing: the message type should switch to 'mms' when the user uses an email.

Comment 11

[Naoya Matsumoto]

Attachment: Bug974867.patch

Comment 12

[Naoya Matsumoto]

Hi, Julien. Thank you for pointed out.
I re-modificate and raised in the attachment 8425389 [details] [diff] [review].
Would you please review again?

Comment 13

[Naoya Matsumoto]

Attachment: Bug974867_r2.patch

Comment 14

[Naoya Matsumoto]

I'm sorry plesase review the attachment 8425397 [details] [diff] [review].

Comment 15

[Naoya Matsumoto]

Hi Julien. 
Please review again.
https://github.com/mozilla-b2g/gaia/pull/19468

Comment 16

[Naoya Matsumoto]

I am sorry. I was wrong destination.
Hi Julien. 
Please review again.
https://github.com/mozilla-b2g/gaia/pull/19468

Comment 17

[Julien Wajsberg [:julienw]]

Yes, I'm sorry, I was very busy with other tasks during this week. Just wanted to report...

I may have some time during the week-end but I don't promise, otherwise it will be monday.

Comment 18

[Julien Wajsberg [:julienw]]

I added some more comments on the pull request.

Please attach the pull request to review here, obsolete the previous patches, and ask the review request once you're reday.

Thanks !

Comment 19

[Naoya Matsumoto]

Attachment: Bug974867_r3.patch

Comment 20

[Naoya Matsumoto]

Hi, Julien. Thank you for pointed out.
I re-modificate and raised in the attachment 8429902 [details] [diff] [review] [diff] [review].
Would you please review again?

Comment 21

[Naoya Matsumoto]

Hi, Julien. Thank you for pointed out.
I re-modificate and raised in the attachment 8429902 [details] [diff] [review] [diff] [review].
Would you please review again?

Comment 22

[Naoya Matsumoto]

Attachment: Bug974867_r4.patch

Hi, Julien. Thank you for pointed out.
Would you please review again?

Comment 23

[Naoya Matsumoto]

Attachment: Bug974867_r5.patch

I modified the patch in response to a change of the "master".
Would you please review again?

Comment 24

[Julien Wajsberg [:julienw]]

Today was already full, will review tomorrow.

Comment 25

[Julien Wajsberg [:julienw]]

I'm sorry, I don't know what to review here.

Please obsolete the old patches (you can "Edit Details" on the attachment page, and check "obsolete"; or alternatively you can obsolete the older attachments when you attach a new one).

Alternatively, please push on a pull request and attach it (just choose "paste text as attachment" and put the pull request link). Please also close older obsolete pull requests.

Once you'll do it, I'll review it as soon as possible.

Thanks.

Comment 26

[Naoya Matsumoto]

Hi, Julien. Thank you for answer.
Would you please review again?

https://github.com/mozilla-b2g/gaia/pull/19977

Comment 27

[Julien Wajsberg [:julienw]]

I'll assume that the last attachment "https://github.com/mozilla-b2g/gaia/pull/18874" is obsolete as well and I'll review https://github.com/mozilla-b2g/gaia/pull/19977.

Comment 28

[Julien Wajsberg [:julienw]]

I added comments on github. The code looks actually quite good.

Please add the comment fixes in a separate commit to ease the next review, and push to the same pull request.

Comment 29

[Julien Wajsberg [:julienw]]

Attachment: github PR

Comment 30

[Naoya Matsumoto]

I'm sorry. I don't know the way to push to the same pull request.
So, I pushed the not same pull request
https://github.com/mozilla-b2g/gaia/pull/20135/files

Thanks in advance.
Naoya.

Comment 31

[Naoya Matsumoto]

Hi, Julien. Thank you for review.
I have corresponded what you pointed out.
Would you please review again?

https://github.com/mozilla-b2g/gaia/pull/20205

Thanks in advance.
Naoya.

Comment 32

[Naoya Matsumoto]

Hi, Julien. Thank you for review.
I have corresponded what you pointed out.
Would you please review again?

This bug is blocking several bugs.
So, I want to put master as soon as possible.

https://github.com/mozilla-b2g/gaia/pull/20205

Thanks in advance.
Naoya.

Comment 33

[Julien Wajsberg [:julienw]]

I added some comments on the bug.

Basically, I'd like that the new functionality lands, but behind a flag. The reason is that the functionality is not complete yet, and I don't want to have unfinished things. And I also don't want to land on a separate branch because it's painful.

Comment 34

[Naoya Matsumoto]

Hi, Julien. Thank you for review.
I have corresponded what you pointed out.
Would you please review again?

https://github.com/mozilla-b2g/gaia/pull/20439/files

Thanks in advance.
Naoya.

Comment 35

[Julien Wajsberg [:julienw]]

Added new comments.

I'd like a flag for the new functionality, as said in comment 33.

Please tell me if you'd need help.

Thanks

Comment 36

[Naoya Matsumoto]

Hi, Julien. Thank you for review.
I have corresponded what you pointed out. (as you said in comment 33)
Would you please review again?

https://github.com/mozilla-b2g/gaia/pull/20569

Thanks in advance.
Naoya.

Comment 37

[Julien Wajsberg [:julienw]]

Added more github comments.

I'm sure you can do a lot more than this copy/paste spaghetti. I've put comments in all locations where we can use the flag in a more useful way, with code example on how to do it.

Hopefully we'll be able to have a good patch next time. I'm very sad because the previous patch (without the flag) was quite good. So I'm really optimistic for next one !

Comment 38

[Naoya Matsumoto]

Sorry. Julien. Thank you for review.
I had thought  it was no problem in redundant processing, because this is temporary correspondence.
I correct it.

Would you please review again?

https://github.com/mozilla-b2g/gaia/pull/20594/files

Thanks in advance.
Naoya.

Comment 39

[Julien Wajsberg [:julienw]]

Added more comments on github.

The code looks good except the early return in contact_renderer, but you still need a rebase to current master and I added comments to the unit tests.

Comment 40

[Naoya Matsumoto]

Hi, Julien. Thank you for review.
I have corresponded what you pointed out.
Would you please review again?

https://github.com/mozilla-b2g/gaia/pull/20680

Thanks in advance.
Naoya.

Comment 41

[Julien Wajsberg [:julienw]]

More comments on github, only for tests.

The code looks right now.

Comment 42

[Naoya Matsumoto]

Hi, Julien. Thank you for review.
I have corresponded what you pointed out.
Would you please review again?

https://github.com/mozilla-b2g/gaia/pull/20726

Thanks in advance.
Naoya.

Comment 43

[Julien Wajsberg [:julienw]]

Comments on github.

Don't forget to run "APP=sms make hint" before asking your next review. Also I think you missed my last comment in [1]: we need test for the new code in thread_ui.js.

[1] https://github.com/mozilla-b2g/gaia/pull/20680#discussion_r13925486

Please ping me if you need help on this.

Note that I'll be in holiday next week, you'll need to continue with Steve Chung (:steveck) for this patch and the other patches.

Comment 44

[Naoya Matsumoto]

Hi, Julien and Steave. Thank you for review.
I have corresponded what you pointed out.
Would you please review again?

https://github.com/mozilla-b2g/gaia/pull/20779

Thanks in advance.
Naoya.

Comment 45

[Julien Wajsberg [:julienw]]

Sorry, but your change in mock_contact made the tests in contact_renderer_test.js fail. See https://travis-ci.org/mozilla-b2g/gaia/jobs/28017891.

Please fix them and I think it's good. But you'll need to ask review from Steve as I'll leave tomorrow...

Comment 46

[Naoya Matsumoto]

Hi, Julien and Steave. Thank you for review.
I have corresponded what you pointed out.
And I remerge on the current master.
Would you please review again?

https://github.com/mozilla-b2g/gaia/pull/20879/files

Thanks in advance.
Naoya.
https://github.com/mozilla-b2g/gaia/pull/20879

Comment 47

[Steve Chung [:steveck]]

(In reply to Naoya Matsumoto from comment #46)
> Hi, Julien and Steave. Thank you for review.
> I have corresponded what you pointed out.
> And I remerge on the current master.
> Would you please review again?
> 
> https://github.com/mozilla-b2g/gaia/pull/20879/files
> 
> Thanks in advance.
> Naoya.
> https://github.com/mozilla-b2g/gaia/pull/20879

Please note that Julien is out of office this week, you could just ping me for review request.

I left some comments on github, 2 major issues here:
1 ) Missing mock for utils.isEmailAddress will break the unit test
2 ) We need different icon size for high dpi device but different resolution

Please fix these issues and request review again, thanks

Comment 48

[Naoya Matsumoto]

Hi, Steve. Thanks in reviewing.

>>1 ) Missing mock for utils.isEmailAddress will break the unit test
Sorry. I missed it. I add this faile.

>> 2 ) We need different icon size for high dpi device but different resolution
I see.
I created PNG icon based on attachment 8399993 [details](SVG File).
In spite of changing "width" and "height" and "viewBox" in attachment 8399993 [details],
I can't create the multiple size PNG icon(size @1.5x or @2x or o2.25).

Please tell me the way to create the multiple size PNG icon.

Thanks in advance.
Naoya.

Comment 49

[Naoya Matsumoto]

Hi, Steve. Thank you for review.
I have corresponded what you pointed out.
Would you please review again?

https://github.com/mozilla-b2g/gaia/pull/21004

Thanks in advance.
Naoya.

Comment 50

[Steve Chung [:steveck]]

Hi Naoya, I left comment on github. BTW, may I know your problem while using github? We think pushing the commits into the same pull request would be helpful for review  process.

Comment 51

[Naoya Matsumoto]

Hi, Steve. Thank you for review.
Sorry, I don't know how to push the commits into the same pull request.
But I pushed the same branch and pull request.
So, I think it is easy for you to understand the difference.

https://github.com/mozilla-b2g/gaia/pull/21089/

I have corresponded what you pointed out. (change icon)
Would you please review again?

Thanks in advance.
Naoya.

Comment 52

[Naoya Matsumoto]

Hi, Steve. I rebase.
And, PR again.

https://github.com/mozilla-b2g/gaia/pull/21105

Would you please review again?

Thanks in advance.
Naoya.

Comment 53

[Naoya Matsumoto]

Hi, Steve. I PR again becaouse ot TRAVIS error.

https://github.com/mozilla-b2g/gaia/pull/21147

Would you please review again?

Thanks in advance.
Naoya.

Comment 54

[Naoya Matsumoto]

Sorry. I removed the extra log output.I PR again.

https://github.com/mozilla-b2g/gaia/pull/21148
Would you please review again?

Thanks in advance.
Naoya.

Comment 55

[Steve Chung [:steveck]]

Hi Naoya, the patch looks fine, thanks!

Could you please add the attachemnt with github and squash the commits into one commit with full title like : Bug 974867 - [MMS]Auto suggestion for email address ? In this case you can use "git rebase -i HEAD~3" to adjust your latest 3 commits. Please squash into one commit and rename the title.

Comment 56

[Julien Wajsberg [:julienw]]

Please read http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html for more information about the squashing process.

Comment 57

[Naoya Matsumoto]

Hi, Steve and Julien. Thanks for your review.
I squash the commit and rebase it.
I PR again.

https://github.com/mozilla-b2g/gaia/pull/21218

Would you please review again?

Thanks in advance.
Naoya.

Comment 58

[Steve Chung [:steveck]]

Attachment: Auto suggestion for email address patch on github

Hi Naoya, please submit the attachment with github pull request(you simply need to paste the pull request link into the file input box) and request review next time.

Hi Julien, Wesley suggested that we can just need to flag the checkin-need instead of merging by ourself, wdyt?

Comment 59

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #58)

> Hi Julien, Wesley suggested that we can just need to flag the checkin-need
> instead of merging by ourself, wdyt?

I have no preference, but we need the "r=" line in either the commit itself (preferred) or the merge commit.

Comment 60

[Naoya Matsumoto]

Hi, Steve and Julien. Thanks for your review.
If there are no findings, could you please put this patch into the master?
This bug is blocking several bugs.

Comment 61

[Steve Chung [:steveck]]

Thanks for the contribution!

In master: 30a34e00f347f447c1466b5b6d628c7d56697be1

Comment 62

[Naoya Matsumoto]

Hi, Steve. 　Thanks to merge it.

I have one question.

Now, "supportEmailRecipient" is set "false" in dfault.
I think the flag must change "true" in the future.
Is it correct?
In that case, please tell me when to be changed.

Comment 63

[Steve Chung [:steveck]]

(In reply to Naoya Matsumoto from comment #62)
> Hi, Steve. 　Thanks to merge it.
> 
> I have one question.
> 
> Now, "supportEmailRecipient" is set "false" in dfault.
> I think the flag must change "true" in the future.
> Is it correct?
> In that case, please tell me when to be changed.

We could reset the flag (or simply remove it) when whole feature is ready, thanks

Comment 64

[at-kitamura]

[Blocking Requested - why for this release]:
Nominating this for v2.0, as desired on a 2.0 product.

Comment 65

[Tony Chung [:tchung]]

ni? Wayne on how we should be treating these late requests for 2.0.  Wayne, can you drive this bug offline?

Comment 66

[Lawrence Mandel [:lmandel] (use needinfo)]

From a thread with Wayne and Lucas I can confirm that it is too late to accept this new feature work in 2.0. If this work is required for partner product, it will need to be cherry picked off of the 2.1 branch.

Comment 67

[Makoto Kato [:m_kato]]

set partner flag requested by partner.