Refactoring the Braintree payment processor code

[Tuuleh]

Refactoring the payment processor code a tad bit (Braintree)

1. Associations

Over time, we've cleaned up the associations and there isn't so much to do there any more, but I would still associate transactions with actions instead of pages.

• One-off transactions would belong the action that is created when the transaction is created
• Subscription transactions would either belong to the action their parent subscription belongs to, or would have a nil action_id, because the action and its page can be accessed through transaction.subscription.action.page_id
• Subscriptions don't really need to be associated with pages because they're already associated with actions, and their page can be accessed through the action with subscription.action.page. There are 113 subscriptions that don't belong to actions on production, the first one created October 12th 2016, and the last one 12 days later, so that must have been a temporary fluke - but these will need to be fixed as the payment data will not be recorded right on AK for these. (I fixed these today.)

Wai?

One-off donations are a kind of a type of action, and an action is always created when a one-off transaction is made, so the association is pretty clear. The order of creation also supports this as it is right now, in PaymentProcessor::Braintree::Transaction#transact:

def transact
        @result = ::Braintree::Transaction.sale(options)
        if @result.success?
          @action = ManageBraintreeDonation.create(params: @user.merge(page_id: @page_id), braintree_result: @result, is_subscription: false, store_in_vault: @store_in_vault)
          Payment::Braintree.write_transaction(@result, @page_id, @action.member_id, existing_customer, store_in_vault: @store_in_vault)
        else
          Payment::Braintree.write_transaction(@result, @page_id, nil, existing_customer, store_in_vault: @store_in_vault)
        end
      end

(ManageBraintreeDonation includes ActionBuilder and creates the action).

There are a classes/methods in handling BT transactions with mega long parameter lists and while just passing an action instead of page_id and member_id would simplify things, that isn't enough to trim things down.

Associating transactions with their actions and dropping the page_id column

Actions to be associated with a customer's transactions on a given page can be found through Action.where(page_id: transaction.page_id, member_id: transaction.customer.member_id). In most cases this'll net just one action, as people tend to make just one transaction on a given page. But in case it nets more than one action, they can be associated with the transactions on the page using their created_at field.

@osahyoun you mentioned that you wanted to have a simple way of getting the transactions of a page, e.g. page.transactions. This could be done with a method that calls Payment::Braintree::Transaction.find(action_id: self.actions.donation.pluck(:id)) or possibly something less hideous but with similar logic :) I'm not actually sure how we're doing this now because there isn't a has_many association from pages to transactions.

The code to associate transactions and subscriptions with actions instead of pages needs to go live first with the page_id column still present. Afterwards can then run the script to associate the existing transactions and subscriptions and drop the column.

2. Other stuff

• I'd like to rethink, or at least rename ManageBraintreeDonation, which is used for creating an action off of a one-off Braintree donation. It looks mighty complicated considering that there are only 8 fields for Action, two of which seem to be duplicates (subscribe_member and subscribed_user).
• There are some mighty long parameter lists that I think we should try to mitigate by seeing whether a given parameter is really necessary, and by passing parameter objects instead. Examples: Payment::Braintree.write_transaction:

  def write_transaction(bt_result, page_id, member_id, existing_customer, save_customer = true, store_in_vault: false)
    BraintreeTransactionBuilder.build(bt_result, page_id, member_id, existing_customer, save_customer, store_in_vault: store_in_vault)
  end

  Payment::Braintree.write_subscription:

  def write_subscription(payment_method_id, customer_id, subscription_result, page_id, action_id, currency)
    if subscription_result.success?
      Payment::Braintree::Subscription.create(payment_method_id:      payment_method_id,
                                              customer_id:            customer_id,
                                              subscription_id:        subscription_result.subscription.id,
                                              amount:                 subscription_result.subscription.price,
                                              merchant_account_id:    subscription_result.subscription.merchant_account_id,
                                              billing_day_of_month:   subscription_result.subscription.billing_day_of_month,
                                              action_id:              action_id,
                                              currency:               currency,
                                              page_id:                page_id)
    end
  end

  There are lots of small improvements that could be made - it'd be great if someone else was up for going through this and brainstorming together / pair programming (!!!), and I'd appreciate it if you could have a look at the BT transaction and subscription handling classes to see if you can identify areas you'd like to improve.

[osahyoun]

Associating transactions with their actions

If we have the action then we also have the page, so it wouldn't be so hard to keep that association... it would make page.transactions a much less expensive operation to run when a page has a large set of transactions. What do you think?

[osahyoun]

2. Other stuff

I agree with everything under this section. Would like to pair on this with you, if you fancy it. Perhaps when you're this side of the pond.