898726 - [B2G][Helix][SMS]Manual inputed receiver's phone number will be overrided after select receiver from contacts

Status: VERIFIED FIXED

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

Description

[zhao_tao59]

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; aff-kingsoft-ciba; Zune 4.7)

Steps to reproduce:

1.messages->new message,
2.input a receiver's phone number by the keypad,
3.then click the + icon to add another receiver from contacts


Actual results:

in step 3,manual inputed receiver's phone number will be overrided after select receiver from contacts.




Expected results:

manual inputed receiver's phone number should not be overrided,it should be recognized as a seperated receiver automatically.

though we can input a semicolon after we input receiver's phone number to achive the goal ,but if it is done automatically,it is more convenient for the end user.

Comment 1

[Wayne Chang [:wchang]]

Paul, can you check if this is consistent behaviour with v1-train (leo)?
Nom'ing this for Leo if this is the current behaviour on v1.1.

Please nominate for hd? if this is only observed on v1.1hd.

Comment 2

[Wayne Chang [:wchang]]

Steve, maybe you can confirm if this is the current implementation or a bug on v1.1HD?

Comment 3

[Steve Chung [:steveck]]

It an existing issue in master, so all the branches will have this issue. Please set as blocker to the desired branch, thanks.

Comment 4

[Steve Chung [:steveck]]

Hi Wayne, this behavior might be a little bit annoying, but it seems not a serious blocking issue. It depends on you that setting leo+ if it's argent or koi+ if not. Thanks.

Comment 5

[Wayne Chang [:wchang]]

Given that this is not a regression on v1.1/v1.1HD, I would leave this koi? for now unless anyone sees this a release blocking bug.


(In reply to Steve Chung [:steveck] from comment #3)
> It an existing issue in master, so all the branches will have this issue.
> Please set as blocker to the desired branch, thanks.

Comment 6

[Joe Cheng [:jcheng] (please needinfo)]

ni? ayman as it seems to be per design

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

ignore comment 6. Whatever user types, when the focus is lost, it should wrap. But it is not happening when + is clicked so the bug is when + is clicked, whatever user typed should wrap. this should be fixed for koi for sure but this is impacting v1.1 as well per Steve. Leo?

Comment 8

[Andrew Overholt [:overholt]]

This seems pretty bad since it makes multi-recipient SMSes impossible if one enters a non-contact # before picking a contact as a second recipient.

Comment 9

[Steve Chung [:steveck]]

Attachment: pull-request.html

Hi Borja, it a small fixing that add recipients assimilate ability before contact picked(with one unit test). Please take a look, thanks.

Comment 10

[Borja Salguero [:borjasalguero]]

Comment on attachment 790081 [details]
pull-request.html

Moving to Julien because Im PTO and I have limited access to Internet!

Comment 11

[Julien Wajsberg [:julienw] (PTO -> Mar 17)]

Comment on attachment 790081 [details]
pull-request.html

So, this actually looks like a good workaround if we need a fix for leo real quick (like, this week). However I've put some thoughts on the github pull request and I'd like to have Rick Waldron review this.

Since I'll be away in the end of this week, you can finish the patch with Rick as he wrote most of this code. It's ok to me to first land this workaround for leo (with the test in the correct place) and to fix it properly in another bug.

Comment 12

[Rick Waldron [:rwaldron]]

Comment on attachment 790081 [details]
pull-request.html

+1 Julien

(https://github.com/mozilla-b2g/gaia/pull/11522#issuecomment-22650626)

Comment 13

[Rick Waldron [:rwaldron]]

Filed bug 905245 for follow up

Comment 14

[Julien Wajsberg [:julienw] (PTO -> Mar 17)]

Comment on attachment 790081 [details]
pull-request.html

sorry, we still need to fix the test location ;)

Rick, I'll let you handle this while I'm in PTO in the end of this week.

Comment 15

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Comment on attachment 790081 [details]
> pull-request.html
> 
> sorry, we still need to fix the test location ;)
> 
> Rick, I'll let you handle this while I'm in PTO in the end of this week.

