[MMS]Auto suggestion for email address

RESOLVED FIXED in 2.0 S4 (20june)

Status

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: ta-matsuura, Assigned: na-matsumoto)

Tracking

unspecified
2.0 S4 (20june)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

[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.
Depends on: 974333
Blocks: 974333
No longer depends on: 974333
Assignee: nobody → na-matsumoto
Group: kddi-confidential
Summary: [MADAI][MMS]Auto suggestion for email address → [MMS]Auto suggestion for email address
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?
Flags: needinfo?(francisco.jordano)
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.
Flags: needinfo?(francisco.jordano)
Hi, Francisco.
Thanks your answer.

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

Would you please review it?
Flags: needinfo?(francisco.jordano)
How might it be the result of the review?
Is there any findings?
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.
Flags: needinfo?(francisco.jordano) → needinfo?(felash)
Hi, Julien. Thank you for answer.
Thanks in advance.
Are there any visual spec for this? Who did them? Were them validated by our visual design team?
Flags: needinfo?(felash)
Posted file github PR (obsolete) —
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!
Attachment #8423896 - Flags: review-
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.
Thought of something now: I think something is missing: the message type should switch to 'mms' when the user uses an email.
Posted patch Bug974867.patch (obsolete) — Splinter Review
Hi, Julien. Thank you for pointed out.
I re-modificate and raised in the attachment 8425389 [details] [diff] [review].
Would you please review again?
Flags: needinfo?(felash)
Posted patch Bug974867_r2.patch (obsolete) — Splinter Review
I'm sorry plesase review the attachment 8425397 [details] [diff] [review].
Hi Julien. 
Please review again.
https://github.com/mozilla-b2g/gaia/pull/19468
Flags: needinfo?(noef)
Flags: needinfo?(noef)
I am sorry. I was wrong destination.
Hi Julien. 
Please review again.
https://github.com/mozilla-b2g/gaia/pull/19468
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.
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 !
Flags: needinfo?(felash)
Posted patch Bug974867_r3.patch (obsolete) — Splinter Review
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?
Flags: needinfo?(felash)
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?
Posted patch Bug974867_r4.patch (obsolete) — Splinter Review
Hi, Julien. Thank you for pointed out.
Would you please review again?
Posted patch Bug974867_r5.patch (obsolete) — Splinter Review
I modified the patch in response to a change of the "master".
Would you please review again?
Today was already full, will review tomorrow.
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.
Flags: needinfo?(felash) → needinfo?(na-matsumoto)
Attachment #8430460 - Attachment is obsolete: true
Flags: needinfo?(na-matsumoto)
Attachment #8429902 - Attachment is obsolete: true
Attachment #8425389 - Attachment is obsolete: true
Attachment #8425397 - Attachment is obsolete: true
Attachment #8432426 - Attachment is obsolete: true
Hi, Julien. Thank you for answer.
Would you please review again?

https://github.com/mozilla-b2g/gaia/pull/19977
Flags: needinfo?(felash)
Target Milestone: --- → 2.0 S4 (20june)
Blocks: 974878
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.
Flags: needinfo?(felash)
Posted file github PR (obsolete) —
Attachment #8423896 - Attachment is obsolete: true
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.
Flags: needinfo?(felash)
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.
Blocks: 982029
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.
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.
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
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
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
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 !
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
Blocks: 1026384
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.
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
More comments on github, only for tests.

The code looks right now.
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
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.
Flags: sec-review?
Flags: needinfo?(schung)
Flags: needinfo?(felash)
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...
Flags: needinfo?(schung)
Flags: needinfo?(felash)
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
Flags: needinfo?(schung)
Flags: needinfo?(felash)
(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
Flags: needinfo?(schung)
Flags: needinfo?(felash)
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.
Flags: needinfo?(schung)
Blocks: 997547
Flags: sec-review? → sec-review?(ptheriault)
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.
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.
Flags: needinfo?(schung)
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.
Flags: needinfo?(schung)
Flags: needinfo?(felash)
Hi, Steve. I rebase.
And, PR again.

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

Would you please review again?

Thanks in advance.
Naoya.
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.
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.
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.
Flags: needinfo?(schung)
Flags: needinfo?(felash)
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.
Flags: needinfo?(schung)
Flags: needinfo?(felash)
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?
Attachment #8435156 - Attachment is obsolete: true
Attachment #8448619 - Flags: review+
Flags: needinfo?(schung)
(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.
Flags: needinfo?(felash)
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.
Flags: needinfo?(schung)
Flags: needinfo?(felash)
Thanks for the contribution!

In master: 30a34e00f347f447c1466b5b6d628c7d56697be1
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(schung)
Flags: needinfo?(felash)
Resolution: --- → FIXED
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.
Flags: needinfo?(schung)
(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
Flags: needinfo?(schung)
[Blocking Requested - why for this release]:
Nominating this for v2.0, as desired on a 2.0 product.
blocking-b2g: --- → 2.0?
ni? Wayne on how we should be treating these late requests for 2.0.  Wayne, can you drive this bug offline?
Flags: needinfo?(wchang)
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.
blocking-b2g: 2.0? → ---
Flags: needinfo?(wchang)
set partner flag requested by partner.
Group: kddi-confidential
Group: kddi-confidential
Flags: sec-review?(ptheriault)
You need to log in before you can comment on or make changes to this bug.