-
Notifications
You must be signed in to change notification settings - Fork 802
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
base: 5.7
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -421,7 +421,7 @@ public function sInsertDiscount() | |||||
$tax = $this->config->get('sDISCOUNTTAX'); | ||||||
} | ||||||
|
||||||
if (!$tax) { | ||||||
if (!$tax && $tax != 0) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @aragon999 , There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 It would still change the logic slightly, as when previously when the tax There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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; | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -361,6 +367,40 @@ public function createTax(array $data = []): TaxModel | |
return $tax; | ||
} | ||
|
||
/** | ||
* @param array<string, string|int|Customer|Group|bool> $data | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. an object shape array annotation would make more sense in this case
|
||
* | ||
* @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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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']); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
@@ -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, | ||
]); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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