Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add action to print conversation #1584

Closed
wants to merge 19 commits into from
Closed

Conversation

gonzalo-bulnes
Copy link
Contributor

@gonzalo-bulnes gonzalo-bulnes commented Oct 25, 2022

Goals

  • Ensure that a conversation snapshot is created or updated when the "Print Conversation" action is triggered
  • Ensure that the action is disabled while the operation is in progress.
  • Ensure that the conversation snapshot is sent to the printer when the "Print Conversation" action is triggered

Fixes #1435

Description

Overview

One action (QAction) called "Print conversation" generates a conversation transcript and enqueues it for printing. It displays a confirmation dialog, which is responsible for keeping the user updated on the printing queue readiness, until the user intent to print is captured. All "Print conversation" actions remains disabled until the printing job is has been enqueued, or the operation failed. If relevant, the action displays an error dialog to inform the user that the printing job wasn't successfully enqueued. The only option at that point for the user is to acknowledge the failure and try again.

Implementation and technical vision

Considerations

The printing queue lives in a different qube (sd-devices) than the application (sd-app). The application qube can initiate communication with sd-devices, but sd-devices does only respond with a restricted set of text strings by design. (Because USB devices are attached to it sd-devices is less trusted than sd-app (which contains the most sensitive information), so no arbitrary content must be written to sd-app from sd-devices.) As a consequence, we've decided that the application code would poll the status of the printer.

A CLI interface is available to send commands from sd-app to sd-devices. Currently, only one printer can be connected to sd-devices at any single time. As a consequence, there is no point in querying/polling the status of the printer from more than one place in the application code. Any response from sd-devices will be true for the entire application (e.g. no matter which conversation the user intends to print). Care must then be taken to ensure that we don't overload the CLI unnecessarily.

Technically, printing and writing to a (removable, USB) disk is handled by the same code in sd-devices. Both operations are usually referred to as "exporting" data from sd-app.

Export service

  • Even though the application technically interacts with a printing queue in sd-devices, for all practical purposes it might as well be a printer from the point of view of the application. Where that leads to simpler naming, the term is used in application code.
  • The export.Service now exposes a printer API. Later on, it will expose a disk API as well. Everything other than those APIs must be private by that point.
  • The printer API consists of two interfaces:
    1. An interface that broadcasts the status of the printing queue:
      • a Printer.Status type (enum)
      • a signal: printer_status_changed
      • a property: printer_status that returns a Printer.Status value
    2. An interface that allows to enqueue files to be printed:
    • a method to register a command signal: enqueue_printing_job_on(signal)
    • three signals that broadcast the printing job outcome:
      • printing_job_succeeded
      • printing_job_failed
      • printing_job_finished (either succeeded or failed)
  • The export.Service still exposes the legacy USB disk API as of this PR.

Action

  • User intent is captured by a QAction instance, because that's the right thing to do by the framework.
  • All dialogs associated with the action are technically optional. The action emits all necessary commands and queries to capture and fulfill the primary user intent. Dialogs enhance the user experience, but are not required to achieve the goal of printing a conversation.
  • All dialogs provide additional user intent to the action via the standard dialog API (accepted, rejected, finished signals).
  • The action is disabled when user intent cannot be captured. Because that's the right thing to do by the framework (aka pretty much any current desktop paradigm). E.g. while printing a conversation, we don't currently capture intent to print other conversations.

