Conversation
|
Dominikrt
force-pushed
the
fix-customergroup-discount-tax
branch
from
e92dd08 to
306dc0f
Compare
Dominikrt
force-pushed
the
fix-customergroup-discount-tax
branch
from
306dc0f to
8b09ee7
Compare
aragon999
left a comment
aragon999
left a comment
There was a problem hiding this comment.
I think I would make the check more explicit by explicitly checking that the tax is false. Maybe the if can also be moved into the if branch of the getMaxTax().
| @@ -421,7 +421,7 @@ public function sInsertDiscount() | |||
| $tax = $this->config->get('sDISCOUNTTAX'); | |||
There was a problem hiding this comment.
| $tax = $this->config->get('sDISCOUNTTAX'); | |
| $tax = (int) $this->config->get('sDISCOUNTTAX'); |
There was a problem hiding this comment.
is this the tax value? if so, it should definitely not be an integer, but a float instead
| } | ||
|
|
||
| if (!$tax) { | ||
| if (!$tax && $tax != 0) { |
There was a problem hiding this comment.
| if (!$tax && $tax != 0) { | |
| if ($tax === false) { |
There was a problem hiding this comment.
Hi @aragon999 ,
this way looks much nicer, I wasn't sure, if there is another datatype that can trigger the fallback.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| @@ -421,7 +421,7 @@ public function sInsertDiscount() | |||
| $tax = $this->config->get('sDISCOUNTTAX'); | |||
There was a problem hiding this comment.
is this the tax value? if so, it should definitely not be an integer, but a float instead
| } | ||
|
|
||
| /** | ||
| * @param array<string, string|int|Customer|Group|bool> $data |
There was a problem hiding this comment.
an object shape array annotation would make more sense in this case
array{foo: string, bar: float}
| } | ||
|
|
||
| /** | ||
| * @param array<string, string|int|Customer|Group|bool> $data |
| ); | ||
|
|
||
| static::assertIsArray($discount); | ||
| static::assertSame(0, (int) $discount['tax_rate']); |
There was a problem hiding this comment.
tax rates should be floats!
| } | ||
|
|
||
| if (!$tax) { | ||
| if (!$tax && $tax != 0) { |
There was a problem hiding this comment.
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?
3 participants
close #2608
1. Why is this change necessary?
Tax for Customer Group Discount can be 0%, currently there is a fallback for 19%
2. What does this change do, exactly?
check issue
3. Describe each step to reproduce the issue or behaviour.
check issue
4. Please link to the relevant issues (if any).
#2608
5. Which documentation changes (if any) need to be made because of this PR?
6. Checklist