#A02 - As admin, I would like to create new hackathon events#99
#A02 - As admin, I would like to create new hackathon events#99stefdworschak merged 27 commits intoCode-Institute-Community:masterfrom hebs87:feature/admin-create-hackathon
Conversation
# Conflicts: # .gitignore
…ned directories to .gitignore
…tes to required fields
# Conflicts: # .gitignore # hackathon/urls.py # hackathon/views.py
# Conflicts: # templates/base.html
# Conflicts: # .gitignore
# Conflicts: # static/css/style.css
stefdworschak
left a comment
There was a problem hiding this comment.
@hebs87 This is a really good PR! Just a few general comments.
My main concern is that you changed and deleted quite a lot of migrations from what I can see. Could you please see what happened here and if instead of changing existing migrations can create new migrations making the changes you want?
| @@ -1,18 +0,0 @@ | |||
| # Generated by Django 3.1.1 on 2020-10-22 15:53 | |||
There was a problem hiding this comment.
Why did you remove this migration?
There was a problem hiding this comment.
After pulling in and merging the latest PR, I kept getting an error when trying to navigate to the create_event page, which was to do with the CustomUser model. StackOverflow suggested that I clear the existing migrations and then run the makemigrations and migrate commands again, which resolved the issue. How do I get the previous migrations back?
There was a problem hiding this comment.
When this happens what I would first do is just try to delete the db.sqlite3 file. If you have an existing file that you have done work on and others it might happen that there are issues then with the migrations. By deleting the database this should give you a clean slate. Running makemigrations then should just show "No changes needed" and on migrate it would just recreate the db with the most up-to-date models.
There was a problem hiding this comment.
To remove the the changes to the migrations you can run git revert <commit_id> (this will delete the changes, but keep the commit history) and you will then have to push the changes to your fork (probably with -f at the end). You can get the commit ID from the list of commits above or by running git log.
There was a problem hiding this comment.
Cool, I'll keep that in mind going forward and delete the db.sqlite3 file as the first port of call 👍 I ran into some trouble with reverting the branch though, so I had to do a hard reset instead and force push to the branch. A couple of my previous commits are now gone from the history, but all the migration files are reverted and my new changes are also added back in.
|
|
||
|
|
||
| class HackathonForm(forms.ModelForm): | ||
| display_name = forms.CharField( |
There was a problem hiding this comment.
Please add a docstring for the class
There was a problem hiding this comment.
Sorry, I normally do add docstrings! This one must have slipped the net. I've added this now and it will be visible when I push the changes.
hackathon/models.py
Outdated
There was a problem hiding this comment.
Why are you adding blank=False, null=True for all of the below fields? They are all required so should not be null.
There was a problem hiding this comment.
This was just from previous experience - when setting a field on an existing model to blank=False and there are existing records in the DB, I've received errors in the past when running the migrations again, or it has asked me to set a value for any existing blank fields. To resolve this, I've had to set null=True on the relevant fields, so this is a force of habit! I've removed null=True from the relevant fields now and it will be visible when I push the changes to the existing PR.
There was a problem hiding this comment.
Sure. That's fine, but I think it's either null=True or set a default. And as per Django documentation default="" for Char and TextFields is the preferred solution.
There was a problem hiding this comment.
Perfect, makes sense. I'll keep that in mind going forward 👍
| {% endblock %} | ||
|
|
||
| {% block js %} | ||
| <script src="https://cdnjs.cloudflare.com/ajax/libs/jquery-datetimepicker/2.5.20/jquery.datetimepicker.full.min.js" |
There was a problem hiding this comment.
Do you need this and the CDN at the top of the template as well?
There was a problem hiding this comment.
When you mention the CDN at the top of the template, are you referring to the link tag? If so, yes they are both needed. The link tag is for the datetimepicker CSS, and the script tag is for the relevant JS.
There was a problem hiding this comment.
Could you please extract the JS to an external JS script file?
There was a problem hiding this comment.
The datetimepicker wouldn't render when I moved the JS into script.js, so I've had to move it into a new JS file - datetimepicker.js - and import it into the create_event.html template. This change will be visible once it is pushed to the existing PR.
hackathon/views.py
Outdated
There was a problem hiding this comment.
Not sure you need to create a context dict just for one key. I'd just pass the form dict directly to the render function.
There was a problem hiding this comment.
This is just a personal preference, but changed as requested.
| def create_event(request): | ||
| """ Allow users to create hackathon event """ | ||
|
|
||
| if request.method == 'GET': |
There was a problem hiding this comment.
I would put this in an else clause for the if request.method == 'POST' instead of having two serparate if statements.
There was a problem hiding this comment.
Again, this is my employer's preference, which is why I did it this way. I've changed that line to an else clause instead now.
hackathon/views.py
Outdated
There was a problem hiding this comment.
Do you need to actually assign start and end_date again or should that not be in the POST already?
There was a problem hiding this comment.
I did this becuase I've formatted the start_date and end_date a couple of lines above it (for date validation), so I passed in the formatted value to the form fields. But, you're right, I've added the input format to the relevant form field anyway so these two lines are unneeded. I've removed them now.
| @@ -1,166 +1,170 @@ | |||
| :root { | |||
There was a problem hiding this comment.
what's happended here? Did you replace the whole file?
There was a problem hiding this comment.
Nope, not sure why it's showing like that there. I added a class but didn't change anything else in the file. I had to re-add the class once I pulled and merged the latest PR, so maybe that may have thrown git? I've checked the repo though and all the existing classes are still there, as well as my new class.
templates/includes/navbar.html
Outdated
There was a problem hiding this comment.
In line with my earlier comment, I would rename "Events" to "View Hackathons" and "Create Event" to "Create Hackathon" please.
There was a problem hiding this comment.
Done. The "Events" link wasn't one that I added by the way, but I've changed it while in the file.
…dated latest changes following PR comments
User Story ID:
Description of PR:
All changes made are outlined below:
Tests:
Manually tested all implemented functionality, which is all working as expected.
Validated all code in relevant validators and all passed - HTML validator threw errors for Django template tags, but no other errors.
Know bugs/errors:
No know bugs or errors after testing.
Screenshots:
Does this introduce a breaking change