Confirmation dialog

  • The confirmation dialog displays detailed information about the conversation to be printed.
  • Additionally, it provides dynamic information about the printer status (printing queue status, here is a case where calling it a printer just makes more sense). It does so by consuming the printer status interface of the export.Service. It does not interact in any way with the job queue interface (commands must be emitted by an action, not a presentational component like a dialog).
  • User intent is captured by buttons (QAbstractButton), because that's the right thing to do by the framework (aka any modern accessibility API).
  • When the printing job cannot be enqueued, the option is not offered. (Don't lie to our users.) In practice, that means buttons are disabled.

Error dialog

  • The error dialog is a trivial informational dialog.

Test plan

TK

@gonzalo-bulnes
Copy link
Contributor Author

gonzalo-bulnes commented Oct 26, 2022

@cfm I've drafted adding the "Print Conversation" action to the conversation menu, but we'll still have plenty to work on together tomorrow!

  • the confirmation dialog that I used is not actually the print dialog, it's a simpler one that I used as a placeholder because:

    • the print dialog triggers several actions operations itself...
    • ...and I am trying to adopt the pattern of having the action trigger operations, and the confirmation dialogs follow the accepted-rejected QDialog API.
  • The action is not currently enabled / disabled dynamically (which I think would be nice to do through the app_state, open to discussion!).

  • We should probably add a functional test to cover the statement I commented with # pragma: nocover


    Note: this PR's branch name doesn't match what we're doing, I'd say too bad 🤷‍♀️, but we can close and re-open it tomorrow if that bugs you.

@gonzalo-bulnes gonzalo-bulnes force-pushed the add-conversation-download branch 2 times, most recently from b098919 to 0cb323e Compare October 31, 2022 23:04
@gonzalo-bulnes gonzalo-bulnes changed the base branch from main to add-conversation-digest October 31, 2022 23:26
@gonzalo-bulnes
Copy link
Contributor Author

gonzalo-bulnes commented Nov 2, 2022

🍐 From offline conversation:

@cfm and I considered the following options to deal with the print dialog:

  • Acknowledging that the current implementation is a multi-step dialog (a.k.a a wizard), and that styling QWizard is not straightforward:
    • 🟡 Implement a print wizard with basic styling, and live with it until new notice
      • pro: that's the current plan for the export wizard
      • pro: it's a simpler wizard because there are less steps / events to deal with
      • con: it is a significant amount of work (creating a printing device representation, a way for it to poll sd-devices, creating the wizard itself and connecting the lot)
    • 🟥 Spending time implementing a QWizard that supports stylesheets
      • con: not trivial at all
    • 🟥 Sub-classing QDialog and implementing the parts of the QWizard API that we care about (pages, page history, page completion signals)
      • con: less daunting, but still a significant amount of work
    • 🟡 Attempt creating a dynamic QDialog like the current implementation, but approaching it more systematically to avoid the most obvious quirks of the current implementation
      • pro: probably quicker
      • con: doesn't go at all towards the vision of having QActions triggering operations and dialogs being as simple as possible (up to only implementing the accepted/rejected QDialog API).
  • Taking advantage that the current dialog is specifically a two-step dialog (so a limited version of a multi-step dialog), and that printing time errors related to printer availability are not currently being handled after an initial printer presence check:
    • 💚 Remove the need for a multi-step dialog by creating a single-step ("Confirm that you want to print"), three-state dialog ("Waiting for printer...", "Printer ready", "Printer missing") with an accepted/rejected API, triggering all operations from the QAction, and handling printer availability errors via an independent error dialog form the QAction, after the confirmation dialog has been closed.
      • pro: the error dialog is trivial, the confirmation dialog isn't very complex (it is dynamic, but doesn't trigger any operation itself, so it's simpler than the other options)
      • pro: goes decidedly towards triggering operations from the QAction and having them decoupled from the dialogs
      • con: the UX doesn't have the "hand holding" feeling that a wizard has. Namely, the confirmation dialog is closed after you click "Print", and any subsequent error open a distinct dialog that tells you what happened, to which printing job and to try again. The wizard allows to keep the ongoing printing job in context until completion... (arguably for better or for worse 🤷‍♀️ ).

We decided to go with the last option (:green_heart:) after seriously considering two of the previous ones (:yellow_circle:). I'll have a go at implementing it to see what it would look like and de-risk it a bit before reaching out to @tina-ux.

Comment on lines +69 to +71
status = "<i>Waiting for printer status to be known...</i>"
self.body.setText("<br /><br />".join([self._body, status]))
self.adjustSize()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it wants to call a self.set_status() method like:

class ConfirmationDialog(ModalDialog):
    # [...]

    def set_status(status: str) -> None:
        # TODO: handle case of *removing* the status
        self.body.setText("<br /><br />").join([self._body, status]))
        self.adjustSize()

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. There are a few things wrong with this "append a string" approach. E.g. self.adjustSize() doesn't have the desired effect (making the last line visible). So there are a few things to revisit.

Intuitively, I think that showing and hiding widgets in a layout will be easier conceprually and probably solve the visibility issues without need for explicit handling. To be confirmed.

No matter how we do that auxiliary method to show/hise/select the adequate widget/copy may well make sense indeed 👍

I'm planning to look into that when writing tests for the dialogs.

@cfm cfm force-pushed the add-conversation-digest branch from d1e8266 to 107f685 Compare November 4, 2022 00:08
@gonzalo-bulnes
Copy link
Contributor Author

Bringing this note to the foreground:

Note: if multiple objects (printers or otherwise) were to be watching the print queue, precautions would need to be taken in order to ensure only one schedule is built. Otherwise we'd be overloading the export service unnecessarily. In other words: this implementation assumes that there is at most one Printer instance at any given time. (original comment)

Copy link
Contributor Author

@gonzalo-bulnes gonzalo-bulnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've verified that the dialog sequence is correct by looking at the sd-log output. 🍏

# sd-log

tail -f ~/QubesIncomingLogs/sd-devices/syslog.log | grep securedrop_export

There is still cleanup to do, but I think we're getting there.

Major insight: The connection between the Printer and the export.Service is asynchronous which allows the GUI to remain responsive, so we want to keep that. But that means that nothing prevents, technically, several conversations to be printed concurrently. (We also want that, form a journalist perspective, it's a nice, reasonable, and likely expected feature.)

Where that currently grinds to a halt is that there is no way to attribute response signals to any one printing job (e.g. export.Service.print_succeeded). Long story short, IMO, we need that.

Jumping to solution mode for a moment: that could be achieved by allowing an opaque identifier to be (optionally) included with any request to the export.Service, and making sure the service includes such opaque identifier in its response in a way that can be parsed by the entity that made the request and received the response. With that, we unlock printing conversations with a familiar UX. 🎁 (cc: @rocodes @cfm Let's put my solution proposal aside and think about this together when it's the right time?)

securedrop_client/export.py Outdated Show resolved Hide resolved
securedrop_client/gui/actions.py Outdated Show resolved Hide resolved
securedrop_client/gui/actions.py Outdated Show resolved Hide resolved
securedrop_client/gui/conversation/print/printer.py Outdated Show resolved Hide resolved
securedrop_client/gui/conversation/print/printer.py Outdated Show resolved Hide resolved
securedrop_client/gui/widgets.py Outdated Show resolved Hide resolved
securedrop_client/gui/widgets.py Outdated Show resolved Hide resolved
securedrop_client/gui/widgets.py Outdated Show resolved Hide resolved
securedrop_client/gui/widgets.py Outdated Show resolved Hide resolved
securedrop_client/gui/widgets.py Outdated Show resolved Hide resolved
@gonzalo-bulnes
Copy link
Contributor Author

I've rebased the branch, updated the PR descriptiom, and made main the base branch for the PR. Scope and implementation are well defined and de-risked now, I'll proceed to start cleaning the commit log and ensure meaningful test coverage.

The new directory structure will make extension easier.
Besides being the dedicated tool to assert on Qt signal emissions,
QSignalSpy can track when signals are emitted from other signals,
which the mocking strategy misses.

Example:

    some_signal.connect(some_other_signal)
    # verify that some_other_signal is emitted

See https://doc.qt.io/qt-5.15/qsignalspy.html
@gonzalo-bulnes gonzalo-bulnes force-pushed the add-conversation-download branch from f6b8cad to 217d38d Compare December 4, 2022 15:31
By default, Qt picks the a Qt.QueuedConnection automatically
whenever needed.

Explicit queued connections also happen to make testing harder.

See https://doc.qt.io/qt-5/qt.html#ConnectionType-enum
I'll use an expand and contract pattern to refactor
the export.Service tests, and keeping this file distinct
while it is used as a reference will be more convenient
and minimize the change sets.

Important: this file is still fully operational and will be
kept as such until all deprecated methods have been removed
from export.Export (soon to be renamed export.Service).
@gonzalo-bulnes gonzalo-bulnes force-pushed the add-conversation-download branch from 217d38d to be2237f Compare December 5, 2022 13:00
@gonzalo-bulnes
Copy link
Contributor Author

(rebased, cleaned up first half of the branch, forgot signing one commit, added remaining work in progress at the end)

@gonzalo-bulnes gonzalo-bulnes force-pushed the add-conversation-download branch from d3621a7 to 10ce1af Compare December 6, 2022 01:05
Decorate static methods (from what didn't need to be class methods)

Tests were minially modified: patching static methods must be done
unsing unittest.mock.patch.object to preserve the independence of
the unit tests. Assigning to the class attributes directly does
modify the class for the entire test session. (Not desirable.)
I am leaving the deprecated tests as untouched as possible to make
easier to review the changes.
@gonzalo-bulnes gonzalo-bulnes force-pushed the add-conversation-download branch from f6dfc90 to 1874fb6 Compare December 6, 2022 02:25
This will ensure that one one instance of the export service
queries the sd-devices CLI.

That was already the case, but nothing was done to suggest it.

Additionally, this will allow to simplify the export service
injection by making possible to retrieve it like a logger.

Note: I haven't stopped injecting the export_service into
the main.Window because I couldn't figure out how to prevent
what seems to be a fixture scope error that affects only
half of the integration tests. That seems like something that
can be resolved, so I don't consider it to be a sign of
export.Service being a bad idea.
@gonzalo-bulnes gonzalo-bulnes force-pushed the add-conversation-download branch from 1874fb6 to fdca857 Compare December 6, 2022 02:31
@gonzalo-bulnes
Copy link
Contributor Author

Note, this PR will be closed from #1621. Please refer to it for details.

@cfm cfm closed this in #1621 Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support printing conversations
2 participants