-
Notifications
You must be signed in to change notification settings - Fork 82
IBX-9680: Extending discounts #2936
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
base: 4.6
Are you sure you want to change the base?
Conversation
1eab7ae to
c2157dc
Compare
| By extending [Discounts](discounts_guide.md), you can increase flexibility and control over how promotions are applied to suit your unique business rules. | ||
| Together with the existing [events](event_reference.md) and the [Discounts PHP API](discounts_api.md), extending discounts gives you the ability to cover additional use cases related to selling products. | ||
|
|
||
| !!! tip |
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.
Experimental, please let me know what you think - I'm ok with removing this as well
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.
I wanted to include the code in two shapes - first only for creating form (Step1) and then in its full form (Step2)
The Step1/Step2 can be confusing because it's not related to wizard steps, but to given example steps.
konradoboza
left a comment
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.
Great work! I reviewed mostly part related to extending API, I will let Paweł check extending the discount wizard. A few remarks:
code_samples/discounts/src/Discounts/ExpressionProvider/CurrentUserRegistrationDateResolver.php
Outdated
Show resolved
Hide resolved
code_samples/discounts/src/Discounts/ExpressionProvider/IsAnniversaryResolver.php
Outdated
Show resolved
Hide resolved
| public function getVariables(PriceContextInterface $priceContext): array | ||
| { | ||
| return [ | ||
| 'current_user_registration_date' => $this->userService->loadUser( |
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.
In general, whenever additional data might be needed for Discount condition/rule, it is heavily suggested to use data loaded on demand, by a function call.
Providing variable to expression engine means that it will be loaded always, regardless if the variable is actually needed for resolution or not.
I would rather suggest making this a function available within the engine. It could even use caching to ensure it's not loaded more than once.
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.
Super valuable feedback, thank you! I've focused on the "how" and missed the "why".
I've added a note about variables vs functions: 6742514 (without rewriting the example, I hope that's ok)
| $newDate = DateTimeImmutable::createFromFormat( | ||
| self::YEAR_MONTH_DAY_FORMAT, | ||
| self::REFERENCE_YEAR . '-' . $date->format(self::MONTH_DAY_FORMAT) | ||
| ); |
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.
Cool concept, but it won't work for leap years - this code breaks on 29th of february :)
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.
I don't see how, could you please elaborate?
The reference year is set to 2000, which is a leap year - on 29.02 it boils down to:
php > $date = \DateTimeImmutable::createFromFormat('Y-m-d', '2000-02-29');
php > var_dump($date);
object(DateTimeImmutable)#1 (3) {
["date"]=>
string(26) "2000-02-29 17:22:05.000000"
["timezone_type"]=>
int(3)
["timezone"]=>
string(3) "UTC"
}I don't think it should break on leap years.
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.
I've used the following code to check.
<?php
final class IsAnniversaryResolver
{
private const YEAR_MONTH_DAY_FORMAT = 'Y-m-d';
private const MONTH_DAY_FORMAT = 'm-d';
private const REFERENCE_YEAR = 2000;
public function __invoke(DateTimeInterface $date, int $tolerance = 0): bool
{
$d1 = $this->unifyYear(new DateTimeImmutable('2025-03-01 12:00:00'));
// Below does not work either
// $d1 = $this->unifyYear(new DateTimeImmutable('2025-02-29 12:00:00'));
$d2 = $this->unifyYear($date);
$diff = $d1->diff($d2, true);
// Check if the difference between dates is within the tolerance
return $diff->days <= $tolerance;
}
private function unifyYear(DateTimeInterface $date): DateTimeImmutable
{
// Create a new date using the reference year but with the same month and day
$newDate = DateTimeImmutable::createFromFormat(
self::YEAR_MONTH_DAY_FORMAT,
self::REFERENCE_YEAR . '-' . $date->format(self::MONTH_DAY_FORMAT)
);
if ($newDate === false) {
throw new RuntimeException('Failed to unify year for date.');
}
return $newDate;
}
}
$resolver = new IsAnniversaryResolver();
var_dump($resolver->__invoke(new DateTime('2024-02-29 00:00:00')));
// bool(false)
var_dump($resolver->__invoke(new DateTime('2026-02-29 00:00:00')));
// bool(true)Basically, 2024-02-29 gets anniversaries only every leap year.
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.
Right, TBH I'm not sure I'd treat 29-02 accounts in "real" implementation, would need a good spec from PM and lots of tests 😄 Probably would extend it to "anniversary week" to make the problem simpler.
I've added a note mentioning the naive approach of this implementation in e105acb , to make the reader aware of the shortcuts taken.
Co-authored-by: Paweł Niedzielski <[email protected]>
docs/discounts/discounts_api.md
Outdated
| | <nobr>`IsUserInCustomerGroup`</nobr> | Cart, Catalog| <nobr>`is_user_in_customer_group`</nobr> | Check if the customer belongs to specified [customer groups](customer_groups.md) | `customer_groups` | | ||
| | <nobr>`IsProductInQuantityInCart`</nobr> | Cart | <nobr>`is_product_in_quantity_in_cart`</nobr> | Checks if the required minimum quantity of a given product is present in the cart | `quantity` | | ||
| | <nobr>`MinimumPurchaseAmount`</nobr> | Cart | <nobr>`minimum_purchase_amount`</nobr> | Checks if purchase amount in the cart exceeds the specified minimum | `minimum_purchase_amount` | | ||
| | <nobr>`IsValidDiscountCode`</nobr> | Cart | <nobr>`is_valid_discount_code`</nobr> | Checks if the correct discount code has been provided and how many times it was used by the customer | `discount_code`, `usage_count` | |
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.
I would use <nobr> also on the last column. For example with this one, I would also try to not have the coma alone at the start of a new line:
| | <nobr>`IsValidDiscountCode`</nobr> | Cart | <nobr>`is_valid_discount_code`</nobr> | Checks if the correct discount code has been provided and how many times it was used by the customer | `discount_code`, `usage_count` | | |
| | <nobr>`IsValidDiscountCode`</nobr> | Cart | <nobr>`is_valid_discount_code`</nobr> | Checks if the correct discount code has been provided and how many times it was used by the customer | <nobr>`discount_code`,</nobr> <nobr>`usage_count`</nobr> | |
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.
Thank you 😄 Your comment made me refactor the tables a bit - I've made the identifier a part of the first column, making it much more readable: 85aa5da

| If the discount existed already and is being edited, the saved values are used to populate the form. | ||
|
|
||
| Finally, the new step is added to the wizard using the `withStep()` method, using `45` as step priority. | ||
| Each of the existing form steps has its own priority, allowing you to add your custom steps between them. |
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.
It's really good to have the table below. But, is there a way for a developer to find those priorities by oneself? To be able to also find the already created custom ones would be great.
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.
I don't see a way of doing this by inspecting the service container, I think the only solution is to:
- Subscribe to the
CreateFormDataEventandMapDiscountToFormDataEventevents - Inspect (with a debugger?) the return value of
$event->getData->getSteps()
Unless someone just reads the source code, but that's a lazy answer (always true).
Unless @Steveb-p you can think of a way of doing this?
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.
Oh, you're right, the priority is not on the listener but in its data…
php bin/console debug:event-dispatcher MapDiscountToFormDataEvent is useless here.
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.
By building a fake event, I can get the custom steps applied:
<?php
declare(strict_types=1);
namespace App\Command;
use DateTime;
use Ibexa\Bundle\Discounts\Form\Data\Conditions;
use Ibexa\Bundle\Discounts\Form\Data\DiscountValue\FixedAmountDiscountValue;
use Ibexa\Bundle\Discounts\Form\Data\EditDiscountData;
use Ibexa\Bundle\Discounts\Form\Data\GeneralProperties;
use Ibexa\Bundle\Discounts\Form\Data\ProductCondition;
use Ibexa\Bundle\Discounts\Form\Data\UserCondition;
use Ibexa\Contracts\Core\Repository\Repository;
use Ibexa\Contracts\Core\Repository\Values\Content\Language;
use Ibexa\Contracts\Discounts\Event\MapDiscountToFormDataEvent;
use Ibexa\Discounts\Value\Discount\Discount;
use Ibexa\Discounts\Value\DiscountRule\FixedAmount;
use Psr\EventDispatcher\EventDispatcherInterface;
use Symfony\Component\Console\Attribute\AsCommand;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
#[AsCommand(name: 'app:discounts_step_event_debug', description: 'Hello PhpStorm')]
class DiscountsStepEventDebugCommand extends Command
{
public function __construct(private EventDispatcherInterface $dispatcher, private Repository $repository)
{
parent::__construct();
}
protected function execute(InputInterface $input, OutputInterface $output): int
{
$user = $this->repository->getUserService()->loadUser($this->repository->getPermissionResolver()->getCurrentUserReference()->getUserId());
$now = new DateTime();
$discount = new Discount(0, 'debug', 'cart|catalog', 0, false,
$now, $now, $now, $user, $user, new FixedAmount(null), [], null, null);
$editDiscountData = new EditDiscountData($discount, new GeneralProperties(new Language()),
new UserCondition(), new ProductCondition(), new Conditions(), new FixedAmountDiscountValue());
$event = new MapDiscountToFormDataEvent($editDiscountData, $discount);
foreach ($this->dispatcher->getListeners(MapDiscountToFormDataEvent::class) as $listener) {
call_user_func($listener, $event);
}
foreach ($event->getData()->getSteps() as $step) {
$output->writeln(sprintf('%s (priority: %d)', $step->getIdentifier(), $step->getPriority()));
}
return Command::SUCCESS;
}
}% php bin/console app:discounts_step_event_debug
general_properties (priority: 50)
target_group (priority: -20)
products (priority: -30)
discount_value (priority: -50)
summary (priority: -1000)
Co-authored-by: Adrien Dupuis <[email protected]>
Things added:
Preview:
Previews: