905173 - PIN code screen should not display "3 tries left." when loading

Status: RESOLVED FIXED

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

Description

[Anthony Ricaud (:rik)]

"3 tries left." is briefly displayed at every device launch before disappearing. We should never display it when there is no error.

Comment 1

[Anthony Ricaud (:rik)]

So this id=triesLeft element showing up. We could just put hidden=true in the markup to fix this.

But I dig up a bit and IccHelper.getCardLockRetryCount() seems to always returns 0. Hence id=triesLeft is never shown. Instead, when we receive a PIN code error, we display id=errorMsg.

This was introduced in bug 860660. Fernando, could we remove the whole triesLeft logic here or am I missing something?

Comment 2

[Fabien Cazenave [:kaze]]

Bug confirmed on b2g18 and moz-central: IccHelper.getCardLockRetryCount() always returns 0.

This can be quite serious in some scenarios, e.g.:
 • a user tries to enable the SIM security and does two errors ⇒ gets scared and quits the Settings app;
 • later, having forgotten that two attempts have been tried, gets back to the Settings app and tries to enable the SIM security again ⇒ no warning is displayed ⇒ risk of blocking.

Comment 3

[Fernando Campo (:fcampo)]

(In reply to Anthony Ricaud (:rik) from comment #1)
> So this id=triesLeft element showing up. We could just put hidden=true in
> the markup to fix this.
I think this is the best option

> But I dig up a bit and IccHelper.getCardLockRetryCount() seems to always
> returns 0. Hence id=triesLeft is never shown. Instead, when we receive a PIN
> code error, we display id=errorMsg.
IccHelper returning always 0 seems like a bug.

> This was introduced in bug 860660. Fernando, could we remove the whole
> triesLeft logic here or am I missing something?
As Kaze states on comment 2, the logic behind showing the number of retries when loading the screen is needed to avoid blocking the phone.

So my proposal here is to hide by default the number of retries, and show it only if it's > 0, then open a new bug about IccHelper returning always 0.

I understand also that having the number of retries shown in two different places is a mistake, but that is more a UX thing, and probably will be dealt with on bug 852682.

Comment 4

[Fernando Campo (:fcampo)]

Attachment: Link to PR - https://github.com/mozilla-b2g/gaia/pull/12758

I was going to put Kaze as reviewer, but saw that his list of pending reviews is longer than my shopping list :D
Rik, if you can't review this, blank the r and I'll try to re-assign (but honestly I think it's a quick one)

Cheers!

Comment 5

[Anthony Ricaud (:rik)]

Comment on attachment 814943 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/12758

OK, I understood the distinction between triesLeft (displayed when arriving on the screen) and errorMsg (displayed after a user submits a code).

I've seen one logic error that I commented in the PR.

Can you adapt the HTML in the tests and probably add new tests?

Comment 6

[Fernando Campo (:fcampo)]

Comment on attachment 814943 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/12758

Good catch with the attribute!

I should change all the class='hidden' for attributes, to be honest, but we can do that on a polishing bug

Also, tests fixed

Comment 7

[Fernando Campo (:fcampo)]

Comment on attachment 814943 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/12758

Just realized that I didn't assign the review, sorry

Comment 8

[Anthony Ricaud (:rik)]

Comment on attachment 814943 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/12758

Excellent, thank you very much!

Comment 9

[Fernando Campo (:fcampo)]

master: 43091d1a13e4bbb2693f5238c80307fca36e2175

Thanks!

Comment 10

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x -m1 43091d1a13e4bbb2693f5238c80307fca36e2175
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 11

[Fernando Campo (:fcampo)]

uplifted to 1.2 91b8ead867fd68bbe30ded6a8aa8861b95f4c536