918970 - [B2G][SMS] Conversation history does not appear when sending a SMS through the contacts app

Status: RESOLVED FIXED

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

Description

[KTucker [:KTucker][Inactive 3/4/2016]]

Description:
If the user selects to send a SMS from a contact's details page, the conversation history with that person will not appear. 

Repro Steps:
1)  Updated to Buri Build ID: 20130916040205
2)  Send a SMS to a saved contact and then have that contact send a reply SMS.
3)  Tap on the "Contacts" icon to open up Contacts.
4)  Tap on the "Contact's Name" that the user sent a sms to earlier.
5)  Tap on the "SMS" icon by the contact's number on their details page.

Actual:
The conversation history with that contact does not appear.

Expected:
The conversation history with that contact appears.

Environmental Variables
Build ID: 20130916040205
Gecko: http://hg.mozilla.org/mozilla-central/rev/c4bcef90cef9
Gaia: a0079597d510ce8ea0b9cbb02c506030510b9eeb
Platform Version: 26.0a1

Notes:
Repro frequency: (100%)
Test Suite Name: (SMS)
UCID: (messages-001)
Link to failed test case: https://moztrap.mozilla.org/manage/case/4986/
See attached: (logcat)

Comment 1

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: logcat

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

On Android, the conversation history appears when sending a sms through the contact's detail page.

Comment 3

[Julien Wajsberg [:julienw]]

I've seen a similar bug this week and I even think I commented there but I can't find it ;)

So, this behaves like actually specified (NI Ayman who will confirm this).

I understand the need, but if we's use the thread view, there would be no way to add other recipients from there. That's probably why it's been specified like this.

The only possible thing that I see is that, from a thread view, we should have a way to switch to a "new message" view that will let you add other recipients. Then the "send sms" activity could display the thread view instead of the "new message" view.

Comment 4

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> I've seen a similar bug this week and I even think I commented there but I
> can't find it ;)
> 
> So, this behaves like actually specified (NI Ayman who will confirm this).

no this is not behaving as specified referencing
wireframe pack: HTML5_Dialer_Contacts_20120810_R2S1_V6.0 
(blow the dust off it)
page: 24
annotation: 04

04 call to action to send an SMS to the associated number
upon tap
if existing SMS conversation already exists:
	- go to existing message thread. refer to wireframe ‘SMS : message thread’
if there is no existing SMS conversation already existing:
	- go to new message composer with the ‘TO’ field prepopulated with the contacts name. refer to wireframe ‘SMS : new message composer’

> 
> I understand the need, but if we's use the thread view, there would be no
> way to add other recipients from there. That's probably why it's been
> specified like this.

There was a discussion (ages ago) and decision made that if the user was starting to send a message directly from a contact detail card the probability was that sending to a single contact. If they were sending a group message the probability was that they would open a new message thread..and that was a good enough basis to maintain the behaviour specified in HTML5_Dialer_Contacts_20120810_R2S1_V6.0 for the time being as it also equalised some of our benchmarks behaviour at that time.

> 
> The only possible thing that I see is that, from a thread view, we should
> have a way to switch to a "new message" view that will let you add other
> recipients. 

I agree. it would bring greater flexibility of use to bring this functionality into the app. It would be something i would certainly support form a UX PoV

> Then the "send sms" activity could display the thread view
> instead of the "new message" view.

Comment 5

[Julien Wajsberg [:julienw]]

Then I was wrong, thanks Ayman !

It's always difficult to refer to another app's wireframe.

Nominating to 1.3, there's nothing urgent here.

Was gonna add [good-first-bug] but maybe not, as we need to request the API, this is not trivial for a newcomer.

Comment 6

[Augustin Trancart [:autra]]

I don't know if this is the right place to talk about UX ideas, but it could also be nice to have this functionality when taping the "new message" button. Something like :
- Tap "new message" button
- add a contact. Then the current thread with this contact is displayed
- If the user adds another contact, we switch from thread view to new message view.

That's how Bada behaves for instance  (it might be dead, but it had great ideas from the UX point of view).
This implies some convergence between the thread and the new message views. The "Thread" and "new message" views were actually the same in Bada. There was always a + to add people to the thread. The title was the person's name (as it is in our "thread view") if only one recipient or something like "Mum and 12 others" if several. It was always possible to add/remove people from recipients list. Then, you actually jumped back and forth between a thread view if there was only one person in the recipient list, or a new message view if several (that's where convergence in displaying names is needed in my opinion). 

I liked this behaviour. In this case, when several persons in the recipient list, there are several possibilities:
- Keep a separate thread for each combination of recipients. Bada behaved this way, but this is a bit awkward : when people answer, the answer actually belongs to one-to-one threads, so you have only your messages in the multi-recipient thread, but not in the one-to-one thread, which makes things difficult to follow. On the other hand, "reply-all" actions were easy.
- fill each one-to-one thread with the sent messages, and don't record the one-to-many thread, so that any conversation that occurs after are easy to follow. The dowside is that you can't easily do a "reply to all".
- both :-) this can gives great flexibilities. The one-to-many threads still contains only the sent messages of course (as it is not really possible to tell if a sms belongs to a thread or not), whereas the one-to-one contains both sent and received messages from this particular contact. This allows a "reply-all" very quickly, and to correctly follow any individual thread that might occur.

If only sms had a cc-list like emails ;-)

Sorry if it is not the appropriate place for these ideas.

Anyway, regardless of that, I can start working on what julienw said. 

