Benjamin Roth

@apneadiving

ruby & javascript freelancer

We all faced or still face issues with our code.

 

Lets see some I came across.

 

 

Statement

  def paypal_event
    uri = URI.parse(ENV['PAYPAL_IPN_URL'])
    http = Net::HTTP.new(uri.host, uri.port)
    http.open_timeout = 60
    http.read_timeout = 60
    http.verify_mode = OpenSSL::SSL::VERIFY_NONE
    http.use_ssl = true
    response = http.post(uri.request_uri, request.raw_post,
      'Content-Length' => "#{request.raw_post.size}",
      'User-Agent' => "My custom user agent"
    ).body

    case response
    when "VERIFIED"
      # The receiver email has to equal the PayPal receiving email set as environment variable
      # The period type, the payment cycle and the recurring payment are set
      if params[:receiver_email] === ENV['PAYPAL_EMAIL'] && params[:period_type] && params[:payment_cycle] && params[:recurring_payment_id]
        # Find the user subscription
        user_subscription = UserSubscription.find_by(pay_id: params[:recurring_payment_id])
        render nothing: true and return unless user_subscription.present?
        # Find the user and the subscription
        user = User.find(user_subscription.user_id)

        # If the transaction type is set
        if params[:txn_type].present? && params[:txn_type] === 'recurring_payment_suspended_due_to_max_failed_payment'
          # Setup PayPal request
          request = Paypal::Express::Request.new(
            username:  ENV['PAYPAL_USER'],
            password:  ENV['PAYPAL_PWD'],
            signature: ENV['PAYPAL_SIGNATURE']
          )
          # Cancel the PayPal subscription
          request.renew!(user_subscription.pay_id, :Cancel)
          if user_subscription.active?
            # Cancel the subscription
            user_subscription.deactivate
            user_subscription.save
          end
        end

        # If payment status and transaction id are set and the transaction id has not been previously processed
        if !params[:payment_status].nil? && !params[:txn_id].nil? && (params[:txn_id] != session[:txn_id])
          # Set the current transaction id
          session[:txn_id] = params[:txn_id]
          # If the payment status is completed
          if params[:payment_status] === 'Completed'
            if user.present? && user_subscription.present?
              # Create a new receipt
              receipt = Receipt.create(
                user_id: user.id,
                charge_id: user_subscription.pay_id,
                data: params,
                totalUSD: params[:mc_gross],
                status: 'paid',
                paid_at: Time.now
              )
              user_subscription.receipts << receipt
            end

            # Set the duration depending on the payment cycle
            subscription_duration = 1.month
            subscription_duration = 1.year if params[:payment_cycle] === 'Yearly'

            # If the subscription was cancelled (because a payment failed in the past), make sure to reactivate it.
            user_subscription.reactivate if(user_subscription.cancelled? || user_subscription.set_for_cancellation?)

            # Set the expiration of the subscription further
            user_subscription.expiration = Time.now + subscription_duration + 2.weeks
            user_subscription.delay(run_at: (user_subscription.expiration + 1.minute)).deactivate_expiration_check
            user_subscription.save

          # If the payment status is refunded or partially refunded
          elsif (params[:payment_status] === 'Refunded') || (params[:payment_status] === 'Partially_Refunded')
            # Find the receipt
            charge_id = params[:parent_txn_id]
            receipt = Receipt.find_by_charge_id(charge_id)

            if receipt.present?
              # params[:mc_gross] is negative so add it to the current total
              new_amount = receipt.totalUSD + params[:mc_gross].to_f
              receipt.totalUSD = new_amount
              receipt.save
            end

          # If the payment status is denied, expired, failed or reversed
          elsif (params[:payment_status] === 'Denied') || (params[:payment_status] === 'Expired') || (params[:payment_status] === 'Failed') || (params[:payment_status] === 'Reversed')
            # Setup PayPal request
            request = Paypal::Express::Request.new(
              username:  ENV['PAYPAL_USER'],
              password:  ENV['PAYPAL_PWD'],
              signature: ENV['PAYPAL_SIGNATURE']
            )

            # Cancel the PayPal subscription
            request.renew!(user_subscription.pay_id, :Cancel)

            # Cancel the subscription
            if user_subscription.active?
              user_subscription.deactivate
              user_subscription.save
            end
          end
        end
      end
    end
    render nothing: true
  end

