Jamal Soueidan
Seek first to understand, then to be understood.
Stephen R. Covey
Clean up SASS

Well well, now it’s time to refactor a little bit of our SCSS stylesheet.

We would like to add more colors everytime and then, but the solution is now is to too add more duplicated code, copy and paste from the old color example.

@mixin color($property) {
  color: $property;
}

$color-green: #1b9773;
.color-green {
  @include color($color-green);
}

$color-black: black;
.color-black {
  @include color($color-black);
}

$color-white: white;
.color-white {
  @include color($color-white);
}

As you see in this code we have allot of code that is duplicated, let’s refactor it.

$color-names: "green", "black", "white", "spring-wood";
$color-hex: #1b9773, black, white, #fcfbf9;

@function get-color($name) {
    $index: index($color-names, $name);
    @return nth($color-hex, $index);
}

@for $i from 1 through length($color-names) {
    .color-#{nth($color-names, $i)} {
        color: nth($color-hex, $i);
    }
}

This is just one solution, but as you see, we don’t have any code that are duplicated, we can just add the color name to $color-names and the hex color to $color-hex. Easy and simple, no need to do anymore.

If we would like to use any color in any other stylesheet declaration, we can simple use the get-color function like this.

.header {
    color: get-color("green");
}
Move values to HTML data-attribute.

Here we have another example where we can refactor the javascript code.

We will split the javascript in two so it’s easier to get a look at it.

Here is the event method.

var switch_gift_details_display;

switch_gift_details_display = function(el, method) {
  var container;
  $(el).addClass('active').siblings('a.active').removeClass('active');
  hide_fields_overlay(el);
  container = $(el).parents('.gift_details_container');
  container.find('form .buttons input.hidden').val(method);
  if (method === 2) {
    container.find('.shipping_details').fadeIn();
    container.find('.service_message_container').fadeIn();
  } else {
    container.find('.shipping_details').fadeOut();
    container.find('.service_message_container').fadeOut();
  }
  if (method === 0) {
    return container.find('.recipient_details div.input.email').fadeIn();
  } else {
    return container.find('.recipient_details div.input.email').fadeOut();
  }
};

and here we attach click events for different elements on HTML page.

$(document).on('click', '.delivery_method_deliver_by_mail', function(event) {
  event.preventDefault();
  return switch_gift_details_display(this, 0);
});

$(document).on('click', '.delivery_method_print_giftcard_self', function(event) {
  event.preventDefault();
  return switch_gift_details_display(this, 1);
});

$(document).on('click', '.delivery_method_physical_gift', function(event) {
  event.preventDefault();
  return switch_gift_details_display(this, 2);
});

$(document).on('click', '.delivery_method_physical_gift_pickup', function(event) {
  event.preventDefault();
  return switch_gift_details_display(this, 3);
});

As you can see in the above code, we listen on click events for different elements, and they have “numbers” for every delivery method, to refactor this, we move all these numbers to HTML data-attribute, since they belong there, and then we keep all the HTML elements inside one root element we call “delivery_methods”.

var delivery_methods = $('.form-gift .buttons.delivery_methods a')
delivery_methods.click(switch_delivery_methods)

Now we listen on click events on all elements in few linies, easier to ready and easier to maintain.

But what about the integer we have on the different events, so we know which delivery-method is chosen? Those we move to the HTML attributes data. (data-delivery-method=1) etc.

What’s left is the switch_gift_details_display method to be refactored, this shouldn’t be that hard.

function switch_delivery_methods(evt) {
  evt.preventDefault();

  var link = $(this)
  var form = $(this).closest('form')
  var input = form.find('input[id="gift_details_delivery_method"]') // hidden input field
  var value = link.data('delivery-method') // which type of delivery-method is clicked?

  input.val(value); //set delivery-method choosen

  link.parent().removeClass("active");
  link.addClass('active')

  var shipping_details = form.find('.shipping_details')
  shipping_details.hide();

  var email_input = form.find('.recipient_details div.input.email')
  email_input.hide();

  switch(value) {
    case 0:
      email_input.show();
    break;
    case 2:
      shipping_details.show();
    break;
  }
}
The biggest communication problem is we do not listen to understand.
We listen to reply.
Everything related to frontend stays there.

