Closed Bug 1037114 Opened 10 years ago Closed 10 years ago

Implement contacts list filtering on Name, Last Name and Mail

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal
Points:
8

Tracking

(firefox34 verified, firefox35 verified)

VERIFIED FIXED
mozilla35
Iteration:
35.3
Tracking Status
firefox34 --- verified
firefox35 --- verified

People

(Reporter: mak, Assigned: Paolo)

References

Details

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

Attachments

(2 files, 2 obsolete files)

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).
Blocks: 1037116
Depends on: 1037117
Whiteboard: [ux]
Whiteboard: [ux]
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+]
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
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+]
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+
Priority: P3 → --
Target Milestone: mozilla34 → mozilla35
Blocks: 972015
Attached 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?
Attached 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)
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+
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: 10 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)
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?
(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.