987428 - Display Pin entry UI for unblocking RUIM perso states.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

Description

[Nivi]

Need to display a pin entry pop up to allow users to unblock the RUIM personalization states.

Comment 1

[Anshul]

Nivi is working on a patch for this issue.

Comment 2

[Jason Smith [:jsmith]]

Can we get more details on what's being requested here? Are there any strings required here?

Comment 3

[Preeti Raghunath(:Preeti)]

The code patch is being worked on by QC. Moz might need to be on the review end. Lets not make changes to the flags.

Comment 4

[Preeti Raghunath(:Preeti)]

Inder,

Please help understand string changes

Comment 5

[Inder]

Nivi/Anshul -- can you guys add some more details here as to what exactly is being done/changed.

Comment 6

[Nivi]

Hi,

With this bugzilla, I am proposing we add gaia support to display pin entry pop-up for entering code for unblocking RUIM perso states. This is the corresponding gecko bug -https://bugzilla.mozilla.org/show_bug.cgi?id=900429. 

We added support in the past for entering pin for other perso states - nck. spck, etc. This is the bugzilla for supporting nck - https://bugzilla.mozilla.org/show_bug.cgi?id=827383 as a reference.

What I propose to do is to just extend the strings and UI as per 827383 so that the UI component is displayed for newer ruim perso states as well. It's a simple change and I am using the changes in bug 827383 to replicate and add new strings and reuse the existing pin entry pop-up UI element.

Let me know if you guys would like to see a draft of the first patch that I have.
I haven't been able to test it yet. But I should get around to doing that soon.

Thanks,
Nivi.

Comment 7

[Preeti Raghunath(:Preeti)]

UX,

Please provide feedback if this is ok by ux.

Comment 8

[Stephany Wilkes]

Flagging Amy and Jacqueline on lockscreen because Casey is sick today. Just a reminder, too, that any patch here must be flagged for ui-review? to firefoxos-us.

Comment 9

[Amy Lee [:amylee] UX]

Attachment: Pin_Code.png

Hi, 

I'm not too clear on what exactly is the screen we need a pin code for. Attached is the pin code screen for lock screen that's going to be implemented.

Comment 10

[Nivi]

Attachment: lockscreen.jpeg

Comment 11

[Nivi]

Hi,

It's the screen that we already have as I have attached in this bug.
We just need to add more strings.
I will upload a patch soon.

Thanks,
Nivi.

Comment 12

[bhavana bajaj [:bajaj]]

(In reply to Nivi from comment #11)
> Hi,
> 
> It's the screen that we already have as I have attached in this bug.
> We just need to add more strings.

Hey nivi, just a quick heads up here, that we have a hard string freeze deadline for 1.4 as 3/28 and this may have missed the ship. If this bugend-up being a blocker:1.4+ (more off a product call) can you workaround by reusing existing strings ?
> I will upload a patch soon.
> 
> Thanks,
> Nivi.

Comment 13

[Nivi]

First draft of the patch - https://github.com/mozilla-b2g/gaia/pull/17775
It's still not working yet. I am in the process of testing.

Comment 14

[Preeti Raghunath(:Preeti)]

Nivi

Can we work with existing strings given that our string freeze was on 3/28?

Comment 15

[Anshul]

Since the official request for this feature didn't come from a partner yet I am okay with moving this to 1.5. However please note that this may be required by our partner and they may ask us to support it for 1.4 close to commercialization.

Comment 16

[Nivi]

Preeti, No we can't use existing strings.

Thanks,
Nivi.

Comment 17

[Nivi]

I have the final patch uploaded. I tested and it works. I uploaded it to gaia/master for now until we decide if we want this on v1.4.

Here's the pull request - https://github.com/mozilla-b2g/gaia/pull/17962

Preeti, could you please add the relevant reviewers?

Thanks,
Nivi

Comment 18

[Nivi]

Attachment: pr.html

Comment 19

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

Comment on attachment 8401618 [details]
pr.html

This is a very bug patch and require reviews from other modules.

The System app part is trivial enough for me to grant a review, however it doesn't include necessary unit tests. The tests there should be trivial too so please add them.

Comment 20

[Fernando Campo (:fcampo)]

Comment on attachment 8401618 [details]
pr.html

Same in my case, code is trivial enough to grant the review, but can't do till it's test covered.
You can use the existing ones on FTU from PIN, PUK, XCK screens as a guide.

Comment 21

[Nivi]

Hi Tim/Fernando,

Thanks for looking at the patch. I will add the necessary tests and upload a patch.

Thanks,
Nivi.

Comment 22

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

Comment on attachment 8401618 [details]
pr.html

r=me for the settings part. It's simply table mapping in settings app, for the part I think it's okay to land it without tests.

Comment 23

[Nivi]

Hi Arthur,

That sounds good to me. Saves me some work on my side. :D

However, if Tim or Fernando still want me to add the tests I could. Please let me know.

Thanks,
Arthur.

Comment 24

[Nivi]

In addition, this change passed verification on our side, if that helps.

Thanks,
Nivi.

(In reply to Nivi from comment #23)
> Hi Arthur,
> 
> That sounds good to me. Saves me some work on my side. :D
> 
> However, if Tim or Fernando still want me to add the tests I could. Please
> let me know.
> 
> Thanks,
> Arthur.

Comment 25

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

I was talking about related unit tests...

https://github.com/mozilla-b2g/gaia/tree/master/apps/system/test/unit

The code passes locally on your side does not means future changes will not break it.

Comment 26

[Fernando Campo (:fcampo)]

I'd rather to have some unit tests also. We already have some for the rest of situations, so shouldn't be difficult, taking the original ones as reference :)

Comment 27

[Nivi]

Tim/Fernando, I will add the tests. Should not be too hard. :)
I just asked because of Arthur's comment #22.