Here is a example of bad practice code, we should not use helper methods in a controller, anything related to frontend move it there.

Let’s use Extract Method Refactoring.

def redeem
  redeem_code = RedeemCodeParser.parse_string_to_redeem_numbers_hash(params[:redemption][:redeem_code])

  if redeem_code
    reference_number = redeem_code[:reference_number]
    code             = redeem_code[:code]

    voucher = current_retailer_vouchers.readonly(false).find_by_reference_number_and_verification_code(reference_number, code)
    msg = voucher.nil? ? :unknown_voucher : voucher.redeem_by(current_user, code)
  else
    msg = :unknown_voucher
  end

  if msg == :success
    reset_throttle
    flash_message = flash_t('.success', :reference_number => params[:redemption][:redeem_code])

    flash_message += content_tag(:p, flash_t('.deal', :title => voucher.order_line.title), :class => 'extra')

    if voucher.order_line.deal_variation
      flash_message += content_tag(:p, flash_t('.deal_variation', :variation => voucher.order_line.deal_variation.title), :class => 'extra')
    end

    if voucher.order_line.has_extra_detail_options?
      flash_message += content_tag(:p, flash_t('.extra_option', :extra_option => voucher.order_line.extra_detail_option), :class => 'extra')
    end

    redirect_to :back, :notice => flash_message.html_safe
  else
    redirect_to :back, :alert => flash_t(".#{msg.to_s}", :reference_number => params[:redemption][:redeem_code])
  end
end

You may remember we have refactored this method before, and we will do that again, but this time we will move all these flash_message stuff out to a partial.

%h3= t('.success', :reference_number => redeem_code)

%p.margin-top-12px= t('.deal', :title => voucher.order_line.title)

- if voucher.order_line.deal_variation
  %p= t('.deal_variation', :variation => voucher.order_line.deal_variation.title)

- if voucher.order_line.has_extra_detail_options?
  %p= t('.extra_option', :extra_option => voucher.order_line.extra_detail_option)

Next we will refactor the controller method, and remove everything related to frontend. Keep in mind we also add partial hash value to flash object. 

def redeem
  redeem_code = params[:redemption][:redeem_code]
  voucher = get_voucher(redeem_code)
  msg = :unknown_voucher

  if voucher
    msg = voucher.redeem_by(current_user, code)
  end

  if msg == :success
    reset_throttle
    flash[:partial] = render_to_string(:partial => 'redeem_success', :locals => {:voucher => voucher, :redeem_code => redeem_code})
  else
    flash[:alert] = flash_t(".#{msg.to_s}", :reference_number => params[:redemption][:redeem_code])
  end

  redirect_to :back
end

Now this is much better then before.

We may get back to this method and refactor it again :)

Step by Step Ruby Refactoring

Here is another example of refactoring some Ruby code, we have two different methods, which do almost the same thing.

Let’s use Extract Method Refactoring.

def redeem
  redeem_code = RedeemCodeParser.parse_string_to_redeem_numbers_hash(params[:redemption][:redeem_code])

  if redeem_code
    reference_number = redeem_code[:reference_number]
    code             = redeem_code[:code]

    voucher = current_retailer_vouchers.readonly(false).find_by_reference_number_and_verification_code(reference_number, code)
    msg = voucher.nil? ? :unknown_voucher : voucher.redeem_by(current_user, code)
  else
    msg = :unknown_voucher
  end

  if msg == :success
    reset_throttle
    flash_message = flash_t('.success', :reference_number => params[:redemption][:redeem_code])

    flash_message += content_tag(:p, flash_t('.deal', :title => voucher.order_line.title), :class => 'extra')

    if voucher.order_line.deal_variation
      flash_message += content_tag(:p, flash_t('.deal_variation', :variation => voucher.order_line.deal_variation.title), :class => 'extra')
    end

    if voucher.order_line.has_extra_detail_options?
      flash_message += content_tag(:p, flash_t('.extra_option', :extra_option => voucher.order_line.extra_detail_option), :class => 'extra')
    end

    redirect_to :back, :notice => flash_message.html_safe
  else
    redirect_to :back, :alert => flash_t(".#{msg.to_s}", :reference_number => params[:redemption][:redeem_code])
  end
