Closed Bug 1061411 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S5 (26sep)
Tracking Status
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: echang, Assigned: arcturus)

References

Details

(Keywords: late-l10n)

Attachments

(3 files, 1 obsolete file)

Attached image 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
Hi Carrie, are we okay with setting 2 ICEs to the same contact?
QA Whiteboard: [COM=Gaia::Contacts]
Flags: needinfo?(cawang)
Blocks: 1026682
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!
Flags: needinfo?(cawang) → needinfo?(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?
Flags: needinfo?(francisco) → needinfo?(cawang)
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 ;))
Flags: needinfo?(cawang)
Assignee: nobody → jpruden92
Status: NEW → ASSIGNED
Attached file 23898.html (obsolete) —
Attachment #8487038 - Flags: review?(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.
Attachment #8487038 - Flags: review?(francisco)
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!
Attachment #8487038 - Flags: review?(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!
Attachment #8487038 - Flags: review?(francisco)
Comment on attachment 8487038 [details]
23898.html

Updated!
Attachment #8487038 - Flags: review?(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/
Attachment #8487038 - Flags: review?(francisco) → review+
'Try' is green :-)
Jorge, sorry, unfortunately we need to rebase again to merge with master.
Flags: needinfo?(jpruden92)
Rebased!
Flags: needinfo?(jpruden92)
Landed:

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

Great job Jorge
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Jorge, one las thing, could you attach a video to show how it works now?
Target Milestone: --- → 2.1 S4 (12sep)
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
Attachment #8487038 - Flags: approval-gaia-v2.1?(bbajaj)
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8487038 [details]
23898.html

Removing the request for approval until we have everything sorted.
Attachment #8487038 - Flags: approval-gaia-v2.1?(bbajaj)
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 :)
Flags: needinfo?(francisco)
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!
(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
Flags: needinfo?(francisco)
Assignee: jpruden92 → francisco
Attached file Pointer to PR 24169
Attachment #8487038 - Attachment is obsolete: true
Attachment #8491385 - Flags: review?(jmcf)
Comment on attachment 8491385 [details] [review]
Pointer to PR 24169

thanks Francisco. good work!
Attachment #8491385 - Flags: review?(jmcf) → review+
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
Attachment #8491385 - Flags: approval-gaia-v2.1?(bbajaj)
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!
Attachment #8491385 - Flags: approval-gaia-v2.1?(bbajaj) → approval-gaia-v2.1-
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!
Flags: needinfo?(l10n)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
This is a release-driver decision. Which you basically got from Fabrice, which was a "no".
Flags: needinfo?(l10n)
(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.
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
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.
Flags: needinfo?(jmcf)
(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
Flags: needinfo?(jmcf)
> 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.
Attached file 24757.html
patch for v2.1, in accordance with the latest solution proposed by product
Attachment #8499612 - Flags: review?(francisco)
Comment on attachment 8499612 [details]
24757.html

Looking good to me!

Thanks for the job Jose!
Attachment #8499612 - Flags: review?(francisco) → review+
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 :)
Attachment #8499612 - Flags: approval-gaia-v2.1?(bbajaj)
Attachment #8499612 - Flags: approval-gaia-v2.1?(bbajaj) → approval-gaia-v2.1+
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [COM=Gaia::Contacts] → [COM=Gaia::Contacts] [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [COM=Gaia::Contacts] [QAnalyst-Triage?] → [COM=Gaia::Contacts] [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.