Vulnerability in EPiServer.Forms
Hello! This post is meant to be a discussion, as I don't have a 100% thought out solution.
Taking a look at the different ways of applying promotions, you can either
This might seem like reasonable when you want to run promotions on a cart, but I feel that something is missing when we're talking about other OrderGroups, e.g. PurchaseOrders. PurchaseOrders are a bit more sensitive in their nature, as they are "made purchases". Someone has paid something (or nothing) and gone through the order flow to have a PurchaseOrder instead of just a cart. Normally the PurchaseOrder should remain intact, but there are a lot of scenarios where you would want to load and make changes to a purchase order. It could be returns, partiall returns, updates from backend systems, etc.
For purchase orders I don't really like either of the two execution paths above. Let's break it down into the problems
The main problem circulates around promotions change. You add new ones, old ones expire.
What if you bought your purchaseOrder at a 1h window where everything was 20% off? If you would fetch all and evaluate, you wouldn't fetch the 20% off since it's expired. Suddenly just because you did a return of 1 item in the order, you now owe the company 20% of all the other items in your order. "Have a happy day! Oh btw we're punishing you because you utilized our very nice option to have free returns within 30 days". Not great.
Another issue in the same area is the other way around. You do a return of 1 item. Suddenly, since there now exists a promotion that applies to your items that didn't apply when you bought them, they are at a lower price! Now the company owes you money. That's neat and a happy customer experience, but I wish your payment, ecommerce and POS system egineers good luck in making this implementation flawlessly what with receipts, historical information, etc.
Summary: Not good.
Unfortunately, it's not as good as it seems, this method struggles with the same issue as Method #1, it fears change.
Let's go into a bit what it means "only fetch the promotions that the customer got" in this case. The PromotionEngine, given the PromotionEngineSettings.ReevaluatePromotions = true, will look into the order's OrderForms and get all the PromotionInformation objects in the Promotions list for each order form. The PromotionInformation doesn't have enough information to actually evaluate though, so we need to get the promotion object from the database. Fortunate for us (note to readers: this isn't how fortune or luck works :P), the PromotionInformation object has a promotionGuid we can use to content load the promotion object. And after we've fetched the promotion object, we evaluate it.
So essentially, we get all the issues from Method #1, except "newly created" promotions, which won't be applied. If you delete a promotion and Reevaluate, the best scenario is for it to not crash and remove the promotion, but suddenly your order has one less promotion applied. Or maybe more likely a promotion expired or changed in some way, leading to your paid order changing the prices again.
Summary: Still not good.
So what can we do to avoid this immensly complicated mess of paid orders changing prices on all or some items seemingly uncontrollably? One could argue that "never run promotions on purchase orders!!!1", but there are too many valid use cases where you would want to do that and actually want to see prices changed. E.g. you bought 3 items with a 3-for-2 promotion and then returns the two most expensive items. If you don't run promotions you can get a free item!
My idea is that promotions needs to work with snapshots, and have a PromotionEngineSetting to actually re run the promotions that were applied, not re run the promotion with the same ID as the one who were applied. So whenever you apply promotions, in addition to adding the promotionInformation object, we should serialize the PromotionData object and store it on the purchase order. This should be overridden every time you evaluate promotions normally. And when you send this new PromotionEngineSetting flag (PromotionEngineSettings.ReevaluatePromotionsForRealThisTime), we don't do any content load, we simply deserialize the PromotionData objects and pass those to evaluation.
Another minor issue I have with the PromotionEngineSettings.ReevaluatePromotions is the naming of the function that will be run if this flag is set. When I first took a look at what happened in the current implementation of IPromotionEngine.Run, I thought the PromotionEngineContentLoader.GetAppliedPromotions(IOrderGroup orderGroup) got the applied promotion objects from the order, as you might argue the naming suggests, but in reality it just gets the promotions with the same ID, which as I've talked about doesn't have to be the same promotion from the user's perspective.
If I'm wrong in anything, please correct me! And if anyone has any other ideas of reducing this complexity, please share! :)
Edit: When I read this back, I think what I'm saying is that I'd like to replace Method #2 with the suggested method, I don't see any value in Method #2...
First thought is if you return an item do you really want to reevaluate all promotions for the order or only the promotions that affected that line item in the first place?
Also just a vague idea but would it be possible for a promotion processor to calculate backwards from a purchase order the refund value of a return?
"It could be returns, partiall returns, updates from backend systems, etc." - I only see retuns as the need for snapshots, updates from a backend system would work if the promotionengine could be run for a specific date (to cover promotions that has past an end date, it is unlikely that the actual promotion data has changed in that time).
Maybe it's unlikely, but you don't want any subsystem touching pricing not to be as foolproof as you can get it. I'd say you'd want 100% snapshots, for that reason.
Probably just the promotions that affected the lineitem, but easiest way of achieving it is probably just evaluating everything again, given the promotions didnt change.