spree / deface

Rails plugin that allows you to customize ERB views in a Rails application without editing the underlying view.
MIT License
520 stars 127 forks source link

Load order of overrides on different systems #162

Open gaqzi opened 8 years ago

gaqzi commented 8 years ago

I recently noticed a weird behavior of Deface when one of our overrides was doing text: '<%= render partial: "...." %>' instead of partial: '.....'.

We only noticed this issue when it got into production on Linux and figured it would be something with sort order and started looking at how Deface loads the modules.

On my Mac I'm always seeing the overrides being returned in the same order, alphabetical. It also performs the overrides in alphabetical order when I have partial as the action, but if I change it to text it'll be in a semi-random order.

To help debug this I did some monkey patching which shows the order (and some testing with setting the order seems to make it pass reliably on Heroku):

Deface::Override.class_eval do
  def self.find(details)
#       .sort { |a, b| [a.sequence, a.name] <=> [b.sequence, b.name] }
    super(details)
      .tap do |value|
        Rails.logger.error("Order of deface being put in: '#{value.map &:name}'") unless value.blank?
      end
  end
end

The default, expected, order: Order of deface being put in: '["add_terms_checkbox", "make_checkout_update_buttons_big_on_mobile", "make_continue_shopping_button_big_on_mobile", "remove_empty_cart_button", "replace_cart_table_with_responsive_containers"]' The out of order: Order of deface being put in: '["make_checkout_update_buttons_big_on_mobile", "make_continue_shopping_button_big_on_mobile", "remove_empty_cart_button", "replace_cart_table_with_responsive_containers", "add_terms_checkbox"]'

For these tests, none of these overrides have a sequence. To work around this and to ensure it continues to work I'm adding in sequence to these templates.

All of this is a bit iffy as I've not been able to understand what's going on, and even worse, all the tests were passing on our CI which is also running on Linux (but whatever the difference between CircleCi and Heroku is…). And whether the order from find is changing stuff I can't make out. But after having put in .sort { |a, b| [a.sequence, a.name] <=> [b.sequence, b.name] } to my patched version of find it seems to have been doing better.

My suggestion would be to add in the sort above as it would make the load order consistent between different operating systems.