Commit 4652489f authored by Rémy Coutable's avatar Rémy Coutable

New Members::DestroyService

This is to ensure we don't send unwanted notifications when deleting a
project. In other words, stop abusing AR callbacks and use services.
Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 00906b5b
...@@ -23,22 +23,24 @@ module MembershipActions ...@@ -23,22 +23,24 @@ module MembershipActions
@member = membershipable.members.find_by(user_id: current_user) @member = membershipable.members.find_by(user_id: current_user)
return render_403 unless @member return render_403 unless @member
@member = Members::DestroyService.new(@member, current_user).execute
source_type = @member.real_source_type.humanize(capitalize: false) source_type = @member.real_source_type.humanize(capitalize: false)
if can?(current_user, action_member_permission(:destroy, @member), @member) if @member.destroyed?
notice = notice =
if @member.request? if @member.request?
"Your access request to the #{source_type} has been withdrawn." "Your access request to the #{source_type} has been withdrawn."
else else
"You left the \"#{@member.source.human_name}\" #{source_type}." "You left the \"#{@member.source.human_name}\" #{source_type}."
end end
@member.destroy
redirect_to [:dashboard, @member.real_source_type.tableize], notice: notice redirect_to [:dashboard, @member.real_source_type.tableize], notice: notice
else else
if cannot_leave? if cannot_leave?
alert = "You can not leave the \"#{@member.source.human_name}\" #{source_type}." alert = "You can not leave the \"#{@member.source.human_name}\" #{source_type}."
alert << " Transfer or delete the #{source_type}." alert << " Transfer or delete the #{source_type}."
redirect_to polymorphic_url(membershipable), alert: alert redirect_to polymorphic_url(membershipable), alert: alert
else else
render_403 render_403
......
...@@ -48,7 +48,6 @@ class Member < ActiveRecord::Base ...@@ -48,7 +48,6 @@ class Member < ActiveRecord::Base
after_create :post_create_hook, unless: [:pending?, :importing?] after_create :post_create_hook, unless: [:pending?, :importing?]
after_update :post_update_hook, unless: [:pending?, :importing?] after_update :post_update_hook, unless: [:pending?, :importing?]
after_destroy :post_destroy_hook, unless: :pending? after_destroy :post_destroy_hook, unless: :pending?
after_destroy :post_decline_request, if: :request?
delegate :name, :username, :email, to: :user, prefix: true delegate :name, :username, :email, to: :user, prefix: true
...@@ -188,7 +187,7 @@ class Member < ActiveRecord::Base ...@@ -188,7 +187,7 @@ class Member < ActiveRecord::Base
end end
def send_request def send_request
# override in subclass notification_service.new_access_request(self)
end end
def post_create_hook def post_create_hook
...@@ -215,10 +214,6 @@ class Member < ActiveRecord::Base ...@@ -215,10 +214,6 @@ class Member < ActiveRecord::Base
post_create_hook post_create_hook
end end
def post_decline_request
# override in subclass
end
def system_hook_service def system_hook_service
SystemHooksService.new SystemHooksService.new
end end
......
...@@ -33,12 +33,6 @@ class GroupMember < Member ...@@ -33,12 +33,6 @@ class GroupMember < Member
super super
end end
def send_request
notification_service.new_group_access_request(self)
super
end
def post_create_hook def post_create_hook
notification_service.new_group_member(self) notification_service.new_group_member(self)
...@@ -64,10 +58,4 @@ class GroupMember < Member ...@@ -64,10 +58,4 @@ class GroupMember < Member
super super
end end
def post_decline_request
notification_service.decline_group_access_request(self)
super
end
end end
...@@ -111,12 +111,6 @@ class ProjectMember < Member ...@@ -111,12 +111,6 @@ class ProjectMember < Member
super super
end end
def send_request
notification_service.new_project_access_request(self)
super
end
def post_create_hook def post_create_hook
unless owner? unless owner?
event_service.join_project(self.project, self.user) event_service.join_project(self.project, self.user)
...@@ -152,12 +146,6 @@ class ProjectMember < Member ...@@ -152,12 +146,6 @@ class ProjectMember < Member
super super
end end
def post_decline_request
notification_service.decline_project_access_request(self)
super
end
def event_service def event_service
EventCreateService.new EventCreateService.new
end end
......
module Members
class DestroyService < BaseService
attr_accessor :member, :current_user
def initialize(member, user)
@member, @current_user = member, user
end
def execute
if can?(current_user, "destroy_#{member.type.underscore}".to_sym, member)
member.destroy
notification_service.decline_access_request(member) if member.request?
end
member
end
private
def abilities
Ability.abilities
end
def can?(object, action, subject)
abilities.allowed?(object, action, subject)
end
def notification_service
NotificationService.new
end
end
end
...@@ -181,15 +181,16 @@ class NotificationService ...@@ -181,15 +181,16 @@ class NotificationService
end end
end end
# Project access request # Members
def new_project_access_request(project_member) def new_access_request(member)
mailer.member_access_requested_email(project_member.real_source_type, project_member.id).deliver_later mailer.member_access_requested_email(member.real_source_type, member.id).deliver_later
end end
def decline_project_access_request(project_member) def decline_access_request(member)
mailer.member_access_denied_email(project_member.real_source_type, project_member.project.id, project_member.user.id).deliver_later mailer.member_access_denied_email(member.real_source_type, member.source_id, member.user_id).deliver_later
end end
# Project invite
def invite_project_member(project_member, token) def invite_project_member(project_member, token)
mailer.member_invited_email(project_member.real_source_type, project_member.id, token).deliver_later mailer.member_invited_email(project_member.real_source_type, project_member.id, token).deliver_later
end end
...@@ -216,15 +217,7 @@ class NotificationService ...@@ -216,15 +217,7 @@ class NotificationService
mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later
end end
# Group access request # Group invite
def new_group_access_request(group_member)
mailer.member_access_requested_email(group_member.real_source_type, group_member.id).deliver_later
end
def decline_group_access_request(group_member)
mailer.member_access_denied_email(group_member.real_source_type, group_member.group.id, group_member.user.id).deliver_later
end
def invite_group_member(group_member, token) def invite_group_member(group_member, token)
mailer.member_invited_email(group_member.real_source_type, group_member.id, token).deliver_later mailer.member_invited_email(group_member.real_source_type, group_member.id, token).deliver_later
end end
......
...@@ -134,18 +134,6 @@ describe Member, models: true do ...@@ -134,18 +134,6 @@ describe Member, models: true do
it { is_expected.to respond_to(:user_email) } it { is_expected.to respond_to(:user_email) }
end end
describe 'Callbacks' do
describe 'after_destroy :post_decline_request, if: :request?' do
let(:member) { create(:project_member, requested_at: Time.now.utc) }
it 'calls #post_decline_request' do
expect(member).to receive(:post_decline_request)
member.destroy
end
end
end
describe ".add_user" do describe ".add_user" do
let!(:user) { create(:user) } let!(:user) { create(:user) }
let(:project) { create(:project) } let(:project) { create(:project) }
......
...@@ -61,16 +61,6 @@ describe GroupMember, models: true do ...@@ -61,16 +61,6 @@ describe GroupMember, models: true do
end end
end end
describe '#post_decline_request' do
it 'calls NotificationService.decline_group_access_request' do
member = create(:group_member, user: build_stubbed(:user), requested_at: Time.now)
expect_any_instance_of(NotificationService).to receive(:decline_group_access_request)
member.__send__(:post_decline_request)
end
end
describe '#real_source_type' do describe '#real_source_type' do
subject { create(:group_member).real_source_type } subject { create(:group_member).real_source_type }
......
...@@ -152,15 +152,5 @@ describe ProjectMember, models: true do ...@@ -152,15 +152,5 @@ describe ProjectMember, models: true do
member.__send__(:after_accept_request) member.__send__(:after_accept_request)
end end
end end
describe '#post_decline_request' do
it 'calls NotificationService.decline_project_access_request' do
member = create(:project_member, user: build_stubbed(:user), requested_at: Time.now)
expect_any_instance_of(NotificationService).to receive(:decline_project_access_request)
member.__send__(:post_decline_request)
end
end
end end
end end
require 'spec_helper'
describe Members::DestroyService, services: true do
let(:user) { create(:user) }
let(:project) { create(:project) }
let!(:member) { create(:project_member, source: project) }
context 'when current user cannot destroy the given member' do
before do
project.team << [user, :developer]
end
it 'does not destroy the member' do
expect(destroy_member(member, user)).not_to be_destroyed
end
end
context 'when current user can destroy the given member' do
before do
project.team << [user, :master]
end
it 'destroys the member' do
expect(destroy_member(member, user)).to be_destroyed
end
context 'when the given member is a requester' do
before do
member.update_column(:requested_at, Time.now)
end
it 'calls Member#after_decline_request' do
expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member)
destroy_member(member, user)
end
end
end
def destroy_member(member, user)
Members::DestroyService.new(member, user).execute
end
end
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