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

"Price by ranges" doesn't calculate "above xx km/kg" correctly when there is no Distance or Weight condition set #2307

Open
Tracked by #32
joel-shiftdelivery opened this issue Apr 21, 2021 · 8 comments

Comments

@joel-shiftdelivery
Copy link
Contributor

joel-shiftdelivery commented Apr 21, 2021

Describe the bug
When using "Price by Ranges" and that pricing condition does not include Distance or Weight, the pricing does not accurately take into account the "above xx km/kg" portion of the string, and instead calculates every interval below that as negative.

Screenshot from 2021-04-20 20-54-58
Screenshot from 2021-04-20 20-52-45

To Reproduce
Steps to reproduce the behavior:

  1. Go to Pricing
  2. Set a condition that is not distance or weight (such as Pickup address)
  3. Choose "Price by ranges" and set parameters and save
  4. Go to a store and select those pricing rules
  5. Try to create a delivery
  6. See error

Expected behavior
Even is a distance or weight condition isn't selected as one of the conditions, "Price by ranges" should always take into account the "above x kg/km" and only calculate when the kg/km is higher than specified.

Desktop (please complete the following information):

  • OS: Linux
  • Browser: Brave

Additional context
The "above" portion of the string does seem to calculate correctly if you do not have a corresponding Distance or Weight condition chosen.

@agichim
Copy link
Member

agichim commented Apr 21, 2021

First of all, there is a bug that @alexsegura needs to correct.

  • in the last rule the price by range is saying 10kg above 20km ← I don't know if it's only a visual bug or js bug.

Second of all, when you use price by range you will mandatorily need a rule which is always true. Either for km or kg. Check this doc here under point "b."
Even if you use a zone, because you are using price by range you will have to use a Rule like in the example above in the docs, you need to specify a base price before the price by range is true and gets calculated. Because if the customer's details do not fall under price by range than he will get an incorrect price.

I hope this makes sense. Although it still needs some testing and trying out more combinations of pricing. Let me know how it goes and we can work it out together than I can add this example to the docs.

@joel-shiftdelivery
Copy link
Contributor Author

joel-shiftdelivery commented Apr 23, 2021

@agichim

