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

Rename files #250

Merged
merged 1 commit into from
Feb 26, 2019
Merged

Rename files #250

merged 1 commit into from
Feb 26, 2019

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Feb 20, 2019

Description

Fixes #238

What changed?

securedrop_client/message_sync.py

  • I think we log too often here. Since it is a debug log, I left 2 of the 4 logs inside the 5-second loop

securedrop_client/storage.py

  • I made some opinions about log levels here
  • New rename_file function
  • update_submissions and update_replies now update data files to match new filenames in the db.

tests/conftest.py

- I added a data_dir pytest function. I plan to make a follow-up PR for a couple renaming refactors that would have made this PR harder to follow had I added them here. But basically I think we need to pass data_dir where we pass homedir in many test instances. Not a huge deal but it would be more correct to mimic what we do in production.

tests/test_storage.py

  • Added some unit tests for the new rename_file function
  • Updated existing tests: test_update_submissions and test_update_replies to check that rename_file is called if the local storage filenames need to be updated

Update:

  • update_submissions and update_replies now update data files to match new filenames in the db.

The reply and submission files are renamed in these functions when we have both the local filename and server filename. This makes it easy to ensure local matches what's on the server.

Another option would be to rename the files in update_sources when the journalist_designation has changed. This would show the connection between a new journalist_designation and filename updates, but the code would be a bit more complicated because the filenames would need to be rebuilt, hard-coding the file naming structure so that we can find and replace the journalist_designation in the middle of the filename.

I went with the first option because it would require less maintenance if in the future we decide to change the way files are named or there ends up being more reasons that file names can become out of sync. This logic says "regardless of reason the names have changed, local should always match the server."

@sssoleileraaa sssoleileraaa force-pushed the rename-files branch 2 times, most recently from cfa4e34 to d710407 Compare February 22, 2019 07:40
@sssoleileraaa
Copy link
Contributor Author

Added additional test, signed and fixited commits.

Copy link
Contributor

@heartsucker heartsucker left a comment

Choose a reason for hiding this comment

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

One nit for code style, otherwise good to merge.

tests/test_storage.py Outdated Show resolved Hide resolved
tests/test_storage.py Show resolved Hide resolved
tests/test_storage.py Show resolved Hide resolved
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.

2 participants