828732 - "Add to existing contacts" should not be available if no contacts exist

Status: RESOLVED FIXED

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

Description

[Vladan Djeric (:vladan)]

Steps to reproduce:

1. Go to your call history and click on a phone number without a contact associated.

Expect: Options to call or create a new contact
Actual: "Add to existing contacts" is also available despite 0 contacts in my address book

Comment 1

[David Scravaglieri [:scravag]]

Would take a patch for that.

Comment 3

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

I've made a patch: https://github.com/mozilla-b2g/gaia/pull/7773

Comment 4

[alberto.pastor]

Hi Sergi!

Thanks for the patch.
In my opinion, that would hurt Dialer loading performance (requesting contacts it's not cheap) just for 1 specific case, and taking in account how critical is Dialer loading time... I would prefer requesting a contacts count only when needed, as soon as Bug 832936 is fixed.
What do you think?

Comment 5

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi Alberto,

Yeah, I guess it makes sense. I was not aware that requesting contacts is expensive, specially when it looks like `find` is an async call.

Thanks for your reply!

Sergi

Comment 6

[alberto.pastor]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8704

Pointer to Github pull-request

Comment 7

[alberto.pastor]

As we don't have a fast way for retrieving the number of contacts, and given the case we are talking about is going to happen not that often (when you have the contacts list empty), we prefer don't delay every request. As an alternative solution, we will show an error message when entering in contacts through the activity.

Comment 8

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 726208 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8704

Pretty clean and safe code.

r+

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 726208 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8704

I found that string hard to understand without context, can you add a comment before the string like:

# LOCALIZATION NOTE(noContactsActivity): Shown when using the context menu to add a number from your call list to a contact, and your contacts list is empty.

r=me with that.

Comment 10

[alberto.pastor]

https://github.com/mozilla-b2g/gaia/commit/7ffcc10a695a357bee7794a84147a805b1cb0dfb

Comment 11

[Maria Angeles Oteo (:oteo)]

This patch is required to fulfill the Acceptance Criteria of the leo+ User Stories bug 838020 and 838021

Comment 12

[Lucas Adamski [:ladamski]]

Please nominate patch for approval instead and we'll take it.

Comment 13

[alberto.pastor]

Comment on attachment 726208 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8704

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: User can try to add to an existing contact when the list  is empty, what will end on a "No contacts screen" and without button for adding a new one. The only option is cancel the activity but there is no feedback.
Testing completed: Unit tests passing, and manual testing. Performance tests doesn't show any loose.
Risk to taking this patch (and alternatives if risky): low risk patch, only adding a check when in an activity and the list is empty for displaying a message.
String or UUID changes made by this patch: Added a new string for the new error message. Validated by [e-mail]

Comment 14

[Lucas Adamski [:ladamski]]

My mistake, I had missed that this is explicitly called out in the user acceptance criteria for the user story per https://bug838021.bugzilla.mozilla.org/attachment.cgi?id=727946.  Marking blocker in case of regression.

Comment 15

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

Uplifted commit 7ffcc10a695a357bee7794a84147a805b1cb0dfb as:
v1-train: 6efaeb9cc32b0086c6c010487ce30a3270b2cd5e

Comment 16

[William Hsu [:whsu]]

Adding a related test case.
 - https://moztrap.mozilla.org/manage/case/8467/

Comment 17

[Jason Smith [:jsmith]]

That should be a + if you confirmed adding a test case or confirmed there's already coverage. - is only used if the bug doesn't make sense to have test cases for.