Skip to content
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

Prevent upvoting or downvoting multiple times on the same restroom #520

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions app/controllers/restrooms_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ def create

def update
if params[:restroom][:downvote]
Restroom.increment_counter(:downvote, @restroom.id)
_vote_for_restroom(:downvote)
elsif params[:restroom][:upvote]
Restroom.increment_counter(:upvote, @restroom.id)
_vote_for_restroom(:upvote)
elsif @restroom.update(permitted_params)
flash[:notice] = I18n.t('restroom.flash.updated')
else
Expand Down Expand Up @@ -76,6 +76,15 @@ def find_restroom
@restroom = Restroom.find(params[:id])
end

def _vote_for_restroom(up_or_downvote)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if session[:voted_for] and session[:voted_for].include? @restroom.id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, this would become a lot easier to read and would probably fix the code climate error.

end
end

def permitted_params
params.require(:restroom).permit(
:name,
Expand Down
1 change: 1 addition & 0 deletions config/locales/en/restroom.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ en:
upvotesuccess: 'This restroom has been upvoted! Thank you for contributing to our community.'
downvoteerror: 'There was an unexpected problem downvoting this post.'
downvotesuccess: 'This restroom has been downvoted! Thank you for contributing to our community.'
alreadyvoted: 'You have already voted for this restroom.'
new: 'A new restroom entry has been created for %{name}.'
updated: 'This restroom entry has been updated'
deleted: 'This restroom entry has been deleted'
Expand Down
1 change: 1 addition & 0 deletions config/locales/es/restroom.es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ es:
upvotesuccess: '¡Este baño se ha calificado en positivo! Gracias por contribuir a nuestra comunidad.'
downvoteerror: 'Se generó un problema inesperado con su calificación negativa.'
downvotesuccess: '¡Este baño se ha calificado en negativo! Gracias por contribuir a nuestra comunidad.'
alreadyvoted: 'Ya calificó a este baño.'
new: 'Una nueva entrada de baño se ha creado para %{name}.'
updated: 'Esta entrada de baño ha sido actualizada'
deleted: 'Esta entrada de baño ha sido borrada'
Expand Down
45 changes: 38 additions & 7 deletions spec/controllers/restrooms_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,31 +8,62 @@

context "voting" do
let(:restroom) { FactoryBot.create(:restroom) }

it "should downvote" do
post_params = {
let(:post_params_downvote) {
{
id: restroom.id,
restroom: {
downvote: true
}
}
}
let(:post_params_upvote) {
{
id: restroom.id,
restroom: {
upvote: true
}
}
}

it "should downvote" do
expect {
post :update, params: post_params
post :update, params: post_params_downvote
}.to change { restroom.reload.downvote }.by 1
end

it "should upvote" do
expect {
post :update, params: post_params_upvote
}.to change { restroom.reload.upvote }.by 1
end

it "shouldn't upvote or downvote twice" do
post :update, params: post_params_upvote

expect {
post :update, params: post_params_upvote
}.not_to change { restroom.reload.upvote }

expect {
post :update, params: post_params_downvote
}.not_to change { restroom.reload.downvote }
end

it "should allow you to vote for multiple restrooms" do
session[:voted_for] = [restroom.id]
restroom_two = FactoryBot.create(:restroom)

post_params = {
id: restroom.id,
id: restroom_two.id,
restroom: {
upvote: true
downvote: true
}
}

expect {
post :update, params: post_params
}.to change { restroom.reload.upvote }.by 1
}.to change { restroom_two.reload.downvote }.by 1
expect(session[:voted_for]).to match_array([restroom.id, restroom_two.id])
end
end
end