Closed Bug 983777 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

VERIFIED FIXED
1.4 S4 (28mar)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: croesch, Assigned: arcturus)

References

()

Details

(Keywords: regression, Whiteboard: dogfood1.4)

Attachments

(3 files)

Attached file 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
Whiteboard: dogfood1.4
Video coming up.
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.
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
blocking-b2g: --- → 1.3?
QA Contact: bzumwalt
Possibly dup of bug 983480 (investigating)
(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).
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.
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.
This was broken by the changes in bug 972600.
Blocks: 972600
triage: 1.3+ regression

Francisco, can you please take a look? Thanks
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(francisco.jordano)
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.
Assignee: nobody → francisco.jordano
Flags: needinfo?(francisco.jordano)
(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 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.
Attachment #8392852 - Attachment description: WIP (needs unit testing) → PR ready for review
Attachment #8392852 - Flags: review?(jmcf)
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 ;-)
This bug has integration test attached to them.
Flags: in-testsuite+
Attachment #8392852 - Flags: review?(jmcf) → review+
Duplicate of this bug: 983480
Target Milestone: --- → 1.4 S4 (28mar)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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]:
Attachment #8392852 - Flags: approval-gaia-v1.3?(fabrice)
Asking for verifyme in order to uplift to 1.3 safely.
Keywords: verifyme
Attachment #8392852 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
v1.3: 4982b2680645e1506d72159ae73b04b14718ee6b
v1.4: 4ab2166a1d8820ff4ca36bc99646ce1269888ec9
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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 ;-)
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
Keywords: verifyme
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!
Attachment #8392852 - Flags: review+ → review?(jmcf)
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
Attachment #8392852 - Flags: review?(jmcf) → review+
Attached file Pointer to PR 17493
Attachment #8395469 - Flags: review?(jmcf)
Comment on attachment 8395469 [details] [review]
Pointer to PR 17493

please land once Travis is green
Attachment #8395469 - Flags: review?(jmcf) → review+
Landed:

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

Again with a green travis:

https://travis-ci.org/mozilla-b2g/gaia/builds/21417661
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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]:
Attachment #8395469 - Flags: approval-gaia-v1.4?(fabrice)
Attachment #8395469 - Flags: approval-gaia-v1.3?(fabrice)
Attachment #8395469 - Flags: approval-gaia-v1.4?(release-mgmt)
Attachment #8395469 - Flags: approval-gaia-v1.4?(fabrice)
Attachment #8395469 - Flags: approval-gaia-v1.3?(release-mgmt)
Attachment #8395469 - Flags: approval-gaia-v1.3?(fabrice)
(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 on attachment 8395469 [details] [review]
Pointer to PR 17493

Lets get this verified on v1.4 before uplifting it to 1.3
Attachment #8395469 - Flags: approval-gaia-v1.4?(release-mgmt) → approval-gaia-v1.4+
Keywords: verifyme
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
Duplicate of this bug: 986418
Marking verified per comment 35
Status: RESOLVED → VERIFIED
Keywords: verifyme
Verified enough now for v1.3?
Flags: needinfo?(bbajaj)
Attachment #8395469 - Flags: approval-gaia-v1.3?(release-mgmt) → approval-gaia-v1.3+
Flags: needinfo?(bbajaj)
This has some non-trivial conflicts with v1.3.
Flags: needinfo?(francisco.jordano)
Taking a look, thanks!
Flags: needinfo?(francisco.jordano)
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.
You need to log in before you can comment on or make changes to this bug.