Commit 5e50d4f3 authored by azul's avatar azul

fix: return 409 when approving redundant requests

We were responding with 500 and an error popup
when a request was approved for an action that had already been performed.

For example when approving the removal of a former member
that had already left the group on their own
we responded with a 500.

This changes the response to 409 - conflict:
`    This response is sent when a request conflicts with the current state of the server.`

I was also considering 404 - especially for requests
to remove a non-member.
However a 404 for an update on a request
would seem more like the request itself could not be found.

This commit introduces the Request::PointlessAction exception.
It will be raised by requests whos action has already been performed.

It allows us to unify error handling on the controller level
and detect the different errors in each request class
and reraise them with a common more semantic error class.
parent 5247682d
......@@ -37,10 +37,15 @@ module Common::Requests
# uses model permissions.
skip_authorization
if mark
@req.mark!(mark, current_user)
success I18n.t(@req.name), success_message
begin
@req.mark!(mark, current_user)
success I18n.t(@req.name), success_message
rescue Request::PointlessAction => e
status = 409
notice I18n.t(@req.name), e.message
end
end
render template: 'common/requests/update'
render template: 'common/requests/update', status: status
end
#
......
......@@ -154,7 +154,10 @@ module Group::Users
#
def remove_user!(user)
membership = memberships.find_by_user_id(user.id)
raise ErrorMessage.new('no such membership') unless membership
unless membership
raise ErrorMessage,
I18n.t(:no_such_membership_error, group: self.name, user: user.name)
end
# removing all participations (makes the stars disappear - not sure
# if we want this)
......
......@@ -10,6 +10,8 @@ class MembershipRequest < Request
def after_approval
group.add_user! user
rescue AssociationError => e
raise PointlessAction, e.message
end
def event
......
......@@ -30,6 +30,12 @@
class Request < ActiveRecord::Base
include AASM
# Raised when approving a Request to do something
# that does not have any consequences.
# (Such as removing a user from a group who is not a member)
class PointlessAction < CrabgrassException
end
##
## ASSOCIATIONS
##
......
......@@ -36,6 +36,8 @@ class RequestToJoinYou < MembershipRequest
def after_approval
group.add_user! created_by
rescue AssociationError => e
raise PointlessAction, e.message
end
def description
......
......@@ -50,6 +50,8 @@ class RequestToRemoveUser < Request
def after_approval
group.remove_user!(user)
rescue ErrorMessage => e
raise PointlessAction, e.message
end
def description
......
......@@ -56,6 +56,7 @@ en:
group_membership_count: "%{count} Members"
membership_destroy_confirm_message: "Are you sure you want to remove %{entity} from %{group}?"
membership_exists_error: "Membership already exists for %{member}"
no_such_membership_error: "%{user} is currently not part of %{group}"
membership_leave_message: "You have been removed from %{group}"
membership_notification: "Membership notification"
membership_requests: "Membership Requests"
......
......@@ -36,7 +36,7 @@ class Me::RequestsControllerTest < ActionController::TestCase
assert_response :success
end
def test_dup_group_request
def test_dup_group_join_request
user = FactoryBot.create(:user)
group = FactoryBot.create(:group)
group.add_user! user
......@@ -46,10 +46,10 @@ class Me::RequestsControllerTest < ActionController::TestCase
recipient: group
group.add_user! requesting
xhr :post, :update, id: request.id, mark: 'approve'
assert_response :success
assert_response :conflict
end
def test_remove_from_group_request
def test_dup_remove_from_group_request
user = FactoryBot.create(:user)
requesting = FactoryBot.create(:user)
remove_me = FactoryBot.create(:user)
......@@ -63,7 +63,7 @@ class Me::RequestsControllerTest < ActionController::TestCase
login_as user
group.remove_user! remove_me
xhr :post, :update, id: request.id, mark: 'approve'
assert_response :success
assert_response :conflict
end
def test_destroy_group_request
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment