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 calculators #1197

Merged
merged 5 commits into from
Mar 16, 2014
Merged

Price calculators #1197

merged 5 commits into from
Mar 16, 2014

Conversation

umpirsky
Copy link
Contributor

Fixes issue #1120.

This will allow us to have custom price calculators. For example, we have something like this in our project.

<?php

namespace Sylius\Bundle\CoreBundle\Calculator;

use Sylius\Bundle\CoreBundle\Entity\Variant;
use Sylius\Bundle\CoreBundle\Provider\RegionalPriceProvider;

class RegionalPriceCalculator implements PriceCalculatorInterface
{
    private $regionalPriceProvider;

    public function __construct(RegionalPriceProvider $regionalPriceProvider)
    {
        $this->regionalPriceProvider = $regionalPriceProvider;
    }

    public function calculate(Variant $variant)
    {
        return $this->regionalPriceProvider->getRegionalCurrentPrice($variant);
    }
}

@@ -24,7 +24,7 @@
* A form resize listener capable of coping with a zone member collection.
*
* @author Tim Nagel <t.nagel@infinite.net.au>
* @author Саша Стаменковић <umpirsky@gmail.com>
* @author Saša Stamenković <umpirsky@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

Old one was nice as well ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was better :)

@winzou
Copy link
Contributor

winzou commented Mar 13, 2014

But you display wrong price in frontend?
This calculator is nice, we shouldn't have all these calculate methods in Order and OrderItem.

@umpirsky
Copy link
Contributor Author

@winzou Ah, true. Missed that :)

Will fix, thanks.

@kayue
Copy link
Contributor

kayue commented Mar 13, 2014

Cool stuff 👍

use Sylius\Bundle\CoreBundle\Model\VariantInterface;

/**
* By default calculator returns variant price.
Copy link
Contributor

Choose a reason for hiding this comment

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

Default calculator simply returns the variant price.

@umpirsky
Copy link
Contributor Author

@winzou umpirsky@85c67f8

@winzou
Copy link
Contributor

winzou commented Mar 13, 2014

I would totally separate the formatting from the calculation. Like this: {{ variant.price|calculate|sylius_price }} or even in the function style (maybe more intuitive than filter): {{ calculate(variant)|sylius_price }}

@kayue
Copy link
Contributor

kayue commented Mar 13, 2014

Agree with @winzou on function style. Maybe calculate_price()?

And beside variant, should we accept Product?

@stloyd
Copy link
Contributor

stloyd commented Mar 13, 2014

TBH. it's kinda confusing for me where to put this code. IMO it's not totally correct to have it in the CoreBundle, while all prices are being calculated by MoneyBundle... I'm confused.

Anyway, we can't depend on Variable only, we should support Product too, as in Product you have i.e. getMasterVariant()...

@kayue
Copy link
Contributor

kayue commented Mar 13, 2014

@stloyd Where does MoneyBundle calculate price? I thought MoneyBundle was just doing currency exchange.

If we put PriceCalculator to MoneyBundle, that would make MoneyBundle depends on Variant wouldn't it?

@pjedrzejewski
Copy link
Member

PricingBundle. Would be cool to put it there... I'll put your implementation on steroids later. (add optional configuration to pricing calculators) What do you think?

@pjedrzejewski
Copy link
Member

And I'd go with PriceableInterface. Instead of relying on the variant. It could return the price calculator name and configuration. (in future)

@stloyd
Copy link
Contributor

stloyd commented Mar 13, 2014

@pjedrzejewski I would say that bundle is overkill, component sound ok but bundle is overkill ;)

But totally agree about that interface, I was thinking about same thing, but placed in ProductBundle, or in MoneyBundle, but prefer the first one :trollface:

@pjedrzejewski
Copy link
Member

IMHO, it definitely should be a separate bundle. In @umpirsky implementation it is quite thin, but for sure it will contain much more and can be used separately from Product bundle. Why would you couple the pricing stuff only to Product bundle?

@umpirsky
Copy link
Contributor Author

@winzou I agree about {{ calculate(variant)|sylius_price }}, will update.

@pjedrzejewski I agree about PriceableInterface, will update. What about keeping it in core for now as long as it is this tiny. We can move it to component or separate bundle once you decide to put it on fire with configuration etc?

@pjedrzejewski
Copy link
Member

@umpirsky Deal! 👍

@umpirsky
Copy link
Contributor Author

@pjedrzejewski Ready for merging. Just waiting for Travis.

@umpirsky
Copy link
Contributor Author

@pjedrzejewski Can you merge it please?

pjedrzejewski pushed a commit that referenced this pull request Mar 16, 2014
@pjedrzejewski pjedrzejewski merged commit 9ff345d into Sylius:master Mar 16, 2014
@pjedrzejewski
Copy link
Member

Thanks Sasha! The build was failing previously. Nice work! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants