-
Notifications
You must be signed in to change notification settings - Fork 0
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
Django-backend #1
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! This looks great. I have added some comments on how to improve it before we merge the PR in.
Another thing we should add is a README file that explains how to install Django and other dependencies, and how to run the webserver on a laptop to test the website locally. I think we should also add a requirements.txt
file and instructions on how to set up a python virtual environment for this project. (See for example https://github.com/etsap-TIMES/times-excel-reader/#development-setup)
<script src="linenumbers.min.js" type="text/javascript"></script> | ||
<script src="index.js" type="text/javascript"></script> | ||
<script src=" {% static 'js/codejar.min.js' %}" type="text/javascript"></script> | ||
<script src="{% static 'js/codejar.min.js' %}" type="text/javascript"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This JS file is being imported twice.
Generated by 'django-admin startproject' using Django 4.0.5. | ||
|
||
For more information on this file, see | ||
https://docs.djangoproject.com/en/4.0/topics/settings/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I go to this link, it says "This document is for an insecure version of Django that is no longer supported. Please upgrade to a newer release!". Maybe we should use v4.2?
# See https://docs.djangoproject.com/en/4.0/howto/deployment/checklist/ | ||
|
||
# SECURITY WARNING: keep the secret key used in production secret! | ||
SECRET_KEY = 'django-insecure-zjnqabq%c2lpq$jgg12z8q6foq^q2i94cr02q76&(2stpwn#a8' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not commit the secret key to a public repo. Let's find out where to store it instead
SECRET_KEY = 'django-insecure-zjnqabq%c2lpq$jgg12z8q6foq^q2i94cr02q76&(2stpwn#a8' | ||
|
||
# SECURITY WARNING: don't run with debug turned on in production! | ||
DEBUG = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, we should set this to False in the repo, and have a way to make it True when we test the website locally.
<script src=" {% static 'js/codejar.min.js' %}" type="text/javascript"></script> | ||
<script src="{% static 'js/codejar.min.js' %}" type="text/javascript"></script> | ||
<script src=" {% static 'js/linenumbers.min.js' %}" type="text/javascript"></script> | ||
<script type="text/javascript" src="{% static 'js/script.js' %}"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This expects script.js
to live in static/js/
but this PR moves it to templates/
No description provided.