Allow only one vote per user on resources #272
Conversation
|
@chynh I don't see it on here, but I previously recall discussing how this leaks the API keys of all users that have voted for a resource. Were you planning on coming back to this to redesign in a more secure way? |
|
Not sure what happened there. But I just pushed it up again so let me know if you still don't see it. I believe with the security concern has been addressed with the redesign. |
There was a problem hiding this comment.
Looks good so far. This PR has taught me a few things about python/flask that I didn't know, so I want to research those and make sure I understand everything that's going on before we merge, but it's looking really good. I should have it merged in the next couple days if no one else has any comments
| @bp.route('/resources/<int:id>/<string:vote_direction>', methods=['PUT']) | ||
| @authenticate | ||
| def change_votes(id, vote_direction): | ||
| if vote_direction not in ['upvote', 'downvote']: |
There was a problem hiding this comment.
I love this. Much better than making this two separate endpoints. ❤️👍
|
I ran this and I only get 400 errors: Additionally, I don't get the 404 when it's something other than upvote/downvote, I still get a 400 Does this code actually work for you? |
192.168.176.1 - - [08/Mar/2020 18:59:08] "PUT /api/v1/resources/5/upvote HTTP/1.1" 200 -
192.168.176.1 - - [08/Mar/2020 18:59:13] "PUT /api/v1/resources/5/downvote HTTP/1.1" 200 -
192.168.176.1 - - [08/Mar/2020 18:59:13] "PUT /api/v1/resources/5/downvote HTTP/1.1" 200 -
192.168.176.1 - - [08/Mar/2020 18:59:26] "PUT /api/v1/resources/5/down HTTP/1.1" 302 -
192.168.176.1 - - [08/Mar/2020 18:59:26] "GET /404 HTTP/1.1" 404 -hmm... not sure what's going on there. Above are the results that I got. I did run into the issue of not getting 404 but other than that, hitting upvote and downvote should work. I did fix the 404 issue and pushed it up again. Let me know if it still does not work for you. |
|
@chynh I realized that the 400 error is coming from the fact that my |
|
Ok, I looked at the docs, we don't have docs for this endpoint yet 😅 Can you go ahead and update the openapi file to add docs for these endpoints? We'll need one section for |
Ah okay. And yes we don't need the body when making this request.
Got it. I never wrote openapi docs before but looking at the file it seems pretty straight forward. I'll push it up again after adding those sections. |
|
It's possible that Flask just automatically tries to parse a body on |
I figured out that 400 error was coming from |
Changes
VoteInformationmodel that stores a user's vote information on a resource.Stopped short of adding more tests to get an opinion on the changes so far. Let me know what you think.
Fixes #211