Implement contacts list filtering on Name, Last Name and Mail

VERIFIED FIXED in Firefox 34

Status

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mak, Assigned: Paolo)

Tracking

unspecified
mozilla35
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite -
qe-verify +

Firefox Tracking Flags

(firefox34 verified, firefox35 verified)

Details

(Whiteboard: [p=8, contacts][desired for first release needed][loop-uplift])

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
We need an object able to filter the whole list of contacts based on the algorithm decided in bug 1037113

This might be done in 2 ways:
1. keep all contacts in a memory cache and filter this list.
2. search the database on-the-fly

The current database implementation suggested in bug 1000112 has an all() method that returns the whole list, on top of that seems to be easier to implement solution 1. It will use more memory , so won't be very good if a user has thousands of contacts. On the other side Solution 2 is lighter on
memory but has more disk usage. I think most users have less than 100 contacts, so solution 1 should be fine.
Telemetry/FHR should be added (I'm going to file a separate bug about this).
Reporter

Updated

5 years ago
Blocks: 1037116
Reporter

Updated

5 years ago
Depends on: 1037117
Whiteboard: [ux]
Whiteboard: [ux]
Reporter

Updated

5 years ago
Blocks: 1037131
Whiteboard: p=8
Flags: firefox-backlog+
Priority: -- → P2
Target Milestone: --- → mozilla34
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Whiteboard: p=8 → p=8 [qa+]
Iteration: --- → 34.1
Points: --- → 8
Whiteboard: p=8 [qa+] → [qa+]

Updated

5 years ago
Whiteboard: [qa+] → [qa+, contacts]
Target Milestone: mozilla34 → mozilla35
QA Contact: anthony.s.hughes
Depends on: 1038716
Whiteboard: [qa+, contacts] → [qa+, contacts] p=8
Whiteboard: [qa+, contacts] p=8 → [qa+, contacts]
...running in a ChromeWorker with in-memory data and real-time index mutations.
Rebased on top of patches in bug 1038716 (required)
Attachment #8461456 - Attachment is obsolete: true
Whiteboard: [qa+, contacts] → [qa+, contacts][first release needed]
Priority: P2 → P1

Updated

5 years ago
Whiteboard: [qa+, contacts][first release needed] → [p=8, qa+, contacts][first release needed]
Whiteboard: [p=8, qa+, contacts][first release needed] → [p=8, contacts][first release needed][qa+]

Updated

5 years ago
Priority: P1 → P3
Target Milestone: mozilla35 → mozilla34
Iteration: 34.1 → 34.2
Un-assigning, because it's unlikely that I'll be working on this before my PTO starts.
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Iteration: 34.2 → ---
Whiteboard: [p=8, contacts][first release needed][qa+] → [p=8, contacts][desired for first release needed][qa+][not needed for Fx34]
Flags: qe-verify+

Updated

5 years ago
Priority: P3 → --
Target Milestone: mozilla34 → mozilla35
Assignee

Comment 4

5 years ago
Posted patch Bare-bones filtering (obsolete) — Splinter Review
If there is one pattern React is designed to handle, this is list filtering.

This simple implementation may be sufficient for the initial release.
Attachment #8497527 - Flags: review?(mdeboer)
Comment on attachment 8497527 [details] [diff] [review]
Bare-bones filtering

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

I like it! It seems to work well for my 100+ contacts list.

Be aware that you need to un-bitrot this patch considerably before landing.

r=me with comments addressed.

::: browser/components/loop/content/js/contacts.jsx
@@ +332,5 @@
> +      if (showFilter) {
> +        let filter = this.state.filter.trim().toLocaleLowerCase();
> +        if (filter) {
> +          let filterFn = contact => {
> +            let prefEmail = contact.email.find(e => e.pref) || contact.email[0];

I'd prefer not to duplicate this code. Can you move the `getPreferredEmail` method out of the ContactDetail class as a separate function that we can call here too?

::: browser/components/loop/content/shared/css/contacts.css
@@ -6,5 @@
>    border-top: 1px solid #ccc;
>    overflow-x: hidden;
>    overflow-y: auto;
> -  /* We need enough space to show the context menu of the first contact. */
> -  min-height: 204px;

I thought we already had this added in bug 1071633?
Attachment #8497527 - Flags: review?(mdeboer) → review+
What shall we do about tests?
Assignee

Comment 7

5 years ago
Posted patch Updated patchSplinter Review
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> I'd prefer not to duplicate this code. Can you move the `getPreferredEmail`
> method out of the ContactDetail class as a separate function that we can
> call here too?

Done, can you double-check this is what you expected?

> ::: browser/components/loop/content/shared/css/contacts.css
> @@ -6,5 @@
> >    border-top: 1px solid #ccc;
> >    overflow-x: hidden;
> >    overflow-y: auto;
> > -  /* We need enough space to show the context menu of the first contact. */
> > -  min-height: 204px;
> 
> I thought we already had this added in bug 1071633?

We need a fixed-height list now, because filtering affecting the height of the panel looks quite unusual.
Assignee: nobody → paolo.mozmail
Attachment #8497527 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8499540 - Flags: review?(mdeboer)
Assignee

Comment 8

5 years ago
Comment on attachment 8499540 [details] [diff] [review]
Updated patch

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

::: browser/components/loop/content/js/contacts.js
@@ +24,5 @@
> +    // The model currently does not enforce a name to be present, but we're
> +    // going to assume it is awaiting more advanced validation of required fields
> +    // by the model. (See bug 1069918)
> +    // NOTE: this method of finding a firstname and lastname is not i18n-proof.
> +    let names = this.props.contact.name[0].split(" ");

Forgot to qrefresh... this reads: contact.name[0].split(" ");
Iteration: --- → 35.3
Comment on attachment 8499540 [details] [diff] [review]
Updated patch

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

I like it! Thanks!
Attachment #8499540 - Flags: review?(mdeboer) → review+
Assignee

Comment 11

5 years ago
As mentioned, we may want to uplift this as a companion to the import feature (we have put the strings in place already) to make a large contacts list manageable.
Whiteboard: [p=8, contacts][desired for first release needed][qa+][not needed for Fx34] → [p=8, contacts][desired for first release needed][qa+][not needed for Fx34][loop-uplift]
https://hg.mozilla.org/mozilla-central/rev/0123a81972b2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [p=8, contacts][desired for first release needed][qa+][not needed for Fx34][loop-uplift] → [p=8, contacts][desired for first release needed][not needed for Fx34][loop-uplift]
Whiteboard: [p=8, contacts][desired for first release needed][not needed for Fx34][loop-uplift] → [p=8, contacts][desired for first release needed][loop-uplift]
Pauly -- This is ready to verify.  Thanks!
Flags: needinfo?(paul.silaghi)
Possible issues:
1. the search box shows up only after adding 7 or more contacts

https://bugzilla.mozilla.org/show_bug.cgi?id=1037113#c3
(In reply to Romain Testard [:RT] from comment #3)
> OK agreed, so:
> Search results against names returned when the search string is even only 1
> character
2. searching for first name/last name works; searching for last name/first name returns 0 results

> Search results against e-mail addresses returned when search string of 3
> characters or more is entered
3. search results returned even of 1 character
Flags: needinfo?(paul.silaghi) → needinfo?(paolo.mozmail)
Assignee

Comment 15

5 years ago
The reported behaviors are known, we implemented a limited version of search to make imported contacts manageable. If we need more complex search behaviors in the future, we should handle them in separate bugs (for example bug 1037117 about a frecency algorithm).
Flags: needinfo?(paolo.mozmail)
QA Contact: anthony.s.hughes → paul.silaghi
Based on comment 15, verified fixed 35.0a1 (2014-10-07) Win 7, Ubuntu 13.04, OS X 10.10
Status: RESOLVED → VERIFIED
Comment on attachment 8499540 [details] [diff] [review]
Updated patch

Approval Request Comment
Part of the staged Loop aurora second uplift set
Attachment #8499540 - Flags: approval-mozilla-aurora?
Flags: needinfo?(paul.silaghi)
Comment on attachment 8499540 [details] [diff] [review]
Updated patch

Approval previously granted to jesup to land this change as part of the second batch of Loop fixes to land on Aurora. Marking the approval after the fact.
Attachment #8499540 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed FF 34b1 Win 7
Flags: needinfo?(paul.silaghi)
Does this have developer automation coverage?
Flags: in-testsuite?
Assignee

Comment 22

5 years ago
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #21)
> Does this have developer automation coverage?

Not that I know of. Bug 1073468 is still open.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.