Closed Bug 951177 Opened 11 years ago Closed 10 years ago

[Contacts] Exporting a contact to SIM card is not working fine under some circumstances

Categories

(Firefox OS Graveyard :: RIL, defect)

Product:
Firefox OS Graveyard
Component:
RIL
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
VERIFIED FIXED
Milestone:
1.3 C3/1.4 S3(31jan)
Project Flags:
blocking-b2g 1.3+
Tracking Flags:
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.2 --- affected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: isabelrios, Assigned: allstars.chh)

Details

(Whiteboard: [FT:RIL])

Attachments

(2 files)

Part 1: remove anr/email.
4.46 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
Part 2: test case.
3.18 KB, patch
vicamo
: review+
Details | Diff | Splinter Review 
Seen with v1.2:
Gecko-f6f325b
Gaia-4f53ba8
also with v1.3 and master

STR
1-There is a contact with Name and Phone number stored in the SIM card
2-Import this contact to DuT
3-Edit this contact removing the phone number
4-Export this contact to the SIM Card
Check how this contact is after that, for example:
5-Remove the contact from the list
6-Import it again from the SIM
Or, insert the sim in another device and check the contact

ACTUAL
The contact is updated losing the info that the exported contact does not have, i.e, phone number. 

EXPECTED
When a contact is exported to the SIM card, contact in the SIM will be updated but never SIM info will be lost.
blocking-b2g: --- → koi?
I'm unsure if this is a bug. Technically the contact that was exported vs. the contact was imported are different contacts - one has a phone number, the other doesn't. Are we sure that these contacts are one of the same or are they in fact different?
(In reply to Jason Smith [:jsmith] from comment #1)
> I'm unsure if this is a bug. Technically the contact that was exported vs.
> the contact was imported are different contacts - one has a phone number,
> the other doesn't. Are we sure that these contacts are one of the same or
> are they in fact different?

Hi,

They're the same contact since the iccContactId is the same. The idea is to avoid duplicate contacts while importing/exporting from/to SIM card. In case of export and a local contact with an Id associated, the corresponding contact within the SIM card will be updated but with loosing information present in the SIM card. ni? to UX to confirm the expected behaviour.
Flags: needinfo?(aymanmaat)
ok this is a bug. At the moment we are not in a position to edit the contacts on the SIM and this is an indirect form of editing. When we re-export a contact to the SIM card, at the moment, we should be adding information new and not removing and existing information. the only information on the SIM that should be directly updated is the contacts name (for reasons of marriage).

Therefore until we can directly edit the sim the flow should be:

1) SIM contact: name, phone number
2) import contact in step 1
3) remove phone number associated to the locally stored contact that was imported in step 2
4) export the contact that was edited in step 3 back to the SIM card

**EXPECTED**
There is no loss of phone number on the SIM card
Flags: needinfo?(aymanmaat)
As additional info (to solve the bug) and since it is related to this one, we just observed that:

STR 2
1-There is a contact with name and 2 phone numbers stored in the SIM card.
2-Import this contact to DuT.
3-Edit this contact removing both phone numbers
4-Export this contact to the SIM Card.
Check how this contact is after that, for example:
5-Remove the contact from the list.
6-Import it again from the SIM.
Or, insert the sim in another device and check the contact.

ACTUAL
The contact is updated losing the first one of the phone numbers but not the second which is kept in the SIM card contact. 

EXPECTED
When the contact is exported to the SIM card, the contact in the SIM will be updated but no SIM info will be lost. This is, the contact in the SIM card maintains the 2 phone numbers and they are included in the initial contact.

In fact, there are variations of the previous STR which suggest that the second phone number is treated differently than the first one. For example:

STR 3
1-There is a contact with name and 2 phone numbers stored in the SIM card.
2-Import this contact to DuT.
3-Edit this contact removing the second phone number
4-Export this contact to the SIM Card.
Check how this contact is after that, for example:
5-Remove the contact from the list.
6-Import it again from the SIM.
Or, insert the sim in another device and check the contact.

