1116889 - [Contacts] Changing Contact Photo in 'Edit Contact' but not Updating the changes will maintain the new Contact Photo between edit sessions.

Status: VERIFIED FIXED

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

Description

[Oliver Nelson [:oliverthor]]

Attachment: logcat_20141231_1602.txt

When the user is editing a contact and changes their 'Preview Image', they will observe that if they do not 'Update' their contact with their changes that returning to editing that contact will maintain the changed 'Preview Image'. The image can be saved to the contact if the user changes another field while editing their contact, otherwise the 'Update' button is not available.
The Preview Contact Photo will return to it's previous image if the application is restarted.
This is different behavior from the text fields within a contact, that upon exiting the 'Edit Contact' page without having updated, those changes will be dismissed and the text fields will return to their previous state (current contact details).
   
Repro Steps:
1) Update a Flame device to BuildID: 20141231010205
2) Open 'Contacts' app.
3) Create a new contact: Contact Photo required (additional text fields filled will help demonstrate issue).
4) Save contact.
5) Edit above contact.
6) Change Contact photo (and text fields for clarity).
7) Tap the 'X' in top left to leave edit mode without updating.
8) Edit the same Contact.
9) Observe Contact Photo (and 'modified' text fields).
  
Actual:
Contact Photo maintains the changed image from the previous 'Edit Contact' state, while text fields reset. Preview Contact Photo can be dismissed by restarting the application.
  
Expected: 
Edited Contact fields are reset if user chooses not to update.
  
******************************
Environmental Variables:
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141231010205
Gaia: 26d479f0fccb7174e06255121e4e938c1b280d67
Gecko: 88037f94b7d7
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (2.2 Master)
Firmware: V18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
******************************
  
This also occurs on Base Image v188-1

Repro frequency: 5/5
See attached: 
video- http://youtu.be/gxs6lyedwH0
logcat

Comment 1

[Oliver Nelson [:oliverthor]]

Issues DOES NOT REPRO on 2.1 flame devices:
Results: Edited Contact fields are reset if user chooses not to update.
Repro Rate: 5/5

********************************************
Environmental Variables:
----------------------------------------------
Device: Flame 2.1
BuildID: 20141231001232
Gaia: 73be51f998031f06db0cd660c0e388fa621c9f4c
Gecko: ea426e47bfc4
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 34.0 (2.1)
Firmware: V18D
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
----------------------------------------------

Comment 2

[Peter Bylenga [:PBylenga]]

Functional regression that exhibits unwanted change in data.

Requesting a window.

Comment 3

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

triage: regression

Comment 4

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20141120110801
Gaia: e327daf01fac1e5fbe2ba732c26e3a21db32443a
Gecko: a3b72fe67767
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20141120143805
Gaia: d695e7cdcd162e779e15594054931c84dec34a95
Gecko: 353f9d83297b
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Gaia & Last Working Gecko - issue DOES repro
Gaia: d695e7cdcd162e779e15594054931c84dec34a95
Gecko: a3b72fe67767

First Broken Gecko & Last Working Gaia - issue does NOT repro
Gaia: e327daf01fac1e5fbe2ba732c26e3a21db32443a
Gecko: 353f9d83297b

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare/e327daf01fac1e5fbe2ba732c26e3a21db32443a...d695e7cdcd162e779e15594054931c84dec34a95

Caused by the patch to Bug 1098218.

Comment 5

[KTucker [:KTucker][Inactive 3/4/2016]]

Jose, could you please take a look at this? Looks like the patch for Bug 1090210 caused this issue.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

I am already working on this

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Fix

This fixes the issue for me. I never looked into form.js before though... http://media.topito.com/wp-content/uploads/2013/01/code-29.gif  :_(

I'll add some tests tomorrow.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27351

Sorry for the late, I hit bug 1061698 while working on the tests.

Comment 9

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

Comment on attachment 8548233 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27351

Hi Fernando, can you rebase please?

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

