1071500 - Facebook contacts with phone number cannot be set as ICE contacts

Status: VERIFIED FIXED

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

Description

[Cristian Rodriguez (:crdlc)]

STR:

1) Import a contact from Facebook with phone number
2) Active ICE Contact 1 in "ICE Contacts" view
3) Click on "Select a contact" button and choose the FB contact with phone number in the list

Expected:

The FB contact is defined as my new ICE contact 1

Current state:

A message "This contact doesn't have a phone number" is displayed

Comment 1

[Jose Manuel Cantera]

Here we have two options:

a/ do nothing and not allow users to set Facebook Contacts as ICE Contacts
b/ allow users to set FB Contacts as ICE Contacts (this is a trivial change) but in that case we must also modify the emergency call part in order to enable access to the FB Datastore (more changes needed). 

So for v2.1 my proposal would be to let things as they are. 

For v2.2 Wilfred, Maria Angeles, Carrie, what do you think? should we aim at fixing this limitation?

thanks

Comment 2

[Carrie Wang [New account: [e-mail]]]

My concern is that if we are not going to allow users select FB contacts as their ICE contacts, can we hide them from the contact picker so that users may not get confused? Thanks!

Comment 3

[Jose Manuel Cantera]

(In reply to Carrie Wang [:carrie] from comment #2)
> My concern is that if we are not going to allow users select FB contacts as
> their ICE contacts, can we hide them from the contact picker so that users
> may not get confused? Thanks!

Carrie,

I'm totally sympathetic to your concern but the same should be done with any contact not suitable for being an ICE Contact, namely Contacts without a tel number. (As you may recall in this case we show the user an error message indicating that the contact does not have a tel number). 

The approach of hinding not suitable contacts will make more difficult to implement the list rendering I believe. Francisco, what do you think?

thanks!

Comment 4

[Wilfred Mathanaraj [:WDM]]

I would say  - contacts that I can not choose to set as an ICE contact should either display an appropriate error message or not shown in the list to pick from.

But we can not now do this for 2.1 - and I would like this to be fixed till 2.2

Comment 5

[Jose Manuel Cantera]

(In reply to Wilfred Mathanaraj [:WDM] from comment #4)
> I would say  - contacts that I can not choose to set as an ICE contact
> should either display an appropriate error message or not shown in the list
> to pick from.

We are now showing an error message

> 
> But we can not now do this for 2.1 - and I would like this to be fixed till
> 2.2

Are you meaning that in 2.2 we should allow users to set Facebook Contacts as ICE Contacts?

Please clarify

Comment 6

[Johan Lorenzo [:jlorenzo]]

(In reply to Jose Manuel Cantera from comment #5)
> We are now showing an error message
As per the first message, we display "This contact doesn't have a phone number", right?

I think we shoule block on that issue. The UX is too confusing for a new user and can break the first time experience of that new feature. We need at least a more explicit error message.

[Blocking Requested - why for this release]: This is a confusing issue in a new feature which can confuse many Facebook users.

Comment 7

[Jose Manuel Cantera]

(In reply to Johan Lorenzo [:jlorenzo] from comment #6)
> (In reply to Jose Manuel Cantera from comment #5)
> > We are now showing an error message
> As per the first message, we display "This contact doesn't have a phone
> number", right?

Yup. We could certainly change it by something more clear and specific

> 
> I think we shoule block on that issue. The UX is too confusing for a new
> user and can break the first time experience of that new feature. We need at
> least a more explicit error message.
> 
> [Blocking Requested - why for this release]: This is a confusing issue in a
> new feature which can confuse many Facebook users.

Comment 8

[Wilfred Mathanaraj [:WDM]]

my point is facebook contacts are not needed to be set as ICE contacts. How we handle it can be in two ways:

1. display an error message that says you can not set this contact because its not allowed to be set.

2. Hide the contact that you can not set as ICE from the list of contacts when we pick IE contacts.

Comment 9

[Jose Manuel Cantera]

(In reply to Wilfred Mathanaraj [:WDM] from comment #8)
> my point is facebook contacts are not needed to be set as ICE contacts. How
> we handle it can be in two ways:
> 
> 1. display an error message that says you can not set this contact because
> its not allowed to be set.
> 
> 2. Hide the contact that you can not set as ICE from the list of contacts
> when we pick IE contacts.

Then my proposal here would be to provide a more accurate message to the user saying 'Facebook Contacts cannot be set as ICE Contacts'

Do everybody agree?

Comment 10

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

I do agree.

The cost of removing/hidding fb contacts from the list is high, since it will require recalculation of the groups. And the list wasn't designed to hide/show contacts (by type) at will.

We do have a pretty easy mechanism to filter the selected ice contacts (right now we are filtering contacts without phone number, and contacts already set as ICe), adding an extra filter to check that a FB contact cannot be set as ICE will be pretty straigh forward.

Comment 11

[Maria Angeles Oteo (:oteo)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #10)
> I do agree.
> 
> The cost of removing/hidding fb contacts from the list is high, since it
> will require recalculation of the groups. And the list wasn't designed to
> hide/show contacts (by type) at will.
> 
> We do have a pretty easy mechanism to filter the selected ice contacts
> (right now we are filtering contacts without phone number, and contacts
> already set as ICe), adding an extra filter to check that a FB contact
> cannot be set as ICE will be pretty straigh forward.

It's a pity that the string change can not land in 2.1 (too late after the string freeze) :(

For 2.2 the option commented by Francisco and Jose Manuel is fine with me, anyway regarding the wording, better to confirm it with Carrie or Matej because I am not sure if that message would be enough for the normal user (as he needs to create specific local contact to be able to include it in the ICE list). 

Setting ni to Carrie to review the wording and the flow.

Comment 13

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: major issue to new feature.
Open problem here is that 2.1 string already freeze. We may need a solution here.
For 2.1, we can use a general string.

Comment 14

[Carrie Wang [New account: [e-mail]]]

I think for now, we can't display "the contact doesn't have a phone number" in this case. It will be super confusing.
I'd suggest providing the string:
-----------------------------------------------
Facebook contacts can't be set as ICE contacts.

Thanks!

Comment 15

[Carrie Wang [New account: [e-mail]]]

OK if we can't make the string change in 2.1, I'm fine with it. thanks

Comment 16

[Jose Manuel Cantera]

(In reply to Carrie Wang [:carrie] from comment #14)
> I think for now, we can't display "the contact doesn't have a phone number"
> in this case. It will be super confusing.
> I'd suggest providing the string:
> -----------------------------------------------
> Facebook contacts can't be set as ICE contacts.
> 
> Thanks!

As per the L10N rules we cannot land new strings in 2.1. Nonetheless, we can implement Carrie's suggestion for v2.2

best

Comment 17

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

During traige I proposed two solutions here:

- 2.2: We go wieh the proper string
- 2.1: We reuse a string, that perhaps is not pretty accurate: ICEUnknownError = There was an error setting the contact as ICE

Carrie, Jose?

Comment 18

[Jose Manuel Cantera]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #17)
> During traige I proposed two solutions here:
> 
> - 2.2: We go wieh the proper string
> - 2.1: We reuse a string, that perhaps is not pretty accurate:
> ICEUnknownError = There was an error setting the contact as ICE

We can certainly do that but I don't like to show a generic error, the app seems buggy

> 
> Carrie, Jose?

Comment 19

[Cristian Rodriguez (:crdlc)]

Attachment: Github pull request

Comment 20

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8495258 [details]
Github pull request

Ready for review

Comment 21

[Jose Manuel Cantera]

Comment on attachment 8495258 [details]
Github pull request

nice work

plase land once we know TBPL is ok

Comment 22

[Cristian Rodriguez (:crdlc)]

Merged in master:

https://github.com/mozilla-b2g/gaia/commit/1bdf7c01628fced0eaccf9e9b4f8a562cf4369f8

Comment 23

[Francesco Lodolo [:flod]]

Nothing critical, but can we stop doing massive changes to .properties files just to align the equal sign? It makes patches unreadable, and in code they would be deemed as "unnecessary changes".

Comment 24

[Jose Manuel Cantera]

(In reply to Francesco Lodolo [:flod] from comment #23)
> Nothing critical, but can we stop doing massive changes to .properties files
> just to align the equal sign? It makes patches unreadable, and in code they
> would be deemed as "unnecessary changes".

well that's your personal opinion, but I would prefer to keep things properly indented, despite of making "unnecessary changes". Applying the same reasoning to code it would mean that we will never change original indentations when we find they are wrong our outdated.

Comment 25

[Francesco Lodolo [:flod]]

(In reply to Jose Manuel Cantera from comment #24)
> well that's your personal opinion, but I would prefer to keep things
> properly indented, despite of making "unnecessary changes". Applying the
> same reasoning to code it would mean that we will never change original
> indentations when we find they are wrong our outdated.

FYI moved discussion to dev-gaia. Personally I don't see any advantage in this kind of alignment and a lot of negatives, coding style guides like PEP8 agree with me, but I'd like to see what other Gaia devs think.

Comment 26

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

This is a major problem on a new feature, but we cannot take this patch to 2.1

Please ask for approval for 2.1 and provide a patch that doesn't make use of a new string. Despite that is not the best solution is better than having this error in 2.1 branch.

Comment 27

[David Scravaglieri [:scravag]]

Triage thinks thi is a blocker. In the worst case if a user has only facebook contact ICE will not be usable at all.

Comment 28

[Cristian Rodriguez (:crdlc)]

Hola Fran, what do you prefer? The same patch but with ICEUnknownError instead of ICEFacebookContactNotAllowed which lives in 2.2, right? Thanks a lot to clarify me this

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #26)
> This is a major problem on a new feature, but we cannot take this patch to
> 2.1
> 
> Please ask for approval for 2.1 and provide a patch that doesn't make use of
> a new string. Despite that is not the best solution is better than having
> this error in 2.1 branch.

Comment 29

[Cristian Rodriguez (:crdlc)]

Hi all,

   I cannot implement a patch because there is not anything related to error handling for version 2.1. See [1]. The ICE module in 2.1 does not check if a contact has a phone number or control unexpected errors. And no locales there sorry :(

[1] https://github.com/mozilla-b2g/gaia/blob/v2.1/apps/communications/contacts/js/views/ice_settings.js

Comment 30

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

So there is not much we can do there.

If those patches were not uplifted already, and we don't have any string that we can reuse, we are in the same case.

Comment 31

[Carrie Wang [New account: [e-mail]]]

So per comment30, we have nothing to do in 2.1 but we can provide a correct string in 2.2. Am I right? Thanks!

Comment 32

[Cristian Rodriguez (:crdlc)]

Yes, you are right https://github.com/mozilla-b2g/gaia/commit/1bdf7c01628fced0eaccf9e9b4f8a562cf4369f8#diff-a8e39eba575534c9d35a2b692b6550e1R187

(In reply to Carrie Wang [:carrie] from comment #31)
> So per comment30, we have nothing to do in 2.1 but we can provide a correct
> string in 2.2. Am I right? Thanks!

Comment 33

[Loli (:lolimartinezcr)]

Tested and working BUT I have a question when a FB contact (with phones numbers) is linked with other created contact this FB contact can be an ICE contact but *only* with phone number of created contact. Is this correct? 

Flame 
Eng
2.2
Gecko-a310d15
Gaia-931d547

Comment 34

[Cristian Rodriguez (:crdlc)]

Yes, you're right, linked or promoted contacts from FB can be ICE contacts but only with local phone numbers

(In reply to Loli (:lolimartinezcr) from comment #33)
> Tested and working BUT I have a question when a FB contact (with phones
> numbers) is linked with other created contact this FB contact can be an ICE
> contact but *only* with phone number of created contact. Is this correct? 
> 
> Flame 
> Eng
> 2.2
> Gecko-a310d15
> Gaia-931d547

Comment 35

[Loli (:lolimartinezcr)]

In 2.1 user can set (In reply to Cristian Rodriguez (:crdlc) from comment #34)
> Yes, you're right, linked or promoted contacts from FB can be ICE contacts
> but only with local phone numbers
> 
> (In reply to Loli (:lolimartinezcr) from comment #33)
> > Tested and working BUT I have a question when a FB contact (with phones
> > numbers) is linked with other created contact this FB contact can be an ICE
> > contact but *only* with phone number of created contact. Is this correct? 
> > 
> > Flame 
> > Eng
> > 2.2
> > Gecko-a310d15
> > Gaia-931d547

Thanks!

Comment 36

[Loli (:lolimartinezcr)]

In 2.1, FB contact can be an ICE contact but when user presses "Emergency call" button can't see FB contact. 
Flame
User
Gecko-9ba6d65
Gaia-13973ab

Comment 37

[Jose Manuel Cantera]

(In reply to Loli (:lolimartinezcr) from comment #36)
> In 2.1, FB contact can be an ICE contact but when user presses "Emergency
> call" button can't see FB contact. 
> Flame
> User
> Gecko-9ba6d65
> Gaia-13973ab

Yes, and that's an issue. We do need to upgrade to 2.1 but unfortunately we cannot due to l10N. However, I would urge to reconsider this situation ...

ni Francisco to know his opinion

Comment 38

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

Hi, I've been talking with several people trying to solve this.

The l10n team won't get more strings, so there is no way to solve this via new strings. We tried to reuse new strings but we don't have some of them (potentially to display the error) in 2.1

As having an alternative string is the only possible solution, so far, I've been talking with product and they agree in showing a message even if it's not completely translated (not the best solution, we all know, but the only way to unblock this situation and not have this error in 2.1).

Same can be applied to bug 1061411.

In this case we could reuse a facebook string: 'error_1':
error1          = Error {{from}}

where we can pass a parameter, I've checked with Wilfred and he is ok displaying this, so the message ({{from}}) can be the error kind. That message will appear just in english, but the 'Error' string will be transalated.

Again, not the perfect solution but bettern than nothing, product is happy to show that there is an error, but the message wasn't transalted.

Jose, what do you think?

Comment 39

[Jose Manuel Cantera]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #38)
> Hi, I've been talking with several people trying to solve this.
> 
> The l10n team won't get more strings, so there is no way to solve this via
> new strings. We tried to reuse new strings but we don't have some of them
> (potentially to display the error) in 2.1
> 
> As having an alternative string is the only possible solution, so far, I've
> been talking with product and they agree in showing a message even if it's
> not completely translated (not the best solution, we all know, but the only
> way to unblock this situation and not have this error in 2.1).
> 
> Same can be applied to bug 1061411.
> 
> In this case we could reuse a facebook string: 'error_1':
> error1          = Error {{from}}
> 
> where we can pass a parameter, I've checked with Wilfred and he is ok
> displaying this, so the message ({{from}}) can be the error kind. That
> message will appear just in english, but the 'Error' string will be
> transalated.
> 
> Again, not the perfect solution but bettern than nothing, product is happy
> to show that there is an error, but the message wasn't transalted.
> 
> Jose, what do you think?

In the particular case of this bug once bug 1061422 lands in v2.1 , we will be solving the FB ICE Contact problem as well, so nothing to be really done here

thanks Francisco for the efforts!

Comment 40

[Carrie Wang [New account: [e-mail]]]

Attachment: [2.1 Contacts] Support for ICE Contacts v0.3.pdf

Update my spec here as a reference. Thanks!

Comment 41

[Ryan VanderMeulen [:RyanVM]]

So this bug is wontfix for v2.1 if I'm understanding comment 39 correctly?

Comment 42

[Cristian Rodriguez (:crdlc)]

It has been fixed in bug 1061422 for 2.1

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #41)
> So this bug is wontfix for v2.1 if I'm understanding comment 39 correctly?

Comment 43

[Loli (:lolimartinezcr)]

Tested and now I can see a confuse hehavior, when i select a Facebook contact (with phone number inserted in FB) in ICE contact I can see "This contact does not have a phone number". I think it is confuse. Please, I need info UX.

Tested 
2.1
Flame
Gecko-b8db156
Gaia-86467df

Comment 44

[Jose Manuel Cantera]

(In reply to Loli (:lolimartinezcr) from comment #43)
> Tested and now I can see a confuse hehavior, when i select a Facebook
> contact (with phone number inserted in FB) in ICE contact I can see "This
> contact does not have a phone number". I think it is confuse. Please, I need
> info UX.
> 
> Tested 
> 2.1
> Flame
> Gecko-b8db156
> Gaia-86467df

That's because we were not allowed to add new L10N strings to v2.1 

thanks Loli

Comment 45

[[:RJ]]

This issue is verified on Flame 2.2 Master KK (319mb) (Full Flash).

When Facebook contact with phone number is being attempted to be set as an ICE contact the error "Facebook contacts cannot be set as ICE contact ".

Flame 2.2 Master KK (319mb) (Full Flash)

Device: Flame 2.2 Master
BuildID: 20141011040204
Gaia: 95f580a1522ffd0f09302372b78200dab9b6f322
Gecko: 3f6a51950eb5
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

This issue is not verified in 2.1 KK (319mb) (Full Flash).

The error message "this contact does not have a phone number" is still prompted when setting a facebook contact to an ICE contact, which has been noted with in this issue in comment 44 which states because of localization because we were not allowed to add new L10N strings to v2.1 , but with out it I can not verify it on the flame 2.1.

2.1 KK (319mb) (Full Flash)

Environmental Variables:
Device: Flame 2.1 KK (319mb) (Full Flash)
Build ID: 20141010000201
Gaia: d71f8804d7229f4b354259d5d8543c25b4796064
Gecko: 7fa82c9acdf2
Version: 34.0a2 Flame 2.1 KK (319mb)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 47

[Johan Lorenzo [:jlorenzo]]

Per comment 45 and bug 1097548, this issue still occurs on 2.1 even though bug 1061422 has landed. This issue is actually a WONTFIX in 2.1. Do you confirm, Jose?

Comment 48

[Jose Manuel Cantera]

Johan,

Strictly speaking can be considered a won't fix as in v2.2 we have an specific message for Facebook Contacts, 

'Facebook contacts cannot be set as ICE contacts'

However, in no version can FB Contacts can be set as ICE Contacts.

I hope this clarifies

Comment 49

[Paladin]

This bug has been verified to fail on Flame 2.1.

Issue steps: 
Set up: There is a facebook contact with phone number in Address Book.
  1) Open Contacts and tap gettings icon.
  2) Active ICE Contact 1 in "ICE Contacts" view
  3) Click on "Select a contact" button and choose the FB contact with phone number in the list.
** "The contact does not have a phone number" page pop up.

See attachment: Verify_video.3gp and logcat_flame_0603.txt
Reproducing rate: 5/5
Flame2.1 build:
Gaia-Rev        afdfa629be209dd53a1b7b6d6c95eab7077ffcd9
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/dc3018cbdbe6
Build-ID        20141123001201
Version         34.0

Comment 50

[Paladin]

Attachment: Verify_video.3gp

Comment 51

[Paladin]

Attachment: logcat_flame_0603.txt

Comment 52

[Hubert Lu[:hlu] <[e-mail]>]

(In reply to Paladin from comment #49)
> This bug has been verified to fail on Flame 2.1.
> 
> Issue steps: 
> Set up: There is a facebook contact with phone number in Address Book.
>   1) Open Contacts and tap gettings icon.
>   2) Active ICE Contact 1 in "ICE Contacts" view
>   3) Click on "Select a contact" button and choose the FB contact with phone
> number in the list.
> ** "The contact does not have a phone number" page pop up.
> 
> See attachment: Verify_video.3gp and logcat_flame_0603.txt
> Reproducing rate: 5/5
> Flame2.1 build:
> Gaia-Rev        afdfa629be209dd53a1b7b6d6c95eab7077ffcd9
> Gecko-Rev      
> https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/dc3018cbdbe6
> Build-ID        20141123001201
> Version         34.0

Hi Paladin,
    Please see comment 45 and comment 48.