I've been working on a large Ruby on Rails application for several years. It was inherited in a poor state but most of the production bugs have been ironed out with time. There are some sections that have not been touched such as the payment processing code. The code works for the most part, except that whenever a charge is denied by the payment processor, the user gets a 500 error instead of a helpful message. I'd like to refactor the code to make it easier to maintain. I'll provide a brief run-down of how it works.
I've removed all the error-handling code from the following snippets.
The maze begins in a controller:
def submit_credit_card ... @credit_card = CreditCard.new(params[:credit_card].merge(:user => @user)) @credit_card.save ... @submission.do_initial_charge(@user) ... end Then in the Submission model:
def do_initial_charge(user) ... self.initial_charge = self.charges.create(:charge_type => ChargeType.find(1), :user => user) self.initial_charge.process! self.initial_charge.settled? end In the Charge model:
aasm column: 'state' do ... event :process do transitions :from => [:created, :failed], :to => :settled, :guard => :transaction_successful? end ... end def initialize(*params) super(*params) ... self.amount = self.charge_type.amount end def transaction_successful? user.reload credit_card = CreditCard.where(user_id: user_id).last cct = self.cc_transactions.build(:user => user, :credit_card => credit_card, :cc_last_four => credit_card.num_last_four, :amount => amount, :charge_id => id) cct.process! if self.last_cc_transaction.success self.update_attribute(:processed, Time.now) return true else self.fail! return false end end There are a lot of questionable bits above such as reloading the user and finding the last CreditCard rather than passing in the one just saved. Also this code depends on a ChargeType loaded from the database with a hard-coded ID.
In CcTransaction we continue down the trail:
def do_process response = credit_card.process_transaction(self) self.authorization = response.authorization self.avs_result = response.avs_result[:message] self.cvv_result = response.cvv_result[:message] self.message = response.message self.params = response.params.inspect self.fraud_review = response.fraud_review? self.success = response.success? self.test = response.test self.response = response.inspect self.save! self.success end All this appears to do is save a record in the cc_transactions database table. The actual payment processing is performed in the CreditCard model. I won't bore you with the details of that class. The actual work is performed by ActiveMerchant::Billing::AuthorizeNetCimGateway.
So we have at least 5 models involved (Submission, Charge, ChargeType, CcTransaction, and CreditCard). If I were to do this from scratch, I would only use a single Payment model. There are only 2 charge types, so I would hard code those values as class variables. We don't store credit card details, so that model is unnecessary. Transaction info can be stored in the payments table. Failed payments do not need to be saved.
I could go in and do this refactoring fairly easily except for the requirement that nothing should ever go wrong on the production server. Each of the redundant classes has many methods that could be called from anywhere in the code base. There is a suite of integration tests but the coverage is not 100%.
How should I go about refactoring this while ensuring nothing breaks? If I went through the 5 payment classes and greped every method to find out where they're called there's a high probability I will miss something. The client is already used to how the current code runs and introducing any new bugs is unacceptable. Apart from increasing test coverage to 100%, is there any way to refactor this with certainty that nothing will break?
AASM::InvalidTransition: Event 'process' cannot transition from 'failed'exception which masks the real error which is an unsuccessful transaction. There is so much indirection it's hard to get the response back to the user and allow a resubmission. I'm sure it's possible but it seems almost as difficult as refactoring.