Closed Bug 989852 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S5 (11apr)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: sync-1, Assigned: frsela)

References

Details

(Whiteboard: [cert])

Attachments

(6 files)

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:
Attached file fail adb log
Attached image spy log image
Attached image issue image
Dear Fernando,
Can you have a look at this? Thanks a lot! It happens occasionally.
blocking-b2g: --- → 1.3?
Flags: needinfo?(frsela)
Hi XiaoKang -

It this reproducible on Buri V1.1?
Flags: needinfo?(chenxk)
Hi Vance,
I have asked QA about this. I will add comment here if I got the answer.
Flags: needinfo?(chenxk)
I'll check asap
Flags: needinfo?(frsela)
Vance,

Please check for cert blocking here.
Flags: needinfo?(vchen)
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(vchen)
Resolution: --- → INVALID
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
blocking-b2g: 1.3? → ---
(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???
Flags: needinfo?(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
Assignee: nobody → frsela
Status: RESOLVED → REOPENED
Flags: needinfo?(frsela)
Resolution: INVALID → ---
Attached file Proposed patch
Attachment #8401203 - Flags: feedback?(sync-1)
Flags: needinfo?(sync-1)
(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.
blocking-b2g: --- → 1.3?
Flags: needinfo?(vchen)
Flags: needinfo?(sync-1) → needinfo?(chenxk)
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
Yes, this is a cert blocker. But i believe Fernando already has patch ready
Flags: needinfo?(vchen)
blocking-b2g: 1.3? → 1.3+
Hi Fernando,
I will test it and give you feedback ASAP.
Flags: needinfo?(chenxk)
(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 on attachment 8401203 [details] [review]
Proposed patch

Can you add some ICC unit tests too ?
Attachment #8401203 - Flags: review?(21) → review+
(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.
Flags: needinfo?(21)
(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.
Flags: needinfo?(21)
Target Milestone: --- → 1.4 S5 (11apr)
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
Attachment #8401203 - Flags: approval-gaia-v1.4?(release-mgmt)
Attachment #8401203 - Flags: approval-gaia-v1.3?(release-mgmt)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Comment on attachment 8401203 [details] [review]
Proposed patch

Thanks for the test. Patch is already verified, looks good to land.
Attachment #8401203 - Flags: approval-gaia-v1.4?(release-mgmt)
Attachment #8401203 - Flags: approval-gaia-v1.4+
Attachment #8401203 - Flags: approval-gaia-v1.3?(release-mgmt)
Attachment #8401203 - Flags: approval-gaia-v1.3+
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: needinfo?(21)
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(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?
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.
To uplift in 1.3 and 1.4, bug 988362 should be uplifted first to avoid race-conditions.
Depends on: 988362
Attached file Patch for v1.3
WIP (For travis checking)
Attached file Patch for v1.4
WIP (For travis checking)
Whiteboard: cert
Whiteboard: cert → [cert]
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
Attachment #8403297 - Flags: approval-gaia-v1.3?(release-mgmt)
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
Flags: needinfo?(21)
Might be an intermittent issue. I've restarted the job and let's see what it does.
Flags: needinfo?(21)
(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 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
Attachment #8403300 - Flags: approval-gaia-v1.4?(release-mgmt)
Patches for 1.3 and 1.4 finally fixed and travis is Ok.
Closing bug.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Attachment #8403297 - Flags: approval-gaia-v1.3?(release-mgmt)
Attachment #8403300 - Flags: approval-gaia-v1.4?(release-mgmt)
Flags: in-testsuite+
Loli - Can you look into adding test coverage for this?
Flags: in-moztrap?(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.
(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.
(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.
Ah okay - in that case, I'll mark in-moztrap- then.
Flags: in-moztrap?(lolimartinezcr) → in-moztrap-
You need to log in before you can comment on or make changes to this bug.