856085 - [sms] support multiple term search highlight

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

Bug 853788 implements multiple-term search. In that bug, we didn't fix how the search highlighting works so it still highlights only one-term search.

In this bug, we'd like to make highlight work also for multiple-term searchs.

Comment 1

[Peter Dolanjski [:pdol]]

Am I understanding this correctly:
If I start typing in a recipient name "Julien W" this bug is to highlight my full search string in the Contact results that start to appear?

Comment 2

[Julien Wajsberg [:julienw]]

Yes that's it :)

That's the current behavior when searching.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

This looks like a nice-to-have, not part of a user story so only tracking - prioritize accordingly and nominate for uplift when ready & qa'd.

Comment 4

[Rick Waldron [:rwaldron]]

(In reply to [e-mail] from comment #3)
> This looks like a nice-to-have, not part of a user story so only tracking -
> prioritize accordingly and nominate for uplift when ready & qa'd.

Comment 5

[Dietrich Ayala (:dietrich)]

Hi Rick. Since you've marked 5 P1 Leo+ user story bugs as blocked by this one, can we infer that you think comment #3 is incorrect?

Comment 6

[Rick Waldron [:rwaldron]]

This isn't a matter of "nice to have", the current behaviour is flat out broken. If I have some contacts that highlight and some that don't because of poorly designed software, it's a bug.

Comment 7

[Dietrich Ayala (:dietrich)]

Looked a bit more into this. Bug 853788 isn't blocking, and there's a ton of release-blocking work to do. Seems like we should back-out bug 853788 if it's broken, and not spend more time fixing this feature that is not required.

Comment 8

[Rick Waldron [:rwaldron]]

No, https://bugzilla.mozilla.org/show_bug.cgi?id=853788 _fixes_ the broken behavior where multi-term searches for a contact simply did not work. This bug was filed as a complement to that fix, instead of piling too much onto the same ticket.

Comment 9

[Julien Wajsberg [:julienw]]

To me comment 3 is correct: this bug is a nice to have, but we won't hold a release because of this. But I suspect this is very quick to do so it's a nice to have that we should do anyway, maybe just not now, rather in a few days.

I think however that Bug 853788 is really mandatory, I personally would hold a release because of that bug.

Comment 10

[Rick Waldron [:rwaldron]]

Attachment: Fix for 856085

Comment 11

[Peter Dolanjski [:pdol]]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> To me comment 3 is correct: this bug is a nice to have, but we won't hold a
> release because of this. But I suspect this is very quick to do so it's a
> nice to have that we should do anyway, maybe just not now, rather in a few
> days.
> 
> I think however that Bug 853788 is really mandatory, I personally would hold
> a release because of that bug.

I fully agree with the above.  Given that this bug is being tracked and a patch is ready, it should make it in anyways.

Comment 13

[Borja Salguero [:borjasalguero]]

Rick, Could you check the scenario of my comment in the PR? Currently it's failing. Thanks!

Comment 14

[Rick Waldron [:rwaldron]]

@Borja, that's not an existing behaviour and has nothing to do with this ticket. I will file a new ticket and handle it separately.

Comment 15

[Borja Salguero [:borjasalguero]]

Ok, could you add me as CC of that bug? Thanks!

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 733374 [details] [review]
Fix for 856085

r=me 
thanks a lot for this work

Comment 17

[Julien Wajsberg [:julienw]]

master: https://github.com/mozilla-b2g/gaia/commit/60c354ac09320a284c8be948e75424a4b7db9c31

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 733374 [details] [review]
Fix for 856085

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: no highlight of what's been found in the contact name/number. Also this fixes at least one bug (Bug 858029) but I think we also improved the general safety of the code by using a tested minimal template engine instead of concatenating strings and putting the result in innerHTML.
Testing completed: yes
Risk to taking this patch (and alternatives if risky): low to medium, but there are lots of unit test, and the changes are quite well contained.
String or UUID changes made by this patch: none

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 733374 [details] [review]
Fix for 856085

Seems like there's no push to block here so approving given this is well tested and a nice-to-have even though I'm concerned we're adding a feature post feature-complete so if this causes any significant regressions, we'll have to look at a backout.

Comment 20

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

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 60c354ac09320a284c8be948e75424a4b7db9c31
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 21

[Julien Wajsberg [:julienw]]

v1-train: 023f1a6ff692e39b980482119b27371a4f5f3ea8