Add safe deletion parity with journalist interface

[rmol]

Description

Fixes #1202.

Permit the deletion of a source's conversation items, without deleting their account.

This does not yet trigger an accelerated sync, because it's useful to be able to start several concurrent deletions while evaluating the work so far, and the existing interval makes that easier.

Test Plan

• Get SDK support for the new conversation deletion endpoint: just update securedrop-sdk in your virtualenv if that PR has been merged, or pip install git+https://github.com/freedomofpress/securedrop-sdk@delete-conversation#egg=securedrop-sdk to install from its branch.
• Run the client as usual. Use the source menu to delete both conversations and source accounts, comparing appearance and flow to the prototypes linked from "Safe Delete" parity with JI #1202.
• One change will require you to delete a single submission via the journalist interface, then wait for a sync before you can check the marker for deletion of earlier submissions.
• Compose a draft reply, but don't send it before deleting the conversation. Your input in the reply text box should survive the deletion and next sync, whereupon you should be able to send it.

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

• I have tested these changes in the appropriate Qubes environment
• I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
• These changes should not need testing in Qubes

If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:

• I have updated the AppArmor profile
• No update to the AppArmor profile is required for these changes
• I don't know and would appreciate guidance

If these changes modify the database schema, you should include a database migration. Please check as applicable:

• I have written a migration and upgraded a test database based on main and confirmed that the migration applies cleanly
• I have written a migration but have not upgraded a test database based on main and would like the reviewer to do so
• I need help writing a database migration
• No database schema changes are needed

[sssoleileraaa]

just a couple quick comments about the tear feature to get started:

1. the tear should be moved to the top when a message is sent as a journalist from the client directly below the tear (the way it is when a message is sent from the journalist interface)
2. the tear needs some padding between it and the top of the conversation view
3. the tear styling, first it looks like:
   then after a reply or message is sent, it looks like:
   