> Was gonna add [good-first-bug] but maybe not, as we need to request the API, this is not trivial for a newcomer.
I am a newcomer in gaia :-(. Do you think I can try it anyway?

Comment 7

[Julien Wajsberg [:julienw]]

I agree with most of what you said here. However Ayman is a bit busy right now to help on the UX perspective, and Victorial on the Visual side, so we should probably stick to what's been specified for now.

You can try it, but don't hesitate to ask here or on IRC if you have any problem !

Comment 9

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

QAWANTED, 
can QA confirm the current behavior on the phone?
after a message is sent to a contact from the contacts app, it seems like the conversation history will be lost.wonder what happens after a message is responded back to the phone?

Thanks

Comment 10

[Julien Wajsberg [:julienw]]

Hey,

This bug is not about what happens after sending but about what happens before sending.

The goal of this bug is that the thread history is loaded for this contact in this situation, if a thread exists.

Ayman said in comment 4 it is what was specified initially and that the current implementation is therefore incorrect according to the spec.

Comment 11

[Julien Wajsberg [:julienw]]

I don't think we need qawanted then, this was only a misunderstanding :)

Comment 12

[Julien Wajsberg [:julienw]]

Removing the nomination because it's a mentored bug.

Comment 13

[Augustin Trancart [:autra]]

Attachment: Draft 1 : Add an entry in the menu

Epic come back : I'm not dead! :-)

Here a first (very) little draft. Just the entry in the menu actually.

I'm now wondering what is the best way to redirect to a new message view with one tel: do I create a MozActivity like in the contact detail page? Or do I add logic for it on function onHashChange in message_manager.js? 

Thank you very much for your answer

Comment 14

[Rick Waldron [:rwaldron]]

(In reply to Augustin Trancart [:autra] from comment #13)
> Created attachment 8350906 [details] [diff] [review]
> Draft 1 : Add an entry in the menu
> 
> Epic come back : I'm not dead! :-)
> 
> Here a first (very) little draft. Just the entry in the menu actually.
> 

I'm not sure I understand this addition—there is already the "+" for adding recipients from Contacts.

> I'm now wondering what is the best way to redirect to a new message view
> with one tel: 

This already happens—I thought the goal was to instead direct to an existing conversation thread, if one exists?

> do I create a MozActivity like in the contact detail page? Or
> do I add logic for it on function onHashChange in message_manager.js? 

There is already an activity handler, you want to change its behaviour so that when the activity completes and the activity handler receives the number of the selected contact, it can then use that number to determine which path to take: 

1. If a thread exists with this number in the participants, call ActivityHandler.toView({ threadId: ... })
2. Else, the current behaviour


For #1, you'll want to start here: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/activity_handler.js#L61 and look through all the current threads with Threads.forEach(...) find a thread object with `number` in its participants array and call ActivityHandler.toView({ threadId: thread.id }).

You'll likely want to set a state bit inside Threads.forEach (if a thread is found) eg. "isComplete", that can be checked after Threads.forEach completes, where if "false": continue with the current behaviour, if "true": return immediately.

Take a look at the existing tests for the "new" handler, you'll need to add additional tests that assert the new alternate behaviour while not changing the existing behaviour.

Please r? me when ready!

Comment 15

[Augustin Trancart [:autra]]

(In reply to Rick Waldron [:rwaldron] from comment #14)
> I'm not sure I understand this addition—there is already the "+" for adding
> recipients from Contacts.

Are you talking about the + from the new message view? What I want to do is to give a way to switch from thread view to new message view (by clicking this button). I didn't realize that it would also be displayed in new message view though. So I guess this addition still makes sense, but need to be hidden somehow for new messages and only be displayed for thread view. I didn't want to add another button in the title bar next to the name, because there's already 2 there.

That said, I think I misunderstood something. I thought there were two steps : 
- Redirect to thread view (if it exists) instead of new message view from contact app.
- Add a "add recipient button" (or something similar) to thread view (which would redirect to new message view).

I was trying to do the latter with my patch, but now I realize from comments #7 and #10 that this is out of scope right now. Do you confirm that? This bug is only about what you just described, right?

Thanks for the explanation :-)

Comment 16

[Julien Wajsberg [:julienw]]

Sorry, I was in holiday in the end of the year.

I need a little more than 5 minutes to read the UX design again, so I'm defering more comment to Monday :)

Comment 17

[Julien Wajsberg [:julienw]]

So, in this bug, I think we should focus on the original issue only, and file a new bug for any more work. The 2 issues can be separate, and actually, even this single issue has edge cases so better do it separately. And lastly, Augustin, since this is your first bug, better make it as simple as possible :) I'm sure you'd prefer closing 2 bugs instead of only one ;)

Let's then clarify the goal of this bug:
* When receiving an activity "new", instead of launching the "new message" composer, we need to launch the thread.

This will be done in activity_handler.js, in the "toView" method. If you look closely, you'll see that if there is a "threadId" in parameter, then we launch the thread; if there is no "threadId" then we load the "new message" view.
So, at this place, you need to find the threadId for a specific contact.

So now, finding the correct threadId is where it gets tricky. The threading is done in Gecko so I think the best way is to use the Gecko API to do it. See [1] for the reference on MozSmsFilter. I'd call MessageManager.getMessages with a MozSmsFilter with the "numbers" property containing the number, a "each" callback storing the first message and returning false, and a "end" callback to see if we had at least one message. See [2] for more information about MessageManager.getMessages.

Don't forget to change and add unit tests (see [3] for more information on those).

[1] https://developer.mozilla.org/en-US/docs/Web/API/MozSmsFilter
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/message_manager.js#L417-L430
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Boot_to_Gecko/Gaia_Unit_Tests

I'll add a needinfo on Ayman for another question that I have.

Comment 18

[Julien Wajsberg [:julienw]]

Ayman, the current behavior has something useful: when the "new message" view is displayed, and the user is sending a SMS from Contacts, then the contact is added to the recipient panel. It means that any recipient that was previously there is still there after the activity handling. I think this behaviour is really neat, and maybe we can try to retain some of this behavior here.

How about:
* if the "new message" view is currently displayed and has data
  + we're currently asking if we want to save a draft (I think... I haven't tried, but we should)
  + in this menu, we could add another item "add the contact to the current view"
  + the item "save as draft" could be renamed "save as draft and send a new message"