Thanks,
Nivi.

Comment 28

[Ivan Tsay (:ITsay)]

Feature does not block the release

Comment 29

[Nivi]

Attachment: pr.html

Comment 30

[Nivi]

Hi Tim/Fernando,

I have added the unit tests for the new card states. Could someone from your side run the tests and verify for me.

I would really appreciate that.

Thanks,
Nivi.

Comment 31

[Fernando Campo (:fcampo)]

Sorry Nivi, but I don't see any new tests for the FTU part :(
Is the PR updated?

Comment 32

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

Comment on attachment 8407320 [details]
pr.html

The system part is fine. Thank you.

Please add FTU test cases like what :fcampo said.

Comment 33

[Nivi]

Hi Tim/Fernando,

Yes the PR is updated. I did add the new card states to tests under apps/system/test/unit - simslot_test.js, simcard_dialog_test.js, internet_sharing_test.js and lockscreen_conn_info_manager_test.js. It's already in the PR.

As for the FTU unit test, I didn't add anything there on purpose because we test the xck screen using 'networkLocked' card state and that's good enough for these new card states as well. If it works for the networkLocked case, it will work for others. 

I just looked in the unit tests to see which one tests all the existing supported card states and added new card states there.

Please let me know if I am  missing something here.

Thanks,
Nivi.

Comment 34

[Fernando Campo (:fcampo)]

(In reply to Nivi from comment #33)
> 
> As for the FTU unit test, I didn't add anything there on purpose because we
> test the xck screen using 'networkLocked' card state and that's good enough
> for these new card states as well. If it works for the networkLocked case,
> it will work for others.
Totally right, it would work with that for the tests that we have currently.
But, I think those tests are not enough, as we have many states now, and we should also test the changes that those states provoke (mainly textContents on header and warnings).
If those tests doesn't exist at the moment, I think it was an error (probably on my side as reviewer) on previous pull requests, so I'm sorry for that.

One option would be to add them now using the occasion, or open a specific bug for adding those tests in the future. Personally, I prefer the former, but won't be unhappy with the latter, as long as we get them at the end. 
 
> 
> I just looked in the unit tests to see which one tests all the existing
> supported card states and added new card states there.
I would add a line in all the tests on the form of '<state> shows <screen>' checking that the textContent of the different labels are changed properly:
assert.equal(UIManager.unlockSimHeader.textContent, <something>);
assert.equal(UIManager.xckLabel.textContent, <something>);
...or using a spy to check that the function _() was called with the corresponding parameters.

And also add same kind of test for every other state that we are not currently testing, also checking for the textContent changes. e.g.
test('networkLocked shows XCK screen', function(){});
test(corporateLocked'shows XCK screen', function(){});
test('serviceProviderLocked'shows XCK screen', function(){});
test('network1Locked'shows XCK screen', function(){});
test('network2Locked'shows XCK screen', function(){});
test('hrpdNetworkLocked'shows XCK screen', function(){});
test('ruimCorporateLocked'shows XCK screen', function(){});
test('ruimServiceProviderLocked'shows XCK screen', function(){});

> 
> Please let me know if I am  missing something here.
> 
> Thanks,
> Nivi.
As I said before, either we could do that on this bug, or open a new bug just for adding the tests, but those tests I needed, IMHO.

Please give your opinion and I'll finish the review after.

P.S. Sorry for being a little pain in the ass about this :D

Comment 35

[Nivi]

Thanks Fernando for your input. My objective here was to add support for the new card states and add to the existing tests. I will file a bug for the additional tests so we can keep track of it. I will add your above comments to the bug.

For the time being, lets roll with the bare minimum tests if that's ok with you guys.

Thanks,
Nivi.

Comment 36

[Nivi]

Filed a new bug for additional tests - https://bugzilla.mozilla.org/show_bug.cgi?id=997890

Comment 37

[Fernando Campo (:fcampo)]

Comment on attachment 8407320 [details]
pr.html

In that case, now that bug 997890 is open (and we hopefully won't forget about it), r+ to this one on the FTU part :)
Thanks Nivi for taking care of it

Comment 38

[Nivi]

No problem at all Fernando. Thanks for reviewing.

Nivi.

Comment 39

[Nivi]

No problem at all Fernando. Thanks for reviewing.

Nivi.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/bc86c5e3fdc4e112e8c68d07fabd881bcd756c9c

Comment 42

[Jacqueline Savory [:jsavory] UX]

Removing flag as this bug has been fixed