980396 - Remove navigator.mozMobileConnection uses from the contacts app

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

The contacts app still uses navigator.mozMobileConnection in one of its unit-tests (communications/contacts/test/unit/views/settings_test.js); this should be removed and the test made to rely only on navigator.mozMobileConnections' mock.

Comment 1

[kumar rishav (:rishav_)]

Attachment: Bug 980396 - Remove navigator.mozMobileConnection uses from the contacts app

Hi gabriele
Can you have a look on attached PR? Unit tests are running successfully.
Thanks

Comment 2

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8407045 [details] [review]
Bug 980396 - Remove navigator.mozMobileConnection uses from the contacts app

Clearing the feedback because you still need to remove the uses in apps/communications/contacts/test/unit/views/details_test.js, apps/communications/contacts/test/unit/import/import_contacts_test.js and apps/communications/contacts/js/views/details.js.

When you've updated the PR ask for feedback again.

Comment 3

[kumar rishav (:rishav_)]

Hi  gabriele
In apps/communications/contacts/test/unit/views/details_test.js when i am changing navigator.mozMobileConnection to navigator.mozMobileConnections, unit test is giving error for https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/test/unit/views/details_test.js#L788
Can you help me in this?
Thanks

Comment 4

[Gabriele Svelto [:gsvelto]]

