Closed
Bug 1030564
Opened 10 years ago
Closed 10 years ago
[Contacts][DSDS] SIM label flashing once when tapping on "Add as favorite" and "Remove as favorite"
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Tracking
(tracking-b2g:backlog, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.2 fixed)
People
(Reporter: ericcc, Assigned: jorgep)
Details
(Whiteboard: [priority])
Attachments
(1 file, 1 obsolete file)
### 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
Reporter | ||
Comment 1•10 years ago
|
||
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
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
Reporter | ||
Updated•10 years ago
|
blocking-b2g: --- → 2.1?
Reporter | ||
Updated•10 years ago
|
QA Whiteboard: [COM=Gaia::Contacts]
Comment 2•10 years ago
|
||
it's not a blocker but definitely something we want to improve.
blocking-b2g: 2.1? → backlog
Whiteboard: [priority]
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jpruden92
Target Milestone: --- → 2.1 S3 (29aug)
Comment 4•10 years ago
|
||
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
Attachment #8479853 -
Flags: review?(francisco)
Updated•10 years ago
|
Target Milestone: 2.1 S3 (29aug) → 2.1 S4 (12sep)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8479853 -
Attachment is obsolete: true
Attachment #8483382 -
Flags: review?(francisco)
Comment 6•10 years ago
|
||
Comment on attachment 8483382 [details]
23670.html
Just left a tiny nit.
Also, can we have unit tests for this?
Thx!
Attachment #8483382 -
Flags: review?(francisco) → review+
Updated•10 years ago
|
Flags: needinfo?(jpruden92)
Assignee | ||
Comment 7•10 years ago
|
||
Hello Francisco, I can do it. I'm going to work in the unit tests this days.
Flags: needinfo?(jpruden92)
Assignee | ||
Comment 8•10 years ago
|
||
Hello Francisco, I have already done the tests. Can you review the changes? Thanks!
Flags: needinfo?(francisco)
Comment 9•10 years ago
|
||
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 :)
Attachment #8483382 -
Flags: review+ → review-
Flags: needinfo?(francisco)
Assignee | ||
Comment 10•10 years ago
|
||
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!
Flags: needinfo?(francisco)
Comment 11•10 years ago
|
||
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 :)
Flags: needinfo?(francisco)
Assignee | ||
Comment 12•10 years ago
|
||
Hello Francisco, I have done the changes :-) Thanks!
Flags: needinfo?(francisco)
Comment 13•10 years ago
|
||
Nice, thanks Jorge You can ask for r? again in the pr, anyway I'll be reviewing it :)
Flags: needinfo?(francisco)
Assignee | ||
Updated•10 years ago
|
Attachment #8483382 -
Flags: review- → review?(francisco)
Comment 14•10 years ago
|
||
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•10 years ago
|
||
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 ;))
Attachment #8483382 -
Flags: review?(francisco)
Assignee | ||
Updated•10 years ago
|
Attachment #8483382 -
Flags: review?(francisco)
Comment 18•10 years ago
|
||
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!
Attachment #8483382 -
Flags: review?(francisco) → review-
Updated•10 years ago
|
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
Assignee | ||
Comment 19•10 years ago
|
||
Hello Francisco, Now seems that unit tests work. Thanks!
Flags: needinfo?(francisco)
Comment 20•10 years ago
|
||
Comment on attachment 8483382 [details]
23670.html
Perfect!
It's working pretty nice, thanks for the job!
Attachment #8483382 -
Flags: review- → review+
Flags: needinfo?(francisco)
Comment 21•10 years ago
|
||
Landed: https://github.com/mozilla-b2g/gaia/commit/46e9aae9345107f21da431e16b5ea0abda3532a6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
status-b2g-v2.2:
--- → fixed
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•