851032 - [SMS] Receiving a new message it's not properly threaded

Status: VERIFIED FIXED

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

Description

[Borja Salguero [:borjasalguero]]

After landing https://bugzilla.mozilla.org/show_bug.cgi?id=833291, the ID for a thread has changed (previously was the international number, currently it's a regular ID), so given a new received message it's impossible to know the right thread to append it in the UI (due to the ID it's not exposed to the API). 
Gecko change it's tracked here https://bugzilla.mozilla.org/show_bug.cgi?id=850127, and we need to modify Gaia to take this new ID into account.

Comment 1

[Daniel Coloma:dcoloma]

Ok, so there are only two options here, either we block on this one and fix bug 851032 and bug 850127 or we back-out 833291. 

Vicamo, you are the author of 850127 and 833291, what do you think is safest?

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Daniel Coloma:dcoloma from comment #1)
> Ok, so there are only two options here, either we block on this one and fix
> bug 851032 and bug 850127 or we back-out 833291. 
> 
> Vicamo, you are the author of 850127 and 833291, what do you think is safest?

Of course land bug 850127. When we're supporting MMS in message app, the original way of fetching messages of a specific thread will be totally unusable because now we may have multiple recipients in one message. Please see also bug 849739, which will have another cursor-based GetThreads() API, which provides thread id, participants for MMS in v1.0. All these changes are based on previous discussions of Borja and me.

Comment 3

[Daniel Coloma:dcoloma]

For this issue, I am only interested in v1.0.1 so the impact on MMS is irrelevant to me. Just to be sure, without taking into account MMS are you still suggesting with landing bug 850127 and bug 851032.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Daniel Coloma:dcoloma from comment #3)
> For this issue, I am only interested in v1.0.1 so the impact on MMS is
> irrelevant to me. Just to be sure, without taking into account MMS are you
> still suggesting with landing bug 850127 and bug 851032.

Yes, that's why this patch contains only threadId in SmsMessage object, not including SmsFilter. A full solution for both SMS and MMS will be available in bug 849739 and bug 847760. 

Actually, I think we might also need threadId in SmsThreadListItem, the return data type of GetThreadList(). Borja, what do you think?

Comment 5

[Michael Vines [:m1] [:evilmachines]]

(Daniel, I differ the tef+ call to you since you nominated it yourself)

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Since it's now a tef+, I'll do some more cleanups in the WIP patch and request for review asap.

Comment 7

[Borja Salguero [:borjasalguero]]

Vicamo, could you tell us the name of the new value exposed? 'threadId'??? I would like to know asap for making all changes in Gaia while you are creating the patch in Gecko (I have to modify tests, mockups... with this new param). 

Regarding your question for sure! I need thread ID in the SMS and in the list of Threads.

Thanks!

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Borja Salguero [:borjasalguero] from comment #7)
> Vicamo, could you tell us the name of the new value exposed? 'threadId'??? I
> would like to know asap for making all changes in Gaia while you are
> creating the patch in Gecko (I have to modify tests, mockups... with this
> new param). 

In SmsThreadListItem, its name is "id", in SmsMessage, it's "threadId". I'll have the same naming in both bug 850127 and bug 849739 (need updates). Thank you :)

Comment 9

[Borja Salguero [:borjasalguero]]

One more thing. Are you gonna modify 'getMessages' for requesting ALL MESSAGES given a ThreadID? Because currently we are using an Object like:
{
  senderOrReceiver: '436797',
  body: 'Sending :)',
  timestamp: new Date(Date.now() - 172800000),
  unreadCount: 0
}

And I need 'senderOrReceiver' for creating the filter for getMessages. I would need this param in order to  request 'getMessages' for keeping the already implemented Gaia code working. Something like:
{
  id: 20123
  senderOrReceiver: '436797',
  body: 'Sending :)',
  timestamp: new Date(Date.now() - 172800000),
  unreadCount: 0
}

On the other hand, are we going to modify 'getMessages' for requesting ALL MESSAGES given a ThreadId? Is there any bug for making this change? Thanks!

Comment 10

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Comment 11

[Borja Salguero [:borjasalguero]]

Vicamo, I've created a patch for the 'ThreadID' issue. Could you test it with your Gecko patch? Thanks!

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 726690 [details]
Pull Request

Verified :)

Comment 13

[Borja Salguero [:borjasalguero]]

Comment on attachment 726690 [details]
Pull Request

Julien, for testing it you have to generate Gecko with Vicamo Patch. Both patches will be landed at the same time once everything have R+. Thanks!

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 726690 [details]
Pull Request

Borja, I'm a little bit confused about the review flags. It seems you want Julien's review instead because you have r=julienw in the commit summary. I've placed a feedback+. Maybe it's enough. :)

Comment 15

[Julien Wajsberg [:julienw]]

