Taichi Matsuura Reporter
	Description • 10 years ago

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

	Naoya Matsumoto Assignee
	Comment 1 • 10 years ago

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?

	Francisco Jordano [:arcturus] [:francisco]
	Comment 2 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 3 • 10 years ago

Hi, Francisco.
Thanks your answer.

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

Would you please review it?

	Naoya Matsumoto Assignee
	Comment 4 • 10 years ago

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

	Julien Wajsberg [:julienw]
	Comment 5 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 6 • 10 years ago

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

	Julien Wajsberg [:julienw]
	Comment 7 • 10 years ago

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

	Julien Wajsberg [:julienw]
	Comment 8 • 10 years ago

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!

	Julien Wajsberg [:julienw]
	Comment 9 • 10 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 10 • 10 years ago

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

	Naoya Matsumoto Assignee
	Comment 12 • 10 years ago

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

	Naoya Matsumoto Assignee
	Comment 14 • 10 years ago

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

	Naoya Matsumoto Assignee
	Comment 15 • 10 years ago

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

	Naoya Matsumoto Assignee
	Comment 16 • 10 years ago

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

	Julien Wajsberg [:julienw]
	Comment 17 • 10 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 18 • 10 years ago

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 !

	Naoya Matsumoto Assignee
	Comment 20 • 10 years ago

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?

	Naoya Matsumoto Assignee
	Comment 21 • 10 years ago

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?

	Naoya Matsumoto Assignee
	Comment 22 • 10 years ago

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

	Naoya Matsumoto Assignee
	Comment 23 • 10 years ago

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

	Julien Wajsberg [:julienw]
	Comment 24 • 10 years ago

Today was already full, will review tomorrow.

	Julien Wajsberg [:julienw]
	Comment 25 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 26 • 10 years ago

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

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

	Julien Wajsberg [:julienw]
	Comment 27 • 10 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 28 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 30 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 31 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 32 • 10 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 33 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 34 • 10 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 35 • 10 years ago

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

	Naoya Matsumoto Assignee
	Comment 36 • 10 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 37 • 10 years ago

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 !

	Naoya Matsumoto Assignee
	Comment 38 • 10 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 39 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 40 • 10 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 41 • 10 years ago

More comments on github, only for tests.

The code looks right now.

	Naoya Matsumoto Assignee
	Comment 42 • 10 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 43 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 44 • 10 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 45 • 10 years ago

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

	Naoya Matsumoto Assignee
	Comment 46 • 10 years ago

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

	Steve Chung [:steveck]
	Comment 47 • 10 years ago

(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

	Naoya Matsumoto Assignee
	Comment 48 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 49 • 10 years ago

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.

	Steve Chung [:steveck]
	Comment 50 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 51 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 52 • 10 years ago

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

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

Would you please review again?

Thanks in advance.
Naoya.

	Naoya Matsumoto Assignee
	Comment 53 • 10 years ago

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.

	Naoya Matsumoto Assignee
	Comment 54 • 10 years ago

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.

	Steve Chung [:steveck]
	Comment 55 • 10 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 56 • 10 years ago

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

	Naoya Matsumoto Assignee
	Comment 57 • 10 years ago

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.

	Steve Chung [:steveck]
	Comment 58 • 10 years ago

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?

	Julien Wajsberg [:julienw]
	Comment 59 • 10 years ago

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

	Naoya Matsumoto Assignee
	Comment 60 • 10 years ago

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.

	Steve Chung [:steveck]
	Comment 61 • 10 years ago

Thanks for the contribution!

In master: 30a34e00f347f447c1466b5b6d628c7d56697be1

	Naoya Matsumoto Assignee
	Comment 62 • 10 years ago

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.

	Steve Chung [:steveck]
	Comment 63 • 10 years ago

(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

	at-kitamura
	Comment 64 • 10 years ago • Edited

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

	Tony Chung [:tchung]
	Comment 65 • 10 years ago • Edited

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 66 • 10 years ago • Edited

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.

	Makoto Kato [:m_kato]
	Comment 67 • 10 years ago

set partner flag requested by partner.