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

File download UI / API plumbing #64

Merged
merged 4 commits into from
Oct 29, 2018
Merged

Conversation

ntoll
Copy link
Contributor

@ntoll ntoll commented Oct 24, 2018

Very much a WiP PR for file download.

@redshiftzero
Copy link
Contributor

Behaviour for decryption needs to be decided for further progress in when handling downloaded files.

so for completion of the current ticket, we can just save the files encrypted locally, and then I just filed #66 for adding the decryption step (has a qubes dependency)

We need to agree user feedback in the UI for status indication or for when things go wrong (timeouts / API errors etc...). I've raised a ticket for this #63.

I merged the status footer PR so we can liberally put errors for the user there for now

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

looks great, i think we just need the status bar messages adding and the saving of the downloaded encrypted file to the right spot in the data dir and then i think we can merge!

Called when a file has downloaded. Cause a refresh to the conversation
view to display the contents of the new file.
"""
if result: # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

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

where result will be added to args for on_file_download

@@ -40,6 +40,7 @@

def init(sdc_home: str) -> None:
safe_mkdir(sdc_home)
safe_mkdir(sdc_home, 'data')
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

def on_file_download(self):
"""
Called when a file has downloaded. Cause a refresh to the conversation
view to display the contents of the new file.
Copy link
Contributor

Choose a reason for hiding this comment

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

though we will just indicate that the file is downloaded instead of opening (the actual opening logic will be dealt with in #20), i.e. "download" before DL and "view" after DL (following the language in the relevant snippets from wireframe):

screen shot 2018-10-24 at 6 30 06 pm

screen shot 2018-10-24 at 6 30 16 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right... so I just want to be absolutely certain I grok this:

  • There are two sorts of files that can be downloaded and then decrypted: 1) Plaintext files containing a message (from now on "messages"). 2) Attachments files (such a images or PDF docs -- from now on "attachments").
  • If a file isn't downloaded we just show that it's ready to be downloaded and encrypted. I'm assuming there's a way to distinguish between messages and attachments due to filename naming conventions, right?
  • Once a file (no matter if it is a message or attachment) is downloaded, then this function we will display the contents of the downloaded and decrypted messages as the speech bubbles and the downloaded attachment as a file widget in a viewable/exportable state.
  • All pending-download/encryption messages/attachments will just look like the "download" image in your comment.

Sorry for the painful description of detail, but it's important to get this written out somewhere. ;-)

@ninavizz
Copy link
Member

Hey guys—I've had a lot of issues with my prototyping software being buggy, and have been heads-down preparing for user testing... but will upload a relevant time-based mini-prototype capture demonstrating messaging/etc for this, EOD today (90% of which will be for Beta target—whatevs can get built-in for Alpha, we can discuss next week). Thx for all the hard work making this functionality awesome!

@redshiftzero redshiftzero mentioned this pull request Oct 25, 2018
@redshiftzero
Copy link
Contributor

redshiftzero commented Oct 26, 2018

hey - so I made some good progress this afternoon adding more logic for this ticket in populate-conversation-view (not pushing onto the branch in this PR until 100% done). the two things that remain are:

  • right now if we're not in Qubes, then downloaded files get saved in the SD data dir without issue. but in Qubes, the SDK will save these files in a QubesIncoming directory (not in the SD data dir), so we'll need to move them into the right place
  • adding a smart way to refresh the source conversation view after a successful download. this should not assume the user is still on the same source conversation page as when they began the download (downloads can be long, users may become bored and click around to read other messages)

anywho I will work on this later/tomorrow

@redshiftzero
Copy link
Contributor

hey I finished off the logic for this so pushed here - for @joshuathayer or @ntoll to review (whoever gets to this first 😇 ). I can't formally request your review @ntoll due to the fact you started this PR but I'm going to 👍 these changes to indicate and you should feel free to block if you think changes should be made, or merge if you approve.

The way I've been testing this is:

  1. Login to source interface, submit a couple of files
  2. Login to client
  3. The files should now appear in the conversation view for my source
  4. I double click a file to download, the conversation view should refresh indicating that i can "Open" the file (we can't yet, that's another ticket, but you get the idea):

screen shot 2018-10-26 at 9 14 03 am

  1. The file should be downloaded and stored in the data directory

@redshiftzero
Copy link
Contributor

oh btw, if one is reviewing from Qubes, you'll need to set proxy=True in the API creation

@ntoll
Copy link
Contributor Author

ntoll commented Oct 29, 2018

Oh man... I should check my email before making changes to my local branch. TL;DR I did some minor revisions to my original branch in my local repository, only to get errors when pushing to realise you've covered this stuff already. Just tidying ensuing merge-ageddon and sticking with your changes. Review incoming....

@ntoll
Copy link
Contributor Author

ntoll commented Oct 29, 2018

OK... I like what you did with the API call (current_object). Since signals require a specified type to emit it's not a great way to emit the result of an API call (which could be anything). I'd like to discuss / chat about refactoring this stuff now we have several different API calls happening. This should be done in another ticket (I've created #80 to track our decision making on this).

Everything else makes perfect sense and LGTM. I'll merge fast and break things ™️ ;-)

@ntoll ntoll merged commit f25cf5a into freedomofpress:master Oct 29, 2018
legoktm pushed a commit that referenced this pull request Dec 11, 2023
legoktm pushed a commit that referenced this pull request Dec 11, 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.

3 participants