ACTUAL = EXPECTED
When the contact is exported to the SIM card, the contact in the SIM will be updated but no SIM info will be lost. This is, the contact in the SIM card maintains the 2 phone numbers and they are included in the initial contact.

STR 4
1-There is a contact with name and 2 phone numbers stored in the SIM card.
2-Import this contact to DuT.
3-Edit this contact removing the first phone number
4-Export this contact to the SIM Card.
Check how this contact is after that, for example:
5-Remove the contact from the list.
6-Import it again from the SIM.
Or, insert the sim in another device and check the contact.

ACTUAL
The contact is updated including the "second" phone number 2 times (as 2 distinct phone numbers).

EXPECTED
When the contact is exported to the SIM card, the contact in the SIM will be updated but no SIM info will be lost. This is, the contact in the SIM card maintains the 2 phone numbers and they are included in the initial contact.
Apart from this, I was just wondering the reason why we want the info from the SIM card to prevail over the possible modifications done locally in the device. I mean, if I import a contact, modify it and export it back to the SIM card, wouldn't I want the SIM card to include the "latest version" of the contact just as I currently have it in my device?

As weirder cases we may find the case where the user imports a contact from the SIM, instead of deleting it he decides to reuse it changing all its information (given name, last name, phone numbers, etc.). According to Ayman's guidelines, if the user export the contact back to the SIM card it will be a mix of the 2 contacts maintaining the given name of the contact as it is in the SIM card but the last name of the new contact as it is in the device and the sum of all the info (phone numbers, etc.) from both contacts :S
Flags: needinfo?(aymanmaat)
Let's decide what UX flow actually is.  So, for example, what does android and other OSes do?   Not nomming, as this feels otherwise like an edge case.
Punting to 1.3.
blocking-b2g: koi? → 1.3?
(In reply to John Hammink from comment #6)
> Let's decide what UX flow actually is.  So, for example, what does android
> and other OSes do?   Not nomming, as this feels otherwise like an edge case.

Well apart from the fact that we are not designing iphone or andriod, neither of them offer a native facility to export to SIM.. so there is no comparison/benchmark with these systems to be made.

i dont think the current situation of updating the SIM contacts upon export is a killer one. i just currently have reservations about removing information on the SIM at the point of export because we do not have a facility directly edit information on the SIM just yet... therefore error recovery for the user is converted. Once we have direct editing in place there is no problem. This is why i did not specify updating the SIM

But like i say if we update the SIM its not the end of the world
Flags: needinfo?(aymanmaat)
converted = convoluted 
(damn auto correct)
(In reply to gtorodelvalle from comment #5)

> As weirder cases we may find the case where the user imports a contact from
> the SIM, instead of deleting it he decides to reuse it changing all its
> information (given name, last name, phone numbers, etc.). 

Thats a massive edge case. and really if *all* the information for a given contact is altered its probably a whole new contact.. no? ….and maybe we should handle it as so.

> According to
> Ayman's guidelines, if the user export the contact back to the SIM card it
> will be a mix of the 2 contacts maintaining the given name of the contact as
> it is in the SIM card but the last name of the new contact as it is in the
> device and the sum of all the info (phone numbers, etc.) from both contacts
> :S

true. but like i say in this edge case when *all* the information for a given contact is changed maybe we should be treating it as a completely new contact

The worry i have at the moment (and the reason why i am being conservative in my specification) is that the user cannot see or directly edit contact content on the SIM card. Therefore alterations happen *blindly*. When the user can see and directly edit SIM contact then there is no problem.

But like i say i don’t think updating the SIM at this point is the end of the world (I am juts being cautious) so if there is strong desire plus a good reasoned argument to do it I would not stand in the way.
QA Contact: isabelrios
We should also consider sex change cases which are not that edge these days... :p Kidding!
I think thats a fair point German.

Ok so after internal discussion we have decided to not be so conservative and therefore upon export we will align contact information on the SIM to that stored locally. Therefore:

**PATH**
1) import a contact from the SIM
2) alter (update, delete) any of the local contact information that has been imported in step 1
3) export contact altered in step 2 back to the SIM card

**EXPECTED**
information associated to contact on SIM card after export will reflect information associated to local contact on SIM that is being exported.

