Closed Bug 871433 Opened 7 years ago Closed 7 years ago

WebSMS: Sending the first message to a contact which number contains spaces, the reply will end-up in its own thread (message pane not updated)

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25
blocking-b2g leo+
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- fixed
b2g18 + verified
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: whimboo, Assigned: vicamo)

References

Details

(Whiteboard: [fixed-in-birch] [LeoVB+])

Attachments

(2 files, 2 obsolete files)

No description provided.
If you send a message to a contact imported from Google contacts, the sent message will not be associated with the appropriate contact and remain as number only in the message list. Once the other party answers a new thread gets opened, which then will be used for follow-up messages. But the first sent message is still disconnected from it.
blocking-b2g: --- → leo?
tracking-b2g18: --- → ?
Summary: [SMS] S message to contacts imported from Google → [SMS] Sending a message to contacts imported from Google disconnects message thread
Tested with:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/84f4c17f1605
Gaia   4e7d63a83508caa391c4db164c3f68422d9ca5b6
BuildID 20130509070205
Version 18.0
Would be a nice to have but since the google imported contacts do work (even though in a separate thread for first msg), this is not a blocker.
blocking-b2g: leo? → -
Whiteboard: c=
So I have tested it again and this is not related to contacts imported from Google. It happens with each contact you send the first message to and keep the message pane open.
Summary: [SMS] Sending a message to contacts imported from Google disconnects message thread → [SMS] Sending the first message to a contact, the reply will end-up in its own thread (message pane not updated)
Duplicate of this bug: 865805
Whiteboard: c= → u=fx-os-user c=scravag-sprint-may-20-31 p=1
Whiteboard: u=fx-os-user c=scravag-sprint-may-20-31 p=1 → u=fx-os-user c=scravag-sprint p=1
I can't reproduce anymore on v1-train. If you still can reproduce Henrik, could you give more detailed STRs? Does the contact exist in your contacts? Does it start with 0 or the country code?
Flags: needinfo?(hskupin)
Assignee: nobody → anthony
Yes, the contacts exists. I haven't checked with a not existent contact. Also all number are in the format +49 (XYZ) ZXY, means with the country code as prefix.
Flags: needinfo?(hskupin)
Henrik: Did you try with a recent v1-train? I really can't reproduce :(
Henrik, does the same behaviour occur with a manually entered contact?
The bug likely is about using spaces and brackets in a number, which we didn't support at one point. However I know there were some fixes about this recently.

Henrik, would you please try again your STR with a current v1.1 build ?
Flags: needinfo?(hskupin)
Ah, thank you Julien. Yes, I have brackets and spaces in the number like +49 (xyz) 12 34 56 789. So this is the reason for this problem. When I remove them everything works as expected and no new thread is getting created.
Flags: needinfo?(hskupin)
Summary: [SMS] Sending the first message to a contact, the reply will end-up in its own thread (message pane not updated) → [SMS] Sending the first message to a contact which number contains brackets and spaces, the reply will end-up in its own thread (message pane not updated)
Note to Rik who will investigate: very recently, we changed how we thread: we switched from using the number as a key to using thread id given by gecko.

So I don't know if this bug happens in both situations, but if it does, this will be for different reasons in different places.
Can you check this still happens now that Bug 876774 landed ? I suspect this was caused by the same problem.
No, I can't test this at the moment due to a new regression which has most likely be introduced by that landing. I filed this as bug 877718. Once that has been fixed I will retry here.
It still happens even after bug 876774 landed.
And it seems to only be a problem when a number contains spaces. Brackets seem to not cause this issue.
Summary: [SMS] Sending the first message to a contact which number contains brackets and spaces, the reply will end-up in its own thread (message pane not updated) → [SMS] Sending the first message to a contact which number contains spaces, the reply will end-up in its own thread (message pane not updated)
Attachment #757900 - Flags: review?(gnarf37)
Comment on attachment 757900 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10182

Discussed some changes to tests on IRC - I think this bug might already be fixed by the gecko change, but I really still want to land this SimplePhone stuff - it was a todo in the code, perhaps we should just flip this bug over to that?