* if another view is displayed, then proceed like the spec in comment 4

Comment 19

[Augustin Trancart [:autra]]

Ok things are clearer now. Thanks a lot.

(In reply to Julien Wajsberg [:julienw] from comment #17)
> Let's then clarify the goal of this bug:
> * When receiving an activity "new", instead of launching the "new message"
> composer, we need to launch the thread.

Roger that!

> This will be done in activity_handler.js, in the "toView" method. 

Actually I was thinking of doing that in the 'new' handler, and calling the toView method with an object containing the threadId if I find one. I thought it would limit the scope of what I'm doing to the handling of activities, and allow toView to stay multi-purpose if unmodified. What do you think?


> So now, finding the correct threadId is where it gets tricky. The threading
> is done in Gecko so I think the best way is to use the Gecko API to do it.

Cannot it be done by calling Threads.forEach ?


Thanks a lot! I now have a clearer idea of how things are working in this app, so hopefully I will be faster now.

Comment 20

[Julien Wajsberg [:julienw]]

(In reply to Augustin Trancart [:autra] from comment #19)
> Ok things are clearer now. Thanks a lot.
> 
> (In reply to Julien Wajsberg [:julienw] from comment #17)
> > Let's then clarify the goal of this bug:
> > * When receiving an activity "new", instead of launching the "new message"
> > composer, we need to launch the thread.
> 
> Roger that!
> 
> > This will be done in activity_handler.js, in the "toView" method. 
> 
> Actually I was thinking of doing that in the 'new' handler, and calling the
> toView method with an object containing the threadId if I find one. I
> thought it would limit the scope of what I'm doing to the handling of
> activities, and allow toView to stay multi-purpose if unmodified. What do
> you think?

yep, why not, this works for me as well :)

> 
> 
> > So now, finding the correct threadId is where it gets tricky. The threading
> > is done in Gecko so I think the best way is to use the Gecko API to do it.
> 
> Cannot it be done by calling Threads.forEach ?

The problem is that when the activity is called and if the application was not launched, Threads is not populated yet. And we don't want to load all the data in memory just to be able to search it. Last but not least, we can have thousands of threads, and iterating using forEach is not the right way to be efficient :) We have a database, let's leverage it!


Also, don't forget to write some tests!

Comment 21

[Augustin Trancart [:autra]]

Attachment: patch.diff

So here is the diff for activity_handler.js. I have been very slow, because I had everything to discover. But I've learnt a lot in the process (especially to run my own gaia in the emulator and to debug it with firefox, cool stuff)!

I've started to write a test, but I need your advice on how to do it (that's why I put needInfo instead of review flag). I can't stub MessageManager.getMessages, because I wouldn't test anything this way.
So I thought I could stub _mozMobileMessage.getMessages, but don't really know how. Is there already a kind of MockCursor for this purpose?

BTW should I clean needinfo to ayman?

Comment 22

[Julien Wajsberg [:julienw]]

Sorry, I've been really busy lately (and I am still).

(In reply to Augustin Trancart [:autra] from comment #21)
> Created attachment 8367969 [details] [diff] [review]
> patch.diff
> 
> So here is the diff for activity_handler.js. I have been very slow, because
> I had everything to discover. But I've learnt a lot in the process
> (especially to run my own gaia in the emulator and to debug it with firefox,
> cool stuff)!

\o/

> 
> I've started to write a test, but I need your advice on how to do it (that's
> why I put needInfo instead of review flag). I can't stub
> MessageManager.getMessages, because I wouldn't test anything this way.
> So I thought I could stub _mozMobileMessage.getMessages, but don't really
> know how. Is there already a kind of MockCursor for this purpose?

I actually think you should stub MessageManager.getMessages, because you're testing activity_handler, so this makes sense.

> 
> BTW should I clean needinfo to ayman?

I need to check if my comment 18 is still valid because we changed this to an "inline" activity recently. Keeping my NI then.

Comment 23

[Augustin Trancart [:autra]]

Attachment: BUG 918970 - redirect to existing thread when sending sms from a contact

Now that I know a bit more about sinon.js, it makes sense to stub getMessages indeed :-)
Got some tests \o/

Comment 24

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16216

Comment 25

[Julien Wajsberg [:julienw]]

Comment on attachment 8374860 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16216

I left some comments on github, please request review again once you're ready :)

Vicamo, in this bug, we need to find a thread from a phone number. The trick is that the phone number is not necessarily in a international format, it could really be in national format. Do you have an idea about how we should retrieve the thread id ?

The current WIP code is at [1] in case you're curious.

Thanks!

[1] https://github.com/autra/gaia/blob/bug918970/apps/sms/js/activity_handler.js#L86-L105

Comment 26

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