I am not going to mark this bug as invalid as German raises some deviant behaviour in comment 4 that still needs addressing.
Per comment 12 updating the subject accordingly. Thanks!
Summary: [Contacts] Exporting a contact to SIM card could cause the lost of SIM information → [Contacts] Exporting a contact to SIM card is not working fine under some circumstances
triage: 1.3+ for losing data
blocking-b2g: 1.3? → 1.3+
Assignee: nobody → gtorodelvalle
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
After some more tests, a few comments about the current behaviour:

There is a contact in SIM card with two phone numbers.

STR
1. Impor the contact to the device
2. Edit it
2a. Remove both phone numbers (ER2a)
2b. Remove the first phone number (ER2b)
2c. Remove the second phone number (ER2c)
3. Once edited, export it again to SIM card

ACTUAL 
After exporting the contact to SIM and checking it, in each case this is what is happening:
a. Second number is not removed
b. Second number appears twice
c. Second number is not removed

EXPECTED
a. Both numbers are removed
b. First phone number is removed and only the second one is available, but just once, not repeated
c. Second number is removed

This has been seen with both master and v1.3 buri 1/14 builds:
1.3:
Gecko-a037f1d
Gaia-96c05de

master:
Gecko-4825da9
Gaia-38c4019
Thank you very much, Isabel! ;-)

I looked deeper into it and it seems to be some issue on the backend side, more concretely in the MozIccManager.updateContact() implementation since I just confirmed that for the 2.a case Isabel mentions:
  1. When exporting, the contact passed to mozICCManager.udpateContact() does NOT include either of the phone numbers. Which is fine \o/
  2. When importing, the contact received from mozICCManager.readContacts() somehow does include the second phone number (the one previously in tel[1] but included now as tel[0]) (which was supposedly removed when the contact was exported to the SIM card after deleting the 2 phone numbers).

Consequently, it seems there is some issue in mozICCManager.udpateContact() when updating a contact. Yoshi, could you shed some light on this, please? :) Thanks!
Flags: needinfo?(allstars.chh)
(In reply to ayman maat :maat from comment #3)
> ok this is a bug. At the moment we are not in a position to edit the
> contacts on the SIM and this is an indirect form of editing. When we
> re-export a contact to the SIM card, at the moment, we should be adding
> information new and not removing and existing information. 

Are you sure about this?
Back to v1.2 in Oslo work week, Comms team asks for a feature to remove the FDN (Fixed Dialling Number) contact stored on the SIM, and they did a successful demo there.

Would you like to talk with UX in the Settings app ? (FDN is located in Settings)

As far as it goes, icc.updateContact is used to add/modify/delete SIM contact.
Flags: needinfo?(allstars.chh)
(In reply to Isabel Rios [:isabel_rios] from comment #15)
> After some more tests, a few comments about the current behaviour:
> 
> There is a contact in SIM card with two phone numbers.
> 
> STR
> 1. Impor the contact to the device
> 2. Edit it
> 2a. Remove both phone numbers (ER2a)
> 2b. Remove the first phone number (ER2b)
> 2c. Remove the second phone number (ER2c)
> 3. Once edited, export it again to SIM card
> 
> ACTUAL 
> After exporting the contact to SIM and checking it, in each case this is
> what is happening:
> a. Second number is not removed
> b. Second number appears twice
> c. Second number is not removed
> 

Yeah, it's a bug :P
JoseCartena has told me about this.
But at that time he also said UX doesn't want to do 'removal', which is ayman said before.

At time time we've discussed to split the 'updateContact' to 'saveContact' and 'removeContact', but since it would break compatibility that change should be for v1.4.
Hi guys, so which would be the strategy here? I mean, should we approach this issue in 2 phases?: 1) Fix the issue in 'updateContact' which causes the misbehaviour mentioned in this bug (and uplift it to 1.3) and 2) split 'updateContact' to 'saveContact' and 'removeContact' (or any other updates needed for the future)?
Flags: needinfo?(jmcf)
The bug is marked as 1.3+ and the desired behavior specified by UX is explained in comment #12. 

Yoshi, Do you think it can be feasible to implement this for v1.3?

