Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subs: totals in backend are consistent and include fee breakdown #2346

Merged
merged 6 commits into from
Aug 7, 2018

Conversation

oeoeaio
Copy link
Contributor

@oeoeaio oeoeaio commented May 30, 2018

What? Why?

@myriamboure picked up an issue while testing #2122, whereby the estimated totals for subscriptions that included products or shipping/payment methods with fees were not consistently showing the fees in the backend, and when they were, they were not providing a breakdown.

This PR adds a fee breakdown to the Items tab, and makes sure that the totals displayed in the Orders tab are consistent with the Items tab (ie. they include fees).

What should we test?

  • When fees should apply to shipping or payment fees for a subscription, these should be documented in the Items tab on the subscription index
  • When fee should not apply to shipping or payment fees for a subscription, the fees line should not appear in the Items tab on the subscription index
  • Order totals under the Orders tab should always match the total shown in the Items tab, except when a particular order has been edited

Release notes

This is part of Subscriptions which has not been released yet, so no release notes required.

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. :-)

else
object.subscription.subscription_line_items.sum(&:total_estimate)
end
return nil unless object.total.present?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ that nil is not needed

Copy link
Contributor Author

@oeoeaio oeoeaio Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@oeoeaio oeoeaio force-pushed the subs-totals branch 2 times, most recently from 3c40ae3 to a308a2d Compare June 7, 2018 07:29
@mkllnk mkllnk added the pr-staged-au staging.openfoodnetwork.org.au label Jun 12, 2018
@mkllnk
Copy link
Member

mkllnk commented Jun 12, 2018

Staged on https://staging1.openfood.com.au/.

@myriamboure myriamboure self-assigned this Jun 12, 2018
@myriamboure
Copy link
Contributor

Testing notes: https://docs.google.com/document/d/1p9P4E_4RSX6jnmkJanJ_ai1QYV5jun_vo_JyRIr8p9k/edit#

I'll be happy to have maybe another review from @RachL or @sstead as I'm tired and not sure I got it all, but I see two remaining issues:

  • if a subscription is set up, and after that, hub manager modifies the next OC that applies to the schedule of the subscription, the subscription is not updated, so it doesn't reflect the reality of what is going to be charged. I'm not sure about what UX should be though on this point, I don't master enough the subscription feature, @sstead will know better than me ;-) First thought was that the OC should be updated if fees are changed in the OC but maybe I'm wrong. I think it's more complex and need some deeper UX view for stage 2 maybe, as also linked to some form of "contract" between hub and users to buy products under certains conditions and allow hub to charge directly her card for it... so if conditions change (like OC fees) what should happen to the existing subscriptions? CSA register past contracts, if you change contract you can still see the past contracts, etc.
  • fixed "fee per order" doesn't appear in the fee breakdown and totals while shipping fee per order do (I didn't test all the fees separately though but this one is not working at least)

@myriamboure myriamboure removed their assignment Jun 12, 2018
@mkllnk
Copy link
Member

mkllnk commented Jun 12, 2018

This is worrying. If we don't even know how it should work, the users won't know either. There will probably be a lot of questions.

@oeoeaio
Copy link
Contributor Author

oeoeaio commented Jun 13, 2018

So the issue is this: our fee system is so complex that it's virtually impossible to estimate the fees that will apply to an order without actually placing the order. There are so many conditions and different ways of calculating things that attempting to replicate them is just not a viable option for us.

The price displayed in the subscriptions interface is intended to be an estimate, because the total for two identical orders placed in different order cycles could be completely different. So the logic just uses the next OC (or most recent if no future OCs exist) to estimate the total. This total is used as the estimate for all future orders. Perhaps the solution to this confusion is just to make it much more clear that the prices displayed in the backend are an estimate only?

I will have a look at the issue with the whole order fees, I suspect that I have just not put it any logic to estimate these fees correctly.

@sstead
Copy link

sstead commented Jun 13, 2018

Testing notes
From my testing the main issues arise when changes are made to fees. The predictive totals are correct for a new subscriptions, before any fee changes are made.

Issues seen:

  • If a shipping/payment fee is adjusted the predictive totals don't update.
  • If an enterrpise fee in OCs changes the predictive totals don't update
  • Some fee calculators (at least one) are not being incldued in predictive totals- specifically the 'flat amount per order' enterprise fee.
  • % fees are being predicted in a different way to how they're actually applied. Specifically whether a % fee is calculated pre or post enterprise fees.

From what I saw actual orders placed were always correct, it's just the predictions that cause concern/confusion.

https://docs.google.com/document/d/1ZCEkFBNoXE8M7R9vSzjTyJ60LDa2jXNjRzmsueTjRfg/edit#

@oeoeaio oeoeaio removed the pr-staged-au staging.openfoodnetwork.org.au label Jun 13, 2018
@myriamboure
Copy link
Contributor

I think the "subscription" is more like a "contract" than an order in term of UX - that vision is also more aligned with CSA people for whom a subscription IS a contract like "I commmit to buy one vegetable basket for 12€ per week for 10 months" for instance. Any subscription is an agreement between a customer and a hub, a commitment to buy something at a certain price (or flexibe price depending on contract) for a certain period of time.

