solidus icon indicating copy to clipboard operation
solidus copied to clipboard

Configurable cancelation adjuster

Open mamhoff opened this issue 1 year ago • 2 comments

Summary

While working on the promotion system, I realized that unit cancels use Spree::Adjustment#recalculate. This is unfortunate, as that method does not work well for the improved promotions system, but is still used for cancellation adjustments.

Knowing how taxes work, I was surprised to see that cancellation adjustments are recalculated after taxes are applied. This PR fixes that, and can thus simplify the calculation of the unit cancellation amount significantly: Before taxes, the amount of a cancellation must be simply the line item's price, since an inventory unit always has a quantity of 1.

Tax adjustments will recalculate regardless of their finalized state. I believe this should be the case for cancellation adjustments as well. While it is unlikely that unit cancels change in value, taxes might very well change between order completion and unit cancellation.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

  • [x] I have written a thorough PR description.
  • [x] I have kept my commits small and atomic.
  • [x] I have used clear, explanatory commit messages.

The following are not always needed (~cross them out~ if they are not):

  • [x] I have added automated tests to cover my changes.
  • [ ] ~I have attached screenshots to demo visual changes.~
  • [ ] ~I have opened a PR to update the guides.~
  • [ ] ~I have updated the readme to account for my changes.~

mamhoff avatar Jul 26 '22 17:07 mamhoff

Thanks @mamhoff! I left a couple of small comments, but love the simplified solution you have proposed here and the new configuration preference ❤️

forkata avatar Jul 26 '22 18:07 forkata

I'm hesitant to keep this open after discussions with @forkata on Slack. It seems some stores rely on the pre-existing solution, and Solidus as a whole (i.e. the order updater and Spree::OrderContents all have implicit dependencies on the existing order of things. Here be dragons.

mamhoff avatar Aug 02 '22 12:08 mamhoff

Hey, @mamhoff, do you think that we should therefore close it?

waiting-for-dev avatar Aug 25 '22 08:08 waiting-for-dev

Yeah, let's close it for now; I'll probably get something up in the future that works better.

mamhoff avatar Aug 26 '22 09:08 mamhoff