I think the first kg/km bug (created here: #2306 ) is a bug that doesn't affect calculation, but not sure about the back end reason why.

I did see the docs before posting, but we are unable to set it at always true (distance/weight>0) because we include 2km and 20kg in our base price.

Our base prices are based on how far in advance an order is placed (the top two conditions), and I initially thought that should work for the "base" price part of it.

Here's what we did set for pricing that does work:

Screenshot from 2021-04-23 16-41-10

While I'm not sure of the exact implementation, it seems like the main thing is that Price by ranges isn't respecting the above xx km/kg unless the condition in that box is also distance/weight.

It would be helpful to have it work in conjunction with other conditions too, and allow for km pricing in Zone A to be $1/km, but Zone B is $2/km, as an example.

Edit:
I had another thought and it looks like there can be any condition for price by range, as long as it includes the relevant distance/weight condition as well. See here for two examples that seems to work:

Screenshot from 2021-04-23 17-06-50

Screenshot from 2021-04-23 17-10-30

Based on how this works currently, maybe update the docs to state that "any condition using Price by ranges must include Distance/Weight as part of the condition"
Instead of the existing "Make sure the condition(s) in Rule 1 are always true. E.g.: Distance (km) > 0" which doesn't seem to be necessary.

@agichim
Copy link
Member

agichim commented Apr 24, 2021

So the pricing is super complex still and it needs a lot of testing and trialing in order to make the docs more helpful.

because we include 2km and 20kg in our base price.

This thing you say ↑. What do you do when someone orders and they are less then 2km from the collection address? What is the price for that?

Make sure the condition(s) in Rule 1 are always true. E.g.: Distance (km) > 0

This here ↑ is an example in order to set a base price. When I was testing, I did as in the screenshot and the system wasn't taking into account the base price of 4 from Rule 1.
image
So in that case (if the pricing would be used for real), Rule 1 needs to have Distance (km) > 0 so that anyone living less than 3km away from collection address pays 4.

It's a lot of work to update the docs covering all the conditions. It'll be easier to do once people feedback their different pricings.

any condition using Price by ranges must include Distance/Weight as part of the condition

Yeah, that's a good point. And your point here makes me think again about what happens when someone is less than 2km away?

@joel-shiftdelivery
Copy link
Contributor Author

joel-shiftdelivery commented Apr 24, 2021

Quick note: the two screenshots in the edit section were simply for trialling, and not live use. The top screenshot in my last comment is what we are using right now with CoopCycle.

This thing you say ↑. What do you do when someone orders and they are less then 2km from the collection address? What is the price for that?

For orders less than 2km, the base price is charged, which is determined by the first two Difference (hours) conditions.

Screenshot from 2021-04-23 16-41-10

For this testing example, if an order is less than 2km, there is no delivery charge since there is no other conditions set (which is the correct calculation):
Screenshot from 2021-04-23 17-10-30

This here ↑ is an example in order to set a base price. When I was testing, I did as in the screenshot and the system wasn't taking into account the base price of 4 from Rule 1.

That seems like the correct calculation as you have less than 3km for $4, so if it is 3.5km, then the $4 isn't applicable, and it correctly ignores that condition. If the $4 base rate was applicable to all distances, you are correct that it would need to be >0 for the condition, but you could also the the conditions you have plus one more for >3km with a base rate of $6 for example. In that case, a 3.5km delivery would be $6 +$1 for the distance above 3km ($7 total).

Yeah, that's a good point. And your point here makes me think again about what happens when someone is less than 2km away?

See my comment above, but when an order is less than 2km, it ignores the price by rangescondition and just uses the other conditions to calculate price. The other conditions can be anything (Difference hours, distance, weight, zones, etc) and it seems to calculate correctly.

In summary, the only requirement necessitated by Price by Rangescurrently for correct price calculation is that any condition using Price by ranges must include Distance/Weight as part of the condition
The only reason for this is that there is a calculation bug in CoopCycle otherwise, where it does not properly account for the above x km/kg and instead treats any distance/weight below that number as a negative value.

@agichim
Copy link
Member

agichim commented Apr 28, 2021

Alright, I see. Yes, the price by range makes it mandatory to use km or kg conditions. It might be a bug but it seems to me it's more likely a function/calculation that wasn't programmed.

@joel-shiftdelivery were you using this setup to charge a base price based on the difference condition with the old pricing system?
Hey @alexsegura have you seen this thread?

@agichim
Copy link
Member

agichim commented Apr 28, 2021

I'd suggest editing the title of this issue to something more specific to the issue at hand.

@joel-shiftdelivery joel-shiftdelivery changed the title Price by ranges doesn't calculate properly "Price by ranges" doesn't calculate "above xx km/kg" correctly when there is no Distance or Weight condition set Apr 28, 2021
@joel-shiftdelivery
Copy link
Contributor Author

joel-shiftdelivery commented Apr 28, 2021

Alright, I see. Yes, the price by range makes it mandatory to use km or kg conditions. It might be a bug but it seems to me it's more likely a function/calculation that wasn't programmed.

As I've thought about it more, I agree that's probably the case and it is just a minor tweak to the calculation that's not programmed.

@joel-shiftdelivery were you using this setup to charge a base price based on the difference condition with the old pricing system?

Yes, we used the same pricing setup/structure under the previous system as well without issue.

I'd suggest editing the title of this issue to something more specific to the issue at hand.

Done, thanks.

@Paul-Eraman-CoopCycle
Copy link
Contributor

Really this is a "we need to redesign how pricing works" issue that is much larger than this particular issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants