search

spree-contrib / spree_print_invoice

Create a PDF invoice for Spree orders.
http://guides.spreecommerce.org
BSD 3-Clause "New" or "Revised" License
91 stars 239 forks source link

Update invoice to include options from spree_flexi_variants #12

Closed binaryphile closed 12 years ago

binaryphile commented 12 years ago

The spree_flexi_variants extension makes new ad_hoc option types which aren't in the variant. This patch allows the invoice to show all of the options, including the ad_hoc ones.

GeekOnCoffee commented 12 years ago

Ted,

Does this work if spree_flexi_variants isn't included?

Would it make sense for flexi_variants to add the ad_hoc options to variant_options instead?

binaryphile commented 12 years ago

Yes, it does work if sfv isn't included. The expression first tests for the existence of the ad_hoc options attribute. If it doesn't exist, it isn't called.

It works in the other direction as well. If there are no variant options, it does the listing and joining of the ad_hoc options so that the commas come out correctly (this took some doing, which is why I mention it).

Re adding options to variant_options, I don't have the expertise to comment on that aside from it seems complicated to me. I'm not used to working with models and prefer to stay in the views if possible.

Since the two projects have to be aware of one another for this to work, I considered both angles on which to add it to. I tried it first with spree_flexi_variants, but since you can't use Deface on prawn views, I ended up overriding the entire view, which is obviously a maintenance disaster.

By updating spree_print_invoice with this patch, it gracefully does the same thing as before if sfv isn't there, and does the right thing when it is. The expense is the addition of a conditional expression to the view, which albeit somewhat of a long one-liner, is still a one-liner. This seems more maintainable to me.

Unless the sfv model changes in a major version kind of way, no one should have to touch this modification, and even if it does, the worst likely outcome is that the sfv options simply won't be printed. It shouldn't break the view otherwise.

GeekOnCoffee commented 12 years ago

Perfect. Pulled, thanks!