925566 - A preloaded contact without a type specified for a phone number defaults to mobile, even though mobile isn't the defined type

Status: VERIFIED FIXED

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

Description

[Jason Smith [:jsmith]]

Build: 10/10/2013 Hamachi Master

STR

1. Follow https://wiki.mozilla.org/B2G/QA/Customizations#Build_Setup to setup your build with no SIM included
2. Launch contacts and check the contacts that are preloaded

Expected

The type field should be empty

Actual

The type field is mobile.

Comment 1

[Albert [:albert]]

Gecko returns the type field empty:

I/Gecko   ( 1293): -*- ContactManager: manager result: [{"_name":[],"_honorificPrefix":[],"_givenName":["test 1"],"_additionalName":[],"_familyName":[],"_honorificSuffix":[],"_nickname":[],"_email":[],"_photo":[],"_url":[],"_category":[],"_adr":[],"_tel":[{"type":[],"value":"123456"}],"_org":[],"_jobTitle":[],"_note":[],"_impp":[],"_sex":null,"_genderIdentity":null,"_key":[],"id":"fff018f44e944276860eeb8b42f73f1c","_published":"2013-10-12T07:07:39.724Z","_updated":"2013-10-12T07:07:39.724Z"}]

Comment 2

[Albert [:albert]]

Reuben can you have a look to your code from bug 891723?

Comment 3

[Reuben Morais [:reuben]]

