1048160 - [Contacts Actionable Fields] Remove undo functionality

Status: RESOLVED FIXED

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

Description

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

Remove undo functionality

Comment 1

[Jose Manuel Cantera]

Attachment: 22570.html

Comment 2

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Comment on attachment 8468419 [details]
22570.html

Hi Jose Manuel, 

The tests were not passing for me. I put the ones that were erroring in the pull request.

Comment 3

[Jose Manuel Cantera]

Comment on attachment 8468419 [details]
22570.html

handing over review to Ben, as Sergi is on PTO and I would like to land this before going on PTO

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

So I looked at the patch, but I have to be honest, I'm not comfortable reviewing this code.  I'm not very familiar with this part of the app and, unfortunately, don't have a lot of time to become knowledgeable.  For example, its unclear to me why removing undo would mean renaming a bunch of things to be Facebook related instead.

Jose, I'm sorry, but I think it would be best to wait for Sergi to return.  Or perhaps Francisco could look when he returns (apparently in a day or two).

Comment 5

[Jose Manuel Cantera]

(In reply to Ben Kelly [:bkelly] from comment #4)
> So I looked at the patch, but I have to be honest, I'm not comfortable
> reviewing this code.  I'm not very familiar with this part of the app and,
> unfortunately, don't have a lot of time to become knowledgeable.  For
> example, its unclear to me why removing undo would mean renaming a bunch of
> things to be Facebook related instead.
> 
> Jose, I'm sorry, but I think it would be best to wait for Sergi to return. 
> Or perhaps Francisco could look when he returns (apparently in a day or two).

no problem Ben

thanks!

Comment 6

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

Comment on attachment 8468419 [details]
22570.html

LGTM, not granting the r+ since I realised that we can remove empty fields and we won't have them back until we cancel or save the contact.

Comment 7

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

Comment on attachment 8468419 [details]
22570.html

After sync with Jose, we realised this is not a problem, since we always have a button to add a removed field.

r+

Comment 8

[Jose Manuel Cantera]

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

Comment 9

[Ed Morley [:emorley]]

Reverted for gaia-ui-test failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45935491&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45935746&tree=B2g-Inbound

https://github.com/mozilla-b2g/gaia/commit/19a082634783ef49f080c9332880dc83efb52387

These were also failing on the gaia-try push - why did you merge anyway?

Comment 10

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

I've found this PR introduces a small regression, always adding a empty organization value.

This was catched by a keyboard test (lucky us).

So I'll be adding a new PR, 99% of the code is the same, just fixing the part that we were regressing.

Comment 11

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

Attachment: Pointer to PR 22932

Comment 12

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Looks good, r+ for me

Comment 13

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

Landed:

https://github.com/mozilla-b2g/gaia/commit/9f9e2d948104e0173a7cb145b96661f80d55ff9a

Thanks!