This will enable the user to delete multiple contacts. Steps: Contacts->Settings->Bulk Delete->Select Contacts-> Delete->Confirmation Window->Deleting Status->Update Deleting Contacts.

Please review the patch and update your review comments. Thanks, Ashay.

@Wchang, Please assign a reviewer as Francisco is PTO. Regards Ashay.

Comment on attachment 8355999 [details] Pointer to Pull Request.html as per the comments on GH the patch needs to fix important issues and an architectural change.

Hi Ashay, I created bug 957577 to deal with the updates needed at the navigation.js library level to properly deal with multiple instances of navigationStack objects since it is a kind of sensitive change which is better to have isolated and not included in the patch needed to solve this bug (the multiple contacts deletion one). I will send a pull request including a patch proposal and ask feedback from you ;-) Thanks!

As you may already know, I have just merged the patch to solve bug 957577 so you may safely remove any updates to the navigation.js file from your final patch, Ashay ;-) Thanks!

@francisco, Please check WIP patch for multiple contact delete. I will provide the unit test cases ASAP.

Please review the new pull request.

Hi Guys Good work. Bit of UX feedback following review of video in 8355420. 1) @0:03 Language. ‘Bulk Delete’ should be ‘Delete Contacts’ 2) @0:04 The visual treatment should be aligned to our other delete modes when a list is presented such as delete messages from inbox or thread or delete from call log. ni? to Jose to provide you the visual consultation here when you are ready for it 3) @0:06 Header should reflect the number of contacts selected as we are doing in adding multiple contacts to the ‘to field’ of a new message 4) @0:10 Language. ‘You want continue delete n contacts’ should be ‘Delete n contacts? This cannot be undon’ 5) @0:17 Language. ’30 deletions’ should be ’30 contacts removed’ Please remember to ni? me for feedback on UX work within the Messages and Contacts apps Thanks

Comment on attachment 8367230 [details] Pointer to Pull Request.html Amazing job!! Just left some comments on the github repo, non blocking most of them, but we will need to fix them. Please also add the comments from Ayman regarding wording. Could you provided a rebased patch to try on the phone. Again, incredible job, thanks for the contribution!

@Francisco, Please review the updated pull request.

@francisco, Please find attached updated patch as requested.

Hi Ashay! This is looking amazing! We are almost there, just sent you a PR to that branch to try to bypass the linting errors that we are having in travis. Also unit testing is not passing but the problem seems it's not related to your PR, and also right now Gaia tree is closed so I hope one we are green again we can run the unit tests. Thanks again!

Comment on attachment 8372103 [details] Pointer to Pull Request.html r+, we will merge once the problems with travis are ok. Cheers!

Are you expecting any thing from me to do any further changes or we are done ?

(In reply to ashayb2g from comment #15) > Are you expecting any thing from me to do any further changes or we are done > ? Yes, I sent you a PR to the branch you are working on, there are some fixes for the linting problem in travis. Could you apply my PR to your branch? Cheers, Francisco

Done. Please check and let me know in case any thing. Thanks :)

(In reply to ashayb2g from comment #17) > Done. > Please check and let me know in case any thing. > > Thanks :) We are getting there, could you rebase the patch and squash all the commits in a single one? Thank you very much!!

Please check and let me know in case any thing. Thanks

Code is updated to the same pull request.

Great work guys. Final bit of UX feedback following review of patch referencing comment 9: > 1) @0:03 Language. ‘Bulk Delete’ should be ‘Delete Contacts’ done. thanks > 2) @0:04 The visual treatment should be aligned to our other delete modes > when a list is presented such as delete messages from inbox or thread or > delete from call log. ni? to Jose to provide you the visual consultation > here when you are ready for it The checkboxes are still blue, they need to be red to align to the visual treatment of ‘delete mode’ across the device. Please refer to attachment ’01 - message app edit mode’ for illustration > 3) @0:06 Header should reflect the number of contacts selected as we are > doing in adding multiple contacts to the ‘to field’ of a new message done. thanks > 4) @0:10 Language. ‘You want continue delete n contacts’ should be ‘Delete > n contacts? This cannot be undon’ done. thanks > 5) @0:17 Language. ’30 deletions’ should be ’30 contacts removed’ done. thanks

removing ni? for Jose as no visual design work is currently required. if it is please ni? Jose again

Sounds great! I think we can do in a follow up what Ayman is asking for. So far now that unit tests are working again, I can see that we have a problem in the tests. Will try to provide again a patch over the development branch to have it ready and land. Thanks everyone!