end

def batch_redeem
  @codes = params[:vouchers].split("\n")
  @successful = []
  @unsuccessful = {}

  @codes.each do |code|
    parts = RedeemCodeParser.parse_string_to_redeem_numbers_hash(code)

    if parts == false
      @unsuccessful[code] = '.invalid_code'
    else
      reference_number  = parts[:reference_number]
      verification_code = parts[:code]

      voucher = current_retailer_vouchers.readonly(false).find_by_reference_number_and_verification_code(reference_number, verification_code)

      if voucher
        case voucher.redeem_by(current_user, verification_code)
        when :success
          @successful << voucher
        when :code_already_used
          @unsuccessful[code] = '.already_redeemed'
        when :no_longer_redeemable
          @unsuccessful[code] = '.expired'
        else
          @unsuccessful[code] = '.invalid_code'
        end
      else
        @unsuccessful[code] = '.invalid_code'
      end
    end
  end
end

As you can see redeem method accept only 1 code at the time, while batch_redeem method acccept many codes at time. Both methods parse the code, and try to get the voucher object etc. 

So here, we don’t remove the methods, but we take some of the code out to a new method which both call it. 

def get_voucher(redeem_code)
  redeem_code = RedeemCodeParser.parse_string_to_redeem_numbers_hash(redeem_code)
  if redeem_code
    reference_number  = redeem_code[:reference_number]
    verification_code = redeem_code[:code]

    return current_retailer_vouchers.readonly(false).find_by_reference_number_and_verification_code(reference_number, verification_code)
  end

  return false
end

Here we moved the code which is duplicated in both methods, and let those methods call the new get_voucher method.

The whole code looks like this.

def redeem
  voucher = get_voucher(params[:redemption][:redeem_code])
  msg = :unknown_voucher

  if voucher
    msg = voucher.redeem_by(current_user, code)
  end

  if msg == :success
    reset_throttle
    flash_message = flash_t('.success', :reference_number => params[:redemption][:redeem_code])

    flash_message += content_tag(:p, flash_t('.deal', :title => voucher.order_line.title), :class => 'extra')

    if voucher.order_line.deal_variation
      flash_message += content_tag(:p, flash_t('.deal_variation', :variation => voucher.order_line.deal_variation.title), :class => 'extra')
    end

    if voucher.order_line.has_extra_detail_options?
      flash_message += content_tag(:p, flash_t('.extra_option', :extra_option => voucher.order_line.extra_detail_option), :class => 'extra')
    end

    redirect_to :back, :notice => flash_message.html_safe
  else
    redirect_to :back, :alert => flash_t(".#{msg.to_s}", :reference_number => params[:redemption][:redeem_code])
  end
end

# TODO: Move some of this logic to the model.
def batch_redeem
  @codes = params[:redeem][:vouchers].split("\n")
  @successful = []
  @unsuccessful = {}

  @codes.each do |code|
    voucher = get_voucher(code)

    if voucher
      case voucher.redeem_by(current_user, verification_code)
      when :success
        @successful << voucher
      when :code_already_used
        @unsuccessful[code] = '.already_redeemed'
      when :no_longer_redeemable
        @unsuccessful[code] = '.expired'
      else
        @unsuccessful[code] = '.invalid_code'
      end
    else
      @unsuccessful[code] = '.invalid_code'
    end
  end
end
Refactor Ruby Methods

I’m working on merging the responsive framework I developed in the last month into our Rails app, while I do that I find some places that can be improved.

Here is a example where we can improve the code, we can refactor these 4 methods into 1 one method.

def download_vouchers_csv
  deal     = current_retailer_deals.find params[:deal_id]
  filename = "deal_#{deal.id}_vouchers_#{Time.zone.now.strftime('%Y-%m-%d_%H-%M-%S')}.csv"
  export   = DataExports::DealVouchersExport.new deal, :include_verification_code => deal.include_verification_code_in_retailer_exports,
                                                       :include_owner_email => deal.include_emails_in_retailer_exports

  send_data export.to_csv, :filename => filename, :type => 'text/csv'