(In reply to Julien Wajsberg [:julienw] from comment #25)
> Vicamo, in this bug, we need to find a thread from a phone number. The trick
> is that the phone number is not necessarily in a international format, it
> could really be in national format. Do you have an idea about how we should
> retrieve the thread id ?
> 
> The current WIP code is at [1] in case you're curious.
> 
> Thanks!
> 
> [1]
> https://github.com/autra/gaia/blob/bug918970/apps/sms/js/activity_handler.
> js#L86-L105

That's exactly what comes to my mind after reading the question above.  However, we should not directly compare phone numbers by their literal equality because of the same worry you have in that question -- numbers do not always come in the same format.  Phone number comparison should always be done by phonenumberutils.  Except for this, I think other bits in that PR are quite reasonable.

Comment 27

[Julien Wajsberg [:julienw]]

Vicamo, yes, so how can we move forward here? How can we find the correct thread? Should we resume work on bug 903431?

Comment 28

[Augustin Trancart [:autra]]

Julien, I've updated my push request. 

I've found Utils.probablyMatches to compare phone numbers. Not sure if it what Vicamo meant by phonenumberutils...

Comment 29

[Julien Wajsberg [:julienw]]

Comment on attachment 8374860 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16216

Removing the review flag for now, we need to find solutions to the various issues.

Comment 30

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

(In reply to Julien Wajsberg [:julienw] from comment #27)
> Vicamo, yes, so how can we move forward here? How can we find the correct
> thread? Should we resume work on bug 903431?

Hi, I found myself lost in the context.  Why do we need to compare phone numbers in order to "find a thread from a phone number"?

The messages retrieved from |getMessages()| MUST somehow contains that needle phone number in the owning thread's participant list, or it's a bug.  The real problem becomes that you might get a list of messages that actually belongs to three threads Ta, Tb, Tc, which Ta has [needle, Pa, Pb] in its participant addresses list, Tb has [Pa, needle, Pc, Pd], Tc has [Pe, needle], etc.  However, we always have DISABLE_MMS_GROUPING_FOR_RECEIVING set to true, so it cuts the problem into half but that still exists for outgoing MMS messages.

If you only want to find out one specific thread that has only that needle contact and you in the thread participant list, you should get the thread Id from the message which:

  1) It's a SMS message:

    SMS message threads can only have one thread participant, so it
    must be the case,

  2) Or, (it's a MMS message and) |message.receivers.length| == 1:

    This means the MMS thread is only between you two,

For all other cases, the message threads contain more people than [needle, you].  But for these cases, what should Gaia do remains a question, right?

Comment 31

[Julien Wajsberg [:julienw]]

Vicamo, sorry, I should have made the question clearer with my needinfo :)

We're getting the needle from a contact; that means the needle can be in a national format (eg: 0123456789) instead of international ([phone]).

Does the "numbers" filter work when we use a number with a national format? I would like to avoid getting all messages just to find one thread...

The other explanations you've given all make sense, thanks!

Comment 32

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

(In reply to Julien Wajsberg [:julienw] from comment #31)
> Vicamo, sorry, I should have made the question clearer with my needinfo :)
> We're getting the needle from a contact; that means the needle can be in a
> national format (eg: 0123456789) instead of international ([phone]).
> 
> Does the "numbers" filter work when we use a number with a national format?

Sure.  But the messages you get from the call are the ones "contain" the contact in the participant list, not "contain only" the contact.  There could be many threads, not just one.  But the summary of this bug looks like you want a thread to send SMS, and it's reasonable that thread should be the one that the composing message belongs to, which follows that thread should contains only the phone owner and that contact.

> I would like to avoid getting all messages just to find one thread...

Two things here.  First, |getMessages| call doesn't have a "limit" attribute, so it does iterate through all retrieved messages in the background even without an explicit call to |cursor.continue();|.  MobileMessageDatabaseService prepares all the messages you may need automatically.  But, yes, they'll never leave MobileMessageDatabaseService until a |cursor.continue();| call.

Second, it sounds like you may want to pass a filter object to |getThreads()| call instead?

> The other explanations you've given all make sense, thanks!

Comment 33

[Julien Wajsberg [:julienw]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #32)
> (In reply to Julien Wajsberg [:julienw] from comment #31)
> > Vicamo, sorry, I should have made the question clearer with my needinfo :)
> > We're getting the needle from a contact; that means the needle can be in a
> > national format (eg: 0123456789) instead of international ([phone]).
> > 
> > Does the "numbers" filter work when we use a number with a national format?
> 
> Sure.

great, this is what I didn't really know!

> But the messages you get from the call are the ones "contain" the
> contact in the participant list, not "contain only" the contact.  There
> could be many threads, not just one.  But the summary of this bug looks like
> you want a thread to send SMS, and it's reasonable that thread should be the
> one that the composing message belongs to, which follows that thread should
> contains only the phone owner and that contact.

Yep that advice was a really good advice :)

> 
> > I would like to avoid getting all messages just to find one thread...
> 
> Two things here.  First, |getMessages| call doesn't have a "limit"
> attribute, so it does iterate through all retrieved messages in the
> background even without an explicit call to |cursor.continue();|. 
> MobileMessageDatabaseService prepares all the messages you may need
> automatically.  But, yes, they'll never leave MobileMessageDatabaseService
> until a |cursor.continue();| call.

With the filter I'm confident it won't be too big.

> 
> Second, it sounds like you may want to pass a filter object to
> |getThreads()| call instead?

In this case this is not really necessary, even if it would still be nice ;)

Thanks a lot!

Comment 34

[Augustin Trancart [:autra]]

Back from holidays :-)

So if I understand well :

- We can pass numbers in national format to the filter. That means that probablyMatches will work here right? (I mean it will actually work, and we need to use it, because we can receive numbers in a different format we asked for).

-I think I found a workaround for the problem when I send a sms to myself : the correct test is |(number is in the receivers AND receivers has only one element) OR (number is the sender AND sender != receiver[0])|. 
The first part deals with messages I sent (if this contact is only in a multiple receiver thread, this thread is not relevant anyway, so we're good there), useful in the case I never received any sms from this contact.
The second part deals with messages I received. I eliminate the case where the sender and the unique receiver are the same, because that means I sent it to myself.
Do you see any flaws in this reasoning?

- Vicamo seems to imply we can call |getThreads()|. But we can't because the threads are not always loaded right?

I will comment on github with some questions as well.

Comment 35

[Augustin Trancart [:autra]]

And I've just realized that MozSmsMessage and MozMmsMessage have different signatures, and that I need to take that into account in my tests according to the value of message.type.

Comment 36

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

(In reply to Augustin Trancart [:autra] from comment #34)
> Back from holidays :-)
> 
> So if I understand well :
> 
> - We can pass numbers in national format to the filter. That means that
> probablyMatches will work here right? (I mean it will actually work, and we
> need to use it, because we can receive numbers in a different format we
> asked for).
> 
> -I think I found a workaround for the problem when I send a sms to myself :

Please see comment 30 :)