Huge controller action

 

(yes it handles payment)

  before_action :set_user_subscription

  def pause
    months = params[:months] || 1
    # If "forever" then cancel subscription
    if months == "forever"
      deactivate_if_active(@user_subscription)
      cancel_subscription_tracking(@user_subscription)
      flash[:notice] = "Your subscription has been cancelled."
      redirect_to account_settings_users_path(anchor: "tab_subscriptions") and return
    end

    if months == "now"
      if @user_subscription.active?
        flash[:notice] = "Your subscription is already active."
        redirect_to account_settings_users_path(anchor: "tab_subscriptions") and return
      end
      if @user_subscription.inactive?
        begin
          @user_subscription.reactivate_with_old_payment_info
        rescue => e
          puts "ERROR: in reactivate_with_old_payment_info"
          puts "#{e.message}"
          notify_airbrake(e)
          flash[:error] = "There was an error reactivating your subscription. Please contact support@datacamp.com"
          redirect_to account_settings_users_path(anchor: "tab_subscriptions") and return
        end
      end
      reactivate_tracking
      flash[:notice] = "Your subscription has been successfully reactivated."
      redirect_to account_settings_users_path(anchor: "tab_subscriptions") and return
    end

    if @user_subscription.active? || @user_subscription.set_for_cancellation?
      @user_subscription.pause
      pause_tracking(@user_subscription, months)
      flash[:notice] = "Your subscription has been successfully paused."
    elsif @user_subscription.set_for_pause? || @user_subscription.paused?
      extend_pause_tracking(@user_subscription, months)
      flash[:notice] = "Your subscription pause has been successfully updated."
    end
    next_billing_date = next_billing_date(@user_subscription)
    if next_billing_date.blank?
      flash[:error] = "We encountered an error processing your subscription. Please contact support@datacamp.com."
      redirect_to account_settings_users_path(anchor: "tab_subscriptions") and return
    end

    @user_subscription.set_reactivation_check(next_billing_date.to_date + months.to_i.months)
    redirect_to account_settings_users_path(anchor: "tab_subscriptions")
  end

  private

  def set_user_subscription
    @user_subscription = current_user.current_subscription
    if @user_subscription.blank?
      flash[:error] = "We couldn't find a subscription linked to your account. Please contact support@datacamp.com"
      redirect_to account_settings_users_path(anchor: "tab_subscriptions") and return
    end
  end

  def pause_tracking(user_subscription, months)
    Analytics.track_event(current_user, cookies, 'Subscription Paused')
    track_event("Subscription Paused", "main", "subscription", properties: { months: months }, uid: current_user.id)
  end

  def extend_pause_tracking(user_subscription, months)
    Analytics.track_event(current_user, cookies, 'Subscription Pause Extended')
    track_event("Subscription Pause Extended", "main", "subscription", properties: { months: months }, uid: current_user.id)
  end

  def cancel_subscription_tracking(user_subscription)
    Analytics.track_event(current_user, cookies, 'Subscription Cancelled')
    track_event("Subscription Cancelled", "main", "subscription", uid: current_user.id)
  end

  def reactivate_tracking
    Analytics.track_event(current_user, cookies, 'Subscription Reactivated Manually')
    track_event("Subscription Reactivated Manually", "main", "subscription", uid: current_user.id)
  end

  def deactivate_if_active(user_subscription)
    if user_subscription.active? || user_subscription.set_for_pause? || user_subscription.paused?
      # Cancel subscription
      user_subscription.deactivate
      user_subscription.save
    end
  end

  def next_billing_date(user_subscription)
    return nil if user_subscription.pay_method != "braintree"
    subscription = Braintree::Subscription.find(user_subscription.pay_id)
    if subscription.nil? || subscription.next_billing_date.nil?
      return 'NA'
    else
      return subscription.next_billing_date
    end
  end

Huge controller action

(in disguise)

