Closed Bug 1062269 Opened 7 years ago Closed 7 years ago

Replace the code for loading a JSON file in the WhiteList object with the new shared function

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gsvelto, Assigned: ifadey, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #990047 +++

The custom JSON-loading code here can be replaced with the new shared function

https://github.com/mozilla-b2g/gaia/blob/533d6263a950acf1ce62cd33dd7ecb30e6767316/apps/wappush/js/whitelist.js#L19

Also this code is racy since it doesn't wait for the initialization to finish during startup. That should be changed by returning a promise that is resolved only after the whitelist file has been fully loaded.
Starting work on this.
Attached file Pull Request
Assignee: nobody → ifadey
Status: NEW → ASSIGNED
I've left a comment on GitHub regarding how we do initialization.
Amended my patch but I have a question. How can we test this manually? The only thing I can find about wap push is in sms settings. From source code, it looks like a separate app but where can I find it?
Attachment #8485487 - Flags: review?(gsvelto)
Testing WAP Push isn't easy because the application is hidden by default. It can be tested in the emulator but that can be cumbersome so here's a patch that will make the application visible when working on the desktop and adds a test button to the header that allows you to simulate the reception of a WAP Push message. You might want to test both with and without a populated whitelist to verify this works correctly.
I used the following format of json for testing:

[
  "+923331234567",
  "+923211234567"
]

Tested with and without whitelist.json. The code is working fine.
Now wappush_test is failing because WhiteList.init method is not present in mock_whitelist.js file. We have two options. Either to use whitelist.js file directly in test or add a dummy init method in mock file. What do you suggest?
Flags: needinfo?(gsvelto)
Comment on attachment 8485487 [details] [review]
Pull Request

Sorry for the delay, while testing this I've noticed another issue I hadn't noticed before in the order of the promises during initialization besides the lack of the init() method in the mock. I've left a comment in the PR for that.

In addition please add an init() method to the mock WhiteList object here:

https://github.com/mozilla-b2g/gaia/blob/533d6263a950acf1ce62cd33dd7ecb30e6767316/apps/wappush/test/unit/mock_whitelist.js

It should be enough to return a new already resolved promise for it to work since it's only a mock.

Ask for review again once both issues are fixed.
Attachment #8485487 - Flags: review?(gsvelto)
Flags: needinfo?(gsvelto)
Fawad, your patch was almost ready two weeks ago, do you still have time to finish it? If you need further help don't hesitate to ask, you can also reach me on IRC in the #b2g and #gaia channels.
Flags: needinfo?(ifadey)
Gabriele,
I am sorry I missed your comment. Let me check it again and push a new patch asap. Thanks!
Flags: needinfo?(ifadey)
Pushed a new patch. Waiting for the results then I will flag for review.
Attachment #8485487 - Flags: review?(gsvelto)
Comment on attachment 8485487 [details] [review]
Pull Request

Excellent work, this looks good to me. I've left only one final nit you should address before I merge your PR but it's a minor thing and we don't need to re-test for it. Remember to add r=gsvelto to your patch comment when you refresh your PR.
Attachment #8485487 - Flags: review?(gsvelto) → review+
I removed the check you asked but I added a blank reject hander because I guess after the change in getJSON method, it's required. Otherwise in case of failure from getJSON, the returned promise (from init whitelist method) will fail as well which we don't want. Now a blank reject handler in init whitelist method will exec the resolve handler for the outside code.

I added a detailed comment in the code.
(In reply to Fawad Hassan from comment #13)
> I removed the check you asked but I added a blank reject hander because I
> guess after the change in getJSON method, it's required. Otherwise in case
> of failure from getJSON, the returned promise (from init whitelist method)
> will fail as well which we don't want. Now a blank reject handler in init
> whitelist method will exec the resolve handler for the outside code.

Nice catch!

The green try run is here: https://tbpl.mozilla.org/?rev=442fd4d273df2306f28fb1bf9a5d2de183537527&tree=Gaia-Try

Merged to gaia/master 333872e9e032d46c542aec22e2e9068805b320c1

https://github.com/mozilla-b2g/gaia/commit/333872e9e032d46c542aec22e2e9068805b320c1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.