1084184 - [Midori 2.0][SMS]The result is not filtered clearly after inputting number key words in recipient box.

Status: RESOLVED WONTFIX

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

Description

[sync-1]

Mozilla Build ID: 20140916000205
 Created an attachment (id=975281)
 issue_pic
 
 DEFECT DESCRIPTION:
 The result is not filtered clearly after inputting number key words in recipient box.
 
  REPRODUCING PROCEDURES:
 Preconditions:There should be a contact with at least two phone number stored on the addressbook
 
 1.Open SMS app;
 2.Create a new SMS;
 3.On 'To' field start typing a string which is number of the contact,but the result is not filtered-->KO
 
  EXPECTED BEHAVIOUR:
 The result should be filtered clearly after inputting number key words in recipient box.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:Medium
 
  REPRODUCING RATE:5/5
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[sync-1]

Attachment: issue_pic

Comment 2

[Julien Wajsberg [:julienw]]

QAwanted: can we please try on v1.4 and v1.3?

I tested it myself on v2.0 and v2.2 and I reproduce.

NI Jenny for UX input, but I think the behavior change makes sense. I checked the spec from when we implemented this but it says nothing about using a phone number to match contacts.

Comment 3

[Cody Roesch [:croesch]]

Julien,
    So if i understand this correctly, it's not correctly placing the matched number at the top of the list for when a contact has multiple numbers? So if I have 4 numbers for one contact and them all labeled different things (mobile, home, fax, personal), if I type in a match with the fax number it should be at the top? It makes sense but I just wanna know what "filtered correctly" means and that I understand what is expected here before i do the checks.

Comment 4

[Jenny Lee]

Hi Julien,

I can provide a general rule of thumb for search result but probably not a detail spec/algorithm ;)
The rule would be show the most accurate result on top of the list, if there are multiple result for a particular search term, order by name. This applies to both text and number search term. So in this case, the first two numbers on screen of attachment 8506620 [details] shouldn't be showing at all. Let me know if you think otherwise, thanks!!

Comment 5

[Julien Wajsberg [:julienw]]

The ordering part is bug 1019245.

In this bug we want to only filter the results when the input is a phone number. (filtering means we want to show only the results where the phone number contains the typed number)

Thanks Jenny and Cody !

Comment 6

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> QAwanted: can we please try on v1.4 and v1.3?

This issue is reproducible on Flame base image v164 (v1.4) and Flame base image v123 (v1.3).

Observed behavior: Typing numbers in Messages 'To' field causes the suggested results to display numbers not matching what was typed because the matching contact has multiple sets of numbers.

> I tested it myself on v2.0 and v2.2 and I reproduce.

I also checked 2.1 and issue is occurring on there (so I could set tracking flags)(and there isn't even a 1.3 flag anymore).

Comment 7

[Julien Wajsberg [:julienw]]

Tentatively put this in a mentored list of bugs. The code is likely in contacts.js but also maybe in thread_ui.js. This can be likely tackled using only Firefox by following the instructions in [1] and possibly changing js/desktop-only/navigator_moz_contacts.js to add contacts with several phone numers (if not there already).

[1] https://github.com/julienw/gaia/blob/1082618-sms-readme/apps/sms/README.md

Comment 8

[cvfergus]

I would like to be assigned to this bug. I have reproduced the undesirable behavior and am working on the best way to improve it.

Comment 9

[Julien Wajsberg [:julienw]]

Hey, thanks :)

You won't likely have much help on IRC during this week-end, but next week we'll be up to work.

If you ask "needinfo" for me on this bug (this is the little checkbox after the comment box) I can maybe see and answer your question during my holidays ;)

Comment 10

[cvfergus]

Hi Julien, 

After working on this for a while, the best solution I've come up with looks like this:
- Change the Contacts.isMatch method to return the value that was matched on in addition to True/False. For example, if "800" is typed in, return "800-BUY-A-CAR" in addition to "TRUE".
- Change the Contacts.findBy.onsuccess callback to pass the list of matched field values in it's callback.
- In ThreadUI.listContacts, add the list of matched field values to the "skip" term so they won't be displayed.

Before I try to implement this, I wanted to check that this solution seems reasonable. I wanted to avoid modifying isMatch, but couldn't find a way without reproducing a lot of it's functionality. I have a suspicion that I'm approaching the issue wrong, but can't figure out what to change.

