1061411 - [Contacts] ICE 1 & ICE 2 can be set to the same contact

Status: VERIFIED FIXED

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

Description

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

Attachment: ICE.png

### STR
1. Have a contact in Contacts
2. Set ICE1 to a contact
3. Set ICE2 to the same contact

### Actual
ICE1 & ICE2 can be set to the same contact

### Expected
Not found any info in spec
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]]

Hi Carrie, are we okay with setting 2 ICEs to the same contact?

Comment 2

[Carrie Wang [New account: [e-mail]]]

I'd suggest not displaying the selected contact when user is choosing for the other ICE contact, but I don't know if it's feasible or not. So ni? Francisco for help! \o/
Thanks!

Comment 3

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

Hi,

It will create us a lot of trouble, since implies hidding elements from the list (we show the  normal list). What it's pretty simple to control is to check if it's a ICE contact and display an alert to notify that one has been already selected.

Will that work for you Carrie?

Comment 4

[Carrie Wang [New account: [e-mail]]]

Yes, this is my plan B as well.

Let's show something like this: 

___________________________

This contact is already an 
existing ICE contact




Thanks! (<- this is not the string ;))

Comment 5

[Jorge Prudencio [:jorgep]]

Attachment: 23898.html

Comment 6

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

Comment on attachment 8487038 [details]
23898.html

Jorge, incredible work here.

I'm not setting the r+ cause I have a couple of suggestions in GH.

Mostly nits, also about the tests, since would like to avoid to expose a public method.

Tell me what you think about the suggestions, but it's pretty much done.

Comment 7

[Jorge Prudencio [:jorgep]]

Comment on attachment 8487038 [details]
23898.html

Hello Francisco,

I have included an integration test and I have modified the code. Can you review it?

Thanks!

Comment 8

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

Comment on attachment 8487038 [details]
23898.html

We are pretty close!

I just left a comment, also could you rebase your patch to be able to merge to master?

Please flag me again for final review, but this is looking pretty nice.

Great job Jorge, really good contribution!

Comment 9

[Jorge Prudencio [:jorgep]]

Comment on attachment 8487038 [details]
23898.html

Updated!

Comment 10

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

Comment on attachment 8487038 [details]
23898.html

Thanks for adding the suggestions.

Now we wait for 'try' to be green in order to merge \o/

Comment 11

[Jorge Prudencio [:jorgep]]

'Try' is green :-)

Comment 12

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

Jorge, sorry, unfortunately we need to rebase again to merge with master.

Comment 13

[Jorge Prudencio [:jorgep]]

Rebased!

Comment 14

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

Landed:

https://github.com/mozilla-b2g/gaia/commit/7f951d62784ea557147b1eef10d21df5bf82f4dd

Great job Jorge

Comment 15

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

Jorge, one las thing, could you attach a video to show how it works now?

Comment 16

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

Comment on attachment 8487038 [details]
23898.html

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Cause by new ICE contacts feature
[User impact] if declined:
Wrong information and bad user experience. Specially in a feature that is for users's safety
[Testing completed]:
Added more integration tests to check the good behavior.
[Risk to taking this patch] (and alternatives if risky):
Low risk, we are just applying a tiny filter before setting the value. This patch is 30% code 70% tests.
[String changes made]:
ICERepeatContact = This contact is already an existing ICE contact

Comment 17

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

Unfortunately I had to revert this patch since the integration test that we add is failing in try.

I've passed it locally, but we will need to investigate why is not working on try, meanwhile we cannot stop progress from other teams so, here is the backout commit:

https://github.com/mozilla-b2g/gaia/commit/97e0edce6aac3a5a9db220e4717e81056c97fe96

Comment 18

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

Comment on attachment 8487038 [details]
23898.html

Removing the request for approval until we have everything sorted.

Comment 19

[Germán Toro del Valle (:gtorodelvalle)]

Hi Francisco, I think it would be better if, for the time being at least, Jorge focuses on Dialer related apps, mainly to get a deeper knowledge on them. What do you think about assigning this bug to yourself to track the problems with the integration tests in try? It seems they do not only affect the newly created integration test :)

Comment 20

[Jorge Prudencio [:jorgep]]

Hello, I think that the problem is in:

https://github.com/jpruden92/gaia/blob/f1e3a9a8d1e4b6c5066542943e3c043deb0c0aa4/apps/communications/contacts/test/marionette/ice_test.js#L40

This line is used by other integration tests and it fails too. (Ex. form_test.js) The integration test show a timeout error.

Thanks!

Comment 21

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

(In reply to Germán Toro del Valle from comment #19)
> Hi Francisco, I think it would be better if, for the time being at least,
> Jorge focuses on Dialer related apps, mainly to get a deeper knowledge on
> them. What do you think about assigning this bug to yourself to track the
> problems with the integration tests in try? It seems they do not only affect
> the newly created integration test :)