4. the tear message should show up in the preview (copied from zeplin simulation linked to in "Safe Delete" parity with JI #1202)
   

[rmol]

1. I'm not sure what you mean here. After replying to a source whose conversation has been deleted, the tear message changes to the "Earlier files and messages deleted" marker, which appears at the top, with the reply beneath it. It doesn't happen until the next sync, which right now could be a while. Are you saying it should happen immediately?
2. I added padding above the "Earlier files and messages deleted" tear.
3. I'm not seeing that first screenshot's styling locally -- the "Files and messages deleted for this source" tear is centered vertically for me. Was there something else I should be noticing about these two shots?
4. I fixed the preview text when the conversation has been deleted; it should now be adjusted and styled like the prototype.

[sssoleileraaa]

1. I'm not sure what you mean here. After replying to a source whose conversation has been deleted, the tear message changes to the "Earlier files and messages deleted" marker, which appears at the top, with the reply beneath it. It doesn't happen until the next sync, which right now could be a while. Are you saying it should happen immediately?

Oh, I thought I provided an image, which would have helped clarify what I'm talking about. When all files and messages are deleted, the tear is centered as expected, but when you send a reply it shows up below the centered tear (until the next sync) as you see here:

When the client is full-screen it's pushed further to the bottom and looks more unexpected.

Are you saying it should happen immediately?

Yes, any new messages are replies that are added to the conversation view should be aligned to the top immediately, even if there is a tear and it is a pending reply.

[sssoleileraaa]

3. I'm not seeing that first screenshot's styling locally -- the "Files and messages deleted for this source" tear is centered vertically for me. Was there something else I should be noticing about these two shots?

At first, I couldn't repro my screenshot either, but now I have an STR:

1. Delete all files and messages of a source
2. Resize the client window to be smaller and see that the tear and message are no longer centered

So, for this change, I think you just need to make sure the tear remains centered and that font sizing is correct (see https://app.zeplin.io/project/5c807ea562f734bd2756b243/screen/6080e8b72293ab1e30f35d06: should look the same as the "Select a source from the list" font). Otherwise, you implemented this as designed as far as font and tear-styling goes.

Update

It looks like this may be part of a larger, pre-existing issue. Another similar issue with centering the text in the conversation view (when the entire source isn't refreshed): #1233

[sssoleileraaa]

here and in on_delete_source_success is where you would trigger a sync, but you mentioned not wanting to do that yet. For clarification, do you plan to add this behavior in a follow-up PR?

[rmol]

I was going to add that here after we'd otherwise verified the UX, but it could be done separately.

[sssoleileraaa]

Let's add this in a followup PR that anyone can review while I'm afk for break to prioritize getting all the UI changes in before I leave

[rmol]

Yes, any new messages are replies that are added to the conversation view should be aligned to the top immediately, even if there is a tear and it is a pending reply.

Conversation deletion markers are now being updated when a reply is sent.

[rmol]

At first, I couldn't repro my screenshot either, but now I have an STR:

I still can't trigger this. The deletion marker stays centered for me while I resize the window to its minimum dimensions, then back. Maybe it's because I'm using i3, but I am able to repro #1233, so whatever it is, I suspect I've introduced my own new bug here.

[edit] No, it works properly under XFCE for me as well.

[sssoleileraaa]

I still can't trigger this. The deletion marker stays centered for me while I resize the window to its minimum dimensions, then back. Maybe it's because I'm using i3, but I am able to repro #1233, so whatever it is, I suspect I've introduced my own new bug here.

[edit] No, it works properly under XFCE for me as well.

Okay, could you try these steps:

1. Delete all files and messages of a source and then select another source while that operation is still pending
2. Resize the client window to be smaller
3. After the deletion operation finishes, select the original source again, and see that the tear and message are no longer centered

[rmol]

Okay, could you try these steps:

Good news! This morning, everything was borked. When I started the client, it seemed to be honoring QT_AUTO_SCREEN_SCALE_FACTOR=1 in my environment, and the UI was scaled up, where yesterday (and most of the time lately) it had not been. When I deleted a conversation, the deletion marker text was smaller, and positioned at the top of the conversation area, as you described. I tried in both i3 and XFCE, so we can rule out any weird interaction with the window manager. Unfortunately, after a reboot, I could no longer induce this.

I was still seeing the marker positioned inconsistently, though, just not at the top of the space, and I think it was because the ConversationView's scroll was still shown, though empty, so taking up space in the layout. I think I was just unlucky earlier and checking conversations that had the same amount of content, so weren't changing the marker position. At any rate, hiding the scroll and adding a stretch has the marker being consistently centered for me now.

So if you could pull and try again, the positioning should be fixed, but I'm interested to see if you're still seeing the smaller text.

Finally, are you testing on Qubes or Debian? I'm guessing Debian, from the window manager styling. I may have to set up there and check all this.

[sssoleileraaa]

Good news! This morning, everything was borked.
😂

I tried in both i3 and XFCE, so we can rule out any weird interaction with the window manager. Unfortunately, after a reboot, I could no longer induce this.

To save time, you can just test XFCE (we have not been testing or working towards supporting i3, but if you feel we should add that to our list of environments to test against, let's discuss in the near future).

Interesting that a reboot would fix it. I wonder just restarting the client will also sometimes fix this. I'll do more testing today...

At any rate, hiding the scroll and adding a stretch has the marker being consistently centered for me now.

Great!

So if you could pull and try again, the positioning should be fixed, but I'm interested to see if you're still seeing the smaller text.

Just gave it a spin and now I see the tear marker and text is center-aligned 🎉 The font size should be larger, but it sounds like you're facing an issue with the text not scaling correctly? Just so you know, the font size for "Files and messages deleted for this source" looks the same as it did before your most recent changes.

Finally, are you testing on Qubes or Debian? I'm guessing Debian, from the window manager styling. I may have to set up there and check all this.

I've been testing in a prod-like environment: on Qubes on both a dev vm and the prod sd-app vm using the default desktop environment (Xfce).

---

It looks like the only visual items left are:

• fixing the font size as described in a review comment and here in this comment
• vertically centering the deletion popups where the user can cancel or move forward with deletion
• adding space between the left border and the message "Deleting files and messages..." (similar to how you have it for "Deleting source account..."
• after fixing the three bullets above, perhaps a demo/ visual ux review is in store? it looks really good and would be fun to show off too!

Also, I have a slight preference for you to open a new PR when you introduce triggering syncs if that works for you.

[rmol]

This is how it all looks for me right now. This is under XFCE, in sd-dev. I can try in sd-app tomorrow.

vertically centering the deletion popups

font size, space between the left border and the message "Deleting files and messages..."

font size for "Files and messages deleted for this source"

Am I missing something from the prototypes, or do these things look different to you?

[sssoleileraaa]

• space between the left border and the message "Deleting files and messages..." fixed!
• "Files and messages deleted for this source" font still too small
  • should match the font size for other messages we display in the conversation pane, e.g. when the user needs to select a source, see sdclient.css:

#EmptyConversationView QLabel {
    font-family: Montserrat;
    font-weight: 400;
    font-size: 35px;
    color: #a5b3e9;
}

• right now it looks like this by default:
  

• yes, I am basically seeing the same thing but a little worse than: https://user-images.githubusercontent.com/47430598/124031556-759de980-d9c5-11eb-95d5-04de74c03241.png The issue is that there needs to be padding around the text and the space between the text and buttons should be reduced. Also the mockups show different button positioning and color, see
  
  

[rmol]

I've added padding to ModalDialog, changed the color on the conversation deletion dialog, and corrected the omitted units on the font size specification for the "Files and messages deleted" text. I went with the 30px used in the prototype instead of the 35px used for the empty conversation view placeholder text because 35px caused the text to extend quite a bit left and right of the tear image.

I don't think we should special-case the button positioning for the deletion dialogs to match the prototypes. If we want them centered as in the prototype, we should make the change consistently across export and print dialogs too. Similarly, the space between the dialog body and button bar is a stretch, used across all subclasses of ModalDialog (and creating about the same amount of space in them on my system -- e.g. the export dialog has a similar gap between body text and buttons), so if we want to change that, I think we should make it consistent across all our dialogs.

[sssoleileraaa]

I don't think we should special-case the button positioning for the deletion dialogs to match the prototypes. Similarly, the space between the dialog body and button bar is a stretch, used across all subclasses of ModalDialog (and creating about the same amount of space in them on my system -- e.g. the export dialog has a similar gap between body text and buttons), so if we want to change that, I think we should make it consistent across all our dialogs.

Makes sense! Let's address this outside of this PR

• "Files and messages deleted for this source" font still too small

You fixed this 🎉 but looks like the text should be wrapped as the prototype shows. Right now it looks like:

But Zeplin shows:

[rmol]

Added a newline.

[sssoleileraaa]

Looks like adding a newline doesn't quite get us there. If you don't want to do this level of polishing work, please create followup issue(s) for the remainder of work, including what you've already called out (e.g. changing ModalDialog to fit modals to their contents). It seems like this is getting closer to a code review, so you have plenty of time to compare to the prototype and make more fixes or create followup issues.

[rmol]

Looks like adding a newline doesn't quite get us there.

It wrapped the text at the right place, addressing your last comment:

I don't mind doing the polishing work, but "doesn't quite get us there" isn't much to go on. What else do you want here?

[sssoleileraaa]

@rmol i think now we're getting to the nitty-gritty of the visual review, so it would be easier just to meet in person... switching to a DM to share a meeting invite

Update: "doesn't quite get us there" basically means that it doesn't look just like the prototype, e.g. the padding is incorrect. maybe you just need one more newline or you could update the padding, but it feels inefficient for us to continue with making a one-line change, getting feedback on that one-line change, making another small change, getting more feedback... there are many things that stand out to me when comparing the prototype, so i'd like to point them out to you in person (meeting coming soon!). i find it helpful to have the prototype open while working on the UI to do a side by side comparison, and then the differences stand out quite a bit. we can discuss technique in person too. i think it's also fine to create followup issues.

[eloquence]

These are the notes from our design review today (@rmol @ninavizz @creviera @eloquence participated):

• Overall this looks amazing :-)
• UI dialog language is not exactly the same as in Invision prototype. I'd suggest bringing it in line with the prototype for now, and then we can have a follow-up discussions about improvements to that language.
• The "Are you sure" on account deletion should be aligned with the buttons, and in urgent coral, as in this screen: https://projects.invisionapp.com/share/6210SXNFEVWJ#/screens/450262347
• The "Cancel" button should be highlighted as the primary action on the same dialog
• Bullets should not be indented, and should be styled as middots for now (this may warrant further discussion as we standardize styles)
• The padding in the deletion menu doesn't quite match up with the prototype (issues: horizontal padding a bit too large, no padding next to highlight, vertical padding from "Delete" header too small)
• The "Deleting ..." progress widget during a deletion operation did not appear to be quite centered in the conversation area.

[rmol]

This is ready for another look. I've addressed all the shortcomings from last week's review.

[sssoleileraaa]

All review comments have been addressed, including the most recent requests for changes from the ux review around button alignment, primary default buttons, unindented bullets, and styling bullets to be small. This just needs CI passing. I'll leave code comments in the meantime, but as far as the UI is concerned I think it's where it needs to be.

[sssoleileraaa]

lgtm: all review comments have been addressed so this can be squashed and merged

[sssoleileraaa]

ah, my bad, i only tested the happy path for export and print so it looked like this worked without the keyPressEvent handler, but it looks like tabbing to a non-default button and pressing Return or Enter no longer works. Going to disregard my review and debug further.

need to do more testing around key presses for buttons

[sssoleileraaa]

I pushed a fix proposal for this last issue that came up, which can be dismissed if you want to take a different approach. In any case, it would be great to fixup some these commits. You can either wait for my return to finish review (if you dismiss my change) or someone else can pick up review for the tail end of this PR. So far, all my review comments have been addressed, so if someone picks up review, they should review anything new and do basic client testing around modals and deletion.

[rmol]

Setting focus does the trick. I've rebased the earlier work down to one commit. This is ready for a final review.

[sssoleileraaa]

lgtm!

after all the back and forth, the final review was a breeze; no more issues found other than an issue that i'm still work on an STR for (sometimes the preview snippet doesn't show the deletion animation, see "subcortical footbridge" preview snippet in the following image)

I'll create a follow up issue for this once i'm able to create a reliable STR