thanks
Flags: needinfo?(jmcf) → needinfo?(allstars.chh)
Seems loop back to me now.

Please provide consistent UX here.

If a SIM contact cannot be removed, please have a talk with Settings UX, and this would also be a Gaia bug, if Gaia knows user has removed some contact information, it should return failure or should call icc.updateContact with complete information, otherwise the API cannot know you don't wanna delete it.

If a SIM contact could be removed, please update the bug title, or file another bug, as deleting is not exporting, I'll fix comment 15.
Flags: needinfo?(allstars.chh)
Hi Yoshi, maybe I am missing something here :) but what we need is that if the contact passed to ICCManager.updateContact() is already stored in the SIM card, it is completely overwritten (names, phones, emails, etc.). Right now, it seems that the implementation of this method is not doing this (right) according to my comment 16.
Flags: needinfo?(allstars.chh)
Assignee: gtorodelvalle → allstars.chh
Flags: needinfo?(allstars.chh)
Component: Gaia::Contacts → RIL
Whiteboard: [FT:RIL]
The PM team triaged this and considers this a blocker for 1.3
Hi Vicamo,
Since it's a 1.3+ bug, 
Do you have time to review this ?
Or if you're busy should I forward r? to Hsinyi?
Target Milestone: 1.3 C2/1.4 S2(17jan) → 1.3 C3/1.4 S3(31jan)
Adding need-info to vyang according to comment 26 :p
Flags: needinfo?(vyang)
Comment on attachment 8362955 [details] [diff] [review]
Part 1: remove anr/email.

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

::: dom/system/gonk/ril_worker.js
@@ +13732,5 @@
>        // Check if PBR has this field.
>        if (!pbr[field]) {
>          updateField();
>          return;
>        }

We still have a problem that we can't write new fields to SIM, but per offline discussion, this seems to be another known issue.

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1703,5 @@
>          do_check_eq(recordNumber, ANR0_RECORD_ID);
>        }
> +      if (Array.isArray(aContact.anr)) {
> +        do_check_eq(number, aContact.anr[0]);
> +      }

nit: not a big deal, but this should have been in the 2nd part.
Attachment #8362955 - Flags: review?(vyang) → review+
Comment on attachment 8362956 [details] [diff] [review]
Part 2: test case.

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

This file has now 3000 lines.  Maybe it's time to consider dividing it into several smaller parts.
Attachment #8362956 - Flags: review?(vyang) → review+
Flags: needinfo?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #28)
 > ::: dom/system/gonk/tests/test_ril_worker_icc.js
> @@ +1703,5 @@
> >          do_check_eq(recordNumber, ANR0_RECORD_ID);
> >        }
> > +      if (Array.isArray(aContact.anr)) {
> > +        do_check_eq(number, aContact.anr[0]);
> > +      }
> 
> nit: not a big deal, but this should have been in the 2nd part.

Hi, Vicamo
Thanks for the review.

I run try for multiple patches,
so Part 1 there's a try-push, and Part 2 for another try-push, in case Part 1 breaks something.

putting this in Part 1 because I found a TypeError when running tests if only Part 1 is applied.

If you think we still need to put this to Part 2 I'll do it right away.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #29)
> Comment on attachment 8362956 [details] [diff] [review]
> Part 2: test case.
> 
> Review of attachment 8362956 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This file has now 3000 lines.  Maybe it's time to consider dividing it into
> several smaller parts.

Bug 963535
The tree is closed now, add checkin-needed.
Keywords: checkin-needed
Keywords: checkin-needed
Target Milestone: 1.3 C3/1.4 S3(31jan) → 1.3 C2/1.4 S2(17jan)
Target Milestone: 1.3 C2/1.4 S2(17jan) → 1.3 C3/1.4 S3(31jan)
This bug isn't tested because depend of this  https://bugzilla.mozilla.org/show_bug.cgi?id=957051
Tested and working
1.3
Platform version: 28.0
Build ID: 20140319100231
Git commit: f2f2be55

1.4
Platform version: 30.0a2
Build ID: 20140319105930
Git commit: c036afe
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: