-
Notifications
You must be signed in to change notification settings - Fork 3
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
Priority field to Post/Polls + Refactor #246
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: backend/portal/views.py
Did you find this useful? React with a 👍 or 👎 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #246 +/- ##
==========================================
- Coverage 91.45% 91.42% -0.03%
==========================================
Files 59 59
Lines 2504 2496 -8
==========================================
- Hits 2290 2282 -8
Misses 214 214 ☔ View full report in Codecov by Sentry. |
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! Just one comment worth considering
@@ -37,15 +37,22 @@ class Poll(models.Model): | |||
) | |||
|
|||
club_code = models.CharField(max_length=255, blank=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.
Hmm, not entirely sure of this myself. What are people's thoughts on having club_code
and club_comment
here? Do you think this is general enough for future models or should we push this back down to the Poll
and Post
models.
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.
wait what future models were you thinking of? I was thinking Content is just the "superclass" to Post/Polls, so has all the shared stuff. I guess you're thinking of a different "meaning" to Content?
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.
Hm that is fair. Yea I think posts and polls are really all that is it. I was thinking maybe Ashley's Events could also inherit but thinking about it some more that wouldn't make sense. Okay down to merge haha
priority = models.IntegerField(default=0) | ||
|
||
class Meta: | ||
abstract = 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.
Good stuff haha
backend/portal/views.py
Outdated
@@ -99,7 +99,7 @@ def get_queryset(self): | |||
) | |||
) | |||
|
|||
@action(detail=False, methods=["post"]) | |||
@action(detail=False, methods=["post"]) # WHY IS THIS 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.
POST
because the devices send us a id_hash
which is a hash based on device id. This is because we want to associate votes based on this rather than pennkey to keep users anonymous. For this, if the user voted we don't given them the ability to vote again (based on if id_hash
already voted)
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.
wait wat, but i don't see any posting going on here. am i blind? it looks like its just retreiving stuff
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.
L108 id_hash = request.data["id_hash"]
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.
but there's no up updating to our models going on? like can't we just use a get request for this
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.
oh wait nvm. https://stackoverflow.com/questions/19637459/rest-api-using-post-instead-of-get. so we can send through the body im assuming?
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.
Yea right. I think generally I like sending things via POST body as opposed to query params if it isn't literally for a non trivial query
No description provided.