875159 - [B2G] [Inari] [Contacts] Wrong message is displayed when no Gmail contacts are available to import

Status: RESOLVED FIXED

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

Description

[Deepa Subramanian]

Description:
When the user has no Gmail contacts available to import, the user must be prompted with correct message "No contacts available to import".

Prerequiste: No contacts in Gmail 

Repro Steps:
1) Updated to Unagi Build ID: 20130515070208
2) Go to Contacts App
3) Click on Settings
4) Click on Gmail button
5) Sign in with valid credentials

Actual: Wrong message is displayed "All your friends are imported" 

Expected: Correct message is displayed to inform  the user, that there are no contacts available to import

Environmental  Variables:
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/d06cfe7d67c2
Gaia: 0ddb515f15cbc6b74fc2742b7599d6ae74c6413f
Version:18.0

Notes:
Repro frequency: 100%
Test Suite Name: Contacts
UCID:OWD-23754
Link to failed test case: https://moztrap.mozilla.org/runtests/run/1301/env/315/?pagenumber=1&pagesize=20&sortfield=order&sortdirection=asc&filter-id=7073
Q Analysts Test Team Priority: Pri 3
See attached screenshot and logcat for more info.

Comment 1

[Deepa Subramanian]

Attachment: Screenshot - import Gmail contact

Comment 2

[Deepa Subramanian]

Attachment: logcat

Comment 3

[Jose Manuel Cantera]

not blocking but worth tracking it IMHO

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

In that case putting in c= to get this on product backlog list.

Comment 5

[sarsenyev]

Attachment: screenshot

Having a similar issue
After logged in to Hotmail account and imported all contacts, and then logged out, and signed in with a different account without contacts, the information about the previous account is shown
Check the image attachment

Environmental  Variables:
Build ID: 20130610070206
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8e3f39363c54
Gaia: ce3b99781d182ad550a325206990c249b0dbcf0e
Platform Version: 18.0

Comment 6

[Germán Toro del Valle (:gtorodelvalle)]

Removing myself as the assignee since not already working on this bug ;-)

Comment 7

[robert.sajdok]

Attachment: Proposed fix.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8406889 [details] [review]
Proposed fix.

Thanks for looking at this Robert.  I like the new string text, but I don't think we want to drop the old text.  Ultimately, I think the code needs to distinguish between "no contacts at all" and "found some contacts, but they are imported already".

So I think we want a new string defined in facebook.en-US.properties.  Maybe something like |fbNoFriends = No contacts available to import|.

Then in the importers |markExiting()| function you can check to see if |myFriends.length < 1| and then set fbNoFriends string immediately.

  https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/importer_ui.js#L462

How does that sound?  Thanks again!

Francisco, does this sound correct to you?

Comment 9

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

Comment on attachment 8406889 [details] [review]
Proposed fix.

Hi,

Thanks for the patch, as Ben comments, is better to differentiate when you consciously don't import any, or there are no contacts to import.

A separate string and a bit of code and unit tests will be kindly appreciated :)

Thanks!

Comment 10

[robert.sajdok]

Regarding this issue. If the contacts are imported the method "markExisting" is not called.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

I'm not very familiar with the internals of the importer.  Cristian, does comment 10 look like a problem>

Comment 12

[Jose Manuel Cantera]

