Handle missing files during an export and open #566

sssoleileraaa · 2019-10-10T23:43:15Z

Description

Tells the user that the file they're trying to export is missing from the file system. Next step is to show a Download link once again for the file that no longer exists on the filesystem, captured in this issue: #565

sssoleileraaa · 2019-10-16T00:51:19Z

Note: this fixes an unhandled FileNotFoundError which crashes the app

redshiftzero

this is nice! one other thought: we might want to mark the file as not downloaded such that the user could then redownload the file (right now it's still marked as downloaded in the database so the download button isn't shown)

securedrop_client/logic.py

sssoleileraaa · 2019-10-22T00:18:00Z

I had to rebase due to conflicts. I addressed the PR comment about displaying the "Download" link when the file is missing during either an open or export operation. I also added some cleanup code after export using the new export_usb_call_success signal which I was able to access after rebasing. I still need to update tests but will ask for a new review once ready. We are tracking clean up for the open function in this Issue: #579 (comment)

sssoleileraaa · 2019-10-22T03:25:48Z

Finished writing unit tests. For manual testing, here's what you should do:

Regression Test

Send a file as a source.
Download the file, see that it exists in the data directory.
Open the file.
Close out of the display VM.
Open the file again.
Do the same steps but for export.

Testing new behavior for File Open

Send a file as a source.
Download the file, see that it exists in the data directory.
Delete the file from the data directory using rm
Open the file and see error message and wait about 10 seconds for the api sync to finish before displaying the "Download" link next to file again.
Download the file and open.
Notice the file with the original filename still exists in the data directory, which is fine for now and being tracked in a separate issue.

Testing new behavior for File Export

Send a file as a source.
Download the file, see that it exists in the data directory.
Delete the file from the data directory using rm
Export the file and see error message and wait about 10 seconds for the api sync to finish before displaying the "Download" link next to file again.
Download the file and export successfully.
Notice the file with the original filename no longer exists in the data directory.

redshiftzero

this looks great! from some testing in Qubes:

✅ Regression test: Opening multiple times in dispVM success
✅ Regression test: Export multiple times to USB drive success
✅ New behavior: after file is deleted, after open attempt an error message is shown and on the next sync, download link appears (we'll need to eventually update this file widget via a signal but let's leave this for a followup)
✅ New behavior: after file is deleted and download link appears, then file can be redownloaded and opened successfully
✅ New behavior: after file is deleted and export is clicked, after export an error message is shown and on next sync the download link appears

⚠️ but: the ExportDialog is not closed when the error message is shown

✅ New behavior: after file is deleted and download link appears, then file can be redownloaded and exported successfully

redshiftzero · 2019-10-23T15:09:01Z

securedrop_client/logic.py

+            storage.mark_as_not_downloaded(file_uuid, self.session)
+            self.sync_api()
+            logger.debug(msg)
+            self.gui.update_error_status(msg)


hmm so we have this same stanza in three places (almost) repeated other than the msg having "export" or "open" - what do you think about adding another method for this logic called e.g. _check_if_file_exists? I think it would also make reading this function a bit clearer. We could also make the error message generic so we don't even need to pass in the operation: "Could not complete action on file {}. File does not exist"

I like your _check_if_file_exists suggestion since we use it in three places: opening a file, preflight check, and exporting a file.

actually thinking about this more, it would make more sense to verify the file system accurately reflects the local db during a sync rather than whenever the user tries to access the file. i'll modify the code so that we:

check that the file exists and maybe use https://pypi.org/project/filelock/ so make sure it can't be accessed during open or export

mark the file to be redownloaded if it's missing during a sync

redshiftzero · 2019-10-23T15:14:37Z

securedrop_client/logic.py

+            storage.mark_as_not_downloaded(file_uuid, self.session)
+            self.sync_api()
+            logger.debug(msg)
+            self.gui.update_error_status(msg)


the other thing we need to decide is how to close the export dialog in the file not found case. some options:

Add a little method on the Export object that closes the ExportDialog, which we can call from the controller. This would work, but I don't love this approach because there already is a method for handling errors in the export process which is via the pre_flight and usb failure signals.

At this failure stage, emit the pre_flight or usb export failure signal and modify ExportDialog._update to handle a new error status: when the file is not there. The ExportDialog can just close in this case because we are showing the error in the main window.

Move this file not found logic to before the export dialog even pops up.

I'm a fan of either 2 or 3. What do you think?

yeah, right now, we just wait for the gui to refresh before the dialog closes when the file is missing. i think using the failure signal makes the most sense since the file could be deleted while the dialog is open so it would be good to be able to have code in place to close the dialog upon a file missing error.

sssoleileraaa · 2019-10-26T00:57:54Z

⚠️ but: the ExportDialog is not closed when the error message is shown

