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

Updates the filesize after download and decryption #969

Merged
merged 5 commits into from
Mar 24, 2020

Conversation

kushaldas
Copy link
Contributor

Description

Fixes #917
This still needs unit test case. I need help with that

Test Plan

Follow the steps from #917

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, packaging logic (e.g., the AppArmor profile) may need to be updated. Please check as applicable:

  • I have submitted a separate PR to the packaging repo
  • No update to the packaging logic (e.g., AppArmor profile) is required for these changes
  • I don't know and would appreciate guidance

@redshiftzero
Copy link
Contributor

hey can you elaborate on what you need help on regarding the unit testing?

@kushaldas
Copy link
Contributor Author

hey can you elaborate on what you need help on regarding the unit testing?

I just could not figure out a way to write the test. I I know I can write a functional test, but, not a unit test.

@rmol rmol force-pushed the update_file_size branch 2 times, most recently from a5a335f to 062405e Compare March 24, 2020 18:24
@redshiftzero redshiftzero marked this pull request as ready for review March 24, 2020 19:52
@redshiftzero
Copy link
Contributor

(removing draft status in case I'm the only one that can)

@rmol rmol force-pushed the update_file_size branch from 062405e to 6b30cf3 Compare March 24, 2020 20:44
@@ -2244,6 +2244,9 @@ def __init__(
file_ready_signal.connect(self._on_file_downloaded, type=Qt.QueuedConnection)
file_missing.connect(self._on_file_missing, type=Qt.QueuedConnection)

def update_file_size(self):
self.file_size.setText(humanize_filesize(self.file.size))
Copy link
Contributor

@sssoleileraaa sssoleileraaa Mar 24, 2020

Choose a reason for hiding this comment

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

This could cause a crash if the file was deleted, I believe, so we can either add it to our ticket tracking possible crashers or temporary add a try catch for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually we should do both - add the try-catch and add it to the list

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried adding a test that created the FileWidget, then deleted its File, then called update_file_size and it did not crash. I've added the try/except block and a test with a mock to force an exception, just in case.

@rmol rmol force-pushed the update_file_size branch from a9a493d to b907c09 Compare March 24, 2020 23:08
@sssoleileraaa sssoleileraaa self-requested a review March 24, 2020 23:21
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

followed the steps in #917 and file shows up as 1MB

lgtm!

@sssoleileraaa sssoleileraaa merged commit e4ec38c into master Mar 24, 2020
@sssoleileraaa sssoleileraaa deleted the update_file_size branch March 24, 2020 23:23
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.

Reported file size does not reflect decompression
4 participants