breaking up SDExport

[redshiftzero]

This is related to #15 and contains some thoughts on how we might restructure the code in this repo. Note that these are just opinions, we should discuss before doing a big refactor.

First for reference here's a UML diagram of the existing code as of Nov 26:

We can notice from the UML diagram that each instance of the SDExport class is only ever going to use a subset of the defined methods, i.e. an USB export wouldn't ever use any print-related methods. It's also quite a large class and is doing a lot of different things so imho this is the best class to break up further. This is perfectly natural: originally this was small - and then we added print - and in the future we'll probably add even more export methods.

Here’s one approach for restructuring: instead of each export involving a single SDExport object, each export would involve two objects:

• an action object, that contains anything related to the specific export action that has been requested (more on that below)
• a submission object, that contains any other data/methods which isn’t specifically related to the action, i.e. extract_tarball() seems like it should be on this. This is what we'd define here immediately when we get the export archive.

For the action, we'd create an abstract base class e.g. ExportAction which defines the interface that any export actions should implement. Even an interface as minimal as ExportAction.run I think would be beneficial for organization purposes. With that our main would look something like:

submission.extract_tarball()

try:
    submission.archive_metadata = export.Metadata(submission.tmpdir)
except Exception:
    submission.exit_gracefully(ExportStatus.ERROR_METADATA_PARSING.value)

if not submission.archive_metadata.is_valid():
    submission.exit_gracefully(ExportStatus.ERROR_ARCHIVE_METADATA.value)

if submission.archive_metadata.export_method == "usb-test":
    action = UsbTestAction()  # do we need to pass in submission for these test actions? 
elif submission.archive_metadata.export_method == "disk":
    action = UsbExportAction(submission)
# removing disk-test here as we could collapse into usb-test h/t to @emkll for confirming this
elif submission.archive_metadata.export_method == "printer":
    action = PrintExportAction(submission)
elif submission.archive_metadata.export_method == "printer-test":
    action = PrintTestPageAction()

action.run()

The classes that implement the ExportAction base class could have the following hierarchy:

They'd keep on their classes anything relevant specifically to that action at hand, e.g. for Print* objects they’ll have the print_all_files() and the setup_printer() methods.

Two other smaller things that would be good to do as part of this is:

1. Make it clear which methods are for a class's internal use and which developers should be calling outside the class - i.e. let's prefix internal methods with _.
2. We make some methods of SDExport utility functions: I’m specifically thinking of both safe_check_call and popup_message which we could make module-level functions.

[redshiftzero]

here's a wip with a sketch of how things could be structured: https://github.com/freedomofpress/securedrop-export/tree/spike-reorg

one difference from the above comment is that USBAction and PrintAction I feel like made more sense to be mixin classes, but ExportAction is an ABC that the PrintTestPageAction, PrintExportAction, USBExportAction, USBTestAction classes implement.

I was chatting with @creviera just now and she raised potentially breaking up the export and check actions further. I think this is a good idea because exports need a submission object (of type SDExport) whereas the check actions don't need one (after the refactor). Right now in the current diff we're passing in submission objects to __init__ even where we don't need them, which I don't love.

UML diagram after:

[sssoleileraaa]

This would be a huge improvement.

A couple thoughts:

try:
submission.archive_metadata = export.Metadata(submission.tmpdir)
except Exception:
submission.exit_gracefully(ExportStatus.ERROR_METADATA_PARSING.value)

if not submission.archive_metadata.is_valid():
submission.exit_gracefully(ExportStatus.ERROR_ARCHIVE_METADATA.value)

I think it would be great if we could combine this check and have just one error ExportStatus.ERROR_ARCHIVE_METADATA.value. For details we would look at the logs.

if submission.archive_metadata.export_method == "usb-test":
action = UsbTestAction() # do we need to pass in submission for these test actions?
elif submission.archive_metadata.export_method == "disk":
action = UsbExportAction(submission)

removing disk-test here as we could collapse into usb-test h/t to @emkll for confirming this

elif submission.archive_metadata.export_method == "printer":
action = PrintExportAction(submission)
elif submission.archive_metadata.export_method == "printer-test":
action = PrintTestPageAction()

This is a nitpick but I think it would be clearer if we went with:

if submission.archive_metadata.export_method == "usb-check":
action = UsbCheckAction() # do we need to pass in submission for these test actions?
elif submission.archive_metadata.export_method == "disk":
action = UsbExportAction(submission)

removing disk-test here as we could collapse into usb-test h/t to @emkll for confirming this

elif submission.archive_metadata.export_method == "printer-check":
action = PrinterCheckAction()
elif submission.archive_metadata.export_method == "printer":
action = PrintExportAction(submission)
elif submission.archive_metadata.export_method == "printer-test":
action = PrintTestPageAction(submission)

Since our plan is to include a printer preflight check the same way we do for transferring files to disk.

---

I will spend more time looking at the concept code in a followup review.