972060 - [B2G][Dialer][Contacts] Imported Facebook contact does not show existing phone number when adding information via Dialer app

Status: RESOLVED FIXED

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

Description

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot - Annotated

Description:
After importing a Facebook contact with phone number associated to Contacts app, attempting to add number to this existing contact through the Dialer app results in missing information on Edit Contact screen. The phone number, picture, and email imported along with this contact are not visible while editing the contact, but after updating contact, this existing information is still visible through the contacts app along with all added information.

Repro Steps:
Prerequisite: Import Facebook contact with phone number associated
1) Updated Buri to BuildID: 20140212004003
2) Open Dialer app
3) Dial any number and press "add to contact" button
4) Select "Add to existing contact"
5) Choose imported Facebook contact with number associated

Actual:
Contact edit screen in dialer app is missing contact details for imported Facebook contacts.

Expected:
All existing information is visible while editing contact through dialer app.

Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20140212004003
Gaia: ce17d5eae7b1893ae4397c814b10ae598fcbdb58
Gecko: ab07e61c2eb0
Version: 28.0
Firmware Version: V1.2-device.cfg

Notes:
Repro frequency: 3/3, 100%
See attached: screenshot

Comment 1

[Brogan Zumwalt [Inactive]]

RE: Whiteboard tag - Found while running BuriRun 1.3-1 test case #7005

Comment 2

[Brogan Zumwalt [Inactive]]

1.3-3, not 1.3-1

Comment 3

[Jason Smith [:jsmith]]

What happens on 1.2?

Comment 4

[Anthony Ricaud (:rik)]

Is that a known limitation of edition of Facebook import?

Comment 5

[Ross Kuhlman ([e-mail])]

I am seeing the same results in v1.2. Facebook contact info is not displayed in the 'edit contact' page after adding a number from the dialer.

Environmental Variables:
Device: Buri v1.2 COM RIL
BuildID: 20140210004002
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: 2673f348598c
Version: 26.0
RIL Version: 01.02.00.019.102
Firmware Version: v1.2-device.cfg

If my from previous test runs is correct, there are situations when facebook details are deliberately not displayed on the edit details page for a contact, so this may be intentional.

Comment 6

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

Hi Anthony,

You are right, that's a limitation of the deal that we have with FB, a FB contact should not be editable.

What I'm seeing here is two problems:

- First a UX problem, what should we do when we try to add phone number from the dialer to contacts we cannot edit (should they appear in the list, should we show a message?)
- A proper bug, that allows us to edit the FB contact when is suppose not to be editable.

Flagging Ayman for some UX feedback and Jose for the FB integration problem.

Comment 7

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

@Ayman what we should do from a UX PoV?

@Jose I think we are failing in the FB contact edit, is this ok? There were some fields we cannot edit? Can we edit anything?

Thanks folks!

Comment 8

[ayman maat :maat]

