-
Notifications
You must be signed in to change notification settings - Fork 42
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 (re-)generate and print conversation transcript #1621
Conversation
7bb2cc2
to
e714803
Compare
Good for a first look @cfm! 👁️ I'll add a test plan. Edit: I added the test plan. You can do without a workstation if you don't have one handy, I linked a patch to make that easier. The patch is commented and should be fairly understandable I think, if not, I can walk you through it 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gonzalo-bulnes, I've done a first pass as we discussed last week, focusing on:
- code review, in which I left you a few questions about documentation; and
- printerless testing via
tmp-print-conversation-transcript
, which checks out as expected.
Looking forward to pairing with you on refinement as you see fit this week!
Oh, wow. Something's up with Bookworm. (Some package hashes are not matching anymore.) |
Over to you @cfm! 🏖️ (I'm ignoring the Bookworm failures because they seem unrelated to the topic at hand, I suspect that builds are failing across the board.) |
Thanks, @gonzalo-bulnes! Let's check in tomorrow about what's remaining here for implementation, testing, and review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Penultimate review after discussing with @gonzalo-bulnes:
- I've confirmed that my printer is not automatically usable via
sd-devices
, so I've run through the printerless test plan again. Looks good! - I've done another line-by-line review and requested some changes, all extremely minor nits for clarity.
After your final edits and squash, @gonzalo-bulnes, I'll plan to merge this tomorrow, along with #1623 if the (otherwise irrelevant) *-bookworm
CI failures turn out to be blocking after all.
securedrop_client/gui/conversation/export/print_transcript_dialog.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Cory Francis Myers <cory@freedom.press>
3ac1de0
to
aea7579
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All comments addressed; test plan checks out. Thanks both for this feature and for the back-and-forth, @gonzalo-bulnes!
Description
The shortest path to providing an option to print a conversation transcript to journalists.
Fixes #1435
Context
We're explicitly making the decision to favor speed of delivery for this and next few features in the spirit of making the most of the pilot participant's feedback. (We: @zenmonkeykstop, @cfm, @gonzalo-bulnes)
General availability considerations are out of the table for the time being, and accordingly, I'll be pausing the ongoing work to improve code structure and implementation patterns (namely: testing patterns, usage of native Qt features, etc).
Once this and related work is released, we'll pause and re-assess, whether:
Closes #1615
Closes #1584
Implementation
Test plan
If you've got a printer, print a conversation transcript!
conversation.txt
If you haven't got a printer, you can fake a printer's presence by applying this patch, then do the following after pressing the
"Print""Continue" button:Preview of the log once the patch is applied
metadata,json
file is:{device: "printer"}
conversation.txt
file is presentconversation.txt
file is indeed the file transcript