Closed Bug 1071500 Opened 8 years ago Closed 8 years ago

Facebook contacts with phone number cannot be set as ICE contacts

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.1+, b2g-v2.1 fixed, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S5 (26sep)
blocking-b2g 2.1+
Tracking Status
b2g-v2.1 --- fixed
b2g-v2.2 --- verified

People

(Reporter: crdlc, Assigned: crdlc)

References

Details

Attachments

(4 files)

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
Assignee: nobody → crdlc
Status: NEW → ASSIGNED
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
Flags: needinfo?(wmathanaraj)
Flags: needinfo?(oteo)
Flags: needinfo?(cawang)
Summary: Facebook contacts with phone number cannot be defined as ICE contact → Facebook contacts with phone number cannot be set as ICE contacts
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!
Flags: needinfo?(cawang)
QA Whiteboard: [ICE]
(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!
Flags: needinfo?(francisco)
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
Flags: needinfo?(wmathanaraj)
(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
Flags: needinfo?(wmathanaraj)
(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.
blocking-b2g: --- → 2.1?
(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.
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.
Flags: needinfo?(wmathanaraj)
Flags: needinfo?(oteo)
(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?
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.
Flags: needinfo?(francisco)
(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.
Flags: needinfo?(cawang)
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.
blocking-b2g: 2.1? → 2.1+
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!
blocking-b2g: 2.1+ → 2.1?
Flags: needinfo?(cawang)
OK if we can't make the string change in 2.1, I'm fine with it. thanks
(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
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?
Flags: needinfo?(jmcf)
Flags: needinfo?(cawang)
(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?
Flags: needinfo?(jmcf)
Attached file Github pull request
Comment on attachment 8495258 [details]
Github pull request

Ready for review
Attachment #8495258 - Attachment description: WIP - Github pull request → Github pull request
Attachment #8495258 - Flags: review?(jmcf)
Comment on attachment 8495258 [details]
Github pull request

nice work

plase land once we know TBPL is ok
Attachment #8495258 - Flags: review?(jmcf) → review+
Merged in master:

https://github.com/mozilla-b2g/gaia/commit/1bdf7c01628fced0eaccf9e9b4f8a562cf4369f8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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".
(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.
(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.
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.
blocking-b2g: 2.1? → 2.1+
Triage thinks thi is a blocker. In the worst case if a user has only facebook contact ICE will not be usable at all.
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.
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
Flags: needinfo?(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.
Flags: needinfo?(francisco)
Target Milestone: --- → 2.1 S5 (26sep)
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!
Flags: needinfo?(cawang)
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!
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
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
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!
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
(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
Flags: needinfo?(francisco)
Depends on: 1038701
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?
Flags: needinfo?(francisco) → needinfo?(jmcf)
(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!
Flags: needinfo?(jmcf)
Update my spec here as a reference. Thanks!
So this bug is wontfix for v2.1 if I'm understanding comment 39 correctly?
Flags: needinfo?(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?
Flags: needinfo?(crdlc)
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
(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
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [ICE] → [ICE] [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Whiteboard: [failed-verification]
QA Whiteboard: [ICE] [QAnalyst-Triage?] → [ICE] [QAnalyst-Triage?] [failed-verification]
Whiteboard: [failed-verification]
QA Whiteboard: [ICE] [QAnalyst-Triage?] [failed-verification] → [ICE] [QAnalyst-Triage+] [failed-verification]
Flags: needinfo?(ktucker)
Depends on: 1096626
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?
Flags: needinfo?(jmcf)
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
Flags: needinfo?(jmcf)
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
Flags: needinfo?(jocheng)
Attached video Verify_video.3gp
Attached file logcat_flame_0603.txt
Flags: needinfo?(jocheng) → needinfo?(hlu)
(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.
Flags: needinfo?(hlu)
You need to log in before you can comment on or make changes to this bug.