Skip to content

Commit

Permalink
misc: Remove the need for BaseService current user argument (#2660)
Browse files Browse the repository at this point in the history
## Context

The `BaseService#initialize` method is expecting a first argument of
type `User` that is then assigned to the created `result`. This argument
is not used in any places (last references were removed in previous PRs)

## Description

This PR simply removes this reference, and refactors the
`::PaymentProviders::DestroyService` that was the last remaining service
relying on it.
  • Loading branch information
vincent-pochet authored Oct 7, 2024
1 parent a33c113 commit 2176404
Show file tree
Hide file tree
Showing 6 changed files with 44 additions and 26 deletions.
8 changes: 5 additions & 3 deletions app/graphql/mutations/payment_providers/destroy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@ class Destroy < BaseMutation
field :id, ID, null: true

def resolve(id:)
result = ::PaymentProviders::DestroyService
.new(context[:current_user])
.destroy(id:)
payment_provider = ::PaymentProviders::BaseProvider.find_by(
id:,
organization_id: context[:current_user].organization_ids
)
result = ::PaymentProviders::DestroyService.call(payment_provider)

result.success? ? result.payment_provider : result_error(result)
end
Expand Down
3 changes: 1 addition & 2 deletions app/services/base_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,9 @@ def self.call_async(*, **, &)
end
end

def initialize(current_user = nil)
def initialize(args = nil)
@result = Result.new
@source = CurrentContext&.source
result.user = current_user.is_a?(User) ? current_user : nil
end

def call(**args, &block)
Expand Down
16 changes: 11 additions & 5 deletions app/services/payment_providers/destroy_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@

module PaymentProviders
class DestroyService < BaseService
def destroy(id:)
payment_provider = PaymentProviders::BaseProvider.find_by(
id:,
organization_id: result.user.organization_ids
)
def initialize(payment_provider)
@payment_provider = payment_provider

super
end

def call
return result.not_found_failure!(resource: 'payment_provider') unless payment_provider

customer_ids = payment_provider.customer_ids
Expand All @@ -21,5 +23,9 @@ def destroy(id:)
result.payment_provider = payment_provider
result
end

private

attr_reader :payment_provider
end
end
21 changes: 21 additions & 0 deletions spec/graphql/mutations/payment_providers/destroy_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,25 @@
data = result['data']['destroyPaymentProvider']
expect(data['id']).to eq(payment_provider.id)
end

context 'when payment provider is not attached to the organization' do
let(:payment_provider) { create(:stripe_provider) }

it 'returns an error' do
result = execute_graphql(
current_user: membership.user,
permissions: required_permission,
query: mutation,
variables: {
input: {id: payment_provider.id}
}
)

aggregate_failures do
expect(result['errors'].first['message']).to eq('Resource not found')
expect(result['errors'].first['extensions']['code']).to eq('not_found')
expect(result['errors'].first['extensions']['status']).to eq(404)
end
end
end
end
5 changes: 2 additions & 3 deletions spec/services/base_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,9 @@

context 'with current_user' do
it 'assigns the current_user to the result' do
user = create(:user)
result = described_class.new(user).send :result
result = described_class.new.send :result

expect(result.user).to eq(user)
expect(result).to be_a(::BaseService::Result)
end

it 'does not assign the current_user to the result if it isn\'t a User' do
Expand Down
17 changes: 4 additions & 13 deletions spec/services/payment_providers/destroy_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require 'rails_helper'

RSpec.describe PaymentProviders::DestroyService, type: :service do
subject(:destroy_service) { described_class.new(membership.user) }
subject(:destroy_service) { described_class.new(payment_provider) }

let(:membership) { create(:membership) }
let(:organization) { membership.organization }
Expand All @@ -14,24 +14,15 @@
before { payment_provider }

it 'destroys the payment_provider' do
expect { destroy_service.destroy(id: payment_provider.id) }
expect { destroy_service.call }
.to change(PaymentProviders::BaseProvider, :count).by(-1)
end

context 'when payment provider is not found' do
it 'returns an error' do
result = destroy_service.destroy(id: nil)

expect(result).not_to be_success
expect(result.error.error_code).to eq('payment_provider_not_found')
end
end

context 'when payment provider is not attached to the organization' do
let(:payment_provider) { create(:stripe_provider) }
let(:payment_provider) { nil }

it 'returns an error' do
result = destroy_service.destroy(id: payment_provider.id)
result = destroy_service.call

expect(result).not_to be_success
expect(result.error.error_code).to eq('payment_provider_not_found')
Expand Down

0 comments on commit 2176404

Please sign in to comment.