Closed Bug 1153888 Opened 5 years ago Closed 5 years ago
.find By and other Contacts .* functions should never return "null".
There are cases where Contacts.findBy and friends can return "null" as Promise value. Instead we should always return an array. The cases where we return "null" should either be a rejected promise or an empty array. Places where we check for this should also be updated and simplified.
Whiteboard: [lang=js] → [lang=js][sms-papercuts]
hi! i would like to start working on this bug. can u just give me some idea how to approach it.
Hey ! The main code to change is in views/shared/js/contacts.js. You'll see that there are cases where we return a resolved promise with the "null" value. We should change this and return an empty array instead. I'd start with looking at all cases where this happens. You can also look at the unit tests (I don't know if we test these cases but you'll have to edit them anyway) in views/shared/test/unit/contacts_test.js. Then you can look at all places in code where we use Contacts.* functions, see if we special case for the "null" result, and change this code. There is a fair amount of work here, so please tell me of you intend to do it :) If you feel confident, you can start having a look at our readme file at  and start working :)  https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/README.md
Hello Julien, My contacts.js is in gaia/apps/communications/contacts/js/contacts.js, is correct file? I can see only one return null, in function addExtrasToContact(extrasString), but I can see many return; Its the path to resolve this bug?
No, it's in the SMS application, so the correct path is: gaia/apps/sms/views/shared/js/contacts.js Hope it's clearer ! But Antonio, maybe anubhav already started working on this and I wouldn't want a conflict here... Let's ask him first !
Flags: needinfo?(felash) → needinfo?(anubhav.worklinux)
Thanks, No problem, I still waiting.
hey! @antonio ladeia sorry for late reply. i was busy 1 month from my engineering end semester exams. i have started working on this bug.
Comment on attachment 8626095 [details] Pull request url on github. Hey Oleg, can you have a look at this while I'm away ?
Attachment #8626095 - Flags: review?(felash) → review?(azasypkin)
Comment on attachment 8626095 [details] Pull request url on github. Hey Anubhav, I've left several comments at GitHub that should allow you to move forward with this bug, generally speaking you need to do what Julien mentioned in comment 2. If you have questions you can ask at GitHub, in this bug or at #gaia/#gaia-messaging IRC channels. Once you feel you're ready you can ask early feedback either from Julien or me. Thanks!
Attachment #8626095 - Flags: review?(azasypkin)
Assignee: nobody → anubhav.worklinux
Severity: normal → minor
Status: NEW → ASSIGNED
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Comment on attachment 8626973 [details] [review] Pull request on github Looks good now! Just few nits left (commented at GitHub). Please, reuse the same pull request, just rebase it on the latest upstream master  and change commit message so that it contains "r=reviewer" part (r=azasypkin in our case), see  for more details. We usually add review changes as a separate commit on the same pull request (PR) branch. So basically workflow is the following: 1. Rebase your PR branch on the latest upstream master; 2. Do your changes, commit them as the separate commit; 3. Manually check in Firefox Desktop (or on device) that affected pieces work correctly; 4. Push this branch to origin (your Gaia fork); 5. Once you do this, PR will be automatically updated and all tests will be re-run once again; 6. Wait for the green tests and ask r? again! Thanks!  https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch#Tips_on_Gaia_Rebasing  https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch#Manual_patch_submission
Attachment #8626973 - Flags: review?(azasypkin) → feedback+
Attachment #8626095 - Attachment is obsolete: true
Comment on attachment 8627769 [details] [review] PR request PR's are the same, so moved r? to the first PR only :)
Comment on attachment 8626973 [details] [review] Pull request on github Works good, can't see any flaws in the PR! So it's time to squash your commits into one  so that final commit message consists of bug title and r=azasypkin. Once you're done with this, please add "checkin-needed" into "Keywords" section of this bug page.  https://github.com/ginatrapani/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit Thanks for your contribution!
Attachment #8626973 - Flags: review?(azasypkin) → review+
Please, add checkin-needed only when your PR contains only one squashed commit (currently there are 3 of them).
You need to log in before you can comment on or make changes to this bug.