Sure, no problem, will take this bug

Comment 22

[Germán Toro del Valle (:gtorodelvalle)]

Thanks! ;)

Comment 23

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

Attachment: Pointer to PR 24169

Comment 24

[Jose Manuel Cantera]

Comment on attachment 8491385 [details] [review]
Pointer to PR 24169

thanks Francisco. good work!

Comment 25

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

Landed:

https://github.com/mozilla-b2g/gaia/commit/2ea1c31f53468e3e34ea6191253065289b27a5b1

Comment 26

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

Comment on attachment 8491385 [details] [review]
Pointer to PR 24169

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
ICE contacts feature
[User impact] if declined:
Bad user experience, you can setup the same contact as ICE
[Testing completed]:
Smoke test passed, also added a new integration test to check this out
[Risk to taking this patch] (and alternatives if risky):
Low
[String changes made]:
ICERepeatedContact = This contact is already an existing ICE contact
ICEUnknownError = There was an error setting the contact as ICE

Comment 27

[[:fabrice] Fabrice Desré]

Comment on attachment 8491385 [details] [review]
Pointer to PR 24169

Too late for l10n changes. Try again if you manage to convince the l10n team!

Comment 28

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

Hi,

Pike, I've been talking with :flod in IRC and we still don't have clear if should be a release drivers decision or l10n-drivers.

Can we have your input here?

Thanks!

Comment 29

[Axel Hecht [:Pike]]

This is a release-driver decision. Which you basically got from Fabrice, which was a "no".

Comment 30

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

(In reply to Axel Hecht [:Pike] from comment #29)
> This is a release-driver decision. Which you basically got from Fabrice,
> which was a "no".

Thanks Alex for the clear comment.

Comment 31

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

Hi, I've been talking with several people trying to solve this.

The l10n team won't get more strings, so there is no way to solve this via new strings. We tried to reuse new strings but we don't have some of them (potentially to display the error) in 2.1

As having an alternative string is the only possible solution, so far, I've been talking with product and they agree in showing a message even if it's not completely translated (not the best solution, we all know, but the only way to unblock this situation and not have this error in 2.1).

In this case we could reuse a facebook string: 'error_1':
error1          = Error {{from}}

where we can pass a parameter, I've checked with Wilfred and he is ok displaying this, so the message ({{from}}) can be the error kind. That message will appear just in english, but the 'Error' string will be transalated.

Again, not the perfect solution but bettern than nothing, product is happy to show that there is an error, but the message wasn't transalted.

Comment 32

[Jose Manuel Cantera]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #31)
> Hi, I've been talking with several people trying to solve this.
> 
> The l10n team won't get more strings, so there is no way to solve this via
> new strings. We tried to reuse new strings but we don't have some of them
> (potentially to display the error) in 2.1
> 
> As having an alternative string is the only possible solution, so far, I've
> been talking with product and they agree in showing a message even if it's
> not completely translated (not the best solution, we all know, but the only
> way to unblock this situation and not have this error in 2.1).
> 
> In this case we could reuse a facebook string: 'error_1':
> error1          = Error {{from}}
> 
> where we can pass a parameter, I've checked with Wilfred and he is ok
> displaying this, so the message ({{from}}) can be the error kind. That
> message will appear just in english, but the 'Error' string will be
> transalated.
> 
> Again, not the perfect solution but bettern than nothing, product is happy
> to show that there is an error, but the message wasn't transalted.

Francisco, 

in this bug approval for gaia v2.1 was rejected by Fabrice ... please let me know how to proceed

thanks

Comment 33

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

> Francisco, 
> 
> in this bug approval for gaia v2.1 was rejected by Fabrice ... please let me
> know how to proceed
> 
> thanks

As we have been talking this morning, let's create a specific patch for 2.1 with the string 'error1' from Facdebook, and ask for approval for that patch that doesnt introduce any new string.

Comment 34

[Jose Manuel Cantera]

Attachment: 24757.html

patch for v2.1, in accordance with the latest solution proposed by product

Comment 35

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

Comment on attachment 8499612 [details]
24757.html

Looking good to me!

Thanks for the job Jose!

Comment 36

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

Comment on attachment 8499612 [details]
24757.html

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Feature ICE contacts
[User impact] if declined:
User can create a weird application state in a feature conceived for user's security.
[Testing completed]:
Smoke test passed in 2.1 branch, original patch in 2.2 working for a while.
[Risk to taking this patch] (and alternatives if risky):
Pretty low, this bug wasn't accepted for 2.1 cause a new string, but with this specific approach we are reusing a string that we already have translated.
[String changes made]:
None :)

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Comment 38

[Duane Dixon [:ddixon]]

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

Actual Results:  ICE Contacts 1 and 2 cannot be assigned to the same contact. 

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