> the correct test is |(number is in the receivers AND receivers has only one
> element) OR (number is the sender AND sender != receiver[0])|. 

Like what you had in the next comment, SmsMessage has only |receiver|, no |receivers|.

> The first part deals with messages I sent (if this contact is only in a
> multiple receiver thread, this thread is not relevant anyway, so we're good
> there), useful in the case I never received any sms from this contact.
> The second part deals with messages I received. I eliminate the case where
> the sender and the unique receiver are the same, because that means I sent
> it to myself.
> Do you see any flaws in this reasoning?

Reasonable, but the last question in comment 30 remains.

> - Vicamo seems to imply we can call |getThreads()|. But we can't because the
> threads are not always loaded right?

I don't understand what you mean here. |getThreads()| is a Gecko API and you can call it whenever you want.

Comment 37

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

(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #33)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #32)
> > MobileMessageDatabaseService prepares all the messages you may need
> > automatically.  But, yes, they'll never leave MobileMessageDatabaseService
> > until a |cursor.continue();| call.
> 
> With the filter I'm confident it won't be too big.

After reading this line, somehow I feel we do need a limit for this.  By "all the messages", I mean all the messages that meets the conditions specified in that filter.  And by default, if no filter was passed in the first place, that means no conditions to be applied, so all messages in that database are to be cached.  That's probably not a good idea.

Comment 38

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #37)
> (In reply to Julien Wajsberg [:julienw] (away until March 24) from comment
> #33)
> > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #32)
> > > MobileMessageDatabaseService prepares all the messages you may need
> > > automatically.  But, yes, they'll never leave MobileMessageDatabaseService
> > > until a |cursor.continue();| call.
> > 
> > With the filter I'm confident it won't be too big.
> 
> After reading this line, somehow I feel we do need a limit for this.  By
> "all the messages", I mean all the messages that meets the conditions
> specified in that filter.  And by default, if no filter was passed in the
> first place, that means no conditions to be applied, so all messages in that
> database are to be cached.  That's probably not a good idea.

Had a check, we cache only message ID. :)

Comment 39

[Augustin Trancart [:autra]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #36)
> Please see comment 30 :)

Yes, I need to separate the logic for sms and mms. Is that what you meant by pointing me to comment 30? Please also see below.
And I discovered flaws in my algorithm anyway. My workaround to find my own numbers does not work :-(. So I need to do things in a bit formal way :

For SMS :
there is 4 cases (R is receiver, S sender, F the number I want to filter with) :
1/ R= me, S=me //messages sent to myself
2/ R=other, S=me 
3/ R=me, S=other
4/ R=other1, S=other2 //impossible case, hopefully :-)

So the condition should be for one message :
|( (R matches F OR S matches F) AND F is not my own number) OR (R matches F AND S matches F AND F is my own number) |

The first part matches sms sent to F or received from F, but we exclude the case where F is myself, otherwise we would get all my sent messages to any recipient when I filter with my own numbers.
The second part is when I sent sms to myself, R, F and S are the same. This exclude every other messages.
Here I have a problem: I need somehow to know my own number. Julien said there was no way of knowing that right now. So we need to fix that beforehand I guess.
My idea of comparing R and S does not work actually.
Actually, it is not the first time we have this issue: bug 880251. Is the msisdn exposed to gaia?

If not (or if the SIM does not expose it at all), I think we can only check the receiver. After all, it will work for all cases, except when I never received any texts from this contact. This is incomplete, but does not introduce bugs.

For MMS:

> Reasonable, but the last question in comment 30 remains.

I tend to exclude every cases where Rs.size > 1 anyway. We want candidates to send messages to a contact. So we want to select the 1-to-1 thread if it exists. It would be very confusing for the user if he wants to send a MMS to A, and is brought to thread with A,B,C. We want to go to the thread between A and the user only, if it exists. Is it ok for you if gaia behaves this way?

If we do that, we actually fall back to the same logic as before, by excluding all the cases where recipients.length > 1:
So We do still have the problem about my own number.

Thanks for your help!

>I don't understand what you mean here. |getThreads()| is a Gecko API and you can call it whenever you want.
Forget about it :-) I didn't realize we don't have a filter object for |getThreads()|.

Comment 40

[Augustin Trancart [:autra]]

Actually, what bugs me is that gaia (since 1.2 at least) has already the correct behaviour after you send a messages : you are redirected to the right thread without any bugs (I tried to send messages to myself from the contact page, from a thread, from the new message view, with number in national format or international format etc). Why couldn't we do that if it is already done afterwards? I need to inspect code for that feature.

Comment 41

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

(In reply to Augustin Trancart [:autra] from comment #39)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #36)
> > Please see comment 30 :)
> 
> Yes, I need to separate the logic for sms and mms. Is that what you meant by
> pointing me to comment 30? Please also see below.
> And I discovered flaws in my algorithm anyway. My workaround to find my own
> numbers does not work :-(. So I need to do things in a bit formal way :
> 
> For SMS :
> there is 4 cases (R is receiver, S sender, F the number I want to filter
> with) :
> 1/ R= me, S=me //messages sent to myself
> 2/ R=other, S=me 
> 3/ R=me, S=other
> 4/ R=other1, S=other2 //impossible case, hopefully :-)
> 
> So the condition should be for one message :
> |( (R matches F OR S matches F) AND F is not my own number) OR (R matches F
> AND S matches F AND F is my own number) |

let C1 = "R matches F", C2 = "S matches F", C3 = "F is my own number", the logic expression can be rewritten as:

  ((C1 || C2) && ~C3) || (C1 && C2 && C3)

Its truth table:

             C3
            0  1

  C1 C2 00  0  0
        01  1  0
        11  1  1
        10  1  0

Can be simplified as: ((C1 || C2) && ~C3) || (C1 && C2)

> The first part matches sms sent to F or received from F, but we exclude the
> case where F is myself, otherwise we would get all my sent messages to any
> recipient when I filter with my own numbers.

It's true for filtering messages without the work done by MobileMessageDB, but it's not true in our case.  What we have now:

  1) For SMS, thread participants must be the receiver if it's an outgoing message, or the sender otherwise.  So basically when you call |getMessages()| with a filter P1 and get a sent message back with its sender set to P1, then the receiver must be P1, too.

  2) For a received MMS, thread participants must be the sender alone,

  3) For a sent MMS, thread participants are all the receivers.

