1022115 - [Contacts] Cannot tap and see contact detail after receiving (more than 1) contacts via bluetooth

Status: VERIFIED FIXED

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

Description

[Eric Chang [:ericcc] [:echang]]

### Build Information
Device: Flame
Gaia      908f94fda04462001ece86e6b6c15ad8b05f7526
Gecko     https://hg.mozilla.org/mozilla-central/rev/5e8e9d864a4d
BuildID   20140605160202
Version   32.0a1
ro.build.version.incremental=94
ro.build.date=Tue May 20 09:29:20 CST 2014

### STR
1. Send 2+ contacts to phone via bluetooth.
2. Accept the incoming request.
3. Tap the received vcard, complete the import process.
4. <ISSUE> Cannot tap any entry to see contact details(either newly added or existing)  
5. Tap "+" and cancel can solve this.   
6. http://youtu.be/lqxY0IEsp5E
7. Contacts -> Settings -> Import contacts won't have this problem.

### Reproduction Frequency: 
100% on Flame v2.0

Comment 1

[Jason Smith [:jsmith]]

QA Wanted to check 1.4.

Comment 2

[demerick]

This issue does not reproduce on Flame 1.4

Environmental Variables:
Device: Flame 1.4
Build ID: 20140609000201
Gaia: 8b239e41bbd85aa7b6a2c5d388e775ba7de6fb2b
Gecko: e3f85877db29
Version: 30.0 (1.4) 
Firmware Version: v10G-2

The user was able to tap any entry and see the contact details with no issues after initiating and completing a bluetooth export of contacts from another device to the device under test.

Comment 3

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

Adding to this sprint, we should have cycles :D

Comment 4

[Fernando Campo (:fcampo)]

Attachment: Link to PR - https://github.com/mozilla-b2g/gaia/pull/20392

First patch version, feedback needed.

Problem here is that when we open the app as an activity, code doesn't allow inner navigation. Patch solve the issue of entering contact details, but arise a new whole problem, this is: until which extent should we allow meddling around the app when we are just importing from BT (activity)? 

With this patch, we can see details, edit them, or create a new contact. But the moment that we exit one of the screens (cancel, done), the app is closed without explanation.

I'd like to have a clearer view of it, from the contact's people (and maybe someone from UX?).

Comment 5

[Jose Manuel Cantera]

Comment on attachment 8438809 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/20392

I think Francisco has more context than me in the activity handling code. 

Thanks

Comment 6

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

Hi Carrie,

we need your input here, right now when importing from bluetooth a vcard with a single contact we launch a web activity, import the contact and move to the contact details. When pressing back, we finish the activity.

Thanks ok, the problem is when the vcard has more than 1 contact, where we stop in the list. That's by design, I mean, this is not a bug, when we have multiple contacts on a vcard we just saw the contact list but don't allow to enter the contact details, you stay on the list until you close the activity (there is a close button).

My question is, if we allow to navigate, and allow to use the contacts app once we open it from an activity, we could fall into memory problems and break the navigation if we start opening other activities (like sending sms, email, etc.), so do you think the user should be free to use the contacts app once imported, or do you prefer to keep the isoloation when importing from bluetooth more than 1 contact.

Thanks

Comment 7

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

