Volume-based setPricingConfiguration parameter is counterintuitive

[Goutte]

Look at https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/CoreBundle/Behat/CoreContext.php#L295

If the range is 30+ then the generated pricing configuration for setPricingConfiguration() is

array(
  'min' => null,
  'max' => 30,
  // ...
)

whereas it should be :

array(
  'min' => 30,
  'max' => null,
  // ...
)

What am I missing ?

[stloyd]

Sounds like an issue. Can you submit PR fixing it? Thanks.

[Goutte]

Yeah, will do. I'll add a new scenario to test for an order greater than 30, while I'm at it.

[Goutte]

That was worth it ! The step definition was also buggy in other unexpected ways.

Consider the difference between (int) $data['price'] * 100 and (int) ($data['price'] * 100) when $data['price'] = '55.99'. The first one returns 5500, instead of 5599.

Also, 30+ range was not including the boundaries (here, 30), and other ranges did, so I fixed that as well.

I added some documentation as well in VolumeBasedCalculator.

---

The actual code silently ignores absurd ranges (with a null price or null min quantity). I left it as is, but it's worth considering throwing loudly when absurd ranges are met, don't you think ?

[pjedrzejewski]

@Goutte I agree. Thank you very much for working on this! :)

PS. For some reason travis did not run, let's see on the next push.

[Goutte]

I added a new Exception, untranslated and very crude, and took the liberty of refactoring the ifs, and slightly make the calculator more tolerant of unset minima of ranges, by assuming it is 1, much like we assume that unset maxima are infinity.

Travis CI build is in progress as I write this.

Don't thank me, thank the spec suite making PRs a pleasure ! ;)

[pjedrzejewski]

Awesome, thank you Antoine! 👍