1153888 - [Messages] Contacts.findBy and other Contacts.* functions should never return "null".

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

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.

Comment 1

[anubhav.worklinux]

hi! i would like to start working on this bug. can u just give me some idea how to approach it.

Comment 2

[Julien Wajsberg [:julienw]]

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 [1] and start working :)

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/README.md

Comment 3

[Antonio Ladeia]

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?

Comment 4

[Julien Wajsberg [:julienw]]

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 !

Comment 5

[Antonio Ladeia]

Thanks,

No problem, I still waiting.

Comment 6

[anubhav.worklinux]

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 7

[anubhav.worklinux]

Attachment: Pull request url on github.

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 8626095 [details]
Pull request url on github.

Hey Oleg, can you have a look at this while I'm away ?

Comment 9

[Oleg Zasypkin [: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!

Comment 10

[anubhav.worklinux]

Attachment: Pull request on github

Comment 11

[Oleg Zasypkin [:azasypkin]]

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 [1] and change commit message so that it contains "r=reviewer" part (r=azasypkin in our case), see [2] 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!

[1] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch#Tips_on_Gaia_Rebasing

[2] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch#Manual_patch_submission

Comment 12

[anubhav.worklinux]

Attachment: PR request

Comment 13

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8627769 [details] [review]
PR request

PR's are the same, so moved r? to the first PR only :)

Comment 14

[Oleg Zasypkin [:azasypkin]]

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 [1] 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.

[1] https://github.com/ginatrapani/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit

Thanks for your contribution!

Comment 15

[Oleg Zasypkin [:azasypkin]]

Please, add checkin-needed only when your PR contains only one squashed commit (currently there are 3 of them).

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/b6779a8902a78df0a699830436d457f390b4feaf