(In reply to robert.sajdok from comment #10)
> Regarding this issue. If the contacts are imported the method "markExisting"
> is not called.

Mark existing will mark those contacts that are already imported. At some point in the code (probably on the friendsReady function) you will need to detect when the number to import is equal to the number of already imported

Comment 13

[robert.sajdok]

Attachment: Proposed fix.

Comment 14

[robert.sajdok]

I don't understand the idea of making tests for it. I can't see tests for markExisting functions where I've made changes. Is there a place for the test for markExisting function? Please, give me some advice or show me the place for tests of this code.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8423186 [details] [review]
Proposed fix.

The code looks good here, but I do think we need a test.  Perhaps you can expand this test case or another one in this file?

  https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/test/unit/import/import_test.js#L160

Basically, perform the import and then look for the particular message string in the resulting DOM.

Sorry for the delay on the review.  I have been traveling this week.

Thanks again for your help with this Robert!

Comment 16

[robert.sajdok]

Ok, I will try.
Should I make the different commit for this test or Should it be in the same commit as correction to this bug?

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

(In reply to robert.sajdok from comment #16)
> Ok, I will try.
> Should I make the different commit for this test or Should it be in the same
> commit as correction to this bug?

Please put them in the same commit.  You can use:

  git commit --amend

To update your current commit.  You'll need to force push to the github branch to update the PR, then.

Thanks!

Comment 18

[robert.sajdok]

I am not sure  how to write this test. I think the MockConnector.listDeviceContacts should be empty. Because my function concerns import without contacts. Please check my test function and if it is necessary show me the right direction.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Sorry for the delay in responding.  I was on vacation and then busy trying to catch up last few days.

I think you want something like:

  assert.isTrue(document.getElementById('friends-msg').textContent === 'No contacts available to import');

In your test to verify the correct message text is set.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8423186 [details] [review]
Proposed fix.

Please reflag once the test verifies the message string.  Otherwise this looks good.  Thanks!

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8423186 [details] [review]
Proposed fix.

Thank you Robert!  I really appreciate your hard work and persistence on this. r=me

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Robert, can you rebase this to most recent master and then add checkin-needed keyword to get it landed?

Thanks again.

Comment 23

[robert.sajdok]

> Robert, can you rebase this to most recent master 
Done

Please explain me the second part of your comment "add check in-needed keyword"

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Thanks Robert.

Just by way of explanation, I was going to merge your PR for you, but since it needed to be rebased I thought it would be better to use checkin-needed.  That way you wouldn't need to wait for me if I wasn't around.  The checkin-needed keyword is a flag for the sheriffs to land the patch for you once the tests turn green.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/07cb6457379308e1afe3cfc52af3bd633c159bfc

Comment 26

[Francesco Lodolo [:flod]]

Putting a string between the plural form declaration and the actual plural forms is quite confusing. No actual harm done, just a reminder if you want to fix it here in a follow-up commit.

 fbFriendsFound         = {[ plural(numFriends) ]}
+fbNoFriends            = No contacts available to import
 fbFriendsFound[zero]   = All your friends are imported
 fbFriendsFound[one]    = Found one friend to import

Comment 28

[KTucker [:KTucker][Inactive 3/4/2016]]

This issue is still occurring on the Flame 2.0

2.0 Environmental Variables:
Device: Flame 2.0
Build ID: 20140625000201
Gaia: de77f794db22a45f9d575de2c6e266a30a50de3b
Gecko: 79712bd7b60d
Version: 32.0a2 (2.0)
Firmware Version: v121-2
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0 

Can you look at this Robert?

Comment 29

[robert.sajdok]

Yes, I am going to look at it.

Comment 30

[robert.sajdok]

Yes, I am going to look at it.

Comment 31

[robert.sajdok]

I have not got my own Flame. I  bought one but I've been still waiting for a delivery.                                                        
I tested this bug on today's version of gaia on local machine with firefox and I can't see the problem you described.                       
I have gmail account without contacts and after importing from this account I get the message "No contacts available to import"             
In my opinion everything is ok.

Comment 32

[Roland Kunkel [:RolandK]]

(In reply to robert.sajdok from comment #31)

Hello,

I am still experiencing this on the latest 2.0 Flame build:

Device: Flame 2.0
BuildID: 20140730000206
Gaia: 2e85678de2c8e13e585288d4cec7d6673cee17ee
Gecko: 6e37ecf873da
Version: 32.0 (2.0)
Firmware: V122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

When logging into an account without contacts, I am receving the prompt that "All your friends are imported"

Comment 33

[KTucker [:KTucker][Inactive 3/4/2016]]

At this point let's write up a new issue.

Comment 34

[Roland Kunkel [:RolandK]]

Issue is rewriten: https://bugzilla.mozilla.org/show_bug.cgi?id=1047710

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

As far as I can tell this just needs to be uplifted, no?  It only landed v2.1.

Comment 37

[robert.sajdok]

Yes I see the problem in v2.0 version of gaia. I do not know what exactly I should do with it? Should I make patch for v2.0 version or what?

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

(In reply to robert.sajdok from comment #37)
> Yes I see the problem in v2.0 version of gaia. I do not know what exactly I
> should do with it? Should I make patch for v2.0 version or what?

No.  As Francesco indicated in bug 1047710 comment 3, we can't uplift a bug with string changes to v2.0 at this point in the release cycle.  There would not be enough time to do l10n translations, etc.

So I don't think there is anything to be done for this right now.

Thanks!

Comment 39

[Paladin]

Attachment: Verify_image.png

This issue has been verified successfully on Flame 2.1

See attachment: Verify_image.png
Reproducing rate: 0/5
Flame 2.1 build:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0