I agree that using the blur event for wrapping recipient should be the final solution in v1.2. We did give it a try months ago but it procuded so many side effects just like Corey said in github.

Hi Rick, do you have any suggestion about test location? Not sure moving the assimilate part to thread_ui test is a good idea. Thread_ui test uses mock recipient and it will cause assimilate test failed. If you think it would be better to move these test to thread_ui test
, I can do that with some changes in mock recipient to make test works(But I'm also in PTO until next Mon, feel free to take it if urgent).

Comment 16

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Comment on attachment 790081 [details]
> pull-request.html
> 
> sorry, we still need to fix the test location ;)
> 
> Rick, I'll let you handle this while I'm in PTO in the end of this week.

Yes

Comment 17

[Rick Waldron [:rwaldron]]

(In reply to Steve Chung [:steveck] (PTO until 8/19) from comment #15)
> (In reply to Julien Wajsberg [:julienw] from comment #14)
> > Comment on attachment 790081 [details]
> > pull-request.html
> > 
> > sorry, we still need to fix the test location ;)
> > 
> > Rick, I'll let you handle this while I'm in PTO in the end of this week.
> 
> I agree that using the blur event for wrapping recipient should be the final
> solution in v1.2. We did give it a try months ago but it procuded so many
> side effects just like Corey said in github.
> 
> Hi Rick, do you have any suggestion about test location? Not sure moving the
> assimilate part to thread_ui test is a good idea. Thread_ui test uses mock
> recipient and it will cause assimilate test failed. If you think it would be
> better to move these test to thread_ui test
> , I can do that with some changes in mock recipient to make test works(But
> I'm also in PTO until next Mon, feel free to take it if urgent).

The test is correct where it is, but I think there should be a test in thread_ui_test that uses a spy to check that `this.assimilateRecipients()` has been called (and uses the mock recipients)

Comment 18

[ayman maat :maat]

removing needinfo to me as per comment 7.

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 790081 [details]
pull-request.html

Hi Rick, I updated the unit test including the thread_ui test for verifying assimilateRecipients called and fake picker activity success in thread_ui_integration_test(simulate the behavior that returning a fake recipient form picker). Please have a look again, thanks.

Comment 20

[Julien Wajsberg [:julienw] (PTO -> Mar 17)]

Comment on attachment 790081 [details]
pull-request.html

I take this back since Rick is in holiday ;)

Comment 21

[Julien Wajsberg [:julienw] (PTO -> Mar 17)]

Comment on attachment 790081 [details]
pull-request.html

r=me

great, land it :)

Comment 22

[Steve Chung [:steveck]]

(Travis is busted now... but it should not be related to this patch.)
Landed in master : 2f62e3aaa1e8421e0d8e536d2b61fa69fb1bd983

Thanks, Julien :)

Comment 23

[Al Tsai [:atsai]]

Please help to uplift them to target branches. Thanks in advance.

Comment 24

[Julien Wajsberg [:julienw] (PTO -> Mar 17)]

jhford usually does uplifts to v1-train.

Comment 25

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 2f62e3aaa1e8421e0d8e536d2b61fa69fb1bd983 to:
v1-train: 6f5258fcc22547d45a8d543c7a0533b60f836539

Comment 26

[Steve Chung [:steveck]]

Uplifted to v1.1.0hd ff7ce7971804c196a75d2bfc186e1f9e5b321b13

Comment 27

[Al Tsai [:atsai]]

Thanks for help. Starting the verification work.

Comment 28

[Al Tsai [:atsai]]

Verified w/ v1.1.0hd on helix
Gaia:     b26e980da9819daaf915f09be7eb0b12f69ba24a 
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/b5010b8f604f 
BuildID   20130828042201

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 6f5258fcc22547d45a8d543c7a0533b60f836539