I see an issue with updating original contract term by just updating the predictive total if OC changes and loosing original info. It means we would believe afterward that since the begining the contract was that one, even if it changed on the way. I think we would need more steps, like:
1- The subscription as it was set up originally with basic info: which product and which price for how long? This should be engraved somewhere and not being possibly touched. (original contract) (in stage 2 that would open the door to more formal contracts that could answer CSA needs)
2- If there is a modification of that contract during the period of the contract (like the hub manager adds a fee in an OC that concerns that subscription so price paid changes) there would need to be some form of "consent" from user that she is going to be charged more for that product from next OC for instance, that would be kind of an "amendment to the original contract", so we could see "original contract" and then "amendments" so new contract could replace old ones.
3- Each week (for weekly subscription for instance) there is a predictive order that is going to be made given the last version of the contract terms, so the hub manager can control that everything is going to be all right. This should be with the accurate info of what is in the OC then.
4- Then when the order is made if should be consistent or if there is a difference it should be explained. Like "this product was not available so couldn't be put in and has not been charged".

I'm conscious it is not the way that has been taken here, the logic remains the same but UX would vary a bit. Do you agree about that?
I'm happy to see how we can go over that to be able to release quickly version 1 and iterate. Is there any document about the end to end inception you did at the begining? That woud reallly help understand...

I see two option if we want short term decision on what to do:

  • either we consider the current "subscription" column is not the subscriptions info and rename it "estimate next automatic order" which is based on subscription. Then we need to make the fees work accurately if OC changes and update. But shall we add some info to store and see original subscription info somewhere? Or just accept that we have lost those info?
  • either we consider the current "subscription" column is only the basic agreement and don't update or change anything if OC change (we can rename it "original subscription"), but in the "order" column when it is "pending" we should see the detailed info of what is going to be charged with the fee breakdown uptodate given OC info, and when it is "active" those info are updated with real info so amount could change. We can always compare with original contract and at least see if there is a difference if we can explain it. So basically we would slightly change the way the "order" column work to be a mix of predictive and actual order.

What do you think @oeoeaio @sstead ?

@oeoeaio
Copy link
Contributor Author

oeoeaio commented Jun 14, 2018

I'm conscious it is not the way that has been taken here, the logic remains the same but UX would vary a bit. Do you agree about that?

Errr, no. This is a completely new and different set of requirements to the original spec or anything I have heard while working on this. There are definitely ways that we could achieve what you want, and I really like the direction that you're going with this, but I don't want to give the impression that it will be a small change. I didn't design for this at all. We will need to incept carefully and think about the exactly what we need and what the best way of achieving it is. I suspect that reimplementing the concept of price for subscriptions is not going to be a good approach.

1- The subscription as it was set up originally with basic info: which product and which price for how long? This should be engraved somewhere and not being possibly touched. (original contract) (in stage 2 that would open the door to more formal contracts that could answer CSA needs)

At the moment, a subscription is just a list of products + quantities, mapped onto a list of order cycles. All prices come via the order cycles. There is no concept of price at all inside a subscription, so no basis on which to build a price contract. We could definitely change it so that a subscription knows about prices, but I am unclear about whether this means spot prices (OC prices) are overridden or whether we need some way of deciding between the contract price (subscription price) and the spot price. I think they should be completely independent of each other.

2- If there is a modification of that contract during the period of the contract (like the hub manager adds a fee in an OC that concerns that subscription so price paid changes) there would need to be some form of "consent" from user that she is going to be charged more for that product from next OC for instance, that would be kind of an "amendment to the original contract", so we could see "original contract" and then "amendments" so new contract could replace old ones.

This sounds nice, but it is huge. It also sounds like the contracts are not a contract if they need to be continuously amended because the shop updates their enterprise fees. What happens if contracts are not approved? How do we deal with contract disputes?

3- Each week (for weekly subscription for instance) there is a predictive order that is going to be made given the last version of the contract terms, so the hub manager can control that everything is going to be all right. This should be with the accurate info of what is in the OC then.

This is very complex. I think that contact prices should be entirely independent of OC prices. Once prices are set (which can be done in reference to an OC price), that should be the price that should be paid by the given customer until the contract is terminated or it expires. Otherwise we have to deal with the interaction between prices that differ between contracts, and prices in the OC which are what customers who don't have a subscription need to pay. 💥

4- Then when the order is made if should be consistent or if there is a difference it should be explained. Like "this product was not available so couldn't be put in and has not been charged".

Yep, we're kind of already doing this part, but not in reference to a contracted price, just in reference to what the user would have been expected to pay if their order was able to be fulfilled for the order cycle in question.

@myriamboure
Copy link
Contributor