Your own number does _NOT_ participate the message thread choosing process in any case.

> The second part is when I sent sms to myself, R, F and S are the same. This
> exclude every other messages.
> Here I have a problem: I need somehow to know my own number. Julien said
> there was no way of knowing that right now.

More precisely, there is an unreliable way.

> So we need to fix that beforehand I guess.
> My idea of comparing R and S does not work actually.
> Actually, it is not the first time we have this issue: bug 880251. Is the
> msisdn exposed to gaia?
> If not (or if the SIM does not expose it at all), I think we can only check
> the receiver. After all, it will work for all cases, except when I never
> received any texts from this contact. This is incomplete, but does not
> introduce bugs.

For SMS, you don't have to do anything, just call |getMessages()|.

> For MMS:
> 
> > Reasonable, but the last question in comment 30 remains.
> 
> I tend to exclude every cases where Rs.size > 1 anyway. We want candidates
> to send messages to a contact. So we want to select the 1-to-1 thread if it
> exists.

Maybe.  Maybe the best solution is to repeat such process whenever the receivers list changes.

> It would be very confusing for the user if he wants to send a MMS to
> A, and is brought to thread with A,B,C. We want to go to the thread between
> A and the user only, if it exists. Is it ok for you if gaia behaves this way?
> 
> If we do that, we actually fall back to the same logic as before, by
> excluding all the cases where recipients.length > 1:
> So We do still have the problem about my own number.
> 
> Thanks for your help!

Comment 42

[Julien Wajsberg [:julienw]]

Augustin and I discussed this together in real life.

Basically, we can borrow some of the logic from [1] to know whether a message is incoming or outgoing. Then we can ignore all messages that have several recipients, as they don't interest us here.

So, here what could be the logic to filter the messages:
* if it's an incoming message, we're good. We never have incoming messages in group threads. Takes the threadId from this message and stop here.
* if it's an outgoing message
** if it's a sms, we're good, sms always have one sender and one receiver exactly. Take the threadId and stop here
** if it's a mms, check record.receivers.length. If it's > 1, ignore the message and take the next one. If it's === 1, then we're good, take threadId and stop here.

Now, what happens if I try to send a message to myself using an activity? I _think_ everything actually happens the same and we don't need to special case this. This will need to be tested on an actual device obviously :)

Thanks again Augustin for your work :)

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/threads.js#L40-L46

Comment 43

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

(In reply to Julien Wajsberg [:julienw] from comment #42)
> So, here what could be the logic to filter the messages:
> * if it's an incoming message, we're good. We never have incoming messages
> in group threads. Takes the threadId from this message and stop here.
> * if it's an outgoing message
> ** if it's a sms, we're good, sms always have one sender and one receiver
> exactly. Take the threadId and stop here
> ** if it's a mms, check record.receivers.length. If it's > 1, ignore the
> message and take the next one. If it's === 1, then we're good, take threadId
> and stop here.

The steps here are just another form of comment 30. So I'm good with them. ;)

Comment 44

[Augustin Trancart [:autra]]

I'm getting back to work :-)

I'm implementing this logic, with a little precision regarding the case I send messages to myself : I think I still need to test the sender in the case of an incoming message, because when filtering with my number, I would also get messages in the form :
{ delivery : 'received', receiver: <my number>, sender : <other number> } which are not belonging to the right thread in this case. 

And if it is a sent messages, I also need to test the receiver, because if I'm filtering with my own number, I would actually get all my sent messages from every thread (as a side note, I'm a bit worried about performance in this case. Fortunately sending sms to oneself is not the main use case).

So it would exactly what Julien said with this little addition :
* If it is an incoming message and sender match number : take it and stop here.
* If it is an outgoing message :
** If it is a sms AND receiver match number : take it and stop.
** if it is a MMS, same as above but we also test receiver.length == 1

Does this seem ok to you?

Comment 45

[Julien Wajsberg [:julienw]]

So, I have know

Comment 46

[Julien Wajsberg [:julienw]]

So I have now a big doubt.

* we'll eventually support incoming group MMS, so maybe we need to ignore them as well here
* I agree with you about the performance when you send a message to yourself. So maybe we should have a safety net and bailout after say 50 different messages? If we haven't find anything in the first 50 messages (given we get the message in the "most recent" first mode), maybe we can stop ? What do you think?

Comment 47

[Augustin Trancart [:autra]]

Concerning group MMS, my patch handle this case. I added a test case for it as well. Actually, I didn't know we aren't handling this case yet, so I did it anyway :-)

Concerning the performance hit with the user own number, I tested on my keon, and honestly I didn't see any difference with a regular test (or maybe like half a second, but I'm not even sure). If you thing it is necessary I could benchmark that though.

I still have two questions :
* do I need to make some marionette test ?
* There is still one thing not perfect : when there is no thread, the message area is focus and the keyboard is shown. When you are redirected to a thread, this is not the case. Do we want it ? How can I achieve that ?

Apart from that, I updated my patch :-)

Comment 48

[Julien Wajsberg [:julienw]]

