Extendable adjustable updater

[deodad]

This refactor adds some new extensibility options to adjustments.

AdjustableUpdater uses POROs to update adjustments. This allows for alternate adjustment source types to hook into the update of adjustments either before or after the taxes are calculated.

Calling Spree::ItemAdjustments.new(adjustable).update is equalivent to calling Spree::AdjustableUpdater.update(adjustable).

I am looking for feedback before I go any further. Also, I am manually calling the AdjustmentsUpdaters at this point, but in the comments below how I show how I am proposing to call them.

[jhawthorn]

Hi @adaddeo. Thank you for the PR.

I'm not too sure about this. This is a lot of code and complexity for a level of indirection that doesn't seem to abstract anything that cleanly.

Can I ask what exactly you were looking for in terms of extensibility? Maybe there is a simpler way we can achieve the functionality you're looking for.

[deodad]

That'd be great. We want the ability to create new kinds of adjustment sources that maintain their own total, add them to adjustable items, and have them recalculate appropriately during updates, just like tax_rates and shipments.

Example:
Create a new adjustment source called NewAdjustmentSource, it implements adjust, compute_amount, etc. Add new_adjustment_source_total to orders, line items and shipments. Just like a tax rate or promotion action, we can apply it to adjustables and it is updated whenever that item changes.

[deodad]

@jhawthorn did that make sense or is there any additional information I can provide you with?

[jhawthorn]

If I knew the specific use case it would be helpful.

Could we accomplish the same thing by applying all non-tax, non-calcellation adjustments instead of just promotion adjustments before doing tax?

[deodad]

We have two uses: one is a bulk discount extension that can be assigned to products (similarly to tax categories/rates) and configured to stack on promotions or not; the other is an extension to line items that adds optional customizations (think cafepress.com), line items become unique based on a variant and a customization, with some customizations adding additional fees based on quantity.

These extensions work extremely well for us and the existing adjustment logic is perfect for them. The only negative is having to monkey patch ItemAdjustments to get them updated and persisted, however, once they are setup they work flawlessly.

Applying all non-tax, non-cancellation adjustments instead of just promotion adjustments would work. My PR included a bunch of name changes that I thought made more sense, but I understand the hesitation of making changes without a ton of need/thought. The only crucial things required to open up adjustments are:

• In ItemAdjustments, add hooks for other adjustment types to:
  1. Recalculate other_adjustment_total for item by calling update! on other_adjustments scope
  2. Include other_adjustment_total when persisting the updated adjustment totals
• In OrderUpdater, add hooks for other adjustment types to:
  1. Calculate other_adjustment_total in update_adjustment_total
  2. Persist other_adjustment_total in persist_totals

EDIT: We currently us run_hooks in OrderUpdater to calculate other_adjustment_total for the order and persist it. If we had a similar option in ItemAdjustments it would most likely suffice. It would still be great to have an abstracted, simple way to say hook in another adjustment type.

Assuming that made sense, a few finer grain changes that would really make this open to extension would be an additional scope called competing_adjustments that is passed to Spree::Config.promotion_chooser_class here, so other adjustments can optionally compete for the "best promotion". And also replacing/opening up discounted_amount with something where other adjustments can optionally include themselves to be taxed or not here.

[deodad]

@jhawthorn I know that was long, but wondering if you had anymore insight/feedback before this got stale?

[deodad]

I was reading #425, using a save here rather than update columns would be a nice step in allowing extensions to add their own adjustments here.

[cbrunsdon]

@adaddeo thanks so much for the attempt here, sorry we've been pretty quiet on our end. Every time we sat down to look at this closely we got distracted on other things to get 1.1 and 1.2 out the door.

The approach is pretty neat but a little drastic for a single PR. @mamhoff has been attempting to fix the tax situation in a very similar way in: #685 that is more mature (as we've made him re-write it 4 or 5 times now) and we'll likely be going with that.

After that is merged, if we can scale back the size of the changes into more smaller PR's I reckon we can get something in that is safely merge-able. These types of changes are definitely the direction I think we need to move in, but unfortunately with how bad the state we're starting with it has to progress in some incredibly small steps forward.