Jamal Soueidan
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”]) { ?> > <? } ?>

Ember.js link-to and action helper

I just want to point out the difference between link-to and action is:

link-to is used with conjunction for routes

App.Router.map(function() {
  this.route('login', {path: "/"});
});

{{#link-to "login"}}Login{{/link-to}}
  

whereas action is only used for dispatching events in controllers, and they bubble all the way up to the route.

{{action "confirm"}}

The action helper can be used inside a or form tag.

The #1 reason why people give up so fast, is because they tend to look at how fare they still have to go instead of how fare they have gotten.
Vaibhav Shah