989852 - [Sora][STK]'OK ' item is grayed when execute 'Get input' with a default text and user input command.

Status: RESOLVED FIXED

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

Description

[sync-1]

Mozilla build ID: 20140312164001
 
 Created an attachment (id=675030)
 issue image
 
 DEFECT DESCRIPTION:
 'OK ' item is grayed when execute 'Get input' with a default text and user input command.
 
  REPRODUCING PROCEDURES:
 Precondition:
 MS load simulated SIM card and SATK application which can send "get input" procative SIM command.
 1.Run test case STM03009->1st Command (Response Length 4-8 ), find default text '1234' display in the put screen,but 'OK' item is grayed,user can not press it (refer to attachment)->KO.
 
 Note: Sometimes is OK,but sometimes is fail.
 
  EXPECTED BEHAVIOUR:
 'OK' item should display normally and user can press it to execute the command.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 Spec 11.14 6.4.3
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 Important.
 
  REPRODUCING RATE:
 2/5.
 
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[sync-1]

Attachment: fail adb log

Comment 2

[sync-1]

Attachment: spy log image

Comment 3

[sync-1]

Attachment: issue image

Comment 4

[xiaokang.chen]

Dear Fernando,
Can you have a look at this? Thanks a lot! It happens occasionally.

Comment 5

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi XiaoKang -

It this reproducible on Buri V1.1?

Comment 6

[xiaokang.chen]

Hi Vance,
I have asked QA about this. I will add comment here if I got the answer.

Comment 7

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

I'll check asap

Comment 8

[Preeti Raghunath(:Preeti)]

Vance,

Please check for cert blocking here.

Comment 9

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Hi,

The received command (based on the logs) is:

{
  "commandNumber":1,
  "typeOfCommand":35,
  "commandQualifier":0,
  "options":{
    "text":"Default Text = 24H",
    "duration":{
      "timeUnit":1,
      "timeInterval":2
    },
    "minLength":2,
    "maxLength":3,
    "defaultText":"24H"
  }
}

As you can see, there are to parameters (minLength and maxLength) which control the amount of chars allowed before send response to the SIM Card.

So, until you don't write almost 2 characters, the button will be grayed.
Also, the maxLength will limit the max number of them.

Comment 10

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Sorry, the command for the provided photo is this one:

I/Gecko   (  287): -*- [SUB0] QCContentHelper_QC_B2G: sendMessage to content process: icc-stkcommand{ iccId : '89441000000400617452', command : { commandNumber : 1, typeOfCommand : 35, commandQualifier : 0, options : { text : 'Default Text = 1234', duration : { timeUnit : 1, timeInterval : 2,  }, minLength : 4, maxLength : 8, defaultText : '1234',  },  } }

In this case the length should be between 4 and 8

Comment 11

[xiaokang.chen]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #10)
> Sorry, the command for the provided photo is this one:
> 
> I/Gecko   (  287): -*- [SUB0] QCContentHelper_QC_B2G: sendMessage to content
> process: icc-stkcommand{ iccId : '89441000000400617452', command : {
> commandNumber : 1, typeOfCommand : 35, commandQualifier : 0, options : {
> text : 'Default Text = 1234', duration : { timeUnit : 1, timeInterval : 2, 
> }, minLength : 4, maxLength : 8, defaultText : '1234',  },  } }
> 
> In this case the length should be between 4 and 8

The Qa questioned that the defaultText's length is already between 4 and 8. Why it should be gray? We only ungrayed the OK button until the user input the proper length???

Comment 12

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Yes, I agree, you're right. Sorry for the confusion.

I think this is related to the workaround. I'll fix today:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/icc.js#L447

Comment 13

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Proposed patch

Comment 14

[Noemí Freire (:noemi)]

