Eric Chang [:ericcc] [:echang] Reporter
	Description • 10 years ago

### STR
1. Set ICE to some contact A(with number), make sure it appears on top of contact list.
2. Delete contact A from Contacts, delete any other contact that is also set to ICE.
3. ICE list is empty but the whole section should be removed from Contacts. 

### Actual 
ICE list is empty but the whole section should be removed from Contacts. 

### Expected
Contact A should be removed from ICE list.
Graph 4, Page 15 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

	Eric Chang [:ericcc] [:echang] Reporter
	Comment 1 • 10 years ago

[Blocking Requested - why for this release]: Spec

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 2 • 10 years ago

triage: to align with defined spec of new feature

	Francisco Jordano [:arcturus] [:francisco] Assignee
	Comment 3 • 10 years ago

Hi,

the PR looks big, but actually is mostly moving code around. In ice for contacts we have our own data representation (for enabling or disabling the contact), and the shared DataStore through ICEStore.

Before all the data manipulation was happening in ice_settings.js, I've extracted that to ice_data.js, and added a method to subscribe to changes in contacts.

Then ice_settings and the list can listen to changes happening in ice data to perform operations like, remove the ice group, or updating the settings ui.

	Sergi Mansilla [:sergi] (Telenor) [PTO]
	Comment 4 • 10 years ago

Comment on attachment 8486572 [details] [review]
Pointer to PR 23869

The patch is not working for me. This is what's happening:

1. Set ICE to some contact A(with number), make sure it appears on top of contact list.
1. Set ICE to some contact B(with number), make sure it appears on top of contact list.
1. Set a new contact C(with number), NOT on ICE..
2. Delete contact A and B from Contacts.

These contacts are actually not deleted from the list. I have to restart the Contacts app so that they are not there. But then, the ICE header is still there.

	Dietrich Ayala (:dietrich)
	Comment 5 • 10 years ago

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!

	Francisco Jordano [:arcturus] [:francisco] Assignee
	Comment 6 • 10 years ago

Comment on attachment 8486572 [details] [review]
Pointer to PR 23869

Can you give it a second try?

I updated the pr and also fixed all the unit tests that I broke :)

	Sergi Mansilla [:sergi] (Telenor) [PTO]
	Comment 7 • 10 years ago

Comment on attachment 8486572 [details] [review]
Pointer to PR 23869

Good patch, r+. Thanks Francisco!

	Francisco Jordano [:arcturus] [:francisco] Assignee
	Comment 8 • 10 years ago

Comment on attachment 8486572 [details] [review]
Pointer to PR 23869

I forgot this patch needs to be reviewed by a dialer peer too.

I had to modify some tests on the dialer, since the new mock returns a promise with the results (closer to the original implementation), mocha doesnt accept to call the |done| function with parameters (now we return an array of contacts), and have to workaround that by adding an anonymous function wrapping the call to |done|.

Simple, but better to have Doug taking a look to it.

	Francisco Jordano [:arcturus] [:francisco] Assignee
	Comment 10 • 10 years ago

Comment on attachment 8486572 [details] [review]
Pointer to PR 23869

Now redirecting to my beloved Doug, the real one.

	Doug Sherk (:drs) (inactive)
	Comment 11 • 10 years ago

Comment on attachment 8486572 [details] [review]
Pointer to PR 23869

Looks good. Just a couple of mostly stylistic changes that I commented on in the PR. Note that I only reviewed the dialer changes and MockICEStore.

	Francisco Jordano [:arcturus] [:francisco] Assignee
	Comment 12 • 10 years ago

Landed:

https://github.com/mozilla-b2g/gaia/commit/4ae4d7f68fd9fea6367735425e95a5cdd2140f9f

	Francisco Jordano [:arcturus] [:francisco] Assignee
	Comment 13 • 10 years ago

Loli,

could you verify this bug? Would like to ask for gaia approval and want a QA round on it to be sure it meets our expectations

	Loli (:lolimartinezcr)
	Comment 14 • 10 years ago

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #13)
> Loli,
> 
> could you verify this bug? Would like to ask for gaia approval and want a QA
> round on it to be sure it meets our expectations

Retested and NOT working. When i delete all contacts which were set to ICE, 'ICE contacts' list is empty but the whole ICE section *isn't* removed

Flame
2.1
Gecko-a50444f
Gaia-944e5b4

	Maria Angeles Oteo (:oteo)
	Comment 15 • 10 years ago

(In reply to Loli (:lolimartinezcr) from comment #14)
> (In reply to Francisco Jordano [:arcturus] [:francisco] from comment #13)
> > Loli,
> > 
> > could you verify this bug? Would like to ask for gaia approval and want a QA
> > round on it to be sure it meets our expectations
> 
> Retested and NOT working. When i delete all contacts which were set to ICE,
> 'ICE contacts' list is empty but the whole ICE section *isn't* removed
> 
> Flame
> 2.1
> Gecko-a50444f
> Gaia-944e5b4

Hi Loli, It's not working in 2.1 because it's not uplifted yet.
To confirm that the patch is fine, before asking the approval, Francisco wants you to verify if it's working fine in master first.Could you help us with this?
Thanks a lot!

	Loli (:lolimartinezcr)
	Comment 16 • 10 years ago

Tested and working
2.2
Flame
Gecko-713c41b
Gaia-32d82d2

	Francisco Jordano [:arcturus] [:francisco] Assignee
	Comment 17 • 10 years ago

Comment on attachment 8486572 [details] [review]
Pointer to PR 23869

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Feature ICE contacts
[User impact] if declined:
User will have a section for ice contacts, without content. This is a bad user experience in a feature related to user's security
[Testing completed]:
Addes more unit tests, and verified by QA
[Risk to taking this patch] (and alternatives if risky):
Low risk, the whole patch is about moving data to an external js file to be shared by different parts of the app. We are adding 80% unit test, 20% code in this patch. (and that 20% is moving)
[String changes made]:

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 10 years ago

Needs rebasing for v2.1 uplift.

	Francisco Jordano [:arcturus] [:francisco] Assignee
	Comment 20 • 10 years ago

I've asked for bug 1060725 to be uplifted.

That one should solve the problems with merging, asked for it cause that bug just add more unit tests, and in case of not taking it we will need to rewrite an specific version for this patch.

	Loli (:lolimartinezcr)
	Comment 21 • 10 years ago

Tested and working
Flame 
2.2
Gecko: 725423c
Gaia:7426a62

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 10 years ago

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

	Coler
	Comment 23 • 10 years ago

This issue has been verified successfully on Flame2.1.
Reproducing rate: 0/5
See attachment: Verify_Flame_ICE.mp4

Flame2.1 build version:
Gaia-Rev        5655269098c7e82254e56933f1af05b4abe2a2f3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/86608c9389b5
Build-ID        20141204001201
Version         34.0

Flame2.2 build version:
Gaia-Rev        984e6d79aa799d2695f9ca132dfdc1665a56c019
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/a9fc46355661
Build-ID        20141204040202
Version         37.0a1