947198 - [DSDS][dolphin][OMA CP] Support for DSDS

Status: RESOLVED FIXED

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

Description

[Wilfred Mathanaraj [:WDM]]

When implementing a OMA CP application on top of DSDS support I need a way to differentiate from which SIM the OMA CP message was received from and to which SIM the message applies to.

Comment 1

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Ken,
Per the discussion, we'll need to include the "which SIM gets the message" information.
Please help to file engineering bugs.
This shall also apply to bug 947195 and bug 947192.

Comment 2

[Ken Chang[:kenkai]]

(In reply to Wesley Huang [:wesley_huang] from comment #1)
> Hi Ken,
> Per the discussion, we'll need to include the "which SIM gets the message"
> information.
> Please help to file engineering bugs.
> This shall also apply to bug 947195 and bug 947192.

Have created bug 951999 for this.

Comment 3

[Gabriele Svelto [:gsvelto]]

Since client provisioning messages are handled by the wappush app I think this should be moved to the Gaia::WapPush component. Do we already have a UX flow for displaying from which SIM a message belongs?

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Gabriele Svelto [:gsvelto] from comment #3)
> Since client provisioning messages are handled by the wappush app I think
> this should be moved to the Gaia::WapPush component. Do we already have a UX
> flow for displaying from which SIM a message belongs?

I think Ayman is going to take care of it, at least we had a chat a few days ago about the OMA CP message dance and how it is supposed to work for multi ICC card devices.

Comment 5

[Noemí Freire (:noemi)]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> (In reply to Gabriele Svelto [:gsvelto] from comment #3)
> > Since client provisioning messages are handled by the wappush app I think
> > this should be moved to the Gaia::WapPush component. Do we already have a UX
> > flow for displaying from which SIM a message belongs?
> 
> I think Ayman is going to take care of it, at least we had a chat a few days
> ago about the OMA CP message dance and how it is supposed to work for multi
> ICC card devices.

ni to Neo Hsieh who will be the one providing UX flows for covering dsds OMA CP scenario. Thanks!

Comment 6

[Neo Hsieh [:neo]]

Hi Joe, About OMA CP UX spec document. According to workweek discussion. That will be handled by TEF side. 
Can you help to verify that ? Thank you.

Comment 7

[Noemí Freire (:noemi)]

(In reply to Neo Hsieh from comment #6)
> Hi Joe, About OMA CP UX spec document. According to workweek discussion.
> That will be handled by TEF side. 
> Can you help to verify that ? Thank you.

Hi Neo,

ni to Ayman who will provide dsds OMA CP UX spec. Sorry for the misunderstanding. Thanks!

Comment 8

[ayman maat :maat]

Attachment: FFOS_OMA_DSDS_V1.4_20140205_V2.0.pdf

Release note

new wireframes
- none

updated wireframes
- none

deleted wireframes
- none

new flows
- none

updated flows
OMA CP : USERPIN
- ‘3. PIN not entered’ and ‘3. PIN entered’ PIN indication moved and annotation appropriately
- ‘1. Lock screen notification’ annotation updated

OMA CP : NETWPIN
- ‘1. Lock screen notification’ annotation updated

deleted flows
- none

Comment 9

[Wilfred Mathanaraj [:WDM]]

to be considered for partner contribution or future work - added to backlog.

Comment 12

[jinghua.xing]

Attachment: Apply the OTA message to the SIM card which received this message

The message object the wappush app received contains a serviceId property, and the value of this property figured out whether the SIM 1 or SIM 2 received this OTA message. 
To fix the problem which we cannot configure the OMA CP message right in DSDS devices, my solution is using the value of the serviceId property to obtain the mcc and mnc of the right sim card which received the message, and then add the apns parsed from the message to the apn list of the corresponding  operator.

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8449258 [details] [diff] [review]
Apply the OTA message to  the SIM card which received this message

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

(In reply to jinghua.xing from comment #12)
> Created attachment 8449258 [details] [diff] [review]
> Apply the OTA message to  the SIM card which received this message

First of all thanks so much for taking care of it. I'm glad to see your contribution here.

> The message object the wappush app received contains a serviceId property,
> and the value of this property figured out whether the SIM 1 or SIM 2
> received this OTA message. 

Yes, you're right.

> To fix the problem which we cannot configure the OMA CP message right in
> DSDS devices, my solution is using the value of the serviceId property to
> obtain the mcc and mnc of the right sim card which received the message, and
> then add the apns parsed from the message to the apn list of the
> corresponding  operator.

That's the plan I also had for this.

We would need to follow the UX specs attached in this bug and provide some test cases for the changes being added here.

P.S. I'm not a peer of this app but I'll be happy to provide you some feedback if needed.

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

Tim, do you know who is module owner for wappush? Can't find it in the module list.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Usually that means you should |git count -- apps/wappush| :)

I think Gabriele Svelto and Comms team people knows this app the best.

Comment 16

[Gabriele Svelto [:gsvelto]]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #14)
> Tim, do you know who is module owner for wappush? Can't find it in the
> module list.

I'm the de-facto owner as I was the one who put the app together in the first place. However for the content provisioning messages part which was mostly written by :jaoo himself I usually defer reviews to him as he knows the code best.

If we're blocked on the review here however I can do that if :jaoo doesn't have enough time right now.

Comment 17

[jinghua.xing]

Comment on attachment 8449258 [details] [diff] [review]
Apply the OTA message to  the SIM card which received this message

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

Thank for Gabriele Svelto's reply for my patch.

Hi :jaoo :
Would you like to help us to review this patch?
Thank you.

Comment 18

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8449258 [details] [diff] [review]
Apply the OTA message to  the SIM card which received this message

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

(In reply to Gabriele Svelto [:gsvelto] from comment #16)

> If we're blocked on the review here however I can do that if :jaoo doesn't
> have enough time right now.

I'll be happy to review this.

(In reply to jinghua.xing from comment #17)
> Comment on attachment 8449258 [details] [diff] [review]
> Apply the OTA message to  the SIM card which received this message
> 
> Review of attachment 8449258 [details] [diff] [review]:
> -----------------------------------------------------------------

> Would you like to help us to review this patch?

Sure, as I commented at comment 13 the patch LGMT but we need to follow the UX specs attached in this bug and provide some test cases.

Comment 19

[jinghua.xing]

Attachment: Apply the OTA message to the SIM card which received this message

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #18)
> Sure, as I commented at comment 13 the patch LGMT but we need to follow the
> UX specs attached in this bug and provide some test cases.

Hi :jaoo:

I am sorry for have misunderstood your comment before. I have revise my patch to make it follow the UX specs attached in this bug, please review it. But I have no idea of how to provide the test cases, can you help me or guide me to write the test cases?

Thank you.

Comment 20

[jinghua.xing]

Attachment: patch for add apns to the right sim card and some testcases

Hi :jaoo:

I have revise my patch to make it follow the UX specs attached in this bug, please review it and try to add some testcases. But I have no idea of how to run the test cases, can you help me to review this patch?

Thank you.

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

(In reply to jinghua.xing from comment #20)
> Created attachment 8455124 [details] [diff] [review]
> patch for add apns to the right sim card and some testcases

> I have revise my patch to make it follow the UX specs attached in this bug,
> please review it and try to add some testcases.

Thanks, I'll have a look at it and let you know.

> But I have no idea of how to
> run the test cases, can you help me to review this patch?

Read about unit testing at [1] please.

[1] https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests

Comment 22

[José Antonio Olivera Ortega [:jaoo]]

Hey jinghua.xing, the patch no longer applies cleanly on Gaia master branch. Could you please rebase it and even open a pull request for this patch please? Thanks!

Comment 23

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8455124 [details] [diff] [review]
patch for add apns to the right sim card and some testcases

Please, request review again when you are done please.

Comment 24

[jinghua.xing]

Attachment: configure_apn_to_right_sim_card_master.patch

Hi,:jaoo

Comment 25

[jinghua.xing]

Hi :jaoo
Thank you for your advice.
I tried to rebase it on the master branch. I am sorry for have not open a pull request, but there are two problems block me.

1.When I read the code on the master branch,I find it already contains the code to show which card received the message on the notification,as follows (L212):

>    var title = message.sender;
>    
>    if (navigator.mozIccManager &&
>        navigator.mozIccManager.iccIds.length > 1) {
>      var simName = _('sim', { id: +message.serviceId + 1 });
>
>      title = _(
>        'dsds-notification-title-with-sim',
>         { sim: simName, title: title }
>      );
>    }
>
>    var options = {
>     ...
>    };
>
>    var notification = new Notification(title, options);

But when I tried this method on 1.4 branch, I find "navigator.mozIccManager" is always undefined, even if there are two sim card in the devices. 

2. The test case "Add APN to SIM2: Pannon MMS type" in store_test.js is always run to be error. The error information is:

> 1) [wappush-test/unit/store_test.js]  "after each" hook:
>     Uncaught Error: Error: expected [ { carrier: 'Movistar MMS',
>    apn: 'telefonica.es',
>    user: 'telefonica',
>    password: 'telefonica',
>    _id: '9de53e1b-c55b-4762-ae13-edb0ac790c9d',
>    types: [ 'mms' ] } ] to have a length of 2 but got 1 (app://wappush.gaiamobile.org/common/test/helper.js?time=1405673810923:33)
>      at onerror (app://wappush.gaiamobile.org/common/vendor/mocha/mocha.js:4959:7)
  
Can you help me to review this patch and give me some advice to modify my code?

Thank you.

Comment 26

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8458550 [details] [diff] [review]
configure_apn_to_right_sim_card_master.patch

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

Looking really go. I'll review and test the rest tomorrow.

::: apps/wappush/js/cp_screen_helper.js
@@ +76,4 @@
>  
>    /** All APNs list */
>    var apns = null;
> +  

nit: whitespace

@@ +76,5 @@
>  
>    /** All APNs list */
>    var apns = null;
> +  
> +  var cardId = 0;

We already used `iccCardIndex` in store.js to index the ICC card the message came from so I'll prefer keeping `iccCardIndex` instead `cardId`.
Do not forget to add a comment about the meaning of this new variable please.

::: apps/wappush/js/store.js
@@ +29,4 @@
>     *                            useful for unit testing). This function doesn't
>     *                            any parameter.
>     */
> +  function sp_getMccMncCodes(cardId, callback) {

I'll prefer keeping `iccCardIndex` instead `cardId` here as well. Please add a comment as the one included for the callback parameter in the function description.

@@ +42,3 @@
>      // We must add support for multi ICC card devices to the OMA CP logic.
>      // In the meantime we assume the ICC card the WAP push app is working with
>      // is the first one.

Delete the comment above as you are actually adding the support we mention here.

@@ +76,4 @@
>     *                            useful for unit testing). This function doesn't
>     *                            accetp any parameter.
>     */
> +  function sp_provision(parameters, cardId, callback) {

Please add a comment as the one included for the callback parameter in the function description.

@@ +118,3 @@
>  
>          // XXX: Bug 947198
>          // We must add support for multi ICC card devices to the OMA CP logic.

Delete the comment above as you are actually adding the support we mention here.

Comment 27

[jinghua.xing]

Attachment: config_wappush_message_supported_DSDS.patch

Hi :jaoo :
Thank you for your kindly advice :) 

I have made some changes according to your advice. Please help me review my patch.
The two questions mentioned in comment 25 are still there, can you help me to check them?  If there are something wrong in my patch, please let me know immediately.

Thank you

Comment 28

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8460172 [details] [diff] [review]
config_wappush_message_supported_DSDS.patch

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

Well, I see a couple of issues here. First, you always find `navigator.mozIccManager` undefined because the app is lacking of the permission to use the ICC manager API. Add the '"mobileconnection":{}' permission to the manifest. I really don't know how the existing use of the ICC manager is working in the app (I have not seen it working to be honest). Second, the 'ril.data.cp.apns' setting must be an array from now on as the 'ril.data.apnSettings' setting is. The OMA CP APNs must be saved in the corresponding index of that array. I mean, save the OMA CP APNs for the first ICC card in the first position of the 'ril.data.cp.apns' array and so on. As you are becoming the 'ril.data.cp.apns' setting to an array you must update the logic of the usage of that settings in the setting app.

If you need further information or help please ping me on IRC.

::: apps/wappush/js/store.js
@@ +70,4 @@
>     * Stores the APNs into the settings database.
>     *
>     * @param {Array} parameters The array containing the APNs.
> +   * @param {Number} iccCardIndex The number means which card receive this 

nit: trailing whitespace

Comment 29

[jinghua.xing]

Hi jaoo :
I am sorry about I cannot access to the  IRC. Can I write email to you if I have more questions ?

I think the position where the OMA CP APNs be saved in 'ril.data.cp.apns' setting is due to the mcc and mnc of operator which the card received the message belongs to. The 'ril.data.cp.apns' setting is not an array but an object which stores all operators' apns. My work is just save the  OMA CP APNs to the right position of this object.

How do you see about this ?

Thank you.

Comment 30

[Rachelle Yang [:ryang][[e-mail]]]

Hi José and Jinghua, do you have any further progress on this ? Could you please kindly update ?
Thanks a lot !

Comment 31

[jinghua.xing]

Attachment: pull request for this issue

jaoo:

I have open a pull request for this issue. Can you help me to review it?

Thank you.

Comment 32

[Jason.Liu]

[Blocking Requested - why for this release]:

Comment 33

[Bruce Huang [:bhuang] (inactive)]

Hi Gabriele,
Since Jose is on PTO would you be comfortable to help on the review?

Comment 34

[Gabriele Svelto [:gsvelto]]

(In reply to Bruce Huang [:bhuang] <[e-mail]> from comment #33)
> Hi Gabriele,
> Since Jose is on PTO would you be comfortable to help on the review?

Sure, I can do this.

Comment 35

[Wayne Chang [:wchang]]

1.4 is not taking new features. 

Patch will be reviewed for master

Comment 36

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8463934 [details] [review]
pull request for this issue

Redirecting the review, I will do it ASAP.

Comment 37

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8463934 [details] [review]
pull request for this issue

Sorry for the delay; I have also been on PTO for a few weeks and couldn't finish the review before. The patch logic is mostly sound but the unit-tests require some serious overhauling. I've put extensive comments on the GitHub PR but if you need more information you can reach me at my e-mail address.

Comment 38

[jinghua.xing]

Comment on attachment 8463934 [details] [review]
pull request for this issue

Hi Gabriele:

Thank you for your review for my pull request. I have change the code according to your advices. Can you help me to review my modification？ If there are still mistakes in my code, please let me know without hesitation.

Thank you.

Comment 39

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8463934 [details] [review]
pull request for this issue

LGTM, feel free to land on a green Try.

Comment 40

[Ravi Dandu]

Gabriele, Can you review if this patch will be directly applicable to 1.4 too? A customer is looking at 1.4 deployment of DSDS.

Comment 41

[Gabriele Svelto [:gsvelto]]

(In reply to rdandu from comment #40)
> Gabriele, Can you review if this patch will be directly applicable to 1.4
> too? A customer is looking at 1.4 deployment of DSDS.

Sure, I'll do and since the author hasn't landed this yet I'll also land it myself.

Comment 42

[Gabriele Svelto [:gsvelto]]

Landed on gaia/master 05176f64f15efd92c654d0eed801991ab2199bf8

https://github.com/mozilla-b2g/gaia/commit/05176f64f15efd92c654d0eed801991ab2199bf8

Comment 43

[Gabriele Svelto [:gsvelto]]

The patch here can be cherry picked cleanly to v2.0 but not to v1.4. It depends on bug 1015875 and bug 1023338 and neither of those is 1.4+ so getting approval for that won't be trivial. Could you handle the approval requests? I'm terribly busy right now but once you we have approval I can handle all the uplifting to v1.4 myself.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Reverted for Linter failures.
https://github.com/mozilla-b2g/gaia/commit/feb85187bd77862a48a35970b11a7b21c27b19b7

https://tbpl.mozilla.org/php/getParsedLog.php?id=46096639&tree=B2g-Inbound

Comment 45

[jinghua.xing]

Attachment: pull request for this issue

I feel so sorry for the linter failures caused by my careless. 

I have modified the code for this issue and opened a new pull request for it. Can you help me to review it again and land it if it is right?

Thank you.

Comment 46

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8474333 [details]
pull request for this issue

(In reply to jinghua.xing from comment #45)
> I feel so sorry for the linter failures caused by my careless.

I am to blame since I reviewed the patch and forgot to run the linter and I must have missed it in the try run. Anyway there's some intermittent issues still happening so I'll be waiting until Try runs green before merging this:

https://tbpl.mozilla.org/?rev=2cd9b61a371fa1bf8968b2c7cbaed6134a502851&tree=Gaia-Try

Comment 47

[Gabriele Svelto [:gsvelto]]

Re-landed, this time it should be fine, sorry for the inconvenience.

https://github.com/mozilla-b2g/gaia/commit/b33b4d9558e0b9eabbfda7be23435e2b38fd40bf

Comment 48

[jinghua.xing]

Hi gsvelto:

As the change is landed on the master branch, can you help me uplift it to the v1.4 branch? 

This change is related to bug1023338、bug1015875 and bug947192. And when I looking the bug1023338, I find that there is no gaia/shared/js/uuid.js file in v1.4 branch.Please handle these at the same time.

Thank you.

Comment 49

[Gabriele Svelto [:gsvelto]]

(In reply to jinghua.xing from comment #48)
> As the change is landed on the master branch, can you help me uplift it to
> the v1.4 branch? 

Yes, I've asked :rdandu if he can handle the approval process since this requires uplifting more bugs too, see comment 43.

> This change is related to bug1023338、bug1015875 and bug947192. And when I
> looking the bug1023338, I find that there is no gaia/shared/js/uuid.js file
> in v1.4 branch.Please handle these at the same time.

Yes, as I've already mentioned this requires uplifting bug 1015875 and bug 1023338 too. I'm not sure if bug 947192 is a dependency too but it might. All of this makes uplifting this patch quite complicated so late in the v1.4 branch.

Comment 50

[Ravi Dandu]

We're not pulling into 1.4, due to added risk due to complication of uplifting on 1.4, and customer not mandating this.

Comment 51

[Eric Chang [:ericcc] [:echang]]

Attachment: 2014-10-09-15-28-52.png

Checked on Flame, Message received, APN installed for SIM2 and device can make data call using that APn.
Gaia-Rev        55ce3612bd8a8665139d6b85114ce59993a3fa0a
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/8811060bf3fe
Build-ID        20141008160203
Version         34.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141008.191730
FW-Date         Wed Oct  8 19:17:41 EDT 2014
Bootloader      L1TC00011840