Please ask for another review when the pull is updated / we figure out what happened :)
Attachment #757900 - Flags: review?(gnarf37) → review-
I'm not sure we should include the SimplePhone stuff now. I think this was a possibility a few months ago, but now we've taken the path of making Gecko doing all the sanitization stuff.

I really believe this bug is is a gecko issue.
Component: Gaia::SMS → DOM: Device Interfaces
Product: Boot2Gecko → Core
Whiteboard: u=fx-os-user c=scravag-sprint p=1
Assignee: anthony → vyang
Summary: [SMS] Sending the first message to a contact which number contains spaces, the reply will end-up in its own thread (message pane not updated) → WebSMS: Sending the first message to a contact which number contains spaces, the reply will end-up in its own thread (message pane not updated)
Attached patch patch (obsolete) — Splinter Review
Normalize target phone number before searcing for a participant record.  Test cases included as well.
Attachment #757900 - Attachment is obsolete: true
Attachment #770045 - Flags: review?(gene.lian)
Comment on attachment 770045 [details] [diff] [review]
patch

Review of attachment 770045 [details] [diff] [review]:
-----------------------------------------------------------------

Nice patch! I cannot find any defect at all (although I hope to).

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +824,5 @@
>      // Two types of input number to match here, international(+886987654321),
>      // and local(0987654321) types. The "nationalNumber" parsed from
>      // phonenumberutils will be "987654321" in this case.
>  
> +    // Bug 871433 - normalize address before searcing for participant record.

Why do you put the bug description here since you're solving this issue?

Btw, s/searcing/searching/

::: dom/mobilemessage/tests/marionette/test_phone_number_normalization.js
@@ +77,5 @@
> +      return;
> +    }
> +
> +    let request = mozSms.delete(message.id);
> +    request.onsuccess = deleteAll.bind(null, messages);

Cool! Good to learn another advanced coding skill again. :)

@@ +94,5 @@
> +    let sentMessage = event.target.result;
> +
> +    mozSms.onreceived = function onreceived(event) {
> +      let receivedMessage = event.message;
> +      is(sentMessage.threadId, receivedMessage.threadId, "message threadIds");

s/messages threadIds/message threadIds are supposed to be matched/

@@ +129,5 @@
> +    return;
> +  }
> +
> +  SpecialPowers.removePermission("sms", document);
> +  SpecialPowers.clearUserPref("dom.sms.enabled");

Not related to this bug. We have some lines in other files:

  SpecialPowers.clearUserPref("dom.sms.enabled", false);

which is wrong (due to Bug 883798) but is working. Not sure if we could have chance to correct them in this patch as well? Note that b2g18 is also affected due to Bug 885652. :P
Attachment #770045 - Flags: review?(gene.lian) → review+
(In reply to Gene Lian [:gene] from comment #21)
> which is wrong (due to Bug 883798) but is working. Not sure if we could have
> chance to correct them in this patch as well? Note that b2g18 is also
> affected due to Bug 885652. :P
                  ^^^^^^^^^^ Sorry I mean Bug 887815.
(In reply to Gene Lian [:gene] from comment #21)
> Not related to this bug. We have some lines in other files:
> 
>   SpecialPowers.clearUserPref("dom.sms.enabled", false);
> 
> which is wrong (due to Bug 883798) but is working. Not sure if we could have
> chance to correct them in this patch as well? Note that b2g18 is also
> affected due to Bug 885652. :P

I can fix all of them in this patch, but it's not going to be uplifted to b2g18 :(
Attached patch patch : v2Splinter Review
Attachment #770045 - Attachment is obsolete: true
Attachment #770096 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/249e3e1c6a07
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Leo+ bug 885280 needs changes from this bug.
Blocks: 885280
blocking-b2g: - → leo?
Triage - blocking a blocker, leo+ing
blocking-b2g: leo? → leo+
Attached patch b2g18 patchSplinter Review
Whiteboard: [fixed-in-birch] → [fixed-in-birch] [LeoVB+]
Works great on Unagi with the latest 1.1 nightly.
You need to log in before you can comment on or make changes to this bug.