(In reply to Augustin Trancart [:autra] from comment #47)
> Concerning group MMS, my patch handle this case. I added a test case for it
> as well. Actually, I didn't know we aren't handling this case yet, so I did
> it anyway :-)
> 
> Concerning the performance hit with the user own number, I tested on my
> keon, and honestly I didn't see any difference with a regular test (or maybe
> like half a second, but I'm not even sure). If you thing it is necessary I
> could benchmark that though.

I'll have a look on a slower device using my own data (but not now, so keeping the NI).
I think you need to send a message to yourself but with a dataset that doesn't have a thread for you yet (so that you ends up looking at all messages).

> 
> I still have two questions :
> * do I need to make some marionette test ?

Could be a good idea, yes :) (if you know how to do it, I don't really know ;) )

> * There is still one thing not perfect : when there is no thread, the
> message area is focus and the keyboard is shown. When you are redirected to
> a thread, this is not the case. Do we want it ? How can I achieve that ?

Maybe: `Navigation.toPanel(...).then(function() { Compose.focus(); });`

Comment 49

[Augustin Trancart [:autra]]

(In reply to Julien Wajsberg [:julienw] from comment #48)
> I think you need to send a message to yourself but with a dataset that
> doesn't have a thread for you yet (so that you ends up looking at all
> messages).

Yes, indeed, we must test like that. I've just deleted my 1-to-1 thread with myself it on my keon, and I see no difference. But if you can test with your own device, that is good.


> Maybe: `Navigation.toPanel(...).then(function() { Compose.focus(); });`

Ok, will try that tomorrow.

Thanks a lot !

Comment 50

[Augustin Trancart [:autra]]

Actually, I would need to pass a parameter to ActivityHandler to know when I must focus the keyboard, and when I should not when I'm redirecting to a thread. But this has some impact on the rest of the code. Do you think it's worth the impact?

Comment 51

[Julien Wajsberg [:julienw]]

(In reply to Augustin Trancart [:autra] from comment #50)
> Actually, I would need to pass a parameter to ActivityHandler to know when I
> must focus the keyboard, and when I should not when I'm redirecting to a
> thread. But this has some impact on the rest of the code. Do you think it's
> worth the impact?

Maybe we can try to discuss which cases should focus the message input and which cases should not ?

Seems to me we should focus the message input when we have a recipient, and we should focus the recipient when we have nothing. So we can probably find a good heuristic here.

Comment 52

[Julien Wajsberg [:julienw]]

(In reply to Augustin Trancart [:autra] from comment #49)
> (In reply to Julien Wajsberg [:julienw] from comment #48)
> > I think you need to send a message to yourself but with a dataset that
> > doesn't have a thread for you yet (so that you ends up looking at all
> > messages).
> 
> Yes, indeed, we must test like that. I've just deleted my 1-to-1 thread with
> myself it on my keon, and I see no difference. But if you can test with your
> own device, that is good.

Yep, it doesn't look bad on my data either. Let's keep it like this for now, if we see any performance issue later we can revisit.

Comment 53

[Julien Wajsberg [:julienw]]

Comment on attachment 8374860 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16216

Added comments on github.

Thanks a lot, this is not the easiest bug to start working on FxOS ;)

Comment 54

[Julien Wajsberg [:julienw]]

Removing NI for Ayman as the question is obsolete now that we use the inline activity.

NI Jenny so that she knows about this work. Jenny, most of the discussion here is about how to find the correct thread from the input phone number. The relevant information for UX is probably comment 4. There is especially a comment at the end of that comment about switching from thread view to composer view, and it might be a good idea to have a separate bug for this.

Comment 55

[Jenny Lee]

(In reply to Julien Wajsberg [:julienw] from comment #54)

Thanks for including me ;) I'll take some time to think about this feature switching from thread view to composer view and get back to you.

Comment 56

[Augustin Trancart [:autra]]

Hey Julien, I added a comment to my merge request on github concerning an implementation detail (didn't want to clutter the conversation here). Could you check this out ?

Comment 57

[Julien Wajsberg [:julienw]]

Sorry, it took quite long (you sent this while I was in holiday so I had to recover from the various things happening while I was away...). And I actually can't really answer without seeing the new code, sorry :(

Can you push the new code so that I can have a look?
Thanks !

Comment 58

[Augustin Trancart [:autra]]

Here you are! Splitted in two commits, I'll rebase them (and resolve the merge conflict) when the tests would pass.

I still have one bug: For a contact with 2 numbers and a thread for each of these numbers, contact info are not displaying in the second thread. Not sure if it is a problem with my patch though...

Comment 59

[Julien Wajsberg [:julienw]]

Hey Augustin,

sorry that I took so much time. I answered on github, hope it makes sense !
Please NI me again if you need anything !

Comment 60

[Jenny Lee]

Hey Julien, do you know what's the status of this bug right now? If the correct behavior stated by Ayman has been implemented, I can provide a spec for adding the function to switch from thread view to composer view. Let me know, thanks!

Comment 61

[Augustin Trancart [:autra]]

Hey Jenny,

Things are implemented (and working on my keon), but I still need a bit of work before this could get landed: I still have some nits and a refacto to do and I need to fix my tests as well. I will try to find some time in the following days to accelerate the process.

I would say that you can start the spec if you want, I might be able to work on this as well. 

I was thinking of a + button where there is the attachment button in thread view (but that would mean moving this "add attachment" to the menu), or adding an entry on this very menu, but you're the UX designer :-)

Anyway, better wait for Julien :-)

Comment 62

[Julien Wajsberg [:julienw]]

It's moving slowly but surely :)

Jenny, it's probably better if you file a separate bug that depends on this one, with the spec for the + button :)

Comment 63

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16216

Comment 64

[Steve Chung [:steveck]]

Comment on attachment 8479182 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16216

Since Julien is OOO now, redirect reviewer to other peer.

Hi Oleg, do you have time on this? You could assign back to me if you have no free cycle.

Comment 65

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8479182 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16216

Hey!

Sorry for the delay. I've left some comments on GitHub + please rebase your PR on the latest master.

Please re-request review again once you're ready.

Thanks!

Comment 66

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16216

Thanks for the review! Here is the new version (also rebased on master).

I've realized that my tests were testing basically nothing :-) (I wasn't actually asserting anything). So I fixed that as well.

Comment 67

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8485440 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16216

Hey Augustin,

I've left some more nits on github + question about threads that contain only messages with "sending\error\not-downloaded" delivery status. Looks like we don't cover these cases. I haven't look at the unit tests yet as they can change if my question is valid :)

Thanks for you work and patience!

Comment 68

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24886

Comment 69

[Augustin Trancart [:autra]]

Do you want me to open a bug for the problem with MessageManager.getMessages returning wrong messages ? I can also provide a branch that logs the problem if you want.

Also, what about the focus of the keyboard? It is done by default when going to a new message, but user currently needs to tap when going to an existing thread. I don't know what is good. Maybe it is ok like that, as users might use this to go to an existing thread just to read it.

Comment 70

[Julien Wajsberg [:julienw]]

(In reply to Augustin Trancart [:autra] from comment #69)
> Do you want me to open a bug for the problem with MessageManager.getMessages
> returning wrong messages ? I can also provide a branch that logs the problem
> if you want.

Yes please, that would be very useful !

> 
> Also, what about the focus of the keyboard? It is done by default when going
> to a new message, but user currently needs to tap when going to an existing
> thread. I don't know what is good. Maybe it is ok like that, as users might
> use this to go to an existing thread just to read it.

Clearly the focus behavior is what UX wants :) The reason is exactly what you said: we may open a thread just to read it. I think there are good reasons for each side.

Comment 72

[Josh Cheng [:josh]]

Triage:
polish, not fix on 2.0M.

Comment 73

[Julien Wajsberg [:julienw]]

Hey Jenny, I have a last question for you here.

Say you have a thread for a contact A. This thread has a draft.

You got Contacts, open contact A, press "send SMS" button.

In this patch, we want to show the thread for contact A. Should we use the draft, or should we have an empty composer?

I think an empty composer makes more sense, but the current patch uses the draft (because it's done automatically). What do you think?

Comment 74

[Jenny Lee]

Hey Julien,

If we show empty composer and user types something, hits send, later return to Message app and go to the same thread, will the previous draft still be there? If so, the previous draft would seemed to be a little irrelevant at this point. If not, we'd be deleting something without user's permission.    
So I actually think we should keep the draft so that user is aware of he has typed something before, and make decision whether or not to use the draft. Thanks!

Comment 75

[Julien Wajsberg [:julienw]]

OK good for me, thanks for the answer.

Then the current's patch behavior is right, let's focus on the code :)

Comment 76

[Augustin Trancart [:autra]]

Comment on attachment 8501021 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24886

my PR has a unit test that fails, but it does not seem to be related. Do you confirm that?

The move of findThreadFromNumber implies to move some test. Actually I think it is a lot better now, because tests are more separated in their own area. But don't hesitate to have a close look at them :-)

Comment 77

[Julien Wajsberg [:julienw]]

(In reply to Augustin Trancart [:autra] from comment #76)
> Comment on attachment 8501021 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24886
> 
> my PR has a unit test that fails, but it does not seem to be related. Do you
> confirm that?

Yep, unrelated :)

> 
> The move of findThreadFromNumber implies to move some test. Actually I think
> it is a lot better now, because tests are more separated in their own area.
> But don't hesitate to have a close look at them :-)

Cool, I'll look at it shortly!

Comment 78

[Julien Wajsberg [:julienw]]

I haven't tried the new code yet but I left some comments on github about especially unit tests, but also questions about the code.

Comment 79

[Julien Wajsberg [:julienw]]

Comment on attachment 8501021 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24886

This looks good !

I noticed we don't have the focus on composer but this might be because of bug 1103010. Anyway we can do this in a follow-up bug if necessary.

So just do the changes I ask and we should be good !
Thanks again

Comment 80

[Augustin Trancart [:autra]]

Concerning focus behaviour, isn't it what the UX want? You said so in comment #70... Anyway I can change that if you want (I don't know yet how to though, but that should not be very difficult?)

