Closed Bug 961271 Opened 6 years ago Closed 6 years ago

[B2G][Contacts]Importing a contact whose phone number has special characters from a SIM replaces the spaces with zeros.

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C3/1.4 S3(31jan)
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: astole, Assigned: allstars.chh)

References

Details

Attachments

(4 files, 6 obsolete files)

Attached file logcat
If a contact's phone number has spaces and is imported from a SIM, the imported number replaces the spaces with zeros. Dashes and parentheses are also replaced with zeros.

Repro Steps:
1) Updated Buri to BuildID: 20140117004005
2) Import a contact from an email account whose phone number has spaces within it
3) Open 'Contacts' and tap on settings icon in top right corner
4) Select 'Export' > SIM card and select the contact from Step 2
5) After exporting the contact to the SIM card, select 'Import Contacts'
6) After importing the contact, select it from the contact list and observe the phone number

Actual:
There are zeros in place of spaces in the contact's phone number.

Expected:
The imported number should be the same as when it was exported.

Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20140117004005
Gaia: a81ccdc53e45a6adeaae423e104e91bcc1e12b0e
Gecko: 2c033140eff4
Version: 28.0a2
Firmware Version: V1.2-device.cfg

Repro frequency: 100%
See attached: screenshots, logcat
Attached image Screenshot
Does this reproduce on 1.1?
Keywords: qawanted
Issue does not occur in 1.1, there is no option to export contacts in 1.1

Environmental Variables:
Device: Buri 1.1 COM
BuildID: 20140116041745
Gaia: 6ff3a607f873320d00cb036fa76117f6fadd010f
Gecko: 2075e0864e98
Version: 18.0
RIL Version: 01.01.00.019.281
Firmware Version: V1.2-device.cfg
QA Contact: jschmitt
Does this reproduce on 1.2?
Unable to confirm this bug as I cannot get passed step 4 on the latest 1.2, the device is currently getting stuck on 'exporting to sim' indefinitely while giving no indication as to why.

Environmental Variables:
Device: Buri 1.2 COM
BuildID: 20140116004003
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: 27e469d3cab0
Version: 26.0
RIL Version: 01.02.00.019.102
Firmware Version: V1.2-device.cfg
The whitespace use case doesn't seem realistic to occur, but the dashes & parentheses use cases definitely are. Dashes are common to use in a phone number to separate major parts of a phone number. Parentheses are common for a representation of an area code. The fact that we're displaying these numbers incorrectly will confuse a user into what number the contact actually has.
blocking-b2g: --- → 1.3?
Keywords: qawanted
Summary: [B2G][Contacts]Importing a contact whose phone number has spaces from a SIM replaces the spaces with zeros. → [B2G][Contacts]Importing a contact whose phone number has special characters from a SIM replaces the spaces with zeros.
David,

Please review from contacts view.
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(dscravaglieri)
Flags: needinfo?(noef)
Assignee: nobody → francisco.jordano
Flags: needinfo?(noef)
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Hi,

I've been taking a look, with a contact (Lola) with phone number (1)-2222222

Here are the RIL logs when I export to the SIM:

I/Gecko   (  878): -*- RILContentHelper: Received message 'RIL:UpdateIccContact': {"clientId":0,"data":{"requestId":"id{b015294e-1561-462c-a091-5a58045a727e}","contactType":"adn","contact":{"alphaId":"Lola","number":"(1)-2222222","contactId":"89340711070024031852","pbrIndex":0,"recordId":2},"pin2":null,"rilMessageToken":13,"rilMessageType":"updateICCContact"}}
I/Gecko   (  878): -*- RILContentHelper: fire request success, id: id{b015294e-1561-462c-a091-5a58045a727e}, result: {"id":"89340711070024031852","published":null,"updated":null,"bday":null,"anniversary":null,"sex":null,"genderIdentity":null,"adr":null,"email":null,"url":null,"impp":null,"tel":[{"value":"(1)-2222222"}],"name":["Lola"],"honorificPrefix":null,"givenName":null,"additionalName":null,"familyName":null,"honorificSuffix":null,"nickname":null,"category":null,"org":null,"jobTitle":null,"note":null,"key":null}

We send the phone information correctly: "tel":[{"value":"(1)-2222222"}]

Then I remove the contact (to avoid going through the duplicates and so on), and import from the sim, here is the RIL log:
I/Gecko   (  878): -*- RILContentHelper: fire request success, id: id{526eba17-1c6c-4da0-9d41-c9093e288822}, result: [{"id":"89340711070024031851","published":null,"updated":null,"bday":null,"anniversary":null,"sex":null,"genderIdentity":null,"adr":null,"email":null,"url":null,"impp":null,"tel":[{"value":"010011111"}],"name":["Pepe"],"honorificPrefix":null,"givenName":null,"additionalName":null,"familyName":null,"honorificSuffix":null,"nickname":null,"category":null,"org":null,"jobTitle":null,"note":null,"key":null},{"id":"89340711070024031852","published":null,"updated":null,"bday":null,"anniversary":null,"sex":null,"genderIdentity":null,"adr":null,"email":null,"url":null,"impp":null,"tel":[{"value":"01002222222"}],"name":["Lola"],"honorificPrefix":null,"givenName":null,"additionalName":null,"familyName":null,"honorificSuffix":null,"nickname":null,"category":null,"org":null,"jobTitle":null,"note":null,"key":null}]

Two contacts, for our test 'Lola', look at the tel value: "tel":[{"value":"01002222222"}]

Pinging our fellow RIL heroes for more information about this.

Thanks folks!
Assignee: francisco.jordano → nobody
ni to Yoshi per comment 8. Thanks!
Flags: needinfo?(allstars.chh)
Target Milestone: 1.3 C3/1.4 S3(31jan) → ---
SIM can only store digits, it cannot store '(', ')', nor '-'.