(In reply to demerick from comment #2)
> This issue does not reproduce on Flame 1.4
> 
> Environmental Variables:
> Device: Flame 1.4
> Build ID: 20140609000201
> Gaia: 8b239e41bbd85aa7b6a2c5d388e775ba7de6fb2b
> Gecko: e3f85877db29
> Version: 30.0 (1.4) 
> Firmware Version: v10G-2
> 
> The user was able to tap any entry and see the contact details with no
> issues after initiating and completing a bluetooth export of contacts from
> another device to the device under test.

This must be the same behavior in 1.4, the key point is exporting more than 1 contact, and being careful that the contacts are not merged.

Taking into account that, is the desired behavior.

IMHO, this shouldn't be a blocker, as it's working as it was designed, but I understand that we would like to explore better solutions for 2.1

@Joe wdyt?

Comment 8

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

Comment on attachment 8438809 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/20392

Not leaving feedback for this patch since I prefer that Carrie gives us her opinion first.

Thanks.

Comment 9

[Jason Smith [:jsmith]]

I don't see how this is desired behavior if you are ending up in a situation with a UI that can't be interacted with. The video shows this clearly that user can't do anything with the contacts seen here, which is broken UX.

Comment 10

[Duane Dixon [:ddixon]]

Unable to provide a Central Regression Window (Buri device).

The Last Working build is from 4/6/2014, however, I am blocked by another issue and cannot find a reasonably small window.  

The blocker I encounter allows my devices to connect via bluetooth, transfer multiple files, BUT the transferred files never appear on the device they were sent to.  

Further, my Last Working build and First Broken build are 22 days apart.  

Last Working

Device: Buri Master
Build ID: 20140406100626
Gaia: f1a98bfaa3ab2480945bd7018831fd56c61cdc24
Gecko: 5405d6f4e3c6
Version: 31.0a1 (Master) 
Firmware Version: v1.2device.cfg

First Broken

Device: Buri 2.0
Build ID: 20140428162136
Gaia: 725a23802708eb70e3d7e8a2ce7179adbac806e4
Gecko: d7c07694f339
Version: 32.0a1 (2.0) 
Firmware Version: v1.2device.cfg


The blocking issue I encounter also occurs in the earliest Central Flame build we have access to. 

Device: Flame Master
Build ID: 20140417000006
Gaia: 7591e9dc782ac2e97d63a96f9deb71c7b3588328
Gecko: e71ed4135461
Version: 31.0a1 (Master) 
Firmware Version: v10G-2

Comment 11

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

Hi Francisco, 

My expected behavior will be that users can tap the contact and see the details. The UI is totally the same with what we have in Contact APP. So users will definitely want to check these new-imported contacts.
If this was the design from the previous UX, then maybe it's not a bug and we can provide better solution later. However, when I imported (via BT) the same contacts twice,it didn't show the info of "contacts merged" in this case. I think this might be a bug? Thanks!

Comment 12

[Fernando Campo (:fcampo)]

(In reply to Carrie Wang [:carrie] from comment #11)
> Hi Francisco, 
> 
> My expected behavior will be that users can tap the contact and see the
> details. The UI is totally the same with what we have in Contact APP. So
> users will definitely want to check these new-imported contacts.
> If this was the design from the previous UX, then maybe it's not a bug and
> we can provide better solution later.
My concern, and question here, is inner navigation. With the patch applied, we are indeed able to see the contact details when pressing on a contact while on the list, and go back to the list after. The problem now is, the rest of options are also available (edit contact, create new contact, settings), and we can open them. But, when going back from those side actions, the application is closed (as being opened as an Activity from System).
Also, from details screen, user is able to tap on sms (or dialer), and start a new activity, which can derive in activities inside activities loop.
Apart from that, the only 'normal' option to exit back to the system is to press home button, even if it is an activity, as there is no 'back' or 'exit' button on the UI, but this is independent problem, and probably a different bug.

So going back to the point, my questions are:
- should we limit the functionality of contacts when opening as an import activity? (disabling buttons, showing messages to the user explaining it was just an import, etc)
- should we NOT close the activity when going back from side actions (full access), or this behaviour is the expected?

Comment 13

[Fernando Campo (:fcampo)]

I hate forgetting to ni? :D

Comment 14

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

Ok,

Carrie's point sounds good to me, but as Fernando comments above, we should think about not to close the activity when pressing back.

Technically now that's an inline activity, with an expected result, we should probably change as well that.

Comment 15

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

Per comment 2 and comment 12, I think the issue on "user not able to tap-n-see any entry" is likely unexpected.

Comment 16

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

Sorry, just spoke with Carrie. She'll update the expected behavior.

Comment 17

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

Thanks!

Comment 18

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

Hi Fernando, 

I'd like to provide the design details of a better UI. My expectation will be removing the side actions (ADD/ Settings) on the contact list here and also display only the received contacts here, not all the contacts from Contact APP. However, if users tap a name, they can see the contact details and all the functions on that page shouldn't be disabled. So these in-line activities inside activities loop are something feasible? In addition, can we provide an "x" on the left top of the received contact list to close it and get back to the system rather than tapping the Home button? 

I'll provide the wire-frame if this is doable. Thanks!

Comment 19

[Fernando Campo (:fcampo)]

(In reply to Carrie Wang [:carrie] from comment #18)
> Hi Fernando, 
> 
> I'd like to provide the design details of a better UI. My expectation will
> be removing the side actions (ADD/ Settings) on the contact list here
 this is doable :)

> and also display only the received contacts here, not all the contacts from
> Contact APP.
as far as I know, this is not possible. Opening the contacts app, either as an activity or as a full app, will trigger the contact list screen with all the existing contacts. If it's after import, then we'll show the existing contacts plus the new ones, but creating a new list just with the imported contacts is not possible at the moment, I'm afraid. Anyway, I'm deriving this question to Francisco, owner of the app, in case I'm wrong.

> However, if users tap a name, they can see the contact details
> and all the functions on that page shouldn't be disabled. So these in-line
> activities inside activities loop are something feasible? 
We can do it, till certain point (limited by memory usage mostly). I mean, if I'm on homescreen, and import some contacts from BT, we open Contacts as inline. There we can go to one specific contact, and try to send an sms (SMS app inline), and from there we can check the details of the contact again (here I don't know if we'd be going back, or opening another instance of contacts), and from here try to call (Dialer app). If, in the middle of the process, we run out of memory, my guess is that the apps will be closed without explanation, and not sure about the order. Also, going back from one of the instances might not go back to the previous, but to homescreen, as all of them are opened inline.
Again, requesting better informed people to confirm or give a better answer.

> In addition, can we provide an "x" on the left top of the received contact list to close it
> and get back to the system rather than tapping the Home button? 
yes, feasible too

> 
> I'll provide the wire-frame if this is doable. Thanks!

From my point of view, we are talking about different bugs in here:
- Being able to click on details
- 'X' to close the activity
- List with only the imported contacts
- Hide 'settings' / 'new contact' when opening for import.

Should we open bugs for each one of them, or solve all in here?

Comment 20

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

(In reply to Fernando Campo (:fcampo) from comment #19)
> 
> From my point of view, we are talking about different bugs in here:
> - Being able to click on details

- Doable, pretty simple, the only problem will be if we run out of memory the user will see how we end up in the homescreen since we could kill an app that is waiting for a result and then when going back we don't have that process anymore, so window manager will go to a safe place.

> - 'X' to close the activity

- Doable, I think we had it, to me this is a regression.

> - List with only the imported contacts

- Unfortunately not doable in 2.0 timeframe. Current contact list is quite complex and will require some major re-engineering to setup a list of custom contacts not coming from the API. Other solutions like import from FB, Gmail or Hotmail, don't use the proper contact list (a huge design mistake).

I'll add this as a requirement for future contacts, being able to use the list independently from the source, in this case the source will be a fixed set of contacts, the ones we imported.

> - Hide 'settings' / 'new contact' when opening for import.

Definitely doable, again we had that, this is a regression.

> 
> Should we open bugs for each one of them, or solve all in here?

I think the 3 items douable are kind of the same type, so we could do them in a single bug.

Best,
F.

Comment 21

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

Ok, I'll try to provide a solution base on the feasibility concern here and update here. Thanks! guys
ni? myself.

Comment 22

[Fernando Campo (:fcampo)]

Hi Carrie, just uploaded couple of videos showing how it works with the patch applied:

- Multiple contacts - https://www.youtube.com/watch?v=RN-e0UsKStg
- One contact - https://www.youtube.com/watch?v=5eHe6bhBUgM

if you need better quality (sorry about that, not a very good camera :S), or clarification about them, don't hesitate to ask.

Comment 23

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

Attachment: bug 1022115 receiving contacts via BT.pdf

Hi guys, 

I've attached the spec here. Please noted that I provide 2 proposals for multiple received contacts. One is displaying the whole contact list(p.5) and the other separates the new imported ones and the original ones(p.7). Please take a look and let me know from the feasibility perspective. Thanks!

ps. Vicky will provide the visual work here. Thanks!

Comment 24

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

Hei Carrie!

Thanks for the specs!

In terms of feasibility, option 2 is quite more complicated now and will require refactor in the contacts list, don't know if we will have time for the 2.0 release.

Option 1 is what Fernando has implemented so far on the videos. I guess we will need to add testing to land this.

Fernando thougs about this? Do you feel confident trying option 2?

Comment 25

[Fernando Campo (:fcampo)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #24)
> Hei Carrie!
> 
> Thanks for the specs!
> 
> In terms of feasibility, option 2 is quite more complicated now and will
> require refactor in the contacts list, don't know if we will have time for
> the 2.0 release.
> 
> Option 1 is what Fernando has implemented so far on the videos. I guess we
> will need to add testing to land this.
> 
> Fernando thougs about this? Do you feel confident trying option 2?

Agree with Francisco's comments, option 1 is what we have already on the patch. Option 2 would mean create a different list to show the imported contacts separated, so we go back to the situation described in comment 20 (not doable in this timeframe).

Only downside, not covered on the patch, would be to avoid any side actions on the details screen.
> From the specs:
> The tappable buttons and icons 
> on this page will be removed. 
> We only display contact’s 
> information here.
At the moment we don't have any mechanism to do this, so my proposal is to address the rest of the features on this patch (just need to clean the code and add tests), and create a followup to address this specific situation. I think this can be done on next sprint.

And also probably, another followup to be able to show different list on contacts app. (2.1 ?)

Comment 26

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

Per our discussion, we'll go for proposal one for now. So I think Vicky can start working on the visual design here. 
However, I still think proposal two is more user-friendly. So we will might need to put this into our future plan (different list on Contact). Thanks!

Comment 27

[Fernando Campo (:fcampo)]

Comment on attachment 8438809 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/20392

As we talked before, updated my PR with the agreed features and tests (please review), and opened new bugs for the rest:

Bug 1028114 - For showing an alternative list of contacts when importing
Bug 1028112 - For not allowing click actions when showing contact details on activities.

Comment 28

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

Comment on attachment 8438809 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/20392

Just went through the code and left a comment in github.

Mainly an structural change to make the code more readable. The rest is LGTM.

@FCampo, I understand from the last comment that we won't be adding the make them non clickable, do you think you'll have time in this sprint to try bug 1028112 ?

Thanks!

Comment 29

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

@Fcampo, you blocked here, anything else I can do to help?

Comment 30

[Fernando Campo (:fcampo)]

Sorry, working on it, will try to have it done byt the end of the day. Bad flight on tuesday and checkpoint yesterday, so not much time.
Nothing to do at all with any sidra.

Comment 31

[Fernando Campo (:fcampo)]

Comment on attachment 8438809 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/20392

Updated with comments addressed. I know that name for the new function may be a little meh, so feedback to change it is appreciated.

Comment 32

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

Comment on attachment 8438809 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/20392

Hei sorry Fernando,

but the PR needs rebase.

Thanks.

Comment 33

[Fernando Campo (:fcampo)]

Comment on attachment 8438809 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/20392

done :)

a little hell of a rebase, to be honest, but thankfully the new tests that came with it saved me from a regression :D

Comment 34

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

Comment on attachment 8438809 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/20392

Perfect, execellent job!

Thanks Fernando.

Comment 35

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

Just relaunched the marionette tests that were failing in travis.

In tbpl we had also a integration test failing:
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=8265afc6738732d240e75d54f78d3e2d9e699a4f

Relaunching as well.

Comment 36

[Fernando Campo (:fcampo)]

Green green green, master - 7c7ff70bb834407abd5623fde453d053c9d9bfbe

Thanks for the push francisco

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Needs a branch patch for v2.0 uplift.

Comment 38

[Fernando Campo (:fcampo)]

Attachment: v2.0 patch

Not sure about the policy in here, should I merge it directly to v2.0 or upload a patch in here.
Doing the latter just in case, don't want to break anything. 
But let me know if I should do otherwise.

Comment 39

[Fernando Campo (:fcampo)]

Attachment: v2.0 patch

ooops, wrong file :)

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8450225 [details] [diff] [review]
v2.0 patch

You can just go ahead and push yourself. Just set status-b2g-v2.0 to fixed when you do.

Comment 41

[Fernando Campo (:fcampo)]

so merged on v2.0 - 5725321dd1aef29077b6fc5c4c49b43dccf208dc

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Reverted on v2.0 for Gaia unit test failures.
v2.0: https://github.com/mozilla-b2g/gaia/commit/7cc838cb23376886f9b3243973152cc8a6f1f618

https://tbpl.mozilla.org/php/getParsedLog.php?id=43051738&tree=Mozilla-Aurora

Comment 43

[Victoria Gerchinhoren [:vicky]]

Attachment: Visual Mockup. Contact Details when importing from bluetooth

Comment 44

[Victoria Gerchinhoren [:vicky]]

Attachment: Visual Spec. importviabluetooth

Comment 45

[Fernando Campo (:fcampo)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #42)
> Reverted on v2.0 for Gaia unit test failures.
> v2.0:
> https://github.com/mozilla-b2g/gaia/commit/
> 7cc838cb23376886f9b3243973152cc8a6f1f618
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=43051738&tree=Mozilla-Aurora

Sorry for that, I missed some mocks needed for the tests. Hope now it works, passed all contacts tests on local.
v2.0 - 60b800bcf758a7a668218634a3957907051b2f80

Comment 46

[Eric Chang [:ericcc] [:echang]]

Verified okay with these 2 builds.
Flame - aurora - v2.0 
Gaia      e935f4ff190b76c70d9b2af8856c542a6e4a7546
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/3f9d7a3a0b7b
BuildID   20140707160206
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220

Flame - master - v2.1 
Gaia      1dc9e53393ae4680a174dffa44a958ec564ebbe8
Gecko     https://hg.mozilla.org/mozilla-central/rev/dfef245594b6
BuildID   20140707160202
Version   33.0a1
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220