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


-            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


        // 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

 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

            }

            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

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

            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

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

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

Uh oh!

Conversation

sdalmeida commented Apr 12, 2017

Uh oh!

sdalmeida commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humphd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humphd commented May 8, 2017

Uh oh!

sdalmeida commented May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdalmeida commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gideonthomas commented Jun 12, 2017

Uh oh!

humphd commented Jun 13, 2017

Uh oh!

humphd commented Jul 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sdalmeida commented May 12, 2017 •

edited

Loading