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

Fix misplaced static() #7

Merged
merged 3 commits into from
Jan 27, 2019
Merged

Fix misplaced static() #7

merged 3 commits into from
Jan 27, 2019

Conversation

mbourqui
Copy link
Contributor

@mbourqui mbourqui commented Jul 6, 2018

The static() method is called at the wrong place, needs to have the full page to the file or it will crash.
Learnt it the hard way...

* static() alters the file name if it contains special chars
@coveralls
Copy link

coveralls commented Jul 6, 2018

Coverage Status

Coverage remained the same at 98.198% when pulling d3e75fe on mbourqui:fix/static into 2a8bd08 on zostera:master.

@jieter
Copy link
Member

jieter commented Aug 31, 2018

@mbourqui Thanks for the pull.

Can you explain in what circumstances this crash occurs and what the stack trace is?

@mbourqui
Copy link
Contributor Author

mbourqui commented Sep 4, 2018

Hi @jieter,

Thanks for reaching back. It's been a while, but as far as I remember, the issue was that static() will simply prepend the static root path to the path provided, thus it will be incorrect. And on production the files will not be found, as the generated static path does not match the actual path of the file in the file system.

@jieter
Copy link
Member

jieter commented Sep 4, 2018

Since we don't use this feature ourselves, it is critical that will get covered by a test which checks if it shows the desires behavior. It shouldn't be too hard to add such a test, are you maybe able to do that?

@mbourqui
Copy link
Contributor Author

mbourqui commented Sep 4, 2018

There are actually already some tests here: https://github.com/zostera/django-icons/blob/master/tests/test_image.py
Or would you like some more tests specific to the get_path() function?

@jieter
Copy link
Member

jieter commented Sep 4, 2018

It's always nice to have a test exposing the crash along with the fix. Can we mimic the production situation in the tests, for example by using override_settings

@mbourqui
Copy link
Contributor Author

mbourqui commented Sep 4, 2018

I will update the tests, but I need some more time.

@jieter
Copy link
Member

jieter commented Sep 4, 2018

Great!

@mbourqui
Copy link
Contributor Author

Took me some time, but finally here are some more specific tests!

Copy link
Member

@jieter jieter left a comment

Choose a reason for hiding this comment

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

Looks good to me, @dyve?

@dyve
Copy link
Member

dyve commented Jan 27, 2019

Nice! Thanks @mbourqui!

@dyve dyve merged commit 0c58d58 into zostera:master Jan 27, 2019
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.

None yet

4 participants