983777 - [B2G][Contacts] Tapping the Favorite button on an imported facebook contact will break the Edit Contact button.

Status: VERIFIED FIXED

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

Description

[Cody Roesch [:croesch]]

Attachment: log.txt

Description:
 When a user imports a facebook contact, then taps on the Add as Favorite button on the Contact Details page, the Edit Contact button will not work anymore. The Edit Contact button works fine before pressing the Add as Favorite button.

Repro Steps:
1) Update Buri to Build ID: 20140314004002
2) Launch Contacts app.
3) Log in and Import Facebook contacts using Sync friends toggle.
4) Tap on an imported facebook contact.
5) Tap the Edit Contact button in the upper right corner to verify the button is working properly.
6) Scroll down to the bottom of the contact details page and tap on the Favorites button.
7) Try to tap the Edit Contact button again and notice the button has no functionality anymore.

Actual:
 Edit Contact button becomes unresponsive after tapping the Add to Favorites button.

Expected:
 Edit Contact button should be working as intended after tapping on the Add to Favorites button.

Environmental Variables
Device: Buri V1.4 Moz Ril
Build ID: 20140314040202
Gecko: https://hg.mozilla.org/mozilla-central/rev/fe40387eba1a
Gaia: ea9e23abea5933656555d849b922c8da7530c90b
Platform Version: 30.0a1
Firmware Version: v1.2-device.cfg

Notes:
Repro frequency: 100%
See attached: Video and Logcat

Comment 1

[Cody Roesch [:croesch]]

Video coming up.

Comment 2

[Cody Roesch [:croesch]]

Video URL http://youtu.be/5P0Wt1v9y24
In V1.1 and V1.2 this bug does not happen.
This is a serious bug that is tied into bug 983480 dealing with Add as Favorite button.

Comment 3

[Cody Roesch [:croesch]]

Forgot to add the V1.3 variables

Environmental Variables
Device: Buri v1.3 Moz Ril
Build ID: 20140314004002
Gecko: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/0479f5480378
Gaia: 932789749406a79c5630c27c724f1856bd3c3f10
Platform Version: 28.0
Firmware Version: v1.2-device.cfg

Comment 4

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

Possibly dup of bug 983480 (investigating)

Comment 5

[Jason Smith [:jsmith]]

