Closed
Bug 980396
Opened 10 years ago
Closed 10 years ago
Remove navigator.mozMobileConnection uses from the contacts app
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: gsvelto, Assigned: rishav_, Mentored)
References
Details
(Whiteboard: [lang=javascript])
Attachments
(1 file, 2 obsolete files)
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.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [mentor=:gsvelto][lang=javascript]
Reporter | ||
Updated•10 years ago
|
Blocks: sms-refactoring
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → rishav006
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•10 years ago
|
||
Hi gabriele Can you have a look on attached PR? Unit tests are running successfully. Thanks
Attachment #8407045 -
Flags: feedback?(gsvelto)
Reporter | ||
Comment 2•10 years ago
|
||
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.
Attachment #8407045 -
Flags: feedback?(gsvelto)
Assignee | ||
Comment 3•10 years ago
|
||
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
Reporter | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
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
Attachment #8407045 -
Attachment is obsolete: true
Attachment #8407709 -
Flags: feedback?(gsvelto)
Reporter | ||
Comment 6•10 years ago
|
||
(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.
Reporter | ||
Comment 7•10 years ago
|
||
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.
Attachment #8407709 -
Flags: feedback?(gsvelto) → feedback+
Assignee | ||
Comment 8•10 years ago
|
||
Hi francisco Can you have a look on the PR?All tests are running successfully.Also Gabriele gave his feedback. Thanks
Attachment #8408251 -
Flags: review?(francisco.jordano)
Comment 9•10 years ago
|
||
Comment on attachment 8408251 [details] [review] Bug 980396 - Remove navigator.mozMobileConnection uses from the contacts app Hi Kumar! Thanks for taking care of this.
Attachment #8408251 -
Flags: review?(francisco.jordano) → review+
Comment 10•10 years ago
|
||
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.
Attachment #8408251 -
Flags: review+ → review-
Assignee | ||
Comment 11•10 years ago
|
||
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
Assignee | ||
Comment 12•10 years ago
|
||
Hi gabriele I want to know your views on this as we dicussed this one before. What should i do here? Thanks
Assignee | ||
Comment 13•10 years ago
|
||
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•10 years ago
|
||
(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.
Reporter | ||
Comment 15•10 years ago
|
||
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•10 years ago
|
||
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!!
Updated•10 years ago
|
Mentor: gsvelto
Whiteboard: [mentor=:gsvelto][lang=javascript] → [lang=javascript]
Reporter | ||
Updated•10 years ago
|
No longer blocks: sms-refactoring
Assignee | ||
Comment 17•10 years ago
|
||
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
Attachment #8407709 -
Flags: review?(gsvelto)
Reporter | ||
Updated•10 years ago
|
Attachment #8408251 -
Attachment is obsolete: true
Reporter | ||
Comment 18•10 years ago
|
||
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.
Attachment #8407709 -
Flags: review?(gsvelto) → review+
Reporter | ||
Comment 19•10 years ago
|
||
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.
Reporter | ||
Comment 20•10 years ago
|
||
Try is now fully green, merged to gaia/master 09564e400c0d06006e6afe370978833eb64e8cff https://github.com/mozilla-b2g/gaia/commit/09564e400c0d06006e6afe370978833eb64e8cff
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•