Closed Bug 1153888 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
minor

Tracking

(b2g-master fixed)

RESOLVED FIXED
FxOS-S2 (10Jul)
Tracking Status
b2g-master --- fixed

People

(Reporter: julienw, Assigned: anubhav.worklinux, Mentored)

References

Details

(Whiteboard: [lang=js][sms-papercuts])

Attachments

(1 file, 2 obsolete files)

46 bytes, text/x-github-pull-request
azasypkin
: review+
azasypkin
: feedback+
Details | Review
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 [1] and start working :)

[1] 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?
Flags: needinfo?(felash)
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.
Flags: needinfo?(anubhav.worklinux)
Attached file Pull request url on github. (obsolete) —
Attachment #8626095 - Flags: review?(felash)
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)
Attached file Pull request on github
Attachment #8626973 - 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 [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
Attachment #8626973 - Flags: review?(azasypkin) → feedback+
Attachment #8626095 - Attachment is obsolete: true
Attached file PR request (obsolete) —
Attachment #8627769 - Flags: review?(azasypkin)
Attachment #8626973 - Flags: review?(azasypkin)
Comment on attachment 8627769 [details] [review]
PR request

PR's are the same, so moved r? to the first PR only :)
Attachment #8627769 - Attachment is obsolete: true
Attachment #8627769 - Flags: review?(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!
Attachment #8626973 - Flags: review?(azasypkin) → review+
Mentor: felash → azasypkin
Keywords: checkin-needed
Please, add checkin-needed only when your PR contains only one squashed commit (currently there are 3 of them).
Keywords: checkin-needed
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/b6779a8902a78df0a699830436d457f390b4feaf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S2 (10Jul)
You need to log in before you can comment on or make changes to this bug.