Log all missing files & improve flash message

- If more than one file is missing in bulk download, add a log entry for
  each missing file before returning the FileNotFound exception.
- Update unit test to verify logging of multiple missing files.

- Improve flashed error message.

- Rename tests, fixture and journalist navigation steps to align with
  changes.

- Rename file_ variable to file.

securedrop/journalist_app/col.py

@@ -95,13 +95,16 @@ def download_single_file(filesystem_id: str, fn: str) -> werkzeug.Response:
         if '..' in fn or fn.startswith('/'):
             abort(404)
 
-        file_ = current_app.storage.path(filesystem_id, fn)
-        if not Path(file_).is_file():
+        file = current_app.storage.path(filesystem_id, fn)
+        if not Path(file).is_file():
             flash(
-                gettext("An unexpected error occurred! Please inform your admin."),
+                gettext(
+                    "An error occured: file not found. "
+                    + "An admin can find more information in the system logs."
+                ),
                 "error"
             )
-            current_app.logger.error("File {} could not be found.".format(file_))
+            current_app.logger.error("File {} not found".format(file))
             return redirect(url_for("col.col", filesystem_id=filesystem_id))
 
         # mark as seen by the current user

securedrop/journalist_app/utils.py

@@ -204,14 +204,14 @@ def download(zip_basename: str, submissions: List[Union[Submission, Reply]]) ->
     """
     try:
         zf = current_app.storage.get_bulk_archive(submissions, zip_directory=zip_basename)
-    except FileNotFoundError as e:
+    except FileNotFoundError:
         flash(
-            gettext("An unexpected error occurred! Please inform your admin."),
+            gettext(
+                "An error occured: file not found. "
+                + "An admin can find more information in the system logs."
+            ),
             "error"
         )
-
-        current_app.logger.error("File {} could not be found.".format(e.filename))
-
         referrer = urlparse(str(request.referrer)).path
         return redirect(referrer)
 

securedrop/store.py

@@ -201,25 +201,36 @@ def get_bulk_archive(self,
                        for i in selected_submissions])
         # The below nested for-loops are there to create a more usable
         # folder structure per #383
+        missing_files = False
+
         with zipfile.ZipFile(zip_file, 'w') as zip:
             for source in sources:
                 fname = ""
                 submissions = [s for s in selected_submissions
                                if s.source.journalist_designation == source]
                 for submission in submissions:
                     filename = self.path(submission.source.filesystem_id, submission.filename)
-                    document_number = submission.filename.split('-')[0]
-                    if zip_directory == submission.source.journalist_filename:
-                        fname = zip_directory
+
+                    if os.path.exists(filename):
+                        document_number = submission.filename.split('-')[0]
+                        if zip_directory == submission.source.journalist_filename:
+                            fname = zip_directory
+                        else:
+                            fname = os.path.join(zip_directory, source)
+                        zip.write(filename, arcname=os.path.join(
+                            fname,
+                            "%s_%s" % (document_number,
+                                       submission.source.last_updated.date()),
+                            os.path.basename(filename)
+                        ))
                     else:
-                        fname = os.path.join(zip_directory, source)
-                    zip.write(filename, arcname=os.path.join(
-                        fname,
-                        "%s_%s" % (document_number,
-                                   submission.source.last_updated.date()),
-                        os.path.basename(filename)
-                    ))
-        return zip_file
+                        missing_files = True
+                        current_app.logger.error("File {} not found".format(filename))
+
+        if missing_files:
+            raise FileNotFoundError
+        else:
+            return zip_file
 
     def move_to_shredder(self, path: str) -> None:
         """

securedrop/tests/functional/journalist_navigation_steps.py

@@ -1109,12 +1109,15 @@ def _journalist_uses_js_buttons_to_download_unread(self):
             classes = checkbox.get_attribute("class")
             assert "unread-cb" in classes
 
-    def _journalist_sees_unexpected_error_message(self):
+    def _journalist_sees_missing_file_error_message(self):
         notification = self.driver.find_element_by_css_selector(".error")
 