(In reply to Preeti Raghunath(:Preeti) from comment #8)
> Vance,
> 
> Please check for cert blocking here.

Recovering ni flag to Vance, it was removed by mistake. 

On the other hand, the issue is reproducible and has been reopened so recovering 1.3? flag too.

Sorry for the inconvenience.

Comment 15

[bhavana bajaj [:bajaj]]

Waiting for vance to get back on this isse in case this is a partner blocker. As this is an STK issue and we don't have access to the testcase

Comment 16

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Yes, this is a cert blocker. But i believe Fernando already has patch ready

Comment 17

[xiaokang.chen]

Hi Fernando,
I will test it and give you feedback ASAP.

Comment 18

[xiaokang.chen]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #13)
> Created attachment 8401203 [details] [review]
> Proposed patch

The patch works well. Thanks for your strong support!

Comment 19

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8401203 [details] [review]
Proposed patch

f+ (see https://bugzilla.mozilla.org/show_bug.cgi?id=989852#c18)

Comment 20

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

Comment on attachment 8401203 [details] [review]
Proposed patch

Can you add some ICC unit tests too ?

Comment 21

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #20)
> Comment on attachment 8401203 [details] [review]
> Proposed patch
> 
> Can you add some ICC unit tests too ?

Hi Vivien,

Unit tests added, if you agree, I'll land.

Comment 22

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

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #21)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> needinfo? please) from comment #20)
> > Comment on attachment 8401203 [details] [review]
> > Proposed patch
> > 
> > Can you add some ICC unit tests too ?
> 
> Hi Vivien,
> 
> Unit tests added, if you agree, I'll land.

Sounds good to me then.

Comment 23

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Rebased & Landed: https://github.com/mozilla-b2g/gaia/commit/ff1dcbd8ffcc6e4490f7afb091bac01514fca869

Comment 24

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8401203 [details] [review]
Proposed patch

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

Because a workaround, the dialog buttons were disabled.
This patch fix it checking the input box content as soon as it is filled with default data.

[User impact] if declined: User shall remove and insert the text again not allowing use the default provided value.

[Testing completed]: Yes, and added extra STK tests
[Risk to taking this patch] (and alternatives if risky): Very low
[String changes made]: No

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 8401203 [details] [review]
Proposed patch

Thanks for the test. Patch is already verified, looks good to land.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/4202bbda7118abe1f2bf22d44e6c836f2469a0da
v1.3: https://github.com/mozilla-b2g/gaia/commit/7287309e0e65b937ac3cebae7254565ef1c82e3a

shared/test/unit/mocks/mock_dump.js doesn't exist on v1.3, so I ignored that change.

Comment 27

[Kevin Grandon :kgrandon]

This is causing both linter and unit test issues in 1.3 and 1.4. I would recommend addressing this by preparing pull requests against both branches, and ensuring travis is green before landing.

1.4 backout: https://github.com/mozilla-b2g/gaia/commit/26983f356ecb1bcf30e862d334b5de790071803e
1.3 backout: https://github.com/mozilla-b2g/gaia/commit/225e9ae08f497611c32928b493833bea0ce76197
1.3 follow-up backout: https://github.com/mozilla-b2g/gaia/commit/0249eb8c78832dee159ba6ce0cf5f3aa39c47b07

Example of linter issue: 

----- FILE : /home/travis/build/mozilla-b2g/gaia/apps/system/test/unit/icc_test.js -----

Line 85, E:0002: Missing space after ":"

Line 87, E:0002: Missing space after ":"

----- FILE : /home/travis/build/mozilla-b2g/gaia/apps/system/test/unit/icc_worker_test.js -----

Line 44, E:0002: Missing space after ":"

Line 46, E:0002: Missing space after ":"

Found 4 errors, including 0 new errors, in 2 files (1395 files OK).

Comment 28

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

In master Travis passed: https://travis-ci.org/mozilla-b2g/gaia/builds/22463564

In 1.3 failed linter (different linter) I'll fix these two issues:
https://travis-ci.org/mozilla-b2g/gaia/builds/22470180

Also, in 1.3 failed loading a Mock (MockDump)

I saw that the merge caused some conflicts which could affect to these tests.

In 1.4 failed a test by timeout, anyway, some conflicts in this merge could affect this too.

I'll check the patch and fix it for 1.3 and 1.4.

@vingtetun, Should we backout also the patch landed in master in order to have only one commit for the 3 branches? or is better to create different patches for 1.3 and 1.4 based in this one.

Comment 29

[Kevin Grandon :kgrandon]

It's not always possible to have the same commit in every branch - so generally it's better to have a different commit when needed. I would recommend opening a pull request for each branch to ensure travis is green before landing.

Comment 30

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Kevin Grandon :kgrandon from comment #29)
> It's not always possible to have the same commit in every branch - so
> generally it's better to have a different commit when needed. I would
> recommend opening a pull request for each branch to ensure travis is green
> before landing.

Ok, I agree.

And with the landed in master? we leave it without any change?

Comment 31

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

What Kevin says.

Comment 32

[Kevin Grandon :kgrandon]

Master is green, so it's fine to leave the commit in there. If there's any reason you would prefer to back it out we can certainly do that, though it shouldn't be required.

Comment 33

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

To uplift in 1.3 and 1.4, bug 988362 should be uplifted first to avoid race-conditions.

Comment 34

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Patch for v1.3

WIP (For travis checking)

Comment 35

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Patch for v1.4

WIP (For travis checking)

Comment 36

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8403297 [details] [review]
Patch for v1.3

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


Because a workaround, the dialog buttons were disabled.
This patch fix it checking the input box content as soon as it is filled with default data.

[User impact] if declined: User shall remove and insert the text again not allowing use the default provided value.

[Testing completed]: Yes, and added extra STK tests
[Risk to taking this patch] (and alternatives if risky): Very low
[String changes made]: No

Comment 37

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8403300 [details] [review]
Patch for v1.4

Travis failed but because connection refused  with the test agent which is out of the scope of this patch. Any idea how to solve this?

Running all tests

./node_modules/test-agent/bin/js-test-agent test --server ws://localhost:8789 --reporter Min

events.js:72

throw er; // Unhandled 'error' event

^

Error: connect ECONNREFUSED

at errnoException (net.js:901:11)

at Object.afterConnect [as oncomplete] (net.js:892:19)

make: *** [test-agent-test] Error 8

Comment 38

[Kevin Grandon :kgrandon]

Might be an intermittent issue. I've restarted the job and let's see what it does.

Comment 39

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Kevin Grandon :kgrandon from comment #38)
> Might be an intermittent issue. I've restarted the job and let's see what it
> does.

Oh, thanks

Comment 40

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8403300 [details] [review]
Patch for v1.4

Travis passed.


Because a workaround, the dialog buttons were disabled.
This patch fix it checking the input box content as soon as it is filled with default data.

[User impact] if declined: User shall remove and insert the text again not allowing use the default provided value.

[Testing completed]: Yes, and added extra STK tests
[Risk to taking this patch] (and alternatives if risky): Very low
[String changes made]: No

Comment 41

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Patches for 1.3 and 1.4 finally fixed and travis is Ok.
Closing bug.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/96e880ec5e9839d3bdf229eb339fb7c6d85b6a03
v1.3: https://github.com/mozilla-b2g/gaia/commit/d98c34ba0f36e3c48bd53e5a2fa5e8fb7021ca43

Comment 43

[Jason Smith [:jsmith]]

Loli - Can you look into adding test coverage for this?

Comment 44

[Loli (:lolimartinezcr)]

(In reply to Jason Smith [:jsmith] from comment #43)
> Loli - Can you look into adding test coverage for this?

Hi Jason,

I think that Mozilla's person should add this test cases in Moztrap, because I haven't tested this bug.

Comment 45

[Jason Smith [:jsmith]]

(In reply to Loli from comment #44)
> (In reply to Jason Smith [:jsmith] from comment #43)
> > Loli - Can you look into adding test coverage for this?
> 
> Hi Jason,
> 
> I think that Mozilla's person should add this test cases in Moztrap, because
> I haven't tested this bug.

STK to my understanding is owned by TEF, not Moz, so I don't think Moz can be on point here for adding test cases here.

Comment 46

[Loli (:lolimartinezcr)]

(In reply to Jason Smith [:jsmith] from comment #45)
> (In reply to Loli from comment #44)
> > (In reply to Jason Smith [:jsmith] from comment #43)
> > > Loli - Can you look into adding test coverage for this?
> > 
> > Hi Jason,
> > 
> > I think that Mozilla's person should add this test cases in Moztrap, because
> > I haven't tested this bug.
> 
> STK to my understanding is owned by TEF, not Moz, so I don't think Moz can
> be on point here for adding test cases here.

Jason, STK is owned by TEF. We haven't got SIM card  which we can test it.

Comment 47

[Jason Smith [:jsmith]]

Ah okay - in that case, I'll mark in-moztrap- then.