Gaia needs to tell user those non-digits information will be lost when exporting to SIM.

If you think we still need to export this number to SIM, I'll fix the bug about '(', ')', '-' characters become 0.

Otherwise I don't think this is a Gecko bug.
Flags: needinfo?(allstars.chh)
What's about exporting the number to SIM but ignoring '(',')','-' (non-supported) characters?. It will allow us to maintain a usable number within user's SIM card. wdyt?
Flags: needinfo?(allstars.chh)
(In reply to Noemí Freire (:noemi) from comment #11)
> What's about exporting the number to SIM but ignoring '(',')','-'
> (non-supported) characters?.

Sorry I don't quite sure what do you mean by 'what's about' ?

Are you talking we should exporting the number but ignoring '(',')','-'?
-> Yeah, if UX agrees I can fix it. I think Android AOSP will throw Runtime Exception, but some Android phones just simply ignore non-supported characters.

Or do you mean why?
-> It's defined by 3GPP spec, and the number field should be of BCD format.
Flags: needinfo?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #12)
> (In reply to Noemí Freire (:noemi) from comment #11)
> > What's about exporting the number to SIM but ignoring '(',')','-'
> > (non-supported) characters?.
> 
> Sorry I don't quite sure what do you mean by 'what's about' ?
> 
> Are you talking we should exporting the number but ignoring '(',')','-'?
> -> Yeah, if UX agrees I can fix it. I think Android AOSP will throw Runtime
> Exception, but some Android phones just simply ignore non-supported
> characters.

I'm referring to export the number but ignoring those non-supported characters '(',')','-', so, in this case, just the number itself would be exported to the SIM Card (non-supported characters are not exported). The idea would be to guarantee a valid number exported to user's SIM Card that can be used. i.e, "(1)-2222222" number would be exported as "12222222"
Component: Gaia::Contacts → RIL
Assignee: nobody → allstars.chh
Attached patch Part 1: write BCD chars. (obsolete) — Splinter Review
Attachment #8364997 - Attachment is obsolete: true
Comment on attachment 8364999 [details] [diff] [review]
Part 1: write BCD chars.

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

::: dom/system/gonk/ril_worker.js
@@ +908,5 @@
>      }
>  
>      let contact = options.contact;
> +    if (typeof contact.number === "string") {
> +      contact.number = contact.number.replace(/[^0-9*#,]/g, "")

Here we change the value of |contact.number|, and this contact object will be returned again as |DOMRequest.result|.  The string you'll have in content pages becomes "[0-9abc]*", but the one read from |readICCContact()| is "[0-9*#,]*" and causes some mismatch in comparison.  Please keep it "[0-9*#,]*" here.

@@ +911,5 @@
> +    if (typeof contact.number === "string") {
> +      contact.number = contact.number.replace(/[^0-9*#,]/g, "")
> +                                     .replace(/[*]/g, "a")
> +                                     .replace(/[#]/g, "b")
> +                                     .replace(/[,]/g, "c");

Have to translate ANR as well.
Attachment #8364999 - Flags: review?(vyang)
Comment on attachment 8364998 [details] [diff] [review]
Part 2: xpcshell tests.

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1859,5 @@
> +  let contactHelper = worker.ICCContactHelper;
> +  ril.iccInfo.iccId = "123456789";
> +
> +  contactHelper.updateICCContact = function(appType, contactType, contact, pin2, onsuccess, onerror) {
> +    do_check_eq(contact.number, "123g4b567c8");

It's "abc" or "gbc" after all?  This mismatches that regexp in part 1.
Attachment #8364998 - Flags: review?(vyang)
Attached patch Part 1: write BCD chars. v2 (obsolete) — Splinter Review
Attachment #8364999 - Attachment is obsolete: true
Comment on attachment 8365478 [details] [diff] [review]
Part 1: write BCD chars. v2

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

I'll update test case as well later, having a plane to catch today.
Attachment #8365478 - Flags: review?(vyang)
Comment on attachment 8365478 [details] [diff] [review]
Part 1: write BCD chars. v2

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

By this patch, we keep the 'contact.number' as it is.

::: dom/system/gonk/ril_worker.js
@@ +9670,5 @@
> +               number.substring(numStart)
> +                     .replace(/[^0-9*#,]/g, "")
> +                     .replace(/[*]/g, "a")
> +                     .replace(/[#]/g, "b")
> +                     .replace(/[,]/g, "c");

nit: .replace(/\*/g, "a").replace(/\#/g, "b").replace(/\,/g, "c")
Attachment #8365478 - Flags: review?(vyang) → review+
Addressed Vicamo's comments.
Attachment #8365478 - Attachment is obsolete: true
Attachment #8365841 - Flags: review+
Attached patch Part 2: xpcshell tests. v2. (obsolete) — Splinter Review
Attachment #8364998 - Attachment is obsolete: true
Attached patch Part 2: xpcshell tests. v3 (obsolete) — Splinter Review
Attachment #8365842 - Attachment is obsolete: true
Attachment #8365842 - Flags: review?(vyang)
upload wrong file before.
Attachment #8365857 - Attachment is obsolete: true
Comment on attachment 8365861 [details] [diff] [review]
Part 2: xpcshell tests. v3

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

Thank you!
Attachment #8365861 - Flags: review?(vyang) → review+
https://hg.mozilla.org/mozilla-central/rev/2c7336a890b3
https://hg.mozilla.org/mozilla-central/rev/d024f4e71751
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
clear n?
Flags: needinfo?(dscravaglieri)
Blocks: 1157082
You need to log in before you can comment on or make changes to this bug.