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

fix: customer group discount tax #2757

Draft
wants to merge 1 commit into
base: 5.7
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion engine/Shopware/Core/sBasket.php
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ public function sInsertDiscount()
$tax = $this->config->get('sDISCOUNTTAX');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$tax = $this->config->get('sDISCOUNTTAX');
$tax = (int) $this->config->get('sDISCOUNTTAX');

Copy link
Contributor

Choose a reason for hiding this comment

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

is this the tax value? if so, it should definitely not be an integer, but a float instead

}

if (!$tax) {
if (!$tax && $tax != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!$tax && $tax != 0) {
if ($tax === false) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @aragon999 ,
this way looks much nicer, I wasn't sure, if there is another datatype that can trigger the fallback.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, the only case which needs to be handled is that getMaxTax() returns false.

It would still change the logic slightly, as when previously when the tax 0 has been configured in sDISCOUNTTAX the tax wont be 19 anymore. But that is I think in all cases like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also prefer a type safe comparison. but then we need to max sure, that $tax is really a float or false at this point.

additionally to that, I would like to remove the fallback to a hardcoded value 🙈

what do you think? throwing an exception or getting a tax value from somewhere else?

$tax = 19;
}

Expand Down
40 changes: 40 additions & 0 deletions tests/Functional/Bundle/StoreFrontBundle/Helper.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\DBAL\Connection;
use Doctrine\ORM\Exception\ORMException;
use Doctrine\ORM\OptimisticLockException;
use Enlight_Components_Db_Adapter_Pdo_Mysql;
use Shopware\Bundle\StoreFrontBundle\Service\ListProductServiceInterface;
use Shopware\Bundle\StoreFrontBundle\Struct\Customer\Group as CustomerGroupStruct;
Expand All @@ -45,11 +47,15 @@
use Shopware\Models\Article\Configurator\Option;
use Shopware\Models\Article\Supplier;
use Shopware\Models\Category\Category;
use Shopware\Models\Customer\Customer;
use Shopware\Models\Customer\Discount as CustomerDiscount;
use Shopware\Models\Customer\Group;
use Shopware\Models\Customer\Group as CustomerGroup;
use Shopware\Models\Price\Discount;
use Shopware\Models\Price\Group as PriceGroup;
use Shopware\Models\Shop\Currency;
use Shopware\Models\Shop\Shop as ShopModel;
use Shopware\Models\Tax\Rule as TaxRule;
use Shopware\Models\Tax\Tax as TaxModel;
use Shopware\Tests\Functional\Bundle\StoreFrontBundle\Helper\ProgressHelper;
use Shopware_Components_Config;
Expand Down Expand Up @@ -361,6 +367,40 @@ public function createTax(array $data = []): TaxModel
return $tax;
}

/**
* @param array<string, string|int|Customer|Group|bool> $data
Copy link
Contributor

Choose a reason for hiding this comment

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

an object shape array annotation would make more sense in this case

array{foo: string, bar: float}

*
* @throws ORMException
* @throws OptimisticLockException
*/
public function createTaxRule(array $data): TaxRule
{
$taxRule = new TaxRule();
$taxRule->fromArray($data);

$this->entityManager->persist($taxRule);
$this->entityManager->flush();

return $taxRule;
}

/**
* @param array<string, string|int|Customer|Group|bool> $data
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

*
* @throws OptimisticLockException
* @throws ORMException
*/
public function createCustomerGroupDiscount(array $data): CustomerDiscount
{
$customerDiscount = new CustomerDiscount();
$customerDiscount->fromArray($data);

$this->entityManager->persist($customerDiscount);
$this->entityManager->flush();

return $customerDiscount;
}

public function createCurrency(array $data = []): Currency
{
$currency = new Currency();
Expand Down
64 changes: 64 additions & 0 deletions tests/Functional/Core/BasketTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@
use Shopware\Components\Random;
use Shopware\Models\Article\Detail;
use Shopware\Models\Customer\Customer;
use Shopware\Models\Customer\Discount as CustomerGroupDiscount;
use Shopware\Models\Customer\Group as CustomerGroup;
use Shopware\Models\Tax\Rule as TaxRule;
use Shopware\Tests\Functional\Bundle\StoreFrontBundle\Helper;
use Shopware\Tests\Functional\Helper\Utils;
use Shopware\Tests\Functional\Traits\ContainerTrait;
Expand Down Expand Up @@ -156,6 +159,44 @@ public function testsCheckBasketQuantitiesWithLowerQuantityThanAvailable(): void
static::assertFalse($result['articles'][$inStockProduct['ordernumber']]['OutOfStock']);
}

public function testInsertDiscountWithTaxFreeTaxRule(): void
{
$this->generateBasketSession();

$customerGroupRepo = $this->getContainer()->get('models')->getRepository(CustomerGroup::class);
$customerGroup = $customerGroupRepo->findOneBy(['key' => 'EK']);
static::assertInstanceOf(CustomerGroup::class, $customerGroup);

$taxRule = $this->createTaxFreeTaxRule($customerGroup);
$customerGroupDiscount = $this->createCustomerGroupDiscount($customerGroup);

$this->module->sAddArticle('SW10003');
$this->module->sInsertDiscount();

$discount = $this->connection->fetchAssociative(
'SELECT * FROM s_order_basket WHERE sessionID = :sessionId AND ordernumber = :ordernumber',
[
'sessionId' => $this->getSessionId(),
'ordernumber' => 'sw-discount',
]
);

static::assertIsArray($discount);
static::assertSame(0, (int) $discount['tax_rate']);
Copy link
Contributor

Choose a reason for hiding this comment

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

tax rates should be floats!


// Housekeeping
$this->connection->delete(
's_order_basket',
['sessionID' => $this->getSessionId()]
);

$modelManager = $this->getContainer()->get('models');
$modelManager->remove($taxRule);
$modelManager->remove($customerGroupDiscount);

$modelManager->flush();
}

public function testsCheckBasketQuantitiesWithHigherQuantityThanAvailable(): void
{
$this->generateBasketSession();
Expand Down Expand Up @@ -2677,4 +2718,27 @@ private function getSessionId(): string
{
return $this->session->get('sessionId');
}

private function createTaxFreeTaxRule(CustomerGroup $group): TaxRule
{
$resourceHelper = new Helper($this->getContainer());

return $resourceHelper->createTaxRule([
'name' => 'PHPUNIT-TAX-FREE',
'active' => true,
'customerGroup' => $group,
'groupId' => 1,
]);
}

private function createCustomerGroupDiscount(CustomerGroup $group): CustomerGroupDiscount
{
$resourceHelper = new Helper($this->getContainer());

return $resourceHelper->createCustomerGroupDiscount([
'group' => $group,
'discount' => 1,
'value' => 2,
]);
}
}
Loading