-
Notifications
You must be signed in to change notification settings - Fork 41
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
P05/P11: As Participant, I would like to enroll/withdraw for an upcoming Hackathon. #113
P05/P11: As Participant, I would like to enroll/withdraw for an upcoming Hackathon. #113
Conversation
Added basic view and connected it to ListView.
Includes logic to display judges and the option to enroll.
Does not process any data yet, simply proof of concept.
Merge with original fork
Toast popup function is reformatted. Fetch now catches errors
This should now fix issues #S09 - Code-Institute-Community#38 and #S06 - Code-Institute-Community#35
The first "Unenroll" click wouldn't change because of whitespace.
S06 - Merging so othes can pull my changes easily.
Merging with Ri-Dearg latest commits for issue S06/S09 on Hackaton app
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.
@mkuti This is a very good Pull Request and quite complex at that. Well done!! 👍 I have left some minor comments, please have a look and let me know when I can look at the PR again.
Also, there are a lot of migrations that have been changed and deleted. I would prefer to keep the migration history as it is and only add new migrations to alter the databases. Please make sure that you revert those changes before the next review. Thanks!
hackathon/fixtures/hackathons.json
Outdated
@@ -14,6 +14,7 @@ | |||
"organiser":12, |
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.
I think this as been resolved in #111 and now creates a merge conflict. Could you please resolve the merge conflict?
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.
Will do
hackathon/models.py
Outdated
@@ -66,7 +72,6 @@ class Hackathon(models.Model): | |||
def __str__(self): | |||
return self.display_name | |||
|
|||
|
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.
As per PEP8 there should be two blank lines between classes, please add back the removed line.
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.
done
{% endif %} | ||
{% if hackathon.end_date|event_ended %} | ||
{% if request.user.is_staff %} | ||
{% include 'hackathon/includes/enrollstaff.html' %} |
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.
It seems a bit much to create separate templates for 5 lines of code. I would just add the code directly in the template.
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.
I followed and respected he code of @ri.dearg for his original enroll.html. I do actually find it clearer with separate templates due to the form. But if you really want me to remove those enroll.html templates and add code directly in hackatondetail.html, I can do it
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.
@TravelTimN Hi Tim, could you have a look at this comment here and let me know if I should still apply the change requested by Stefan. I find it clearer with separate templates.
@@ -2,6 +2,8 @@ | |||
# range snippet from: https://www.djangosnippets.org/snippets/1357/ | |||
# adjusted to current project needs based on https://docs.djangoproject.com/en/3.1/howto/custom-template-tags/ | |||
from django.template import Library | |||
import datetime |
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.
As per PEP8 standard libraries should be imported first, then 3rd party libraries (e.g. django libraries) and they should be separated by a blank line.
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.
done
@@ -2,6 +2,8 @@ | |||
# range snippet from: https://www.djangosnippets.org/snippets/1357/ | |||
# adjusted to current project needs based on https://docs.djangoproject.com/en/3.1/howto/custom-template-tags/ | |||
from django.template import Library | |||
import datetime | |||
from django.conf import settings | |||
|
|||
register = Library() | |||
|
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.
Must have overlooked this earlier, but please add an extra blank line here as well.
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.
done
hackathon/views.py
Outdated
# Checks to make sure the user has the permissions to enroll | ||
user = request.user | ||
data = {} | ||
if (request.method == "POST"): |
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.
I don't think the parenthesis here are needed.
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.
done
hackathon/views.py
Outdated
data["message"] = "You have enrolled as a judge." | ||
|
||
else: | ||
'''Check if user is already enrolled |
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.
You are mixing your comment format here. As mentioned above though, I think you can remove this comment as well.
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.
Removed all comments on this view
hackathon/views.py
Outdated
|
||
def ajax_enroll_toggle(request): | ||
"""Swaps between being enrolled as a judge and unenrolling.""" | ||
# Checks to make sure the user has the permissions to enroll |
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 comment is not needed. Also, in general you don't need to comment every condition. The code itself should be readable and only on special occasions you should add comments if extra context is necessary. Please remove also the comments below, I think it should be fine to understand what you are trying to achieve.
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.
Did you not leave another comment on removing 'ajax' in the view name and url?
static/js/script.js
Outdated
@@ -1,3 +1,67 @@ | |||
function toastMessage(tag, message) { | |||
// Sets the toast HTML. |
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.
Please make sure that your indentation here is correct.
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.
Done
@@ -48,6 +48,12 @@ | |||
integrity="sha384-w1Q4orYjBQndcko6MimVbzY0tgp4pWB4lZ7lr30WKz0vr/aWKhXdBNmNb5D92v7s" crossorigin="anonymous"> | |||
</script> | |||
{% block js %} | |||
<script> | |||
// Fires toast popups if there are messages | |||
{% if messages %} |
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.
What is this needed for?
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 is from @Ri-Dearg but I am guessing this is to fire toast alert when a message happens in Django. It's explained here: https://pypi.org/project/django-toast-messages/. We need to insert a script in the middle of a django template tag
Thanks :) the most complex code is from @Ri-Dearg though from whom I followed his choice and added few parts to fix the issue of particpant to enrol and withdraw. |
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.
Looking great! Was there a reason the Tertiary Orange color was deleted and a few items changed for the CSS file?
static/css/style.css
Outdated
@@ -10,7 +10,6 @@ | |||
--s-pink: #FF5A60; | |||
--s-teal: #23BBBB; | |||
--t-grey: #E0E7FF; | |||
--t-orange: #E8461030; |
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.
Why was the tertiary orange color?
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.
Hi @TravelTimN I am not sure why was this added or removed. When I pulled last upstream from master, I had a merging conflict on style.css with two very different files. As advised by Stefan, I accepted incoming change and made sure the only change made by @Ri-Dearg before me was saved.Then I committed and pushed.
static/css/style.css
Outdated
top: 80px; | ||
right: 20px; | ||
min-height: 200px; | ||
z-index: 1021; | ||
} |
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.
Needs a blank line at the end of the file.
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.
done
templates/includes/messages.html
Outdated
{% endfor %} | ||
{% endif %} | ||
</div> | ||
</div> |
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.
Needs a blank line at the end of the file.
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.
done
<script src="{% static 'js/script.js' %}"></script> | ||
|
||
{% endblock %} | ||
{% endif %} |
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.
Needs a blank line at the end of the file.
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.
done
I have applied all changes requested and resolved migration issue as well. There is no current merge conflict. |
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.
LGTM
Description
Fixes issues #22 - #P05 and #28 - #P11
Followed up the pull request of #S06/#S09 As Staff I would like to enroll/withdraw as a Judge #90, merged from https://github.com/Ri-Dearg/ci-hackathon-app
Separated JS script to its separate file in static folder
Added a filter in my_tags.py to compare end date of hackaton to today's date
Added participants field in the model of Hackaton.
Changed enroll.html to enrollstaff.html and added enrollpart.html inside hackaton/templates/include
Added an if statement on hackatondetail template to show a button to enroll or withdraw that is only visible to participant user.
Added if statement in ajax_enroll view to allow the database to add or remove users as participants.
Pull request type
Related Issue
#22
#28
#35
#38
Testing
Manual testing with a different user type to ensure the button to enroll and its text changes between participant and staff.
Tested the feature of date comparator to ensure the enroll button does not show if hackaton has ended
Screenshots
Additional Information
When testing, noticed that all users are automatically showing as judges or participants and the enrol or withdrawing button does not make changes in the admin panel.
Not sure if this is the type of field, 'ManytoManyField' chosen in the Hackaton model.
Does this introduce a breaking change