Prevent upvoting or downvoting multiple times on the same restroom#520
Conversation
|
Thank you for this! Hopefully @mi-wood or @tkwidmer can take a look at this, since they have dealt with this part of the code more than me. But on first look, this appears pretty good! I'm not familiar with the Rails session concept, so I'll perhaps take a look into that before merging. In any case... Happy Hacktoberfest! 🎃 💻 ✨ 🍂 |
DeeDeeG
added
bug
enhancement
Ready for Review
Hacktoberfest
|
Here are some resources about the Rails session feature... Tutorials, explainers and how-to's: Official docs: |
mi-wood
left a comment
mi-wood
left a comment
There was a problem hiding this comment.
Thanks for the pull request! I've left a few comments about reducing the complexity of the _vote_for_restroom method. Other than that, this look good!
| end | ||
|
|
||
| def _vote_for_restroom(up_or_downvote) | ||
| if session[:voted_for] and session[:voted_for].include? @restroom.id |
There was a problem hiding this comment.
Can we get rid of the if session[:voted_for] and just automatically create that at the beginning? It would be premature, but also reduce the complexity of these conditionals.
| @restroom = Restroom.find(params[:id]) | ||
| end | ||
|
|
||
| def _vote_for_restroom(up_or_downvote) |
There was a problem hiding this comment.
It might make sense to break this into a service and reduce the complexity of the controller/tests. There's an example here: https://github.com/RefugeRestrooms/refugerestrooms/tree/develop/app/services
| flash[:notice] = I18n.t('restroom.flash.alreadyvoted') | ||
| else | ||
| Restroom.increment_counter(up_or_downvote, @restroom.id) | ||
| session[:voted_for] = session[:voted_for] ? session[:voted_for].push(@restroom.id) : [@restroom.id] |
There was a problem hiding this comment.
Same as above, this would become a lot easier to read and would probably fix the code climate error.
3 participants
Context
Summary of Changes
sessionand prevent tallying further votes for that restroomConcerns
Checklist
Screenshots
After