-
-
Notifications
You must be signed in to change notification settings - Fork 775
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
As a Gitcoin admin, I want an embedded survey for each Gitcoin hackathon so I know the motivations of each hacker #6128
As a Gitcoin admin, I want an embedded survey for each Gitcoin hackathon so I know the motivations of each hacker #6128
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6128 +/- ##
==========================================
+ Coverage 27.06% 27.21% +0.15%
==========================================
Files 291 291
Lines 26702 26778 +76
Branches 3951 3956 +5
==========================================
+ Hits 7226 7288 +62
- Misses 19209 19223 +14
Partials 267 267
Continue to review full report at Codecov.
|
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.
@zoek1 checked out the video ! Dope stuff ^_^
- Left a bunch of comments + questions
- I noticed you had a migration error -> could you rebase with master. -> delete your migration and recreate it. we wanna avoid having merge migration file
@thelostone-mc done! |
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.
Left a comment on the poll type but otherwise lgtm
Done @thelostone-mc 👍 |
The responsive view is not in this PR, could we roll this out and get responsiv view out next week? |
I'll work on the responsive view today, I'll notify here when is done. |
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.
Looks pretty good - a couple of small improvements id like to see before we merge it in
</h4> | ||
{% else %} | ||
<h4 class="font-weight-bolder"> | ||
Contratulations! <br> Your registration for {{ hackathon.name }} is successful. |
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.
typo here
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.
spelling and spacing as well
{% endif %} | ||
<ul class="list-unstyled"> | ||
<li class="mb-3 gc-text-blue font-weight-bold"><a href="/townsquare?tab=hackathon:{{ hackathon.id }}&trending=0&personal=0"><i class="mr-1 fas fa-check-circle gc-text-blue"></i> Introduce yourself on Town Square + what you like to build (and earn your first Kudos!)</a></li> | ||
<li class="mb-3 gc-text-blue font-weight-bold"><i class="mr-1 fas fa-check-circle gc-text-blue"></i> RSVP our 3 workshops</li> |
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.
should be RSVP to our 3 workshops
app/dashboard/views.py
Outdated
option = Option.objects.get(id=int(entry['value'])) | ||
|
||
try: | ||
Answer.objects.get(user=request.user, question=question, choice=option) |
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.
Can use get_or_create
here instead https://docs.djangoproject.com/en/3.0/ref/models/querysets/#get-or-create
app/dashboard/views.py
Outdated
set_questions = {} | ||
for entry in poll: | ||
try: | ||
question = Question.objects.get(id=int(entry['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.
I'm guessing that you wrapped this is a try/except to catch if these two querysets fail. Lets use get_object_or_404
instead
Thanks for the updates @zoek1! Responsive views look good to me. The modal on responsive view has different options than the normal view and design spec....could you double check why that is? See your above screenshots: |
@PixelantDesign it's because the questions aren't hardcoded, any of the admins could add, modify, or remove options from the admin panel. In the video demo, I showed how to create options. The question text and the options are dynamics |
Hey @zoek1 - after merging this work I spent some additional time testing it and found a lot of issues, so I'm going to disable the popup and tell our staff not to use this functionality until it is fixed and fully tested. Here are some of the problems I found:
There may be other problems as well as I feel you did not test this thoroughly. Please submit a followup PR that is well tested and fixes these issues - thanks |
@danlipert I mentioned in the video demo that "Open-ended questions" do not work and they don't part of the spec, this is a draft for such type of questions. |
@zoek1 I see - in the future, don't include any half-completed features if they don't work properly. You have to consider the end user of this functionality who will not be aware what features work and which don't. |
Description
Refers/Fixes
#6087
Testing
DEMO: https://www.loom.com/share/9d95aa6549e54e779e9124959c0a0bb7
Register views
Form to get expectation from the user
The user is registered but the hackathon doesn't start yet
The user is registered but the hackathon already started
Mobile view