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

(dev only) allow file download directly from django #327

Merged
merged 2 commits into from
Feb 12, 2023

Conversation

kuhnchris
Copy link
Contributor

This is a development change only - no changes on prod!
... also requires setting PROVIDE_FILES_DIRECTLY = True in local_settings.py .

With this change, the entire package can be self-contained to test and develop without using a docker or devcontainers container.

@meeb
Copy link
Owner

meeb commented Feb 12, 2023

You could probably just use settings.DEBUG along with a check for the Django development server for this rather than introduce another toggle that can be set separately from DEBUG. Given this feature is only going to be used in situations where the Django dev server is being used and also not behind an upstream nginx server you could do something like:

if settings.DEBUG and 'runserver' in sys.argv:
   # use manual FileResponse(...) download
else:
  # use nginx X-Accel-Redirect headers

as the feature toggle? Also if possible this should use pathlib and also contain a check for .is_file(), raising a 404 if the file doesn't exist before calling FileResponse(...). Thanks for the PR!

@kuhnchris
Copy link
Contributor Author

Sure, I'll adapt accordingly and let you know if it's ready!

@kuhnchris
Copy link
Contributor Author

Added check, also added pathlib for the final path, also changed the trigger to Debug + "runserver".
Let me know if that works for you. :-)

@meeb
Copy link
Owner

meeb commented Feb 12, 2023

Much nicer, thanks!

@meeb meeb merged commit b3f93dd into meeb:main Feb 12, 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.

2 participants