(In reply to Francisco Jordano [:arcturus] from comment #7)
> @Ayman what we should do from a UX PoV?

This is partly a duplicate of another bug where i gave my opinion of this situation. let me try and find it.
leaving ni? to me

Comment 9

[ayman maat :maat]

(In reply to ayman maat :maat from comment #8)
> (In reply to Francisco Jordano [:arcturus] from comment #7)
> > @Ayman what we should do from a UX PoV?
> 
> This is partly a duplicate of another bug where i gave my opinion of this
> situation. let me try and find it.
> leaving ni? to me

Ok I cannot find the bug, but it was also complaining about Facebook information going missing when the contact goes into Edit mode.

The gist of what i wrote in reply to it was that I don’t understand and I never had understood why we hide Facebook information when the Contact goes into edit mode. When I first saw that design proposition I stated that it would be extremely disorientating for the end user to see information on the contact detail card and have it vanish in edit mode, and we are starting to get feedback that this is indeed the case.

I understand that we cannot edit Facebook data but I would still recommend that all data persisted in edit mode in the same order that it is presented in the contact detail card. What we should do is have two different visual treatments, one for local data and one for facebook data that is applied to both contact detail card and to edit mode to allow the user to make a connection between the data when it is displayed in both modes. However in edit mode local data should be editable and its presentation should afford that, whereas Facebook data should not be editable and again its presentation should afford that.

At this point this would be quite a bit of UX and VD work, but i absolutely believe it should be done because the current behaviour is not optimal for the end users experience of managing their contact information on the phone.

I am not certain whether i should handle this or whether we should handle it directly into Visual Design in view of the looming refresh

ni? to Noemi - what do you think?

Comment 10

[Noemí Freire (:noemi)]

Attachment: 2014-02-17-04-06-36.png

> At this point this would be quite a bit of UX and VD work, but i absolutely
> believe it should be done because the current behaviour is not optimal for
> the end users experience of managing their contact information on the phone.
> 
> I am not certain whether i should handle this or whether we should handle it
> directly into Visual Design in view of the looming refresh
> 
> ni? to Noemi - what do you think?
Hi,

Please find attached the current view for non-editable fields for a FB contact. Once the issue reported here is fixed it will be the way non-editable fields will appear when accessing through dialer path. ni to VD team (Victoria, José) to take a look at the way this is currently presented and check if it should be reviewed somehow as part of the visual refresh. Thanks!

Comment 11

[Jose Manuel Cantera]

Attachment: 16335.html

Comment 12

[ayman maat :maat]

(In reply to Jose M. Cantera from comment #11)
> Created attachment 8377021 [details]
> 16335.html

Thanks for the really quick response to this mr Cantera. I have tested add contact from:

- dialler
- message app
- email app

...and the FB information is presented now. I am still of the opinion that the visual design needs enhancing to improve understandability, but we should be able to address that in the Visual Refresh.

Comment 13

[ayman maat :maat]

...but for the moment, as an intermediate solution Jose's patch is good to land from my PoV

Comment 14

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

Comment on attachment 8377021 [details]
16335.html

Did the review, found a minor problem on the implementation, once solved please ask again for r?

Thanks!

Comment 15

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

Comment on attachment 8377021 [details]
16335.html

Perfect!

Nice work, please merge once travis is green.

Comment 16

[Jose Manuel Cantera]

Fabrice,

Can we land this? This is tiny patch but provides a good improvement in terms of UX consistency

thanks

Comment 17

[Anthony Ricaud (:rik)]

Definitely not without new tests.

Comment 18

[Jose Manuel Cantera]

(In reply to Anthony Ricaud (:rik) from comment #17)
> Definitely not without new tests.

Fair enough. 

For this case I think we are going to need marionnetteJS tests and it will take a bit more of time. My colleague Germán is currently investigating on that ;) 

thanks

Comment 19

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

Hi guys! Geez, this was harder than expected due to getting the integration test running in the Marionette JS Runner and the particularities of the Communications app... ;-9

I have a couple of branches with the proposed integration test with (https://github.com/gtorodelvalle/gaia/compare/contacts-bug-972060-integration-test?expand=1) and without (https://github.com/gtorodelvalle/gaia/compare/contact-bug-972060-jmcantera-patch?expand=1) José Manuel's patch. Of course, in the without case the integration test is not passing (since the Facebook contact phone and email are not included in the form when adding to the existing Facebook contact) whereas in the with José Manuel's patch case it does pass :-)

José Manuel, how would you like to proceed? Would you include the updated files (dialer.js, contacts.js and form_test.js, all under the Contacts app and the Dialer app marionette directories) in your patch and PR or do you prefer to add me as a collaborator to directly update your branch? In case you do it yourself, feel free to ask feedback or review from me ;-) Thanks!

Comment 20

[Jose Manuel Cantera]

Hi Germán,

I think the best way to proceed can be to open a new PR that includes both my commit and yours with integration test. Regarding the integration test I think we should move the Facebook data out of the test source code, including it in a different js or json file. That's the only improvement I see in your code.

Once it is done attach your new PR to the bug, then I will r it and finally if Travis green we will land. 

Many thanks!

Comment 21

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

Hi JM! Sadly there is a reason for that :D The current Marionette JS Runner fails when passing data as arguments to the scripts. It only works fine with primitive data types :-( I mean this line: https://github.com/gtorodelvalle/gaia/compare/contact-bug-972060-jmcantera-patch?expand=1#diff-f69c64f6d0232e3cc129b4183338e360R152

Anyhow, I think I could extract the data to a .js file which stores the data in the window.wrappedJSObject so the saveFBContact could take it form there. Not much cleaner but I think it may work. Do you want me to give it a try?

I will create a new PR including your commit and mine without squashing them.

Comment 22

[Jose Manuel Cantera]

ok, Germán. another option is to keep the data inside the test source but separate it inside an independent function. That maybe easier don't you think?

thanks

Comment 23

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

Attachment: 16946.html

Hey José Manuel! All your requests are covered in this new PR :p It includes 2 commits, yours and mine adding the integration test.

Comment 24

[Jose Manuel Cantera]

Comment on attachment 8386682 [details]
16946.html

thanks German, lands once Travis is green

Comment 25

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

Aaand merging: https://github.com/mozilla-b2g/gaia/commit/3146f89233046901ca22ced0968345e1eea84d33