1153885 - [Messages] Remove Utils.getContactDisplayInfo

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

Utils.getContactDisplayInfo is useless, we should remove it and replace it by its content wherever it's used.

Comment 1

[mrjumpy]

Hi, I want to fix this bug. And I'm on the lookout for a help.
What should I do next?
Any tips?

Comment 2

[mrjumpy]

1.remove that function
2.run testing
3.pull request

Am I correct?

Comment 3

[Julien Wajsberg [:julienw]]

Hey "Mr Jumpy"

Yes, you are correct.

but I think Oleg (part of our team as well) has already a patch which removes it in another bug.

Hey Oleg, is it ok if we remove this function here, or do you prefer to keep the removal in your patch? I don't remember which bug it is so I don't know if this will be ready soon.

Comment 4

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (PTO May 1st) from comment #3)
> Hey "Mr Jumpy"
> 
> Yes, you are correct.
> 
> but I think Oleg (part of our team as well) has already a patch which
> removes it in another bug.
> 
> Hey Oleg, is it ok if we remove this function here, or do you prefer to keep
> the removal in your patch? I don't remember which bug it is so I don't know
> if this will be ready soon.

Yeah, it's in patch for bug 1127398, but I think it's totally fine to remove it here - I'll just rebase my patch on master when this one is landed.

Comment 5

[Julien Wajsberg [:julienw]]

So Mr Jumpy, you can go ahead !

You can find good startup instructions in https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/README.md.

Comment 6

[mrjumpy]

There are so many tests about sms.
Q1: which should I run it?

and I choose ->
test/unit/sms_test.js

Q2:
After that, I got "TypeError: Utils.getContactDisplayInfo is not a function".
So, should I remove the function in the test?

Comment 7

[Julien Wajsberg [:julienw]]

Hey MrJumpy,

The rule of thumb is that we take the test file that has the same name than the JS file we're modifying -- for example utils.js => utils_test.js.

I think you should change also mock_utils.js where this function is also used.
Basically you can run "git grep getContactDisplayInfo" to know where it's used and change the according code.

sms_test.js is some old test file -- it's still being run though so if it fails because of your changes it nees to be fixed too, but otherwise we try to not touch it so much.

Hope this is clearer :)

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] mozilla-b2g:v2.2 > mozilla-b2g:master

Comment 9

[mrjumpy]

review:Bug 1153885 - remove Utils.getContactDisplayInfo #29987

Am I correct?

Comment 10

[Oleg Zasypkin [:azasypkin]]

Since Julien is on PTO - I'll take a look.

Comment 11

[Oleg Zasypkin [:azasypkin]]

Hey MrJumpy,

There is something wrong with your PR, I see hundreds of unrelated commits there :)

Looks like something went wrong when you tried to rebase your PR on master (e.g. git rebase -i master).

Could you please fix it and then re-request feedback/review from me again?

Thanks!

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia] mrjumpy:master > mozilla-b2g:master

Comment 13

[mrjumpy]

review:Bug 1153885 - remove Utils.getContactDisplayInfo #30014

Comment 14

[Oleg Zasypkin [:azasypkin]]

Hey MrJumpy,

I left comments at GitHub - basically you need to rebase on the latest upstream master and replace "getContactDisplayInfo" usage in the code (I left some guidance at github on both matters).

Thanks!

Comment 15

[mrjumpy]

review: Bug 1153885 - remove getContactInfo R=mrjumpy

I'm push a new to the repo.Please check.
Did I need to add a new pull request?

Comment 16

[GitHub Autolander Bot]

Attachment: [gaia] mrjumpy:fix-bugs > mozilla-b2g:master

Comment 17

[mrjumpy]

review: Bug 1153885 - remove getContactInfo R=mrjumpy

I'm push a new to the repo.Please check.

Comment 18

[mrjumpy]

Hi Julien:
What should I do next?
Did I miss something?

Comment 19

[Julien Wajsberg [:julienw]]

Hey, thanks for your work :) It seems we're in a good direction !
I left some comments on github.

First I'm quite sure you didn't run the unit tests because they were failing. Basically, you should request a review only once you're sure all unit tests and jshint checks are passing. This way you're quite sure that at least the basic things are working fine :)

When you open a pull request or push to the branch, you'll notice there is an automatic comment with a link to all tests that are automatically run. There after some time (few minutes these days) you can check if the tests are passing or failing. But you should also run them locally. Some more information about how to run the tests are in our README [1].

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

So please fix the comments (read also below) and run the unit tests before asking a new review :)


Another remark is that you should squash and rebase your branch on top of latest master to make things easier for the reviewer and then to merge.

To achieve this, you can do something like this. I consider you use "upstream" as mozilla-b2g remote, and "origin" for your own remote. Please change them where appropriate if needed.

1 - SQUASHING

  git checkout fix-bugs # to move to the correct branch
  git rebase -i $(git merge-base moz/master HEAD)

This will open up your editor so that you can decide which commits you want to pick or merge. Usually you want to keep the first commit, and then replace "pick" by "squash" (or the shorter "s") for the next commits. "fixup" works too, the difference is that the commit logs of the additional commits are lost.

We try to use the rule "one commit = 1 review" so that it's really easy to review the additional changes. So when you'll handle my comments, please create a first squashed commit with the current commits, and then do your changes, and squash them on a second squashed commit (using the same method) before asking a new review.

For such a simple bug we could keep 1 commit only but it's also good if you get this habit right now :)


2- REBASING

Then you can rebase on top of lastest master. You'll notice that there will be some conflicts because we moved some files, but you should be able to resolve them quite easily.

  git fetch upstream
  git rebase upstream/master
  ... resolve conflicts if any, then run the following line:
  git rebase --continue
  git push -f origin HEAD