(In reply to Francisco Jordano [:arcturus] from comment #4)
> Possibly dup of bug 983480 (investigating)

Are you sure? That bug deals with just the * being persisted, where as this is breaking a button to edit a contact (i.e. first is a VD regression, second is a ID regression).

Comment 6

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

Not exactly, what I'm seeing in bug 983480 is that we have a problem when saving the contact (when we setup the favorite/unfavorite), that is basically editing and saving, this bug.

Anyway, still need to dig more to be sure.

Comment 7

[Brogan Zumwalt [Inactive]]

1.3 Regression Window:

Tinderbox Last Working Build:
Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20140311133411
Gaia: 6b1180917fa4af4e4b7e31b51fc06a4bbaa2ad0f
Gecko: 6308c320a699
Version: 28.0
Firmware Version: v1.2-device.cfg


Tinderbox First Broken Build:
Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20140311141210
Gaia: bfc6b263d296bd32422164d8f42097a4dc52c39c
Gecko: ed1f29e2c4df
Version: 28.0
Firmware Version: v1.2-device.cfg


Last Working Gaia/First Broken Gecko: Issue Does NOT Reproduce
Gaia: 6b1180917fa4af4e4b7e31b51fc06a4bbaa2ad0f
Gecko: ed1f29e2c4df

First Broken Gaia/Last Working Gecko: Issue DOES Reproduce
Gaia: bfc6b263d296bd32422164d8f42097a4dc52c39c
Gecko: 6308c320a699

Issue appears to be Gaia related

Gaia Tinderbox Push Log: https://github.com/mozilla-b2g/gaia/compare/6b1180917fa4af4e4b7e31b51fc06a4bbaa2ad0f...bfc6b263d296bd32422164d8f42097a4dc52c39c

Unable to narrow regression window as Inbound builds are not available for this build range.

Comment 8

[Jason Smith [:jsmith]]

This was broken by the changes in bug 972600.

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+ regression

Francisco, can you please take a look? Thanks

Comment 10

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

Hei as commented in bug 983480, this is not just a regression from bug 972600, also if we backout that bug, this get fixed but the previous one doesnt.

Jose and me have been looking in a patch for solving both cases.

Submitting it now to review and check against travis.

Comment 11

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

Attachment: PR ready for review

Comment 12

[Jason Smith [:jsmith]]

(In reply to Francisco Jordano [:arcturus] from comment #10)
> Hei as commented in bug 983480, this is not just a regression from bug
> 972600, also if we backout that bug, this get fixed but the previous one
> doesnt.

There's only one patch in the regression range here that's contacts related - that's bug 972600. As far as I can tell, that gives pretty strong evidence that it's a regression from that patch. I don't see a strong reason why that wouldn't be the case here.

Comment 13

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

Comment on attachment 8392852 [details] [review]
PR ready for review

Now adding marionette test to do the integration when clicking in favorite and being able to go to the edit form.

Comment 14

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

Hi guys! I am OK with the patch but sadly I could only run tests in my device and I would like to have the merging contacts integration test passing which is currently skipped :( (see https://github.com/arcturus/gaia/blob/bug-983777/apps/communications/contacts/test/marionette/details_test.js#L37) I am currently taking a look at it but you have my blessing to merge Francisco' patch ;-)

Comment 15

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

This bug has integration test attached to them.

Comment 16

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

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/5325e5b1bac0cddc4567226f5191d68301a450a6

Comment 18

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

Comment on attachment 8392852 [details] [review]
PR ready for review

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 
bug 972600, but we had the problem before, with this patch it was just easy to spot the problem.
[User impact] if declined:
broken user experience
[Testing completed]:
on the way, requested VERIFYME
[Risk to taking this patch] (and alternatives if risky):
Simple patch, with integration tests, reviewed by 2 people working on contacts. Not risky.
[String changes made]:

Comment 19

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

Asking for verifyme in order to uplift to 1.3 safely.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

v1.3: 4982b2680645e1506d72159ae73b04b14718ee6b

Comment 21

[Ryan VanderMeulen [:RyanVM]]

v1.4: 4ab2166a1d8820ff4ca36bc99646ce1269888ec9

Comment 22

[Michael Henretty [:mikehenrty][:mhenretty]]

had to revert this, causing intermittents on master, and perma-red (due to a missing file) on 1.3/1.3t

v1.3t: d6818921929ca2a8e7caee735fb27a2c098793b7
v1.3: 5a7e28cf9755f64b19de0e7ef536c120d1ab07f2.
v1.4: 625330017b2c83a3a06ccfad0b3a1892c178256c
master: 2c06eb8cfc7eee1021c868a6a8b030242007b684

Comment 23

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

Hi Michael, i was getting intermittents before landing this bug in master (I had to run tavis 3 times for errors not related to this bug), anyway will try to take a look. 

Thanks F.

Comment 24

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

Francisco, I do not know if it could be related to this but you may want to have a look at bug 982260. Please, feel free to contact me for further details ;-)

Comment 25

[Michael Henretty [:mikehenrty][:mhenretty]]

Also, when re-landed this on 1.3, make sure you uplift the facebook_contact_data.js file. The test depends on this, but it doesn't currently exist in 1.3 AFAICT.

https://github.com/mozilla-b2g/gaia/pull/17288/files#diff-c7d78b9f6cc37597e9e16d8a06f85d9dR99

Comment 26

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

Comment on attachment 8392852 [details] [review]
PR ready for review

Changed the approach, since our integration tests seems that are causing errors, I keep the current patch, but created unit tests for checking the on contact change listener that we have in the contacts class.

Jose would you mind to take a look?

Thanks!

Comment 27

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

Comment on attachment 8392852 [details] [review]
PR ready for review

Sorry, my mistake I had to add a new PR since previous PR was closed

Comment 28

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

Attachment: Pointer to PR 17493

Comment 29

[Jose Manuel Cantera]

Comment on attachment 8395469 [details] [review]
Pointer to PR 17493

please land once Travis is green

Comment 30

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

Landed:

https://github.com/mozilla-b2g/gaia/commit/32078fe945de8001765526fa7e8e1283d3c09669

Again with a green travis:

https://travis-ci.org/mozilla-b2g/gaia/builds/21417661

Comment 31

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

Comment on attachment 8395469 [details] [review]
Pointer to PR 17493

We already got the 1.3+ for the previous patch that was backout cause of problems with integration tests.

This patch is the same code, and changing integration tests for unit tests to avoid some timeouts with integration test and travis.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 32

[bhavana bajaj [:bajaj]]

(In reply to Francisco Jordano [:arcturus] from comment #31)
> Comment on attachment 8395469 [details] [review]
> Pointer to PR 17493
> 
> We already got the 1.3+ for the previous patch that was backout cause of
> problems with integration tests.
> 
> This patch is the same code, and changing integration tests for unit tests
> to avoid some timeouts with integration test and travis.
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #):
> [User impact] if declined:
> [Testing completed]:
> [Risk to taking this patch] (and alternatives if risky):
> [String changes made]:

 Francisco, can you please help fill all the inline details as requested in the approval request (eg : https://bugzilla.mozilla.org/show_bug.cgi?id=983594#c21 for what we are exactly looking for) next time you request approval. This time around I have gone through the bug comments and git PR and this looks good to land, but having this form filled really helps us a lot.

Comment 33

[bhavana bajaj [:bajaj]]

Comment on attachment 8395469 [details] [review]
Pointer to PR 17493

Lets get this verified on v1.4 before uplifting it to 1.3

Comment 34

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/00e4bdaa8488d3ec3952f67ee1d47c8b61d9e3c1

Comment 35

[Noemí Freire (:noemi)]

Hi,

It properly works for me on today's (3/25) master build:
Device: Hamachi
BuildId: 20140325064012
Gecko: b31f92b
Gaia: 5c0b527
Platform version: 31.0a1

Comment 37

[Jason Smith [:jsmith]]

Marking verified per comment 35

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Verified enough now for v1.3?

Comment 39

[Ryan VanderMeulen [:RyanVM]]

This has some non-trivial conflicts with v1.3.

Comment 40

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

Taking a look, thanks!

Comment 41

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

It's not trivial indeed!!

Oh gosh, just finished with the conflicts quick, but making the tests work took me a while in 1.3 .

Just giving some heads up, now doing for 1.3T before pushing both of them.

Comment 42

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

Landed in v1.3t:

https://github.com/mozilla-b2g/gaia/commit/ce34ecaed54f9a12476da078bed8625d4193a09f

Laned in v1.3:

https://github.com/mozilla-b2g/gaia/commit/5b1dc506333010b28eba0a1c955a2077b80f9e6c

Thanks folks