Closed Bug 1048160 Opened 8 years ago Closed 8 years ago

[Contacts Actionable Fields] Remove undo functionality

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S2 (15aug)

People

(Reporter: arcturus, Assigned: jmcf)

References

Details

Attachments

(2 files)

Remove undo functionality
Target Milestone: --- → 2.1 S2 (15aug)
No longer blocks: 1048165
No longer blocks: 1048163
Blocks: 1043166
No longer blocks: 1048159
Assignee: nobody → jmcf
Attached file 22570.html
Attachment #8468419 - Flags: feedback?(sergi.mansilla)
Attachment #8468419 - Flags: feedback?(francisco)
Attachment #8468419 - Flags: feedback?(francisco) → feedback+
Attachment #8468419 - Flags: feedback?(sergi.mansilla) → review?(sergi.mansilla)
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.
Attachment #8468419 - Flags: review?(sergi.mansilla) → review-
Attachment #8468419 - Flags: review- → review?(sergi.mansilla)
Attachment #8468419 - Flags: review?(sergi.mansilla)
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
Attachment #8468419 - Flags: review?(bkelly)
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).
Attachment #8468419 - Flags: review?(bkelly) → review?(sergi.mansilla)
(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 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.
Attachment #8468419 - Flags: review?(sergi.mansilla)
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+
Attachment #8468419 - Flags: review+
https://github.com/mozilla-b2g/gaia/commit/f09ae1c020d88bb63f62ac499fe557700e8bb5fc
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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?
Status: RESOLVED → REOPENED
Flags: needinfo?(jmcf)
Resolution: FIXED → ---
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.
Attached file Pointer to PR 22932
Attachment #8473799 - Flags: review?(sergi.mansilla)
Looks good, r+ for me
Attachment #8473799 - Flags: review?(sergi.mansilla) → review+
Landed:

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

Thanks!
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Depends on: 1058312
Flags: needinfo?(jmcf)
Depends on: 1058927
You need to log in before you can comment on or make changes to this bug.