Ok, agree @oeoeaio let's see in subs v2 how we want to evolve this, but what should we decide for short term solution to solve that feeling of "inconsistency" in order to release quickly? I shared 2 options, but maybe you have others to propose... what do you think Rob and @sstead ?
Option 1 is what you have started to do and what Sally and I reported, if we change OC info in the subs column change.
Option 2 is keep column subscription only with original info as you designed it but adapt slighty the orders columns then to show some details about products an fees of the order that is going to be made /made in order to avoid confusion.

@sauloperez
Copy link
Contributor

this is a friendly poke not to forget about it.

@kirstenalarsen
Copy link
Contributor

hi guys - just scanning this to add my 2c about whether it needs to be done before release of subs-1 or not. Seems to me that there is no problem as long as the Enterprise either has an agreement with Customers that they can change fees throughout the subscription cycle, or they can't. And if they have agreed with Customers that they can't, then they don't. Obviously there is a lot of communication between Hubs and Customers about setting up a subscription that isn't being handled in the platform, and we probably just need to make sure the Enterprise Users are very aware of the implications of changing a fee, and clear on whether they have agreed with their Customers that they can or they can't

@myriamboure
Copy link
Contributor

@kirstenalarsen I don't disagree, but here what annoy me is the inconsistency in the info that the hub manager read, with no explanation why. I find the UX very poor for the hub manager. In one column ("subscription") she sees info of the original subscription set up for the user with all the details about products, prices, fees, etc. In the other one ("order") she sees the order about to be made but have no detail about this order, so if some changes are made since the subscription was set up (like fees, or product prices) the hub manager can't understand the difference as there are no details. I was not talking about the communication between hub and customer here, only about clarity of the info for the hub manager.

@kirstenalarsen
Copy link
Contributor

yep there's room to improve for sure, but at the moment I think things like this need to be clear in the user guide and the hand-holding of beta testers. An order is generated by an order cycle, so if you change product prices or fees in the OC then the actual order price will change. Perhaps it could be a tooltip when the totals are different?

@myriamboure
Copy link
Contributor

Yet, at the minimum if we don't want to improve UX now... user guide explanation + maybe a small sentence on the "orders" tab saying "Warning: if you modify enterprise fees on products or orders in your OC, the predicted amount for the orders will diverge from the one in the original subscription set-up. To understand the details of it, check your OC details." could work.

@mkllnk
Copy link
Member

mkllnk commented Jul 30, 2018

I guess the workaround would be to change the subscription so that the prices are re-calculated.

@mkllnk mkllnk assigned mkllnk and unassigned oeoeaio Aug 2, 2018
@mkllnk
Copy link
Member

mkllnk commented Aug 2, 2018

@myriamboure @kirstenalarsen I'm not sure how we should proceed with this. Would you like to create a new issue for some warnings within the admin interface?

The problem is that the estimated price is cached. When we change a product price or a fee, we would need to search for all affected subscriptions and re-calculate their estimate. That seems complicated and slow.

The other option is to not cache the estimate, but calculate it whenever you look at the subscription. That is probably slow as well (hence Rob cached it). Once we have the Spree upgrade, we can calculate real prices for subscriptions. At that point we can see how fast that is and if we still need to cache it.

My verdict is that we deal with the inaccurate values with documentation and hints in the interface and revisit this after the Spree upgrade.

@myriamboure
Copy link
Contributor

As I said in my last comment, I'm happy with that in v1, just suggested as well to add a sentence on the orders tab to avoid interrogation from users when they see conflicting info... see above.

@mkllnk
Copy link
Member

mkllnk commented Aug 3, 2018

Okay, since there are many places that can affect a subscription total, I added a notice at the displayed total:

screenshot from 2018-08-03 15-03-35

Is that a good solution?

@sstead
Copy link

sstead commented Aug 3, 2018

I didn't get a chance to test this today. Feel free to test @myriamboure or @RachL , but no problem if you leave it to Monday when I can look. It's on staging1.openfood.com.au

@mkllnk mkllnk added the pr-staged-au staging.openfoodnetwork.org.au label Aug 3, 2018
@mkllnk
Copy link
Member

mkllnk commented Aug 3, 2018

Staged on https://staging1.openfood.com.au/.

@RachL
Copy link
Contributor

RachL commented Aug 3, 2018

@sstead @myriamboure I'm sorry but I won't have time to dive into this one today :(

@RachL
Copy link
Contributor

RachL commented Aug 6, 2018

@mkllnk @sstead @myriamboure just to be sure, is the sentence the only change being made ? If so, I do see it:
image
Also I went through the two other testing documents and recreated the tests, step by step and I had the same results than your screenshots (I've used Sally's producer of honey with a new schedule). Other than that I'm a bit confused on what is supposed to be tested here... Can someone give me more info? Thanks :)

@RachL RachL self-assigned this Aug 6, 2018
@myriamboure
Copy link
Contributor

I guess that's all with @RachL, since last tests only the sentence was added. So if you retested the things from before and the new sentence thing it should all be good then!

@RachL RachL removed the pr-staged-au staging.openfoodnetwork.org.au label Aug 6, 2018
@mkllnk mkllnk merged commit 03468ef into openfoodfoundation:master Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants