1030564 - [Contacts][DSDS] SIM label flashing once when tapping on "Add as favorite" and "Remove as favorite"

Status: RESOLVED FIXED

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

Description

[Eric Chang [:ericcc] [:echang]]

### STR
1. Have a dual sim device with 2 SIMs inserted.
2. Have an entry in Contacts with phone number.
3. Open contact details.(it happened when entering details)
4. Tapping on "Add as favorite" and "Remove as favorite". (it also happened by doing this)


### Actual
Sim label disappears and appears again when Tapping on "Add as favorite" and "Remove as favorite".
https://www.youtube.com/watch?v=0UesHpFNnJ8

### Version
Flame v2.0 aurora 
Gaia      c478c43229883cee2afd09c6edb42d29a46cc500
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/8940337ccb5c
BuildID   20140625160200
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
t2m.sw.version=B1TC00011220

Comment 1

[Eric Chang [:ericcc] [:echang]]

flame v1.4 
https://www.youtube.com/watch?v=Y0oWm_IrNMg

Gaia      c478c43229883cee2afd09c6edb42d29a46cc500
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/8940337ccb5c
BuildID   20140625160200
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
t2m.sw.version=B1TC00011220

Comment 2

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

it's not a blocker but definitely something we want to improve.

Comment 3

[Jorge Prudencio [:jorgep]]

Attachment: 23370.html

Comment 4

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

Comment on attachment 8479853 [details]
23370.html

Hi Jorge,

thanks for the contribution, unfortunately we have landed some feature work that prevents your patch from landing.

Could you rebase and ask again for r?

We landed some code to share the way a phone or email is rendered in the contact detail, since now this code will also be used in dialer, so you should look in the 'shared' folder.

Thx

Comment 5

[Jorge Prudencio [:jorgep]]

Attachment: 23670.html

Comment 6

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

Comment on attachment 8483382 [details]
23670.html

Just left a tiny nit.

Also, can we have unit tests for this?

Thx!

Comment 7

[Jorge Prudencio [:jorgep]]

Hello Francisco, I can do it. I'm going to work in the unit tests this days.

Comment 8

[Jorge Prudencio [:jorgep]]

Hello Francisco, I have already done the tests. Can you review the changes?

Thanks!

Comment 9

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

Comment on attachment 8483382 [details]
23670.html

Thanks for adding the unit tests, left some comments on github.

Still intrigued why we are using a promise here, not that I don't like it, but trying to figure out what's the real need.

Personally I think we should move everything to promises, but this render method so far didn't have a callback to call.

So far don't see the need for the promise, if there is any tiny reason, let's do the promise, but let's use all the capabilities :)

Comment 10

[Jorge Prudencio [:jorgep]]

Hello Francisco,

I have used primises because I have to wait "navigator.mozContacts.save" finish. It is an asynchronous funcion and I have to wait it finish. I'm wrong?, it is the first time that I use promises :-)

Thanks!

Comment 11

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

No, you are not wrong, and I love the use of promises.

I left some suggestions, for example, instead of doing early returns:
https://github.com/mozilla-b2g/gaia/pull/23670#discussion-diff-17235315R187

We will need to return a promise as well, in this case, as we want to return the value straigh forward we can do:

| return Promise.resolve(isAFavoriteChange) |

That inmediately returns the value (but we still have a promise ;))

Also if we have an error path in the code, as we do here:
https://github.com/mozilla-b2g/gaia/pull/23670/files

We should call the |reject| callback that we have in the promise.

Apart from that looking pretty good. Amazing job :)

Comment 12

[Jorge Prudencio [:jorgep]]

Hello Francisco,

I have done the changes :-)

Thanks!

Comment 13

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

Nice, thanks Jorge

You can ask for r? again in the pr, anyway I'll be reviewing it :)

Comment 14

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

Hei Jorge,

sorry, my fault just discovered something we need to fix.

Left comments on GH, basically instead of calling always resolve, we should call resolve on the onsuccess callback of getContactById and we should call reject in the onerror callback.

Sorry again for realising late.

Comment 15

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

Comment on attachment 8483382 [details]
23670.html

See my previous comment, we are pretty pretty close, just a tiny modification.

Removing ? since just did it, waiting for the final changes. (Will be final I promise ;))

Comment 16

[Jorge Prudencio [:jorgep]]

I have left a question on github. ;-)

Comment 17

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

Just replied ;)

Comment 18

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

Comment on attachment 8483382 [details]
23670.html

The code looks pretty good.

Just r- cause whe have a unit test failing, details.js about the favourite rendering.

Probably have to do with the fact that now we are not rendering twice, we will need to fix that unit test before merging.

Thanks a lot for the great work!

Comment 19

[Jorge Prudencio [:jorgep]]

Hello Francisco,

Now seems that unit tests work.

Thanks!

Comment 20

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

Comment on attachment 8483382 [details]
23670.html

Perfect!

It's working pretty nice, thanks for the job!

Comment 21

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

Landed:

https://github.com/mozilla-b2g/gaia/commit/46e9aae9345107f21da431e16b5ea0abda3532a6