(In reply to kumar rishav from comment #3)
> In apps/communications/contacts/test/unit/views/details_test.js when i am
> changing navigator.mozMobileConnection to navigator.mozMobileConnections,
> unit test is giving error for
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/
> test/unit/views/details_test.js#L788

That test is using the MMI manager which is probably being trying to read from the mock mobile connection, see the code here:

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/mmi.js#L42

You have to trace the test code and verify exactly where it is failing, when a test fails it will print a stack trace which will help you with that. Once you have found why the test is failing you might need to use the mock to create the necessary fake structures for the test to pass.

Comment 5

[kumar rishav (:rishav_)]

Attachment: Bug 980396 - Remove navigator.mozMobileConnection uses from the contacts app

Hi gabriele
I have made the required changes in all those files and unit test is running successfully.Have a look on the attached PR.

I have to ask one thing, here we are removing mozMoblieConnection for different app, say contact or ftu, then i have to check where and all mozMoblieConnection  is used in that app.Like in this bug, first i did for setting_test.js.Then you comment three more files where it is used.How i supposed to know where it is used or i have to search in all files.

Also check patch for bug 980402.Do i have to do for other files also other than that?
Also can i report bug ,if i find mozMobileConnection some where in some app?

Thanks

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to kumar rishav from comment #5)
> I have to ask one thing, here we are removing mozMoblieConnection for
> different app, say contact or ftu, then i have to check where and all
> mozMoblieConnection  is used in that app.Like in this bug, first i did for
> setting_test.js.Then you comment three more files where it is used.How i
> supposed to know where it is used or i have to search in all files.

You should remove all uses of mozMobileConnection (and replace them with mozMobileConnectsion where applicable) within the affected app. So in this case it's everything under apps/communications/contacts. I checked and besides the files I mentioned in comment 2 there should be no others.

Comment 7

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8407709 [details] [review]
Bug 980396 - Remove navigator.mozMobileConnection uses from the contacts app

Looks good to me except for a couple of issues I have described in the PR. Ask for review to one of the contacts peers once you've addressed those.

Comment 8

[kumar rishav (:rishav_)]

Attachment: Bug 980396 - Remove navigator.mozMobileConnection uses from the contacts app

Hi francisco
Can you have a look on the PR?All tests are running successfully.Also Gabriele gave his feedback.
Thanks

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8408251 [details] [review]
Bug 980396 - Remove navigator.mozMobileConnection uses from the contacts app

Hi Kumar!

Thanks for taking care of this.

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8408251 [details] [review]
Bug 980396 - Remove navigator.mozMobileConnection uses from the contacts app

Oh sorry, just realised about one unit test failing cause of this patch:

 [communications-contacts/test/unit/views/details_test.js] Render contact > User actions > Not loading MultiSimActionButton if we have a MMI code:

Error: expected isMMI to have been called at least once but was never called

You could check it in travis here:
https://travis-ci.org/mozilla-b2g/gaia/jobs/23208035

I'm leaving comments on github for making this work.

We are pretty close!!

Thanks again Kumar.

Comment 11

[kumar rishav (:rishav_)]

Hi Franscisco
when i ran the test on my system, everything was fine(this too   Render contact > User actions > Not loading MultiSimActionButton if we have a MMI code:).
Why this happened?
Thanks

Comment 12

[kumar rishav (:rishav_)]

Hi gabriele
I want to know your views on this as we dicussed this one before.
What should i do here?
Thanks

Comment 13

[kumar rishav (:rishav_)]

Hi Francisco
After adding navigator.mozMobileConnections = true; Now this error (Render contact > User actions > Not loading MultiSimActionButton if we have a MMI code:)is showing.When it was not there, unit test ran fine but after adding this i am getting error.Why it's happening?
Thanks

Comment 14

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to kumar rishav from comment #13)
> Hi Francisco
> After adding navigator.mozMobileConnections = true; Now this error (Render
> contact > User actions > Not loading MultiSimActionButton if we have a MMI
> code:)is showing.When it was not there, unit test ran fine but after adding
> this i am getting error.Why it's happening?
> Thanks

hei Kumar!

Right now I can see the error, without adding anything else.

I was suggesting to add:

navigator.mozMobileConnections = [true];

If you see it's an array, as it's suppose to be the mozMobileConnections object.

Cheers.

Comment 15

[Gabriele Svelto [:gsvelto]]

I think that the navigator.mozMobileConnections = [true] line is actually causing the issue and it's not needed, let me elaborate on why. The test code is calling MmiManager.isMMI() which not only requires at least a mozMobileConnections object but will actually inspect the voice field of that object, see the code here:

https://github.com/mozilla-b2g/gaia/blob/d9cbae6bf9421a4d29e0f063f53b5eca837cc158/apps/communications/dialer/js/mmi.js#L290

Now, by default the mock_navigator_moz_mobile_connections.js mock populates navigator.mozMobileConnections with a single connection which has all the required fields, including voice, see here:

https://github.com/mozilla-b2g/gaia/blob/d9cbae6bf9421a4d29e0f063f53b5eca837cc158/shared/test/unit/mocks/mock_navigator_moz_mobile_connections.js#L125

So this all that's needed to make the test pass, adding |navigator.mozMobileConnections = true| removes the mock mozMobileConnection object that's been created by the mock and causes isMMI() to fail.

Comment 16

[Francisco Jordano [:arcturus] [:francisco]]

Oh, nice, didn't realised that we are using a proper mock.

In this case, the test that is failing: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/test/unit/views/details_test.js#L788

is mocking the MmiManager.isMMI code to return always |true| so we don't go through this path:
https://github.com/mozilla-b2g/gaia/blob/d9cbae6bf9421a4d29e0f063f53b5eca837cc158/apps/communications/dialer/js/mmi.js#L290

So, I do agree on using the mock, but somehow we were not using it, the current PR, in the test without the [true] hack is failing:
https://travis-ci.org/mozilla-b2g/gaia/jobs/23208035

Case we don't call the stub |isMMI|, not cause it's producing an error, is cause it's not being called:
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/test/unit/views/details_test.js#L794

That's what I think, but it's my first day after holidays perhaps I'm missing something for sure ;)

Thanks guys!!

Comment 17

[kumar rishav (:rishav_)]

Comment on attachment 8407709 [details] [review]
Bug 980396 - Remove navigator.mozMobileConnection uses from the contacts app

Hi Gabriele
This patch was left over from more than one month.I forgot about this. Just now i checked it.It will be great if you review this patch.
Thanks

Comment 18

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8407709 [details] [review]
Bug 980396 - Remove navigator.mozMobileConnection uses from the contacts app

I saw you addressed :francisco's comment and the rest looks fine to me.

Comment 19

[Gabriele Svelto [:gsvelto]]

There was an intermittent test that failed in the try run. I've re-triggered the test and I'm waiting for it to turn green before merging.

Comment 20

[Gabriele Svelto [:gsvelto]]

Try is now fully green, merged to gaia/master 09564e400c0d06006e6afe370978833eb64e8cff

https://github.com/mozilla-b2g/gaia/commit/09564e400c0d06006e6afe370978833eb64e8cff