-        if not hasattr(self, "accept_languages"):
-            expected_text = "An unexpected error occurred! Please inform your admin."
-            assert expected_text in notification.text
+        if self.accept_languages is None:
+            expected_text = (
+                "An error occured: file not found. "
+                + "An admin can find more information in the system logs."
+            )
+            assert expected_text == notification.text
 
     def _journalist_is_on_collection_page(self):
         return self.wait_for(

securedrop/tests/functional/test_journalist.py

@@ -116,23 +116,23 @@ def test_download_source_unread(self, missing_msg_file):
         journalist home page."""
         self._journalist_logs_in()
         self._journalist_clicks_source_unread()
-        self._journalist_sees_unexpected_error_message()
+        self._journalist_sees_missing_file_error_message()
         self._is_on_journalist_homepage()
 
     def test_select_source_and_download_all(self, missing_msg_file):
         """Test behavior when the journalist selects the source then clicks the "Download" button
         from the journalist home page."""
         self._journalist_logs_in()
         self._journalist_selects_first_source_then_download_all()
-        self._journalist_sees_unexpected_error_message()
+        self._journalist_sees_missing_file_error_message()
         self._is_on_journalist_homepage()
 
     def test_select_source_and_download_unread(self, missing_msg_file):
         """Test behavior when the journalist selects the source then clicks the "Download Unread"
         button from the journalist home page."""
         self._journalist_logs_in()
         self._journalist_selects_first_source_then_download_unread()
-        self._journalist_sees_unexpected_error_message()
+        self._journalist_sees_missing_file_error_message()
         self._is_on_journalist_homepage()
 
     def test_download_message(self, missing_msg_file):
@@ -141,7 +141,7 @@ def test_download_message(self, missing_msg_file):
         self._journalist_logs_in()
         self._journalist_checks_messages()
         self._journalist_downloads_message_missing_file()
-        self._journalist_sees_unexpected_error_message()
+        self._journalist_sees_missing_file_error_message()
         self._journalist_is_on_collection_page()
 
     def test_select_message_and_download_selected(self, missing_msg_file):
@@ -150,5 +150,5 @@ def test_select_message_and_download_selected(self, missing_msg_file):
         self._journalist_logs_in()
         self._journalist_selects_the_first_source()
         self._journalist_selects_message_then_download_selected()
-        self._journalist_sees_unexpected_error_message()
+        self._journalist_sees_missing_file_error_message()
         self._journalist_is_on_collection_page()

securedrop/tests/test_journalist.py

@@ -2,6 +2,7 @@
 import base64
 import binascii
 import io
+from mock import call
 import os
 from pathlib import Path
 import pytest
@@ -2288,24 +2289,25 @@ def test_download_selected_submissions_previously_downloaded(
 
 
 @pytest.fixture(scope="function")
-def selected_missing_file(journalist_app, test_source):
-    """Fixture for the download tests with missing file in storage."""
+def selected_missing_files(journalist_app, test_source):
+    """Fixture for the download tests with missing files in storage."""
     source = Source.query.get(test_source["id"])
     submissions = utils.db_helper.submit(source, 2)
-    selected = [s.filename for s in submissions]
+    selected = sorted([s.filename for s in submissions])
 
     storage_path = Path(journalist_app.storage.storage_path)
-    msg_files = [p for p in storage_path.rglob("*") if p.is_file()]
+    msg_files = sorted([p for p in storage_path.rglob("*") if p.is_file()])
     assert len(msg_files) == 2
-    msg_files[0].unlink()
+    for file in msg_files:
+        file.unlink()
 
     yield selected
 
 
-def test_download_selected_submissions_missing_file(
-    journalist_app, test_journo, test_source, mocker, selected_missing_file
+def test_download_selected_submissions_missing_files(
+    journalist_app, test_journo, test_source, mocker, selected_missing_files
 ):
-    """Tests download of selected submissions with missing file in storage."""
+    """Tests download of selected submissions with missing files in storage."""
     mocked_error_logger = mocker.patch('journalist.app.logger.error')
     journo = Journalist.query.get(test_journo["id"])
 
@@ -2321,31 +2323,32 @@ def test_download_selected_submissions_missing_file(
             data=dict(
                 action="download",
                 filesystem_id=test_source["filesystem_id"],
-                doc_names_selected=selected_missing_file,
+                doc_names_selected=selected_missing_files,
             )
         )
 
     assert resp.status_code == 302
 
-    missing_file = (
-        Path(journalist_app.storage.storage_path)
-        .joinpath(test_source["filesystem_id"])
-        .joinpath(selected_missing_file[0])
-        .as_posix()
-    )
+    expected_calls = []
+    for file in selected_missing_files:
+        missing_file = (
+            Path(journalist_app.storage.storage_path)
+            .joinpath(test_source["filesystem_id"])
+            .joinpath(file)
+            .as_posix()
+        )
+        expected_calls.append(call("File {} not found".format(missing_file)))
 
-    mocked_error_logger.assert_called_once_with(
-        "File {} could not be found.".format(missing_file)
-    )
+    mocked_error_logger.assert_has_calls(expected_calls)
 
 
 def test_download_single_submission_missing_file(
-    journalist_app, test_journo, test_source, mocker, selected_missing_file
+    journalist_app, test_journo, test_source, mocker, selected_missing_files
 ):
-    """Tests download of single submissions with missing file in storage."""
+    """Tests download of single submissions with missing files in storage."""
     mocked_error_logger = mocker.patch('journalist.app.logger.error')
     journo = Journalist.query.get(test_journo["id"])
-    missing_file = selected_missing_file[0]
+    missing_file = selected_missing_files[0]
 
     with journalist_app.test_client() as app:
         _login_user(
@@ -2372,7 +2375,7 @@ def test_download_single_submission_missing_file(
     )
 
     mocked_error_logger.assert_called_once_with(
-        "File {} could not be found.".format(missing_file)
+        "File {} not found".format(missing_file)
     )