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

Essential part of https://bugs.oxid-esales.com/view.php?id=7100 as PR #983

Open
wants to merge 1 commit into
base: b-7.2.x
Choose a base branch
from

Conversation

SvenBrunk
Copy link
Contributor

No description provided.

Signed-off-by: Anton Fedurtsya <anton@fedurtsya.com>
@CLAassistant
Copy link

CLAassistant commented Oct 3, 2024

CLA assistant check
All committers have signed the CLA.

@Sieg Sieg changed the base branch from b-8.0.x to b-7.2.x October 3, 2024 12:07
@Sieg
Copy link
Member

Sieg commented Oct 3, 2024

Hey @SvenBrunk, the issue with this approach is that adding this condition would prevent inactive voucher series from being selected at all. That could be fine, but there's already a validation in place elsewhere that throws an error if the voucher is no longer valid.

With this new condition, the voucher wouldn't be selected at all if the series is expired, resulting in a different error – "Invalid coupon" – instead of the current one - "Expired". I can't think of a simple solution that avoids this inconsistency for the current vouchers implementation.

I have added the test for the original (the one we are fixing) case, but it doesn't cover the validation part I have mentioned (as its different subsystem, and that part is still working correctly for the expired coupons if they are given as parameter, but they will not be anymore) and rebased for 7.2.x.

Maybe this is all fine and we should still merge this?

@SvenBrunk
Copy link
Contributor Author

@Sieg okay, thanks. Please do not merge this. We need to think about a better solution then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants