807312 - Speakout Wireless needs default APN

Status: RESOLVED FIXED

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

Description

[Hubert Figuiere [:hub]]

Speakout Wireless needs default APN settings too.

Attaching patch.

Comment 1

[Hubert Figuiere [:hub]]

Attachment: Bug 807312 - Add APN settings for Speakout Wireless (Canada, 7-Eleven MVNO).

Comment 2

[Fabien Cazenave [:kaze]]

Comment on attachment 677014 [details] [diff] [review]
Bug 807312 - Add APN settings for Speakout Wireless (Canada, 7-Eleven MVNO).

Thinking out loud: we should add this diff to the apps/settings/resources/ directory (e.g. in a apns_conf_additions.diff file) so we don’t lose it next time I update the Android database.

Comment 3

[Fabien Cazenave [:kaze]]

Comment on attachment 677014 [details] [diff] [review]
Bug 807312 - Add APN settings for Speakout Wireless (Canada, 7-Eleven MVNO).

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: prevents connection forom Speakout Wireless users
Testing completed: 
Risk to taking this patch (and alternatives if risky): none

Comment 4

[Hubert Figuiere [:hub]]

This is blocking dogfooding here in Canada. This is for the only pre-paid option available to us.

Comment 5

[Hubert Figuiere [:hub]]

Pull request still waiting

https://github.com/mozilla-b2g/gaia/pull/6170

Comment 6

[Hubert Figuiere [:hub]]

Merged 
https://github.com/mozilla-b2g/gaia/commit/0437b7a70fa418cb3d939a4ce032a2c0b824ee0d

Comment 7

[Hubert Figuiere [:hub]]

Reopening as some recent changes broke it.

Comment 8

[Hubert Figuiere [:hub]]

It seems that I do need to regenerate shared/resources/apn.json (and commit) but I can't figure how to.

Comment 9

[Hubert Figuiere [:hub]]

Attachment: Bug 807312 - Part 2: update the apn.json file.

Comment 10

[Hubert Figuiere [:hub]]

This last change is now needed. It was not when the first patch got initial approval.

Note: I did the changes by hand as I haven't been able to regenerate the file.

Comment 11

[Fabien Cazenave [:kaze]]

Comment on attachment 687172 [details] [diff] [review]
Bug 807312 - Part 2: update the apn.json file.

The apn.json file is the merge of the three XML databases in /shared/resources/apn/*.xml:
 • apns_conf.xml is the Android database, and it’s our root
 • serviceproviders.xml is the Gnome database — it’s used to retrieve additional information (e.g. voicemail);
 • operator-variant.xml is our own database (Firefox OS) — it’s used to add specific information.

So please don’t modify the apn.json file, but either fix the Android database by submitting an upstream patch or add your operator (“SpeakOut”) to the operator-variant.xml and ensure that query.js is able to merge this information properly.in

Comment 12

[Hubert Figuiere [:hub]]

(In reply to Fabien Cazenave [:kaze] from comment #11)
> Comment on attachment 687172 [details] [diff] [review]
> Bug 807312 - Part 2: update the apn.json file.
> 
> The apn.json file is the merge of the three XML databases in
> /shared/resources/apn/*.xml:
>  • apns_conf.xml is the Android database, and it’s our root
>  • serviceproviders.xml is the Gnome database — it’s used to retrieve
> additional information (e.g. voicemail);
>  • operator-variant.xml is our own database (Firefox OS) — it’s used to add
> specific information.
> 
> So please don’t modify the apn.json file, but either fix the Android
> database by submitting an upstream patch or add your operator (“SpeakOut”)
> to the operator-variant.xml and ensure that query.js is able to merge this
> information properly.in

As I said in Comment 10, I didn't succeed in regenerating the file - so I edited it myself. The information is in the apns_conf.xml already, so if you regenerate it, you'll see that there shouldn't be any difference.

Comment 13

[Fabien Cazenave [:kaze]]

(In reply to Hub Figuiere [:hub] from comment #12)
> As I said in Comment 10, I didn't succeed in regenerating the file - so I
> edited it myself. The information is in the apns_conf.xml already, so if you
> regenerate it, you'll see that there shouldn't be any difference.

Did you add this information on your own without submitting it upstream? If yes, this will be lost next time we update the Android database.

We should *not* modify the apns_conf.xml or serviceproviders.xml databases on our side. They’re directly pulled from upstream sources.

Comment 14

[Hubert Figuiere [:hub]]

> Did you add this information on your own without submitting it upstream? If
> yes, this will be lost next time we update the Android database.

I had no indication about whether I should do or not.

> We should *not* modify the apns_conf.xml or serviceproviders.xml databases
> on our side. They’re directly pulled from upstream sources.

So what are the options? It does not look like operator-variant.xml has the same

Also how does one regenerate the .json ?

Comment 15

[Fabien Cazenave [:kaze]]

(In reply to Hub Figuiere [:hub] from comment #14)
> > Did you add this information on your own without submitting it upstream? If
> > yes, this will be lost next time we update the Android database.
> 
> I had no indication about whether I should do or not.
> 

Then please excuse my lack of clarity.

> > We should *not* modify the apns_conf.xml or serviceproviders.xml databases
> > on our side. They’re directly pulled from upstream sources.
> 
> So what are the options? It does not look like operator-variant.xml has the
> same
> 

Not sure to get the question, but yes if you choose to add your APN locally you’ll probably have to adapt the code in query.js. You can also create another XML file, e.g. apns_conf_new.xml, if that’s easier to you.

As long as query.js allows to merge these databases into apn.json and that the Android & Gnome databases are not modified locally, that’s fine.

> Also how does one regenerate the .json ?

I assumed the README.md file in /shared/resources/apn/ would be enough but apparently my communication skills are even worse than I thought. Sorry. :-/

Open the /shared/resources/apn/index.html file in a browser (through a local webserver), you should see the existing apn.json database. If you remove the apn.json database, this HTML document will generate a new JSON string from the existing XML databases — and you can copy/paste this string to a new apn.json file.

Comment 16

[Hubert Figuiere [:hub]]

Attachment: Bug 807312 - Part 2: Make an apns_conf-local.xml and update the .json file. Clarify the README.

Comment 17

[Fabien Cazenave [:kaze]]

Comment on attachment 688426 [details] [diff] [review]
Bug 807312 - Part 2: Make an apns_conf-local.xml and update the .json file. Clarify the README.

Review of attachment 688426 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but please address the two nitpicks. Thanks for your work Hubert!

::: shared/resources/apn/query.js
@@ +9,4 @@
>    var OPERATOR_VARIANT_FILE = '../apn.json';
>    var GNOME_DB_FILE = 'serviceproviders.xml';
>    var ANDROID_DB_FILE = 'apns_conf.xml';
> +  var LOCAL_ANDROID_DB_FILE = 'apns_conf-local.xml';

nit: please don’t use any dash in the file name and use an underscore instead. That’s one of the frustrating guidelines we follow for this project…

@@ +226,5 @@
>            output.textContent = '\n merging databases, this takes a while...';
>            gAndroidDB = loadXML(ANDROID_DB_FILE);
> +          // First merge the local DB
> +          var localAndroidDB = loadXML(LOCAL_ANDROID_DB_FILE);
> +          var localApns = localAndroidDB.documentElement.getElementsByTagName("apn");

Nit: please make sure query.js passes the linter. This line does not.

Comment 18

[Hubert Figuiere [:hub]]

> > +          var localApns = localAndroidDB.documentElement.getElementsByTagName("apn");
> 
> Nit: please make sure query.js passes the linter. This line does not.


How do I run the linter?

Comment 19

[Fabien Cazenave [:kaze]]

  gjslint --nojsdoc my_file.js

https://wiki.mozilla.org/Gaia/Hacking#Before_submitting_a_patch

Comment 20

[Hubert Figuiere [:hub]]

> 
> nit: please don’t use any dash in the file name and use an underscore
> instead. That’s one of the frustrating guidelines we follow for this project…


Also funny thing: operator-variant.xml use a dash....

Comment 21

[Fabien Cazenave [:kaze]]

Oh, right. Feel free to rename it as well, while you’re at it. :-)

Comment 22

[Hubert Figuiere [:hub]]

Pull Request. Please merge.
https://github.com/mozilla-b2g/gaia/pull/6824

Comment 23

[Fabien Cazenave [:kaze]]

Comment on attachment 688426 [details] [diff] [review]
Bug 807312 - Part 2: Make an apns_conf-local.xml and update the .json file. Clarify the README.

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not a regression
User impact if declined: dogfooding is impossible in Toronto
Testing completed: manual
Risk to taking this patch (and alternatives if risky): low, as the resulting `apn.json' file is almost the same (only one additional entry)

Comment 24

[Hubert Figuiere [:hub]]

I don't have credentials on github.

Comment 25

[Fabien Cazenave [:kaze]]

As this bug is not blocking-basecamp+, we need an explicit approval before — which I’ve just requested.

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 688426 [details] [diff] [review]
Bug 807312 - Part 2: Make an apns_conf-local.xml and update the .json file. Clarify the README.

Review of attachment 688426 [details] [diff] [review]:
-----------------------------------------------------------------

I feel like adding a third player into the APN databases is a nice win. That would let people add their own custom APNs without breaking the main databases. a+.

Comment 27

[Fabien Cazenave [:kaze]]

https://github.com/mozilla-b2g/gaia/commit/83ca96ba1658d7df16e539d11f261bac1bce244a

Comment 28

[Fabien Cazenave [:kaze]]

Attachment: follow-up

https://github.com/mozilla-b2g/gaia/pull/6889/files