class ApplicationController < ActionController::Base
  include FlashMessagesHelper
  include NumbersHelper
  include ProfilingHelper
  include SessionHelper
  include AppconfigsHelper
  include Trackable
  include Lovable::Controller
  include Pundit
  protect_from_forgery
  helper_method :mixpanel_distinct_id
  helper_method :default_after_sign_in_redirect_path
  helper_method :xeditable?
  helper :all
  before_filter :log_ram_before
  before_filter do
    if current_user.blank? && Tenant.app_config('sso_configured') == true
      redirect_to saml_init_path
    else
      authenticate_user!
    end
  end
  before_filter :set_paper_trail_whodunnit
  before_filter :assign_bunch_of_stuff, if: lambda { !!params.fetch('sign_in_via_token', false) }
  before_filter :prepare_filter_data, except: [:after_sign_in_path_for, :set_user_variables]
  before_filter :config_base_currency_id
  before_filter :set_locale
  before_filter :set_global_scope
  before_filter :detect_ie_users
  before_filter do
    NewRelic::Agent.add_custom_parameters({ tenant: Apartment::Tenant.current, user_id: session[:user_id] })
  end
  after_filter :flash_to_headers
  after_filter :log_ram_after
  after_filter :count_objects
  around_filter :profile

Application Controller madness

But...

So...

Good practices reminder

Sometimes its just a matter of context

class ApplicationController

  before_filter :authenticate_user!

end

class PublicController < ApplicationController

  skip_before_action :authenticate_user!

end

class PrivateController < ApplicationController

end
class ApplicationController

end

class LoggedAreaController < ApplicationController

  before_filter :authenticate_user!

end

class PublicController < ApplicationController

end

class PrivateController < LoggedAreaController

end

Would avoid

Would do

Or leaked logic...

<% if user.reputation > 500 %>
  ...
<% end %>
<% if user.with_high_reputation? %>
  ...
<% end %>
# in some controller
user.active = true
# in some controller
user.set_active

Would avoid

Would do

Or bad practices...

def with_high_reputation?
  reputation > 500
end
def with_high_reputation?
  reputation > HIGH_REPUTATION_SCORE
end

Would avoid

Would do

or maybe useless callbacks

before_save :downcase_email

def downcase_email
  self.email = email.respond_to?(:downcase) ? email.downcase : email
end
def email=(value)
  super value.respond_to?(:downcase) ? value.downcase : value
end

Would avoid

Would do

Callbacks ain't right

 

~ Me

Personal quest

Use Callbacks &

I won't call you back

 

~ Bob Cat

Personal quest

Don't you dare...

 

~ Posh Dog

Personal quest

What we'll focus on today

Extract contexts!

13:50 - 14:45 

14:45 - 14:55

14:55 - 16:00

BREAK!

Basic Implementation

Advanced Implementation

  def accept
    if invitation.accepted?
      render json: { errors: ['Invitation already accepted'] }, status: 422
    else
      user = User.create_from_invitation(invitation)
      invitation.accepted = true
      invitation.save
      render json: { user: user }
    end
  end

Create user

from invitation

Change invitation status

What do you think?

In case you didn't notice

(because you can't really)

Create user

from invitation

Change invitation status

Pay referrer

In case you didn't notice

(because you can't really)

Create user

from invitation

Change invitation status

Pay referrer

1- warm up: signup

related spec:  github

possible implementation: github

 

Notice: we don't handle failure (yet)

2- warm up: create invitation

related spec:  github

possible implementation: github

 

 

related spec:  github

possible implementation: github

 

 

related spec:  github

possible implementation: github

 

 

related spec:  github

possible implementation: github

 

 

6- Are we good?

No, at all.

 

Uncomment specs

xcontext 'errors happen'

7- use boolean services

They should respond to:

- success?

- issues

 

 

 

Warning! if and even nested if are coming

 

A solution: github

Another approach

Chaining

@thread = Thread.new(thread_params)
if @thread.save
  @message = thread.messages.build(msg_params)
  if @message.save
    render json: { message: @message }
  else
    render_errors(@message.errors)
  end
else
  render_errors(@thread.errors)
end    

Example

chain do
  @thread = Thread.new(thread_params)
  dam(@thread.errors) unless @thread.save
end
chain do
  @message = @thread.messages.build(msg_params)
  dam(@message.errors) unless @message.save
end
chain { render json: { message: @message } }
on_dam {|error_pool| render_errors(error_pool) }

8- use waterfall

Usage: github

A solution: github

Made with Slides.com