(In reply to Borja Salguero [:borjasalguero] from comment #0)
> After landing https://bugzilla.mozilla.org/show_bug.cgi?id=833291, the ID
> for a thread has changed (previously was the international number, currently
> it's a regular ID), so given a new received message it's impossible to know
> the right thread to append it in the UI. 

Sorry, I don't understand why.

I mean, I really like the idea of having a thread id, but I don't understand why it's broken in 1.0.1, and therefore why this needs to be tef+. Am I missing something ?

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 726690 [details]
Pull Request

comments on the PR.

Comment 17

[Borja Salguero [:borjasalguero]]

Sorry Julien, I should add more info to this bug. The problem here is that, for analyzing if some received sms belongs to the current thread, we were using the 'normalized phone number'. However, before all latest changes in Gecko, we can't use this method due to we are not going to use this criteria anymore. Gecko will give us a 'thread_id' which will be the unique identifier for knowing if one new message belogs to which thread. 

The patch was landed in Gecko B2G-18, so if you send a SMS to a NON-normalized number (for example to 612123123) and, WITHOUT going back to thread list, you receive a response (from [phone] in this case), the message is not threaded properly.

If you have any doubt dont hesitate to ask me or Vicamo. I will take a look to your comments. Thanks.

Comment 18

[Julien Wajsberg [:julienw]]

I'm perfectly happy with these changes in master or v1-train, what I don't like is having this in 1.0.1, and why it's needed.

Comment 19

[Borja Salguero [:borjasalguero]]

Im gonna try to explain again. The change in Gecko it's **already** landed in B2g-18, what it means that SMS App it's **not** working properly in v1.0.1. That's why this patch it's needed, because currently it's not working (this is the reason of having TEF+). 

In fact this patch should be landed at the same time than Gecko patches.

Comment 20

[Julien Wajsberg [:julienw]]

Bug 833291 comment 93 implies that everything works correctly in 1.0.1 but I just verified and indeed the following STR doesn't work correctly :

* send an SMS to a local number
* stay on the newly-created thread while your correspondend replies
* you get the reply, but it isn't shown in the current thread whereas it should.
* you have to go to the thread list screen and show the thread again to display it.

Comment 21

[Borja Salguero [:borjasalguero]]

Yep. Without both patches (Gecko & Gaia) this is the result. For testing it build your own Gecko with Vicamo's patch and use Gaia with my patch. Everything should work.

Comment 22

[Julien Wajsberg [:julienw]]

borja, you've seen that I already made a review a few days ago, right ? :)

Comment 23

[Borja Salguero [:borjasalguero]]

Yep, I'll udpate the PR with your suggestions asap ;)

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

Hi all, bug 850127 is ready. I'll go ahead to prepare patches for b2g18_v1_0_1 :)

Comment 25

[Borja Salguero [:borjasalguero]]

Im gonna take a look to Julien suggestions today!

Comment 26

[Borja Salguero [:borjasalguero]]

I will test it again based on Julien's comment.

Comment 27

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Comment 28

[Borja Salguero [:borjasalguero]]

Vicamo, Julien, could you test it? 

@Julien: I've changed the proposal in order to get a cleaner code. Please take a look.

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 732346 [details]
Pull Request

Verified! Sending & receiving both international & national number messages results in only one thread and Gaia presents them well immediately.

Comment 31

[Julien Wajsberg [:julienw]]

Comment on attachment 732346 [details]
Pull Request

r=me

let's land this together with Bug 850127.

Comment 32

[Julien Wajsberg [:julienw]]

master: 86a79ba3fdfb0eb6f593e34887fa89e3553d1a66

Comment 33

[Julien Wajsberg [:julienw]]

v1-train : 2f64311e0f23b85c29b20be9502cbdeaee8342ae
v1.0.1 : f6a48343e3b5aaed19688ab50b90f1605a519365

Comment 34

[Julien Wajsberg [:julienw]]

should be backed out of all branches because Bug 850127 got backed out.

Right now github seems busted, will do it later if nobody does it before.

Comment 35

[Julien Wajsberg [:julienw]]

backed out master: 3afa858a84f422b3a631e4e7b81cc7cad428ef2b
backed out v1-train: 61aa777dba52a54d5e7c0040f1f4a4a27b4d4aa9
backed out v1.0.1: 28c144fa7e8061ee6bcbbb47ca911ef33b87814b

I think we should land on master only when Bug 850127 lands on gecko b2g18 because that's the branch we usually use with master.

Comment 37

[Staś Małolepszy :stas]

Julien:  after the backout, what's the plan to fix this on v1.0.1?  I still see the initial problem (incoming messages are not threaded correctly) in today's v1.0.1 build.

Comment 38

[Julien Wajsberg [:julienw]]

The plan is to do land and uplift now as Bug 850127 just landed on the branches ;)

I'm on it now.

Comment 39

[Julien Wajsberg [:julienw]]

I'm testing this before relanding but I see a bug that I thought was fixed.

Comment 40

[Julien Wajsberg [:julienw]]

ok, no bug, it was my fault.

Relanded all :

master: 8292d09e18cf82364f07fe0948176245d68630cb
v1-train: 8bab6b6803e046522a2b4c5ef9f81518895e1598
v1.0.1: f2546c5a6a6175aa9b661d5110f5ea00d19ef48a

Comment 41

[Bartosz Piec]

Attachment: Messages view after the changes

This is how my messages view looks after getting the latest changes. I saw that this messages were some testing stuff (https://github.com/mozilla-b2g/gaia/pull/8716/files#L2R2) and I cannot receive messages anymore and setting one of message as read doesn't do anything (it is still unread after the next start of Messages).

Didn't the patch break something?

Comment 42

[Julien Wajsberg [:julienw]]

Right, looks like you get the sms_mock's data.

I'm not sure this bug is the culprit though, would you please file another bug, we'll take a look tomorrow.

Comment 43

[Bartosz Piec]

Done, bug 860049.

Comment 44

[Borja Salguero [:borjasalguero]]

Hi Bartosz. Are u testing in the device? or emulator?

Comment 45

[Carlos Martínez Toral [:carlosmartinez]]

Tested on ikura device and working fine with: 

Gecko 
Gaia   715de4016b63d417c8fdc20b1ae8b15b666e1afb
BuildID 20130416092518
Version 18.0