Comment 81

[Julien Wajsberg [:julienw]]

When the user taps a thread from thread list, than we should not have the focus.
However, when coming from an activity, I think it makes sense to have the focus.

I noticed that the issue I saw in bug 1103010 is not happening on Flame, but only on Buri, so I think you can move forward with this on Flame.

Hope this is clearer!

Comment 82

[Julien Wajsberg [:julienw]]

Just added new comments about the unit tests. We're very close to the end ;)

Comment 83

[Julien Wajsberg [:julienw]]

Comment on attachment 8501021 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24886

Please flip the r flag again once you're ready :)

Comment 84

[Julien Wajsberg [:julienw]]

Hey Augustin, I left a comment on github, tell me what you think!

Comment 85

[Julien Wajsberg [:julienw]]

Comment on attachment 8501021 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24886

r=me

thanks!

I'm squashing and landing for you.

Comment 86

[Julien Wajsberg [:julienw]]

master: bd4dcc8c4582e2368b47b0e62506d3031fb2fc09

good job, thanks !!

Comment 87

[Augustin Trancart [:autra]]

yay \o/

Comment 88

[Alissa（leave from Mozilla）]

This issue has been verified successfully on flame 2.2.
Gaia-Rev        69ac77cfa938fae2763ac426a80ca6e5feb6ad25
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/33781a3a5201
Build-ID        20150107010216
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150107.044401
FW-Date         Wed Jan  7 04:44:12 EST 2015
Bootloader      L1TC000118D0

Comment 89

[Lancy(Leave from Mozilla)]

Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/5681/