I rebased, but it seems that TBPL is not happy with the new tests.

12:45:07     INFO -  TEST-UNEXPECTED-FAIL | apps/communications/contacts/test/marionette/form_test.js | Contacts > Form > Edit Contact > Discard edited contact photo Select photo and discard it
12:45:07     INFO -  NoSuchElement: (7) Unable to locate element: .thumbnail img

That means that we have no pictures on the gallery on TBPL. Any idea about how to fix this?

Comment 11

[Johan Lorenzo [:jlorenzo]]

Looking at your test, I don't think there is an image in the B2G internal memory. To make sure there are here, you need to push the image to B2G like they do in the Gallery tests: https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/test/marionette/delete_image_test.js#L21

Comment 12

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

Fernando do you need a hand with that test?

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Yes, any help is highly appreciated :) Thanks

I just tried another approach. I open the gallery app and wait for it to be populated with the test pictures before starting the test. Again, it works locally, let's see how it behaves in treeherder https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=477a54cf7310

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

It failed with the same error.

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Jonathan, I am not sure if you are the right person to ask about marionette issues in treeherder, but I was wondering if you could give me a hand here. The issue is that I have this marionette test that takes a photo from the gallery. It works great locally cause b2g desktop takes the pictures from the local disk (at least in OSX). But it fails in treeherder cause we have no pictures there apparently. So to be sure that we have at least one picture I add a test image from the test itself this way [1]. But still it says that it cannot find the photo in the gallery :( Any idea of what I might be doing wrong here?

Thanks in advance for any help. 

[1] https://github.com/ferjm/gaia/blob/bug1116889.contactphoto/apps/communications/contacts/test/marionette/form_test.js#L202

Comment 16

[Jonathan Griffin (:jgriffin)]

Fernando, I'm probably not the best person to help here.  The failing log is linux, not OSX...have you verified it works correctly for you on that platform?

You might ask James Lal to suggest someone who might know what's going on here.

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Thanks Jonathan. The test works locally on linux as well.

Comment 18

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

Should we land this?

Comment 19

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

Comment on attachment 8548233 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27351

Thanks Fernando

Comment 20

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/8731c8e4bada0822064403b3e639e3e8822dc304

Comment 21

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue is verified fixed in the latest Nightly Flame 3.0 build.  

Actual Results: The changes to the photo are not remembered between edits of the same contact.

Environmental Variables:
Device: Flame 3.0 KK (319 MB) (Full Flash)
BuildID: 20150310010227
Gaia: 2fb09da0cb9cefad9c6e40f57533fafda6d12557
Gecko: 6686aacf006f
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Comment 22

[KTucker [:KTucker][Inactive 3/4/2016]]

Adding verifyme to check 2.2 once uplifted.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please request Gaia v2.2 approval on this patch when you get a chance.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8548233 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27351

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Contacts edit form
[User impact] if declined: Confusing UX. The user needs to restart the app to revert changes done while editing a contact photo.
[Testing completed]: Manual tests, unit test added.
[Risk to taking this patch] (and alternatives if risky): No risk. The change is pretty specific to the issue.
[String changes made]: None

Comment 25

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/810c1ab43a00cce71637176187bc901c1d1ba8ed

Comment 26

[Shally Li（leave from Mozilla）]

This bug has been verified as "pass" on latest Nightly build of Flame v2.2 by the STR in Comment 0.

Actual results: Edited contact fields are reset as expected when user chooses 'X' icon instead of 'Update' icon.
See attachment: verified_v2.2.3gp
Reproduce rate: 0/10


Device: Flame v2.2 (Verified) 
Build ID               20150630162500
Gaia Revision          bd386f346eb1591fddbc84bf034b22700e7e2a58
Gaia Date              2015-06-30 15:53:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f16c1125b9d6
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150630.200238
Firmware Date          Tue Jun 30 20:02:49 EDT 2015
Bootloader             L1TC000118D0

Comment 27

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.2.3gp