991628 - [Settings][OMA CP] Not possible to select an APN received via OMA CP message if there is already one with the same name

Status: RESOLVED FIXED

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

Description

[isabelrios]

1.3 04/03 buri build: 
Gecko-1bbed3c
Gaia-c5cd3a1

STEPS
Receive an OMA CP message with data APN
Accept it and store the APN
Receive the same OMA CP message again
Accept it again and store the APN
Go to Settings -> Cellular and Data -> Data settings and try to select the second APN received and stored

EXPECTED
It should be possible to select any of the APN stored and listed in Data Settings

ACTUAL
When there are two APNs with the same name, it is not possible to select the second one of the list

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Go for it. I'll fix this bug asap. Thanks.

Comment 2

[Beatriz Rodríguez [:brg]]

Thanks Jose Antonio.
Bug reported as certification blocker, nominating to 1.3.

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP

WIP patch.

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

v1 patch. It still needs tests.

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Pointer to Github PR https://github.com/mozilla-b2g/gaia/pull/18069

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

Arthur, let me share some background about what is going on here. If a couple or more APNs have the same name (same value for carrier property) there was an issue while switching between those APNs. This patch fixes that issue by using a hash code for the value property for the input element. Could you take a look a it please? I added a test case for the issue, hope it helps.

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v3

Arthur, just addressed the comment you left in the PR. Thanks. Could you take a look a it again please? Thanks.

PS. PR updated. Feel free to review the patch through it.

Comment 8

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8403839 [details] [diff] [review]
v3

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

The patch looks good to me. The only one concern is that is it possible to use a more sophisticated hash algorithm? The chance of collision may be small but I think it is always good using a more robust method.

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v4

Function changed. Could you take a look a it again please? Thanks.

Comment 10

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8403933 [details] [diff] [review]
v4

r=me, thank you! Please also create a pull request against v1.3 branch for running travis.

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

Attachment: [v1.3] Pointer to Github PR https://github.com/mozilla-b2g/gaia/pull/18172

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

Travis passes for both master and v1.3 branches. I tested both patches on peak device and everything seems to work correctly. Gaia master branch patch landed at https://github.com/mozilla-b2g/gaia/commit/5c56b25486e2b9ac438ef9c3a7825ed4445d5b34.

Thanks Arthur!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please request approval-gaia-v1.3 when this is ready for uplift.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/a859e2d3e7bbc3fd9f33b1a1bdd2b324469586f9

Still waiting on the v1.3 approval request.

Comment 15

[isabelrios]

Just checked the current behavior on 1.4 today's buri build: 
Gecko-3f64d65
Gaia-961f7d4

Now it is possible to select any of the apns with the same name, that is great. But, another issue happens. If there are for example three apns with the same name, and the third of them is selected, if data settings menu is closed and open again, then the first one is selected automatically. Please see video file attached.
Instead of re-opening this bug if you agree I will open a follow up one.

Comment 16

[isabelrios]

Attachment: first apn selected automatically

Comment 17

[isabelrios]

Follow up bug 998914 already open

Comment 18

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> Still waiting on the v1.3 approval request.

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8404676 [details] [review]
[v1.3] Pointer to Github PR https://github.com/mozilla-b2g/gaia/pull/18172

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): DSDS stuff / bug 962453
[User impact] if declined: If there are some APNs with the same carrier name the user is not able to switch between them.
[Testing completed]: Yes, unit test added too.
[Risk to taking this patch] (and alternatives if risky): Low, in fact this is a corner case. I mean all the APN in the apn.json data base have different names. Only APNs set through OMA client provisioning (CP) could have the same carrier name for all of them and the carrier rarely sends this APNs to the device.
[String changes made]: No changes.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

v1.3: https://github.com/mozilla-b2g/gaia/commit/32a9e3db738e0b3bc44a4d4d5c16512a2617a2cf