jsqu99 / spree_flexi_variants

Spree extension to create product variants as-needed
BSD 3-Clause "New" or "Revised" License
116 stars 143 forks source link

Error trying to edit order #91

Open jalberto opened 10 years ago

jalberto commented 10 years ago
, [2013-12-19T10:12:51.548328 #5549]  INFO -- : Completed 500 Internal Server Error in 81ms                                                                       [131/9267]
F, [2013-12-19T10:12:51.555352 #5549] FATAL -- : 
ActionView::Template::Error (wrong number of arguments (1 for 3)):
    1: <% shipment.manifest.each do |item| %>
    2:   <% line_item = order.find_line_item_by_variant(item.variant) %>
    3: 
    4:   <tr class="stock-item" data-item-quantity="<%= item.quantity %>">
    5:     <td class="item-image"><%= mini_image(item.variant) %></td>
  /home/deploy/pequecanciones/shared/bundle/ruby/2.0.0/bundler/gems/spree_flexi_variants-b4d1f186f568/app/models/spree/order_decorator.rb:36:in `find_line_item_by_variant'
  spree_backend (2.1.2) app/views/spree/admin/orders/_shipment_manifest.html.erb:2:in `block in _71513894e59cfae549481e80232ff5f4'
  spree_backend (2.1.2) app/views/spree/admin/orders/_shipment_manifest.html.erb:1:in `each'
  spree_backend (2.1.2) app/views/spree/admin/orders/_shipment_manifest.html.erb:1:in `_71513894e59cfae549481e80232ff5f4'
  actionpack (4.0.1) lib/action_view/template.rb:143:in `block in render'
  activesupport (4.0.1) lib/active_support/notifications.rb:161:in `instrument'
  actionpack (4.0.1) lib/action_view/template.rb:141:in `render'
  deface (1.0.0.rc4) lib/deface/action_view_extensions.rb:41:in `render'
  actionpack (4.0.1) lib/action_view/renderer/partial_renderer.rb:306:in `render_partial'
  actionpack (4.0.1) lib/action_view/renderer/partial_renderer.rb:279:in `block in render'
  actionpack (4.0.1) lib/action_view/renderer/abstract_renderer.rb:38:in `block in instrument'
  activesupport (4.0.1) lib/active_support/notifications.rb:159:in `block in instrument'
  activesupport (4.0.1) lib/active_support/notifications/instrumenter.rb:20:in `instrument'
  activesupport (4.0.1) lib/active_support/notifications.rb:159:in `instrument'
  actionpack (4.0.1) lib/action_view/renderer/abstract_renderer.rb:38:in `instrument'
  actionpack (4.0.1) lib/action_view/renderer/partial_renderer.rb:278:in `render'
  actionpack (4.0.1) lib/action_view/renderer/renderer.rb:47:in `render_partial'
  actionpack (4.0.1) lib/action_view/helpers/rendering_helper.rb:27:in `render'
  spree_backend (2.1.2) app/views/spree/admin/orders/_shipment.html.erb:36:in `_8390c1f65fc8dd2c255ab0c35409605c'
  actionpack (4.0.1) lib/action_view/template.rb:143:in `block in render'
  activesupport (4.0.1) lib/active_support/notifications.rb:161:in `instrument'
  actionpack (4.0.1) lib/action_view/template.rb:141:in `render'
  deface (1.0.0.rc4) lib/deface/action_view_extensions.rb:41:in `render'
radar commented 10 years ago

Hi @jalberto, please provide the information we ask for in the Contributing Guidelines.

Thanks!

jalberto commented 10 years ago

Steps:

Expecting:

Happening:

Stack trace:

Gemfile:

source 'https://rubygems.org'

# Bundle edge Rails instead: gem 'rails', github: 'rails/rails'
gem 'rails', '4.0.1'

# Use sqlite3 as the database for Active Record
# gem 'sqlite3'
gem 'pg'

# Use SCSS for stylesheets
gem 'sass-rails', '~> 4.0.0'

# Use Uglifier as compressor for JavaScript assets
gem 'uglifier', '>= 1.3.0'

# Use CoffeeScript for .js.coffee assets and views
gem 'coffee-rails', '~> 4.0.0'

# See https://github.com/sstephenson/execjs#readme for more supported runtimes
gem 'therubyracer', platforms: :ruby

# Use jquery as the JavaScript library
gem 'jquery-rails'

# Turbolinks makes following links in your web application faster. Read more: https://github.com/rails/turbolinks
gem 'turbolinks'

# Build JSON APIs with ease. Read more: https://github.com/rails/jbuilder
gem 'jbuilder', '~> 1.2'

group :doc do
  # bundle exec rake doc:rails generates the API under doc/api.
  gem 'sdoc', require: false
end

# Use ActiveModel has_secure_password
# gem 'bcrypt-ruby', '~> 3.1.2'

# Use unicorn as the app server
# gem 'unicorn'

# Use debugger
# gem 'debugger', group: [:development, :test]
group :development do
  gem 'jazz_hands'
  gem "quiet_assets", ">= 1.0.2"
  gem "better_errors", ">= 0.7.2"
  gem 'meta_request'
  gem 'capistrano', '~> 3.0.0', require: false
  gem 'capistrano-rails'
  gem 'capistrano-bundler'
  gem 'cap-deploy-tagger'
end
gem "binding_of_caller", ">= 0.7.1", :group => :development, :platforms => [:mri_19, :rbx]

gem 'whenever', require: false
gem 'backup',   require: false

gem 'money', '~> 5.1'

gem 'spree',                     '2.1.2'
gem 'spree_gateway',             github: 'spree/spree_gateway',               branch: '2-1-stable'
gem 'spree_auth_devise',         github: 'spree/spree_auth_devise',           branch: '2-1-stable'
gem 'spree_fancy',               github: 'spree/spree_fancy',                 branch: '2-1-stable'
gem 'spree_paypal_express',      github: "radar/better_spree_paypal_express", branch: "2-1-stable"
gem "spree_social_products",     github: "spree/spree_social_products",       branch: "2-1-stable"
gem 'spree_reviews',             github: 'spree/spree_reviews',               branch: "2-1-stable"
gem 'spree_i18n',                github: 'spree/spree_i18n',                  branch: "2-1-stable"
gem 'globalize',                 github: 'globalize/globalize'
gem 'spree_zero_stock_products', github: 'swrobel/spree_zero_stock_products', branch: "2-1-stable"
gem 'spree_editor',              github: 'spree/spree_editor'
gem 'tinymce-rails-langs'
gem 'spree_flexi_variants',      github: 'jsqu99/spree_flexi_variants',       branch: "spree-2-1-wip"
# gem "spree_comments",            github: 'spree/spree_comments',              branch: '2-1-stable'
binaryphile commented 10 years ago

Can confirm I'm also seeing this error.

It appears as though the spree view backend/app/views/spree/admin/orders/_shipment_manifest.html.erb has a call to find_line_item_by_variant which needs to be overridden by this extension but isn't yet.

Since I don't ship product in my store, I've added a view to my app with the same filename but empty inside. This has solved my particular issue.

jalberto commented 10 years ago

thanks @binaryphile for the help

Any news on this error?

It yet happens 2-1-stable branch

radar commented 10 years ago

@jalberto This is happening because shipment/manifest expects a certain method, but spree_flexi_variants expects another. Overriding a method to take different parameter sets is kind of bad imo. I don't know how we can fix this and make spree_flexi_variants AND spree happy.

huoxito commented 10 years ago

Agreed that overriding a method using different parameters is trouble. Maybe a solution for now could be changing the signature of https://github.com/jsqu99/spree_flexi_variants/blob/2de9573eee5a460e76bbcf798cda3a64afbbd212/app/models/spree/order_decorator.rb#L36-L42 to something like:

def find_line_item_by_variant(variant, ad_hoc_option_value_ids = [], product_customizations = [])

No idea what those other two parameters are for though.

coorasse commented 10 years ago

No solution yet? I have the same problem and can't figure out how to solve it

radar commented 10 years ago

@coorasse If there was a solution you'd be sure that we would post about it here :)

coorasse commented 10 years ago

@radar can you explain what are these parameters for? I can't get it really. We can try to find a solution

radar commented 10 years ago

@coorasse I've never worked on this extension in any large capacity. I would guess @jsqu99 or maybe @jdutil might know more.

JDutil commented 10 years ago

@coorasse I'm not really familiar with the internals of this extension either. The general idea of what is happening there as far as I know is that you pass in the custom options with the variant in order to determine whether it's a new line item or just incrementing the quantity. To make things more stable those parameters should probably be made into an options hash so calls with just a variant work same as calls with the added options.

jsqu99 commented 10 years ago

Hi All,

Sorry for not responding earlier. I didn't see the notification.

FYI, we have been having discussions on how to deal with this type of thing in spree and extensions going forward. We don't yet have a solution, but are actively working on one.

Those two parameters are key for this extension. The represent the customizations to a variant (ad hoc option types selected and/or customizations (like engraving) and they are ultimately attached to the line item.

JDutil commented 10 years ago

They are key to this extension, but they are optional...

If the method was refactored so that you passed those params in as an options hash instead of setting default values of empty then you could maintain this extensions requirement of having those options, without breaking the usage of this method outside of the extension where those parameters are not being passed.

Basically the solution for this is to make those params optional rather than being required, which is the case when you define them in the way that's being done. So something like:

def find_line_item_by_variant(variant, options = {})
  ad_hoc_option_value_ids = options[:ad_hoc_option_value_ids]
  product_customizations = options[:product_customizations]
end

Would prevent the error in question from being raised because the required parameter count remains 1 instead of being 3.

jsqu99 commented 10 years ago

I'm in total agreement Jeff. He had just asked what those were for so I was trying to convey what they were and how important they were. I'm +10000 on the suggested refactor.