1061422 - [Contacts] ICE can be set to a contact without number

Status: VERIFIED FIXED

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

Description

[Eric Chang [:ericcc] [:echang]]

### STR
1. Set ICE to some contact A(without number), make sure it appears on top of contact list.

### Actual 
ICE can be set 

### Expected
Warning should pop up, ICE should not be set
Graph 3, Page 16 of https://bugzilla.mozilla.org/attachment.cgi?id=8459474

### Version
Gaia      e7d31f0e9b6b19d9b484eeec8fb980718bc40d79
Gecko     https://hg.mozilla.org/mozilla-central/rev/532b5fb77ba1
BuildID   20140901160203
Version   34.0a1
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014
B1TC00011230

Comment 1

[Eric Chang [:ericcc] [:echang]]

[Blocking Requested - why for this release]: Spec

Comment 2

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: major issue on new feature.

Comment 3

[Dietrich Ayala (:dietrich)]

Triage group reviewed, this doesn't meet blocker criteria, defined at https://wiki.mozilla.org/B2G/Triage#Blocker_Triage_Guidelines. There's no crash, dataloss or feature breakage.

This issue is slightly odd, but doesn't block the usage of ICE features at all.

2.1 is open for approved landings, so request approval from Fabrice or Bhavana in order to land this if it's fixed in time!

Comment 4

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: we need this so please ask approval for uplift

Comment 5

[Francisco Jordano [:arcturus] [:francisco]]

Let's fix this and ask for approval.

The fix should be pretty simple and it will enhance the user experience a lot.

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Pointer to PR 14186

Comment 8

[Jose Manuel Cantera]

Comment on attachment 8491573 [details] [review]
Pointer to PR 14186

I believe a new round is needed

thanks Francisco

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8491573 [details] [review]
Pointer to PR 14186

PR updated, thanks!

Comment 10

[Jose Manuel Cantera]

Comment on attachment 8491573 [details] [review]
Pointer to PR 14186

working perfectly

thanks Francisco

Comment 11

[Francisco Jordano [:arcturus] [:francisco]]

Landed:

https://github.com/mozilla-b2g/gaia/commit/12d634c63ec8a41553a9f150c38260c992dc0020

Comment 12

[Jose Manuel Cantera]

We need to uplift this bug to v2.1. It is not acceptable to allow the user to make the mistake of choosing as ICE Contact a Contact which does not have a tel number. Fortunately, thanks to Cristian We have found we can reuse an existing L10N string

https://github.com/mozilla-b2g/gaia/blob/v2.1/apps/communications/contacts/locales/contacts.en-US.properties#L21

Comment 13

[Jose Manuel Cantera]

[Blocking Requested - why for this release]:

see comment #12

Comment 14

[Dietrich Ayala (:dietrich)]

[Blocking Requested - why for this release]:(In reply to Jose Manuel Cantera from comment #12)
> We need to uplift this bug to v2.1. It is not acceptable to allow the user
> to make the mistake of choosing as ICE Contact a Contact which does not have
> a tel number. Fortunately, thanks to Cristian We have found we can reuse an
> existing L10N string
> 
> https://github.com/mozilla-b2g/gaia/blob/v2.1/apps/communications/contacts/
> locales/contacts.en-US.properties#L21

Jose, can you describe the level of risk in this patch?

Comment 15

[Jose Manuel Cantera]

(In reply to Dietrich Ayala (:dietrich) from comment #14)
> [Blocking Requested - why for this release]:(In reply to Jose Manuel Cantera
> from comment #12)
> > We need to uplift this bug to v2.1. It is not acceptable to allow the user
> > to make the mistake of choosing as ICE Contact a Contact which does not have
> > a tel number. Fortunately, thanks to Cristian We have found we can reuse an
> > existing L10N string
> > 
> > https://github.com/mozilla-b2g/gaia/blob/v2.1/apps/communications/contacts/
> > locales/contacts.en-US.properties#L21
> 
> Jose, can you describe the level of risk in this patch?

the patch will be the same as the one for 2.2 

https://github.com/mozilla-b2g/gaia/pull/24186/files 

excepting the L10N stuff that will not be added. The patch contains an integration test as well. for those reasons, I would say the risk is low

thanks

Comment 16

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Not blocking according to comment 3. (Though, this actually creates significant user impact)
We should still get the patch uplifted to 2.1.

Comment 17

[Cristian Rodriguez (:crdlc)]

Attachment: Github pull request for 2.1

Basically the same code but reusing a string already existing in 2.1

Comment 18

[Jose Manuel Cantera]

Comment on attachment 8498786 [details]
Github pull request for 2.1

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): ICE Contacts new feature
[User impact] if declined: High. A user can choose as ICE Contact a Contact which actually does not have a phone number. As a result in an emergency situation there will be nobody to phone. 
[Testing completed]: Yes, integration test included and manual testing done by the reviewer. 
[Risk to taking this patch] (and alternatives if risky): Low risk patch. Alternative is a useless app. 
[String changes made]:

Comment 19

[Ryan VanderMeulen [:RyanVM]]

The Gaia-Try for the v2.1 PR appears to have real-looking test failures. Please take a look.

Comment 20

[Francisco Jordano [:arcturus] [:francisco]]

Redirecting the needinfo to Cristian.

Cristian can you check the errors that Ryan is commenting in 2.1 branch?

Thx!

Comment 21

[Cristian Rodriguez (:crdlc)]

Yes, I fixed the problems at least related to this patch because there are more fails regarding to system, sms, etc...

https://tbpl.mozilla.org/?rev=18159d9e9ddda1f835f13225d8c66b231a7e6082&tree=Gaia-Try

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #20)
> Redirecting the needinfo to Cristian.
> 
> Cristian can you check the errors that Ryan is commenting in 2.1 branch?
> 
> Thx!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/eaf3b6b96d497698970fd8e3699a2b1218ae02d2

Comment 23

[Duane Dixon [:ddixon]]

Issue is verified fixed in Flame 2.2, 2.1 (Full Flash, nightly) 

Actual Results: Contacts without a phone number cannot be set as ICE Contacts. 

Device: Flame Master
Build ID: 20141015040201
Gaia: 5f1f0960ae9d22acf2a324ad37a48174d6df87f6
Gecko: 62f0b771583c
Version: 36.0a1 (Master)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.1
Build ID: 20141015001201
Gaia: 379ea4c9dd6d3f8ca2f79ce59c15f6afe6e557c3
Gecko: 4853208cb48a
Version: 34.0 (2.1)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0