{"action":"opened","issue":{"url":"https://api.github.com/repos/SumOfUs/Champaign/issues/786","repository_url":"https://api.github.com/repos/SumOfUs/Champaign","labels_url":"https://api.github.com/repos/SumOfUs/Champaign/issues/786/labels{/name}","comments_url":"https://api.github.com/repos/SumOfUs/Champaign/issues/786/comments","events_url":"https://api.github.com/repos/SumOfUs/Champaign/issues/786/events","html_url":"https://github.com/SumOfUs/Champaign/issues/786","id":193649599,"number":786,"title":"Refactoring the Braintree payment processor code","user":{"login":"Tuuleh","id":6524052,"avatar_url":"https://avatars.githubusercontent.com/u/6524052?v=3","gravatar_id":"","url":"https://api.github.com/users/Tuuleh","html_url":"https://github.com/Tuuleh","followers_url":"https://api.github.com/users/Tuuleh/followers","following_url":"https://api.github.com/users/Tuuleh/following{/other_user}","gists_url":"https://api.github.com/users/Tuuleh/gists{/gist_id}","starred_url":"https://api.github.com/users/Tuuleh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Tuuleh/subscriptions","organizations_url":"https://api.github.com/users/Tuuleh/orgs","repos_url":"https://api.github.com/users/Tuuleh/repos","events_url":"https://api.github.com/users/Tuuleh/events{/privacy}","received_events_url":"https://api.github.com/users/Tuuleh/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2016-12-06T00:22:02Z","updated_at":"2016-12-06T00:22:02Z","closed_at":null,"body":"# Refactoring the payment processor code a tad bit (Braintree)\r\n## 1. Associations\r\nOver 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`.\r\n- One-off transactions would belong the action that is created when the transaction is created\r\n- 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`\r\n- Subscriptions don't really need to be associated with pages because 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.\r\n\r\n### Wai?\r\nOne-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`:\r\n```ruby\r\ndef transact\r\n        @result = ::Braintree::Transaction.sale(options)\r\n        if @result.success?\r\n          @action = ManageBraintreeDonation.create(params: @user.merge(page_id: @page_id), braintree_result: @result, is_subscription: false, store_in_vault: @store_in_vault)\r\n          Payment::Braintree.write_transaction(@result, @page_id, @action.member_id, existing_customer, store_in_vault: @store_in_vault)\r\n        else\r\n          Payment::Braintree.write_transaction(@result, @page_id, nil, existing_customer, store_in_vault: @store_in_vault)\r\n        end\r\n      end\r\n``` \r\n(`ManageBraintreeDonation` includes `ActionBuilder` and creates the action).\r\n\r\nThere 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.\r\n\r\n#### Associating transactions with their actions and dropping the page_id column\r\nActions 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.\r\n\r\n@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.\r\n\r\nThe 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. \r\n\r\n## 2. Other stuff\r\n- 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`). \r\n- 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:\r\n`Payment::Braintree.write_transaction`:\r\n```ruby\r\n    def write_transaction(bt_result, page_id, member_id, existing_customer, save_customer = true, store_in_vault: false)\r\n      BraintreeTransactionBuilder.build(bt_result, page_id, member_id, existing_customer, save_customer, store_in_vault: store_in_vault)\r\n    end\r\n```\r\n`Payment::Braintree.write_subscription`:\r\n```ruby\r\ndef write_subscription(payment_method_id, customer_id, subscription_result, page_id, action_id, currency)\r\n      if subscription_result.success?\r\n        Payment::Braintree::Subscription.create(payment_method_id:      payment_method_id,\r\n                                                customer_id:            customer_id,\r\n                                                subscription_id:        subscription_result.subscription.id,\r\n                                                amount:                 subscription_result.subscription.price,\r\n                                                merchant_account_id:    subscription_result.subscription.merchant_account_id,\r\n                                                billing_day_of_month:   subscription_result.subscription.billing_day_of_month,\r\n                                                action_id:              action_id,\r\n                                                currency:               currency,\r\n                                                page_id:                page_id)\r\n      end\r\n    end\r\n```\r\nThere 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.\r\n"}}
{"actor":{"display_login":"Tuuleh"}}