end

def download_vouchers_pdf
  deal     = current_retailer_deals.find params[:deal_id]
  filename = "deal_#{deal.id}_vouchers_#{Time.zone.now.strftime('%Y-%m-%d_%H-%M-%S')}.pdf"
  export   = DataExports::DealVouchersExport.new deal, :include_verification_code => deal.include_verification_code_in_retailer_exports,
                                                       :include_owner_email => deal.include_emails_in_retailer_exports

  send_data export.to_pdf, :filename => filename, :type => :pdf
end

def download_shipping_list_csv
  deal     = current_retailer_deals.find params[:deal_id]
  filename = "deal_#{deal.id}_shipping_list_#{Time.zone.now.strftime('%Y-%m-%d_%H-%M-%S')}.csv"
  export   = DataExports::DealShippingListExport.new deal, :include_owner_email => deal.include_emails_in_retailer_exports

  send_data export.to_csv, :filename => filename, :type => 'text/csv'
end

def download_shipping_list_pdf
  deal     = current_retailer_deals.find params[:deal_id]
  filename = "deal_#{deal.id}_shipping_list_#{Time.zone.now.strftime('%Y-%m-%d_%H-%M-%S')}.pdf"
  export   = DataExports::DealShippingListExport.new deal, :include_owner_email => deal.include_emails_in_retailer_exports

  send_data export.to_pdf, :filename => filename, :type => :pdf
end

Before you start refactoring you must understand what the code is doing and most important what is equal for these methods.

Here we can say that allot is the same, and it’s easy to spot what needs to be done. 

Here is the refactored method.

def download
  filetype = "pdf"
  if params[:type] == "csv"
    filetype = "csv"
  end

  filename = ["deal_#{@deal.id}"]
  filename << params[:list]
  filename << Time.zone.now.strftime('%Y-%m-%d_%H-%M-%S')
  filename << "." + filetype

  options = {:include_owner_email => @deal.include_emails_in_retailer_exports}
  if params[:list] == "shipping"
    options[:include_verification_code] = @deal.include_verification_code_in_retailer_exports
  end

  export = DataExports::DealVouchersExport.new(@deal, options)
  send_data export.send("to_#{filetype}"), :filename => filename.join('_'), :type => "text/" + filetype
end

As you can see the download method accept type and list parameter, type can be either .pdf or .csv depending on what filetype the user wants to download, and list can be shipping og vouchers.

member do
  post :download_shipping_list_csv
  post :download_shipping_list_pdf
  post :download_vouchers_csv
  post :download_vouchers_pdf
end

We can just live with one download method now.

member do
  post :download
end

:)

Media queries and last-child working in IE8

Selectivizr and respond.JS both have been modified to work together, respond.js handle media queries to work in IE6-8 and selctivizr take care of all selectors etc.

  1. You first need selectivizr version 1.0.3b, since this is version that are integrated with respond.js. 

    https://raw.github.com/keithclark/selectivizr/master/selectivizr.js

  2. Respond is more prompt with releases, so the latest official version will do.


Note that Selectivizr should be included first, Respond second.

http://selectivizr.com/tests/respond/

image

Ember - Views access Controller property

Many times you want to access the property in the controller from the your view, or you want to call action on the controller from the view, this is done using get(‘controller’) like this.

App.NotesShowStep1View = Ember.View.extend({
  submit: function() {
    var seller = this.get('controller.seller')
    if (!seller) {
      //validate
      return;
    } 

    this.get('controller').send('save');
  }
})

Here we validate the seller if it’s set on the controller, if it’s not then we alert the user, or call an action on the controller using send method.

Bit-torrent XSS - PHP Source

I have list of security issues I discovered in the past in some source code or sites which I never made them public in the past. 

Date: 8/6/05

Link: sendmessage.php?receiver=2&returnto=><script>alert(‘a’)</script>

Problem:

XSS injection when you send files, this one does not show any php errors whatsoever. 

Filename: sendmessage.php

Solution:

$returnto = htmlentities($_GET[“returnto”]);
<? if (!empty($returnto)|| $_SERVER[“HTTP_REFERER”]) { ?> > <? } ?>