Fixed mozilla/thimble.mozilla.org#1887: File overwrites without notifying user #712

sdalmeida · 2017-04-12T18:00:45Z

Fixed bug where imported items could overwrite local files without notifying the user (As shown here: https://github.com/mozilla/thimble.mozilla.org/issues/1887).

While working on the file, I've added some spaces (for better code indentation). Let me know if thats something you want reverted 👍

sdalmeida · 2017-04-12T18:11:44Z

By the way, I just noticed that this PR needs to land first

#659

I've changed the title to WIP. Once that lands, I'll add my fix 👍

humphd · 2017-04-23T16:25:40Z

src/filesystem/impls/filer/ArchiveUtils.js

-                            fs.writeFile(path.absPath, path.data, callback);
-                        }
+                            if (stats.type === "FILE") {
+                                //console.log("File exists!", path.absPath);


Remove this please.

humphd · 2017-04-23T16:27:16Z

src/filesystem/impls/filer/ArchiveUtils.js

                return callback();
            }

-            fs.mkdirp(basedir, function(err) {
-                if(err && err.code !== "EEXIST") {
+            fs.mkdirp(basedir, function (err) {


It looks to me like we're duplicating code here from above. Is there any way to refactor this to a function that both call?

humphd · 2017-04-23T16:28:22Z

src/filesystem/impls/filer/ArchiveUtils.js

+                    if (err && err.code !== "ENOENT") {
+                        return callback(err);
+                    }
+                    if (stats.type === "FILE") {


If you don't care about any case other that "FILE" do this:

if (stats.type !== "FILE") { return callback(); } // continue without indenting ...

humphd

Left a few comments, and I'll do a deeper dive after you've dealt with these. Overall it looks good.

gideonthomas · 2017-04-24T19:40:08Z

src/filesystem/impls/filer/ArchiveUtils.js


    // Mac and Windows clutter zip files with extra files/folders we don't need
    function _skipFile(filename) {
        var basename = Path.basename(filename);

        // Skip OS X additions we don't care about in the browser fs
-        if(/^__MACOSX\//.test(filename)) {
+        if (/^__MACOSX\//.test(filename)) {


undo all these space changes

humphd · 2017-05-08T19:33:45Z

I've landed #659 so, feel free to rebase and finish this.

sdalmeida · 2017-05-12T19:55:29Z

This might take a bit to complete. If this is an urgent issue that needs to be completed soon, let me know. I'll continue working on it :)

From debugging, looks like the code was moved to "WebKitFileImport.js". Going to make my changes there and test 👍

sdalmeida · 2017-05-12T23:29:27Z

Wasn't as hard as I thought.
For some odd reason, the import doesn't trigger if the .tar archive contains empty files (0 bytes). Not too sure how files are stored when they are tar'd.

Going to remove the [WIP] label.
Please let me know if there's anything you want changed 👍

humphd · 2017-05-31T18:08:19Z

locales/en-US/editor.properties

@@ -136,6 +139,7 @@ DND_SUCCESS_UNTAR_TITLE=Untar Completed Successfully
 DND_SUCCESS_UNZIP=Successfully unzipped <b>{0}</b>.
 # {0} will be replaced by a tar filename
 DND_SUCCESS_UNTAR=Successfully untarred <b>{0}</b>.
+DND_FILE_REPLACE=A file named \"{0}\" already exists in this location. Do you want to use the imported file or keep the existing?


Have all these strings been added to Thimble's repo yet?

humphd · 2017-05-31T18:08:35Z

src/filesystem/impls/filer/ArchiveUtils.js

+    var Strings         = require("strings");
+    var StringUtils     = require("utils/StringUtils");
+
+    // These are const variables


Get rid of this comment.

humphd · 2017-05-31T18:09:33Z

src/filesystem/impls/filer/ArchiveUtils.js

                        }
+                        fs.stat(path.absPath, function(err, stats) {
+                            if(err && err.code !== "ENOENT") {
+                                    return callback(err);


Too much indent here.

humphd · 2017-05-31T18:10:30Z

src/filesystem/impls/filer/ArchiveUtils.js

@@ -227,11 +288,33 @@ define(function (require, exports, module) {
            }

            fs.mkdirp(basedir, function(err) {
-                if(err && err.code !== "EEXIST") {


Why this change?

humphd · 2017-05-31T18:11:05Z

src/filesystem/impls/filer/lib/WebKitFileImport.js

            file.write(buffer, {encoding: encoding}, function(err) {
                if (err) {
                    onError(deferred, filename, err);
                    return;
                }
-
+                                


Get rid of this change.

humphd · 2017-05-31T18:11:18Z

src/filesystem/impls/filer/lib/WebKitFileImport.js

+        function handleRegularFile(deferred, file, filename, buffer, encoding) {
+            fs.exists(filename, function(doesExist) {
+                if (doesExist) {
+                    console.log("File: ", filename, " already exists!");


Remove this.

humphd · 2017-05-31T18:12:55Z

src/filesystem/impls/filer/lib/WebKitFileImport.js

+                        }
+                    });
+                } else {
+                    // File doesn't exist. Save without prompt


Do this case first, and then return, and then you don't need to indent everything above so much:

if (!doesExist) { // File doesn't exist. Save without prompt saveFile(deferred, file, filename, buffer, encoding); return; } // rest of code here for other case.

humphd · 2017-05-31T18:13:15Z

src/utils/DragAndDrop.js

@@ -182,6 +181,7 @@ define(function (require, exports, module) {
            return result.promise();
        }, false)
            .fail(function () {
+                console.log("fail");


Revert all the changes in this file.

gideonthomas · 2017-06-12T23:34:02Z

@Simon66 are you still working on this?

humphd · 2017-06-13T00:04:33Z

If he's not, I'll finish it.

humphd · 2017-07-20T16:32:38Z

Finishing this in #847

sdalmeida changed the title ~~Fixed mozilla/thimble.mozilla.org#1887: File overwrites without notifying user~~ [WIP] Fixed mozilla/thimble.mozilla.org#1887: File overwrites without notifying user Apr 12, 2017

flukeout added the Launch label Apr 13, 2017

humphd reviewed Apr 23, 2017

View reviewed changes

humphd suggested changes Apr 23, 2017

View reviewed changes

gideonthomas reviewed Apr 24, 2017

View reviewed changes

flukeout removed the Launch label May 4, 2017

Added Dialog prompt for single file import

25bfe6b

sdalmeida force-pushed the issue-1887 branch from ce35879 to 25bfe6b Compare May 12, 2017 19:56

Added warning if imported file overwrites local files.

fca9257

sdalmeida force-pushed the issue-1887 branch from ae3dd32 to fca9257 Compare May 12, 2017 22:30

Fixed Cancel on tar archives

0e9075d

sdalmeida changed the title ~~[WIP] Fixed mozilla/thimble.mozilla.org#1887: File overwrites without notifying user~~ Fixed mozilla/thimble.mozilla.org#1887: File overwrites without notifying user May 12, 2017

humphd suggested changes May 31, 2017

View reviewed changes

humphd mentioned this pull request Jul 20, 2017

Fix mozilla/thimble.mozilla.org#1887: File overwrites without notifying user #847

Open

humphd closed this Jul 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed mozilla/thimble.mozilla.org#1887: File overwrites without notifying user #712

Fixed mozilla/thimble.mozilla.org#1887: File overwrites without notifying user #712

sdalmeida commented Apr 12, 2017

sdalmeida commented Apr 12, 2017

humphd Apr 23, 2017

humphd Apr 23, 2017

humphd Apr 23, 2017

humphd left a comment

gideonthomas Apr 24, 2017

humphd commented May 8, 2017

sdalmeida commented May 12, 2017 •

edited

Loading

sdalmeida commented May 12, 2017

humphd May 31, 2017

humphd May 31, 2017

humphd May 31, 2017

humphd May 31, 2017

humphd May 31, 2017

humphd May 31, 2017

humphd May 31, 2017

humphd May 31, 2017

gideonthomas commented Jun 12, 2017

humphd commented Jun 13, 2017

humphd commented Jul 20, 2017

Fixed mozilla/thimble.mozilla.org#1887: File overwrites without notifying user #712

Fixed mozilla/thimble.mozilla.org#1887: File overwrites without notifying user #712

Conversation

sdalmeida commented Apr 12, 2017

sdalmeida commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd commented May 8, 2017

sdalmeida commented May 12, 2017 • edited Loading

sdalmeida commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gideonthomas commented Jun 12, 2017

humphd commented Jun 13, 2017

humphd commented Jul 20, 2017

sdalmeida commented May 12, 2017 •

edited

Loading