this has been fixed and now syncing includes marking files as not downloaded if they are missing from the file system. this is useful if say a user decided to move a bunch of files in the data directory (people working in qubes often run linux commands so this is plausible but user testing will be helpful here) and now we will set them all to be marked as not downloaded rather than waiting unti lthe user tries to open one.

redshiftzero

lgtm! Retested the export scenarios with the latest changes: the export UI does not appear in the file missing case and export to USB occurs without issue once the file is redownloaded

redshiftzero · 2019-10-28T15:11:53Z

securedrop_client/gui/widgets.py

@@ -1799,6 +1799,10 @@ def _on_export_clicked(self):
        """
        Called when the export button is clicked.
        """
+        if not self.controller.downloaded_file_exists(self.file.uuid):
+            self.controller.sync_api()


do we need to resync the API? we could just update the file with is_downloaded=False and then update the UI. I'll file a followup for this though since imho we'll should do this via sending a signal to FileWidget which is a larger change

I think maybe we could do both? Let's discuss in the followup ticket you opened.

sssoleileraaa requested review from kushaldas and redshiftzero as code owners October 10, 2019 23:43

sssoleileraaa force-pushed the check-file branch from b1eba24 to 48c7f13 Compare October 10, 2019 23:43

eloquence changed the title ~~handle missing file case~~ Handle missing files during an export attempt Oct 11, 2019

redshiftzero suggested changes Oct 16, 2019

View reviewed changes

securedrop_client/logic.py Outdated Show resolved Hide resolved

securedrop_client/logic.py Outdated Show resolved Hide resolved

sssoleileraaa force-pushed the check-file branch from 48c7f13 to 4ac6354 Compare October 22, 2019 00:06

sssoleileraaa added the bug Something isn't working label Oct 22, 2019

redshiftzero self-requested a review October 22, 2019 14:25

sssoleileraaa changed the title ~~Handle missing files during an export attempt~~ Handle missing files during an export and open Oct 22, 2019

redshiftzero suggested changes Oct 23, 2019

View reviewed changes

redshiftzero reviewed Oct 23, 2019

View reviewed changes

Allie Crevier added 7 commits October 25, 2019 17:52

handle missing file case

81e4d89

dummy intead of _ to avoid gettext conflict

9e51395

pass session to storage function

76005de

check if file exists before preflight

5dd8657

test more paths for opening and exporting a file

84dd123

cleanup files during the failure case too

8b0d526

do not open export dialog if file missing

83d5e7f

sssoleileraaa force-pushed the check-file branch from 6d3e940 to 83d5e7f Compare October 26, 2019 00:52

sssoleileraaa requested a review from redshiftzero October 26, 2019 00:54

redshiftzero approved these changes Oct 28, 2019

View reviewed changes

redshiftzero merged commit 4472a9c into master Oct 28, 2019

redshiftzero deleted the check-file branch October 28, 2019 15:17

redshiftzero mentioned this pull request Oct 28, 2019

update FileWidget via signal to mark as not downloaded #596

Closed

sssoleileraaa mentioned this pull request Oct 31, 2019

Show download link if the file is missing #565

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle missing files during an export and open #566

Handle missing files during an export and open #566

sssoleileraaa commented Oct 10, 2019

sssoleileraaa commented Oct 16, 2019

redshiftzero left a comment

sssoleileraaa commented Oct 22, 2019

sssoleileraaa commented Oct 22, 2019

redshiftzero left a comment

redshiftzero Oct 23, 2019

sssoleileraaa Oct 24, 2019

sssoleileraaa Oct 25, 2019

redshiftzero Oct 23, 2019

sssoleileraaa Oct 24, 2019

sssoleileraaa commented Oct 26, 2019

redshiftzero left a comment

redshiftzero Oct 28, 2019

sssoleileraaa Oct 28, 2019

Handle missing files during an export and open #566

Handle missing files during an export and open #566

Conversation

sssoleileraaa commented Oct 10, 2019

Description

sssoleileraaa commented Oct 16, 2019

redshiftzero left a comment

Choose a reason for hiding this comment

sssoleileraaa commented Oct 22, 2019

sssoleileraaa commented Oct 22, 2019

Regression Test

Testing new behavior for File Open

Testing new behavior for File Export

redshiftzero left a comment

Choose a reason for hiding this comment

redshiftzero Oct 23, 2019

Choose a reason for hiding this comment

sssoleileraaa Oct 24, 2019

Choose a reason for hiding this comment

sssoleileraaa Oct 25, 2019

Choose a reason for hiding this comment

redshiftzero Oct 23, 2019

Choose a reason for hiding this comment

sssoleileraaa Oct 24, 2019

Choose a reason for hiding this comment

sssoleileraaa commented Oct 26, 2019

redshiftzero left a comment

Choose a reason for hiding this comment

redshiftzero Oct 28, 2019

Choose a reason for hiding this comment

sssoleileraaa Oct 28, 2019

Choose a reason for hiding this comment