Tax rates with dates

[everwhatever]

Q	A
Branch?	1.12
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	jira
License	MIT

[coldic3]

Could you also remove composer.phar and env.local?

[vvasiloi]

Can this be reduced to a custom query?
Are repository methods part of the BC promise?

[vvasiloi]

Can this be reduced to a custom query? Are repository methods part of the BC promise?

There isn't a custom repository for tax rates, so there won't be a BC break.
The repository could look like this:

class TaxRateRepository extends EntityRepository implements TaxRateRepositoryInterface
{
    public function findOneByCategoryAndDate(TaxCategoryInterface $taxCategory, ?\DateTimeInterface $date = null): ?TaxRateInterface {
        if (null === $date) {
            $date = new \DateTime();
        }

        $expr = $this->_em->getExpressionBuilder();

        return $this->createQueryBuilder('o')
            ->where(
                $expr->andX(
                    $expr->eq('o.category', ':category'),
                    $expr->orX(
                        $expr->isNull('o.startDate'),
                        $expr->lte('o.startDate', ':date')
                    ),
                    $expr->orX(
                        $expr->isNull('o.endDate'),
                        $expr->gte('o.endDate', ':date')
                    ),
                )
            )
            ->setParameter('category', $taxCategory)
            ->setParameter('date', $date)
            ->getQuery()
            ->getOneOrNullResult();
    }
}

However, a way to add the zone in the core bundle is needed, which complicates things a bit. 🤔
We could add a child repo in the core bundle with another method that also takes in the zone and extracts the common query builder/expression, but the fact that the resolver accepts a criteria array that's passed directly to the findBy/findOneBy makes things weird.

If we keep the date filter logic in PHP, then I suggest transforming the TaxRateDateChecker into an "eligibility checker" (which is common concept in Sylius) that will take a single tax rate and return true if it fulfils the conditions and false otherwise.
Basically, the checker will contain the logic that's now in TaxRateDateChecker::isInDate, which now is a public method that's not part of the abstraction.
Then the resolver will iterate over the tax rates returned from the repository and return the first one that's eligible.

[NoResponseMate]

Missing behat tests for tax rate form validation

[vvasiloi]

🤔 maybe we can avoid the deprecation by decorating the original resolver instead of modifying it? WDYT?

Edit:

• Add an EligibleTaxRateResolver next to the current TaxRateResolver in the taxation component
• Add the decoration to the taxation bundle

[Zales0123]

I believe that the deprecation is not a bad option here - of course, it can cause discomfort for the service user, but on the other hand, we transparently inform about important changes in tax rates resolving logic 🚀 Moreover, as the interface allows to return only one tax rate, we could not use the result of the base resolver to filter the ineligible tax rates, as we need an array of them 🖖

[vvasiloi]

@Zales0123 my idea was to completely ignore the output from the current resolver, as it remains only for compatibility for those using the service somehow else. Now that I said it out loud it doesn't sound that good anymore.

[coldic3]

Suggested change

	<argument type="service" id="sylius.tax_rate_date_eligibility_checker" on-invalid="null" />
	<argument type="service" id="sylius.tax_rate_date_eligibility_checker" />

I guess it's not needed.

[Zales0123]

Thanks, @everwhatever! 🎉