909201 - Editing names doesn't update properly

Status: RESOLVED FIXED

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

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Version 1.1.0.0-prerelease 20130816041203

Steps to reproduce:

* Search for a contact my first name
* Observe a list of names
* Choose one of the names
* Click on the edit icon
* Modify the surname
* Click on Update
* Click on <

Expected results:

The name is updated on the list

Actual results:

The old name is shown

Comment 1

[Teiichiro Fukuda]

"contacts.Search" is not updated, when "mozContacts.oncontactchange" is notified.
Therefore, the name seems old.

Comment 2

[Teiichiro Fukuda]

Attachment: update of a search list.pdf

I made the screen transition image of the search screen.
Please do a review of the user interface.("search screen")

Thanks.

Comment 3

[Stephany Wilkes]

Flagging Eric to review the attachment (search list PDF).

Comment 4

[robert.sajdok]

I am going to try fix it, please assign me to this bug.

Comment 5

[Eric Pang [:epang] UX]

Comment on attachment 825679 [details]
update of a search list.pdf

This looks good to me, thanks!

Comment 6

[Teiichiro Fukuda]

Hi Stephany,

Please assign it to robert if the result of the review is OK.

Thanks.

Comment 7

[robert.sajdok]

For me everything is clear. I've been working on it.

Comment 8

[Stephany Wilkes]

Hello - Eric has given the UI review and marked it review+ so that means it is OK. Please proceed (though it sounds like you already are). Thanks!

Comment 9

[robert.sajdok]

I made my patch for this bug. I am not sure what I should do next. How to ask someone to review my patch?
https://github.com/mozilla-b2g/gaia/pull/15280

Comment 10

[Manish Goregaokar [:manishearth]]

Assigned, as per request in IRC

Comment 11

[robert.sajdok]

Attachment: Bug 909201 proposed fix.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8367601 [details] [review]
Bug 909201 proposed fix.

Robert, this is a good start!  I made some comments in the Github PR about some ways we can improve the fix.

Also, I think we need a test to catch this kind of error in the future.  Can you add a new function to contacts/test/unit/view/list.js?  You should be able to copy an existing test as a start, like this:

  https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/test/unit/views/list_test.js#L1088

You can trigger a search, then use doRefreshContact() to update the contact, then verify the search shows the modified contact.

The only trick will be to know when the search update is complete.  A setTimeout(func, 0) may be adequate there.  (Generally, though we try not to use timeouts when possible in tests.)

To run the tests locally, see this page:

  https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests?redirectlocale=en-US&redirectslug=Mozilla%2FFirefox_OS%2FPlatform%2FTesting%2FGaia_unit_tests

Please reflag me for review when your ready again.  Thanks!

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8367601 [details] [review]
Bug 909201 proposed fix.

Please add a check for contacts.Search in list.js as we lazy load that functionality:

  if (contacts.Search) {
    contacts.Search.updateSearchList();
  }

You can see this is currently making the marionette tests fail.

Also, please squash your commits into a single commit.  You can do this with:

  git rebase -i master

See here for more information on doing that:

  http://davidwalsh.name/squash-commits-git

Also, please provide a better commit message.  It should follow the form:

  Bug <bug number>: <description of change>

When we finally merge it to master we then add r=<reviewer> to the message.

Please reflag me for review with those changes.  Should be a quick r+ after that assume the travis tests all pass.

Thanks for you work on this!

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8367601 [details] [review]
Bug 909201 proposed fix.

Looks good.  Thanks Robert!

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Merged to master:

  https://github.com/mozilla-b2g/gaia/commit/41da6e3ce207a0eb444ca8e94ccfaeab03577ae0

Comment 16

[Ed Morley [:emorley]]

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

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

Ben, please test locally before merging to master :-)

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Looks like a case of travis running with multiple tests in a sandbox vs tbpl using separate sandboxes.  Our main method of running tests locally matches travis, so it would not have found this.

I really wish we would make travis run things in their own sandboxes. :-(

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Ugh.  We have enabledSearch() with searchEnabled flag and the enterSearchMode() with inSearchMode flag.  This seems... confusing.  Francisco, do you think we should unify these?

If not, I think we should have this code check inSearchMode in addition to searchEnabled.

Comment 19

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

Definitely, let's merge them, we have same meaning. In fact the |enableSearch| function doesn't look to be used:

./js/views/search.js
370:  var enableSearch = function enableSearch() {
538:    'enableSearch': enableSearch,

./test/unit/mock_contacts_search.js
13:  'enableSearch': function() {},

Comment 20

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

Is this already merged?

Oh, it was back out!

Is that the case?

Comment 22

[Noemí Freire (:noemi)]

Per comment 21 nominating this bug to v1.5?

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Francisco Jordano [:arcturus] from comment #20)
> Is this already merged?
> 
> Oh, it was back out!
> 
> Is that the case?

Correct.  The patch just needs the check against |searchEnabled| to look at |inSearchMode| instead.

Comment 24

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

1.5+ to align with bug 955972

Comment 25

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

Hi Robert,

you up for applying the needed changes?

Comment 26

[robert.sajdok]

If you need to do it quickly then remove me from this bug otherwise please give me more time on it.

Comment 27

[robert.sajdok]

Attachment: Proposed fix.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8402225 [details] [review]
Proposed fix.

Thanks for following up on this for us Robert!  Looks good and I verified it works on my device.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Merged to master:

  https://github.com/mozilla-b2g/gaia/commit/8f5a447f5e11af28885be8b2c7210979e6ed171f

Comment 30

[Anthony Ricaud (:rik)]

This patch is missing new tests. Can Robert or Ben explain why it was landed without no new tests?

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Anthony Ricaud (:rik) from comment #30)
> This patch is missing new tests. Can Robert or Ben explain why it was landed
> without no new tests?

Don't blame Robert, this was my call.

Normally I ask for tests, but have noticed people stop contributing to the bug when I do that.  I made the decision here that we had reasonable coverage already and not throwing unnecessary roadblocks up for a contributor was more important.

Feel free to back out if you disagree.

Comment 32

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

+1 to Ben's comment.

In this specific case the unit test for the bug are not simple, I worked on them for an alternative version in bug 955972, but we decided to land this one.

I just created bug 993406 to follow up just with the unit test for this, again needed since this is a 1.5+ bug.

Cheers.

Comment 33

[Anthony Ricaud (:rik)]

(In reply to Ben Kelly [:bkelly] from comment #31)
> Don't blame Robert, this was my call.
That also looks like the correct path to me but we should be explicit about it (if only to inform the contributor about our landing policies). I've marked the test bug as a blocker for tracking purposes.