You maybe read that we should not usually use "git push -f", but as long as it happens in your own branch and your own repository it's fine. Here we need to use it because we rewrote history when we use "git rebase".


We can do both 1 and 2 in one go, but when you have several commits we could get conflicts for each commits... so it's better to squash them beforehand.


I hope these explanations will help you moving forward ! You can request a new needinfo whenever you want if anything is not clear enough :)

Thanks again, it's really valuable for us to have external contributors !

Comment 20

[mrjumpy]

Sorry for missing testing.
So, you're saying:
1. push before testing.
2. 1 commit = 1 review.

I'll fix these things later.
Thank you for your tips again.

Comment 21

[mrjumpy]

I can pass the testing now, if I'm using ./bin/gaia-test to start testing server.
But, 
if I'm running App=sms make test-agent-test, it got some errors.
What should I do?

youchongweideMacBook-Air:gaia mrjumpy$ APP=sms make test-agent-test
Running tests for sms
./node_modules/test-agent/bin/js-test-agent test  --server ws://localhost:8789 -t ""./build/config/test-agent-coverage.json"" -m "://([a-zA-Z-_]+)\." --reporter spec /Users/mrjumpy/Documents/gaia/apps/sms/services/test/unit/drafts_test.js /Users/mrjumpy/Documents/gaia/apps/sms/services/test/unit/message_manager_test.js /Users/mrjumpy/Documents/gaia/apps/sms/services/test/unit/threads_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/conversation/test/unit/attachment_renderer_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/conversation/test/unit/attachment_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/conversation/test/unit/compose_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/conversation/test/unit/conversation_integration_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/conversation/test/unit/conversation_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/conversation/test/unit/information_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/conversation/test/unit/link_action_handler_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/conversation/test/unit/link_helper_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/conversation/test/unit/recipients_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/conversation/test/unit/subject_composer_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/inbox/test/unit/inbox_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/activity_handler_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/activity_picker_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/app_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/contact_renderer_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/contacts_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/dialog_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/error_dialog_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/errors_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/inter_instance_event_dispatcher_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/localization_helper_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/navigation_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/notify_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/selection_handler_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/settings_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/shared_components_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/silent_sms_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/smil_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/sms_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/startup_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/task_runner_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/time_headers_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/utils_test.js /Users/mrjumpy/Documents/gaia/apps/sms/views/shared/test/unit/wbmp_test.js

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: connect ECONNREFUSED
    at errnoException (net.js:905:11)
    at Object.afterConnect [as oncomplete] (net.js:896:19)
make: *** [test-agent-test] Error 8

Comment 22

[Julien Wajsberg [:julienw]]

So, you need to run "bin/gaia-test" in a terminal, leave this running, and then in another terminal, run "APP=sms make test-agent-test".

Here it looks like the "gaia-test" server is not running.

Make sure it's not running somewhere else (for example you can use "ps ux | grep node" to kill any remaining node servers). Also take care that both ports 8080 and 8789 are used so you need to check that you don't have anything running on these ports beforehand.

Hope this helps !

Comment 23

[GitHub Autolander Bot]

Attachment: [gaia] mrjumpy:master > mozilla-b2g:master

Comment 24

[mrjumpy]

OK!
I'm pass SMS local testing. And, I'm left 1 commit this time.
I'm made a new pull request.
Thank you for your patience. I'm really enjoying it.
Hope I can pass this time!! 

review:Bug 1153885 - remove getContactInfo R=mrjumpy #30244

Comment 25

[Julien Wajsberg [:julienw]]

I added some comments on github. Not much more to do, but still some little changes to do. Thanks !

Comment 26

[mrjumpy]

OK, I'm follow your instruction.
I have 2 question.

Q1:
I'm running jsHint in utils_test.js
I'm found suiteSetup and suiteTearDown have warning notation and not in global. 
Maybe we should add that in the /*global?

Q2:
Did I need to delete previous PR?

review:Bug 1153885 - remove getContactInfo r=julien #30244

Thank you!!

Comment 27

[Julien Wajsberg [:julienw]]

(In reply to mrjumpy from comment #26)
> OK, I'm follow your instruction.
> I have 2 question.
> 
> Q1:
> I'm running jsHint in utils_test.js
> I'm found suiteSetup and suiteTearDown have warning notation and not in
> global. 
> Maybe we should add that in the /*global?

Normally this is caught by `"mocha": true` we have in .jshintrc. Can you check you have a recent enough version of jshint? You can use the one that is installed by gaia (in gaia/node_modules/.bin/jshint)

> 
> Q2:
> Did I need to delete previous PR?

Yes :)
Later on you should also learn how to reuse previous PR !

> 
> review:Bug 1153885 - remove getContactInfo r=julien #30244
> 
> Thank you!!

Comment 28

[Julien Wajsberg [:julienw]]

Comment on attachment 8610642 [details] [review]
[gaia] mrjumpy:master > mozilla-b2g:master

Looks good, thanks !

r=me

Comment 29

[Julien Wajsberg [:julienw]]

I'm adding "checkin-needed" so that the PR will be automatically checked in.

Thanks a lot for your work :)

Comment 30

[mrjumpy]

Does it mean I'm passed?
I'm so happy!!

Comment 31

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/f676149b496fe3c3ab84f7b3ec217553cd100a8b

Comment 32

[Julien Wajsberg [:julienw]]

(In reply to mrjumpy from comment #30)
> Does it mean I'm passed?
> I'm so happy!!

Yes !

Thanks a lot :)