Charley

Comment 11

[cvfergus]

I meant to make the previous comment "Needs Information", oops.

Comment 12

[Julien Wajsberg [:julienw]]

You add it the "need information" flag separately :)
(I'm adding it myself just so that I don't lose track of your question)

Comment 13

[Julien Wajsberg [:julienw]]

I admit I don't remember everything I read in this area of code, it's a long time I haven't touched this.

I have a concern with what you propose in comment 10: you would skip everything, right ? But we want to keep the matched values, and skip the unmatched.

The issue here, IMO, is that we use a "renderAll" renderer (see at the start of contact_renderer); this means we want to render all phone numbers for a contact. This is what we want if we match using the given or family name, but not when we match using the phone number.

Therefore, I agree with you that we need something else from findBy: we need to know the fields that matches. If "tel" matched, then I think that we don't want to use "renderAll". Then either we create another renderer, or we allow overriding this from render(renderOpts).

What do you think?

Comment 14

[cvfergus]

Thanks for the help, I hadn't seen that the contact_renderer file is the heart of this. After reading through this a couple of times I think that I'd prefer to allow overriding the renderAll variable from render(renderOpts).

My reasoning for this is that the different renderers all use different combinations of templates, the new renderer would have the same templates as 'suggestion', so really is a type of 'suggestion'.

I'll start working on the change to findBy, then see about implementing the change to render(renderOpts). If that starts looking ugly, I'll instead create a new renderer called 'suggestionPartial' (since it might be useful for multiple email contacts in the future).

Comment 15

[Julien Wajsberg [:julienw]]

Agreed, let's go this way, thanks a lot :)

Comment 16

[cvfergus]

Attachment: A potential fix.

I suspect I'm going to need some help cleaning this up, but this fixes the issue.

Comment 17

[cvfergus]

I need to work on this further before review.

Comment 18

[Julien Wajsberg [:julienw]]

Please request feedback if you have any question :)

Comment 19

[cvfergus]

Attachment: Link to the branch on github

Hello Julien, after quite a bit of work I have a working solution. I suspect that it's going to need significant changes so I'm including a link to the branch I'm working on rather than a pull request. Is this the correct way to ask for feedback here?

Comment 20

[Julien Wajsberg [:julienw]]

Hey, it's better to include a real pull request so that I can add comments directly on it :)

Comment 21

[GitHub Autolander Bot]

Attachment: [gaia] charleyf:Bug1084184 > mozilla-b2g:master

Comment 22

[cvfergus]

Attachment: Pull Request Containing fix for 1084184

Okay, thanks Julien I just wanted to make sure that making new pull requests didn't cause issues. Attaching my pull request.

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 8564532 [details] [review]
Pull Request Containing fix for 1084184

Hey Charley, thanks a lot for your patch.

I know what disturbs me in your proposal: what you're doing in this patch is merely filtering what's displayed.

But then we can have unconsistent behaviors.

For example, take a contact with 2 phone numbers, first you type the name of the contact, then try to filter out the first displayed number (so the second number is displayed only), then you press "enter" => you'll have the wrong entry added.

For this issue, I think we need to patch what happens in js/contacts.js instead.

In [1], I'd replace "callback" by a function that would filter out phone numbers or emails, but only if the term does not match the names (because we don't want to filter out my phone number if my email also has "julien"). Maybe we can make findBy return a little more metadata, to know which term matched which part of the contact, which would make the later filtering easier?

[1] https://github.com/mozilla-b2g/gaia/blob/c5b086914cba6eb98961724423a0563d00d1f312/apps/sms/js/contacts.js#L242

What do you think?

Comment 24

[Julien Wajsberg [:julienw]]

BTW, the same bug happens if a contact has a phone number and an email, and we try to select the email (even after bug 1135649 is fixed).

Comment 25

[cvfergus]

Julien, thanks for the suggestions (and hints), I think that I'll be able to get to a better fix. I'll definitely handle the email case as well.

Comment 26

[Julien Wajsberg [:julienw]]

Hey Charley,

wondering if you could move forward on this :)
If not, no worry, I just want to know if you're still on this, so that somebody else could take it from here if you're busy with something else.

Thanks !

Comment 27

[cvfergus]

Apologies, I am no longer working on this and did not indicate that. Sorry!

Comment 28

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 29

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(