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)
Hello (Loop)
Client
Tracking
(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)
30.16 KB,
patch
|
Details | Diff | Splinter Review | |
15.51 KB,
patch
|
mikedeboer
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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).
Updated•10 years ago
|
Whiteboard: [ux]
Updated•10 years ago
|
Whiteboard: [ux]
Updated•10 years ago
|
Whiteboard: p=8
Updated•10 years ago
|
Flags: firefox-backlog+
Updated•10 years ago
|
Priority: -- → P2
Target Milestone: --- → mozilla34
Updated•10 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Whiteboard: p=8 → p=8 [qa+]
Updated•10 years ago
|
Iteration: --- → 34.1
Points: --- → 8
Whiteboard: p=8 [qa+] → [qa+]
Updated•10 years ago
|
Whiteboard: [qa+] → [qa+, contacts]
Target Milestone: mozilla34 → mozilla35
Updated•10 years ago
|
Whiteboard: [qa+, contacts] → [qa+, contacts] p=8
Updated•10 years ago
|
Whiteboard: [qa+, contacts] p=8 → [qa+, contacts]
Comment 1•10 years ago
|
||
...running in a ChromeWorker with in-memory data and real-time index mutations.
Comment 2•10 years ago
|
||
Rebased on top of patches in bug 1038716 (required)
Attachment #8461456 -
Attachment is obsolete: true
Updated•10 years ago
|
Whiteboard: [qa+, contacts] → [qa+, contacts][first release needed]
Updated•10 years ago
|
Priority: P2 → P1
Updated•10 years ago
|
Whiteboard: [qa+, contacts][first release needed] → [p=8, qa+, contacts][first release needed]
Updated•10 years ago
|
Whiteboard: [p=8, qa+, contacts][first release needed] → [p=8, contacts][first release needed][qa+]
Updated•10 years ago
|
Priority: P1 → P3
Target Milestone: mozilla35 → mozilla34
Updated•10 years ago
|
Iteration: 34.1 → 34.2
Comment 3•10 years ago
|
||
Un-assigning, because it's unlikely that I'll be working on this before my PTO starts.
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Updated•10 years ago
|
Iteration: 34.2 → ---
Updated•10 years ago
|
Whiteboard: [p=8, contacts][first release needed][qa+] → [p=8, contacts][desired for first release needed][qa+][not needed for Fx34]
Updated•10 years ago
|
Flags: qe-verify+
Updated•10 years ago
|
Priority: P3 → --
Target Milestone: mozilla34 → mozilla35
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Comment 6•10 years ago
|
||
What shall we do about tests?
Assignee | ||
Comment 7•10 years ago
|
||
(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•10 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(" ");
Updated•10 years ago
|
Iteration: --- → 35.3
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0123a81972b2
Assignee | ||
Comment 11•10 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]
Comment 12•10 years ago
|
||
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]
Updated•10 years ago
|
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]
Comment 14•10 years ago
|
||
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•10 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)
status-firefox35:
--- → fixed
QA Contact: anthony.s.hughes → paul.silaghi
Comment 16•10 years ago
|
||
Based on comment 15, verified fixed 35.0a1 (2014-10-07) Win 7, Ubuntu 13.04, OS X 10.10
Status: RESOLVED → VERIFIED
Comment 17•10 years ago
|
||
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?
Updated•10 years ago
|
status-firefox34:
--- → fixed
Flags: needinfo?(paul.silaghi)
Comment 19•10 years ago
|
||
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+
Assignee | ||
Comment 22•9 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.
Description
•