Hi! Travis is still red so we cannot merge, could you review your unit tests? My guess is that we are not correctly cleaning the mocks at some point, but didn't see where. Thanks!

Hi Francisco, I checked unit tests, but couldn't found any mocks left uncleaned. As per travis I see there is a problem with contacts_test.js. I'll see it further and let you know in case I found any thing related.

Hi Francisco, PFA updated PR for fixing Unit test cases and lint, there were some issues with existing test cases also, so please check and land the patch on master. As of now travis is failing in "test_sms_to_settings.py" ui test which is intermittent. Discussed with 'Zac' on IRC , the test case is failing intermittently and they are working on it and we see that other engineers landing patches.

Comment on attachment 8382945 [details] Pointer to Pull Request.html Looking good! Just left a tiny comment on github. Now we need travis to get green! Excellent job, thank you so much!

Tests Cases created, still this bug isn't tested because it have to be landed.

@Loli, ni: I could not get your point.

(In reply to ashayb2g from comment #30) > @Loli, > > ni: I could not get your point. Sorry, only i want to say that i have created tests cases for this bug.

TEF has to send test cases (when this bug is tested) to be included in Moztrap

Hi Francisco, PFA updated pull request, this time travis is green, so we can land it.

Hi, Ashay I was looking at latest comments from Jose in github regarding the problem with FB contacts. We could land this in master and then do a follow up, but I prefer to do that once we branch 1.4, and we land this on master. Cheers, F.

Could you mark the patches that are obsolete? I got confused looking at different patches :S

Hi Francisco, I have removed all previous PRs and updated this new one with travis as green. Regarding fb contact delete, the code is present in contacts_remover.js. Please check and let me know incase any thing.

Comment on attachment 8386538 [details] Pointer to Pull Request.html Time has come to land this feature! Sorry for the delay but we branched past monday and now we should land this ASAP in current master. Would you mind to rebase, we did already the review, after a rebase a quick review and we are ready to land!

Hi! Can you rebase? So we can land the multiple delete?

Just did the rebase myself, it's pretty straigh forward, just a conflict in the settings test that is pretty simple. Here is the branch if someone want to test: https://github.com/arcturus/gaia/tree/multipledelete Working like charm, once we have the merge in the proper branch we just merge :) Thanks for the hard work and the patience!

I'll go and merge this bug in the behalf of Ashay, since this feature is amazingly needed and the work done here is outstanding.

I'm submitting this patch rebased in Ashay behalf, carrying over the r+ from my self. Also took a look anyway and looking perfect, so will directly merge in master.

Landed: https://github.com/mozilla-b2g/gaia/commit/32078fe945de8001765526fa7e8e1283d3c09669 Again with a green travis: https://travis-ci.org/mozilla-b2g/gaia/builds/21417661

(In reply to Francisco Jordano [:arcturus] from comment #42) > Landed: > > https://github.com/mozilla-b2g/gaia/commit/ > 32078fe945de8001765526fa7e8e1283d3c09669 > > Again with a green travis: > > https://travis-ci.org/mozilla-b2g/gaia/builds/21417661 This comment is for a different bug, sorry for the inconvenience :(

In behalf of Ashay final PR mergeable, with green travis

Finally!! Landed: https://github.com/mozilla-b2g/gaia/commit/ba7d1fe9ad667750f75755873539678454ebd760 Thanks a lot Ashay for the contribution!

Thanks Francisco :)

Created a follow up for this bug to solve inconsitencies: https://bugzilla.mozilla.org/show_bug.cgi?id=991141 Thanks!

Hi Al, I haven't found any manual test case for this feature in Moztrap. I created one [1] to match the next automated test I'll implement [2]. Do you have the other test case ready to be imported in Moztrap? [1] https://moztrap.mozilla.org/manage/case/15181/ [2] Bug 1113188

I don't have test cases for the feature. Would you mind to help on this one? Eric may also provide some suggestion.

Here is another case I found in moztrap, it seems we don't have a lot on this feature. https://moztrap.mozilla.org/manage/case/9903/ https://moztrap.mozilla.org/manage/case/15181/

I don't really agree with minussing this feature, I think we should have some coverage around it, like we need to check what happens if we try to delete a FB or a merged contact, check if a user is still selected when you use the search bar. These are only example. We would need to take some time to get a test plan. What do you think, Eric?

Oh Yes, I agree that we should have coverage on this, another thing is that since we do not have an idea of what was covered and what was not, we can either create cases for uncovered features once we see that or create an overview of test case coverage.