(In reply to Albert [:albert] from comment #2)
> Reuben can you have a look to your code from bug 891723?

My code simply creates a mozContact with the data from the file. And according to the log in comment 1, the type is empty for the API, so this can only be a problem in the Contacts app.

Comment 4

[Jose Manuel Cantera]

good point. I need to cross-check with Ayman if at the UI level we allow empty types for phone numbers. Currently it seems the Contacts App defaults to mobile

Comment 5

[Jason Smith [:jsmith]]

(In reply to Reuben Morais [:reuben] from comment #3)
> (In reply to Albert [:albert] from comment #2)
> > Reuben can you have a look to your code from bug 891723?
> 
> My code simply creates a mozContact with the data from the file. And
> according to the log in comment 1, the type is empty for the API, so this
> can only be a problem in the Contacts app.

It's a contacts app bug - it's assuming a type not defined means mobile. Which likely won't be compatible with third party privileged apps integrating with the contacts API.

(In reply to Jose M. Cantera from comment #4)
> good point. I need to cross-check with Ayman if at the UI level we allow
> empty types for phone numbers. Currently it seems the Contacts App defaults
> to mobile

Note - whatever we decide here has to follow the underlying API spec. Otherwise, we're going to run into compatibility problems if privileged apps define an empty type differently than Gaia's Contacts app.

Comment 6

[ayman maat :maat]

(In reply to Jose M. Cantera from comment #4)
> good point. I need to cross-check with Ayman if at the UI level we allow
> empty types for phone numbers. Currently it seems the Contacts App defaults
> to mobile

We designed the form of the Contact input so that is was never possible for a contact to have a 'phone number' without a 'type' in order to maintain data cleanliness. Of course as we can see this can be breached at the point of importing from a source where a phone number has no type associated to it. 

So in the case where a contact that has been imported has a phone number that has no type associated to it we should associate it to the (miscellaneous) type tag of 'other'. This aligns with the behaviour of our benchmarks and also helps to ensure that we maintain a certain standard of data cleanliness. Defaulting associating to the tag of 'Mobile' could lead to erroneous data in the case that the phone number is not a mobile number and allowing a phone number to enter into the Db untagged constitutes incomplete data, both instances of which degrade the cleanliness of the data. 

Therefore we should assign the type tag of 'other'.

Comment 7

[Jose Manuel Cantera]

We will have to guarentee that all importers in Gaia put the default type as 'other' when there is no a direct mapping between the sources and the types recognized in Gaia.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Jose M. Cantera from comment #7)
> We will have to guarentee that all importers in Gaia put the default type as
> 'other' when there is no a direct mapping between the sources and the types
> recognized in Gaia.

I don't think we can guarantee that, so long as the Contacts API spec allows for the type to not be specified. If we need to require this, then we should discuss this on the relevant W3C discussion forum to get this added to the spec.

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

triage: should not block release. please ask for approval to land in v1.2. thanks

Comment 10

[Albert [:albert]]

Attachment: Set default mobile type in ContactManager

Comment 11

[Albert [:albert]]

Attachment: Set default mobile type in ContactManager

Changed default type 'Other' by 'other'

Comment 12

[Reuben Morais [:reuben]]

(In reply to Albert [:albert] from comment #11)
> Created attachment 823914 [details] [diff] [review]
> Set default mobile type in ContactManager
> 
> Changed default type 'Other' by 'other'

Albert, why is this necessary? It is perfectly valid to have type set to [""] or null, for that matter. We want to fix this in the Contacts app. If the type is empty, show "other" or, don't show the type at all.

Comment 13

[Marina Rodríguez [:mai]]

Attachment: patch v1.0

Please, could you review my code?

Comment 14

[Albert [:albert]]

(In reply to Reuben Morais [:reuben] from comment #12)
> (In reply to Albert [:albert] from comment #11)
> > Created attachment 823914 [details] [diff] [review]
> > Set default mobile type in ContactManager
> > 
> > Changed default type 'Other' by 'other'
> 
> Albert, why is this necessary? It is perfectly valid to have type set to
> [""] or null, for that matter. We want to fix this in the Contacts app. If
> the type is empty, show "other" or, don't show the type at all.

Talking with Jose M. we thought it can avoid to have different apps with different default values, if gecko sets a default type field all apps will have the same one. And also, the fact of setting the default value in the manager instead of saving it in the database, will make easy to change it to another value in the future.

Comment 15

[Reuben Morais [:reuben]]

(In reply to Albert [:albert] from comment #14)
> (In reply to Reuben Morais [:reuben] from comment #12)
> > (In reply to Albert [:albert] from comment #11)
> > > Created attachment 823914 [details] [diff] [review]
> > > Set default mobile type in ContactManager
> > > 
> > > Changed default type 'Other' by 'other'
> > 
> > Albert, why is this necessary? It is perfectly valid to have type set to
> > [""] or null, for that matter. We want to fix this in the Contacts app. If
> > the type is empty, show "other" or, don't show the type at all.
> 
> Talking with Jose M. we thought it can avoid to have different apps with
> different default values, if gecko sets a default type field all apps will
> have the same one. And also, the fact of setting the default value in the
> manager instead of saving it in the database, will make easy to change it to
> another value in the future.

|type| is not an enum, any string value is valid there, and whatever value is used should be shown to the user (the Contacts app even lets you specify your own type). It really doesn't make sense to enforce a non empty default there.

Comment 16

[Jose Manuel Cantera]

Marina, 

Please rebase the PR as we will try to land this for v1.4

thanks!

Comment 17

[Marina Rodríguez [:mai]]

PR updated. ;)

Regards

Comment 18

[Jose Manuel Cantera]

Comment on attachment 824505 [details]
patch v1.0

Sergi, 

Please could you review thr vCard part of this patch. It introduces some changes that might be worth reviewing by you. 

thanks

Comment 19

[Jose Manuel Cantera]

Marina,

I left some comments on GH and there is, in theory, also a marionnette js broken test with this patch. please check

thanks

Comment 20

[Jose Manuel Cantera]

Sergi,

Please react on the requested review

thanks!

Comment 21

[Marina Rodríguez [:mai]]

Updated the PR, please, could you review the code?
Regards

Comment 22

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi, reviewing the patch right now.

Comment 23

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Comment on attachment 824505 [details]
patch v1.0

The vcard part looks good to me. r+

Comment 25

[Jose Manuel Cantera]

Comment on attachment 824505 [details]
patch v1.0

thanks for the work

Comment 26

[Jose Manuel Cantera]

https://github.com/mozilla-b2g/gaia/commit/25f535053a0fdad6e9545bb2bae0b58ae6739bd9

Comment 27

[isabelrios]

Check on master with today's build (01/29):
Gecko-fc156ab
Gaia-072cb1a

Now it appears 'OTHER' as the mobile type for pre-loaded contacts. See screenshot attached.

Comment 28

[isabelrios]

Attachment: Customer care pre loaded contact