-
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 print integration #631
Conversation
d519846
to
d77f3f2
Compare
Resolves #632 |
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.
Looks good @creviera !
I've managed to successfully print a few submissions 🎉
I've tested with pdf and {ms, libre}office documents, as follows:
- Built securedrop-client on this branch, install in svs
- Fix bug in securedrop-export script logging (see 1 below)
- Switch back and forth between source panes (see 2 below)
Then, proceeded with your test plan, as follows:
- ✔️ Test ERROR_PRINTER_NOT_FOUND: If no printer is connected, I get the expected message
- 〰️ Test ERROR_PRINTER_NOT_SUPPORTED: I plugged in an HP printer (currently unsupported), I get the ERROR_PRINTER_NOT_FOUND message: this is because the printer is not exposed over USB but using this
direct hp
instead ofusb <printer_url>
. This is a very old HP inkjet printer so this might be the reason why. - ✔️ Test ERROR_PRINTER_DRIVER_UNAVAILABLE: I moved the file and see the expected error
- ❌ Test ERROR_PRINT: Did not test, could you please clarify the steps
-
There was a bug in the securedrop-export script, that was easily resolved (see https://github.com/freedomofpress/securedrop-export/compare/fix-print-for-477?expand=1). Have you seen this failure locally?
-
On first download, the print button is not visible until one changes sources (and comes back to the source), this is probably resolved by showing the button on this line : https://github.com/freedomofpress/securedrop-client/pull/631/files#diff-16a175b4662541f3889b432ac9e7e0e8L1864
Once 2 is fixed (and I test the corrupt print) I am satisfied with the functionality as implemented, and good to merge from my perspective (though @redshiftzero said they wanted to review this one as well).
Yes, I did have that fix in my export branch, maybe it didn't get pushed (will push that change and open an export pr against the branch I linked to above so we can start tracking changes.
Good find! I thought I fixed this.
I will clarify after fixing #2 and opening the export PR Thanks for the review so far! |
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.
this is looking good to me, minor comments inline (though i can't functionally test atm due to no printer)
1ce0767
to
0b8d712
Compare
pr comments addressed with regression test for showing print and export links immediately after file download and tests around refactored code to create and cleanup hardlinks when opening, exporting, and printing files (no cleanup yet for opening with the way it was designed -- a ticket should already exist for this). to clarify how you would test the scenario where we would get back |
2ff1f5d
to
625c736
Compare
Thanks @creviera, changes look good! What changed from last week's review is that I am using Buster templates, and I've observed some errors with the printer tooling, and I have pushed a commit to the associated securedrop-export branch here: freedomofpress/securedrop-export#34 With those changes, everything works as expected 🎉 One last comment from me: as I was debugging the Buster compatibility this morning, I observed some strange Client behavior: a segfault occurred when I (manually) introduced an error in the If it's an easy fix, let's include it as part of this PR, otherwise we can open a follow-up to track to see if others can reliably reproduce.
|
So at the time I wrote this, we were building and testing for stretch. But now that securedrop-workstation has upgraded to buster, I need to run through testing this again. |
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.
Just tested this again with the export script changes in freedomofpress/securedrop-export#34 , and everything works! Thanks for your hard work on this one @creviera
Opened #641 to track the issue I encountered but could not reproduce.
Description
Resolves #477
Resolves #632
Test Plan
If you have an HP LaserJet printer like me, then update the export script with this change: https://github.com/freedomofpress/securedrop-export/tree/laserjet-printers.
Test
ERROR_PRINTER_NOT_FOUND
:Test
ERROR_PRINTER_NOT_SUPPORTED
Test
ERROR_PRINTER_DRIVER_UNAVAILABLE
Test
ERROR_PRINT
unoconv -o converted_path /tmp/tmp123/export_data/hello.doc.pdf /tmp/tmp123/export_data/hello.doc
failsChecklist