1036171 - Don't Import Contacts from .Trash

Status: RESOLVED FIXED

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

Description

[dp]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20100101

Steps to reproduce:

Tap on Contacts App, tap on the gear, tap on Import contacts, Tapo on Memory card.


Actual results:

1446 Contacts are imported


Expected results:

723 Contacts should have been imported.

Being unsure of wheter the application will look for vcf in the SD card or in the phone memory, I placed a vcf (of 723 contacts each) in both supports. When I saw that the app looked into both, I connected the phone to my computer and moved through my file system window one of the vcf to the trash. Still, the app looked into /SDCard/.Trash and imported the contacts from the trash. I don't think this should happen, as it would be a problem for most of the unexperienced users. Contacts shouldn't be imported from .Trash

Comment 1

[Sudheesh Singanamalla (:ShellHacker)]

Hey,

I've been looking through the code base and want to work on fixing this particular problem,I already have gaia set up.

I have a few doubts,
Is the code related to import from sd card at : /shared/js/contacts/import/utilities/sdcard.js

and, does a change have to happen here by adding an exception condition for the .Trashes 
```
SdCard.retrieveFiles = function retrieveFilesContent(mimes, exts, cb) {
    var fileArray = [];

    // getDeviceStorages('sdcard') can return more than one storage unit
    // in different order, depends on the default media storage setting,
    // so we need to iterate though them to be sure we checked the proper
    // SD Card
    var storages = navigator.getDeviceStorages('sdcard');
    var numberOfStorages = storages.length;
    var currentStorage = 0;

    var cursorOnError = function() {
      cb(this.error.name);
    };

    var cursorOnSuccess = function(e) {
      var file = e.target.result;
      // If we don't have any more files on the storage...
      if (!file) {
        // ... but we have more storages...
        if (++currentStorage < numberOfStorages) {
          // ... check the next one
          cursor = storages[currentStorage].enumerate();
          cursor.onsuccess = cursorOnSuccess;
          cursor.onerror = cursorOnError;
        } else {
          cb(null, fileArray);
        }
      } else {
        if ((mimes.indexOf(file.type) === -1) &&
          file.name.search(new RegExp('.(' + exts.join('|') + ')$')) === -1) {
          cursor.continue();
        } else {
          fileArray.push(file);
          cursor.continue();
        }
      }
    };

    var cursor = storages[currentStorage].enumerate();
    cursor.onsuccess = cursorOnSuccess;
    cursor.onerror = cursorOnError;
  };
```

Comment 2

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

Hi Sudheesh!

Happy that you want to take this bug, do you know how to contribute to the gaia project? 

I can help you with that.

The first thing will be clonning gaia, so having your own repo, and do a pull request to the official mozilla gaia repo.

Then you link that pull request to this bug and we help you with the review process.

I was thinking using this fix to allow define maybe more directories, perhaps we can just start forbidding the .Trash directory, but we can make it generic, like an array of forbidden directories. What do you think?

If you need help with any of the steps just let me know!

Thanks again!

Comment 3

[Sudheesh Singanamalla (:ShellHacker)]

I have cloned the gaia project and tried setting it up, it was working fine and then I encountered a weird issue [1].

I was thinking of the same thing, to have an array of forbidden directories as strings. Am I looking at the correct code segment as of comment 1, the card import path has been commented out "/shared/js/contacts/import/utilities/sdcard.js"

Am I on the right file or do I have to look somewhere else ?

[1] https://gist.github.com/sudheesh001/77cf2610f74d1edf0f23

Comment 4

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

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #3)
> I was thinking of the same thing, to have an array of forbidden directories
> as strings. Am I looking at the correct code segment as of comment 1, the
> card import path has been commented out
> "/shared/js/contacts/import/utilities/sdcard.js"
> 
> Am I on the right file or do I have to look somewhere else ?
> 

Yes you are in the correct file :)

If we do changes there, that shared file will be used for importing contacts from the contacts app and the first time use app.

Comment 5

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: patch_1036171

Comment 6

[Sudheesh Singanamalla (:ShellHacker)]

I put a patch here, in case this works, I will commit and send a pull request, I am feeling slightly confused about 
```
else {
        if ((mimes.indexOf(file.type) === -1) &&
          file.name.search(new RegExp('.(' + exts.join('|') + ')$')) === -1) {
          cursor.continue();
        } 
```

Comment 7

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

Comment on attachment 8459148 [details] [diff] [review]
patch_1036171

Redirecting the feedback to Sergi, since he created the sdcard part.

Sergi could you provide the feedback on Sudheesh work?

Comment 8

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Comment on attachment 8459148 [details] [diff] [review]
patch_1036171

Hi sudheesh,

In your code, the line

var forbiddenLength = forbiddenDirectories.length;

should be

```
var forbiddenLength = setOfValues.length;
```

Otherwise this function would operate with two different base arrays (setOfValues and forbiddenDirectories), and since you want to make a generic function to find a value in an Array, that would yield incorrect values in some cases.

Moreover, the whole function `inArray` is unnecessary. You should be using Array.prototype.indexOf, which tells you if an array contains a particular item. In your case you would use it like this:

```
if (forbiddenDirectories.indexOf(file.name) === -1){
  fileArray.push(file);
  cursor.continue();
}
```

Also notice that you should check for `file.name`, and not `file` because we are dealing with strings.

About the snippet of code you are confused about, it is checking that the file type is NOT in `mimes` and that it has NOT any of the extensions passed as parameters. The code is admittedly confusing, and compiles a RegExp every time, which is inefficient. You could add the following changes to your patch in order to improve it:


diff --git a/shared/js/contacts/import/utilities/sdcard.js b/shared/js/contacts/import/utilities/sdcard.js
index c8f363c..94f3af2 100644
--- a/shared/js/contacts/import/utilities/sdcard.js
+++ b/shared/js/contacts/import/utilities/sdcard.js
@@ -101,6 +101,8 @@ if (!utils.sdcard) {
       cb(this.error.name);
     };

+    // RegExp that checks if a string contains any of the extensions.
+    var reExt = new RegExp('.(' + exts.join('|') + ')$');
     var cursorOnSuccess = function(e) {
       var file = e.target.result;
       // If we don't have any more files on the storage...
@@ -115,8 +117,7 @@ if (!utils.sdcard) {
           cb(null, fileArray);
         }
       } else {
-        if ((mimes.indexOf(file.type) === -1) &&
-          file.name.search(new RegExp('.(' + exts.join('|') + ')$')) === -1) {
+        if ((mimes.indexOf(file.type) === -1) && !reExt.test(file.name)) {
           cursor.continue();
         } else {
           fileArray.push(file);


Please let me know if that makes sense to you.

Comment 9

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Patch 1036171 Update

Thanks, I hope this patch works ?

Comment 10

[Sudheesh Singanamalla (:ShellHacker)]

Can I know the status of this ? Should I send this as a PR ?

Comment 11

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

Comment on attachment 8460275 [details] [diff] [review]
Patch 1036171 Update

Asking Sergi for second round of reviews :)

Didn't do any review, but could we add unit tests to this? Sudheesh, if you don't know how, we can fill a follow up bug to add the test to this bug and land the code.

Comment 12

[Sudheesh Singanamalla (:ShellHacker)]

I've never worked with test in gaia, maybe I could look through this if a new bug is filed for that and assigned :)

Comment 13

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

Yes,

unit test are mandatory for gaia code, but also we understand that are hard to learn. So as we like to get new contributors on board and help them to learn how it's the process, we can do that.

We create a new bug, we call it, Follow up bug 1036171 Add unit tests to 'Dont import contacts from .Trash' and we reference this bug in the new one.

So you have another bug to learn how to do the unit test, and you'll have learn everything you need :)

Comment 14

[Sudheesh Singanamalla (:ShellHacker)]

I've made the bug to track the unit test, I've been looking through the test_contacts.py which tests a mock contact data. I also understand that we have to write a new function

def test_retrieve_contact_sdcard():

Comment 15

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

Hi Sudheesh,

those are integration tests, we will need to add unit tests, since we will need to mock the use of a sdcard. You can take a look to the sdcard test file here:
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/test/unit/import_vcard_test.js

Thanks!

Comment 16

[Sudheesh Singanamalla (:ShellHacker)]

I think I understand whats happening there, Do I have to make a new multiple vCard for testing with the .Trash ?

This could be called vCardTrash = 'Dont Import' and the suiteSetup() function doesn't take place. ?

Comment 17

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

Hei Sergi,

any news with this review?

Comment 18

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

Comment on attachment 8460275 [details] [diff] [review]
Patch 1036171 Update

Changing the review to myself, since sergi seems to be not active.

Comment 19

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

Comment on attachment 8460275 [details] [diff] [review]
Patch 1036171 Update

LGTM, the only problem is the ".Trash", double quoted string, should be '.Trash', singled quoted.

Sudheesh, could you do a PR against gaia and assign to me the review?

Comment 20

[GitHub Autolander Bot]

Attachment: [gaia] sudheesh001:Patch_1036171 > mozilla-b2g:master

Comment 21

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Patch_1036171

Francisco, I've made the required changes and kept the patch upto date with master. Could you please review this now.

Thanks

Comment 22

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

Comment on attachment 8682313 [details] [review]
Patch_1036171

Looking perfect thanks!

Comment 23

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

Landed:

https://github.com/mozilla-b2g/gaia/commit/347b22c572fee1bfe97a455679a37c373c37a0e3

And nga branch:

5cc476b