Last Comment Bug 867793 - Use fancy new SIM API to determine location
: Use fancy new SIM API to determine location
Status: RESOLVED FIXED
:
Product: Marketplace
Classification: Server Software
Component: Integration (show other bugs)
: 1.0
: All All
: P1 normal (vote)
: 2013-05-23
Assigned To: Christopher Van Wiemeersch [:cvan]
:
:
Mentors:
: 871680 (view as bug list)
Depends on: 865132 868571
Blocks: 872902
  Show dependency treegraph
 
Reported: 2013-05-01 15:26 PDT by Wil Clouser [:clouserw]
Modified: 2013-05-20 15:24 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments

Description Wil Clouser [:clouserw] 2013-05-01 15:26:58 PDT
Bug 866272 is giving access to an API which the packaged marketplace can use to query the SIM and determine what marketplace to show the user.  Our current detection path, defined over in bug 865132 comment 6, is:

> URL parameter -> localStorage -> cookie -> geoip

We should prepend this list with SIM, if the device has one:

> SIM -> URL parameter -> localStorage -> cookie -> geoip

Setting tef? because if this bug finds a problem with the API it'll need to be fixed, so suggesting we have this as a blocker until verified.
Comment 1 Wraithan (Chris McDonald) [:wraithan] 2013-05-02 11:12:54 PDT
If Fireplace side could pass the region from SIM as a GET param that would make life a lot easier for me. I assume that is what you were planning but wanted to make it explicit.
Comment 2 Matt Basta [:basta] 2013-05-02 11:18:09 PDT
We already do!

https://github.com/mozilla/fireplace/blob/master/hearth/media/js/urls.js#L71

That parameter will always contain what we believe the user's region to be, or `null` or an empty string if we don't know the user's region.
Comment 3 Jason Smith [:jsmith] 2013-05-03 09:58:32 PDT
Triage results - triage is concerned about the following comment implying significant 1.01 risk:

"Setting tef? because if this bug finds a problem with the API it'll need to be fixed, so suggesting we have this as a blocker until verified"

The original argument to my understanding for why bug 866272 was needed was to allow marketplace to figure out what country it was in to serve country-specific content.

However, there's open questions here to resolve whether to block or not. I'm following up with clouserw in IRC to talk more about this.
Comment 4 Jason Smith [:jsmith] 2013-05-03 12:18:38 PDT
Talked this over with a few people. I understand now that there isn't a different option to meet the requirements needed here. However, there's still a large concern about risk here that needs to be addressed here after talking with krupa.

Such questions that we talked about include:

1. What bugs may we find from these changes?
2. What fallback behavior are we implementing in case the underlying SIM API fails? Gives incorrect data?
3. Are the certification testing processes covering workflows involving the underlying SIM API?

Asking for a risk assessment from Wil on what he can answer from the above questions.
Comment 5 Wil Clouser [:clouserw] 2013-05-03 12:43:19 PDT
(In reply to Jason Smith [:jsmith] from comment #4)
> Talked this over with a few people. I understand now that there isn't a
> different option to meet the requirements needed here. However, there's
> still a large concern about risk here that needs to be addressed here after
> talking with krupa.
> 
> Such questions that we talked about include:
> 
> 1. What bugs may we find from these changes?

You're changing an API.  We're the first customers of that API.  Bugs may range from complete failure to minor changes we work around.

> 2. What fallback behavior are we implementing in case the underlying SIM API
> fails? Gives incorrect data?

See comment 0 for the fallback pattern.  If it fails or doesn't exist, we follow that pattern.  If it gives incorrect data I'd file a new TEF+ bug.

> 3. Are the certification testing processes covering workflows involving the
> underlying SIM API?

A question for Krupa.  I don't know the status of what SIMs we currently can test with.

> Asking for a risk assessment from Wil on what he can answer from the above
> questions.
Comment 6 Jason Smith [:jsmith] 2013-05-03 13:26:34 PDT
The blocking request here is updating the app on device, not the hosted work per talking with Wil. Clearing the flag. I filed bug 868571 for the Gaia-specific work here.
Comment 7 Christopher Van Wiemeersch [:cvan] 2013-05-10 01:27:30 PDT
https://github.com/mozilla/zamboni/commit/2f07d90

Unsigned package:
https://marketplace-dev.allizom.org/media/packaged-apps/yulelog_prod_2013.05.10_01.16.03.zip

To sign the package:
manage.py sign_marketplace --settings=settings_local_mkt

Region/carrier detection code:
https://github.com/mozilla/fireplace/blob/master/yulelog/index.html#L27

I'll keep this bug open as we still need code in Zamboni to map MCC/MNC to country/carrier, respectively.
Comment 8 Christopher Van Wiemeersch [:cvan] 2013-05-10 13:20:29 PDT
Not really looking forward to adding these to Zamboni, but here are the MCC/MNC mappings:

https://en.wikipedia.org/wiki/Mobile_country_code
Comment 9 Wil Clouser [:clouserw] 2013-05-10 13:31:52 PDT
MCC is already there, just need MNC: https://github.com/mozilla/zamboni/blob/master/mkt/constants/regions.py
Comment 10 Christopher Van Wiemeersch [:cvan] 2013-05-10 13:33:03 PDT
(In reply to Wil Clouser [:clouserw] from comment #9)
> MCC is already there, just need MNC:
> https://github.com/mozilla/zamboni/blob/master/mkt/constants/regions.py

That's correct, yeah - we just need middleware to parse the region's MCC instead of by region's slug.
Comment 11 Matt Basta [:basta] 2013-05-10 13:47:45 PDT
If we have the MCC/MNC in Fireplace, I don't see why we couldn't just have a map in settings.js with something like this:

{
732:'co',
734:{1:'ve'}
}

The MCC is probably all that we'll need for now. Each carrier can already force their own carrier code in settings.js.

No sense in making a request to Zamboni to get a mapping that could easily fit in a few dozen (someday a few hundred) bytes of code.
Comment 12 Christopher Van Wiemeersch [:cvan] 2013-05-13 12:00:49 PDT
*** Bug 871680 has been marked as a duplicate of this bug. ***
Comment 13 Christopher Van Wiemeersch [:cvan] 2013-05-20 15:24:38 PDT
https://github.com/mozilla/fireplace/commit/7c68e0462

Note You need to log in before you can comment on or make changes to this bug.