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

Fixes #3354 - Handles FileNotFoundError for assets #3355

Merged
merged 4 commits into from
Jun 18, 2020

Conversation

karlcow
Copy link
Member

@karlcow karlcow commented Jun 17, 2020

This PR fixes issue #3354

Proposed PR background

Basically in case the actual files where missing, we got a FileNotFound spiraling up and returning a 500.

This now will help a bit more by failing gracefully. The resource still does not exist but we will be able to see it in the network panel of devtools. The uris will carry a ?missing_file?

@karlcow karlcow requested a review from miketaylr June 17, 2020 06:17
Copy link
Member

@miketaylr miketaylr left a comment

Choose a reason for hiding this comment

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

Thanks, this is a nice simple improvement.

How hard would it be to write some unit tests for this? Give it a fixture file, assert expected checksum, give it a non-existant file, assert missing_file returned?

@karlcow
Copy link
Member Author

karlcow commented Jun 17, 2020

@miketaylr do you remember if there was a reason to put the get_checksum inside? it's slightly weird.

@karlcow
Copy link
Member Author

karlcow commented Jun 17, 2020

This was done a long time ago
133cffe

:)

@miketaylr
Copy link
Member

do you remember if there was a reason to put the get_checksum inside? it's slightly weird.

you might have left out a word. inside _____?

@karlcow
Copy link
Member Author

karlcow commented Jun 17, 2020

@miketaylr do you remember if there was a rationale for doing this nesting instead of two flat functions?

def bust_cache(file_path):
    def get_checksum(file_path):
         …
         return
    return

@karlcow
Copy link
Member Author

karlcow commented Jun 18, 2020

Let's see the test coverage.

coverage report -m webcompat/templates/__init__.py 
Name                              Stmts   Miss  Cover   Missing
---------------------------------------------------------------
webcompat/templates/__init__.py      75     19    75%   32-37, 42, 47-61, 96

We can do better.

Let me complete this PR. Do not merge for now.

@karlcow karlcow marked this pull request as draft June 18, 2020 03:12
@karlcow
Copy link
Member Author

karlcow commented Jun 18, 2020

I have switched to draft to not break anything.

@karlcow
Copy link
Member Author

karlcow commented Jun 18, 2020

(env) ~/code/webcompat.com-karlcow % coverage report -m webcompat/templates/__init__.py 
Name                              Stmts   Miss  Cover   Missing
---------------------------------------------------------------
webcompat/templates/__init__.py      79      1    99%   99

ok better the 1% missing is because we have one case which is not tested because the function doesn't make sense. I will open another issue for it.

@karlcow karlcow marked this pull request as ready for review June 18, 2020 08:11
@karlcow karlcow requested a review from miketaylr June 18, 2020 08:12
@karlcow
Copy link
Member Author

karlcow commented Jun 18, 2020

ok better the 1% missing is because we have one case which is not tested because the function doesn't make sense. I will open another issue for it.

opened in #3356

@miketaylr
Copy link
Member

@miketaylr do you remember if there was a rationale for doing this nesting instead of two flat functions?

Ah, no -- no memory. Nest functions seems nicer and more pythonic.

@miketaylr
Copy link
Member

Thanks @karlcow

@miketaylr miketaylr closed this Jun 18, 2020
@miketaylr miketaylr reopened this Jun 18, 2020
@miketaylr miketaylr merged commit 363a6eb into webcompat:master Jun 18, 2020
@karlcow karlcow changed the title Issue #3354 - Handles FileNotFoundError for assets Fixes #3354 - Handles FileNotFoundError for assets Jun 19, 2020
@karlcow karlcow linked an issue Jun 19, 2020 that may be closed by this pull request
@karlcow karlcow deleted the 3354/1 branch August 24, 2020 04:43
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.

get_checksum should not be able to break the app
2 participants